Re: [PATCH v5 3/3] cat-file: add --follow-symlinks to --batch

From: Junio C Hamano <gitster@pobox.com>
Date: May 12, 2015, 20:00 UTC
Message-ID: <xmqqlhgtftw0.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1431456922.16652.26.camel@ubuntu>

David Turner <dturner@twopensource.com> writes:

Show 21 quoted lines

> On Tue, 2015-05-12 at 11:43 -0700, Junio C Hamano wrote:
>> David Turner <dturner@twopensource.com> writes:
>> 
>> >> We need to also say something about the "missing" vs "loop" case, if
>> >> we choose to leave that part broken.  I'd rather see it fixed, but
>> >> that is not a very strong preference.
>> >
>> > Will add an example.
>> 
>> I do not think we need an example.  By "also say", I meant in
>> addition to "This and that does not currently work", we also need to
>> say that loops do not work well.  In other words, it is enough to
>> just mention that it is a current limitation (or a bug, whichever we
>> choose to call) that loops are reported as missing.
>
> The version of the patch that we are commenting on contained the text: 
>> +     --batch-check.  In the event of a symlink loop (or more than
>> +     40 symlinks in a symlink resolution chain), the file will be
>> +     treated as missing.  If a symlink points outside the tree-ish
>
> Is that sufficient?

Not really, as I think people would say "treated as missing" is a bug, and it would be better to mark it as "currently does not work" in the sense that "you cannot use this feature to tell HEAD:link that is not in HEAD and HEAD:link that is a symbolic link that points to itself apart".

> Actually, we could simply have a separate output for broken links.
> Instead of [original path] SP missing, [original path] SP loop.

Hmm, I do not quite see where this resistance against keeping the original request in order to say "You gave HEAD:link to me, but that is a symbolic link that leads to 'link'" comes from. After all, wouldn't that be more consistent with what you already show for a link that cannot be resolved, i.e. "You gave HEAD:link to me, that is a link that points at 'nosuch'" when I do this:

    $ ln -s nosuch link
    $ git add link
    $ echo "$(git write-tree):link" |
      git cat-file --batch --follow-symlinks

Ahh, that would also give us "missing", so in that sense you are being consistent.

But I do not think that consistency is useful. Showing just "missing" instead is losing information and that is what bothers me.

Showing "symlink 6 nosuch" to this "link points at a target that would be in-tree but there is no such object in the tree" symbolic link instead of "missing" would make it more useful, and I do not offhand think of a downside, but maybe I am missing something.

For a link that points outside, the code already gives

    $ ln -s ../outside outlink
    $ git add outlink
    $ echo "$(git write-tree):outlink" |
      git cat-file --batch --follow-symlinks

"symlink ../outside", so the script reading from the batch output already has to be prepared to handle "symlink" and understand it as saying "the link does not point an object that is inside the tree".