Package: emacs;
Reported by: John Yates <john <at> yates-sheets.org>
Date: Thu, 26 Jan 2023 03:25:02 UTC
Severity: wishlist
Tags: moreinfo, patch
Found in version 30.0.50
Done: Sean Whitton <spwhitton <at> spwhitton.name>
Bug is archived. No further changes may be made.
View this message in rfc822 format
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: 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.