git log fails to show all changes for a file

git log fails to show all changes for a file

[Olaf Hering]

I wonder why this command fails to show all commits that modify a given
function: 

  linux.git $ git log -p -M --stat -- drivers/hv/channel_mgmt.c

With commit 1f656ff3fdddc2f59649cc84b633b799908f1f7b init_vp_index() has
"const uuid_le *type_guid" already. And somewhere between commit
d3ba720dd58cdf6630fee4b89482c465d5ad0d0f and the one above the "const"
was added. But git log does not show this commit.

Why is that so, and whats the command to really show all and every change?

Olaf

Re: git log fails to show all changes for a file

[John Keeping]

On Tue, Jul 14, 2015 at 09:30:35AM +0200, Olaf Hering wrote:
> 
> I wonder why this command fails to show all commits that modify a given
> function: 
> 
>   linux.git $ git log -p -M --stat -- drivers/hv/channel_mgmt.c
> 
> With commit 1f656ff3fdddc2f59649cc84b633b799908f1f7b init_vp_index() has
> "const uuid_le *type_guid" already. And somewhere between commit
> d3ba720dd58cdf6630fee4b89482c465d5ad0d0f and the one above the "const"
> was added. But git log does not show this commit.
> 
> Why is that so, and whats the command to really show all and every change?

It was added in an evil merge (f9da455b93f6ba076935b4ef4589f61e529ae046),
try:

	git log -p -M --stat --cc -- drivers/hv/channel_mgmt.c

Re: git log fails to show all changes for a file

[Olaf Hering]

On Tue, Jul 14, John Keeping wrote:

> It was added in an evil merge (f9da455b93f6ba076935b4ef4589f61e529ae046),
> try:
> 
> 	git log -p -M --stat --cc -- drivers/hv/channel_mgmt.c

Thanks. Thats rather useless output...

@@@ -404,7 -365,7 +404,7 @@@ static u32  next_vp
   * performance critical channels (IDE, SCSI and Network) will be uniformly
   * distributed across all available CPUs.
   */
- static void init_vp_index(struct vmbus_channel *channel, uuid_le *type_guid)
 -static u32 get_vp_index(const uuid_le *type_guid)
++static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guid)
  {
        u32 cur_cpu;
        int i;


Olaf

Re: git log fails to show all changes for a file

[Andreas Schwab]

Olaf Hering <olaf@aepfle.de> writes:

> On Tue, Jul 14, John Keeping wrote:
>
>> It was added in an evil merge (f9da455b93f6ba076935b4ef4589f61e529ae046),
>> try:
>> 
>> 	git log -p -M --stat --cc -- drivers/hv/channel_mgmt.c
>
> Thanks. Thats rather useless output...

Why do you think this is useless?

> @@@ -404,7 -365,7 +404,7 @@@ static u32  next_vp
>    * performance critical channels (IDE, SCSI and Network) will be uniformly
>    * distributed across all available CPUs.
>    */
> - static void init_vp_index(struct vmbus_channel *channel, uuid_le *type_guid)
>  -static u32 get_vp_index(const uuid_le *type_guid)
> ++static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guid)

One branch renamed get_vp_index to init_vp_index, the other branch added
the const attribute.  This hunk combines both changes.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: git log fails to show all changes for a file

[Linus Torvalds]

On Tue, Jul 14, 2015 at 12:59 AM, Olaf Hering <olaf@aepfle.de> wrote:
> On Tue, Jul 14, John Keeping wrote:
>
>> It was added in an evil merge (f9da455b93f6ba076935b4ef4589f61e529ae046),

That's not an evil merge. That's just a regular merge. One side had
changed the argument to "const":

 - 1b9d48f2a579 ("hyper-v: make uuid_le const")

