From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: John Yates Newsgroups: gmane.emacs.bugs Subject: bug#61071: New features: VC timemachine and BackupOnSave to RCS Date: Mon, 11 Sep 2023 09:04:34 -0400 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="708"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 61071@debbugs.gnu.org To: Stefan Kangas , Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Sep 11 15:05:16 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qfgbM-0000JN-8q for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 11 Sep 2023 15:05:16 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qfgbB-000662-SA; Mon, 11 Sep 2023 09:05:05 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qfgb5-00065f-4z for bug-gnu-emacs@gnu.org; Mon, 11 Sep 2023 09:04:59 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qfgb4-0000tt-S7 for bug-gnu-emacs@gnu.org; Mon, 11 Sep 2023 09:04:58 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qfgb8-0004BQ-H9 for bug-gnu-emacs@gnu.org; Mon, 11 Sep 2023 09:05:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: John Yates Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 11 Sep 2023 13:05:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 61071 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 61071-submit@debbugs.gnu.org id=B61071.169443750216078 (code B ref 61071); Mon, 11 Sep 2023 13:05:02 +0000 Original-Received: (at 61071) by debbugs.gnu.org; 11 Sep 2023 13:05:02 +0000 Original-Received: from localhost ([127.0.0.1]:52335 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qfgb6-0004B8-Sz for submit@debbugs.gnu.org; Mon, 11 Sep 2023 09:05:01 -0400 Original-Received: from mail-ej1-f47.google.com ([209.85.218.47]:42460) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qfgb3-0004At-H8 for 61071@debbugs.gnu.org; Mon, 11 Sep 2023 09:04:59 -0400 Original-Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-99c93638322so946949966b.1 for <61071@debbugs.gnu.org>; Mon, 11 Sep 2023 06:04:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694437487; x=1695042287; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=rL5UXoZBXy1awwagyV0gJxR0IvBQPzaEYx9uV+Ri8ts=; b=LLgJsXgbbQ3imR4ZGUv7riAaNWrxH6F8DvTfBaTCMCH/AzLQQ8RRwTlYmrvm124vfE 3a1GEp3KJb+LD+9CT0GvZX2i/hPKBzdtSoFEsAqdXwWdq7hsR/6FqYzzaWzj5emsBR/l UxvwDTE3Fvx9RT+n7AZ4gzqWxI6cw0NF6e/cuFQDbCQkqRT4W/6vKEGVck0fV8txFmPh bptc8UpFaTCmEokdM06ZtyDusoBhj0SPBc5EvwcWLCVsBI6t79CAYFGZ6ot3lO4u5wF5 pTDoXRmxgden+LWfcn3VJypd1+Z6cmSg1xyjT9vV5/ZzfjVe85qkN0a6Y9Syz0Ejhpuc D9Wg== X-Gm-Message-State: AOJu0YwxtFMOevxS8hGAoarmblgp9dlnPzz5DlmYi2+A5/1Pl6hFojEp wg7C9Xd2AfpMJu3T1IfF03Oi/obY1E/Q/JFn9s4B8NlUALQ= X-Google-Smtp-Source: AGHT+IF3D5OlG/JuEd7JgbFPSs83SkH93n8KZlDP04WYxzsEqFn16dHikS/Cj1tqK/Du4RZ9EbLNFYwE0QvEVm63S2Q= X-Received: by 2002:a17:906:5a63:b0:9a5:f038:a4c1 with SMTP id my35-20020a1709065a6300b009a5f038a4c1mr11935331ejc.26.1694437487011; Mon, 11 Sep 2023 06:04:47 -0700 (PDT) In-Reply-To: X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:270089 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 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