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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Apr 17, 2020, 21:19 UTC
Message-ID: <CAHd-oW7B1wtHOE7KCtOR-kAXhuj8Du_MAYobWd9nSsSvm_sChQ@mail.gmail.com>
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:

Show 25 quoted lines

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