Filenames and prefixes in extended diffs

I'm having a problem filename prefixes in git's extended diffs for patches which rename or copy files: those patches include the old and new filenames in "rename from", "rename to", "copy from", and "copy to" headers, e.g.,

	$ git show -M
	diff --git a/f b/g
	similarity index 87%
	rename from f
	rename to g
	index f00c965..3bb459b 100644
	--- a/f
	+++ b/g
	@@ -8,3 +8,4 @@
	 8
	 9
	 10
	+11

Unlike the filenames in the "diff --git", "---", and "+++" headers, the "rename from", "rename to", "copy from", and "copy to" filenames do not include prefixes.

Now when applying a patch, GNU patch's -p option determines the number of pathname components to strip off from filenames. This obviously can't work consistently for the prefixed and prefix-less headers.

Can git be changed to include prefixes in all filenames?

The only alternative I see is to ignore the filenames in the rename/copy headers and rely only on the "diff --git" line. (The "---" and "+++" headers are not guaranteed to exist.) What's worse, as already discussed here, the "diff --git" line uses space as a separator between filenames yet it doesn't quote spaces in filenames. When being forced to ignore rename/copy headers, this defect would make things much worse.

Any ideas?

Thanks, Andreas

Andreas Gruenbacher <agruen@suse.de> writes:

How about studying what "git-apply" does?

Andreas Gruenbacher <agruen@suse.de> writes:

Just to save your time coming up with more ways to *change* git diff...

Even though I wouldn't say _any_ change is too late to bring in, change in the output format from "git diff" family _must_ be usable by "git apply" people have been using for the last 4 years or so.

Suppose your updated version of "git diff" with a certain set of options produces output A, which is different from the output B you would get out of today's "git diff" that is run with the same set of options.

If "git apply" people have been using understands B (i.e. current output) and does something, the format change between A and B must be designed in such a way that the same "git apply" accepts A (i.e. your output) and do the same thing.

Two examples:

 - "git diff -M" (or "git show -M") is _defined_ to show the filenames
   without prefix on "rename from" line, and deployed "git apply" relies
   on this definition to apply the patch to the file the patch was meant
   to apply.  If your modified "git diff -M" changes it to add the prefix,
   and existing "git apply" changes behaviour (either by rejecting your
   output, or applying the patch to a wrong file), then such a change has
   *no chance* of getting in.  It is merely a breakage.

 - If you say "git diff --src-prefix=a/b/c --dst-prefix=x/y", it _might_
   produce something "git apply" won't grok (I haven't checked this,
   though).  You can suggest to change the output from such a case to work
   better.  We didn't work as expected so a change _could_ be a fix.

The output from "git diff --no-index" is an exception to the above rule. It is primarily for people who have unmanaged contents and want to use features of the git diff engine that are not found in other people's diff implementations (e.g. wordwise colored diff), and the header part of its output does not currently follow "git diff" convention to be grokkable by "git apply".

Fixing _that_ is a welcome change, but I suspect that there are corner cases, e.g. "git diff --no-index frotz-1.2.36/ /tmp/frotz/" (i.e. you have a pristine version in frotz-1.2.36 directory, but your modified version is in /tmp/frtoz/) that might make fixing it fundamentally impossible (I haven't looked into it for a long time, so it could be easy, but my gut feeling is it isn't).

Quoting Junio C Hamano <gitster@pobox.com>

Is it possible to give --no-index option to "git grep", please?

Nanako Shiraishi <nanako3@lavabit.com> writes:

Surely. And "grep" is much easier to do than "diff". Will send a patch perhaps during my lunch break.

This moves the call to setup_git_directory() for running "grep" from the "git" wrapper to the implementation of the "grep" subcommand. A new variable "use_index" is always true at this stage in the series, and when it is on, we require that we are in a directory that is under git control. To make sure we die the same way, we make a second call into setup_git_directory() when we detect this situation.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Junio C Hamano <gitster@pobox.com> writes:

 > Nanako Shiraishi <nanako3@lavabit.com> writes:
 >
 >> Is it possible to give --no-index option to "git grep", please?
 >
 > Surely.  And "grep" is much easier to do than "diff".  Will send a
 > patch perhaps during my lunch break.

 This is merely a preparatory step.

 builtin-grep.c |    7 +++++++
 git.c          |    2 +-
 2 files changed, 8 insertions(+), 1 deletions(-)

