threads / rfc / 34762

RFC patchFix path prefixing in grep_object

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

## tl;dr

19 messages between Aug 25, 2013 and Aug 27, 2013. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Phil Hord· Aug 25, 2013, 01:35 UTC · lore

When the pathspec given to grep includes a tree name, the full name of matched files is assembled using colon as a separator. If the pathspec includes a tree name, it should use a slash instead.

Check if the pathspec already names a tree and ref (including a colon) and use a slash if so. ---

I'm not sure about the detection I used here. It works, but it is not terribly robust. Is there a better way to handle this? Maybe something like 'prefix_pathspec(name,"");'.

 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to builtin/grep.c +2 −1
diff --git a/builtin/grep.c b/builtin/grep.c
index 03bc442..d0deae4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -480,8 +480,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 		len = name ? strlen(name) : 0;
 		strbuf_init(&base, PATH_MAX + len + 1);
 		if (len) {
+			int has_colon = !!strchr(name,':');
 			strbuf_add(&base, name, len);
-			strbuf_addch(&base, ':');
+			strbuf_addch(&base, has_colon?'/':':');
 		}
 		init_tree_desc(&tree, data, size);
 		hit = grep_tree(opt, pathspec, &tree, &base, base.len,

-- 
1.8.4.557.g34b3a2e
Phil Hord· Aug 25, 2013, 02:07 UTC · re: Phil Hord · lore

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

On Sat, Aug 24, 2013 at 9:35 PM, Phil Hord <hordp@cisco.com> wrote:
Show 8 quoted lines
>
> When the pathspec given to grep includes a tree name, the full
> name of matched files is assembled using colon as a separator.
> If the pathspec includes a tree name, it should use a slash
> instead.
>
> Check if the pathspec already names a tree and ref (including
> a colon) and use a slash if so.

I think I used lots of wrong terminology there. What do I call these things?

HEAD:path is a tree.
HEAD is a commit name.
Maybe like this?
  When a tree is given to grep, the full name of matched files
  is assembled using colon as a separator.
  If the tree name includes an object name, as in
  HEAD:some/path, it should use a slash instead.
Phil
Junio C Hamano· Aug 25, 2013, 03:35 UTC · re: Phil Hord · lore

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

Phil Hord <phil.hord@gmail.com> writes:
Show 14 quoted lines
> On Sat, Aug 24, 2013 at 9:35 PM, Phil Hord <hordp@cisco.com> wrote:
>>
>> When the pathspec given to grep includes a tree name, the full
>> name of matched files is assembled using colon as a separator.
>> If the pathspec includes a tree name, it should use a slash
>> instead.
>>
>> Check if the pathspec already names a tree and ref (including
>> a colon) and use a slash if so.
>
> I think I used lots of wrong terminology there.  What do I call these
> things?
>
> HEAD:path is a tree.

It is one way to name a tree object, yes (another obvious way is to spell it out, e.g. 3610ac62).

> HEAD is a commit name.

It is one way to name the commit object, yes, but I am guessing that you are interested in these in the context of resolving HEAD:path. If that is the case, then "commit"-ness is not something you are interested in---any tree-ish would do (e.g. v1.8.4, maint^{tree}).

Show 7 quoted lines
> Maybe like this?
>
>   When a tree is given to grep, the full name of matched files
>   is assembled using colon as a separator.
>
>   If the tree name includes an object name, as in
>   HEAD:some/path, it should use a slash instead.

What problem are you trying to solve? It should use HEAD:some/path, not HEAD/some/path.

Jonathan Nieder· Aug 25, 2013, 04:23 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>> On Sat, Aug 24, 2013 at 9:35 PM, Phil Hord <hordp@cisco.com> wrote:
>>> When the pathspec given to grep includes a tree name, the full
>>> name of matched files is assembled using colon as a separator.
>>> If the pathspec includes a tree name, it should use a slash
>>> instead.
[...]
Show 5 quoted lines
>>   If the tree name includes an object name, as in
>>   HEAD:some/path, it should use a slash instead.
>
> What problem are you trying to solve?  It should use HEAD:some/path,
> not HEAD/some/path.

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"). Otherwise I cannot copy and paste "git grep" output and get something suitable for passing to "git show".

I don't think we have a standard name for the tree:path syntax. I've always just called it tree:path syntax. :)

