bug#12507: 24.2.50; `bookmark-write-file': use `write-file', not `write-region', to get backups

bug#12507: 24.2.50; `bookmark-write-file': use `write-file', not `write-region', to get backups

[Drew Adams]

I believe that `bookmark-write-file' should use `write-file', not
`write-region', because otherwise no backup file is made for the user's
bookmark file.
 
The original code used `write-file'.  The change was made to use
`write-region' on 2005-11-12:
 
 Revision ID: kfogel@red-bean.com-20051112203022-g0v8q4n9urlfew61
 
 (bookmark-write-file): Don't visit the destination file, just write
 the data to it using write-region.  This is similar to revision 1.32
 of saveplace.el, but with an additional change to avoid visiting the
 file in the first place.
 
I'm no expert on this, but from what I see by experimenting, using
`write-region' means that no backup is made, and using `write-file'
creates a backup.  It is important for users to be able to have a backup
for their bookmark file.
 
 
In GNU Emacs 24.2.50.1 (i386-mingw-nt5.1.2600)
 of 2012-09-17 on MARVIN
Bzr revision: 110062 cyd@gnu.org-20120917054104-r93rtwkrtva73ewe
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -ID:/devel/emacs/libs/libXpm-3.5.8/include
 -ID:/devel/emacs/libs/libXpm-3.5.8/src
 -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include
 -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include
 -ID:/devel/emacs/libs/giflib-4.1.4-1/include
 -ID:/devel/emacs/libs/jpeg-6b-4/include
 -ID:/devel/emacs/libs/tiff-3.8.2-1/include
 -ID:/devel/emacs/libs/gnutls-3.0.9/include
 -ID:/devel/emacs/libs/libiconv-1.13.1-1-dev/include
 -ID:/devel/emacs/libs/libxml2-2.7.8/include/libxml2'
 

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

Why on earth would you relegate this to the wishlist?
This is about possible loss of user data.

I provided the fix, which takes 5 seconds to implement.
What's the problem?

Am I wrong that `write-region' does not provide for backups?
If not, then this is clearly a bug, and a pretty serious one, IMO.

bookmark.el even provides a user option, `bookmark-version-control', for
deciding whether and when to make numbered backups of the bookmark file.
AFAICT, that cannot possibly work if `write-region' does not create backups, as
is apparently the case.

FWIW, I made the same fix in Bookmark+.  I just think vanilla Emacs should
benefit its users as well.

Please read the bug report again and reconsider.  Thx.

> > severity 12507 wishlist
> > Bug #12507 [emacs] 24.2.50; `bookmark-write-file': use 
> > `write-file', not `write-region', to get backups
> > Severity set to 'wishlist' from 'normal'

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Chong Yidong]

"Drew Adams" <drew.adams@oracle.com> writes:

> Am I wrong that `write-region' does not provide for backups?
> If not, then this is clearly a bug, and a pretty serious one, IMO.

It would be annoying litter the filesystem with backups for every
internal configuration file.  The abbrev file and desktop file are not
backed up either, and I think that's fine.

I wouldn't mind adding a global feature to optionally enable backups for
such files.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> > Am I wrong that `write-region' does not provide for backups?
> > If not, then this is clearly a bug, and a pretty serious one, IMO.
> 
> It would be annoying litter the filesystem with backups for every
> internal configuration file.

Why not let _users_ decide what is annoying to them and their filesystems?  Some
users (and I know some) might well want a backup file for their bookmarks.  See
bug #12503 for a hint why.

But if you've already decided that a fix for this bug is not to be, because it
is a bad idea for a user to back up her bookmark file, then why send this bug to
the wishlist instead of marking it wontfix?  Or does wishlist for you really
mean the same thing as wontfix?

> The abbrev file and desktop file are not
> backed up either, and I think that's fine.

Is your `custom-file' an "internal configuration file"?  Do you back it up?

It is not just about what you think is fine for users - not when it comes to
user data.  It's about what individual users think about their data.  Let them
decide, please.

And as I said, we even have a user option for whether you want your
bookmark-file backups to be numbered: `bookmark-version-control'.  Imagine that!
Besides `version-control', which applies to all files, we even give you a
special option that applies only to your bookmark file.

What do you think is the point of that option, if your bookmark file is in fact
NEVER backed up at all?  Do you not see a bug here?

> I wouldn't mind adding a global feature to optionally enable 
> backups for such files.

Fine.  Let users enable backups, please.  I have no problem with making that a
user option.

But in that case please document the fact that `bookmark-version-control' has no
effect when your new option is turned off (no backups).  Let's make it as clear
as possible how to (a) turn backing up on/off and (b) control the backup naming
when on.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Stefan Monnier]

> the wishlist instead of marking it wontfix?  Or does wishlist for you really
> mean the same thing as wontfix?

wishlist means "patches welcome".


        Stefan

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> > the wishlist instead of marking it wontfix?  Or does 
> > wishlist for you really mean the same thing as wontfix?
> 
> wishlist means "patches welcome".

Oh, come on.  I provided the patch: use `write-file' instead of `write-region'.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Stefan Monnier]

>> > the wishlist instead of marking it wontfix?  Or does 
>> > wishlist for you really mean the same thing as wontfix?
>> wishlist means "patches welcome".
> Oh, come on.  I provided the patch: use `write-file' instead of
> `write-region'.

Oh come on, you know it's not good enough because we'd rather not have
backups by default.


        Stetfan

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> Oh come on, you know it's not good enough because we'd rather not have
> backups by default.

You don't have backups at all.  So maybe you'd better get rid of option
`bookmark-version-control', while waiting for your wishlist to come true
someday.

Someone obviously did prefer to have backups by default, and not only by
default, or you wouldn't have that neutered option hanging around as a vestige.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

I propose the following fix:

  * As Drew suggested, change `bookmark-write-file' to use `write-file'
    instead of `write-region'.

  * Also change the default value of `bookmark-version-control' to be
    `nil' instead of `nospecial', so that backups of the bookmark data
    file are no longer on by default (unless there are already backup
    files present).

But... the only thing that makes me hesitate is the first step, because
back in 2005 we changed `bookmark-write-file' to use `write-region':

  2005-11-12  Karl Fogel  <kfogel@red-bean.com>
        * bookmark.el (bookmark-write-file): Don't visit the destination
        file, just write the data to it using write-region.  This is
        similar to revision 1.32 of saveplace.el, but with an additional
        change to avoid visiting the file in the first place.

The corresponding change in saveplace.el has just this comment:

  ;; Don't use write-file; we don't want this buffer to visit it.

Why didn't we want to visit the file?  Was there some reason why that
was a bad thing?  Unfortunately, I don't remember, but I don't want to
introduce a regression.

Drew or anyone, any idea what problem we were avoiding?

The status quo does seem a bug.  There are two fixes: make backups work
again, or deprecate `bookmark-version-control' and don't claim that the
bookmark data file can have automatic backups.

(In the meantime, Drew's suggestion in #12503 that `print-circle' be
bound to `t' seems right to me -- I'm trying to get outstanding
bookmark.el bugs fixed in time for the feature freeze on Oct. 1 and that
should be one of the fixes.  If so, then one of the reasons for being
able to back up the bookmarks data file will go away anyway.)

-Karl

"Drew Adams" <drew.adams@oracle.com> writes:
>It is not just about what you think is fine for users - not when it comes to
>user data.  It's about what individual users think about their data.  Let them
>decide, please.
>
>And as I said, we even have a user option for whether you want your
>bookmark-file backups to be numbered: `bookmark-version-control'.  Imagine that!
>Besides `version-control', which applies to all files, we even give you a
>special option that applies only to your bookmark file.
>
>[...]
>
>What do you think is the point of that option, if your bookmark file is in fact
>NEVER backed up at all?  Do you not see a bug here?
>
>> I wouldn't mind adding a global feature to optionally enable 
>> backups for such files.
>
>Fine.  Let users enable backups, please.  I have no problem with making that a
>user option.
>
>But in that case please document the fact that `bookmark-version-control' has no
>effect when your new option is turned off (no backups).  Let's make it as clear
>as possible how to (a) turn backing up on/off and (b) control the backup naming
>when on.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> I propose the following fix:
> 
>   * As Drew suggested, change `bookmark-write-file' to use 
>     `write-file' instead of `write-region'.
> 
>   * Also change the default value of `bookmark-version-control' to be
>     `nil' instead of `nospecial', so that backups of the bookmark data
>     file are no longer on by default (unless there are already backup
>     files present).

But how does that help a user turn backup on in the first place?  Not a
rhetorical question - I really don't know.  How should a user create the first
backup file?

What would the doc suggest to the user for that?  Copy the file to one with a
`~' suffix (error prone)?  Visit the bookmark file, type SPC then DEL, then `C-x
C-s' (error prone)?

What is an easy, sure way for a user who has never backed up a file (one that is
not typically visited interactively) to create a backup?

The question is not bookmark-specific.  I don't know a good answer.  It's
probably obvious, but I'm not seeing it.

> But... the only thing that makes me hesitate is the first 
> step, because back in 2005 we changed `bookmark-write-file' to use
> `write-region':
> 
>   2005-11-12  Karl Fogel  <kfogel@red-bean.com>
>         * bookmark.el (bookmark-write-file): Don't visit the 
>         destination file, just write the data to it using
>         write-region.  This is similar to revision 1.32 of
>         saveplace.el, but with an additional change to avoid
>         visiting the file in the first place.
> 
> The corresponding change in saveplace.el has just this comment:
> 
>   ;; Don't use write-file; we don't want this buffer to visit it.
> 
> Why didn't we want to visit the file?  Was there some reason why that
> was a bad thing?  Unfortunately, I don't remember, but I don't want to
> introduce a regression.
> 
> Drew or anyone, any idea what problem we were avoiding?

Sorry, I don't know.  I bisected the change logs from the start, to locate that
commit as the culprit change.  But I don't know more than what the log says.

Perhaps the reason was what Yidong expressed: a belief that a bookmark file is
only an "internal configuration file", rather than user data (presumably because
users do not typically edit it directly).  His contention is that backing up the
file would annoy users by littering their filesystems.

If that was the rationale for the 2005 change then it was misguided, IMO.

A bookmark file is not just an internal config file.  It contains user data that
can be valuable (to users).  Among other things, it can contain metadata (e.g.
annotations) about other files.  It has some things in common with Org mode for
keeping track of positions and other relations among documents.

Users can make mistakes that lead to losing individual bookmarks that they might
really want to keep, or even to losing all bookmarks.

