Re: [PATCH] grep: follow conventions for printing paths w/ unusual chars

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Andreas Heiduk" <asheiduk@gmail.com>, git <git@vger.kernel.org>,
	"Greg Hurrell" <greg@hurrell.net>, "René Scharfe" <l.s.r@web.de>
Subject: Re: [PATCH] grep: follow conventions for printing paths w/ unusual chars
Date: Fri, 17 Apr 2020 18:19:07 -0300	[thread overview]
Message-ID: <CAHd-oW7B1wtHOE7KCtOR-kAXhuj8Du_MAYobWd9nSsSvm_sChQ@mail.gmail.com> (raw)
In-Reply-To: <xmqq8siuwqxp.fsf@gitster.c.googlers.com>

On Fri, Apr 17, 2020 at 3:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > +     if (opt->relative && opt->prefix_length)
> > +             quote_path_relative(filename + tree_name_len, opt->prefix, out);
> > +     else
> > +             quote_c_style(filename + tree_name_len, out, NULL, 0);
>
> Yup.  This solves the discrepancy reported correctly (i.e. both
> sides should do the quoting, the original only quoted when relative,
> and the new code corrects the other side).
>
> > +     if (tree_name_len)
> > +             strbuf_insert(out, 0, filename, tree_name_len);
>
> I am not quite sure about this part, though.
>
> Earlier we inserted the latter part of filename (after offset
> tree_name_len) to strbuf "out" after quoting, and then we are
> prefixing the earlier part of the filename without quoting to that
> same "out".  Wouldn't a path ABCDEF (I do not literally mean that
> these ascii alphabets need quoting---just imagine each of these
> stands for a different letter and some causes the path to be quoted)
> with tree_name_len pointing somewhere in the middle be added as (an
> analog of) ABC"DEF", i.e. literal prefix with remainder quoted?

Right. But the ABC prefix here is always a tree name, so the output is
one of the following:

<sha1>:"unusual path":line
<ref name>:"unusual path":line
"unusual path":line

(Always having the entire "unusual path" inside the double quotes, though)

> I would (perhaps näively) expect that the whole thing would be placed
> inside a dq pair in such a case, even if the prefix part alone would
> not require quoting.

Hm, it might be just me, but I think that including the tree name and
path in the same dq pair could be a bit confusing, as they can seem
like one unique thing.

There is also the case of ref names containing unusual chars, as dq's.
Such ref names are not currently escaped in the output of other
commands as checkout, branch or describe. But if we include the ref
name in grep's output quote, it would be escaped, which would be a
minor inconsistency (I don't know if that's relevant, though).