GNU bug report logs - #14989
24.3.50; log-view-diff and log-view-diff-changeset default to different `to' revisions for git merge commits

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 14989 in the body.
You can then email your comments to 14989 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3.50;
 log-view-diff and log-view-diff-changeset default to different `to'
 revisions for git merge commits
Date: Wed, 31 Jul 2013 01:42:57 +0300

To reproduce:

Have a merge commit where the second branch (merged from) has a newer
last commit than the first branch (merged to).

`vc-print-root-log' buffer would look like this:

*  (HEAD, devel)b044fba..: Some Guy 2013-07-25 foo foo
*   2152d76..: Dmitry Gutov 2013-07-25 Merge branch 'bar' into devel
|\
| *  (bar)1554943..: Another Guy 2013-07-25 Abc
| * f92770a..: Another Guy 2013-07-25 Def
* | 5f5c068..: Another Guy 2013-07-25 Ghi

If I move to commit 2152d76 and press `d', it will show the diff against
1554943. If I press `D', if will show the diff against 5f5c068, which is
*the* parent commit, as far as Git is concerned (the first parent, to be
more exact), because it belongs to the branch `devel', and the merge was
done from `bar' to `devel'.

Since this is a `vc-print-root-log' buffer, both commands will show the
full changesets, not just changes in an individual file.

But the behavior in `vc-print-log' is the same in this respect, except
the diff `d' shows is just for the current file.

`log-view-diff' behavior makes certain amount of sense (it always diffs
against the revision on the line below), but it's inconsistent with
`log-view-diff-changeset', and it also backfires when the current commit
is not a child of the commit on the previous line. Example: f92770a and
5f5c068. As a result, we get a meaningless diff.

This fix is simple enough. Apply?

=== modified file 'lisp/vc/log-view.el'
--- lisp/vc/log-view.el 2013-06-11 06:36:06 +0000
+++ lisp/vc/log-view.el 2013-07-30 22:31:40 +0000
@@ -565,10 +565,7 @@
   (let ((fr (log-view-current-tag beg))
         (to (log-view-current-tag end)))
     (when (string-equal fr to)
-      (save-excursion
-        (goto-char end)
-        (log-view-msg-next)
-        (setq to (log-view-current-tag))))
+      (setq to (vc-call-backend log-view-vc-backend 'previous-revision nil fr)))
     (vc-diff-internal
      t (list log-view-vc-backend
             (if log-view-per-file-logs

The functions could also use some further cleanup:
1) Swap the `to' and `fr' local variables.
2) Extract the function bodies to one function with a third argument
(called `whole-changeset', maybe), and make them call it.

Objections?

Message #8 received at 14989 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 14989 <at> debbugs.gnu.org
Subject: Re: bug#14989: 24.3.50;
 log-view-diff and log-view-diff-changeset default to different `to'
 revisions for git merge commits
Date: Tue, 30 Jul 2013 21:38:03 -0400

> This fix is simple enough. Apply?

Looks good, thanks.

> The functions could also use some further cleanup:
> 1) Swap the `to' and `fr' local variables.

I think that would make sense, yes.

> 2) Extract the function bodies to one function with a third argument
> (called `whole-changeset', maybe), and make them call it.

If that can be done without too much extra code, that sounds good.


        Stefan

Message #13 received at 14989-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14989-done <at> debbugs.gnu.org
Subject: Re: bug#14989: 24.3.50; log-view-diff and log-view-diff-changeset
 default to different `to' revisions for git merge commits
Date: Wed, 31 Jul 2013 15:26:45 +0300

Done!

On 31.07.2013 04:38, Stefan Monnier wrote:
>> This fix is simple enough. Apply?
>
> Looks good, thanks.
>
>> The functions could also use some further cleanup:
>> 1) Swap the `to' and `fr' local variables.
>
> I think that would make sense, yes.
>
>> 2) Extract the function bodies to one function with a third argument
>> (called `whole-changeset', maybe), and make them call it.
>
> If that can be done without too much extra code, that sounds good.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.