Just like some people wanted diff features that are not found in other people's diff implementations outside of a git repository and added --no-index mode to the command, this adds --no-index mode to the "git grep" command.

Also, inside a git repository, --no-index mode allows you to grep in untracked (but not ignored) files.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is much easier than "git diff --no-index" because it does not have
   to worry about corner cases like "git diff --no-index file1 file2".

 builtin-grep.c  |   26 ++++++++++++++++++++++++++
 t/t7002-grep.sh |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 0 deletions(-)

On Fri, Jan 15, 2010 at 12:52:40PM -0800, Junio C Hamano wrote:

Out of curiosity, what are the interesting features in git grep versus other greps?

-Peff

Jeff King <peff@peff.net> writes:

Three examples:

    git grep -e Junio --and -e Dscho --and -e Peff

is different from

    grep "Junio.*Dscho.*Peff"

in that the latter wouldn't find a line that has these names in different order. You can of course give permutations explicitly, like

    grep -e "Junio.*Dscho.*Peff" \
         -e "Dscho.*Junio.*Peff" \
         ...
	 -e "Peff.*Dscho.*Junio"

I don't know how you would do these with "grep":

    git grep -e Junio --and -e Dscho --and --not -e Linus

    git grep --all-match -e Junio -e Dscho

On Fri, Jan 15, 2010 at 05:05:42PM -0800, Junio C Hamano wrote:

Right. I would do:

  grep Junio | grep Dscho | grep Peff

No, it's not quite as accurate, as you are grepping the filenames too.

And no, it's not as efficient, but given that the first grep eliminates most of your input anyway, it's generally not a big deal.

So the short answer to my question seems to be "git grep has logical operators". I don't find that compelling, but I guess some people do. Thanks for satisfying my curiosity.

I would do "grep Junio | grep Dscho | grep -v Linus".

That one is a little harder (though it is not something I do very often, and I had to actually read the docs to find what --all-match does):

  grep Junio `grep -l Dscho *`

which of course has problems with exotic filenames.

-Peff

Jeff King <peff@peff.net> writes:

Also it doesn't find lines that match Dscho in the result ;-)

Realistically, this most often is used when grepping in the log, e.g.

    git log --all-match --author=peff --grep=test

I actually wish "log" to somehow default to --all-match mode at least when using the --author option. "Change by Jeff, or about test by anybody" is rarely what I would want to look for.

On Fri, Jan 15, 2010 at 08:15:49PM -0800, Junio C Hamano wrote:

Kinda like this?

I originally had it set grep_filter.all_match in --author only, but then I thought "why author and not commiter too", so changing the default seemed like the natural thing to do. Or it could be a cat brained idea, I dunno ;)

-- -- -- 8< -- -- -- 8< -- -- --
From 2277a6e512c2f597c6240f06c9e7d5ff83e2fe3f Mon Sep 17 00:00:00 2001
From: David Aguilar <davvid@gmail.com>
Date: Fri, 15 Jan 2010 21:18:36 -0800
Subject: [PATCH] Make --all-match the default in "log" family

'git log --author=peff --grep=test' means "search for commits by Jeff, or about test by anybody," which is rarely what what we want to do. The original behavior can by achieved by specifying --no-all-match.

Reference: http://article.gmane.org/gmane.comp.version-control.git/137197
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/rev-list-options.txt |    1 +
 revision.c                         |    3 +++
 t/t7002-grep.sh                    |    2 +-
 3 files changed, 5 insertions(+), 1 deletions(-)

David Aguilar <davvid@gmail.com> writes:

Not quite.  What I really want is

    git log --author=davvid --grep=difftool --grep=mergetool

to find all commits by you that is about (either diff or mergetool). I think your patch will limit the search only to your patch that talks about both of these two tools (not necessarily on the same line, but in the same commit).

