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

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