Martin Erik Werner <martinerikwer...@gmail.com> writes:
> The path being exactly equal to the work tree is handled separately,
> since then there is no directory separator between the work tree and
> in-repo part.
What is an "in-repo part"? Whatever it is, I am not sure if I
follow that logic. After the while (*path) loop checks each level,
you check the whole path---wouldn't that code handle that special
case already?
Because it is probably the normal case not to have any symbolic link
in the leading part (hence not having to dereference them), I think
checking "is work_tree[] a prefix of path[]" early is justified from
performance point of view, though.
> /*
> + * No checking if the path is in fact an absolute path is done, and it must
> + * already be normalized.
This is not quite parsable to me.
> + * Find the part of an absolute path that lies inside the work tree by
> + * dereferencing symlinks outside the work tree, for example:
> + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file
> + * /dir/file (work tree is /) -> dir/file
> + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2
> + * /dir/repolink/file (repolink points to /dir/repo) -> file
> + * /dir/repo (exactly equal to work tree) -> (empty string)
> + */
> +static inline int abspath_part_inside_repo(char *path)
It looks a bit too big to be marked "inline"; better leave it to the
compiler to notice that there is only a single callsite and decide
to (or not to) inline it.
> +{
> + size_t len;
> + size_t wtlen;
> + char *path0;
> + int off;
> +
> + const char* work_tree = get_git_work_tree();
> + if (!work_tree)
> + return -1;
> + wtlen = strlen(work_tree);
> + len = strlen(path);
I expect that this is called from a callsite that _knows_ how long
path[] is. Shouldn't len be a parameter to this function instead?
> + off = 0;
> +
> + /* check if work tree is already the prefix */
> + if (strncmp(path, work_tree, wtlen) == 0) {
I wonder if we want to be explicit and compare wtlen and len before
even attempting strncmp(). If work_tree is longer than path, it
cannot be a prefix of path, right?
In other words, also with a style-fix to prefer !XXcmp() over
XXcmp() == 0:
if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
perhaps? That would make it clear why referring to path[wtlen] on
the next line is permissible (it is obvious that it won't access
past the end of path[]):
> + if (path[wtlen] == '/') {
> + memmove(path, path + wtlen + 1, len - wtlen);
> + return 0;
> + } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
> + /* work tree is the root, or the whole path */
> + memmove(path, path + wtlen, len - wtlen + 1);
If work_tree[] == "/", path[] == "/a", then len == 2, wtlen == 1,
path[wtlen - 1] == '/' and this shifts path[] to the left by one,
leaving path[] = "a", which is what we want. OK.
If work_tree[] == path[] == "/a", then len == wtlen == 2,
path[wtlen] == '\0', and this becomes equivalent to path[0] = '\0'.
Hmph.... How do our callers treat an empty path? In other words,
should these three be equivalent?
cd /a && git ls-files /a
cd /a && git ls-files ""
cd /a && git ls-files .
> + return 0;
> + }
> + /* work tree might match beginning of a symlink to work tree */
> + off = wtlen;
> + }
> + path0 = path;
> + path += offset_1st_component(path) + off;
> +
> + /* check each level */
> + while (*path) {
> + path++;
> + if (*path == '/') {
> + *path = '\0';
> + if (strcmp(real_path(path0), work_tree) == 0) {
> + memmove(path0, path + 1, len - (path - path0));
> + return 0;
> + }
> + *path = '/';
> + }
> + }
> +
> + /* check whole path */
> + if (strcmp(real_path(path0), work_tree) == 0) {
> + *path0 = '\0';
> + return 0;
> + }
> +
> + return -1;
> +}
> +
> +/*
> * Normalize "path", prepending the "prefix" for relative paths. If
> * remaining_prefix is not NULL, return the actual prefix still
> * remains in the path. For example, prefix = sub1/sub2/ and path is
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
