Re: [PATCH v2 2/3] sha1_name: get_sha1_with_context learns to follow symlinks

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2015, 17:39 UTC
Message-ID: <xmqqlhgxhcqb.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqq7fsiifcz.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

> dturner@twopensource.com writes:
>
>> From: David Turner <dturner@twitter.com>
>>
>> Wire up get_sha1_with_context to call get_tree_entry_follow_symlinks
>> when GET_SHA1_FOLLOW_SYMLINKS is passed in flags. G_S_FOLLOW_SYMLINKS
>> is incompatible with G_S_ONLY_TO_DIE because the diagnosis that
>> ONLY_TO_DIE triggers does not consider symlinks.
>
> Is "does not consider" something fundamental, or it just happens to
> be that way right now?

Regardless of the answer to this question, I find the last part of this hunk puzzling.

Show 18 quoted lines

> +			if (flags & GET_SHA1_FOLLOW_SYMLINKS) {
> +				ret = get_tree_entry_follow_symlinks(tree_sha1,
> +					filename, sha1, oc->path, &oc->mode);
> +			} else {
> +				ret = get_tree_entry(tree_sha1, filename,
> +						     sha1, &oc->mode);
> +				if (ret && only_to_die) {
> +					diagnose_invalid_sha1_path(prefix,
> +								   filename,
> +								   tree_sha1,
> +								   name, len);
> +				}
> +				hashcpy(oc->tree, tree_sha1);
> +				strlcpy(oc->path, filename, sizeof(oc->path));
>  			}
> -			hashcpy(oc->tree, tree_sha1);
> -			strlcpy(oc->path, filename, sizeof(oc->path));
> -

Both variants of get_tree_entry() receive tree_sha1 and &oc->mode as places to store the discovered results and that is why we do hashcpy and strlcpy in the original codepath.

 - With your patch, the new codepath discards tree_sha1[] because it
   lost the copy back to oc->tree[]; is this change intended?  As we
   are not passing oc itself to the function, there is no way for it
   to return the object name directly to oc->tree[], no?

 - In the new codepath, oc->path[] is also not copied but I can
   sort-of guess why (you want to return something other than
   "filename" from get-tree-entry-follow-symlinks in it, or
   something).  But then the caller is losing the result of parsing
   the extended SHA-1.

You explain why "if (ret && only_to_die)" part is skipped, but these two differences are equally, if not more, important differences between the two codepaths. I do not think I saw it explained.

In any case, I would think that get_sha1_with_context() should have an external interface that is as close as the original, with enhancement (i.e. not with modification of what existing fields mean) [*1*].

That is, if oc->path[] is meant to store filename parsed from the end-user input, it should keep doing so with or without follow-symlinks. And if follow-symlinks feature needs to return extra information to the caller, it should add a new field to return that information.

And my gut feeling is that such a correction to the way how the updated get_sha1_with_context() behaves would mean you can (and need to) keep hashcpy() and strlcpy() common to both codepaths in this patch.

Thanks.

[Footnote]

*1* The reason is simple. On a user input without any symbolic link, a caller (not just the caller you are adding in patch 3/3) should be able to expect to get the identical outcome from get_sha1_with_context(), with or without GET_SHA1_FOLLOW_SYMLINKS.