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

From: David Turner <dturner@twopensource.com>
Date: May 12, 2015, 18:36 UTC
Message-ID: <1431455779.16652.20.camel@ubuntu>
In-Reply-To: <xmqqoalpzn3s.fsf@gitster.dls.corp.google.com>

On Tue, 2015-05-12 at 11:07 -0700, Junio C Hamano wrote:

Show 22 quoted lines

> > +	Both plink and alink point outside the tree, so they would
> > +	respectively print:
> > +	symlink 4
> > +	../f
> > +
> > +	symlink 11
> > +	/etc/passwd
> > +
> > +
> > +
> 
> A few points I noticed:
> 
>  * It is not clear that this is (currently) only for --batch and
>    --batch-check until you read four lines into the description.
> 
>    Perhaps start the description like this instead?
> 
>    --follow-symlinks::
>            When answering `--batch` or `--batch-check` request,
>            follow symlinks inside the repository when requesting objects
>            with extended SHA-1 expressions of the form tree-ish:path-in-tree.

Will rearrange.

Show 7 quoted lines

>    Also I'd lose the "This option requires ..." sentence in the middle
>    (I'll come back to the reason why later).
> 
>  * Is it fundamental that this is only for --batch family, or is it
>    just lack of need by the current implementor and implementation?
>    "git cat-file --follow-symlinks blob :RelNotes" does not sound
>    a nonsense request to me.

The reason that --follow-symlinks doesn't work for non-batch requests is that it is impossible to distinguish out-of-tree symlinks from valid output in non-batch output. I will add text explaining this.

>  * I am not sure if HEAD:link that points at HEAD:link should be
>    reported as "missing".  It may be better to report the original
>    without any dereferencing just like a link that points at outside
>    the tree? i.e. "symlink 4 LF link".

Unfortunately, a symlink loop might include relative symlinks (e.g. ../a). If we return a relative symlink, the user will not be able to distinguish it from a non-loop, out-of-tree symlink. So I think we may not return symlink 4 LF ../a for these cases.

We could, I guess, have a separate output like loop <size> LF link <LF>", but, unless we always save and output the first link in the chain, we won't know what any link is relative to. Since reasonable people do not create symlink loops, and since there are other mechanisms for symlink loop debugging (e.g. plain cat-file), I think it is OK not to put special effort into handling loops.

Show 7 quoted lines

>  * I think "echo :RelNotes | git cat-file --batch --follow-symlinks"
>    that does not follow a symlink is a BUG.  Unless there is
>    something fundamental that in-index object should never support
>    this feature, that is.  But I do not think of a good reason
>    why---it feels that this is just the lack of implementation that
>    can be addressed by somebody else in the future who finds the
>    need for the support.

Yes, this should definitely be addressed in the future. I didn't see a straightforward way to generalize this code to also address the index, so a new version of this function would have to be written. That's why I didn't add that feature yet. The lack of it is definitely a bug, though.

Show 7 quoted lines

>         Also the option does not (currently) work correctly when an
> 	object in the index is specified (e.g. `:link` instead of
> 	`HEAD:link`) rather than one in the tree.
> 
> 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.

> By the way, the text after your patch would not format well thru
> AsciiDoc.  See attached for a suggested mark-up fix that can be
> squashed.

I'll squash that in when I re-roll.  Thanks for the formatting.