~pkal/public-inbox

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
5 2

[PATCH] vc-backup: bug: Normal backups inhibited after registering in this backend

James Thomas <jimjoe@gmx.net>
Details
Message ID
<87led3vw63.fsf@outlook.com>
DKIM signature
missing
Download raw message
Patch: +5 -2
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.

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)))
            (backup-file-name-p file))
    (unless vc-make-backup-files
      (setq-local vc-make-backup-files t))
    t))

(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.

--
Details
Message ID
<87led2bl0d.fsf@posteo.net>
In-Reply-To
<87led3vw63.fsf@outlook.com> (view parent)
DKIM signature
missing
Download raw message
James Thomas <jimjoe@gmx.net> 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.

> +            (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.

> +    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?

>  (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.

> --
James Thomas <jimjoe@gmx.net>
Details
Message ID
<87r0mtbokp.fsf@gmx.net>
In-Reply-To
<87led2bl0d.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
Patch: +15 -1
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)))

(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
Details
Message ID
<87wmwivdgd.fsf@posteo.net>
In-Reply-To
<87r0mtbokp.fsf@gmx.net> (view parent)
DKIM signature
missing
Download raw message
James Thomas <jimjoe@gmx.net> writes:

>>> +            (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.

I wouldn't concern myself too much with these kinds of things, if they
come at the cost of greater complexity.

>>> +    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.

I agree, advising a deprecated function is something I'd want to avoid
in general.

>>>  (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.
>>

How did you format this patch?  Isn't the header missing?

> 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
James Thomas <jimjoe@gmx.net>
Details
Message ID
<87fs35zjt3.fsf@gmx.net>
In-Reply-To
<87wmwivdgd.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
Philip Kaludercic wrote:

> I wouldn't concern myself too much with these kinds of things, if they
> come at the cost of greater complexity.

You're right.

>> +(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.

My reasoning: Since the variable starts with `vc-' it's only applicable
to that package. For other uses one could simply set `backup-inhibited'.

> How did you format this patch?  Isn't the header missing?

Attached:
Details
Message ID
<875y3zdh58.fsf@posteo.net>
In-Reply-To
<87fs35zjt3.fsf@gmx.net> (view parent)
DKIM signature
missing
Download raw message
James Thomas <jimjoe@gmx.net> writes:

> Philip Kaludercic wrote:
>
>> I wouldn't concern myself too much with these kinds of things, if they
>> come at the cost of greater complexity.
>
> You're right.
>
>>> +(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.
>
> My reasoning: Since the variable starts with `vc-' it's only applicable
> to that package. For other uses one could simply set `backup-inhibited'.

But vc-backup is not vc, which to me makes it more difficult to
legitimise this approach.

>> How did you format this patch?  Isn't the header missing?
>
> Attached:
>
> From c03d0af3bf3f449a4784e57dfe0cf8fe8f615043 Mon Sep 17 00:00:00 2001
> From: James Thomas <james.thomas@ahimsa.global>
> Date: Wed, 20 Sep 2023 07:11:40 +0530
> Subject: [PATCH] Do not inhibit normal backups due to vc-make-backup-files
>
> 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)))
> +
> +(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
>
>
>
> (Use only the first hunk if that's what you prefer)

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.

> --
>

-- 
Philip Kaludercic
Reply to thread Export thread (mbox)