bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch


If hg log doesn't receive a -r argument, it logs starting from the
most recent commit created anywhere ("tip").  But vc-print-log is
supposed to log starting from the working revision (".").  Those are
usually the same, but not always; the current fileset might not even
exist in "tip".  Fix this by just logging "." if no START-REVISION is
passed to vc-hg-print-log.

* lisp/vc/vc-hg.el (vc-hg-print-log): If start-revision is nil,
reliably log the working revision.

In GNU Emacs 29.2.50 (build 3, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-09-23 built on
 igm-qws-u22796a
Repository revision: 340ed90ce4518de238610461047b6c8767ca0cdc
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-tree-sitter --with-native-compilation=aot
 PKG_CONFIG_PATH=/usr/local/home/garnish/libtree-sitter/0.22.6-1/lib/pkgconfig/'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Properly-operate-on-current-fileset-revision-in-vc-h.patch --]
[-- Type: text/patch, Size: 1329 bytes --]

From 2af767a869fd9ac5b959bc5382ac09e38fc735af Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 2 Oct 2024 15:20:31 -0400
Subject: [PATCH] Properly operate on current fileset revision in
 vc-hg-print-log

If hg log doesn't receive a -r argument, it logs starting from the
most recent commit created anywhere ("tip").  But vc-print-log is
supposed to log starting from the working revision (".").  Those are
usually the same, but not always; the current fileset might not even
exist in "tip".  Fix this by just logging "." if no START-REVISION is
passed to vc-hg-print-log.

* lisp/vc/vc-hg.el (vc-hg-print-log): If start-revision is nil,
reliably log the working revision.
---
 lisp/vc/vc-hg.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index c0afb225871..58a2df3469f 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -408,8 +408,8 @@ vc-hg-print-log
     (with-current-buffer
 	buffer
       (apply #'vc-hg-command buffer 'async files "log"
+             (format "-r%s:0" (or start-revision "."))
 	     (nconc
-	      (when start-revision (list (format "-r%s:0" start-revision)))
 	      (when limit (list "-l" (format "%s" limit)))
               (when (eq vc-log-view-type 'with-diff)
                 (list "-p"))
-- 
2.39.3

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Sean Whitton]

Eli, can this go on emacs-30?

-- 
Sean Whitton

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: Spencer Baugh <sbaugh@janestreet.com>, 73604@debbugs.gnu.org
> Date: Thu, 03 Oct 2024 10:16:33 +0800
> 
> Eli, can this go on emacs-30?

Why is it important enough to install on emacs-30?  The situation
sounds quite rare to me.

Moreover, I'm not sure the change is correct in general.  If you
invoke "C-x v l" with an argument, the prompt says "...(default: last
revision)".  It says "last", and doesn't say anything about the
working revision.  So I don't understand why you say the current
operation of the command is incorrect even when the tip and the
current working revision are different.  I think this is precisely a
use case where the user should invoke the command with a prefix arg
and specify the revision from which to start.

Am I missing something?

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Sean Whitton]

Hello,

On Thu 03 Oct 2024 at 09:50am +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: Spencer Baugh <sbaugh@janestreet.com>, 73604@debbugs.gnu.org
>> Date: Thu, 03 Oct 2024 10:16:33 +0800
>>
>> Eli, can this go on emacs-30?
>
> Why is it important enough to install on emacs-30?  The situation
> sounds quite rare to me.

Yeah, it's relatively rare, but I was thinking that it would be
appropriate because of the simplicity of the fix.  Anyway, master is
fine, I just wanted to ask.

> Moreover, I'm not sure the change is correct in general.  If you
> invoke "C-x v l" with an argument, the prompt says "...(default: last
> revision)".  It says "last", and doesn't say anything about the
> working revision.  So I don't understand why you say the current
> operation of the command is incorrect even when the tip and the
> current working revision are different.  I think this is precisely a
> use case where the user should invoke the command with a prefix arg
> and specify the revision from which to start.

Well, the description of C-x v l is

    Show in another window the VC change history of the current fileset.

If the current fileset doesn't exist in tip, then we'd better not just
log tip.

Possibly the prompt should also change.  Let's see what Spencer thinks.

-- 
Sean Whitton

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Sean Whitton <spwhitton@spwhitton.name> writes:
> Hello,
>
> On Thu 03 Oct 2024 at 09:50am +03, Eli Zaretskii wrote:
>
>>> From: Sean Whitton <spwhitton@spwhitton.name>
>>> Cc: Spencer Baugh <sbaugh@janestreet.com>, 73604@debbugs.gnu.org
>>> Date: Thu, 03 Oct 2024 10:16:33 +0800
>>>
>>> Eli, can this go on emacs-30?
>>
>> Why is it important enough to install on emacs-30?  The situation
>> sounds quite rare to me.
>
> Yeah, it's relatively rare, but I was thinking that it would be
> appropriate because of the simplicity of the fix.  Anyway, master is
> fine, I just wanted to ask.

I'm in no rush to put this on emacs-30 (I've already backported it for
my users anyway)

