James Thomas: 2
vc-backup: bug: Normal backups inhibited after registering in this backend
vc-backup: bug: Normal backups inhibited after registering in this backend
2 files changed, 20 insertions(+), 3 deletions(-)
I think I'll do so for now. I am working on undeprecating
`vc-switch-backend' (that will now be known under a new name), and will
suggest adding a hook or something comparable.
Normal backups are inhibited after vc-backup is enabled on a file due to
the default setting of vc-make-backup-files. I use this patch to change
this behaviour while keeping backups disabled for other backends.
I am the kind of person who thinks of `when' having an undefined return
value, meaning that it should only be used when the return-value of the
expression is not used. If you are interested in what the evaluation,
then `and' is more appropriate.
One issue I can think of there is that if someone were to use
`vc-switch-backend' (that is currently deprecated but I hope to change
that), then the value would still be modified. Does the argument, that
if the user has package-vc installed and uses it, this implies they are
more interested in having versioned backup hold, and legitimate this
"leak"?
Or would having a separate user option for this in vc-backup turn out to
be necessary?
(defun vc-backup-state (_file)
"Inform VC that there is no information about any file."
I've signed the copyright papers, if you want to use this patch.
I'd gladly apply the change. If you want to, you can also submit it as
a "git format-patch"-style patch with a commit message, but no problem
if you don't, I can do that myself as well.
Philip Kaludercic wrote:
> James Thomas writes:
>
>> Normal backups are inhibited after vc-backup is enabled on a file due to
>> the default setting of vc-make-backup-files. I use this patch to change
>> this behaviour while keeping backups disabled for other backends.
>
> Right, good point.
>
>> diff --git a/vc-backup.el b/vc-backup.el
>> index 5c6ca2e..097a035 100644
>> --- a/vc-backup.el
>> +++ b/vc-backup.el
>> @@ -214,8 +214,11 @@ file."
>> ;;;###autoload
>> (defun vc-backup-registered (file)
>> "Inform VC that FILE will work if a backup can be found."
>> - (or (not (null (diff-latest-backup-file file)))
>> - (backup-file-name-p file)))
>> + (when (or (not (null (diff-latest-backup-file file)))
>
> I am the kind of person who thinks of `when' having an undefined return
> value, meaning that it should only be used when the return-value of the
> expression is not used. If you are interested in what the evaluation,
> then `and' is more appropriate.
Right. It's not needed if the `unless' isn't either.
>> + (backup-file-name-p file))
>> + (unless vc-make-backup-files
>> + (setq-local vc-make-backup-files t))
>
> Why the `unless'? There is no inherent harm in setting a buffer local
> value to be the same as the global value.
>
I was thinking of perturbing the state as little as possible, while also
saving a bit of memory. But that way would be fine too.
>> + t))
>
> One issue I can think of there is that if someone were to use
> `vc-switch-backend' (that is currently deprecated but I hope to change
> that), then the value would still be modified. Does the argument, that
> if the user has package-vc installed and uses it, this implies they are
> more interested in having versioned backup hold, and legitimate this
> "leak"?
>
> Or would having a separate user option for this in vc-backup turn out to
> be necessary?
I can't find an easier way around this than using an advice, as in the
patch below (ignore the earlier one, btw). IMO the 'leak' should be
avoided since not wanting backups could very well be the reason for the
user switching the backend. And a separate user option is not ideal as
it'd be meant to cope with a failing in program logic: I think we know
what the user wants.
But if it's ugly then perhaps we could leave the question for later.
>> (defun vc-backup-state (_file)
>> "Inform VC that there is no information about any file."
>>
>>
>> I've signed the copyright papers, if you want to use this patch.
>
> I'd gladly apply the change. If you want to, you can also submit it as
> a "git format-patch"-style patch with a commit message, but no problem
> if you don't, I can do that myself as well.
>
Do not inhibit normal backups even if vc-make-backup-files is the
default nil.
* vc-backup.el (vc-backup-find-file-hook): New function.
(vc-backup-reset-make-backup-files): Advice for vc-switch-backend.
(vc-backup-unload-function): New function; to clean up the advice.
---
vc-backup.el | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/vc-backup.el b/vc-backup.el
index 5c6ca2e..c87e370 100644
--- a/vc-backup.el+++ b/vc-backup.el
@@ -443,13 +443,27 @@ BUFFER and ASYNC as interpreted as specified in vc.el."
(let ((new-backup (concat new-part (substring backup (length old-part)))))
(rename-file backup new-backup t)))))
-;; - find-file-hook ()+(defun vc-backup-find-file-hook ()+ "Make sure backups are not inhibited."+ (setq-local vc-make-backup-files t))++(defun vc-backup-reset-make-backup-files (_ backend)+ "Reset the value of `vc-make-backup-files'. For use as advice to+`vc-switch-backend'."+ (if (eq backend 'Backup)+ (setq-local vc-make-backup-files t)+ (kill-local-variable 'vc-make-backup-files)))
E.g. here, is it really legitimate to kill the local variable, just
because the backend is switched. I could imagine someone setting this
value locally, and then switching between say Git and CVS, and having
their setup broken because of this configuration.
++(advice-add 'vc-switch-backend :after #'vc-backup-reset-make-backup-files);; - extra-menu ()
;; - extra-dir-menu ()
;; - conflicted-files (dir)
++(defun vc-backup-unload-function ()+ (advice-remove 'vc-switch-backend #'vc-backup-reset-make-backup-files));;; This snippet enables the Backup VC backend so it will work once
;;; this file is loaded. By also marking it for inclusion in the
--
2.34.1