In the other direction, it is very easy to load a second bookmark file into your
main bookmark file and save the result without necessarily meaning to.  To get
back what you had (by deleting the additions or replacing the replacements) is
laborious and error prone, unless you have a backup copy.

For such reasons, some users might want to have automatic backup for their
bookmarks.  I agree that backup should be optional and up to the user, of
course.
> The status quo does seem a bug.  There are two fixes: make 
> backups work again, or deprecate `bookmark-version-control'
> and don't claim that the bookmark data file can have automatic backups.
> 
> (In the meantime, Drew's suggestion in #12503 that `print-circle' be
> bound to `t' seems right to me -- I'm trying to get outstanding
> bookmark.el bugs fixed in time for the feature freeze on Oct. 
> 1 and that should be one of the fixes.  If so, then one of the reasons
> for being able to back up the bookmarks data file will go away anyway.)

Thank you for that, in advance.

There are however plenty of other ways a user can lose a bookmark file that took
a long time to construct.  To me, we should not only provide automatic backup
but turn it on by default.

(Would I apply the same arguments to some other "internal config files"?
Dunno/depends.  Maybe desktop files.  A lot depends on how important the given
"config" might be to a user and how long it takes to reconstruct it from
scratch.  In any case, I don't buy the blanket argument that dot files or
"internal config files" are necessarily things that a user does not want backed
up.)

---

I would in any case like to know an answer to my question above about creating
the first backup.

I also have a question about the idiom to use that would make a code change
analogous to the write-region --> write-file change discussed, but for
(write-region (point-min) (point-max 'APPEND), i.e., for appending the buffer
content to a file.

It's not clear to me what the best way would be to replace that code with code
that will not only append and write but also back up (if backing up is enabled).
I can code something up by appending the text to a buffer and then calling
`save-buffer' etc., but I wonder if there isn't some standard, simple way to get
this effect.

Thx - Drew

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> >   * Also change the default value of `bookmark-version-control'
> >     to be `nil' instead of `nospecial', so that backups of the 
> >     bookmark data file are no longer on by default (unless there
> >     are already backup files present).
> 
> But how does that help a user turn backup on in the first 
> place?  Not a rhetorical question - I really don't know.
> How should a user create the first backup file?
> 
> What would the doc suggest to the user for that?  Copy the 
> file to one with a `~' suffix (error prone)?  Visit the
> bookmark file, type SPC then DEL, then `C-x C-s' (error prone)?
> 
> What is an easy, sure way for a user who has never backed up 
> a file (one that is not typically visited interactively) to create
> a backup?
> 
> The question is not bookmark-specific.  I don't know a good 
> answer.  It's probably obvious, but I'm not seeing it.

Sorry, I wasn't paying attention.  It's not about creating the first backup
file, but the first numbered backup file.  The question remains (how do users do
that?), but my examples were incorrect.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Stefan Monnier]

>   ;; Don't use write-file; we don't want this buffer to visit it.

After write-file, the buffer is marked as visiting that file, which
affects the behavior of C-x C-f and a lot more (e.g. asks the user
for confirmation if the file was modified by some other process, ...).


        Stefan

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Thierry Volpiatto]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>   ;; Don't use write-file; we don't want this buffer to visit it.
>
> After write-file, the buffer is marked as visiting that file, which
> affects the behavior of C-x C-f and a lot more (e.g. asks the user
> for confirmation if the file was modified by some other process, ...).
What about improving write-region to use backup when needed?
Possibly writing a new write-region-something function that handle
backup, or a write-file-noselect function.

-- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: `bookmark-write-file': use `write-file', not `write-region', to get backups

[Juri Linkov]

> Is your `custom-file' an "internal configuration file"?  Do you back it up?

`custom-file' has the opposite problem: it creates backups forcefully
even when backups should not be created in VC-controlled directories.

There is a bug in `custom-save-all', it binds `print-length'
and `print-level' to nil (but not `print-circle' to t), and
calls `save-buffer' that ignores the VC status and creates backups.

I have no problems with backups for configuration files
like e.g. .recentf~ or .emacs.desktop~ when a VCS is not used,
but while implementing backups for bookmarks please take into account
that it should not create backups in VC-controlled directories.

I guess normal `save-buffer' or `write-file' should take care about this
by creating backups only in non-VC directories, or creating numbered
backups if there are already existing numbered backups.

bug#12507: `bookmark-write-file': use `write-file', not `write-region', to get backups

[Drew Adams]

> while implementing backups for bookmarks please take into account
> that it should not create backups in VC-controlled directories.
> 
> I guess normal `save-buffer' or `write-file' should take care 
> about this by creating backups only in non-VC directories, or
> creating numbered backups if there are already existing numbered
> backups.

What you say in the second sentence sounds like the right approach.
This problem doesn't sound like it is specific to bookmarks.
IOW, it should be the subject of another bug ("wishlist") report.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

"Drew Adams" <drew.adams@oracle.com> writes:
>>
>>   * As Drew suggested, change `bookmark-write-file' to use 
>>     `write-file' instead of `write-region'.
>> 
>>   * Also change the default value of `bookmark-version-control' to be
>>     `nil' instead of `nospecial', so that backups of the bookmark data
>>     file are no longer on by default (unless there are already backup
>>     files present).
>
>But how does that help a user turn backup on in the first place?  Not a
>rhetorical question - I really don't know.  How should a user create the first
>backup file?
>
>What would the doc suggest to the user for that?  Copy the file to one with a
>`~' suffix (error prone)?  Visit the bookmark file, type SPC then DEL, then `C-x
>C-s' (error prone)?
>
>What is an easy, sure way for a user who has never backed up a file
>(one that is not typically visited interactively) to create a backup?

Set `bookmark-version-control' to `t', of course.

Best,
-K

>The question is not bookmark-specific.  I don't know a good answer.  It's
>probably obvious, but I'm not seeing it.
>
>> But... the only thing that makes me hesitate is the first 
>> step, because back in 2005 we changed `bookmark-write-file' to use
>> `write-region':
>> 
>>   2005-11-12  Karl Fogel  <kfogel@red-bean.com>
>>         * bookmark.el (bookmark-write-file): Don't visit the 
>>         destination file, just write the data to it using
>>         write-region.  This is similar to revision 1.32 of
>>         saveplace.el, but with an additional change to avoid
>>         visiting the file in the first place.
>> 
>> The corresponding change in saveplace.el has just this comment:
>> 
>>   ;; Don't use write-file; we don't want this buffer to visit it.
>> 
>> Why didn't we want to visit the file?  Was there some reason why that
>> was a bad thing?  Unfortunately, I don't remember, but I don't want to
>> introduce a regression.
>> 
>> Drew or anyone, any idea what problem we were avoiding?
>
>Sorry, I don't know.  I bisected the change logs from the start, to locate that
>commit as the culprit change.  But I don't know more than what the log says.
>
>Perhaps the reason was what Yidong expressed: a belief that a bookmark file is
>only an "internal configuration file", rather than user data (presumably because
>users do not typically edit it directly).  His contention is that backing up the
>file would annoy users by littering their filesystems.
>
>If that was the rationale for the 2005 change then it was misguided, IMO.
>
>A bookmark file is not just an internal config file.  It contains user data that
>can be valuable (to users).  Among other things, it can contain metadata (e.g.
>annotations) about other files.  It has some things in common with Org mode for
>keeping track of positions and other relations among documents.
>
>Users can make mistakes that lead to losing individual bookmarks that they might
>really want to keep, or even to losing all bookmarks.
>
>In the other direction, it is very easy to load a second bookmark file into your
>main bookmark file and save the result without necessarily meaning to.  To get
>back what you had (by deleting the additions or replacing the replacements) is
>laborious and error prone, unless you have a backup copy.
>
>For such reasons, some users might want to have automatic backup for their
>bookmarks.  I agree that backup should be optional and up to the user, of
>course.
>> The status quo does seem a bug.  There are two fixes: make 
>> backups work again, or deprecate `bookmark-version-control'
>> and don't claim that the bookmark data file can have automatic backups.
>> 
>> (In the meantime, Drew's suggestion in #12503 that `print-circle' be
>> bound to `t' seems right to me -- I'm trying to get outstanding
>> bookmark.el bugs fixed in time for the feature freeze on Oct. 
>> 1 and that should be one of the fixes.  If so, then one of the reasons
>> for being able to back up the bookmarks data file will go away anyway.)
>
>Thank you for that, in advance.
>
>There are however plenty of other ways a user can lose a bookmark file that took
>a long time to construct.  To me, we should not only provide automatic backup
>but turn it on by default.
>
>(Would I apply the same arguments to some other "internal config files"?
>Dunno/depends.  Maybe desktop files.  A lot depends on how important the given
>"config" might be to a user and how long it takes to reconstruct it from
>scratch.  In any case, I don't buy the blanket argument that dot files or
>"internal config files" are necessarily things that a user does not want backed
>up.)
>
>---
>
>I would in any case like to know an answer to my question above about creating
>the first backup.
>
>I also have a question about the idiom to use that would make a code change
>analogous to the write-region --> write-file change discussed, but for
>(write-region (point-min) (point-max 'APPEND), i.e., for appending the buffer
>content to a file.
>
>It's not clear to me what the best way would be to replace that code with code
>that will not only append and write but also back up (if backing up is enabled).
>I can code something up by appending the text to a buffer and then calling
>`save-buffer' etc., but I wonder if there isn't some standard, simple way to get
>this effect.
>
>Thx - Drew

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> >>   * Also change the default value of 
> >>     `bookmark-version-control' to be `nil' instead of
> >>     `nospecial', so that backups of the bookmark data
> >>     file are no longer on by default (unless there are 
> >>     already backup files present).
> >
> > But how does that help a user turn backup on in the first 
> > place?  Not a rhetorical question - I really don't know.
> > How should a user create the first backup file?
> >
> > What would the doc suggest to the user for that?  Copy the 
> > file to one with a `~' suffix (error prone)?  Visit the
> > bookmark file, type SPC then DEL, then `C-x C-s' (error prone)?
> >
> > What is an easy, sure way for a user who has never backed up
> > a file (one that is not typically visited interactively) to
> > create a backup?
> 
> Set `bookmark-version-control' to `t', of course.

OK, I thought of that, but that seems like a strange thing for the doc to
suggest: customize it to `t', save your bookmark file, then re/un-customize it
back to the default, `nil'.

Is that really the best recommendation?  I have no special problem with it, but
somehow I was expecting something else.  

