Re: [PATCH v4 17/21] cat-file: read batch stream with strbuf_getline()

[Jeff King <peff@peff.net>]

On Thu, Jan 14, 2016 at 03:58:32PM -0800, Junio C Hamano wrote:

> It is possible to prepare a text file with a DOS editor and feed it
> as a batch command stream to the command.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/cat-file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index d2ebaf1..54db118 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt)
>  	save_warning = warn_on_object_refname_ambiguity;
>  	warn_on_object_refname_ambiguity = 0;
>  
> -	while (strbuf_getline_lf(&buf, stdin) != EOF) {
> +	while (strbuf_getline(&buf, stdin) != EOF) {

This function can actually take a pretty wide variety of input. It can
take anything that get_sha1() will handle. So I think that:

  printf ':/foo\r\n' | git cat-file --batch

will behave differently (before, we looked for the literal "foo\r" in a
commit message). That's sufficiently crazy that I'm OK with giving it
up.

A more interesting case is "rev-list" output, combined with "%(rest)".
The former does not quote its filenames.

So in a repo like this:

  echo bar >$(printf 'foo\r')
  git add .
  git commit -m one

you can currently do:

  git rev-list --objects --all |
  git cat-file --batch-check='%(objectsize) %(rest)' |
  cat -A

and get:

  154 $
  32 $
  4 foo^M$

but with your patch, we lose the CR, and get a filename that is not
actually contained in the repo.

I think I'm still in favor of the change, though, for the same reason as
some of the earlier ones. We already can't handle "foo\n", so this
cannot be considered a completely robust interface. And we are much more
likely to do good than harm by stripping such a CR.

-Peff