while the other side had renamed the function and added an argument,
and changed the return type:

 - d3ba720dd58c ("Drivers: hv: Eliminate the channel spinlock in the
callback path")

an "evil merge" is something that makes changes that came from neither
side and aren't actually resolving a conflict.

That said, I do wonder if we should just make "-p" imply "--cc". Right
now we have the kind of odd situation that "git log -p" doesn't show
merge patches, but "git show <cmit>" does show them. And you kind of
have to know a lot about git to know the "--cc" option.

In fact, that "git show" behavior really is very subtle, but very
useful. It comes from show_rev_tweak_rev(), which is a magic git-show
thing.

               Linus

Re: git log fails to show all changes for a file

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> That said, I do wonder if we should just make "-p" imply "--cc". Right
> now we have the kind of odd situation that "git log -p" doesn't show
> merge patches, but "git show <cmit>" does show them. And you kind of
> have to know a lot about git to know the "--cc" option.
>
> In fact, that "git show" behavior really is very subtle, but very
> useful. It comes from show_rev_tweak_rev(), which is a magic git-show
> thing.

I would think "git log -p" that implies "--cc" would be a good
change, as long as there is an easy escape hatch to let us do what
we have to do with a rather lengthy "git log -p --first-parent -m"
(i.e. show the change relative to its first parent while traversing
the first parent chain) today.  Perhaps only when there is no
explicit "-m" but "-p" is given, automatically enable "--cc", or
something like that.

Re: git log fails to show all changes for a file

[Linus Torvalds]

On Wed, Jul 15, 2015 at 9:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I would think "git log -p" that implies "--cc" would be a good
> change, as long as there is an easy escape hatch to let us do what
> we have to do with a rather lengthy "git log -p --first-parent -m"
> (i.e. show the change relative to its first parent while traversing
> the first parent chain) today.  Perhaps only when there is no
> explicit "-m" but "-p" is given, automatically enable "--cc", or
> something like that.

That's very close to what "git show" does through that magic
show_rev_tweak_rev() logic, with the crucial difference being that
"git show" is designed to always show the diff, so the "-p" is
implied.

That said, having thought about it more, I'm not entirely sure we can
do it.  The big conceptual difference between "git log" and "git show"
is obviously that "git show" doesn't walk the revision list, and you
always explicitly say "show _this_ commit".

And that means that with "git show", you kind of _know_ that the
commit is relevant and interesting, in a way that "git log" does not.
So got "git show", it's very natural to say "show all the relevant
information", while for "git log" we did make the choice that maybe
commit diffs aren't relevant by default.

And the whole issue ends up boiling down to "maybe we picked the wrong
choice default". We default to that "ignore_merges = 1" behavior.

Now, we defaulted to ignoring merge diffs because long long ago, in a
galaxy far away, we didn't have a great way to show the diffs. The
whole "--cc" option goes back to January -06 and commit d8f4790e6fe7
("diff-tree --cc: denser combined diff output for a merge commit") .
And before that option - so for about 8 months - we had no good way to
show the diffs of merges in a good dense way.

So the whole "don't show diffs for merges by default" actually made a
lot of sense originally, because our merge diffs were not very useful.

But that does mean that if we now enable "--cc" by default when you
ask for diffs, we have no good way to _disable_ it. We picked "disable
by default", and "-m" means "enable merge diffs". And that made sense
back in 2005 because we really wanted to disable the whole "show diffs
for merges" thing.

Of course, you can use "--no-merges" to basically not show merges at
all, so maybe that's ok. But I get the feeling that if we make "-p"
imply "--cc", we should probably add a "--no-merge-diff" option too to
replace the (broken) "-m" flag properly. And I'm a tiny bit worried
that it might break some script (although I'm really not seeing
how/why you'd script "git log -p" output and not want to get a --cc
patch for a merge).

And "-m"? We should probably get rid of it. The diffs we get for
merges when "-c" or "--cc" isn't given are _not_ useful. The original
non-combined diff format was really just useful for showing that
"yeah, we have multiple parents, and they are different in all these
ways".  So there is no actual valid use for "-m" that I can imagine.
It's simply just an odd historical artifact from a time when we didn't
know how to show merges.

                 Linus

Re: git log fails to show all changes for a file

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> ...
> So the whole "don't show diffs for merges by default" actually made a
> lot of sense originally, because our merge diffs were not very useful.

I agree with most of your analysis of the history, but not with "-m"
(rather, a part of what "-m" does).

> And "-m"? We should probably get rid of it. The diffs we get for
> merges when "-c" or "--cc" isn't given are _not_ useful. The original
> non-combined diff format was really just useful for showing that
> "yeah, we have multiple parents, and they are different in all these
> ways".  So there is no actual valid use for "-m" that I can imagine.

When combined with "--first-parent" it is a way to view what the
merge brought in from a side branch.

    $ git log --first-parent -m -p ko/master..master

is something I do every time I merge a batch of topics to the
'master' branch (where ko/master keeps track of what was pushed
to k.org the last time).

The fact that "-m" shows diffs with each and every parent is an odd
historical behaviour, and the only justification for it is that
people might want to view differences with second and later parents,
which is weak, and one way to allow them to view differences with
later parents is to show all (and force people to skip them in
normal use case), which is even weaker.

So to be constructive and to pu a different proposal on the table,
how about aiming for this end-game state?

 * When '-p' is given, we show only diff with first-parent by
   default, regardless of the traversal (i.e. --first-parent option
   currently controls both traversal and patch display, but in the
   new world order, it reverts back to purely a traversal option).

 * We could allow you to say '-p -m2' to get second-parent diff for
   merges, but I do not think we should bother, for at least two
   reasons.  It opens a can of worms, like "what should '-p -m4' to
   for a two-parent merge?"  Also "log" unlike "show" is about many
   commits, and I do not think of a reason why you would want a
   series of diff between the side-branch tip and the result.  '-m2'
   would only be useful for commits that was made by mistake,
   merging the trunk into a side branch, swapping the
   first-parenthood, which ought to be rare.

 * If somebody wants to omit patch for merges, while still showing
   the merge log, use '-p --no-show-merge-diff'.

 * When people want '--cc' or '-c', they can with 'log -p --cc',
   just like today.  We might want to make '--cc' imply '-p', but I
   vaguely recall some discussion against this [*2*].

Transition plan is a separate matter.


[Footnotes]

*1* With modern Git if you were pushing only to a single
    destination, you could say master@{push} instead, but I do not
    have remote.origin in my repository and push to multiple places,
    so master@{push} does not resolve ;-)