Whatever the recommended procedure is, I think the doc (for this and
`version-control') should suggest it to users.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

"Drew Adams" <drew.adams@oracle.com> writes:
>> Set `bookmark-version-control' to `t', of course.
>
>OK, I thought of that, but that seems like a strange thing for the doc to
>suggest: customize it to `t', save your bookmark file, then re/un-customize it
>back to the default, `nil'.

Sorry?  I'm maybe not understanding your question.

If a user wants numbered backups of their bookmark data file, then they
would set (customize) `bookmark-version-control' to `t'.  If they don't,
then it's nil.  That is: they can leave it as the default (which would
be nil in my proposal) or if they really want to be certain, I suppose
they could explicitly customize it to nil.

>Is that really the best recommendation?  I have no special problem with
>it, but somehow I was expecting something else.

Can you describe what you were expecting?

Controlling numbered backups of the bookmark data file is the whole
reason the variable exists in the first place, so users should expect
that if they want to control that behavior, this variable is the place
to look.  (Of course it should be documented; my point is just that I
can't imagine what *other* mechanism would control this, since the only
reason the current mechanism exists is ... to control exactly this.)

-K

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> >> Set `bookmark-version-control' to `t', of course.
> >
> > OK, I thought of that, but that seems like a strange thing 
> > for the doc to suggest: customize it to `t', save your bookmark
> > file, then re/un-customize it back to the default, `nil'.
> 
> Sorry?  I'm maybe not understanding your question.

My bad.  Please ignore the question.

The values essentially come from `version-control', where what's being decided
is for all files.  Since this is only for the bookmark file there is not a big
need for offering both `nil' and `never' values.  I think that is what confused
me somehow.

The only real choices for the bookmark file are using numbered or unnumbered
backups, and whether to let `version-control' decide that: i.e., values `t',
`never', and `nospecial'.

`nil' (vs `t') has meaning here only if you somehow already had a numbered
backup for the bookmark file.  `nil' makes sense for `version-control', however,
because that option governs multiple files.

If the value here is `nospecial' and the `version-control' value is `nil', then
you get the same result as `never' for the bookmark file, unless you previously
customized `b-v-c' to `t'.  I think that's the case that I was finding
confusing.  I was wondering how, in that case, a user might have created a
numbered backup in the first place.

It's not important.  Sorry for the noise.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> >>   ;; Don't use write-file; we don't want this buffer to visit it.
> >
> > After write-file, the buffer is marked as visiting that file, which
> > affects the behavior of C-x C-f and a lot more (e.g. asks the user
> > for confirmation if the file was modified by some other process, ...).
>
> What about improving write-region to use backup when needed?
> Possibly writing a new write-region-something function that handle
> backup, or a write-file-noselect function.

+1

And please let us know how best to accomplish that (in the doc perhaps, but also
in this thread).

It's not clear to me how to make a backup copy of a file without visiting that
file in some buffer, however temporarily.

For example, I can imagine this as a way to append the region to a file and back
it up:

 (write-region (point-min) (point-max) FILE 'append)
 (with-current-buffer (find-file-noselect FILE) (backup-buffer))

But IIUC `find-file-noselect' visits the buffer (and so "asks the user for
confirmation if the file was modified by some other process").  So that's
apparently not the way to go.  What is?

Leaving the question of visiting aside for the moment, what about
`backup-buffer' here?  Should it be `save-buffer' instead, so that the modes of
FILE get updated properly?  Should it be just `basic-save-buffer-1' instead of
`save-buffer'?

And should any such code take what Juri mentioned wrt vc into account?  If so,
how?

It's not clear to me how best to handle this
write-stuff-to-a-file-and-back-it-up-when-appropriate, but I (and perhaps
others) would like to learn.  I haven't found the answer by looking at the
manuals or perusing the source code.  Can you help?

Thx.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Thierry Volpiatto]

"Drew Adams" <drew.adams@oracle.com> writes:

