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

[Junio C Hamano <gitster@pobox.com>]

dturner@twopensource.com writes:

> +--follow-symlinks::
> +	Follow symlinks inside the repository when requesting objects
> +	with extended SHA-1 expressions of the form tree-ish:path-in-tree.
> +	Instead of providing output about the link itself, provide output
> +	about the linked-to object.  This option requires --batch or
> +	--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
> +	(e.g. a link to /foo or a root-level link to ../foo), the
> +	portion of the link which is outside the tree will be printed.
> +	Follow-symlinks will be silently turned off if <object>
> +	specifies an object in the index rather than one in the object
> +	database.
> +
> +	For example, consider the a git repository containing:
> +	f: a file containing "hello\n"
> +	link: a symlink to f
> +	dir/link: a symlink to ../f
> +	plink: a symlink to ../f
> +	alink: a symlink to /etc/passwd
> +
> +	echo HEAD:f |git cat-file --batch --follow-symlinks would print
> +	ce013625030ba8dba906f756967f9e9ca394464a blob 6
> +
> +	echo HEAD:link |git cat-file --batch --follow-symlinks would print
> +	the same thing, as would HEAD:dir/link.
> +	Without follow-symlinks, these would print data about the
> +	symlink itself.  In the case of HEAD:link, you would see
> +	4d1ae35ba2c8ec712fa2a379db44ad639ca277bd blob 1
> +
> +	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.

   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.

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

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

I do not necessarily think the latter three need to be addressed in
this 3-patch series, but they should be listed as known bugs, I
would think.  That would invite others to fix them and save time for
users to file unnecessary bug reports.

So (now I came back) the last part of the description may want to
become more like this:

	...
	portion of the link which is outside the tree will be printed.

	This option (currently) cannot be used unless `--batch` or
	`--batch-check` is used.

        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.

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.

Thanks.


diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 9bdfced..3226f3e 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -82,31 +82,43 @@ OPTIONS
 	Follow-symlinks will be silently turned off if <object>
 	specifies an object in the index rather than one in the object
 	database.
-
-	For example, consider the a git repository containing:
++
+For example, consider a git repository containing:
++
+--
 	f: a file containing "hello\n"
 	link: a symlink to f
 	dir/link: a symlink to ../f
 	plink: a symlink to ../f
 	alink: a symlink to /etc/passwd
-
-	echo HEAD:f |git cat-file --batch --follow-symlinks would print
+--
++
+For a regular file `f`, `echo HEAD:f | git cat-file --batch` would print
++
+--
 	ce013625030ba8dba906f756967f9e9ca394464a blob 6
-
-	echo HEAD:link |git cat-file --batch --follow-symlinks would print
-	the same thing, as would HEAD:dir/link.
-	Without follow-symlinks, these would print data about the
-	symlink itself.  In the case of HEAD:link, you would see
+--
++
+And `echo HEAD:link | git cat-file --batch --follow-symlinks` would print
+the same thing, as would `HEAD:dir/link`, as they both point at `HEAD:f`.
++
+Without `--follow-symlinks`, these would print data about the
+symlink itself.  In the case of `HEAD:link`, you would see
++
+--
 	4d1ae35ba2c8ec712fa2a379db44ad639ca277bd blob 1
-
-	Both plink and alink point outside the tree, so they would
-	respectively print:
+--
++
+Both `plink` and `alink` point outside the tree, so they would
+respectively print:
++
+--
 	symlink 4
 	../f
 
 	symlink 11
 	/etc/passwd
-
+--
 
 
 OUTPUT
-- 
2.4.0-363-gef77c54