From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#61071: New features: VC timemachine and BackupOnSave to RCS Date: Sat, 11 Feb 2023 18:02:22 -0500 Message-ID: References: Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4633"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 61071@debbugs.gnu.org To: John Yates Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Feb 12 00:03:23 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 1pQytu-0000yF-G0 for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 12 Feb 2023 00:03:22 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pQytg-0001AU-WB; Sat, 11 Feb 2023 18:03:09 -0500 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 1pQyta-0001AG-No for bug-gnu-emacs@gnu.org; Sat, 11 Feb 2023 18:03:03 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pQyta-0000M1-Em for bug-gnu-emacs@gnu.org; Sat, 11 Feb 2023 18:03:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pQyta-0006u1-2l for bug-gnu-emacs@gnu.org; Sat, 11 Feb 2023 18:03:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 11 Feb 2023 23:03: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.167615655826500 (code B ref 61071); Sat, 11 Feb 2023 23:03:02 +0000 Original-Received: (at 61071) by debbugs.gnu.org; 11 Feb 2023 23:02:38 +0000 Original-Received: from localhost ([127.0.0.1]:43510 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pQytC-0006tM-1J for submit@debbugs.gnu.org; Sat, 11 Feb 2023 18:02:38 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:21265) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pQyt8-0006t8-0L for 61071@debbugs.gnu.org; Sat, 11 Feb 2023 18:02:36 -0500 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 5437F44215C; Sat, 11 Feb 2023 18:02:28 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 362A6442160; Sat, 11 Feb 2023 18:02:25 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1676156545; bh=j6qP0qk1jXmCNezHcdJLPLz8gQewd8iI45YVz4ZAoJ0=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=odrA8ZFY+u0Dvf9mHC1L0VXaZBdOWWucilsLfpNP9uGbBoXTwurpgFa7G07Zw+YB6 qM5AsV3je0FfuxmAKICsbdzwuZUb7jheesCiYYecShPOQrdeeqNoef574VNBHdIFB2 cZ7AnJ0dNBQRiPgmIiHcyjhOT42kuVfvLt+xF3CmJEeZJTMYn3B4Z4iFPwk9wvwbnn VEDVWuN1WMDr82egdlI9F8NjtqPpIERPh/nTeKRWBUvMT4hKL1ZERE8S0lzNxaIxAT ogCctqhxtKEtSWcefLcLsDoV9g5sakreKqJTFmALhwKYBdAUYyQEAdEjw05f6bgBmx T/frDVeixAcDQ== Original-Received: from ceviche (unknown [45.44.229.252]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 05BF0123190; Sat, 11 Feb 2023 18:02:24 -0500 (EST) In-Reply-To: (John Yates's message of "Wed, 25 Jan 2023 22:24:30 -0500") 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:255351 Archived-At: 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