GNU bug report logs - #61071
New features: VC timemachine and BackupOnSave to RCS

Previous Next

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

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

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

From: John Yates <john <at> yates-sheets.org>
To: submit <at> debbugs.gnu.org
Subject: New features: VC timemachine and BackupOnSave to RCS
Date: Wed, 25 Jan 2023 22:24:30 -0500

[Message part 1 (text/plain, inline)]

Package: emacs
Version: 30.0.50
Tags: patch

Please see the cover letter.

[0002-Introduce-VC-timemachine-capability.patch (text/x-patch, attachment)]

[0003-Introduce-vc-bos-backup-on-save-to-an-RCS-file.patch (text/x-patch, attachment)]

[0000-cover-letter.patch (text/x-patch, attachment)]

[0001-Refactor-and-document-vc-find-revision-caching.patch (text/x-patch, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: John Yates <john <at> yates-sheets.org>
Cc: 61071 <at> debbugs.gnu.org
Subject: Re: bug#61071: New features: VC timemachine and BackupOnSave to RCS
Date: Sat, 11 Feb 2023 18:02:22 -0500

Sorry about the ridiculous delay.

The general functionality sounds great to me.
Some comments below:

> -(defcustom vc-find-revision-no-save nil
> -  "If non-nil, `vc-find-revision' doesn't write the created buffer to file."
> +(defcustom vc-find-revision-cache nil
> +  "When non-nil, `vc-find-revision' caches a local copy of returned revision."
>    :type 'boolean
> -  :version "27.1")
> +  :version "30.1")

This throws away `vc-find-revision-no-save` without first marking it
obsolete.  I suggest you keep `vc-find-revision-no-save` (and maybe
provide an alias like `vc-find-revision-no-cache`).  It shouldn't affect
the rest of your code much, and it will preserve compatibility with
users's settings.

Whether the default value should stay nil or become t is a separate
question and in this respect I agree with your patch that the default
should be changed.

> +(defcustom vc-cache-root nil
> +  "If non-nil, the root of a tree of cached revisions (no trailing '/').
> +
> +When `vc-find-revision-cache' is non-nil, if `vc-cache-root' is nil then the
> +cached revision will be a sibling of its working file, otherwise the cached
> +revision will be saved to a mirror path beneath `vc-cache-root.'
> +
> +To use `vc-bos-mode', `vc-cache-root' must include a /RCS component."
> +  :type 'string
> +  :version "30.1")

At this point in the patch sequence, `vc-bos-mode` doesn't exist yet.

> +(defvar-local vc-tm--revision nil
> +  "Convey a revision buffer's VCS specific unique revision id to VC-TM." )
> +(put 'vc-tm--revision 'permanent-local t)

What's "VC-TM"?  The `vc-tm` prefix is not used anywhere else, so it'd
be better not to use it.  How 'bout `vc--revbuf-revision` and just
document the info it holds rather than the intended use of that info
when the code was written?
Or otherwise, wait until the next patch to introduce this var.

> +         (parent (or buffer (get-file-buffer file) (current-buffer)))
> +         (revd-file (vc-version-backup-file-name file revision 'manual))
> +         (true-dir (file-name-directory file))
> +         (true-name (file-name-nondirectory file))
> +         (save-dir (concat vc-cache-root true-dir))

Please add a comment explaining why `expand-file-name` would not
be right.

> +         (revd-name (file-name-nondirectory revd-file))
> +         (save-file (concat vc-cache-root revd-file))
> +         ;; Some hooks assume that buffer-file-name associates a buffer with
> +         ;; a true file.  This mapping is widely assumed to be one-to-one.
> +         ;; To avoid running afoul of that assumption this fictitious path
> +         ;; is expected to be unique (bug#39190).  This path also has the

The GNU convention is to call those things "file names" rather than
"paths", because "path" is only used to mean a list of directories, as
in `load-path`.

> +         ;; virtue that it exhibits the same file type (extension) as FILE.
> +         ;; This improves setting the buffers modes.
> +         (pretend (concat true-dir "PRETNED/" true-name))

The old code just used `file` for `buffer-file-name` during
`set-auto-mode`.  I believe it was safer.
Why do you need this "PRETNED/", it seems to be a change unrelated to
the rest of the patch.

> +            ;; Prep revbuf in case it is being reused.
> +            (setq buffer-file-name nil) ; Cancel any prior file visitation
> +            (setq vc-parent-buffer nil)
> +            (setq vc-tm--revision nil)
> +            (setq buffer-read-only nil)

Why not set them directly to their intended value?

> +             ;; A cached file is viable IFF it is not writable.
> +             ((and (file-exists-p save-file) (not (file-writable-p save-file)))
> +              (insert-file-contents save-file t))

Rather than repeat what the code tests, the comment should explain why
(you think) it needs to be "not writable" to be viable.

> +              ;; Backend's output was read with 'no-conversions; do the same for write
> +              (setq buffer-file-coding-system 'no-conversion)
> +              (write-region nil nil save-file)

Why `setq` rather than let-binding?

Also, I can't see where in the new code you do the decoding which the
old code does with (decode-coding-inserted-region (point-min)
(point-max) file)?

> +              (set-file-modes save-file (logand (file-modes save-file) #o7555)))

There can be circumstances where a file is always writable, no
matter how hard we try to use `set-file-modes`.

> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index 7689d5f879..1f45aa7e96 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -82,6 +82,9 @@
>  ;; - annotate-time ()                              OK
>  ;; - annotate-current-time ()                      NOT NEEDED
>  ;; - annotate-extract-revision-at-line ()          OK
> +;; TIMEMACHINE
> +;; * tm-revisions (file)
> +;; * tm-map-line (file from-revision from-line to-revision from-is-older)
>  ;; TAG/BRANCH SYSTEM
>  ;; - create-tag (dir name branchp)                 OK
>  ;; - retrieve-tag (dir name update)                OK

Any specific reason you used `*` rather than `-`?
[ I have no objection to this choice, just curious.  ]

> @@ -101,6 +104,8 @@
>  
>  (require 'cl-lib)
>  (require 'vc-dispatcher)
> +(require 'transient)
> +(require 'vc-timemachine)
>  (eval-when-compile
>    (require 'subr-x) ; for string-trim-right
>    (require 'vc)

Are these really indispensable here?

`vc-git` will be loaded into many more sessions than those where
`transient` and `vc-timemachine` will be used, so it would be *much*
better if those packages could be loaded more lazily here.

> @@ -166,6 +171,12 @@ vc-git-program
>    :version "24.1"
>    :type 'string)
>  
> +(defcustom vc-git-global-git-arguments
> +  '("-c" "log.showSignature=false" "--no-pager")
> +  "Common arguments for all git commands."
> +  :type 'list
> +  :group 'vc-timemachine)

Why is this in the `vc-timemachine` group?
Why do we need to add those args to all the Git commands?

> -    (vc-git-command
> -     buffer 0
> -     nil
> -     "cat-file" "blob" (concat (if rev rev "HEAD") ":" fullname))))
> +    (ignore-errors
> +      (vc-git-command
> +       buffer 0
> +       nil
> +       "cat-file" "blob" (concat (if rev rev "HEAD") ":" fullname)))))

Please add a comment here explaining why we need `ignore-errors` (you
can probably just move the text you currently have in the commit
message: in many cases it's better to put those comments in the code
rather than in the commit message).

Also, if you can use `ignore-error` instead, it would be better.

> +        (setq new-date (vc-tm-format-date (match-string 2 line)))

How important is it to use the `vc-tm-date-format`?
I'm not super happy about the current design in this regard.  I think it
would make more sense to define `tm-revisions` as returning dates in the
"cheapest" format possible (the one that takes least effort), e.g.
as an ELisp time value (e.g. as returned by `date-to-time`) or as
"any format that `date-to-time` understands"?
Then the call to `vc-tm-date-format` can be moved to
`vc-timemachine.el`.

> -(eval-when-compile (require 'cl-lib))
> +(eval-when-compile
> +  (require 'cl-lib))

Why?
[ It's likely a question of taste, so I' recommend not to touch it.
  Of course, I noticed it because my taste prefers the current format
  (because in `outline-minor-mode` the `require` is otherwise hidden
  for no benefit).  ]

> @@ -41,6 +45,7 @@
>    (require 'cl-lib)
>    (require 'vc))
>  (require 'log-view)
> +(require 'vc-timemachine)

Same comment as for `vc-git`.

> +        (setq line (buffer-substring-no-properties (line-beginning-position) (line-end-position)))

Please try very hard to always stay within 80 columns.

> +(defgroup vc-timemachine nil
> +  "Time-machine functionality for VC backends."
> +  :group 'vc
> +  :version "30.1")
> +
> +(defcustom vc-tm-date-format
> +  "%a %I:%M %p %Y-%m-%d"
> +  "Revision creation date format (emphasis on easy date comparison)."
> +  :type 'string
> +  :group 'vc-timemachine
> +  :version "30.1")

The `:group` arg here is redundant (`defcustom` would use that group by
default here anyway).  Same for the subsequent `defcustom`s and `defface`s.

> +(defvar-local vc--time-machine nil
> +  "Cache a TM hint on various buffers.")
> +(put 'vc--time-machine 'permanent-local t)

The docstring seems of very little as it stands.  You could just as well
remove it (tho it'd be better to actually describe what this var should
hold, of course :-).

> +(defvar-local tmbuf--abs-file nil
> +  "Absolute path to file being traversed by this time-machine.")

"path" => "file name".

Also, please use the "vc-" prefix.  Same for the other "tmbuf-" vars.

> +(defvar-local tmbuf--branch-index nil
> +  "Zero-base index into tmbuf--branch-revisions.")
> +(put 'tmbuf--branch-revisions 'permanent-local t)
> +(defvar-local tmbuf--branch-revisions nil
> +  "When non-nil, a vector of revision-info lists.")
> +(put 'tmbuf--branch-revisions 'permanent-local t)

You make `tmbuf--branch-revisions` permanent twice (the other should
probably be for `tmbuf--branch-index`, right?).

> +(defvar-local tmbuf--source-buffer nil
> +  "A non-time-machine buffer for which this time-machine was created.")
> +(put 'tmbuf--source-buffer 'permanent-local t)

Any reason we can't use `vc-parent-buffer`?

> +(defun vc-tm--time-machine ()
> +  "Return a valid time-machine for the current buffer."

Indicate how it's returned.  I.e. either by changing current-buffer, or
by returning a buffer object (in which case it should *not* change
current-buffer).

> +        (when vc-tm-echo-area
> +          (vc-tm--show-echo-area-details new-revision-info))))
> +      (vc-tm--erm-workaround))))

Please avoid this `vc-tm--erm-workaround` here.  Better add a "generic
hook" (i.e. a hook whose meaning makes sense independently from this ERM
problem), and then arrange for some code somewhere (probably best if
it's in `enh-ruby-mode`) to add a function to this hook.

> +(declare-function erm-reset-buffer "ext:enh-ruby-mode")
> +
> +(defun vc-tm--erm-workaround ()
> +  "Workaround for enhanced ruby mode not detecting revision change."
> +  (when (eq major-mode 'enh-ruby-mode)
> +    (ignore-errors (erm-reset-buffer))))

Prefer `derived-mode-p`.  Add a comment explaining the problem or
pointing to info about it.
Arrange to try and make this unnecessary in the future (i.e. open a bug
report with the enh-ruby-mode guys?  Or fix the source of the bug if
it's elsewhere in Emacs itself?).

> +;; - tm-map-line (rel-file from-revision from-line to-revision from-is-older)
> +;;
> +;;   Return TO-REVISION's line corresponding to FROM-REVISION's FROM-LINE.
> +;;   FROM-REVISION and TO-REVISION are guaranteed distinct.  FROM-IS-OLDER
> +;;   indicates relative temporal ordering of FROM-REVISION and TO-REVISION
> +;;   on the branch.

Please clarify that you're talking about line *numbers* (I thought at
first you were talking about some completely different notion of lines,
such as "product line" or "history line").

OK, I'll stop here for now.


        Stefan

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

From: stefankangas <at> gmail.com
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 61071 <at> debbugs.gnu.org, John Yates <john <at> yates-sheets.org>
Subject: Re: bug#61071: New features: VC timemachine and BackupOnSave to RCS
Date: Mon, 4 Sep 2023 12:47:58 -0700

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> Sorry about the ridiculous delay.
>
> The general functionality sounds great to me.
> Some comments below:

John, did you make any progress with this patch?

Thanks in advance.

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

From: John Yates <john <at> yates-sheets.org>
To: Stefan Kangas <stefankangas <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 61071 <at> debbugs.gnu.org
Subject: Re: bug#61071: New features: VC timemachine and BackupOnSave to RCS
Date: Mon, 11 Sep 2023 09:04:34 -0400

Stefan Kangas,

Personal and family issues have arisen that make me quite unsure of
when I will be able to return to this activity.

I did address just about all of Stefan Monnier's feedback (see
appended, never sent reply below).

Currently the code is on a scratch/backup-on-save-to-rcs branch in my
local repository.  I tried unsuccessfully to push it to Savannah:

|   jyates <at> envy:~/repos/emacs.sv
|   $ git push -u origin scratch/backup-on-save-to-rcs
|   fatal: remote error: access denied or repository not exported: /emacs.git

I was under the impression that no special permissions are needed to
push a scratch branch.  Am I doing something wrong?

In my testing, the code works nicely.  My current sense of things that
could be improved are:
- Finding the proper commit in a series whose only metadata is the
commit timestamp is sub-optimal.
- It might be nice to have some kind of cron-based clean-up or commit squashing

/john

=============================

Hi Stefan,

Thank you so much for this clearly deep review.  Responses inline.

If nothing else, please see the places labeled 'QUESTION:'.

Still TODO:
- vc-find-revision: sort out whether PRETEND is really needed
- vc-find-revision: try to reduce ignore-errors to ignore-error
- vc-tm-revision-next: support prefix arg for count
- vc-tm-revision-previous: support prefix arg for count
- once vc-timemachine.el is available on master request an
  enh-ruby-mode enhancement is still needed (MELPA)

/john

On Sat, Feb 11, 2023 at 6:02 PM Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:
>
> Sorry about the ridiculous delay.

No harm.  I too am regularly slow to follow up.  To whit, look how long
it has taken me to act fully on your review.  (Life has a way of intervening,
but that is only a partial excuse.)

> > -(defcustom vc-find-revision-no-save nil
> > -  "If non-nil, `vc-find-revision' doesn't write the created buffer to file."
> > +(defcustom vc-find-revision-cache nil
> > +  "When non-nil, `vc-find-revision' caches a local copy of returned revision."
> >    :type 'boolean
> > -  :version "27.1")
> > +  :version "30.1")
>
> This throws away `vc-find-revision-no-save` without first marking it
> obsolete.  I suggest you keep `vc-find-revision-no-save` (and maybe
> provide an alias like `vc-find-revision-no-cache`).  It shouldn't affect
> the rest of your code much, and it will preserve compatibility with
> users's settings.
>
> Whether the default value should stay nil or become t is a separate
> question and in this respect I agree with your patch that the default
> should be changed.

It turned out easiest just to revert to the original -no-save option.
So nothing to obsolete.  But, I did change the default.

> > +(defcustom vc-cache-root nil
> > +  "If non-nil, the root of a tree of cached revisions (no trailing '/').
> > +
> > +When `vc-find-revision-cache' is non-nil, if `vc-cache-root' is nil then the
> > +cached revision will be a sibling of its working file, otherwise the cached
> > +revision will be saved to a mirror path beneath `vc-cache-root.'
> > +
> > +To use `vc-bos-mode', `vc-cache-root' must include a /RCS component."
> > +  :type 'string
> > +  :version "30.1")
>
> At this point in the patch sequence, `vc-bos-mode` doesn't exist yet.

That last documentation line now gets added in the vc-bos patch.

> > +(defvar-local vc-tm--revision nil
> > +  "Convey a revision buffer's VCS specific unique revision id to VC-TM." )
> > +(put 'vc-tm--revision 'permanent-local t)
>
> What's "VC-TM"?  The `vc-tm` prefix is not used anywhere else, so it'd
> be better not to use it.  How 'bout `vc--revbuf-revision` and just
> document the info it holds rather than the intended use of that info
> when the code was written?
> Or otherwise, wait until the next patch to introduce this var.

|    (defvar-local vc--revbuf-revision nil
|      "Remember a revision buffer's VCS-specific unique revision." )
|    (put 'vc--revbuf-revision 'permanent-local t)

> > +         (parent (or buffer (get-file-buffer file) (current-buffer)))
> > +         (revd-file (vc-version-backup-file-name file revision 'manual))
> > +         (true-dir (file-name-directory file))
> > +         (true-name (file-name-nondirectory file))
> > +         (save-dir (concat vc-cache-root true-dir))
>
> Please add a comment explaining why `expand-file-name` would not
> be right.

|         ;; Use concat because true-dir and revd-file are already absolute.
|         ;; Here each is being mirrored beneath vc-mirror-root.
|         (save-dir (concat vc-mirror-root true-dir))
|         (save-file (concat vc-mirror-root revd-file))

> > +         (revd-name (file-name-nondirectory revd-file))
> > +         (save-file (concat vc-cache-root revd-file))
> > +         ;; Some hooks assume that buffer-file-name associates a buffer with
> > +         ;; a true file.  This mapping is widely assumed to be one-to-one.
> > +         ;; To avoid running afoul of that assumption this fictitious path
> > +         ;; is expected to be unique (bug#39190).  This path also has the
>
> The GNU convention is to call those things "file names" rather than
> "paths", because "path" is only used to mean a list of directories, as
> in `load-path`.

Done.

QUESTION:
In code I see the identifier 'filename', but in documentation I see the
phrase 'file name'.  Is that a correct statement of the convention?

> > +         ;; virtue that it exhibits the same file type (extension) as FILE.
> > +         ;; This improves setting the buffers modes.
> > +         (pretend (concat true-dir "PRETEND/" true-name))
>
> The old code just used `file` for `buffer-file-name` during
> `set-auto-mode`.  I believe it was safer.
> Why do you need this "PRETEND/", it seems to be a change unrelated to
> the rest of the patch.

TODO: provide an answer

> > +            ;; Prep revbuf in case it is being reused.
> > +            (setq buffer-file-name nil) ; Cancel any prior file visitation
> > +            (setq vc-parent-buffer nil)
> > +            (setq vc-tm--revision nil)
> > +            (setq buffer-read-only nil)
>
> Why not set them directly to their intended value?

Done.

> > +             ;; A cached file is viable IFF it is not writable.
> > +             ((and (file-exists-p save-file) (not (file-writable-p save-file)))
> > +              (insert-file-contents save-file t))
>
> Rather than repeat what the code tests, the comment should explain why
> (you think) it needs to be "not writable" to be viable.

|            ;; Do not trust an existing file to be an intact cached copy
|            ;; of the desired revision unless it is read-only.  This is
|            ;; because, in spite of having the desired filename, it may
|            ;; have been corrupted subsequent to its creation.  Since this
|            ;; function creates cached copies as read-only, some other agent
|            ;; would have had to have change the permissions and, most
|            ;; likely, changed the file's contents as well.

> > +              ;; Backend's output was read with 'no-conversions; do the same for write
> > +              (setq buffer-file-coding-system 'no-conversion)
> > +              (write-region nil nil save-file)
>
> Why `setq` rather than let-binding?

Fixed.

> Also, I can't see where in the new code you do the decoding which the
> old code does with (decode-coding-inserted-region (point-min)
> (point-max) file)?

Thank you for pointing out this omission.

My vc-find-revision function attempts to unify the behavior of the
earlier vc-find-revision-no-save and vc-find-revision-save functions.
Investigating the omission you identified has helped me to better
understand the relationship between those two functions and the
version that I have attempted to craft.  Based on your many bits of
feedback my function is now shorter and better commented.
I hope that it is clearer, more idiomatic, and more nearly correct.

> > +              (set-file-modes save-file (logand (file-modes save-file) #o7555)))
>
> There can be circumstances where a file is always writable, no
> matter how hard we try to use `set-file-modes`.

Indeed.  Now mentioned in a comment.

> > diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> > index 7689d5f879..1f45aa7e96 100644
> > --- a/lisp/vc/vc-git.el
> > +++ b/lisp/vc/vc-git.el
> > @@ -82,6 +82,9 @@
> >  ;; - annotate-time ()                              OK
> >  ;; - annotate-current-time ()                      NOT NEEDED
> >  ;; - annotate-extract-revision-at-line ()          OK
> > +;; TIMEMACHINE
> > +;; * tm-revisions (file)
> > +;; * tm-map-line (file from-revision from-line to-revision from-is-older)
> >  ;; TAG/BRANCH SYSTEM
> >  ;; - create-tag (dir name branchp)                 OK
> >  ;; - retrieve-tag (dir name update)                OK
>
> Any specific reason you used `*` rather than `-`?
> [ I have no objection to this choice, just curious.  ]

From vc.el's front matter:

|    ;; In the list of functions below, each identifier needs to be prepended
|    ;; with `vc-sys-'.  Some of the functions are mandatory (marked with a
|    ;; `*'), others are optional (`-').

> > @@ -101,6 +104,8 @@
> >
> >  (require 'cl-lib)
> >  (require 'vc-dispatcher)
> > +(require 'transient)
> > +(require 'vc-timemachine)
> >  (eval-when-compile
> >    (require 'subr-x) ; for string-trim-right
> >    (require 'vc)
>
> Are these really indispensable here?
>
> `vc-git` will be loaded into many more sessions than those where
> `transient` and `vc-timemachine` will be used, so it would be *much*
> better if those packages could be loaded more lazily here.

The unnecessary requires came in as part of refactoring git-timemachine.
Both are now gone.  Here are vc-git's current requires:

|    (require 'cl-lib)
|    (require 'vc-dispatcher)
|    (eval-when-compile
|      (require 'subr-x) ; for string-trim-right
|      (require 'vc)
|      (require 'vc-dir))

> > @@ -166,6 +171,12 @@ vc-git-program
> >    :version "24.1"
> >    :type 'string)
> >
> > +(defcustom vc-git-global-git-arguments
> > +  '("-c" "log.showSignature=false" "--no-pager")
> > +  "Common arguments for all git commands."
> > +  :type 'list
> > +  :group 'vc-timemachine)
>
> Why is this in the `vc-timemachine` group?
> Why do we need to add those args to all the Git commands?

This came from moving the renamed function vc-git-process-file and
the renamed option vc-git-global-git-arguments out of git-timemachine
and into vc-git.  The vc-timemachine group was a vestige of that same
refactoring.  I have improved the naming and documentation:

|    (defcustom vc-git-global-git-process-file-arguments
|      '("-c" "log.showSignature=false" "--no-pager")
|      "Common arguments for all invocations of `vc-git--process-file'."
|      :type 'list)

|    (defun vc-git--process-file (&rest args)
|      "Run `process-file' with ARGS and
`vc-git-global-git-process-file-arguments'."
|      (apply #'process-file vc-git-program nil t nil
|             (append vc-git-global-git-process-file-arguments args)))

> > -    (vc-git-command
> > -     buffer 0
> > -     nil
> > -     "cat-file" "blob" (concat (if rev rev "HEAD") ":" fullname))))
> > +    (ignore-errors
> > +      (vc-git-command
> > +       buffer 0
> > +       nil
> > +       "cat-file" "blob" (concat (if rev rev "HEAD") ":" fullname)))))
>
> Please add a comment here explaining why we need `ignore-errors` (you
> can probably just move the text you currently have in the commit
> message: in many cases it's better to put those comments in the code
> rather than in the commit message).

Done.

> Also, if you can use `ignore-error` instead, it would be better.

TODO: determine actual error and switch to ignore-error

> > +        (setq new-date (vc-tm-format-date (match-string 2 line)))
>
> How important is it to use the `vc-tm-date-format`?
> I'm not super happy about the current design in this regard.  I think it
> would make more sense to define `tm-revisions` as returning dates in the
> "cheapest" format possible (the one that takes least effort), e.g.
> as an ELisp time value (e.g. as returned by `date-to-time`) or as
> "any format that `date-to-time` understands"?
> Then the call to `vc-tm-date-format` can be moved to
> `vc-timemachine.el`.

Done, exactly as you suggested.  Thanks.

> > -(eval-when-compile (require 'cl-lib))
> > +(eval-when-compile
> > +  (require 'cl-lib))
>
> Why?
> [ It's likely a question of taste, so I' recommend not to touch it.
>   Of course, I noticed it because my taste prefers the current format
>   (because in `outline-minor-mode` the `require` is otherwise hidden
>   for no benefit).  ]
>
> > @@ -41,6 +45,7 @@
> >    (require 'cl-lib)
> >    (require 'vc))
> >  (require 'log-view)
> > +(require 'vc-timemachine)
>
> Same comment as for `vc-git`.

These are now gone.  Further, I have attempted to eliminate unnecessary
requires in all files that I have touched.

> > +        (setq line (buffer-substring-no-properties (line-beginning-position) (line-end-position)))
>
> Please try very hard to always stay within 80 columns.

Got it.  Corrected all violations that I could find.

> > +(defgroup vc-timemachine nil
> > +  "Time-machine functionality for VC backends."
> > +  :group 'vc
> > +  :version "30.1")
> > +
> > +(defcustom vc-tm-date-format
> > +  "%a %I:%M %p %Y-%m-%d"
> > +  "Revision creation date format (emphasis on easy date comparison)."
> > +  :type 'string
> > +  :group 'vc-timemachine
> > +  :version "30.1")
>
> The `:group` arg here is redundant (`defcustom` would use that group by
> default here anyway).  Same for the subsequent `defcustom`s and `defface`s.

Fixed

> > +(defvar-local vc--time-machine nil
> > +  "Cache a TM hint on various buffers.")
> > +(put 'vc--time-machine 'permanent-local t)
>
> The docstring seems of very little as it stands.  You could just as well
> remove it (tho it'd be better to actually describe what this var should
> hold, of course :-).

Updated:

|    (defvar-local vc--tmbuf nil
|      "Bind a non-timemachine buffer to its tmbuf.")
|    (put 'vc--tmbuf 'permanent-local t)

> > +(defvar-local tmbuf--abs-file nil
> > +  "Absolute path to file being traversed by this time-machine.")
>
> "path" => "file name".
>
> Also, please use the "vc-" prefix.  Same for the other "tmbuf-" vars.

Updated:

|    (defvar-local vc--tmbuf-file nil
|      "Version controlled file being traversed by this tmbuf.")
|    (put 'vc--tmbuf-file 'permanent-local t)
|    (defvar-local vc--tmbuf-backend nil
|      "The VC backend being used by this tmbuf")
|    (put 'vc--tmbuf-backend 'permanent-local t)
|    (defvar-local vc--tmbuf-branch-index nil
|      "Zero-base index into vc--tmbuf-branch-revisions.")
|    (put 'vc--tmbuf-branch-index 'permanent-local t)
|    (defvar-local vc--tmbuf-branch-revisions nil
|      "When non-nil, a vector of revision-info lists.")
|    (put 'vc--tmbuf-branch-revisions 'permanent-local t)

> > +(defvar-local tmbuf--branch-index nil
> > +  "Zero-base index into tmbuf--branch-revisions.")
> > +(put 'tmbuf--branch-revisions 'permanent-local t)
> > +(defvar-local tmbuf--branch-revisions nil
> > +  "When non-nil, a vector of revision-info lists.")
> > +(put 'tmbuf--branch-revisions 'permanent-local t)
>
> You make `tmbuf--branch-revisions` permanent twice (the other should
> probably be for `tmbuf--branch-index`, right?).

Fixed.  See immediately preceding quoted source.

> > +(defvar-local tmbuf--source-buffer nil
> > +  "A non-time-machine buffer for which this time-machine was created.")
> > +(put 'tmbuf--source-buffer 'permanent-local t)
>
> Any reason we can't use `vc-parent-buffer`?

Ultimately, tmbuf--source-buffer was unused.  Now gone.

> > +(defun vc-tm--time-machine ()
> > +  "Return a valid time-machine for the current buffer."
>
> Indicate how it's returned.  I.e. either by changing current-buffer, or
> by returning a buffer object (in which case it should *not* change
> current-buffer).

Done.

|      "Return a valid tmbuf for the current buffer.
|
|    A tmbuf (timemachine buffer) has a properly filled-out set of vc--tmbuf*
|    local variables.  If the current buffer is already a valid tmbuf then
|    just return that buffer.  Otherwise create a revbuf for the
current buffer's
|    file (or, if the current buffer is an indirect buffer, then for the base
|    buffer's file) and set its vc--tmbuf* data.  Most importantly, set
|    vc--tmbuf-branch-revisions  to an ordered vector of revision information
|    lists for the revisions on the work file's branch."

> > +        (when vc-tm-echo-area
> > +          (vc-tm--show-echo-area-details new-revision-info))))
> > +      (vc-tm--erm-workaround))))
>
> Please avoid this `vc-tm--erm-workaround` here.  Better add a "generic
> hook"

Done.

> i.e. a hook whose meaning makes sense independently from this ERM
> problem), and then arrange for some code somewhere (probably best if
> it's in `enh-ruby-mode`) to add a function to this hook.
>
> > +(declare-function erm-reset-buffer "ext:enh-ruby-mode")
> > +
> > +(defun vc-tm--erm-workaround ()
> > +  "Workaround for enhanced ruby mode not detecting revision change."
> > +  (when (eq major-mode 'enh-ruby-mode)
> > +    (ignore-errors (erm-reset-buffer))))
>
> Prefer `derived-mode-p`.  Add a comment explaining the problem or
> pointing to info about it.
> Arrange to try and make this unnecessary in the future (i.e. open a bug
> report with the enh-ruby-mode guys?  Or fix the source of the bug if
> it's elsewhere in Emacs itself?).

This is a bit hard to do.  enh-ruby-mode is only available from
MELPA.  And opening a bug mentioning an available hook in
a package that is not yet available feels strange.

TODO: Once vc-timemachine is available on master post
an enhancement request to enh-ruby-mode.

> > +;; - tm-map-line (rel-file from-revision from-line to-revision from-is-older)
> > +;;
> > +;;   Return TO-REVISION's line corresponding to FROM-REVISION's FROM-LINE.
> > +;;   FROM-REVISION and TO-REVISION are guaranteed distinct.  FROM-IS-OLDER
> > +;;   indicates relative temporal ordering of FROM-REVISION and TO-REVISION
> > +;;   on the branch.
>
> Please clarify that you're talking about line *numbers* (I thought at
> first you were talking about some completely different notion of lines,
> such as "product line" or "history line").

How is this?

|    ;;   Return a TO-REVISION line number corresponding to FROM-REVISION's
|    ;;   FROM-LINE.  FROM-REVISION and TO-REVISION are guaranteed distinct.
|    ;;   FROM-IS-OLDER indicates relative temporal ordering of FROM-REVISION
|    ;;   and TO-REVISION on the branch.

> OK, I'll stop here for now.

That was a great review!  I know that it took concentration and
effort.  Thank you!



/john

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: John Yates <john <at> yates-sheets.org>
Cc: 61071 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#61071: New features: VC timemachine and BackupOnSave to RCS
Date: Wed, 10 Jan 2024 14:42:11 -0800

John Yates <john <at> yates-sheets.org> writes:

> Stefan Kangas,
>
> Personal and family issues have arisen that make me quite unsure of
> when I will be able to return to this activity.
>
> I did address just about all of Stefan Monnier's feedback (see
> appended, never sent reply below).
>
> Currently the code is on a scratch/backup-on-save-to-rcs branch in my
> local repository.  I tried unsuccessfully to push it to Savannah:
>
> |   jyates <at> envy:~/repos/emacs.sv
> |   $ git push -u origin scratch/backup-on-save-to-rcs
> |   fatal: remote error: access denied or repository not exported: /emacs.git
>
> I was under the impression that no special permissions are needed to
> push a scratch branch.  Am I doing something wrong?

Apologies for the late reply here.

You need commit access to push a scratch branch.  Do you have that?

Perhaps you could send it as a patch if you can't get pushing to work?

> In my testing, the code works nicely.  My current sense of things that
> could be improved are:
> - Finding the proper commit in a series whose only metadata is the
> commit timestamp is sub-optimal.
> - It might be nice to have some kind of cron-based clean-up or commit squashing

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 61071 <at> debbugs.gnu.org, John Yates <john <at> yates-sheets.org>
Subject: Re: bug#61071: New features: VC timemachine and BackupOnSave to RCS
Date: Wed, 10 Jan 2024 22:44:31 -0500

> You need commit access to push a scratch branch.  Do you have that?
> Perhaps you could send it as a patch if you can't get pushing to work?

You can also push it to some other public repository.


        Stefan

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: John Yates <john <at> yates-sheets.org>
Cc: 61071 <at> debbugs.gnu.org, control <at> debbugs.gnu.org
Subject: Re: bug#61071: New features: VC timemachine and BackupOnSave to RCS
Date: Fri, 14 Mar 2025 13:59:30 +0800

tag 61071 + moreinfo
thanks

Dear John,

Were you able to make any progress here?

We probably won't be able to progress this without you, and it's been
two years since you were working on it actively, so I would propose to
close this bug for now if you aren't able to work on it.

-- 
Sean Whitton

Message #32 received at 61071-close <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: 61071-close <at> debbugs.gnu.org
Subject: Re: bug#61071: New features: VC timemachine and BackupOnSave to RCS
Date: Sat, 15 Mar 2025 15:35:13 +0800

Hello,

On Fri 14 Mar 2025 at 01:59pm +08, Sean Whitton wrote:

> tag 61071 + moreinfo
> thanks
>
> Dear John,
>
> Were you able to make any progress here?
>
> We probably won't be able to progress this without you, and it's been
> two years since you were working on it actively, so I would propose to
> close this bug for now if you aren't able to work on it.

John wrote privately to say that he wants to resubmit this as a series
of discrete features and said that closing this bug would be fine in the
meantime.

-- 
Sean Whitton

Previous Next

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