Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2014, 21:00 UTC
Message-ID: <xmqq1tzjewsf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1391358940-17373-5-git-send-email-martinerikwerner@gmail.com>

Martin Erik Werner <martinerikwerner@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.

Show 9 quoted lines

> + * 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.

Show 11 quoted lines

> +{
> +	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[]):

Show 6 quoted lines

> +		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 .

Show 34 quoted lines

> +			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