Hope that helps, Jonathan

Junio C Hamano· Aug 25, 2013, 05:25 UTC · re: Jonathan Nieder · lore

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

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

Junio C Hamano· Aug 26, 2013, 07:14 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> 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.

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.
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
So...
Phil Hord· Aug 26, 2013, 11:44 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Aug 26, 2013, 16:23 UTC · re: Phil Hord · lore

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

Phil Hord <phil.hord@gmail.com> writes:
Show 32 quoted lines
> On Mon, Aug 26, 2013 at 3:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.

But you need to realize that the manual page gives a white lie in order to stay short enough to be readable. It does give filenames when reading from the working tree, the index or a top-level tree object. When given arbitrary tree object that is not top-level, it does give filenames relative to the given tree object, which is the answer HEAD:some/path:inner/path.c to the question "where in the tree HEAD:some/path do we have hits?".

Show 12 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.

That statement is trapped in a narrow "implementor" mind. I know you are not rewriting the question in your implementation, but what do the end users see? It gives an answer to a question different from they asked; the observed behaviour is as if the question was rewritten before the machinery went to work.

If your justification were "above says 'there may be a readon why the user wanted to ask it in that way', i.e. 'find in this tree object HEAD:some/path and report where hits appear', but the reason can only be from laziness and/or broken script and the user always wants the answer from within the top-level tree-ish", then that argument may make some sense. You need to justify why it is OK to lose information in the answer by rewriting the colon that separates the question ("in this tree object") and the answer ("at this path relative to the tree object given").

Whether you rewrite the input or the output is not important; you are trying to give an answer to a different question, which is what I find questionable.

Phil Hord· Aug 26, 2013, 16:49 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 26, 2013 at 12:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 75 quoted lines
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Mon, Aug 26, 2013 at 3:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 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.
>
> But you need to realize that the manual page gives a white lie in
> order to stay short enough to be readable.  It does give filenames
> when reading from the working tree, the index or a top-level tree
> object.  When given arbitrary tree object that is not top-level, it
> does give filenames relative to the given tree object, which is the
> answer HEAD:some/path:inner/path.c to the question "where in the
> tree HEAD:some/path do we have hits?".
>
>>> 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.
>
> That statement is trapped in a narrow "implementor" mind. I know you
> are not rewriting the question in your implementation, but what do
> the end users see? It gives an answer to a question different from
> they asked; the observed behaviour is as if the question was
> rewritten before the machinery went to work.
>
> If your justification were "above says 'there may be a readon why
> the user wanted to ask it in that way', i.e. 'find in this tree
> object HEAD:some/path and report where hits appear', but the reason
> can only be from laziness and/or broken script and the user always
> wants the answer from within the top-level tree-ish", then that
> argument may make some sense. You need to justify why it is OK to
> lose information in the answer by rewriting the colon that separates
> the question ("in this tree object") and the answer ("at this path
> relative to the tree object given").
>
> Whether you rewrite the input or the output is not important; you
> are trying to give an answer to a different question, which is what
> I find questionable.
Ok, so if I can summarize what I am inferring from your objection:
 1. The (tree-path, found-path) pair is useful information to get back
from git-grep.
 2. A colon is used to delimit these pieces of information, just as a
colon is used to delimit the filename from the matched-line results.
 3. The fact that the colon is also the separator used in object refs
is mere coincidence; the colon was _not_ chosen because it
conveniently turns the results list into valid object references.  A
comma could have been instead, or even a \t.
Have I got that right?

If so, then I would like to point out to you the convenience I accidentally encountered using this tool. Perhaps you didn't realize how helpful it was when you chose to use a colon there.

On the other hand, perhaps a colon is an unfortunate syntactic collision which should be corrected in the future.

Phil
Phil Hord· Aug 26, 2013, 17:07 UTC · re: Phil Hord · lore

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

On Mon, Aug 26, 2013 at 12:49 PM, Phil Hord <phil.hord@gmail.com> wrote:
> If so, then I would like to point out to you the convenience I
> accidentally encountered using this tool.  Perhaps you didn't realize
> how helpful it was when you chose to use a colon there.

My itch comes from a case where I am looking for references in some other branch and I want to narrow my search.

  $ git grep object origin/master
  origin/master:.gitignore:/git-count-objects
  origin/master:.gitignore:/git-fsck-objects
  origin/master:.gitignore:/git-hash-object
  <8600 lines more deleted>

I find hits in the region that interests me and then I try to zoom in there. Conveniently, the path I want to search is right there on my screen. So I copy the part of the object reference I want to limit my search to, and I try again:

   $ git grep object origin/master:builtin/

Now, I find the file that has the code I wanted. But I want to do something fancier to it than grep, so this time I copy-and-paste the filename into my command line after typing 'git show':

   $ git show origin/master:builtin/pack-objects.c | vim -
But this doesn't work if the delimiter used was a colon.
   $ git show origin/master:builtin:pack-objects.c | vim -

I have to edit my copy-and-pasted text, which means I have think about it a bit, and it interrupts all the other things I was thinking about already. Eventually I might learn to do this edit automatically, except it is not needed most of the time. It is only needed when I provide a tree-path instead of a "branch <space> path". In the latter case, the full path is spelled out for me correctly. And in all other cases the filenames provided by git-grep are valid filenames or object names.

Junio C Hamano· Aug 26, 2013, 17:26 UTC · re: Phil Hord · lore

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

Phil Hord <phil.hord@gmail.com> writes:
Show 18 quoted lines
>> If your justification were "above says 'there may be a readon why
>> the user wanted to ask it in that way', i.e. 'find in this tree
>> object HEAD:some/path and report where hits appear', but the reason
>> can only be from laziness and/or broken script and the user always
>> wants the answer from within the top-level tree-ish", then that
>> argument may make some sense. You need to justify why it is OK to
>> lose information in the answer by rewriting the colon that separates
>> the question ("in this tree object") and the answer ("at this path
>> relative to the tree object given").
>>
>> Whether you rewrite the input or the output is not important; you
>> are trying to give an answer to a different question, which is what
>> I find questionable.
>
> Ok, so if I can summarize what I am inferring from your objection:
>
>  1. The (tree-path, found-path) pair is useful information to get back
> from git-grep.

At least that was the intent. I can be persuaded that your change will not break anybody if you successfully argue that it is not a useful information, though.

Show 7 quoted lines
>  2. A colon is used to delimit these pieces of information, just as a
> colon is used to delimit the filename from the matched-line results.
>
>  3. The fact that the colon is also the separator used in object refs
> is mere coincidence; the colon was _not_ chosen because it
> conveniently turns the results list into valid object references.  A
> comma could have been instead, or even a \t.

Not necessarily. If the user is asking the question in a more natural way (I want to see where in 'next' branch's tip commit hits appear, by the way, I know I am only interested in builtin/ so I'd give pathspec as well when I am asking this question), the output does give <commit> <colon> <path>, so it is more than coincidence.

I do not think it is worth doing only for this particular use case, but it might be a good change to allow A:B:C to be parsed as a proper extended SHA-1 expression and make it yield

	git rev-parse $(git rev-parse $(git rev-parse A):B):C

Right now, "B:C" is used as a path inside tree-ish "A", but I think we can safely fall back, when path B:C does not appear in tree-ish A, to see if path B appears in it and is a tree, and then turn it into a look-up of path C in that tree A:B.

Phil Hord· Aug 26, 2013, 17:45 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 26, 2013 at 1:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Phil Hord <phil.hord@gmail.com> writes:
>
>>> If your justification were "above says 'there may be a readon why
>>> the user wanted to ask it in that way', i.e. 'find in this tree
>>> object HEAD:some/path and report where hits appear', but the reason
>>> can only be from laziness and/or broken script and the user always
>>> wants the answer from within the top-level tree-ish", then that
>>> argument may make some sense. You need to justify why it is OK to
>>> lose information in the answer by rewriting the colon that separates
>>> the question ("in this tree object") and the answer ("at this path
>>> relative to the tree object given").
>>>
>>> Whether you rewrite the input or the output is not important; you
>>> are trying to give an answer to a different question, which is what
>>> I find questionable.
>>
>> Ok, so if I can summarize what I am inferring from your objection:
>>
>>  1. The (tree-path, found-path) pair is useful information to get back
>> from git-grep.
>
> At least that was the intent. I can be persuaded that your change
> will not break anybody if you successfully argue that it is not a
> useful information, though.

Anyone who is interested in the matched trees from the original command-line only needs to compare the matched paths' prefixes with the original args to see which one is responsible for each hit. But this is not convincing to me because they may not know the original args to the grep command.

I do not know if this a good reason to keep supporting this mode, though. I can just as easily see some script being confused by four colons in

    origin/master:some/:path/file.c:1:int main()

instead of only three that he is used to because someone passed in a longer tree-path than expected.

I don't know if I can make that argument.

I think I _can_ argue that the colon separator here is historically just a part of the filename because it is not affected by "--null".

Show 24 quoted lines
>>  2. A colon is used to delimit these pieces of information, just as a
>> colon is used to delimit the filename from the matched-line results.
>>
>>  3. The fact that the colon is also the separator used in object refs
>> is mere coincidence; the colon was _not_ chosen because it
>> conveniently turns the results list into valid object references.  A
>> comma could have been instead, or even a \t.
>
> Not necessarily.  If the user is asking the question in a more
> natural way (I want to see where in 'next' branch's tip commit hits
> appear, by the way, I know I am only interested in builtin/ so I'd
> give pathspec as well when I am asking this question), the output
> does give <commit> <colon> <path>, so it is more than coincidence.
>
> I do not think it is worth doing only for this particular use case,
> but it might be a good change to allow A:B:C to be parsed as a
> proper extended SHA-1 expression and make it yield
>
>         git rev-parse $(git rev-parse $(git rev-parse A):B):C
>
> Right now, "B:C" is used as a path inside tree-ish "A", but I think
> we can safely fall back, when path B:C does not appear in tree-ish
> A, to see if path B appears in it and is a tree, and then turn it
> into a look-up of path C in that tree A:B.

That would also solve this problem, usually. But I don't like it nearly as much as my patch, and I agree it seems extreme for this one corner-case.

Phil
Junio C Hamano· Aug 27, 2013, 04:07 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> Not necessarily.  If the user is asking the question in a more
> natural way (I want to see where in 'next' branch's tip commit hits
> appear, by the way, I know I am only interested in builtin/ so I'd
> give pathspec as well when I am asking this question), the output
> does give <commit> <colon> <path>, so it is more than coincidence.

This part needs to be qualified. "Natural" is of course in the eyes of beholder. If we assume that your #1 holds true (i.e. the tuple <in which tree object are we reporting, what path we saw hits> is important and merging them into one <in what blob we saw hits> lose information), then it is natural to ask "inside origin/master tree, where do I have hits? By the way, I am only interested in builtin/" and get "origin/master:builtin/pack-objects.c" as an answer (this is from your earlier example), than asking "inside origin/master:builtin tree, where do I have hits?"

If we do not consider #1 is false and the tree information can be discarded, then it does not matter if we converted the colon after origin/master to slash when we answer the latter question, and the latter question stops being unnatural.

Show 10 quoted lines
> ...
> but it might be a good change to allow A:B:C to be parsed as a
> proper extended SHA-1 expression and make it yield
>
> 	git rev-parse $(git rev-parse $(git rev-parse A):B):C
>
> Right now, "B:C" is used as a path inside tree-ish "A", but I think
> we can safely fall back, when path B:C does not appear in tree-ish
> A, to see if path B appears in it and is a tree, and then turn it
> into a look-up of path C in that tree A:B.

And if we want to keep the <tree, path> tuple, but still want to make it easier to work with the output, allowing A:B:C to be parsed as an extended SHA-1 expression would be a reasonable solution, not a work-around. The answer is given to the question asked in either way (either "in origin/master, but limited to these pathspecs" or "in the tree origin/master:builtin/") without losing information, but the issue you had is that the answer to the latter form of question is not understood by the object name parser, which I personally think is a bug.

Phil Hord· Aug 27, 2013, 11:54 UTC · re: Junio C Hamano · lore

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

On Tue, Aug 27, 2013 at 12:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Not necessarily.  If the user is asking the question in a more
>> natural way (I want to see where in 'next' branch's tip commit hits
>> appear, by the way, I know I am only interested in builtin/ so I'd
>> give pathspec as well when I am asking this question), the output
>> does give <commit> <colon> <path>, so it is more than coincidence.
>
> This part needs to be qualified.  "Natural" is of course in the eyes
> of beholder.  If we assume that your #1 holds true (i.e. the tuple
> <in which tree object are we reporting, what path we saw hits> is
> important and merging them into one <in what blob we saw hits> lose
> information),

"My #1" is really "what I inferred from your statements." I did not think the tuple was important, but I agree that may be my shortsightedness. If the tuple is important, then it seems to me that the --null behavior is a bug (the colon is left intact); but changing it now seems senseless or harmful.

Show 31 quoted lines
> then it is natural to ask "inside origin/master tree,
> where do I have hits?  By the way, I am only interested in builtin/"
> and get "origin/master:builtin/pack-objects.c" as an answer (this is
> from your earlier example), than asking "inside origin/master:builtin
> tree, where do I have hits?"
>
> If we do not consider #1 is false and the tree information can be
> discarded, then it does not matter if we converted the colon after
> origin/master to slash when we answer the latter question, and the
> latter question stops being unnatural.
>
>> ...
>> but it might be a good change to allow A:B:C to be parsed as a
>> proper extended SHA-1 expression and make it yield
>>
>>       git rev-parse $(git rev-parse $(git rev-parse A):B):C
>>
>> Right now, "B:C" is used as a path inside tree-ish "A", but I think
>> we can safely fall back, when path B:C does not appear in tree-ish
>> A, to see if path B appears in it and is a tree, and then turn it
>> into a look-up of path C in that tree A:B.
>
> And if we want to keep the <tree, path> tuple, but still want to
> make it easier to work with the output, allowing A:B:C to be parsed
> as an extended SHA-1 expression would be a reasonable solution, not
> a work-around. The answer is given to the question asked in either
> way (either "in origin/master, but limited to these pathspecs" or
> "in the tree origin/master:builtin/") without losing information,
> but the issue you had is that the answer to the latter form of
> question is not understood by the object name parser, which I
> personally think is a bug.

I am beginning to agree, though we discovered some other weird output from grep which also does not parse. (origin/master:../relative/path).

It seems that fixing this bug could be very confusing for get_sha1_with_context unless the context was rewritten to match the traditional syntax.

P
Junio C Hamano· Aug 26, 2013, 17:03 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> If your justification were "above says 'there may be a readon why
> the user wanted to ask it in that way', i.e. 'find in this tree
> object HEAD:some/path and report where hits appear', but the reason
> can only be from laziness and/or broken script and the user always
> wants the answer from within the top-level tree-ish", then that
> argument may make some sense. You need to justify why it is OK to
> lose information in the answer by rewriting the colon that separates
> the question ("in this tree object") and the answer ("at this path
> relative to the tree object given").
>
> Whether you rewrite the input or the output is not important; you
> are trying to give an answer to a different question, which is what
> I find questionable.

For example, one of the cases the proposed change will break that I am worried about is a script that wants to take N trees and a pattern, and report where in the given trees hits appear, something like:

git grep -c -e $pattern "$@" |
perl -e '
	my @trees = @ARGV;
	my %found = ();
	while (<>) {
	        my $line = $_;
	        for (@trees) {
			my $tree_prefix = $_ . ":";
			my $len = len($tree_prefix);
			if (substr($line, 0, $len) eq $tree_prefix) {
				my ($path_count) = substr($line, $len);
				my ($path, $count) = $path_count =~ /^(.*):(\d+)$/
				$found{$tree} = [$path, $count];
			}
		}
	}
	# Do stats on %found
' "$@"
Phil Hord· Aug 26, 2013, 17:19 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 26, 2013 at 1:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 40 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If your justification were "above says 'there may be a readon why
>> the user wanted to ask it in that way', i.e. 'find in this tree
>> object HEAD:some/path and report where hits appear', but the reason
>> can only be from laziness and/or broken script and the user always
>> wants the answer from within the top-level tree-ish", then that
>> argument may make some sense. You need to justify why it is OK to
>> lose information in the answer by rewriting the colon that separates
>> the question ("in this tree object") and the answer ("at this path
>> relative to the tree object given").
>>
>> Whether you rewrite the input or the output is not important; you
>> are trying to give an answer to a different question, which is what
>> I find questionable.
>
> For example, one of the cases the proposed change will break that I
> am worried about is a script that wants to take N trees and a
> pattern, and report where in the given trees hits appear, something
> like:
>
> git grep -c -e $pattern "$@" |
> perl -e '
>         my @trees = @ARGV;
>         my %found = ();
>         while (<>) {
>                 my $line = $_;
>                 for (@trees) {
>                         my $tree_prefix = $_ . ":";
>                         my $len = len($tree_prefix);
>                         if (substr($line, 0, $len) eq $tree_prefix) {
>                                 my ($path_count) = substr($line, $len);
>                                 my ($path, $count) = $path_count =~ /^(.*):(\d+)$/
>                                 $found{$tree} = [$path, $count];
>                         }
>                 }
>         }
>         # Do stats on %found
> ' "$@"
>

I do understand there is potential breakage when a script is parsing the output. I did not consider that this was a feature someone may want; it really only looks like a breakage to me, for the reasons I've already given.

It's surprising just how broken it looks to me (given that I now know it is not) since all the other filenames output from 'git-grep -l' are valid filenames or object references. There is only this one tree-path instance which does not. I suppose I will learn to live with it.

Jeff King· Aug 25, 2013, 04:41 UTC · re: Phil Hord · lore

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

On Sat, Aug 24, 2013 at 09:35:58PM -0400, Phil Hord wrote:
Show 7 quoted lines
> When the pathspec given to grep includes a tree name, the full
> name of matched files is assembled using colon as a separator.
> If the pathspec includes a tree name, it should use a slash
> instead.
> 
> Check if the pathspec already names a tree and ref (including
> a colon) and use a slash if so.
Makes sense.
> I'm not sure about the detection I used here.  It works, but it is
> not terribly robust.  Is there a better way to handle this?  Maybe
> something like 'prefix_pathspec(name,"");'.

I think the information you want has been thrown away by the time we get to grep_object. Only get_sha1 knows whether the name was really a direct tree reference or if it had to traverse paths.

So we are necessarily reconstructing based on what we know of the syntax. And I think that your rule is OK, because we know that refnames cannot contain a colon. So even though pathnames can, we do not have to care; we only want to know "is there a path in the name", and if we have at least one colon, the answer is yes.

>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

A test would be nice. Both to make sure we do not re-break it, and because it helps demonstrate the problem very easily (it took me a minute to figure out what was going on from your description).

Show 12 quoted lines
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 03bc442..d0deae4 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -480,8 +480,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
>  		len = name ? strlen(name) : 0;
>  		strbuf_init(&base, PATH_MAX + len + 1);
>  		if (len) {
> +			int has_colon = !!strchr(name,':');
>  			strbuf_add(&base, name, len);
> -			strbuf_addch(&base, ':');
> +			strbuf_addch(&base, has_colon?'/':':');

Please use whitespace with your ternary operator. The '/':':' made me think I was reading Perl for a minute. :)

-Peff
Jonathan Nieder· Aug 25, 2013, 05:41 UTC · re: Jeff King · lore

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

Jeff King wrote:
> So we are necessarily reconstructing based on what we know of the
> syntax. And I think that your rule is OK, because we know that refnames
> cannot contain a colon.
What happens with expressions like HEAD^{/test:}?
Jonathan
Jeff King· Aug 25, 2013, 05:54 UTC · re: Jonathan Nieder · lore

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

On Sat, Aug 24, 2013 at 10:41:42PM -0700, Jonathan Nieder wrote:
Show 7 quoted lines
> Jeff King wrote:
> 
> > So we are necessarily reconstructing based on what we know of the
> > syntax. And I think that your rule is OK, because we know that refnames
> > cannot contain a colon.
> 
> What happens with expressions like HEAD^{/test:}?
Ugh, right. Names are more than just refnames.

So I think the only way to do this robustly is to ask get_sha1 to remember more about what happened. We might even be able to get away without teaching get_sha1_with_context anything else; it already records the path, so we should be able to just check whether that is non-empty.

But we use an object_array to store the list of objects, and it has no room for such a bit. So we'd probably want to refactor that, too.

-Peff

← back to recent threads