bug#35624: log-view-diff regression

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
bug#28466 caused a regression in 26.0.50.  Maybe it's too late to fix it
in the release branch, so the patch below is for master.

The problem is that after the change a year and a half ago
log-view-diff always falls back to the previous revision
even when point is in the middle of the log buffer,
and not after the last entry.

This patch uses the previous revision only at the end of the log buffer:


diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index e47fad8908..1f7d578610 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -618,10 +618,9 @@ log-view-diff-common
     ;; When TO and FR are the same, or when point is on a line after
     ;; the last entry, look at the previous revision.
     (when (or (string-equal fr to)
-              (>= (point)
-                  (save-excursion
-                    (goto-char (car fr-entry))
-                    (forward-line))))
+              (save-excursion
+                (goto-char end)
+                (eobp)))
       (setq fr (vc-call-backend log-view-vc-backend 'previous-revision nil fr)))
     (vc-diff-internal
      t (list log-view-vc-backend
Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Dmitry Gutov
On 08.05.2019 0:56, Juri Linkov wrote:
> bug#28466 caused a regression in 26.0.50.  Maybe it's too late to fix it
> in the release branch, so the patch below is for master.
>
> The problem is that after the change a year and a half ago
> log-view-diff always falls back to the previous revision
> even when point is in the middle of the log buffer,
> and not after the last entry.
>
> This patch uses the previous revision only at the end of the log buffer:

Hi Juri,

I think the patch should look like the one below instead. Does it fix
your problem? It also looks "obviously correct" in my opinion.

Your proposal would fail in the presence of "Show 2X entries" (when the
log is long enough).

diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index e47fad8908..e1e453115b 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -621,7 +621,8 @@ log-view-diff-common
                (>= (point)
                    (save-excursion
                      (goto-char (car fr-entry))
-                    (forward-line))))
+                    (forward-line)
+                    (point))))
        (setq fr (vc-call-backend log-view-vc-backend 'previous-revision
nil fr)))
      (vc-diff-internal
       t (list log-view-vc-backend



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Date: Wed, 08 May 2019 00:56:29 +0300
>
> bug#28466 caused a regression in 26.0.50.  Maybe it's too late to fix it
> in the release branch, so the patch below is for master.

If you want me to consider installing a change in emacs-26, please
show a reproducible recipe, because I don't think I understand the
situation where the problematic behavior happens.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
>> bug#28466 caused a regression in 26.0.50.  Maybe it's too late to fix it
>> in the release branch, so the patch below is for master.
>
> If you want me to consider installing a change in emacs-26,

I'm not sure about a new change for emacs-26, I thought rather about
reverting the previous change in emacs-26, because it is still nor clear
what the right change should be, as the comment from Dmitry indicates.
So a safer option would be just revert the previous change in emacs-26.

> please show a reproducible recipe, because I don't think I understand
> the situation where the problematic behavior happens.

Here is an illustration of the problem:

1. the case when region's beginning (b) and region's end (e)
   is on the same revision in the log-view buffer:

  * h8..: 2019-05-08 Revision h8.
be* g7..: 2019-05-07 Revision g7.
  * f6..: 2019-05-06 Revision f6.
  * e5..: 2019-05-05 Revision e5.

compares the revision f6 (the previous revision of g7) and g7.
This behavior was unchanged by the bug#28466.

2. This case demonstrates the behavior
   BEFORE the change in bug#28466:

  * h8..: 2019-05-08 Revision h8.
b * g7..: 2019-05-07 Revision g7.
e * f6..: 2019-05-06 Revision f6.
  * e5..: 2019-05-05 Revision e5.

compared the revision f6 (from region's end) and g7 (from region's beginning).
This was the correct behavior.

3. Now this case demonstrates the incorrect behavior
   AFTER the change in bug#28466:

  * h8..: 2019-05-08 Revision h8.
b * g7..: 2019-05-07 Revision g7.
e * f6..: 2019-05-06 Revision f6.
  * e5..: 2019-05-05 Revision e5.

compares the revision e5 (the previous revision of the revision at region's end)
and g7 (from region's beginning).

4. This demonstrates the case that the change in bug#28466
   was intended to fix:

  * h8..: 2019-05-08 Revision h8.
b * g7..: 2019-05-07 Revision g7.
  * f6..: 2019-05-06 Revision f6.
  * e5..: 2019-05-05 Revision e5.
e

When region's end is after the last visible revision,
it should compare g7 (from region's beginning)
with the previous revision of the last revision e5
(a hypothetical revision d4, not visible in the buffer).

Before the fix, it compared e5 and g7, that was wrong.
But the fix broke the case when region's end is in the middle
of the buffer.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
In reply to this post by Dmitry Gutov
> I think the patch should look like the one below instead. Does it fix your
> problem? It also looks "obviously correct" in my opinion.

This is exactly what was my initial thought, but this is a wrong fix,
as I realized later.

> diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
> index e47fad8908..e1e453115b 100644
> --- a/lisp/vc/log-view.el
> +++ b/lisp/vc/log-view.el
> @@ -621,7 +621,8 @@ log-view-diff-common
>                (>= (point)
>                    (save-excursion
>                      (goto-char (car fr-entry))
> -                    (forward-line))))
> +                    (forward-line)
> +                    (point))))
>        (setq fr (vc-call-backend log-view-vc-backend 'previous-revision nil fr)))
>      (vc-diff-internal
>       t (list log-view-vc-backend

This patch doesn't check if the region's end is after the last revision,
also fails if the summary line is expanded to multiline revision's header/body.

> Your proposal would fail in the presence of "Show 2X entries" (when the log
> is long enough).

Yes, I know my previous patch is not perfect, I also tried

  (not (re-search-forward log-view-message-re nil t))

but it seems this is impossible to do because currently
log-view.el doesn't support the notion of the end of
the last revision expanded body.  For example,

1. with the expanded last visible revision

  * h8..: 2019-05-08 Revision h8.
b * g7..: 2019-05-07 Revision g7.
  * f6..: 2019-05-06 Revision f6.
  * e5..: 2019-05-05 Revision e5.
  commit e5
  Date:   2019-05-05
  first line
  second line
e third line

should compare e5 and g7

2. but when region's end (e) is after the last line
   of the last expanded revision:

  * h8..: 2019-05-08 Revision h8.
b * g7..: 2019-05-07 Revision g7.
  * f6..: 2019-05-06 Revision f6.
  * e5..: 2019-05-05 Revision e5.
  commit e5
  Date:   2019-05-05
  first line
  second line
  third line
e

should compare d4 (a previous revision of the last revision) and g7.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Dmitry Gutov
On 08.05.2019 22:52, Juri Linkov wrote:

> This is exactly what was my initial thought, but this is a wrong fix,
> as I realized later.

Okay, let's work on it.

I do want to just commit that patch first, since it was obviously the
idea behind the previous change.

>> diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
>> index e47fad8908..e1e453115b 100644
>> --- a/lisp/vc/log-view.el
>> +++ b/lisp/vc/log-view.el
>> @@ -621,7 +621,8 @@ log-view-diff-common
>>                 (>= (point)
>>                     (save-excursion
>>                       (goto-char (car fr-entry))
>> -                    (forward-line))))
>> +                    (forward-line)
>> +                    (point))))
>>         (setq fr (vc-call-backend log-view-vc-backend 'previous-revision nil fr)))
>>       (vc-diff-internal
>>        t (list log-view-vc-backend
>
> This patch doesn't check if the region's end is after the last revision,

What do you mean it doesn't? That's the whole purpose of the comparison
(the part of the function the diff above changes).

> also fails if the summary line is expanded to multiline revision's header/body.

Isn't it the only situation where it fails?

I wouldn't say it's a hugely important case. The whole approach becomes
iffy once the lower bound position is *inside* the revision entry.

Should it be the lower bound? Should the changes be included in the
diff? I could never be sure without looking at the docstring.

>> Your proposal would fail in the presence of "Show 2X entries" (when the log
>> is long enough).
>
> Yes, I know my previous patch is not perfect, I also tried
>
>    (not (re-search-forward log-view-message-re nil t))
>
> but it seems this is impossible to do because currently
> log-view.el doesn't support the notion of the end of
> the last revision expanded body.

The other option is check whether all lines between the point and EOB
are either empty of start with "Show 2X entries". Which is a design I
don't particularly like, but it could serve your goal.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
found 35624 26.1
tags 35624 + patch
quit

>> This is exactly what was my initial thought, but this is a wrong fix,
>> as I realized later.
>
> Okay, let's work on it.
>
> I do want to just commit that patch first, since it was obviously the idea
> behind the previous change.

Since it's wrong in any case, better to revert it altogether in emacs-26.

>> I also tried
>>
>>    (not (re-search-forward log-view-message-re nil t))
>>
>> but it seems this is impossible to do because currently
>> log-view.el doesn't support the notion of the end of
>> the last revision expanded body.
>
> The other option is check whether all lines between the point and EOB are
> either empty of start with "Show 2X entries". Which is a design I don't
> particularly like, but it could serve your goal.
Actually there is the much needed function log-view-inside-comment-p
for expanded comments, and this patch correctly handles all cases:


diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index e47fad8908..f7aeacc273 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -618,10 +618,11 @@ log-view-diff-common
     ;; When TO and FR are the same, or when point is on a line after
     ;; the last entry, look at the previous revision.
     (when (or (string-equal fr to)
-              (>= (point)
-                  (save-excursion
-                    (goto-char (car fr-entry))
-                    (forward-line))))
+              (and (not (log-view-inside-comment-p end))
+                   (save-excursion
+                     (goto-char end)
+                     (beginning-of-line)
+                     (not (re-search-forward log-view-message-re nil t)))))
       (setq fr (vc-call-backend log-view-vc-backend 'previous-revision nil fr)))
     (vc-diff-internal
      t (list log-view-vc-backend
Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Dmitry Gutov
On 09.05.2019 22:41, Juri Linkov wrote:

> Since it's wrong in any case, better to revert it altogether in emacs-26.

I kind of think that the feature the last change added is more valuable
than the regression (especially if the regression is only triggered when
a commit message is expanded).

> Actually there is the much needed function log-view-inside-comment-p
> for expanded comments, and this patch correctly handles all cases:

Great! The patch LGTM.

Note that we could write a smaller change using that function, if the
goal is to produce as-obvious-as-possible change for emacs-26.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Cc: [hidden email]
> Date: Wed, 08 May 2019 22:52:34 +0300
>
> 1. the case when region's beginning (b) and region's end (e)
>    is on the same revision in the log-view buffer:
>
>   * h8..: 2019-05-08 Revision h8.
> be* g7..: 2019-05-07 Revision g7.
>   * f6..: 2019-05-06 Revision f6.
>   * e5..: 2019-05-05 Revision e5.
>
> compares the revision f6 (the previous revision of g7) and g7.
> This behavior was unchanged by the bug#28466.
>
> 2. This case demonstrates the behavior
>    BEFORE the change in bug#28466:
>
>   * h8..: 2019-05-08 Revision h8.
> b * g7..: 2019-05-07 Revision g7.
> e * f6..: 2019-05-06 Revision f6.
>   * e5..: 2019-05-05 Revision e5.
>
> compared the revision f6 (from region's end) and g7 (from region's beginning).
> This was the correct behavior.
>
> 3. Now this case demonstrates the incorrect behavior
>    AFTER the change in bug#28466:
>
>   * h8..: 2019-05-08 Revision h8.
> b * g7..: 2019-05-07 Revision g7.
> e * f6..: 2019-05-06 Revision f6.
>   * e5..: 2019-05-05 Revision e5.
>
> compares the revision e5 (the previous revision of the revision at region's end)
> and g7 (from region's beginning).

What does "e" signify in these examples?  Does it mean the line
indicated with "e" is part of the region?  If so, then the current
behavior (item 3) looks correct to me, and previous behavior (item 2)
looks INcorrect.  The '=' command should show the combined effect of
all the changes included in the region, so it should compare the last
revision with the one before the first.  This is also what I see with
Emacs 26.2 on my system.

Am I missing something?

> But the fix broke the case when region's end is in the middle
> of the buffer.

I don't yet see how it breaks anything, sorry.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Date: Wed, 08 May 2019 22:52:41 +0300
> Cc: [hidden email]
>
> 1. with the expanded last visible revision
>
>   * h8..: 2019-05-08 Revision h8.
> b * g7..: 2019-05-07 Revision g7.
>   * f6..: 2019-05-06 Revision f6.
>   * e5..: 2019-05-05 Revision e5.
>   commit e5
>   Date:   2019-05-05
>   first line
>   second line
> e third line
>
> should compare e5 and g7
>
> 2. but when region's end (e) is after the last line
>    of the last expanded revision:
>
>   * h8..: 2019-05-08 Revision h8.
> b * g7..: 2019-05-07 Revision g7.
>   * f6..: 2019-05-06 Revision f6.
>   * e5..: 2019-05-05 Revision e5.
>   commit e5
>   Date:   2019-05-05
>   first line
>   second line
>   third line
> e
>
> should compare d4 (a previous revision of the last revision) and g7.

You seem to be assigning some meaning to the case where the region
includes only part of a revision's display.  Why is that meaning
useful?  From my POV, it just increases the probability of user errors
if they position the region end inaccurately.  Why not consider a
revision included if even some part of it is in the region, let alone
its header line?



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Eli Zaretskii
In reply to this post by Dmitry Gutov
> From: Dmitry Gutov <[hidden email]>
> Date: Fri, 10 May 2019 03:14:34 +0300
> Cc: [hidden email]
>
> On 09.05.2019 22:41, Juri Linkov wrote:
>
> > Since it's wrong in any case, better to revert it altogether in emacs-26.
>
> I kind of think that the feature the last change added is more valuable
> than the regression (especially if the regression is only triggered when
> a commit message is expanded).

And I don't yet see any regression at all, so I'm definitely against
reverting on the emacs-26 branch.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
In reply to this post by Eli Zaretskii
>> 1. the case when region's beginning (b) and region's end (e)
>
> What does "e" signify in these examples?

"e" signifies the end of the region.

> The '=' command should show the combined effect of all the changes
> included in the region, so it should compare the last revision with
> the one before the first.
>
> Am I missing something?

If the beginning of the region is on the line with revision g7
and the end of the region is on the line with revision f6,
do you think it should show differences between g7 and f6?



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
In reply to this post by Dmitry Gutov
> I kind of think that the feature the last change added is more valuable
> than the regression (especially if the regression is only triggered when
> a commit message is expanded).

It is triggered not only when a commit message is expanded.
Also it is triggered on single-file logs.  Admittedly, my previous patch
didn't take this into account.  Here is a new patch that works also
for single-file logs.  It relies on the function 'log-view-end-of-defun'
that takes care about the "Show 2X entries" footer:


diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index e47fad8908..3389264ce6 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -618,10 +618,11 @@ log-view-diff-common
     ;; When TO and FR are the same, or when point is on a line after
     ;; the last entry, look at the previous revision.
     (when (or (string-equal fr to)
-              (>= (point)
+              (>= end
                   (save-excursion
-                    (goto-char (car fr-entry))
-                    (forward-line))))
+                    (goto-char end)
+                    (log-view-end-of-defun)
+                    (point))))
       (setq fr (vc-call-backend log-view-vc-backend 'previous-revision nil fr)))
     (vc-diff-internal
      t (list log-view-vc-backend
Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Cc: [hidden email]
> Date: Sat, 11 May 2019 23:53:44 +0300
>
> >> 1. the case when region's beginning (b) and region's end (e)
> >
> > What does "e" signify in these examples?
>
> "e" signifies the end of the region.

I asked a detailed question, but your answer doesn't make me wiser
about what "e" means.  Could you please humor me with a more detailed
answer?

> If the beginning of the region is on the line with revision g7
> and the end of the region is on the line with revision f6,
> do you think it should show differences between g7 and f6?

Depends on what "e" signifies.  It could be.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
>> >> 1. the case when region's beginning (b) and region's end (e)
>> >
>> > What does "e" signify in these examples?
>>
>> "e" signifies the end of the region.
>
> I asked a detailed question, but your answer doesn't make me wiser
> about what "e" means.  Could you please humor me with a more detailed
> answer?

Let me describe how log-view-diff was supposed to work since its introduction
~20 years ago until its behavior was changed accidentally one and half year ago:
when the user puts the beginning of the region on the line with one revision,
and the end of the region on the line with another revision, the function
'log-view-diff-common' extracts the 'from' revision from the line
with the first revision, and the 'to' revision from the line
with the second revision.

bug#28466 tried to improve the logic when the end of the region is
at the end of the buffer, but mistakenly broke long-established behavior,
so currently 'from' falls thru to its previous revision.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Dmitry Gutov
In reply to this post by Juri Linkov-2
On 11.05.2019 23:58, Juri Linkov wrote:
> Here is a new patch that works also
> for single-file logs.  It relies on the function 'log-view-end-of-defun'
> that takes care about the "Show 2X entries" footer:

Looks good. Please install when you're comfortable with it.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Cc: [hidden email]
> Date: Sun, 12 May 2019 22:15:20 +0300
>
> > I asked a detailed question, but your answer doesn't make me wiser
> > about what "e" means.  Could you please humor me with a more detailed
> > answer?
>
> Let me describe how log-view-diff was supposed to work since its introduction
> ~20 years ago until its behavior was changed accidentally one and half year ago:
> when the user puts the beginning of the region on the line with one revision,
> and the end of the region on the line with another revision, the function
> 'log-view-diff-common' extracts the 'from' revision from the line
> with the first revision, and the 'to' revision from the line
> with the second revision.

Then the fix in bug#28466 makes no sense, because it treats the case
when the region ends on the last revision specially.

But I feel that I've exhausted my grace in trying to make sense out of
this, so I will now shut up.

> bug#28466 tried to improve the logic when the end of the region is
> at the end of the buffer

Treating EOB specially makes no sense to me, FWIW.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Dmitry Gutov
On 13.05.2019 17:24, Eli Zaretskii wrote:
> Then the fix in bug#28466 makes no sense, because it treats the case
> when the region ends on the last revision specially.

It's attempting to specially treat the case when the region ends *after*
the last revision. To diff against the preceding one, because it's
impossible to do otherwise in the "outgoing log".



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Eli Zaretskii
> Cc: [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Mon, 13 May 2019 17:44:16 +0300
>
> On 13.05.2019 17:24, Eli Zaretskii wrote:
> > Then the fix in bug#28466 makes no sense, because it treats the case
> > when the region ends on the last revision specially.
>
> It's attempting to specially treat the case when the region ends *after*
> the last revision.

Yes, and that's what makes no sense.  There's no "after", the region
ends in the last revision in the buffer, by definition.

Anyway, I already said that I will shut up.



Reply | Threaded
Open this post in threaded view
|

bug#35624: log-view-diff regression

Juri Linkov-2
In reply to this post by Juri Linkov-2
>> Here is a new patch that works also for single-file logs.  It relies
>> on the function 'log-view-end-of-defun' that takes care about the
>> "Show 2X entries" footer:
>
> Looks good. Please install when you're comfortable with it.

Pushed to master.

But there is another regression.  Do you remember that in older versions
there was a header in the *vc-change-log* buffer?  It was very useful to
put the beginning of the region on the first non-revision line in the header
to compare the current working revision with the last committed revision.

Now this feature is gone, and the first line can't be used to compare
with the working revision because now the first line contains the last
committed revision.

This patch restores this useful feature:

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 192e6cf68f..040b0832be 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1017,8 +1017,8 @@ vc-git-print-log
     ;; If the buffer exists from a previous invocation it might be
     ;; read-only.
     (let ((inhibit-read-only t))
-      (with-current-buffer
-          buffer
+      (with-current-buffer buffer
+ (insert "Working\n")
  (apply 'vc-git-command buffer
        'async files
        (append




12