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

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:58:31

Phil Hord [off-list ref] writes:
On Mon, Aug 26, 2013 at 3:14 AM, Junio C Hamano [off-list ref] wrote:
quoted
Junio C Hamano [off-list ref] writes:
quoted
Jonathan Nieder [off-list ref] writes:
quoted
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?".
quoted
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.

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

From: Phil Hord <hidden>
Date: 2016-06-15 22:58:31

On Mon, Aug 26, 2013 at 12:23 PM, Junio C Hamano [off-list ref] wrote:
Phil Hord [off-list ref] writes:
quoted
On Mon, Aug 26, 2013 at 3:14 AM, Junio C Hamano [off-list ref] wrote:
quoted
Junio C Hamano [off-list ref] writes:
quoted
Jonathan Nieder [off-list ref] writes:
quoted
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?".
quoted
quoted
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

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

From: Phil Hord <hidden>
Date: 2016-06-15 22:58:31

On Mon, Aug 26, 2013 at 12:49 PM, Phil Hord [off-list ref] 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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help