The extended "grep" expression parser by default creates a list of OR'ed terms. What --all-match does is to make this top-level chain to mean "all of these must trigger somewhere in the whole _document_ (not an individual line), for the document to be considered a hit" for the purpose of "grep -l", and when used with "log" family, --author/--committer/--grep are used to limit the output to commits "grep -l" would say "yes, this document has matched".

Currently,

    git log --author=davvid --grep=difftool --grep=mergetool

will parse to a list of three terms:

    GREP_PATTERN_HEAD("^author .*davvid")
    GREP_PATTERN_BODY("difftool")
    GREP_PATTERN_BODY("mergetool")

And giving --all-match will require all of these OR'ed terms to appear in the commit object.

My dream one will probably has to make a list of two terms as its parse tree instead, like this:

    GREP_PATTERN_HEAD("^author .*davvid")
    GREP_NODE_OR(
        GREP_PATTERN_BODY("difftool")
        GREP_PATTERN_BODY("mergetool")
    )           

and then run it with --all-match semantics. The top-level consists of two terms, and they both must hit, but the second term is an OR'ed one.

It is unclear how we would want to throw the committer in the mix. If we make this parse tree:

    GREP_PATTERN_HEAD("^author .*davvid")
    GREP_PATTERN_HEAD("^committer .*gitster")
    GREP_NODE_OR(
        GREP_PATTERN_BODY("difftool")
        GREP_PATTERN_BODY("mergetool")
    )           

we would be looking for your patch about either diff or mergetool _and_ it has to be committed by me. On the other hand, if we do this:

    GREP_NODE_OR(
        GREP_PATTERN_HEAD("^author .*davvid")
        GREP_PATTERN_HEAD("^committer .*gitster")
    )
    GREP_NODE_OR(
        GREP_PATTERN_BODY("difftool")
        GREP_PATTERN_BODY("mergetool")
    )           

we would be looking for a patch about (either diff or mergetool) _and_ (either committed by me or written by you).

I think the former makes more sense in _our_ project (because there are very few committers), but in the context of other projects, e.g. the Linux kernel, you may want to give "Linus" to both --author and --committer to track what he did (either as an author to some other subsystem, or as the top-level integrator for the entire system), and for such a use case, the latter would make more sense.

Unfortunately, the parsing of --grep/--author/--committer options to the log family is quite limited (you cannot give --and, --or and --not, for example), and it would be hard to express these distinction.

On Fri, Jan 15, 2010 at 08:15:49PM -0800, Junio C Hamano wrote:

Agreed. That is the most common log grep pattern for me (author + grep), and I always want all-match. I see from later in the thread, though, that implementing it is not as straightforward as we might hope.