*2* http://thread.gmane.org/gmane.comp.version-control.git/215341/focus=215470

Re: git log fails to show all changes for a file

[Linus Torvalds]

On Wed, Jul 15, 2015 at 10:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>  * When '-p' is given, we show only diff with first-parent by
>    default, regardless of the traversal (i.e. --first-parent option
>    currently controls both traversal and patch display, but in the
>    new world order, it reverts back to purely a traversal option).

So this is a suggested change to "-p -m" behavior?

Yes, that sounds sane. The current "-p -m" behavior is not useful at all.

So if I understand rightly, we'd have:

 "-p" would be what is currently  "-p --cc"
 "-p -m" would be what is currently "-p --first-parent"
 "-p --no-show-merge-diffs" would be what is currently "-p"

and the rationale would be that

 (a) the current "-p" is hiding things, and while you can add "--cc",
that requires that you really understand what is being hidden, which
is a bad default (the complaint that started this discussion)

 (b) the current "-p -m" is useless crazy stuff, and you'd rather use
it for something that you actually find very common and useful

If so, I agree entirely.

              Linus

Re: git log fails to show all changes for a file

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, Jul 15, 2015 at 10:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>  * When '-p' is given, we show only diff with first-parent by
>>    default, regardless of the traversal (i.e. --first-parent option
>>    currently controls both traversal and patch display, but in the
>>    new world order, it reverts back to purely a traversal option).
>
> So this is a suggested change to "-p -m" behavior?

Not really.  This is a suggested behaviour for "git log -p"; I
wasn't very enthused by the idea to turn --cc when user said -p
without telling them what we are doing.  In other words, if the
users want combined, they should say --cc (and they will get a
single-parent patch for non-merges with --cc) so there is no reason
not to do this, as long as we fix --cc so that "git log --cc"
implies "git log --cc -p".