>> >>   ;; Don't use write-file; we don't want this buffer to visit it.
>> >
>> > After write-file, the buffer is marked as visiting that file, which
>> > affects the behavior of C-x C-f and a lot more (e.g. asks the user
>> > for confirmation if the file was modified by some other process, ...).
>>
>> What about improving write-region to use backup when needed?
>> Possibly writing a new write-region-something function that handle
>> backup, or a write-file-noselect function.
>
> +1
>
> And please let us know how best to accomplish that (in the doc perhaps, but also
> in this thread).
>
> It's not clear to me how to make a backup copy of a file without visiting that
> file in some buffer, however temporarily.
>
> For example, I can imagine this as a way to append the region to a file and back
> it up:
>
>  (write-region (point-min) (point-max) FILE 'append)
>  (with-current-buffer (find-file-noselect FILE) (backup-buffer))
>
> But IIUC `find-file-noselect' visits the buffer (and so "asks the user for
> confirmation if the file was modified by some other process").  So that's
> apparently not the way to go.  What is?
>
> Leaving the question of visiting aside for the moment, what about
> `backup-buffer' here?  Should it be `save-buffer' instead, so that the modes of
> FILE get updated properly?  Should it be just `basic-save-buffer-1' instead of
> `save-buffer'?
>
> And should any such code take what Juri mentioned wrt vc into account?  If so,
> how?
>
> It's not clear to me how best to handle this
> write-stuff-to-a-file-and-back-it-up-when-appropriate, but I (and perhaps
> others) would like to learn.  I haven't found the answer by looking at the
> manuals or perusing the source code.  Can you help?
Will try to look better to this tomorrow if I find time, just for
`bookmark-write-file', what about something like this?

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -1355,7 +1355,7 @@
 (defun bookmark-write-file (file)
   "Write `bookmark-alist' to FILE."
   (bookmark-maybe-message "Saving bookmarks to file %s..." file)
-  (with-current-buffer (get-buffer-create " *Bookmarks*")
+  (with-current-buffer (find-file-noselect file)
     (goto-char (point-min))
     (delete-region (point-min) (point-max))
     (let ((print-length nil)
@@ -1374,7 +1374,7 @@
               ((eq 'nospecial bookmark-version-control) version-control)
               (t t))))
         (condition-case nil
-            (write-region (point-min) (point-max) file)
+            (save-buffer)
           (file-error (message "Can't write %s" file)))
         (kill-buffer (current-buffer))
         (bookmark-maybe-message



-- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Thierry Volpiatto]

"Drew Adams" <drew.adams@oracle.com> writes:

>> >>   ;; Don't use write-file; we don't want this buffer to visit it.
>> >
>> > After write-file, the buffer is marked as visiting that file, which
>> > affects the behavior of C-x C-f and a lot more (e.g. asks the user
>> > for confirmation if the file was modified by some other process, ...).
>>
>> What about improving write-region to use backup when needed?
>> Possibly writing a new write-region-something function that handle
>> backup, or a write-file-noselect function.
>
> +1
>
> And please let us know how best to accomplish that (in the doc perhaps, but also
> in this thread).
>
> It's not clear to me how to make a backup copy of a file without visiting that
> file in some buffer, however temporarily.
>
> For example, I can imagine this as a way to append the region to a file and back
> it up:
>
>  (write-region (point-min) (point-max) FILE 'append)
>  (with-current-buffer (find-file-noselect FILE) (backup-buffer))
This is not efficient IMO, like the actual version of
`bookmark-write-file':

1) create a new buffer named "*Bookmarks*".
2) erase buffer
3) write data to it.
4) write contents of this buffer to FILE.
5) save this FILE.

instead:

1) open FILE buffer
2) erase buffer
3) write data to it
4) save buffer.

> But IIUC `find-file-noselect' visits the buffer (and so "asks the user for
> confirmation if the file was modified by some other process").  So that's
> apparently not the way to go.  What is?
What is the problem for this?
What if you open another emacs session, bookmark something in this
session, (don't save and don't quit session) switch to the initial
session, bookmark something there and save bookmarks?
It is good if it ask you for confirmation at some point, no? 

> Leaving the question of visiting aside for the moment, what about
> `backup-buffer' here?  Should it be `save-buffer' instead, so that the modes of
> FILE get updated properly?  Should it be just `basic-save-buffer-1' instead of
> `save-buffer'?
`save-buffer' do all the job (saving and backing up), so why writing to
buffer and then using `backup-buffer'

> And should any such code take what Juri mentioned wrt vc into account?  If so,
> how?
`save-buffer' handle that.

> It's not clear to me how best to handle this
> write-stuff-to-a-file-and-back-it-up-when-appropriate, but I (and perhaps
> others) would like to learn.  I haven't found the answer by looking at the
> manuals or perusing the source code.  Can you help?

I think that doing like in the patch I sent is not too bad:

(with-current-buffer (find-file-noselect file)
   write--data--here
   [...]
   (save-buffer))

No more is needed I think, but maybe I miss something?

Also, the use of `bookmark-version-control' is questionable, why
handling this file specially?
If following that, we should have a special variable for .emacs-custom.el,
desktop, history, etc... which is non--sense.

If this variable is removed, the global value of version-control will be
used and .emacs.bmk will be backed up like any other file,
like it does (or should do because it is broken with write-region) with
its default value 'nospecial.

Note that with the patch I sent, it seems a little bit faster to save,
but maybe I am wrong, need to verify. (we write one time the data and
save instead of writing two time and save)

-- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> > > What about improving write-region to use backup when needed?
> > > Possibly writing a new write-region-something function that
> > > handle backup, or a write-file-noselect function.
> > 
> > +1
> >
> > And please let us know how best to accomplish that (in the 
> > doc perhaps, but also in this thread).

Anyone?

> > It's not clear to me how to make a backup copy of a file 
> > without visiting that file in some buffer, however temporarily.

Stefan?  You were the one who pointed out why there was the change here to
`write-region' from `write-file'.  And you seemed to be suggesting that we
should still avoid `write-file' for that reason.

> > For example, I can imagine this as a way to append the 
> > region to a file and back it up:
> >
> >  (write-region (point-min) (point-max) FILE 'append)
> >  (with-current-buffer (find-file-noselect FILE) (backup-buffer))
>
> This is not efficient IMO,

No, of course not.  That's one reason I asked for a good way to do it.

My question for the example here was for the APPEND case of `write-region',
which has no equivalent wrt `write-file'.  But I certainly agree that if we're
going to visit the file anyway, then it is better to write to disk only once.
For that I have no problem with

(with-current-buffer (find-file-noselect FILE)
  ; erase if replacing, or leave in place if appending
  ; insert new (at end, if appending)
  (save-buffer))

> instead: 1) open FILE buffer 2) erase buffer
> 3) write data to it 4) save buffer.

Right.  Sounds good to me.  The question is open wrt visiting the file, however.

> > But IIUC `find-file-noselect' visits the buffer (and so 
> > "asks the user for confirmation if the file was modified
> > by some other process").  So that's apparently not the way
> > to go.  What is?
>
> What is the problem for this?

I was quoting Stefan.  He mentioned this problem as the reason for the 2005
change from the original `write-file' to the current `write-region'.  His point
was that we should avoid _visiting_ the file, for the reasons he gave.

My question is how to create a backup if you do not visit the file.

> What if you open another emacs session, bookmark something in this
> session, (don't save and don't quit session) switch to the initial
> session, bookmark something there and save bookmarks?
> It is good if it ask you for confirmation at some point, no?
> 
> > Leaving the question of visiting aside for the moment, what about
> > `backup-buffer' here?  Should it be `save-buffer' instead, 
> > so that the modes of FILE get updated properly?  Should it be just 
> > `basic-save-buffer-1' instead of `save-buffer'?
>
> `save-buffer' do all the job (saving and backing up), so why 
> writing to buffer and then using `backup-buffer'
> 
> > And should any such code take what Juri mentioned wrt vc 
> > into account?  If so, how?
>
> `save-buffer' handle that.

`save-buffer' means visiting the file first.  As do all of the other, more
partial, code combinations that might back up (`backup-buffer' etc.), AFAICT.

> > It's not clear to me how best to handle this
> > write-stuff-to-a-file-and-back-it-up-when-appropriate, but 
> > I (and perhaps others) would like to learn.  I haven't found
> > the answer by looking at the manuals or perusing the source
> > code.  Can you help?
> 
> I think that doing like in the patch I sent is not too bad:
> (with-current-buffer (find-file-noselect file)
>    write--data--here[...] (save-buffer))
> 
> No more is needed I think, but maybe I miss something?

You are missing Stefan's point about not visiting the file.

> Also, the use of `bookmark-version-control' is questionable, why
> handling this file specially?  If following that, we should have
> a special variable for .emacs-custom.el,
> desktop, history, etc... which is non--sense.

No, it's not nonsense.  Where I agree with you is that there can also be other
places, besides bookmarking, where you might want what `version-control' does
only in a blanket way to be done differently for specific files.

That was the aim, I assume, behind creating `bookmark-version-control' in the
first place.  It might be better to have a more general or central way to
specify this, rather than having additional user options for different files or
libraries.

(In Bookmark+, `bookmark-version-control' governs not only the bookmark file but
also two other bookmarking files: Bmenu state and user-generated command
definitions.)

One approach might be to generalize (or particularize, depending on your point
of view) `version-control', so that users can use it to also specify individual
behaviors for specific files or file-name regexp matches.

We could either modify `version-control' itself to get that effect or add a new
option like this to do it:

(defcustom version-control-overrides ()
  "Control the use of version numbers for backing up specific files.
Each entry is of the form (REGEXP-OR-VARIABLE . VALUE), where:
REGEXP-OR-VARIABLE is a regexp matching file names or the name of a
 file name-valued variable.
VALUE has the same meaning as the value of option `version-control,
 but affects only the files whose names match REGEXP."
  :type '(repeat (cons :tag "File & when"
                  (choice
                   (regexp   :tag "File-name regexp")
                   (variable :tag "File-name variable"))
                  (choice
                   (const :tag "Never"       never)
                   (const :tag "If existing" nil)
                   (other :tag "Always"      t))))
  :group 'backup :group 'vc)

The latter sounds better to me.  Then, to have specific control for your
bookmark file you would just add an entry like this: (bookmark-file . t).

(BTW, bug #12533 concerns the incorrect behavior of Customize for such a
defcustom.)

> If this variable is removed, the global value of 
> version-control will be used and .emacs.bmk will be backed
> up like any other file,

But that might not be what you want.  That's the point, presumably, of giving
you specific control for your bookmark file.  But I would be OK with removing
variable `bookmark-version-control' if a more general variable such as
`version-control-overrides' were added.

> like it does (or should do because it is broken with 
> write-region) with its default value 'nospecial.
> 
> Note that with the patch I sent, it seems a little bit faster to save,
> but maybe I am wrong, need to verify. (we write one time the data and
> save instead of writing two time and save)

See above.  Of course it is faster to write to disk only once.  The question
about visiting the file is still open, though.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Thierry Volpiatto]

"Drew Adams" <drew.adams@oracle.com> writes:

> See above.  Of course it is faster to write to disk only once.  The question
> about visiting the file is still open, though.
I am using now the patch I sent here yesterday and it works really good,
faster and do backups (numered) as expected.
Hope it will be applied here in emacs because it DTRT.

I don't understand what is the problem with "visiting the file".
See in precedent post why it is not bad visiting the file.
In the special case of bookmark-write-file, it is really not the
problem.

-- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> > The question about visiting the file is still open, though.
>
> I am using now the patch I sent here yesterday and it works 
> really good, faster and do backups (numered) as expected.
> Hope it will be applied here in emacs because it DTRT.
> 
> I don't understand what is the problem with "visiting the file".
> See in precedent post why it is not bad visiting the file.
> In the special case of bookmark-write-file, it is really not the
> problem.

Your question is for Stefan.

Your patch is equivalent to the change I proposed originally: just replace
`write-region' with `write-file'.  Of course it works.  It is what I have been
using, with no problem.

But it has the other effects that Stefan mentioned, since the file is visited.
He seems to be objecting to that.  But it's not too clear - perhaps he was just
relating why the change from `write-file' was made, without expressing any
opinion.

And we have not heard anything from him in reply to my statement that there
seems to be no way to make a backup without also visiting the file.  If that
guess is correct then the choices would seem to be:

1. Provide for optional backups, visiting the file systematically.

2. Do not provide for optional backups, and never visit the file.

3. Provide for optional backups, but if the user chooses not to back up, then do
not visit the file.

With #3, the user would pay the price that Stefan mentions for visiting the file
only when s?he chooses backup.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Thierry Volpiatto]

"Drew Adams" <drew.adams@oracle.com> writes:

>> > The question about visiting the file is still open, though.
>>
>> I am using now the patch I sent here yesterday and it works 
>> really good, faster and do backups (numered) as expected.
>> Hope it will be applied here in emacs because it DTRT.
>> 
>> I don't understand what is the problem with "visiting the file".
>> See in precedent post why it is not bad visiting the file.
>> In the special case of bookmark-write-file, it is really not the
>> problem.
>
> Your question is for Stefan.
>
> Your patch is equivalent to the change I proposed originally: just replace
> `write-region' with `write-file'.
No, this is ineficient too, you write twice the same data.
The important thing is writing directly to the buffer of file.
For the backup thing, yes it is similar, but with unneeded steps,
going straight to save-buffer is better and faster IMO (of course if you
have started writing data in the file buffer)

But the worst thing is the actual version with write-region:

Slow and backup broken.

 -- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> >> > The question about visiting the file is still open, though.
> >>
> >> I am using now the patch I sent here yesterday and it works 
> >> really good, faster and do backups (numered) as expected.
> >> Hope it will be applied here in emacs because it DTRT.
> >> 
> >> I don't understand what is the problem with "visiting the
> >> file". See in precedent post why it is not bad visiting the
> >> file. In the special case of bookmark-write-file, it is
> >> really not the problem.
> >
> > Your question is for Stefan.  Your patch is equivalent to
> > the change I proposed originally: just replace
> > `write-region' with `write-file'.
>
> No, this is ineficient too, you write twice the same data.

How so?  What am I missing?  What part of `write-file' writes the same data
twice?  All I see in the `write-file' definition, in terms of writing, is a call
to `save-buffer'.

> The important thing is writing directly to the buffer of file.
> For the backup thing, yes it is similar, but with unneeded steps,

Steps that you seem to claim constitute an additional disk write.  I don't see
that.  What part of `write-file' performs an extra disk write?

The only "extra" steps I see in `write-file' are setting the visited file name,
setting the buffer status to modified, checking that the file is
`file-writable-p', and setting `buffer-read-only' to nil.  And running
`vc-find-file-hook'.

You are, I think, side-tracking the issue a bit.  The question to be decided is
whether to allow backups.  It is not whether to use `write-file', `save-buffer',
`basic-save-buffer', or something else.  I don't really care exactly how it's
done.  I have confidence it will be done efficiently if it is decided to be
done.

> going straight to save-buffer is better and faster IMO (of 
> course if you have started writing data in the file buffer)
> 
> But the worst thing is the actual version with write-region:
> Slow and backup broken.

I don't see that the current version is slow, either.  But it certainly does not
provide for backing up.  That is the question to be decided.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Thierry Volpiatto]

"Drew Adams" <drew.adams@oracle.com> writes:

> How so?  What am I missing?  What part of `write-file' writes the same data
> twice?  All I see in the `write-file' definition, in terms of writing, is a call
> to `save-buffer'.
What I want to say is, why writing to buffer "*Bookmarks*", renaming
this buffer to the filename buffer, and then saving it?
Just use find-file-noselect, write data and save.

-- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> What I want to say is, why writing to buffer "*Bookmarks*", renaming
> this buffer to the filename buffer, and then saving it?
> Just use find-file-noselect, write data and save.

I see.  You're talking only about the first arg of `with-current-buffer'.  I
thought you were talking about `write-file' vs `save-buffer' and claiming that
there were two writes with the former.

Yes, of course.  There is no need to use a different buffer, *Bookmarks*,
instead of the return value of `find-file-noselect'.  I thought we had already
covered that.

There still might be a reason to use `write-file' instead of `save-buffer'.  But
as I say, what's important is to decide about whether to allow backing up (and
visiting the file).

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Thierry Volpiatto]

"Drew Adams" <drew.adams@oracle.com> writes:

>> What I want to say is, why writing to buffer "*Bookmarks*", renaming
>> this buffer to the filename buffer, and then saving it?
>> Just use find-file-noselect, write data and save.
>
> I see.  You're talking only about the first arg of `with-current-buffer'.  I
> thought you were talking about `write-file' vs `save-buffer' and claiming that
> there were two writes with the former.
>
> Yes, of course.  There is no need to use a different buffer, *Bookmarks*,
> instead of the return value of `find-file-noselect'.  I thought we had already
> covered that.
>
> There still might be a reason to use `write-file' instead of `save-buffer'.  But
> as I say, what's important is to decide about whether to allow backing up (and
> visiting the file).
Agree.


-- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: Option `(bookmark-)version-control': Use :tag so doc string matches menu

[Karl Fogel]

Fixed (see below), but please review.  

I don't fully understand the whole customization system because I never
use it myself (I just read doc strings and set variables directly in
Elisp), so I don't quite get what `other' mean if used instead of
`const', and I didn't fully understand the last paragraph of the
original bug report.  I looked in the Info pages, but they didn't
clarify much about this.

-Karl

Revision info:

  revno: 110305
  revision-id: kfogel@red-bean.com-20121001033206-5eja4ztyhs1sjm7q
  parent: cyd@gnu.org-20121001031702-2mei04wuzv2pk1e7
  committer: Karl Fogel <kfogel@red-bean.com>
  branch nick: trunk
  timestamp: Sun 2012-09-30 22:32:06 -0500
  message:
    * lisp/bookmark.el (bookmark-version-control): Give tags in the
      :type choices (Bug#12309), and improve doc string.

Diff:

  === modified file 'lisp/ChangeLog'
  --- lisp/ChangeLog	2012-10-01 02:07:14 +0000
  +++ lisp/ChangeLog	2012-10-01 03:31:41 +0000
  @@ -1,3 +1,8 @@
  +2012-10-01  Karl Fogel  <kfogel@red-bean.com>
  +
  +	* bookmark.el (bookmark-version-control): Give tags in the
  +	:type choices (Bug#12309), and improve doc string.
  +
   2012-10-01  Paul Eggert  <eggert@cs.ucla.edu>
   
   	Revert the FOLLOW-SYMLINKS change for file-attributes.
  
  === modified file 'lisp/bookmark.el'
  --- lisp/bookmark.el	2012-09-25 04:13:02 +0000
  +++ lisp/bookmark.el	2012-10-01 03:32:18 +0000
  @@ -99,12 +99,14 @@
   
   (defcustom bookmark-version-control 'nospecial
     "Whether or not to make numbered backups of the bookmark file.
  -It can have four values: t, nil, `never', and `nospecial'.
  +It can have four values: t, nil, `never', or `nospecial'.
   The first three have the same meaning that they do for the
  -variable `version-control', and the final value `nospecial' means just
  -use the value of `version-control'."
  -  :type '(choice (const nil) (const never) (const nospecial)
  -		 (other t))
  +variable `version-control'; the value `nospecial' (the default) means
  +just use the value of `version-control'."
  +  :type '(choice (const :tag "If existing" nil)
  +                 (const :tag "Never" never)
  +                 (const :tag "Use the value of `version-control'" nospecial)
  +                 (const :tag "Always" t))
     :group 'bookmark)

bug#12507: Option `(bookmark-)version-control': Use :tag so docstring matches menu

[Drew Adams]

Ouch!

You fixed bug #12309, Karl (thanks; looks good).  But you closed bug #12507
instead.

Please reopen #12507 and correct the subject lines etc. of the mails if needed,
so that things are put right again.  It is #12309 that should be closed, not
@12507.

To answer your questions:

`other' in a `choice' just means any value other than the other choices listed.
If the user picks the `other' choice interactively then the value given (`t'
here) is used.  E.g., if you set the variable to 99999 (outside of Customize)
then it will act just like a value of `t'.

The last sentence of the original report just meant to please at least use a
:tag for the `other' choice.  It is the one where a :tag is most important for
clarity.  But you've added :tag for each of them, which is even better.

Thx - Drew

P.S. FWIW, here is what I've been using for this defcustom.  It's almost the
same as what you have.

"Whether to make numbered backups of your bookmarking files.
The option can have value `nospecial', `t', `nil', or `never' .  Value
`nospecial' means to use the `version-control' value.  The others have
the same meanings as for option `version-control'.

Use value `t' if your bookmarks are important to you.  Consider also
using numeric backups.  See also nodes `Backup Names' and `Backup
Deletion' in the Emacs manual."

  :type '(choice :tag "When to make numbered backups"
          (const :tag "Use value of option `version-control'" nospecial)
          (const :tag "Never"                                 never)
          (const :tag "If existing already"                   nil)
          (other :tag "Always"                                t))

> From: Karl Fogel Sent: Sunday, September 30, 2012 8:38 PM
> To: 12507-done@debbugs.gnu.org
> Subject: bug#12507: Option `(bookmark-)version-control': Use 
> :tag so docstring matches menu
> 
> Fixed (see below), but please review.  
> 
> I don't fully understand the whole customization system 
> because I never
> use it myself (I just read doc strings and set variables directly in
> Elisp), so I don't quite get what `other' mean if used instead of
> `const', and I didn't fully understand the last paragraph of the
> original bug report.  I looked in the Info pages, but they didn't
> clarify much about this.
> 
> -Karl
> 
> Revision info:
> 
>   revno: 110305
>   revision-id: kfogel@red-bean.com-20121001033206-5eja4ztyhs1sjm7q
>   parent: cyd@gnu.org-20121001031702-2mei04wuzv2pk1e7
>   committer: Karl Fogel <kfogel@red-bean.com>
>   branch nick: trunk
>   timestamp: Sun 2012-09-30 22:32:06 -0500
>   message:
>     * lisp/bookmark.el (bookmark-version-control): Give tags in the
>       :type choices (Bug#12309), and improve doc string.
> 
> Diff:
> 
>   === modified file 'lisp/ChangeLog'
>   --- lisp/ChangeLog	2012-10-01 02:07:14 +0000
>   +++ lisp/ChangeLog	2012-10-01 03:31:41 +0000
>   @@ -1,3 +1,8 @@
>   +2012-10-01  Karl Fogel  <kfogel@red-bean.com>
>   +
>   +	* bookmark.el (bookmark-version-control): Give tags in the
>   +	:type choices (Bug#12309), and improve doc string.
>   +
>    2012-10-01  Paul Eggert  <eggert@cs.ucla.edu>
>    
>    	Revert the FOLLOW-SYMLINKS change for file-attributes.
>   
>   === modified file 'lisp/bookmark.el'
>   --- lisp/bookmark.el	2012-09-25 04:13:02 +0000
>   +++ lisp/bookmark.el	2012-10-01 03:32:18 +0000
>   @@ -99,12 +99,14 @@
>    
>    (defcustom bookmark-version-control 'nospecial
>      "Whether or not to make numbered backups of the bookmark file.
>   -It can have four values: t, nil, `never', and `nospecial'.
>   +It can have four values: t, nil, `never', or `nospecial'.
>    The first three have the same meaning that they do for the
>   -variable `version-control', and the final value 
> `nospecial' means just
>   -use the value of `version-control'."
>   -  :type '(choice (const nil) (const never) (const nospecial)
>   -		 (other t))
>   +variable `version-control'; the value `nospecial' (the 
> default) means
>   +just use the value of `version-control'."
>   +  :type '(choice (const :tag "If existing" nil)
>   +                 (const :tag "Never" never)
>   +                 (const :tag "Use the value of 
> `version-control'" nospecial)
>   +                 (const :tag "Always" t))
>      :group 'bookmark)

bug#12507: Have I mentioned how much I hate Debbugs?

[Karl Fogel]

So, immediately after I mis-closed this report, I realized what happened
and ever since then I've been trying to reopen it.

First I emailed "12507-open@".  Then I tried "12507-reopen@".  Maybe
those emails are stuck in the pipe somewhere, or maybe those weren't
valid ways to issue the command to reopen.  In any case, now I'm reduced
to sending this mail and praying that it goes through, since unlike
every other bug tracker created since 1852 debbugs does not have a way
to manipulate bugs via the web browser.

In any case:

This bug is too complex to solve by the Oct. 1st freeze, sorry.  Just
reading & comprehending the conversation takes twenty minutes now.  My
preference would be to remove the `bookmark-version-control' variable
entirely, since probably so few people use it and it's now a bug source.
However, maybe it's important enough to keep, in which case we probably
need to fix `write-region' to DTRT with backups, or at least have some
kind of `write-region-make-backups' variable we can dynamically bind.

I don't want to revert to using `write-file'.  We switched *away* from
`write-file' for a reason.  Going back will probably mean a regression
of some sort.

-Karl

bug#12507: Have I mentioned how much I hate Debbugs?

[Drew Adams]

> So, immediately after I mis-closed this report, I realized 
> what happened and ever since then I've been trying to reopen it.
> 
> First I emailed "12507-open@".  Then I tried "12507-reopen@".  Maybe
> those emails are stuck in the pipe somewhere, or maybe those weren't
> valid ways to issue the command to reopen.

I don't think those are valid ways.

> In any case, now I'm reduced to sending this mail and praying that
> it goes through, since unlike every other bug tracker created
> since 1852 debbugs does not have a way to manipulate bugs via the
> web browser.

;-)

I just sent a reopen message, which looks like it worked.

I too know little about Debbugs.  For this, all you need to do is to send a
message to 'control@debbugs.gnu.org' with this in the message body (without
quotes): "reopen 12507".


> In any case:
> This bug is too complex to solve by the Oct. 1st freeze, sorry.

It's not complex, IMO, but it definitely won't be fixed by Oct 1!  I think we
need to wait for Stefan to weigh in on the question, for one thing.

> Just reading & comprehending the conversation takes twenty minutes
> now.  My preference would be to remove the `bookmark-version-control'
> variable entirely, since probably so few people use it and it's now a 
> bug source.

I disagree with that, but I can keep a local version if you decide to do that.

What I would prefer is a general solution, along the lines I suggested (in my
mail of 9/28): extend the general `version-control' to let users specify backup
for particular files.  I proposed adding an option like this, as one way to do
that:

(defcustom version-control-overrides ()
  "Control the use of version numbers for backing up specific files.
Each entry is of the form (REGEXP-OR-VARIABLE . VALUE), where:
REGEXP-OR-VARIABLE is a regexp matching file names or the name of a
 file name-valued variable.
VALUE has the same meaning as the value of option `version-control,
 but affects only the files whose names match REGEXP."
  :type '(repeat (cons :tag "File & when"
                  (choice
                   (regexp   :tag "File-name regexp")
                   (variable :tag "File-name variable"))
                  (choice
                   (const :tag "Never"       never)
                   (const :tag "If existing" nil)
                   (other :tag "Always"      t))))
  :group 'backup :group 'vc)

Then, to handle the file that is the value of variable `bookmark-file' you would
just add an entry like this: (bookmark-file . t).

> However, maybe it's important enough to keep, in which case 
> we probably need to fix `write-region' to DTRT with backups,
> or at least have some kind of `write-region-make-backups'
> variable we can dynamically bind.
> 
> I don't want to revert to using `write-file'.  We switched *away* from
> `write-file' for a reason.  Going back will probably mean a regression
> of some sort.

We could do what I suggested in my message of 9/29:

d> 3. Provide for optional backups, but if the user chooses not
d>    to back up, then do not visit the file.
d>
d> With #3, the user would pay the price that Stefan mentions for
d> visiting the file only when s?he chooses backup.

I based that on my understanding (still asking the question though, since I'm
not sure) that you cannot back up the file unless you visit it.  Stefan's
objection, and the reason we moved away from `write-file', is that a user might
not want to visit the file, since that has some additional effects (e.g. asking
for confirmation if some other process modified the file).

But those effects are anyway desirable, IF you want to back up the file.  So it
seems to me that what we want is to either (a) visit the file and do
`save-buffer' or `write-file or equivalent IF the option value says to back up
the file, or (b) do what we do not IF NOT.

In any case, it sounds like we have all agreed at least on the need of a way for
a user to say whether or not s?he wants backups.  `bookmark-version-control'
does not do that - it controls only whether to use ordinary backups or numeric
backups.  So I think the first step is to add an option so that a user can
express that choice.

bug#12507: Have I mentioned how much I hate Debbugs?

[Karl Fogel]

"Drew Adams" <drew.adams@oracle.com> writes:
>What I would prefer is a general solution, along the lines I suggested (in my
>mail of 9/28): extend the general `version-control' to let users specify backup
>for particular files.  I proposed adding an option like this, as one way to do
>that:
>
>(defcustom version-control-overrides ()
>  "Control the use of version numbers for backing up specific files.
>Each entry is of the form (REGEXP-OR-VARIABLE . VALUE), where:
>REGEXP-OR-VARIABLE is a regexp matching file names or the name of a
> file name-valued variable.
>VALUE has the same meaning as the value of option `version-control,
> but affects only the files whose names match REGEXP."
>  :type '(repeat (cons :tag "File & when"
>                  (choice
>                   (regexp   :tag "File-name regexp")
>                   (variable :tag "File-name variable"))
>                  (choice
>                   (const :tag "Never"       never)
>                   (const :tag "If existing" nil)
>                   (other :tag "Always"      t))))
>  :group 'backup :group 'vc)
>
>Then, to handle the file that is the value of variable `bookmark-file' you would
>just add an entry like this: (bookmark-file . t).

I like this general solution.

>We could do what I suggested in my message of 9/29:
>
>d> 3. Provide for optional backups, but if the user chooses not
>d>    to back up, then do not visit the file.
>d>
>d> With #3, the user would pay the price that Stefan mentions for
>d> visiting the file only when s?he chooses backup.
>
>I based that on my understanding (still asking the question though, since I'm
>not sure) that you cannot back up the file unless you visit it.  Stefan's
>objection, and the reason we moved away from `write-file', is that a user might
>not want to visit the file, since that has some additional effects (e.g. asking
>for confirmation if some other process modified the file).

One thing I'm confused by:

Why does backing up a file have anything to do with visiting it?
Backing up just means making a copy.  There is no reason why visiting
the file in a buffer is necessary for that (surely `copy-file' does not
visit the file, for example).

Yet in this discussion, the assumption is that to get backups, we have
to also visit the file.

>But those effects are anyway desirable, IF you want to back up the
>file.  So it seems to me that what we want is to either (a) visit the
>file and do `save-buffer' or `write-file or equivalent IF the option
>value says to back up the file, or (b) do what we do not IF NOT.

Hmm, this feels like a workaround.  Instead, let's get to the bottom of
why backing up and visiting are linked at all.

>In any case, it sounds like we have all agreed at least on the need of
>a way for a user to say whether or not s?he wants backups.
>`bookmark-version-control' does not do that - it controls only whether
>to use ordinary backups or numeric backups.  So I think the first step
>is to add an option so that a user can express that choice.

Yes, but...

Is it worth it to have even `bookmark-version-control' at all?  The
number of people who need backups on this file must be small, since most
users presumably do not edit it directly nor even know where it is.

A more general solution might be `bookmark-before-save-hook'.  The few
people who want backups can DTRT in the hook, and bookmark's code
wouldn't need to worry about this at all.

­K

bug#12507: Have I mentioned how much I hate Debbugs?

[Drew Adams]

> >We could do what I suggested in my message of 9/29:
> >
> >d> 3. Provide for optional backups, but if the user chooses not
> >d>    to back up, then do not visit the file.
> >d>
> >d> With #3, the user would pay the price that Stefan mentions for
> >d> visiting the file only when s?he chooses backup.
> >
> >I based that on my understanding (still asking the question 
> >though, since I'm not sure) that you cannot back up the file
> >unless you visit it.
>
> One thing I'm confused by:
> 
> Why does backing up a file have anything to do with visiting it?

Dunno.  That's why I wrote "still asking the question though".  And I've asked
Stefan several times now (a) whether it is the case that you cannot back up
without visiting and (b) if so, why.  No response, so far.

> Backing up just means making a copy.

Not exactly.  There is also a certain amount of checking and possibly
interaction with the user depending on various states.

> There is no reason why visiting the file in a buffer is necessary
> for that (surely `copy-file' does not visit the file, for example).

But the code for "backing up" (i.e., doing all that is necessary for that) seems
to be in `save-buffer'.  I don't see see any of that logic (checking this and
that) done elsewhere.

This kind of comes back to Thierry's suggestion that we might want to come up
with a version of `write-region', which does not visit the file it writes to,
that also backs up that file first.  Or to do something similar.

IOW, I don't see how to do it currently, other than visiting the file.

> Yet in this discussion, the assumption is that to get backups, we have
> to also visit the file.

I'm the one who said that.  But I am not sure.  That's the conclusion I came to
by looking around the code.  But I'm no expert at all.  And no one has corrected
my conclusion.

> >But those effects are anyway desirable, IF you want to back up the
> >file.  So it seems to me that what we want is to either (a) visit the
> >file and do `save-buffer' or `write-file or equivalent IF the option
> >value says to back up the file, or (b) do what we do not IF NOT.
> 
> Hmm, this feels like a workaround.  Instead, let's get to the 
> bottom of why backing up and visiting are linked at all.

Feel free to investigate.  My guess is that Stefan probably has the answer and
perhaps a simple solution.

> >In any case, it sounds like we have all agreed at least on 
> >the need of a way for a user to say whether or not s?he wants
> >backups.  `bookmark-version-control' does not do that - it controls 
> >only whether to use ordinary backups or numeric backups.  So I
> >think the first step is to add an option so that a user can
> >express that choice.
> 
> Yes, but...
> 
> Is it worth it to have even `bookmark-version-control' at all?
> The number of people who need backups on this file must be small, 

I think _most_ users should back it up.  I think the same for a user's
`custom-file' and for other user-data files that are typically not edited
directly.

The point is that users can make changes to such files, albeit indirectly, and
they can later wish that they had a previous version to revert to.

> since most users presumably do not edit it directly nor even know
> where it is.

See previous.  The directly vs indirectly difference is a red herring, IMO.  You
can easily change the file - you can even delete it.  It does not matter much
whether you do that by editing it directly, interactively, or by issuing a
general command that makes a change to it.

The real question - for a given user - is whether what is in the file is
important to that user and whether it would be difficult or take too long to
re-create it from scratch.

And also whether s?he might want to control/check/compare such changes
incrementally - January vs July versions, for instance, instead of all or
nothing - starting again from scratch.

It's about the cost of re-creation and the difficulty of knowing how to do that
- just what to re-create, and how to re-create the contexts that allow that
bookmark re-creation.

And yes, different users will have different answers.  People can use bookmarks
very differently.

> A more general solution might be `bookmark-before-save-hook'.  The few
> people who want backups can DTRT in the hook, and bookmark's code
> wouldn't need to worry about this at all.

I don't think it is (should be) only a few people.  Personally, I would suggest
turning numeric backups for this on by default.  Apparently I'm alone in that
opinion.

But the only argument given so far against this is the presumed "annoyance" of
users by "littering the filesystem".  That, to me, is presumptuous indeed.

Far better to, by default, protect user data, and let users opt out of that
default protection (backing up).

But we can agree to disagree.

bug#12507: Have I mentioned how much I hate Debbugs?

[Thierry Volpiatto]

"Drew Adams" <drew.adams@oracle.com> writes:

>
> This kind of comes back to Thierry's suggestion that we might want to come up
> with a version of `write-region', which does not visit the file it writes to,
> that also backs up that file first.  Or to do something similar.
Just to clarify (I am not sure what mean "come up"):

My final proposition is:

(with-current-buffer (find-file-no-select FILE) ; [1]
  ;; 2) erase-buffer
  ;; 3) write DATA => bookmark-alist
  ;; 4) let-bound version-control to bookmark-version-control
  (save-buffer) [5]

In [1] I still not understanding what is this paranoia about "visiting
file" specially for this file that no body want to edit manually:
bookmark save bookmark-alist in file in two different ways:
- Immidiately when you bookmark something.
- When emacs quit if bookmark-alist have been modified.
So the file should never hang in a non--saved state at any point.

In [4] if you remove bookmark-version-control, this don't mean the file
will not be backed up, hopefully `version-control' will do its job.
But it doesn't arm to keep bookmark-version-control.

In [5], if you use [1] (i.e find-file-no-select) you don't want to use
`write-file' because it use `set-visited-file-name'; you don't need it
because find-file-no-select do it, so save-buffer is enough.

So I don't understand why there is an interminable discussion on such
simple changes.

-- 
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Lars Ingebrigtsen]

Chong Yidong <cyd@gnu.org> writes:

> "Drew Adams" <drew.adams@oracle.com> writes:
>
>> Am I wrong that `write-region' does not provide for backups?
>> If not, then this is clearly a bug, and a pretty serious one, IMO.
>
> It would be annoying litter the filesystem with backups for every
> internal configuration file.  The abbrev file and desktop file are not
> backed up either, and I think that's fine.
>
> I wouldn't mind adding a global feature to optionally enable backups for
> such files.

Indeed; data files are normally not backed up, and the bookmark file is
one such file.  So I'm closing this bug report.

If somebody wants a global "make write-region make backup files"
(possibly on a per-mode basis?), then a new bug report should be opened.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> >> Am I wrong that `write-region' does not provide for backups?
> >> If not, then this is clearly a bug, and a pretty serious one, IMO.
> >
> > It would be annoying litter the filesystem with backups for every
> > internal configuration file.  The abbrev file and desktop file are not
> > backed up either, and I think that's fine.
> >
> > I wouldn't mind adding a global feature to optionally enable backups for
> > such files.
> 
> Indeed; data files are normally not backed up, and the bookmark file is
> one such file.  So I'm closing this bug report.
> 
> If somebody wants a global "make write-region make backup files"
> (possibly on a per-mode basis?), then a new bug report should be opened.

