bug#61071: New features: VC timemachine and BackupOnSave to RCS

[John Yates]

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