> Yes, that sounds sane. The current "-p -m" behavior is not useful at all.
>
> So if I understand rightly, we'd have:
>
>  "-p" would be what is currently  "-p --cc"

Not really.

>  "-p -m" would be what is currently "-p --first-parent"

Not really.  I was dropping "-m" entirely with "we could do -p -m2
but I do not think we should bother".

>  "-p --no-show-merge-diffs" would be what is currently "-p"

Yes.

> and the rationale would be that
>
>  (a) the current "-p" is hiding things, and while you can add "--cc",
> that requires that you really understand what is being hidden, which
> is a bad default (the complaint that started this discussion)
>
>  (b) the current "-p -m" is useless crazy stuff, and you'd rather use
> it for something that you actually find very common and useful
>
> If so, I agree entirely.
>
>               Linus

Re: git log fails to show all changes for a file

[Jeff King]

On Wed, Jul 15, 2015 at 11:17:50AM -0700, Junio C Hamano wrote:

> > So this is a suggested change to "-p -m" behavior?
> 
> Not really.  This is a suggested behaviour for "git log -p"; I
> wasn't very enthused by the idea to turn --cc when user said -p
> without telling them what we are doing.  In other words, if the
> users want combined, they should say --cc (and they will get a
> single-parent patch for non-merges with --cc) so there is no reason
> not to do this, as long as we fix --cc so that "git log --cc"
> implies "git log --cc -p".

Like you, I frequently use "--first-parent -m". If I understand your
proposal, a regular "git log" would have the first-parent-diff behavior
of those options, but still traverse other parents.

One oddity of that proposal is that the user ends up seeing any given
change on a side-branch _twice_. Once in the original commit that
introduced it, and once in the merge of the branch. And commit-selection
tools like "git log -Ssome_code" will select both, too, and they'll see
the merge commit along with the original. I can't decide if that's a
good thing or not.

-Peff

Re: git log fails to show all changes for a file

[Linus Torvalds]

On Wed, Jul 15, 2015 at 11:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> So this is a suggested change to "-p -m" behavior?
>
> Not really.  This is a suggested behaviour for "git log -p"

Oh, in that case I say NAK NAK NAK.

That would be totally horrible, and completely unacceptable.

I do "git log -p" all the time, and merge-diffs with --first-parent
are regularly hundreds of thousands of lines for the kernel.

So no. That is COMPLETELY unacceptable. Not even worth discussing.

                   Linus

Re: git log fails to show all changes for a file

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 15, 2015 at 11:17:50AM -0700, Junio C Hamano wrote:
>
>> > So this is a suggested change to "-p -m" behavior?
>> 
>> Not really.  This is a suggested behaviour for "git log -p"; I
>> wasn't very enthused by the idea to turn --cc when user said -p
>> without telling them what we are doing.  In other words, if the
>> users want combined, they should say --cc (and they will get a
>> single-parent patch for non-merges with --cc) so there is no reason
>> not to do this, as long as we fix --cc so that "git log --cc"
>> implies "git log --cc -p".
>
> Like you, I frequently use "--first-parent -m". If I understand your
> proposal, a regular "git log" would have the first-parent-diff behavior
> of those options, but still traverse other parents.
>
> One oddity of that proposal is that the user ends up seeing any given
> change on a side-branch _twice_. Once in the original commit that
> introduced it, and once in the merge of the branch. And commit-selection
> tools like "git log -Ssome_code" will select both, too, and they'll see
> the merge commit along with the original. I can't decide if that's a
> good thing or not.

Hmm, you are right.  That may be a problem, and Linus's makes tons
of practical sense (especially for us experienced Git users).

It just is that '-p', that clearly stands for 'patch' but does more
than 'patch' to produce something that cannot be fed to 'apply' by
defaulting to '--cc', makes me hesitate.  By making it a lot more
convenient for experienced people who understand these issues, I
have this suspicion that it would make the options less orthgonal
and harder to explain to new people.