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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2014, 18:09 UTC
Message-ID: <xmqqlhxqagwt.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140203231659.GC15607@mule>

Martin Erik Werner <martinerikwerner@gmail.com> writes:

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

Either is fine.  Thanks.

Show 15 quoted lines

> On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote:
>> 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?
>
> 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.

Show 7 quoted lines

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

Show 13 quoted lines

>> 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.
> Hmm, what about
> 	The input parameter must contain an absolute path, and it must
> 	already be normalized.

OK.

Show 11 quoted lines

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

Show 9 quoted lines

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