I would personally be fine with "--all-match" being the default, but that may be too big a change in behavior for some people to swallow (I would also be fine with log.allmatch in the config, but every time I suggest something like that people's heads explode and I get told to make an alias).

-Peff

Jeff King <peff@peff.net> writes:

I haven't looked at the codepath for quite some time but I have a feeling that it probably won't be too bad.

It just won't be as simple as flipping the all_match bit with a one-liner.

Before calling compile_grep_patterns() in revision.c::setup_revisions(), we probably would want to massage revs->grep_filter to result in the desired grep expression parse tree, i.e. from

    GREP_PATTERN_HEAD("^author .*davvid")
    GREP_PATTERN_HEAD("^committer .*gitster")
    GREP_PATTERN_BODY("difftool")
    GREP_PATTERN_BODY("mergetool")

to

    GREP_PATTERN_HEAD("^author .*davvid")
    GREP_PATTERN_HEAD("^committer .*gitster")
    GREP_NODE_OR(
        GREP_PATTERN_BODY("difftool")
        GREP_PATTERN_BODY("mergetool")
    )           

Junio C Hamano <gitster@pobox.com> writes:

Perhaps something like this.

-- >8 --
Subject: "log --author=me --grep=it" should find intersection, not union

Historically, any grep filter in "git log" family of commands were taken as restricting to commits with any of the words in the commit log message. However, the user almost always want to find commits "done by this person on that topic". With "--all-match" option, a series of grep patterns can be turned into a requirement that all of them must produce a match, but that makes it impossible to ask for "done by me, on either this or that" with:

	log --author=me --grep=this --grep=that

because it will require both "this" and "that" to appear.

Change the "header" parser of grep library to treat the headers specially. When parsing the above, behave as if it was specified like this on the command line:

	--all-match --author=me '(' --grep=this --grep=that ')'

Even though the "log" command line parser doesn't give direct access to the extended grep syntax to group terms with parentheses, this change will cover the majority of the case the users would want.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-grep.c |    1 +
 grep.c         |   20 ++++++++++++++++++--
 grep.h         |    2 ++
 revision.c     |    1 +
 4 files changed, 22 insertions(+), 2 deletions(-)

On Sun, Jan 17, 2010 at 08:02:25PM -0800, Junio C Hamano wrote:

Hmm. I like the new behavior. The implementation feels a little hack-ish, like we should really be supporting full-on:

  git log --author=me --and --grep=foo

That gets a little weird, though. We already have "--not" for ref limiting, so clearly there is some conflict over exactly what logical operators would be operating on. I guess we could use context to see that the adjacent arguments were grep-related.

So perhaps, as you say, this is enough as it covers the usual case.

-Peff

Jeff King <peff@peff.net> writes:

That is fundamentally wrong.

Remember, "grep" works on two levels: a line matches or does not match the given set of patterns (rather, the expression given), and matched lines are shown. A file as a whole is considered to have matched if one or more lines produced a match, or under the --all-match option, only when all of the top-level ORed terms in the expression have fired for some lines in it.

And --not and --and are both elements of grep expression that determines if the expression matches "a single line". --author=me --and --grep=foo would ask: does the "^author " line in the header have "me" _and_ also string "foo" on it at the same time?

IOW, most of the "logical" stuff (including the precedence binding parentheses) works at a line level. --all-match is currently the only thing that affects "grep -l" (and "will the commit get shown") behaviour by collecting hits from the whole buffer.

On Sun, Jan 17, 2010 at 10:30:19PM -0800, Junio C Hamano wrote:

Fundamentally wrong for the way "log --grep" is currently implemented perhaps, but I don't see anything wrong with considering each commit as a single "record", just as regular grep considers each line to be a record. That is a much more useful distinction for log traversal than lines, which are useless from the user's perspective. If searching for two terms, I care about whether they are in the same commit message, but I don't care at all about line breaks.

Yes, I know that internally --author is really about line-matching the commit headers, but that is an implementation detail. The mental model we should present to the user is record-matching based on specific fields like author, committer, or body text.

-Peff

On Thursday 14 January 2010 01:16:21 am Junio C Hamano wrote:

Git apply is currently broken in some (uncommon) cases. Consider the following two patches:

	$ cat add.diff
	diff --git a/d/f b/d/f
	new file mode 100644
	index 0000000..6a69f92
	--- /dev/null
	+++ b/d/f
	@@ -0,0 +1 @@
	+f

	$ cat rename.diff
	diff --git a/d/f b/d/g
	similarity index 100%
	rename from d/f
	rename to d/g

They apply fine after each other with plain "git apply". When you try to apply them to a different location, things break though:

	$ git apply --directory e -p2 add.diff
	$ git apply --directory e -p2 rename.diff
	error: e/d/f: No such file or directory

Had the second patch not been generated with "git diff -M", things would just have worked; in other words, the -M format is broken.

I think the easiest way to fix this in "git apply" would be to figure out what the missing prefixes are in the rename and copy lines (in this case, just "a/" and "b/"), and to prepend those prefixes to the decoded filenames before stripping off pathname components. This is the same as just taking the filenames from the "diff --git" line, and ignoring the filenames in the "copy from", "copy to", "rename from", and "rename to" headers.

I don't see a way how to fix this in the existing headers in a backwards compatible way. Do you?

Independent of whether and how this is addressed, can I please at least have the "diff --git" line parsing problem fixed so that filenames which contain spaces are put in double quotes there? Then I can at least ignore all the prefix-less filenames in GNU patch and still make it understand git's output.

The output format for that is fine.

Patches with a different number of components in the from and to prefixes are a really bad idea. (GNU patch will prefer the pathname with the fewer components, but this yould just as well be the wrong one.)

Thanks, Andreas