>> Moreover, I'm not sure the change is correct in general.  If you
>> invoke "C-x v l" with an argument, the prompt says "...(default: last
>> revision)".  It says "last", and doesn't say anything about the
>> working revision.  So I don't understand why you say the current
>> operation of the command is incorrect even when the tip and the
>> current working revision are different.  I think this is precisely a
>> use case where the user should invoke the command with a prefix arg
>> and specify the revision from which to start.
>
> Well, the description of C-x v l is
>
>     Show in another window the VC change history of the current fileset.
>
> If the current fileset doesn't exist in tip, then we'd better not just
> log tip.
>
> Possibly the prompt should also change.  Let's see what Spencer thinks.

I think you both may be misinterpreting this prompt - the typed-in
revision doesn't actually specify where the log starts.  It doesn't
affect what goes in the buffer at all; the value typed in doesn't get
passed to 'print-log at all.

The prompt is just about where point goes in the buffer - "last
revision" just means that point starts at point-min.  Typing a revision
at the prompt just moves point to that revision.

(This is of course extremely confusing; I have definitely typed
revisions in the vc-print-log prompt and expected them to change the log
output.  Maybe this prompt and command should be changed in other ways,
but let's defer that for other bugs.  Also note that this is very
different behavior from M-1 C-x v L (vc-print-root-log), where typing a
revision at the prompt *does* change what's logged.)

So, anyway, that prompt says nothing about what actual logs should print
with vc-print-log.  And (potentially) including commits which don't even
contain the current fileset seems very wrong.

Note also that the print-log docs also say:

;;   If START-REVISION is given, then show the log starting from that
;;   revision ("starting" in the sense of it being the _newest_
;;   revision shown, rather than the working revision, which is normally
;;   the case).

vc-print-log always passes nil for START-REVISION, so it seems we expect
to start from the working revision.

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Sean Whitton]

Hello,

On Thu 03 Oct 2024 at 11:18am -04, Spencer Baugh wrote:

