Re: What's cooking in git.git (Aug 2013, #01; Thu, 1)

[Thomas Rast <trast@inf.ethz.ch>]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> * tr/log-full-diff-keep-true-parents (2013-08-01) 1 commit
>>>  - log: use true parents for diff even when rewriting
>>>
>>>  Output from "git log --full-diff -- <pathspec>" looked strange,
>>>  because comparison was done with the previous ancestor that touched
>>>  the specified <pathspec>, causing the patches for paths outside the
>>>  pathspec to show more than the single commit has changed.
>>>
>>>  I am not sure if that is necessarily a problem, though.  Output
>>>  from "git log --full-diff -2 -- <pathspec>" without this change
>>>  will be applicable to some codebase, but after this change that
>>>  will no longer be true (you will get only tiny parts of the change
>>>  that were made by the two commits in question, while missing all
>>>  the other changes).
>>
>> Hmm.  Uwe's original complaint was that --stat -- as in "what other
>> things are touched when we modify foo" -- is nonsensical.
[...]
> I am not so worried about actually applying that kind of patch, but
> more about reviewing what happened in each of the single logical
> step shown.  If you made two changes in two far-apart commits to
> files you are interested in and want to look at the changes made to
> other files to make each of them a complete solution, depending on
> how the change was split across files, you would get useful to
> useless result with your patch.  In one extreme, if your commits are
> too fine-grained and you committed one path at a time, then
> "full-diff" will not show anything useful, as the commits that hit
> the pathspec do not have changes to any other paths.  In another
> extreme, you may have preparatory changes to other paths in commits
> that immediately precede the commit that hits the pathspec and then
> finally conclude the topic by introducing a caller of the prepared
> codepath in other files to the file you are interested in, and your
> patch will show only the endgame change without showing the
> preparatory steps, which may or may not be useful, depending on what
> you are looking for.
>
> So while I do understand why sometimes you wish to see full diff 
> "git log --full-diff -- <pathspec>" to match output from "git show"
> without pathspec, I am not sure doing it unconditionally and by
> default like your patch does is the best way to go.

I'm utterly unconvinced.

First, note that the behavior only shows when you use an option that
enables parent rewriting, such as --graph or --parents.

So if we went with a default-off option, there would be something like
--diff-original-parents.  The user would have to discover this option,
and use it whenever he wants to use --full-diff in combination with
--graph.  To obtain the same diffs as without --graph.

How do you explain that to a user?  "git-log has about 150 options.
Some of them interact badly, but don't worry, there are options in there
somewhere that make the weirdness go away."

  [No, I didn't actually count, but it seems a good estimate from
  looking at

    git grep -c -E '^-.*::' Documentation/rev-list-options.txt Documentation/diff-options.txt

  ]


Second, gitk's option "Limit diffs to listed paths", corresponding to
the inverse of --full-diff, shows the commits as with git-show.  (Not
internally, but still, the format is very similar.)  In particular it
only shows the changes from the commit itself.


Third, the only actual user data point I have is Uwe, who clearly
expected his diffs to remain the same across --graph.  There's a lot of
selection bias in this, because a happy user would not have complained,
but we can look at his example again (I added a right-hand side to the
range to make it reproducible).

With the patched version:

  $ git log --parents --stat v3.8..v3.9 --full-diff -- drivers/net/ethernet/freescale/fec.c
  commit 8d7ed0f051caf192994c02fbefb859eac03d1646 14109a59caf93e6eae726a34bf2f0897508ec8c1
  [...]
      net: fec: fix regression in link change accounting
  [...]
   drivers/net/ethernet/freescale/fec.c | 1 +
   1 file changed, 1 insertion(+)

With current master:

  commit 8d7ed0f051caf192994c02fbefb859eac03d1646 14109a59caf93e6eae726a34bf2f0897508ec8c1
  [...]
      net: fec: fix regression in link change accounting
  [...]
   Documentation/sound/alsa/ALSA-Configuration.txt    |    5 +-
   MAINTAINERS                                        |   22 +-
   Makefile                                           |    2 +-
   arch/alpha/Makefile                                |    2 +-
  [...]
   509 files changed, 9507 insertions(+), 7014 deletions(-)

509 files.  I don't know about you, but I think that's completely
useless for reviewing anything.


The only compromise I can see flying is using something like
--diff-parents=original and --diff-parents=rewritten, defaulting to
'original'.  Behind the scenes they would toggle the appropriate
machinery: 'rewritten' would enable parent simplification even when it
is not otherwise enabled, and 'original' would enable saving parents
whenever simplification is enabled.  Naturally all of this only applies
with --full-diff.


> In the meantime:
>
>     git rev-list --header --parents HEAD -- <pathspec> |
>     git -p diff-tree -p --stdin
>
> would be one way to do so.

Well, as a data point on how realistic this is, consider two things:

* I've hacked around on git for about five years now, and do not
  remember ever noticing or using --header.  Nor, for that matter, ever
  using rev-list|diff-tree for actual work.

* It doesn't work at my end: it doesn't show any diffs.  Removing
  --header helps, but then you lose the log messages.  --pretty again
  breaks the diffs.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch