Re: [PATCH] add log.fulldiff config option

Jeff King [off-list ref] writes:

I use --full-diff all the time, so this should save some typing. I can't
think of a time when I wouldn't want it on, but if there is, we probably
need a --no-full-diff.

Absolutely.

Also, should this instead be diff.fulldiff?

Probably not.

Also also, I was going to submit a patch to document --full-diff,
but I had a few questions. Should it go in diff-options? That makes some
sense to me, but the parsing actually happens in setup_revisions.

Not at all.

Furthermore, it seems to do the same thing as --pickaxe-all. Should we
try to combine these?

No.

Try this with various combinations of with and without
the two flags.

    $ git log -1 -Sstate --full-diff --pickaxe-all b3d9899 -- git-fetch.sh

Your understanding needs to be clarified on how paths are
filtered, and how log family and diff interacts, and probably
these need to be better documented.

First, lets forget about 'log', but talk about diff (any of
diff-tree, diff-index --cached, diff-index, and diff-files).
They take paths as arguments.  The arguments limit the input
side of the diff.

    Side note: that is why "diff -C arch/x86_64" would not
    care about renames or copies from arch/i386 to
    arch/x86_64.  The diff machinery does not even see files
    under arch/i386 with that command line.

The pickaxe (-S) is defined at the 'diff' layer, and works as
the output filter from the 'diff' machinery.  It omits the
filepair (pair of preimage and postimage) whose both sides have
the same number of specified string in them ("pickaxe
criteria").

    $ git-diff-tree --name-status b3d9899

will show three paths, git-fetch.sh, git-ls-remote.sh, and
pkt-line.c.  But only git-fetch.sh matches the pickaxe criteria.
That's why:

    $ git-diff-tree -p -Sstate b3d9899

shows only git-fetch.sh and omits the other two paths.

The option --pickaxe-all changes this paths filtering to
all-or-none.  If any of the incoming paths matches the pickaxe
criteria, all changed paths, even the ones that do not match the
criteria, are included in the output.  Thus,

    $ git-diff-tree -p -Sstate --pickaxe-all b3d9899

gives three paths, although the other two paths do not change
the number of string "state".

With me so far?

Now, when you give paths to commands of the log family, they are
used to filter commits considered for the output by the revision
traversal machinery.  The revision machinery feeds only commits
whose tree is different from its parents at the specified paths
to the commands in the log family.  b3d9899 is eligible for
output in the first example because it changes "git-fetch.sh".

Then the commit is fed to the tree-diff machinery, with the same
paths limiter given to the log command.  If the diff machinery
says there is no interesting diff, the commit is not even
output.

    Side note: that is why

        $ git log -p -1 -Snosuchstring b3d9899 -- git-fetch.sh

    does not say anything.  Pickaxe says git-fetch.sh does not
    change "nosuchstring" and nothing comes out of the diff
    machinery.

What --full-diff does is to dissociate the paths limiter from
the diff machinery.  Before talking about pickaxe interaction,
let's try one without it.

Compare:

    $ git log -p -1             b3d9899 -- git-fetch.sh
    $ git log -p -1 --full-diff b3d9899 -- git-fetch.sh

In both, git-fetch.sh is first used by the revision traversal to
determine that b3d9899 is worth considering (the commit changes
that path).  Then the former uses the same paths limiter to
compute the diff for the commit.  Hence you will see only
git-fetch.sh.  The latter, however, does not use git-fetch.sh as
the paths limiter to generate diff because of --full-diff; it
feeds the full trees and that is why you can see three files.

Finally.

How do all of the above interact with the pickaxe?

    $ git log -p -1 -Sstate b3d9899 -- git-fetch.sh

Because git-fetch.sh is changed by b3d9899, revision traversal
makes the commit eligible.  It feeds that single path to diff
machinery and pickaxe says it changes "state".  Then it outputs
that path.

How about this one?

    $ git log -p -1 -Sstate --full-diff b3d9899 -- git-fetch.sh

git-fetch.sh makes b3d9899 eligible, --full-diff makes all three
paths touched by the commit (ignoring the paths you gave from
the command line) to be fed to the diff machinery, and pickaxe
picks git-fetch.sh because that is the only one that changes
"state".  For the same reason, you would see git-fetch.sh out of
this one:

    $ git log -p -1 -Sstate --full-diff b3d9899 -- pkt-line.c

Truly finally.

If you add --pickaxe-all to the last one:

    $ git log -p -1 -Sstate --pickaxe-all --full-diff b3d9899 -- pkt-line.c

pkt-line.c makes b3d9899 eligible, --full-diff feeds three
changed paths to diff machinery, pickaxe notices that one of
them (git-fetch.sh which is different from what you gave from
the command line) changes "state", --pickaxe-all causes all
three incoming paths to be output.

On Wed, Dec 20, 2006 at 12:14:14AM -0800, Junio C Hamano wrote:

quoted

Also, should this instead be diff.fulldiff?

Probably not.

Based on your explanation, I think that makes sense.

quoted

Also also, I was going to submit a patch to document --full-diff,
but I had a few questions. Should it go in diff-options? That makes some
sense to me, but the parsing actually happens in setup_revisions.

Not at all.

Then where should it go? whatchanged and log have a great deal of
overlap in terms of parsing, but they don't seem to share any
documentation (or note the fact that git-whatchanged is basically just
git-log -p).

quoted

Furthermore, it seems to do the same thing as --pickaxe-all. Should we
try to combine these?

No.

Thanks for the thorough explanation. I think this makes my config option
description a little bit off, then, since it doesn't actually show all
of the changed files for a commit in the case of pickaxe.

What has me confused, then, is that you said this:

I typically do:

        git log --full-diff -p -SCOLLISION

The --full-diff option helps because it shows the diff for other
files (that do not have different number of substring COLLISION
in the pre and postimage) in the same commit as well.

But that isn't the case, AFAIK (e.g., the command line you mention shows
only changes to sha1_file.c in commit aac17941, but cache.h was also
changed). Did you mean --pickaxe-all?

Your understanding needs to be clarified on how paths are
filtered, and how log family and diff interacts, and probably
these need to be better documented.

To be honest, I'm not sure when one would really _want_ the flexibility
this provides. It might be more intuitive to git-{log,whatchanged} users
to have a single option (config and command line) to set --full-diff and
--pickaxe-all together. At the very least, I think I now want a
diff.pickaxeall config option.

Signed-off-by: Jeff King <redacted>
---
On Wed, Dec 20, 2006 at 12:14:14AM -0800, Junio C Hamano wrote:

quoted

I use --full-diff all the time, so this should save some typing. I can't
think of a time when I wouldn't want it on, but if there is, we probably
need a --no-full-diff.

Absolutely.

I took this to mean "absolutely we need --no-full-diff." :)

I note that --full-diff sets rev->diff = 1. The log.fulldiff config
option does not, and nor does --no-full-diff unset it. However, I'm not
sure it makes sense to set it. Doing "git-log --full-diff" outputs an
extra line (separating the diff from the commit log) but since we
haven't told it any type of diff to output, the diff is blank. And if we
had told it a type, then that would have turned on rev->diff. So I don't
see a point in setting it.

 revision.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 56819f8..ac4dbf2 100644
--- a/revision.c
+++ b/revision.c

@@ -955,6 +955,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 				revs->full_diff = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--no-full-diff")) {
+				revs->full_diff = 0;
+				continue;
+			}
 			if (!strcmp(arg, "--full-history")) {
 				revs->simplify_history = 0;
 				continue;

-- 
1.4.4.2.gee7ba-dirty