No, `write-region' should not, itself, make a backup file.

It's not blanket either-everything-or-nothing.  Some
"data" files deserve backup; some don't.

A bookmark file definitely does deserve it.  It's
typically updated frequently (each time a bookmark
is visited).

Emacs goes out of its way to protect user data.  Why
now we would think that only "code" or non-"data"
files deserve backing up, I can't imagine.

Some "data" files are quite important.  A bookmark
file is one such.  `bookmark-write-file' should
use `write-file', not `write-region'.  I made that
change in Bookmark+ 8 years ago.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> No, `write-region' should not, itself, make a backup file.

No, but there should perhaps be a global mechanism for modes to decide
whether the data it writes should have backup files, and these modes
should then call a wrapper around `write-region' that does this stuff.

And it should be user-customisable, if it exists.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> > No, `write-region' should not, itself, make a backup file.
> 
> No, but there should perhaps be a global mechanism for modes to decide
> whether the data it writes should have backup files, and these modes
> should then call a wrapper around `write-region' that does this stuff.
> 
> And it should be user-customisable, if it exists.

Huh?  This is about a particular context (mode, if
you like).  In _this particular context_ the proper
behavior is to use `write-file'.  There's zero reason
to use `write-region' in this context.  And to fix
this context in this regard there's zero reason to
propose some far-reaching, general change to
`write-region'.

`write-region' is simply the wrong function to use
in this context.  Trying to generalize this for all
contexts is a different solution (in search of a
problem).

The subject of this bug report is:
"`bookmark-write-file': use `write-file', not
`write-region' , to get backups"

To be clear, if this isn't clear already: This
doesn't affect me or my code, personally, because
I fixed this long ago in Bookmark+.  I'm just
trying to help vanilla Emacs DTRT.

There's a real bug here, which I can attest to
because Bookmark+ users have lamented not having
backed up their bookmark files (before I fixed the
code to use `write-file').

There's no reason to expect or require users to
manually back up their bookmark files, especially
when it's trivial for Emacs itself to DTRT and
provide backup by default.  By _default_.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

I've just been deep-diving into this again.  I'd like feedback on this general plan for a solution:

1) In files.el: define a new function `back-up-file' [a] that takes a
   file name as argument and does most of what `backup-buffer' currently
   does -- basically, everything from the second `when' on down to the
   end of the function.

2) Change `backup-buffer' to use the new function `back-up-file'.

   In other words, we abstract out most of the Emacs file backup
   functionality to this new function `back-up-file' so we can call it
   in (3) below, passing a file name argument.

3) In bookmark.el, in `bookmark-write-file', call `back-up-file' right
   before the `write-region' call.  (Note that the ambient value of
   `version-control' will already be correct at this point.)

Before I make the patch, I wanted to propose the general idea here and see if it seems okay to others.

This way we would still use `write-region' and avoid visiting the file.  Even though we don't know exactly why Richard made the original motivating change (commit cfde584f6d, see [b] below), there was some problem with `write-file' -- so we shouldn't just go back to that, because a regression would be likely.

Thoughts on this plan?

Best regards,
-Karl

[a] Yes, I know I've inserted a hyphen into "back-up" in that function
    name.  That's because in English "back up" is a compound verb,
    whereas "backup" as one word is a noun.  The existing function name
    "backup-buffer" is thus grammatically suspect, but I'm not proposing
    to fix that here; I'm just trying to avoid repeating the confusion.
    If folks would prefer "backup-file" for consistency, I could do
    that, or maybe just find another name.  The details of the function
    names are not the point here -- I'm just looking for feedback on the
    general plan.

[b] Commit cfde584f6d:

    ===============================================
    commit cfde584f6d25db8c0ba229ebb85fba60e99eb1af
    Author:     Richard M. Stallman <rms@gnu.org>
    AuthorDate: Sun May 29 08:36:26 2005 +0000
    Commit:     Richard M. Stallman <rms@gnu.org>
    CommitDate: Sun May 29 08:36:26 2005 +0000
    
        (save-place-alist-to-file): Write the file using write-region.
    
    M	lisp/saveplace.el
    ===============================================
    --- lisp/saveplace.el
    +++ lisp/saveplace.el
    @@ -222,7 +222,8 @@ save-place-alist-to-file
                   (t
                    t))))
     	(condition-case nil
    -	    (write-file file)
    +	    ;; Don't use write-file; we don't want this buffer to visit it.
    +	    (write-region (point-min) (point-max) file)
     	  (file-error (message "Can't write %s" file)))
             (kill-buffer (current-buffer))
             (message "Saving places to %s...done" file)))))
    ===============================================

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Lars Ingebrigtsen]

Karl Fogel <kfogel@red-bean.com> writes:

> I've just been deep-diving into this again.  I'd like feedback on this general plan for a solution:
>
> 1) In files.el: define a new function `back-up-file' [a] that takes a
>    file name as argument and does most of what `backup-buffer' currently
>    does -- basically, everything from the second `when' on down to the
>    end of the function.
>
> 2) Change `backup-buffer' to use the new function `back-up-file'.
>
>    In other words, we abstract out most of the Emacs file backup
>    functionality to this new function `back-up-file' so we can call it
>    in (3) below, passing a file name argument.

Sounds perfect.  There's a bunch of modes in Emacs that want to do a
backup file, and they go through all these contortions to make this
happen.  

> 3) In bookmark.el, in `bookmark-write-file', call `back-up-file' right
>    before the `write-region' call.  (Note that the ambient value of
>    `version-control' will already be correct at this point.)

Sure.

> [a] Yes, I know I've inserted a hyphen into "back-up" in that function
>     name.  That's because in English "back up" is a compound verb,
>     whereas "backup" as one word is a noun.  The existing function name
>     "backup-buffer" is thus grammatically suspect, but I'm not proposing
>     to fix that here; I'm just trying to avoid repeating the confusion.
>     If folks would prefer "backup-file" for consistency, I could do
>     that, or maybe just find another name.

I'd rather go with `backup-file', just because of the analogy to
`backup-buffer'.  `back-up-file' just looks odd in that context, even if
more correct.  Or perhaps just make it even more explicit:
`make-backup-file'?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Wed, 23 Sep 2020 01:41:52 -0500
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, Chong Yidong <cyd@gnu.org>,
>  12507@debbugs.gnu.org
> 
> This way we would still use `write-region' and avoid visiting the file.

Why do we have to keep to use write-region?

> Even though we don't know exactly why Richard made the original motivating change (commit cfde584f6d, see [b] below), there was some problem with `write-file' -- so we shouldn't just go back to that, because a regression would be likely.

You already had this discussion with Richard back then, see

  https://lists.gnu.org/archive/html/emacs-devel/2005-05/msg01376.html

If what Richard wrote in response is not clear enough, we can ask him
to try to elaborate.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 23 Sep 2020 15:34:01 +0200
> Cc: Chong Yidong <cyd@gnu.org>, 12507@debbugs.gnu.org
> 
> Karl Fogel <kfogel@red-bean.com> writes:
> 
> > I've just been deep-diving into this again.  I'd like feedback on this general plan for a solution:
> >
> > 1) In files.el: define a new function `back-up-file' [a] that takes a
> >    file name as argument and does most of what `backup-buffer' currently
> >    does -- basically, everything from the second `when' on down to the
> >    end of the function.
> >
> > 2) Change `backup-buffer' to use the new function `back-up-file'.
> >
> >    In other words, we abstract out most of the Emacs file backup
> >    functionality to this new function `back-up-file' so we can call it
> >    in (3) below, passing a file name argument.
> 
> Sounds perfect.  There's a bunch of modes in Emacs that want to do a
> backup file, and they go through all these contortions to make this
> happen.  

Maybe this would be a good feature to have, but going back to the
original problem of bookmark file, I think that the easiest fix is to
go back to using write-file.  Karl changed that 15 years ago because
he followed suit of saveplace.el, but I think there's a significant
difference between the files these two packages maintain: the file
with bookmarks is much more close to being "user data" than the file
written by saveplace.el.  So I don't think they need both to treat
their files the same, wrt backing them up.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> > This way we would still use `write-region' and avoid visiting the file.
> 
> Why do we have to keep to use write-region?

We don't.

> > Even though we don't know exactly why Richard made the original motivating
> change (commit cfde584f6d, see [b] below), there was some problem with
> `write-file' -- so we shouldn't just go back to that, because a regression
> would be likely.
> 
> You already had this discussion with Richard back then, see
> 
>   https://urldefense.com/v3/__https://lists.gnu.org/archive/html/emacs-
> devel/2005-
> 05/msg01376.html__;!!GqivPVa7Brio!OSjUJblLyJ3EpHMJp5DOIs2JXP2on_caEr0IyNh9zTt
> iU5_MphK2KXh6pWfteriy$
> 
> If what Richard wrote in response is not clear enough, we can ask him
> to try to elaborate.

Thanks for finding that.  Clearly, the save-place use
case is particular, and the reason for it to use
`write-region' doesn't apply to the use of a bookmark
file.  That's clear to me, at least, from what RMS said:

  If bookmark.el currently visits the file when reading it,
  changing the way it writes the file would not alter the outcome.

  How often do users visit the bookmark file?  If they do this commonly,
  then it is better to visit the file.  If they do this quite rarely,
  then could be is better to work like save-place-alist-to-file.

  Another alternative, which could be best if you want Emacs to make backups
  of the bookmark file in the usual way, is to visit the file normally,
  but kill the buffer after saving it, unless the buffer existed previously.

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

Hi Karl,

At this point I don't have anything special to say
about your general proposal regarding backups.  My
comments regard the bookmarks code.

1. I don't see why this proposal is in bug #12507.
   It seems like a general proposal, which should
   probably go to emacs-devel.

2. I don't see why the bookmark code shouldn't visit
   the file.  That hasn't been explained.  You cite
   a commit for save-place, not for bookmark.el.
   And even for that commit there's no explanation
   of _why_ the particular file shouldn't be visited.

I switched to using `write-file' for bookmark files
long ago, and I haven't seen any problem.  When using
a bookmark file, you're anyway visiting it, in some
sense.  I see no inconvenience in having the bookmark
file be added to a list of visited files.  You might
not be reading or editing it directly (as text), but
you're definitely reading it or editing it indirectly.

Until we hear a reason for why `write-region' should
be used for bookmark files, using it seems like a
solution (for save-place?) looking for a problem.

