Junio C Hamano <gitster@xxxxxxxxx> writes:
> dturner@xxxxxxxxxxxxxxxx writes:
>
>> From: David Turner <dturner@xxxxxxxxxxx>
>>
>> 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.
> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html