bug#35624: log-view-diff regression

bug#35624: log-view-diff regression

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: log-view-diff-eobp.patch --]
[-- Type: text/x-diff, Size: 724 bytes --]

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

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

bug#35624: log-view-diff regression

[Juri Linkov]

> 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.

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.

bug#35624: log-view-diff regression

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: log-view-diff-message.patch --]
[-- Type: text/x-diff, Size: 886 bytes --]

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

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.

bug#35624: log-view-diff regression

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 10 May 2019 03:14:34 +0300
> Cc: 35624@debbugs.gnu.org
> 
> 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.

bug#35624: log-view-diff regression

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: log-view-diff-end-of-defun.patch --]
[-- Type: text/x-diff, Size: 772 bytes --]

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

bug#35624: log-view-diff regression

[Dmitry Gutov]

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.

bug#35624: log-view-diff regression

[Juri Linkov]

>> 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

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 13.05.2019 23:40, Juri Linkov wrote:
> Do you remember that in older versions
> there was a header in the*vc-change-log*  buffer?

Sorry, I don't. Can you find the change that removed it?

> +      (with-current-buffer buffer
> +	(insert "Working\n")

How does the result look? Just the word "Working" at the beginning of 
the buffer?

bug#35624: log-view-diff regression

[Juri Linkov]

>> Do you remember that in older versions
>> there was a header in the *vc-change-log* buffer?
>
> Sorry, I don't. Can you find the change that removed it?

I don't remember what VC backend produced such header, maybe
vc-bzr or vc-svn.  Or probably vc-cvs because CVS has the same
header as RCS, and I can easily check what headers RCS produces
because it doesn't require any configuration.  For brevity
only essential part of RCS log is left here for demonstration:

Working file:
----------------------------
revision 1.4
----------------------------
revision 1.3
----------------------------
revision 1.2
----------------------------
revision 1.1

When the beginning of the region is on the "Working file" line
in the header, and the end of the region is on a revision line,
e.g. "revision 1.4", then typing `=' displays:

  No changes between 1.4 and workfile

and if the current workfile has some changes, then differences
between 1.4 and workfile are displayed.

This means that RCS supports this nice feature, but Git doesn't.

>> +      (with-current-buffer buffer
>> +	(insert "Working\n")
>
> How does the result look? Just the word "Working" at the beginning of
> the buffer?

If the word "Working" is too ambiguous, then at least an empty line
at the beginning of the buffer will enable this feature for Git.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 14.05.2019 23:29, Juri Linkov wrote:

>> Sorry, I don't. Can you find the change that removed it?
> 
> I don't remember what VC backend produced such header, maybe
> vc-bzr or vc-svn.  Or probably vc-cvs because CVS has the same
> header as RCS, and I can easily check what headers RCS produces
> because it doesn't require any configuration.  For brevity
> only essential part of RCS log is left here for demonstration:

Thank you.

> Working file:

I guess RCS at least outputs some pertinent information here, not just 
that text, so it's less puzzling. I don't know how useful it is, though.

> ----------------------------
> revision 1.4
> ----------------------------
> revision 1.3
> ----------------------------
> revision 1.2
> ----------------------------
> revision 1.1
> 
> When the beginning of the region is on the "Working file" line
> in the header, and the end of the region is on a revision line,
> e.g. "revision 1.4", then typing `=' displays:
> 
>    No changes between 1.4 and workfile

You can also type 'C-u C-x v =', then '1.4', RET and C-j to omit the end 
version. That would also show the diff against the workfile.

> and if the current workfile has some changes, then differences
> between 1.4 and workfile are displayed.
> 
> This means that RCS supports this nice feature, but Git doesn't.

Just how nice is it, really? It seems pretty niche to me, and I don't 
remember the last time I needed something like this exactly.

>>> +      (with-current-buffer buffer
>>> +	(insert "Working\n")
>>
>> How does the result look? Just the word "Working" at the beginning of
>> the buffer?
> 
> If the word "Working" is too ambiguous, then at least an empty line
> at the beginning of the buffer will enable this feature for Git.

Should we really make the log look weirder for the sake of a feature one 
would use at most, I don't know... once a month?

Why don't we create a 'fake history' entry in vc-diff and vc-root-diff 
instead? Then the user could move point to a revision and type

   C-u C-x v = M-n RET C-j

It's not as quick maybe, but this way you also avoid having to set the 
"other' region bound.

bug#35624: log-view-diff regression

[Juri Linkov]

>> I don't remember what VC backend produced such header, maybe
>> vc-bzr or vc-svn.  Or probably vc-cvs because CVS has the same
>> header as RCS, and I can easily check what headers RCS produces
>> because it doesn't require any configuration.  For brevity
>> only essential part of RCS log is left here for demonstration:
>
> Thank you.
>
>> Working file:
>
> I guess RCS at least outputs some pertinent information here, not just that
> text, so it's less puzzling. I don't know how useful it is, though.

Yes, RCS displays a multi-line header.

>> ----------------------------
>> revision 1.4
>> ----------------------------
>> revision 1.3
>> ----------------------------
>> revision 1.2
>> ----------------------------
>> revision 1.1
>>
>> When the beginning of the region is on the "Working file" line
>> in the header, and the end of the region is on a revision line,
>> e.g. "revision 1.4", then typing `=' displays:
>>
>>    No changes between 1.4 and workfile
>
> You can also type 'C-u C-x v =', then '1.4', RET and C-j to omit the end
> version. That would also show the diff against the workfile.

10 keys more to type.

>>>> +      (with-current-buffer buffer
>>>> +	(insert "Working\n")
>>>
>>> How does the result look? Just the word "Working" at the beginning of
>>> the buffer?
>>
>> If the word "Working" is too ambiguous, then at least an empty line
>> at the beginning of the buffer will enable this feature for Git.
>
> Should we really make the log look weirder for the sake of a feature one
> would use at most, I don't know... once a month?

When this feature was enabled, I used it every day.  Currently I'm forced
to type 10 more keys every time.  I agree the log not to look weirder,
so better to not display Working.

>> and if the current workfile has some changes, then differences
>> between 1.4 and workfile are displayed.
>>
>> This means that RCS supports this nice feature, but Git doesn't.
>
> Just how nice is it, really? It seems pretty niche to me, and I don't
> remember the last time I needed something like this exactly.

Or course, you didn't think about it, because you didn't know it exists.
I didn't know too until discovered it accidentally.  After that it hard
to lose this ability.  The problem is that this useful feature is
undocumented.  Here is the patch that documents it:

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index 4986c11103..12cee5d922 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -1033,6 +1033,7 @@ VC Change Log
 Visit the revision indicated at the current line.
 
 @item d
+@itemx =
 Display a diff between the revision at point and the next earlier
 revision, for the specific file.
 
@@ -1047,6 +1048,16 @@ VC Change Log
 revision at point.
 @end table
 
+To compare two arbitrary revisions, activate the region: set the
+beginning of the region to the line with the first revision and the
+end of the region to the line with the second revision to compare,
+then type @kbd{d} or @kbd{=}.  When the beginning of the region is on
+the top line that has no revision, it uses the current work file as
+the first revision to compare.  When the end of the region is on the
+bottom non-revision line after the last revision line, then it uses
+the next earlier revision after the last displayed revision as the
+second revision to compare.
+
 @vindex vc-log-show-limit
 Because fetching many log entries can be slow, the
 @file{*vc-change-log*} buffer displays no more than 2000 revisions by
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 61c13026cc..b6feb3b8d1 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 "\n")
 	(apply 'vc-git-command buffer
 	       'async files
 	       (append

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 16.05.2019 0:12, Juri Linkov wrote:

>> You can also type 'C-u C-x v =', then '1.4', RET and C-j to omit the end
>> version. That would also show the diff against the workfile.
> 
> 10 keys more to type.

You would normally paste the revision. And 'C-x v =' is already in my 
muscle memory.

> When this feature was enabled, I used it every day.

Was it back in the days of CVS?

> Currently I'm forced
> to type 10 more keys every time.  I agree the log not to look weirder,
> so better to not display Working.

Could you explain what are you using it for? I understand a diff between 
revisions, and I understand a diff of the working tree against the 
index, but this kind of a diff seems pretty unusual.

> Or course, you didn't think about it, because you didn't know it exists.
> I didn't know too until discovered it accidentally.  After that it hard
> to lose this ability.  The problem is that this useful feature is
> undocumented.  Here is the patch that documents it:

I have tried to imagine using it, but it's still hard. Maybe doing it 
once a few days to compare the current progress against the master 
branch. Though C-u C-x v d master RET C-j might be faster that looking 
for the revision that corresponds to the master branch in the log.

> +To compare two arbitrary revisions, activate the region: set the
> +beginning of the region to the line with the first revision and the
> +end of the region to the line with the second revision to compare,
> +then type @kbd{d} or @kbd{=}.  When the beginning of the region is on
> +the top line that has no revision, it uses the current work file as
> +the first revision to compare.  When the end of the region is on the
> +bottom non-revision line after the last revision line, then it uses
> +the next earlier revision after the last displayed revision as the
> +second revision to compare.

The description is okay, but feature-wise, I'm not convinced.

Would somebody else like to express an opinion here?

>   @vindex vc-log-show-limit
>   Because fetching many log entries can be slow, the
>   @file{*vc-change-log*} buffer displays no more than 2000 revisions by
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index 61c13026cc..b6feb3b8d1 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 "\n")

Any other ideas how to reach the same functionality without making the 
log buffer weirder?

Maybe add a prefix argument handling to log-view-diff?

bug#35624: log-view-diff regression

[Juri Linkov]

>>> You can also type 'C-u C-x v =', then '1.4', RET and C-j to omit the end
>>> version. That would also show the diff against the workfile.
>>
>> 10 keys more to type.
>
> You would normally paste the revision. And 'C-x v =' is already in my
> muscle memory.

Coping and then pasting requires more keypresses.

>> When this feature was enabled, I used it every day.
>
> Was it back in the days of CVS?

Nowadays I'm using it every day on Git: first I make the log buffer
writable by typing `C-x C-q', then type `C-o' at the top of the log
buffer to insert an empty line to make this feature available.

>> Currently I'm forced
>> to type 10 more keys every time.  I agree the log not to look weirder,
>> so better to not display Working.
>
> Could you explain what are you using it for? I understand a diff between
> revisions, and I understand a diff of the working tree against the index,
> but this kind of a diff seems pretty unusual.

The last time I used this feature today: I needed to revert some
previous commits partially, and before committing the reverting changes
I needed to check if necessary changes were reverted.  To do this, I
inserted an empty line at the top and set the beginning of the region here,
then set the end of the region on the line with the revision that
started a set of related commits.  Then just typed `=' and the displayed
diff confirmed that the right hunks were reverted, showing the diff
as it should have looked like without these reverted changes.

>> Or course, you didn't think about it, because you didn't know it exists.
>> I didn't know too until discovered it accidentally.  After that it hard
>> to lose this ability.  The problem is that this useful feature is
>> undocumented.  Here is the patch that documents it:
>
> I have tried to imagine using it, but it's still hard. Maybe doing it once
> a few days to compare the current progress against the master
> branch.

Yes, I believe a try is worth a thousand words.

> Though C-u C-x v d master RET C-j might be faster that looking for
> the revision that corresponds to the master branch in the log.

The log buffer helps to select the revisions to compare
because it displays dates and summaries of commits.

bug#35624: log-view-diff regression

[Juri Linkov]

> Yes, I believe a try is worth a thousand words.

So let's try this.  Pushed to master.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 19.05.2019 23:11, Juri Linkov wrote:
>> Yes, I believe a try is worth a thousand words.
> 
> So let's try this.  Pushed to master.

Sorry, I still don't like how it looks. For the main purpose of the log 
buffer, the empty first line is weird (saying "working revision" would 
be just as weird).

I could maybe live with it if were 1-character tall (and in a terminal, 
maybe invisible until you move point to it), but I'm still not convinced 
personally. And I see no others voting on this feature.

One alternative which I have already mentioned is to make a different 
command handle this. E.g. 'C-u d' might diff workfile and the revision 
at point. This actually seems faster that go to bob and create the 
necessary region.

bug#35624: log-view-diff regression

[Juri Linkov]

> One alternative which I have already mentioned is to make a different
> command handle this. E.g. 'C-u d' might diff workfile and the revision at
> point. This actually seems faster that go to bob and create the
> necessary region.

This is not self-evident and not WYSIWYG as the visual region selection is.

> For the main purpose of the log buffer, the empty first line is weird
> (saying "working revision" would be just as weird).

vc-rcs and vc-cvs inserts 12-lines long header in the log buffer
and no one complained for several decades :)

OTOH, when recently Android developers changed the position of the clock
from the right side of the screen to the left, billions of users silently
adapted their habits to the new position without protests :(

> I could maybe live with it if were 1-character tall (and in a terminal,
> maybe invisible until you move point to it)

It's already 1-character tall.

But it you meant 1-pixel tall, then I agree it's a good idea
(like 1-pixel line is already used in log-edit.el):

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index b6feb3b8d1..238b04929b 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1018,7 +1018,7 @@ vc-git-print-log
     ;; read-only.
     (let ((inhibit-read-only t))
       (with-current-buffer buffer
-	(insert "\n")
+	(insert (propertize "\n" 'font-lock-face '(:height 0.1)))
 	(apply 'vc-git-command buffer
 	       'async files
 	       (append

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 21.05.2019 23:12, Juri Linkov wrote:
>> One alternative which I have already mentioned is to make a different
>> command handle this. E.g. 'C-u d' might diff workfile and the revision at
>> point. This actually seems faster that go to bob and create the
>> necessary region.
> 
> This is not self-evident and not WYSIWYG as the visual region selection is.

If showing diffs was the main purpose of the log buffer, I might agree 
with you.

Okay, it's not self-evident, but going above the first visible line is 
not self-evident either. For instance, most users would never on purpose 
try C-p instead of p to get to the line before the first revision, 
especially if it's 1-pixel tall like your last patch did.

Not to mention that most users would not simply think to create an 
active region before pressing 'd'.

>> For the main purpose of the log buffer, the empty first line is weird
>> (saying "working revision" would be just as weird).
> 
> vc-rcs and vc-cvs inserts 12-lines long header in the log buffer
> and no one complained for several decades :)

Guess that's because those lines show something remotely helpful and 
pertinent to the main purpose of the buffer.

> OTOH, when recently Android developers changed the position of the clock
> from the right side of the screen to the left, billions of users silently
> adapted their habits to the new position without protests :(

We're talking about adding a banana to a clock hand.

>> I could maybe live with it if were 1-character tall (and in a terminal,
>> maybe invisible until you move point to it)
> 
> It's already 1-character tall.
> 
> But it you meant 1-pixel tall, then I agree it's a good idea
> (like 1-pixel line is already used in log-edit.el):

Yes, that's what I meant, sorry.

> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index b6feb3b8d1..238b04929b 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1018,7 +1018,7 @@ vc-git-print-log
>       ;; read-only.
>       (let ((inhibit-read-only t))
>         (with-current-buffer buffer
> -	(insert "\n")
> +	(insert (propertize "\n" 'font-lock-face '(:height 0.1)))
>   	(apply 'vc-git-command buffer
>   	       'async files
>   	       (append

It's better, but a) it shifts the buffer text by 1 pixel, which I 
actually find annoying now that I look at the bottom entry that fits in 
that window, b) from your side, it should look suboptimal as well, 
because the cursor is basically invisible when it's on the top line 
(speaking of WYSIWYG).

If you really must have it this way, do we have an example of invisible 
text expanding when cursor moves inside, and then contracting when it's 
out again? Meaning if would look like an empty line you wanted after you 
press 'C-p', but not visible at all otherwise.

bug#35624: log-view-diff regression

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

> Not to mention that most users would not simply think to create an active
> region before pressing 'd'.

This feature is documented.

> It's better, but a) it shifts the buffer text by 1 pixel, which I actually
> find annoying now that I look at the bottom entry that fits in that window,

My bottom line is partially visible in all buffers anyway.

> b) from your side, it should look suboptimal as well, because the cursor is
> basically invisible when it's on the top line (speaking of WYSIWYG).

Maybe just set window-start to the first non-empty line initially,
thus moving the empty line out of screen?

> If you really must have it this way, do we have an example of invisible
> text expanding when cursor moves inside, and then contracting when it's out
> again? Meaning if would look like an empty line you wanted after you press
> 'C-p', but not visible at all otherwise.

Yes, this is possible:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-git-print-log-cursor-sensor.patch --]
[-- Type: text/x-diff, Size: 977 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index b6feb3b8d1..29acb047c6 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1018,7 +1018,11 @@ vc-git-print-log
     ;; read-only.
     (let ((inhibit-read-only t))
       (with-current-buffer buffer
-	(insert "\n")
+	(insert (propertize "\n"
+                            'invisible t
+                            'cursor-sensor-functions
+                            (list #'(lambda (_w _p dir)
+                                      (visible-mode (if (eq dir 'entered) 1 0))))))
 	(apply 'vc-git-command buffer
 	       'async files
 	       (append
@@ -1122,7 +1126,8 @@ vc-git-log-view-mode
 	     (1 'change-log-acknowledgment)
 	     (2 'change-log-acknowledgment))
 	    ("^Date:   \\(.+\\)" (1 'change-log-date))
-	    ("^summary:[ \t]+\\(.+\\)" (1 'log-view-message)))))))
+	    ("^summary:[ \t]+\\(.+\\)" (1 'log-view-message))))))
+  (cursor-sensor-mode 1))
 
 
 (defun vc-git-show-log-entry (revision)

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 22.05.2019 0:32, Juri Linkov wrote:
>> Not to mention that most users would not simply think to create an active
>> region before pressing 'd'.
> 
> This feature is documented.

Whatever new feature of 'd' would be documented as well.

>> It's better, but a) it shifts the buffer text by 1 pixel, which I actually
>> find annoying now that I look at the bottom entry that fits in that window,
> 
> My bottom line is partially visible in all buffers anyway.

It's not a huge issue, but I like the other options better.

>> b) from your side, it should look suboptimal as well, because the cursor is
>> basically invisible when it's on the top line (speaking of WYSIWYG).
> 
> Maybe just set window-start to the first non-empty line initially,
> thus moving the empty line out of screen?

Thought about this too. Could work, but the fact that it's not easy to 
"undo" the view if you pressed 'C-p' accidentally is a downside.

>> If you really must have it this way, do we have an example of invisible
>> text expanding when cursor moves inside, and then contracting when it's out
>> again? Meaning if would look like an empty line you wanted after you press
>> 'C-p', but not visible at all otherwise.
> 
> Yes, this is possible:

It almost works fine, but going from the second log entry to the first 
with 'p' while at bol leads to the first (empty) line becoming visible.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 22.05.2019 0:52, Dmitry Gutov wrote:
> It almost works fine, but going from the second log entry to the first 
> with 'p' while at bol leads to the first (empty) line becoming visible.

BTW, if we implement this, here's something we could add as well:

Instead of the empty line, how about it will contain some quick help 
string for the 'd' command, so that the user is not too puzzled if they 
hit 'C-p' by accident?

bug#35624: log-view-diff regression

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

>> Maybe just set window-start to the first non-empty line initially,
>> thus moving the empty line out of screen?
>
> Thought about this too. Could work, but the fact that it's not easy to
> "undo" the view if you pressed 'C-p' accidentally is a downside.

"undo" is a minuscule issue, or rather a non-issue at all,
whereas the idea is good.  Created a feature request bug#35860

>>> If you really must have it this way, do we have an example of invisible
>>> text expanding when cursor moves inside, and then contracting when it's out
>>> again? Meaning if would look like an empty line you wanted after you press
>>> 'C-p', but not visible at all otherwise.
>>
>> Yes, this is possible:
>
> It almost works fine, but going from the second log entry to the first with
> 'p' while at bol leads to the first (empty) line becoming visible.

Asked about this in http://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00827.html
and the answer was to use reveal-mode.

> BTW, if we implement this, here's something we could add as well:
>
> Instead of the empty line, how about it will contain some quick help string
> for the 'd' command, so that the user is not too puzzled if they hit 'C-p'
> by accident?

Good idea, added too:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-git-print-log-reveal-mode.patch --]
[-- Type: text/x-diff, Size: 936 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index b6feb3b8d1..1637f1106f 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1018,7 +1018,10 @@ vc-git-print-log
     ;; read-only.
     (let ((inhibit-read-only t))
       (with-current-buffer buffer
-	(insert "\n")
+        (overlay-put (make-overlay (point) (progn (insert (propertize "\n" 'help-echo
+          "You can use `d' on the top empty line to compare with the current working revision"))
+          (point)))
+          'invisible t)
 	(apply 'vc-git-command buffer
 	       'async files
 	       (append
@@ -1122,7 +1125,8 @@ vc-git-log-view-mode
 	     (1 'change-log-acknowledgment)
 	     (2 'change-log-acknowledgment))
 	    ("^Date:   \\(.+\\)" (1 'change-log-date))
-	    ("^summary:[ \t]+\\(.+\\)" (1 'log-view-message)))))))
+	    ("^summary:[ \t]+\\(.+\\)" (1 'log-view-message))))))
+  (reveal-mode 1))
 
 
 (defun vc-git-show-log-entry (revision)

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 24.05.2019 0:07, Juri Linkov wrote:
> -	(insert "\n")
> +        (overlay-put (make-overlay (point) (progn (insert (propertize "\n" 'help-echo
> +          "You can use `d' on the top empty line to compare with the current working revision"))
> +          (point)))
> +          'invisible t)

Is this a tested patch? It doesn't seem to work over here. 'C-p' does 
nothing.

BTW, why help-echo? We have an otherwise unoccupied line, might as well 
show the text there directly.

What do you think about this version, though:

   (Press 'd' here to see the diff against the current working revision)

?

I don't mind the words "compare with", but they seem to elicit the 
question, "compare what?" The empty line?

bug#35624: log-view-diff regression

[Juri Linkov]

>> -	(insert "\n")
>> +        (overlay-put (make-overlay (point) (progn (insert (propertize "\n" 'help-echo
>> +          "You can use `d' on the top empty line to compare with the current working revision"))
>> +          (point)))
>> +          'invisible t)
>
> Is this a tested patch? It doesn't seem to work over here. 'C-p'
> does nothing.

It's just a sketch, it doesn't work when trying.  This shows that
due to reveal-mode's glitches, this solution can't be used here.

> BTW, why help-echo? We have an otherwise unoccupied line, might as well
> show the text there directly.
>
> What do you think about this version, though:
>
>   (Press 'd' here to see the diff against the current working revision)
>
> ?

Ok, will show this text dimmed.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 24.05.2019 21:41, Juri Linkov wrote:

>> Is this a tested patch? It doesn't seem to work over here. 'C-p'
>> does nothing.
> 
> It's just a sketch, it doesn't work when trying.  This shows that
> due to reveal-mode's glitches, this solution can't be used here.

Apparently it only works with invisibilities that leave an ellipsis.

What's our next step, then?

>> What do you think about this version, though:
>>
>>    (Press 'd' here to see the diff against the current working revision)
>>
>> ?
> 
> Ok, will show this text dimmed.

Sounds good.

bug#35624: log-view-diff regression

[Juri Linkov]

>>> Is this a tested patch? It doesn't seem to work over here. 'C-p'
>>> does nothing.
>>
>> It's just a sketch, it doesn't work when trying.  This shows that
>> due to reveal-mode's glitches, this solution can't be used here.
>
> Apparently it only works with invisibilities that leave an ellipsis.
>
> What's our next step, then?

The next step is to implement 'window-start-marker' in bug#35860,
and set it to the second line, so after displaying the log buffer
the first line will be off-screen.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 27.05.2019 22:47, Juri Linkov wrote:

> The next step is to implement 'window-start-marker' in bug#35860,
> and set it to the second line, so after displaying the log buffer
> the first line will be off-screen.

OK. But since there has been no progress there in the meantime, I'm 
reverting your experiment now.

bug#35624: log-view-diff regression

[Juri Linkov]

>> The next step is to implement 'window-start-marker' in bug#35860,
>> and set it to the second line, so after displaying the log buffer
>> the first line will be off-screen.
>
> OK. But since there has been no progress there in the meantime, I'm
> reverting your experiment now.

This is not constructive.  Why to remove useful feature instead of helping
to improve it?  While it's not yet perfect, no one was disturbed by it.

What is worse, you also removed documentation updates that documented
the feature that already existed for many years.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 10.06.2019 23:50, Juri Linkov wrote:
> This is not constructive.  Why to remove useful feature instead of helping
> to improve it?

The new behavior was annoying, and there was no consensus on it.

Please don't write "let's try and see" and then act hurt then the 
feedback is negative. I never approved your patch in the first place.

 > While it's not yet perfect, no one was disturbed by it.

Obviously, I was. It felt like a sore thumb every time I called 
vc-print-log.

> What is worse, you also removed documentation updates that documented
> the feature that already existed for many years.

The documentation updates heavily relied on the new log-view behavior, I 
think. But please feel free to re-add them in any shape or form without 
the change in behavior, if you feel they are still valuable.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 27.05.2019 22:47, Juri Linkov wrote:
> The next step is to implement 'window-start-marker' in bug#35860,
> and set it to the second line, so after displaying the log buffer
> the first line will be off-screen.

Speaking about this approach... What happens when the whole log buffer 
is shorter than the window it's displayed in?

It's not a frequent occurrence is our usual work, but when someone is 
starting a small side-project, the UI should still work, and it would be 
jarring if it was suddenly different from the usual.

bug#35624: log-view-diff regression

[Juri Linkov]

>> The next step is to implement 'window-start-marker' in bug#35860,
>> and set it to the second line, so after displaying the log buffer
>> the first line will be off-screen.
>
> Speaking about this approach... What happens when the whole log buffer is
> shorter than the window it's displayed in?

It was difficult for me to test this case because I rely heavily on ediff
for development, but ediff has been broken for a week in master.
It makes sense to revert changes that break some function completely
because currently ediff is unusable giving this error:

Debugger entered--Lisp error: (void-function nil)
  nil(#("..." 39 51 (face (diff-file-header diff-header) fontified t)) (list (lambda nil (add-hook 'ediff-after-quit-hook-internal (lambda nil (if (ediff-buffer-live-p #<buffer *Ediff Session Group Panel*>) (ediff-show-meta-buffer #<buffer *Ediff Session Group Panel*> 1))) nil 'local) (setq ediff-meta-buffer #<buffer *Ediff Session Group Panel*> ediff-meta-session-number 1) (setq ediff-merge-store-file nil) (setcar '(nil nil (#("..." 39 51 ...) nil) (#<marker ...> nil) (#<marker ...> nil)) ediff-control-buffer))))
  ediff-filegroup-action()
  funcall-interactively(ediff-filegroup-action)
  call-interactively(ediff-filegroup-action nil nil)
  command-execute(ediff-filegroup-action)

But I tried to do ediff's work manually, and can confirm now that
when the whole log buffer is shorter than the window then the patch
from bug#35860 still works correctly hiding the first line initially.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 16.06.2019 23:13, Juri Linkov wrote:

>> Speaking about this approach... What happens when the whole log buffer is
>> shorter than the window it's displayed in?
> 
> It was difficult for me to test this case because I rely heavily on ediff
> for development, but ediff has been broken for a week in master.
> It makes sense to revert changes that break some function completely
> because currently ediff is unusable giving this error:

That's unfortunate, but I never use it. Maybe file a bug?

> But I tried to do ediff's work manually, and can confirm now that
> when the whole log buffer is shorter than the window then the patch
> from bug#35860 still works correctly hiding the first line initially.

Great news.

I wonder if it would still trip up people who use C-v/M-v sometimes. But 
a defcustom would solve that, anyway.

bug#35624: log-view-diff regression

[Juri Linkov]

>>> Speaking about this approach... What happens when the whole log buffer is
>>> shorter than the window it's displayed in?
>>
>> It was difficult for me to test this case because I rely heavily on ediff
>> for development, but ediff has been broken for a week in master.
>> It makes sense to revert changes that break some function completely
>> because currently ediff is unusable giving this error:
>
> That's unfortunate, but I never use it. Maybe file a bug?

There is some slow-moving discussion in bug#36157.

>> But I tried to do ediff's work manually, and can confirm now that
>> when the whole log buffer is shorter than the window then the patch
>> from bug#35860 still works correctly hiding the first line initially.
>
> Great news.
>
> I wonder if it would still trip up people who use C-v/M-v sometimes. But
> a defcustom would solve that, anyway.

C-v/M-v will help them to discover this useful feature.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 17.06.2019 23:42, Juri Linkov wrote:
> C-v/M-v will help them to discover this useful feature.

OK. I'm fine with waiting for user complaints before adding the variable.

bug#35624: log-view-diff regression

[Dmitry Gutov]

On 16.05.2019 23:04, Juri Linkov wrote:

>> Was it back in the days of CVS?
> 
> Nowadays I'm using it every day on Git: first I make the log buffer
> writable by typing `C-x C-q', then type `C-o' at the top of the log
> buffer to insert an empty line to make this feature available.

Interesting. I could probably just use Git from console for this, though.

>> Could you explain what are you using it for? I understand a diff between
>> revisions, and I understand a diff of the working tree against the index,
>> but this kind of a diff seems pretty unusual.
> 
> The last time I used this feature today: I needed to revert some
> previous commits partially, and before committing the reverting changes
> I needed to check if necessary changes were reverted.

OK. But just for the sake of the argument, here's how you could do the 
same thing:

1. You do the "partially revert" commit, then check whether it does what 
you want it to do without having to insert the extra line.
2. If it doesn't, you make the necessary extra change and do an amend 
commit (C-x v v; C-x C-e; C-c C-c), then goto 1. Otherwise end scenario.

>> I have tried to imagine using it, but it's still hard. Maybe doing it once
>> a few days to compare the current progress against the master
>> branch.
> 
> Yes, I believe a try is worth a thousand words.

I actually meant that it's hard for me to imagine really needing this. 
Or doing it more often than once in a several days.

And I do sometimes revert parts of commits, usually doing something 
other than what you explained.

>> Though C-u C-x v d master RET C-j might be faster that looking for
>> the revision that corresponds to the master branch in the log.
> 
> The log buffer helps to select the revisions to compare
> because it displays dates and summaries of commits.

How about 'C-u d', then? Like I suggested in the other email.

bug#35624: log-view-diff regression

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 08 May 2019 22:52:41 +0300
> Cc: 35624@debbugs.gnu.org
> 
> 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?

bug#35624: log-view-diff regression

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> 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.

bug#35624: log-view-diff regression

[Juri Linkov]

>> 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.

bug#35624: log-view-diff regression

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 35624@debbugs.gnu.org
> 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.

bug#35624: log-view-diff regression

[Juri Linkov]

>> 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?

bug#35624: log-view-diff regression

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 35624@debbugs.gnu.org
> 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.

bug#35624: log-view-diff regression

[Juri Linkov]

>> >> 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.

bug#35624: log-view-diff regression

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 35624@debbugs.gnu.org
> 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.

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".

bug#35624: log-view-diff regression

[Eli Zaretskii]

> Cc: 35624@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 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.