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

[Stefan Monnier]

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