What's wrong with the obvious, simple solution to the
real, recognized problem of no backups for bookmark
files: use `write-file'?  That's the first question
to ask and answer, no?

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe this would be a good feature to have, but going back to the
> original problem of bookmark file, I think that the easiest fix is to
> go back to using write-file.

Sure; I have no opinion about going back to using write-file in
bookmark.el, but Karl's backup-file (or whatever it's going to be
called) patch should be applied in any case.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

On 23 Sep 2020, Drew Adams wrote:
>At this point I don't have anything special to say
>about your general proposal regarding backups.  My
>comments regard the bookmarks code.
>
>1. I don't see why this proposal is in bug #12507.
>   It seems like a general proposal, which should
>   probably go to emacs-devel.

When the first motivation for a change that has potentially general use is for fixing a specific bug, I normally initiate the discussion of that change in the ticket for the bug, since there isn't anything else directly motivating the change (even if we suspect it has more general use beyond just being part of the bugfix).  

The more general change may be worth mentioning on emacs-devel once the bug-specific thread has consensed, of course; it's just that I wouldn't start out on emacs-devel, because the discussion may go elsewhere and the change might not happen after all.

>2. I don't see why the bookmark code shouldn't visit
>   the file.  That hasn't been explained.  You cite
>   a commit for save-place, not for bookmark.el.
>   And even for that commit there's no explanation
>   of _why_ the particular file shouldn't be visited.
>
>I switched to using `write-file' for bookmark files
>long ago, and I haven't seen any problem.  When using
>a bookmark file, you're anyway visiting it, in some
>sense.  I see no inconvenience in having the bookmark
>file be added to a list of visited files.  You might
>not be reading or editing it directly (as text), but
>you're definitely reading it or editing it indirectly.
>
>Until we hear a reason for why `write-region' should
>be used for bookmark files, using it seems like a
>solution (for save-place?) looking for a problem.
>
>What's wrong with the obvious, simple solution to the
>real, recognized problem of no backups for bookmark
>files: use `write-file'?  That's the first question
>to ask and answer, no?

I can't think of any reason now.  I have a vague memory that there *was* a specific reason, but if there was, I failed to document adequately at the time and have been unable to recover it now.

Eli helpfully linked to Richard's message (https://lists.gnu.org/archive/html/emacs-devel/2005-05/msg01376.html), in which Richard points out that if bookmark visits the file anyway when reading, then there's nothing to be gained (from the perspective of saveplace.el interference) from avoiding visiting it when writing.

And `bookmark-load' *does* visit the file when reading, so I'm leaning toward just taking your suggestion and reverting to `write-file' -- but this time leaving better historical breadcrumbs (in comments and/or log messages) in case my vaguely-remembered bug turns out to be real and reappears.

I will do this as soon as I have time to focus on it and make sure I don't make a silly blunder; that may be several days.

Best regards,
-Karl

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

On 24 Sep 2020, Lars Ingebrigtsen wrote:
>Eli Zaretskii <eliz@gnu.org> writes:
>Sure; I have no opinion about going back to using write-file in
>bookmark.el, but Karl's backup-file (or whatever it's going to be
>called) patch should be applied in any case.

It looks like my solution in bookmark.el won't need that 'backup-file' patch after all.

Lars, if you still want that change, shall we have quick conversation on emacs-devel about it, just people have a chance to weigh in?  I'm happy to make the change, but I also think it would be more coherent to do it together with some accompanying change that actually *uses* the new functionality -- and you suggested you might have some of the latter that you'd like to make.

Feel free to respond with a new thread on emacs-devel; I'll keep an eye open for it.

Best regards,
-Karl

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Lars Ingebrigtsen]

Karl Fogel <kfogel@red-bean.com> writes:

> Lars, if you still want that change, shall we have quick conversation
> on emacs-devel about it, just people have a chance to weigh in?  I'm
> happy to make the change, but I also think it would be more coherent
> to do it together with some accompanying change that actually *uses*
> the new functionality -- and you suggested you might have some of the
> latter that you'd like to make.

I don't think further discussion of the feature is necessary, really --
it's something that I've wanted for a long time, and basically any
package that has some non-file-based data it wishes to store has to do
weird stuff if they don't want to actually visit a file to get a backup
file.

For instance `gnus-save-newsrc-file' for a particularly horrid old
example that could be reduced to a couple lines with a backup-file
function instead.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Drew Adams]

> >What's wrong with the obvious, simple solution to the
> >real, recognized problem of no backups for bookmark
> >files: use `write-file'?  That's the first question
> >to ask and answer, no?
> 
> I can't think of any reason now.  I have a vague memory that there *was* a
> specific reason, but if there was, I failed to document adequately at the
> time and have been unable to recover it now.
> 
> Eli helpfully linked to Richard's message
> (https://urldefense.com/v3/__https://lists.gnu.org/archive/html/emacs-
> devel/2005-
> 05/msg01376.html__;!!GqivPVa7Brio!OesHeMbHNKqFeUCyyW2WJ0UeXBZjSXngBYlvA1Q5G9F
> zXXgAa30g7LGrcwolObMB$ ), in which Richard points out that if bookmark visits
> the file anyway when reading, then there's nothing to be gained (from the
> perspective of saveplace.el interference) from avoiding visiting it when
> writing.
> 
> And `bookmark-load' *does* visit the file when reading, so I'm leaning toward
> just taking your suggestion and reverting to `write-file' -- but this time
> leaving better historical breadcrumbs (in comments and/or log messages) in
> case my vaguely-remembered bug turns out to be real and reappears.
> 
> I will do this as soon as I have time to focus on it and make sure I don't
> make a silly blunder; that may be several days.

Thanks for looking into this, Karl.  (No hurry.)

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

Okay, this is done now:

  | commit 17fa17be3d93fc10f6ca91d738d5056b1b9f1f1e
  | Author:     Karl Fogel <kfogel@red-bean.com>
  | AuthorDate: Sat Nov 28 18:17:46 2020 -0600
  | 
  | Save bookmarks by using `write-file' (bug#12507)
  | 
  | Go back to using `write-file' to save bookmarks, instead of using
  | `write-region'.  This means numbered backups of the bookmark file may
  | get made again, depending on the value of `bookmark-version-control'.
  | 
  | Thanks especially to Drew Adams and Eli Zaretskii for their
  | persistence in tracking down information relevant to this change.

I committed it on the 'emacs-27' branch.  That seemed like the right place for a small and safe change like this (in addition to running 'make check', I tested it manually to make sure it has the effects we expected).  

Although there's been a fair amount of activity in bookmark.el on 'master' recently (e.g., Stefan Kangas's changes to base `bookmark-bmenu-mode' on `tabulated-list-mode', for bug #39293), my commit does not conflict with any of that activity, so there should be no problem when 'emacs-27' is next automerged to 'master'.

Closing bug #12507 with this message.

Best regards,
-Karl

On 29 Sep 2020, Drew Adams wrote:
>> >What's wrong with the obvious, simple solution to the
>> >real, recognized problem of no backups for bookmark
>> >files: use `write-file'?  That's the first question
>> >to ask and answer, no?
>> 
>> I can't think of any reason now.  I have a vague memory that there *was* a
>> specific reason, but if there was, I failed to document adequately at the
>> time and have been unable to recover it now.
>> 
>> Eli helpfully linked to Richard's message
>> (https://urldefense.com/v3/__https://lists.gnu.org/archive/html/emacs-
>> devel/2005-
>> 05/msg01376.html__;!!GqivPVa7Brio!OesHeMbHNKqFeUCyyW2WJ0UeXBZjSXngBYlvA1Q5G9F
>> zXXgAa30g7LGrcwolObMB$ ), in which Richard points out that if bookmark visits
>> the file anyway when reading, then there's nothing to be gained (from the
>> perspective of saveplace.el interference) from avoiding visiting it when
>> writing.
>> 
>> And `bookmark-load' *does* visit the file when reading, so I'm leaning toward
>> just taking your suggestion and reverting to `write-file' -- but this time
>> leaving better historical breadcrumbs (in comments and/or log messages) in
>> case my vaguely-remembered bug turns out to be real and reappears.
>> 
>> I will do this as soon as I have time to focus on it and make sure I don't
>> make a silly blunder; that may be several days.
>
>Thanks for looking into this, Karl.  (No hurry.)

bug#12507: [debbugs-tracker] Processed: severity 12507 wishlist

[Karl Fogel]

Okay, this is done now:

  | commit 17fa17be3d93fc10f6ca91d738d5056b1b9f1f1e
  | Author:     Karl Fogel <kfogel@red-bean.com>
  | AuthorDate: Sat Nov 28 18:17:46 2020 -0600
  | 
  | Save bookmarks by using `write-file' (bug#12507)
  | 
  | Go back to using `write-file' to save bookmarks, instead of using
  | `write-region'.  This means numbered backups of the bookmark file may
  | get made again, depending on the value of `bookmark-version-control'.
  | 
  | Thanks especially to Drew Adams and Eli Zaretskii for their
  | persistence in tracking down information relevant to this change.

I committed it on the 'emacs-27' branch.  That seemed like the right place for a small and safe change like this (in addition to running 'make check', I tested it manually to make sure it has the effects we expected).  

Although there's been a fair amount of activity in bookmark.el on 'master' recently (e.g., Stefan Kangas's changes to base `bookmark-bmenu-mode' on `tabulated-list-mode', for bug #39293), my commit does not conflict with any of that activity, so there should be no problem when 'emacs-27' is next automerged to 'master'.

Closing bug #12507 with this message.

Best regards,
-Karl

On 29 Sep 2020, Drew Adams wrote:
>> >What's wrong with the obvious, simple solution to the
>> >real, recognized problem of no backups for bookmark
>> >files: use `write-file'?  That's the first question
>> >to ask and answer, no?
>> 
>> I can't think of any reason now.  I have a vague memory that there *was* a
>> specific reason, but if there was, I failed to document adequately at the
>> time and have been unable to recover it now.
>> 
>> Eli helpfully linked to Richard's message
>> (https://urldefense.com/v3/__https://lists.gnu.org/archive/html/emacs-
>> devel/2005-
>> 05/msg01376.html__;!!GqivPVa7Brio!OesHeMbHNKqFeUCyyW2WJ0UeXBZjSXngBYlvA1Q5G9F
>> zXXgAa30g7LGrcwolObMB$ ), in which Richard points out that if bookmark visits
>> the file anyway when reading, then there's nothing to be gained (from the
>> perspective of saveplace.el interference) from avoiding visiting it when
>> writing.
>> 
>> And `bookmark-load' *does* visit the file when reading, so I'm leaning toward
>> just taking your suggestion and reverting to `write-file' -- but this time
>> leaving better historical breadcrumbs (in comments and/or log messages) in
>> case my vaguely-remembered bug turns out to be real and reappears.
>> 
>> I will do this as soon as I have time to focus on it and make sure I don't
>> make a silly blunder; that may be several days.
>
>Thanks for looking into this, Karl.  (No hurry.)