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

Martin Erik Werner [off-list ref] writes:

Will you add that test or should I place it in the series with you as
author?

Either is fine.  Thanks.

On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote:

quoted

Martin Erik Werner [off-list ref] writes:

quoted

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?

Given "/dir1/repo/dir2/file" I've used 'in-repo' to refer to
"dir2/file", sometimes interchangably with "part inside the work tree"
which might be better terminology, and should replace it?

Yes, "inside the working tree" is much clearer than "inside repo",
because the word "repository" often is used to mean only the stuff
inside $GIT_DIR, i.e. what controls the working tree files.

I was trying to convey that if path is simply "/dir/repo", then the while
loop method of replacing a '/' and checking from the beginning won't
work for the last level, since it has no terminating '/' to replace, so
hence it's a special case, mentioning the "part inside the work tree"
is arguably confusing in that case, since there isn't really one, maybe
it should be left out completely, since the "check each level"
explanation covers it already?

I dunno about the explanation, but it still looks strange to have
the special case to deal with "/dir/repo" before you enter the while
loop, and then also have code immediately after the loop that seems
to handle the same case.  Isn't the latter one redundant?

quoted

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.

quoted

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

Hmm, what about
	The input parameter must contain an absolute path, and it must
	already be normalized.

OK.

quoted

quoted

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

Currently, it actually doesn't, since 'normalize_path_copy_len' is
called on it prior, which can mess with the string length.

OK, strlen() here is perfectly fine, then.

quoted

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 .

Here I have only gone by the assumption that previous code did the right
thing,...

Good to know.  And the answer to the above question I think is yes,
these should be equivalent.

Thanks.

On Tue, 2014-02-04 at 10:09 -0800, Junio C Hamano wrote:

Martin Erik Werner [off-list ref] writes:

(...)

quoted

I was trying to convey that if path is simply "/dir/repo", then the while
loop method of replacing a '/' and checking from the beginning won't
work for the last level, since it has no terminating '/' to replace, so
hence it's a special case, mentioning the "part inside the work tree"
is arguably confusing in that case, since there isn't really one, maybe
it should be left out completely, since the "check each level"
explanation covers it already?

I dunno about the explanation, but it still looks strange to have
the special case to deal with "/dir/repo" before you enter the while
loop, and then also have code immediately after the loop that seems
to handle the same case.  Isn't the latter one redundant?

The check before the loop doesn't use 'real_path', the one after does:
"/dir/repo" vs "/dir/repolink"

-- 
Martin Erik Werner [off-list ref]