> I think you both may be misinterpreting this prompt - the typed-in
> revision doesn't actually specify where the log starts.  It doesn't
> affect what goes in the buffer at all; the value typed in doesn't get
> passed to 'print-log at all.
>
> The prompt is just about where point goes in the buffer - "last
> revision" just means that point starts at point-min.  Typing a revision
> at the prompt just moves point to that revision.
>
> (This is of course extremely confusing; I have definitely typed
> revisions in the vc-print-log prompt and expected them to change the log
> output.  Maybe this prompt and command should be changed in other ways,
> but let's defer that for other bugs.  Also note that this is very
> different behavior from M-1 C-x v L (vc-print-root-log), where typing a
> revision at the prompt *does* change what's logged.)
>
> So, anyway, that prompt says nothing about what actual logs should print
> with vc-print-log.  And (potentially) including commits which don't even
> contain the current fileset seems very wrong.
>
> Note also that the print-log docs also say:
>
> ;;   If START-REVISION is given, then show the log starting from that
> ;;   revision ("starting" in the sense of it being the _newest_
> ;;   revision shown, rather than the working revision, which is normally
> ;;   the case).
>
> vc-print-log always passes nil for START-REVISION, so it seems we expect
> to start from the working revision.

Thanks.  Based on this explanation I'd like to install your patch, but
can you move the substantive commentary in the commit message into the
code as a comment, please?  Generally that is preferred in this repo.

-- 
Sean Whitton

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Sean Whitton <spwhitton@spwhitton.name> writes:
> Hello,
>
> On Thu 03 Oct 2024 at 11:18am -04, Spencer Baugh wrote:
>
>> I think you both may be misinterpreting this prompt - the typed-in
>> revision doesn't actually specify where the log starts.  It doesn't
>> affect what goes in the buffer at all; the value typed in doesn't get
>> passed to 'print-log at all.
>>
>> The prompt is just about where point goes in the buffer - "last
>> revision" just means that point starts at point-min.  Typing a revision
>> at the prompt just moves point to that revision.
>>
>> (This is of course extremely confusing; I have definitely typed
>> revisions in the vc-print-log prompt and expected them to change the log
>> output.  Maybe this prompt and command should be changed in other ways,
>> but let's defer that for other bugs.  Also note that this is very
>> different behavior from M-1 C-x v L (vc-print-root-log), where typing a
>> revision at the prompt *does* change what's logged.)
>>
>> So, anyway, that prompt says nothing about what actual logs should print
>> with vc-print-log.  And (potentially) including commits which don't even
>> contain the current fileset seems very wrong.
>>
>> Note also that the print-log docs also say:
>>
>> ;;   If START-REVISION is given, then show the log starting from that
>> ;;   revision ("starting" in the sense of it being the _newest_
>> ;;   revision shown, rather than the working revision, which is normally
>> ;;   the case).
>>
>> vc-print-log always passes nil for START-REVISION, so it seems we expect
>> to start from the working revision.
>
> Thanks.  Based on this explanation I'd like to install your patch, but
> can you move the substantive commentary in the commit message into the
> code as a comment, please?  Generally that is preferred in this repo.

Sure - even better, I put it in the docstring.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Properly-operate-on-current-fileset-revision-in-vc-h.patch --]
[-- Type: text/x-patch, Size: 1742 bytes --]

From d353b8eef6134cd2a25e0a778d9ac9cd7fcae50f Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 2 Oct 2024 15:20:31 -0400
Subject: [PATCH] Properly operate on current fileset revision in
 vc-hg-print-log

* lisp/vc/vc-hg.el (vc-hg-print-log): If start-revision is nil,
reliably log the working revision. (bug#73604)
---
 lisp/vc/vc-hg.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index c0afb225871..677cb3fd166 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -397,8 +397,11 @@ vc-hg-log-format
 (defun vc-hg-print-log (files buffer &optional shortlog start-revision limit)
   "Print commit log associated with FILES into specified BUFFER.
 If SHORTLOG is non-nil, use a short format based on `vc-hg-root-log-format'.
-If START-REVISION is non-nil, it is the newest revision to show.
-If LIMIT is non-nil, show no more than this many entries."
+If LIMIT is non-nil, show no more than this many entries.
+
+If START-REVISION is nil, the commit log is printed starting from the
+working directory parent (revset \".\").  If START-REVISION is non-nil,
+the log is printed starting from that revision."
   ;; `vc-do-command' creates the buffer, but we need it before running
   ;; the command.
   (vc-setup-buffer buffer)
@@ -408,8 +411,8 @@ vc-hg-print-log
     (with-current-buffer
 	buffer
       (apply #'vc-hg-command buffer 'async files "log"
+             (format "-r%s:0" (or start-revision "."))
 	     (nconc
-	      (when start-revision (list (format "-r%s:0" start-revision)))
 	      (when limit (list "-l" (format "%s" limit)))
               (when (eq vc-log-view-type 'with-diff)
                 (list "-p"))
-- 
2.39.3

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Dmitry Gutov <dmitry@gutov.dev>,  Eli Zaretskii <eliz@gnu.org>,
>    73604@debbugs.gnu.org
> Date: Fri, 11 Oct 2024 16:53:29 -0400
> 
>  (defun vc-hg-print-log (files buffer &optional shortlog start-revision limit)
>    "Print commit log associated with FILES into specified BUFFER.
>  If SHORTLOG is non-nil, use a short format based on `vc-hg-root-log-format'.
> -If START-REVISION is non-nil, it is the newest revision to show.
> -If LIMIT is non-nil, show no more than this many entries."
> +If LIMIT is non-nil, show no more than this many entries.

This should say that LIMIT must be a positive integer number.

> +If START-REVISION is nil, the commit log is printed starting from the
                             ^^^^^^^^^^^^^^^^^^^^^^^^^
Passive tense alert!

> +working directory parent (revset \".\").  If START-REVISION is non-nil,
> +the log is printed starting from that revision."
       ^^^^^^^^^^^^^^
Same here.  In addition, I'm guessing START-REVISION must be a string,
right?

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Sean Whitton]

Hello,

Installed and closing -- thanks!

-- 
Sean Whitton

bug#73604: [PATCH] Properly operate on current fileset revision in vc-hg-print-log

[Sean Whitton]

Hello,

On Sat 12 Oct 2024 at 10:36am +03, Eli Zaretskii wrote:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: Dmitry Gutov <dmitry@gutov.dev>,  Eli Zaretskii <eliz@gnu.org>,
>>    73604@debbugs.gnu.org
>> Date: Fri, 11 Oct 2024 16:53:29 -0400
>>
>>  (defun vc-hg-print-log (files buffer &optional shortlog start-revision limit)
>>    "Print commit log associated with FILES into specified BUFFER.
>>  If SHORTLOG is non-nil, use a short format based on `vc-hg-root-log-format'.
>> -If START-REVISION is non-nil, it is the newest revision to show.
>> -If LIMIT is non-nil, show no more than this many entries."
>> +If LIMIT is non-nil, show no more than this many entries.
>
> This should say that LIMIT must be a positive integer number.
>
>> +If START-REVISION is nil, the commit log is printed starting from the
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^
> Passive tense alert!
>
>> +working directory parent (revset \".\").  If START-REVISION is non-nil,
>> +the log is printed starting from that revision."
>        ^^^^^^^^^^^^^^
> Same here.  In addition, I'm guessing START-REVISION must be a string,
> right?

Sorry, missed this message.  I've made these revisions.

-- 
Sean Whitton