Re: [RFC/PATCH] Fix path prefixing in grep_object

From: Phil Hord <phil.hord@gmail.com>
Date: Aug 26, 2013, 11:44 UTC
Message-ID: <CABURp0oGMTEgX3TKKEMAOxe6T0=uij+bAyc+5u0x_UHwEPo3CQ@mail.gmail.com>
In-Reply-To: <xmqqfvtwkjp8.fsf@gitster.dls.corp.google.com>

On Mon, Aug 26, 2013 at 3:14 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> I think Phil meant that when "git grep" is asked to search within
>>> "HEAD:some/path", filenames tacked on at the end should be appended
>>> with a '/' separator instead of the usual ':' (e.g.,
>>> "HEAD:some/path/inner/path.c", not "HEAD:some/path:inner/path.c").
>>
>> Ah, OK.
>>
>> I am not sure if we have a proper hint in the revision parser
>> machinery, but it may not be too hard to teach rev-cmdline interface
>> to keep that kind of information (i.e. "This tree object name is a
>> result of parsing '<tree-ish>:path' syntax").
>
> Actually, having thought about this a bit more, I am no longer sure
> if this is even a good idea to begin with.
>
> An output line that begins with HEAD:some/path:inner/path.c from
> "git grep" is an answer to this question: find the given pattern in
> a tree-ish specified with "HEAD:some/path".
>
> On the other hand, HEAD:some/path/inner/path.c is an answer to a
> different question: find the pattern in a tree-ish specified with
> "HEAD".  The question may optionally be further qualified with "but
> limit the search to some/path".  Both the question and its answer
> are more intuitive than the former one.

I disagree. The man page says that git grep lists the filenames of matching files. And it usually does. When told to search in a different branch, the filename is helpfully shown in a form that other git commands recognize, namely $branch:$path. This is useful for scripts that want to do something with the resulting file names.

But when a path included in the query with the branch, the output is useless to my scripts or finger memory without some cleanup first. The aim of this patch is to fix that so the cleanup is not necessary.

   $ git grep -l setup_check HEAD Documentation
   HEAD:Documentation/technical/api-gitattributes.txt

   $ git grep -l setup_check HEAD:Documentation
   HEAD:Documentation:technical/api-gitattributes.txt

The path in the first example is meaningful. The path in the second example is erroneous.

Show 9 quoted lines

> And we have a nice way to ask that question directly, i.e.
>
>     $ git grep -e pattern HEAD some/path
>
> which can be extended naturally to more than one path, e.g.
>
>     $ git grep -e pattern HEAD some/path another/hierarchy
>
> without having to repeat HEAD: part again for each path.

Yes, but that's not always what I want. Sometimes I want to search on different trees. When doing so, why should I be crippled with broken output?

    $ git grep -e pattern origin/master:some/path origin/next:another/hierarchy
    origin/master:some/path:sub/dir/foo.txt
    origin/next:another/hierarchy:path/frotz.c

I would prefer to have real paths I can pass to 'git show', ones with just one meaningful colon rather than two vague ones:

    origin/master:some/path/sub/dir/foo.txt
    origin/next:another/hierarchy/path/frotz.c

Show 10 quoted lines

> If the user asked the question of the former form, i.e.
>
>     $ git grep -e pattern HEAD:some/path
>
> there may be a reason why the user wanted to ask it in that
> (somewhat strange) way.  I am not 100% sure if it is a good idea to
> give an answer to a question different from what the user asked by
> internally rewriting the question to
>
>     $ git grep -e pattern HEAD -- some/path

We are not rewriting the question at all.

The current code assumes the user gave only an object name and is trying to help by prefixing that name on the matched path using the colon as a separator, as would be the norm. But that is the wrong separator in some cases, specifically when the tree reference includes a path.

Phil