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'

[Chong Yidong]

"Drew Adams" <drew....@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.

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

[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

[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'.

[Stefan Monnier]

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


Stetfan

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

[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 <kfo...@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....@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.
>

>[...]

[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 <kfo...@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

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

[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

[Thierry Volpiatto]

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

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

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

[Karl Fogel]

"Drew Adams" <drew....@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 <kfo...@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.
>>

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

[Karl Fogel]

"Drew Adams" <drew....@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

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

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

[Thierry Volpiatto]

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 Volpiatto]

"Drew Adams" <drew....@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)

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

[Thierry Volpiatto]

"Drew Adams" <drew....@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.

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

[Thierry Volpiatto]

"Drew Adams" <drew....@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.

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

[Thierry Volpiatto]

"Drew Adams" <drew....@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.

[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).

[Thierry Volpiatto]

Agree.

[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: kfo...@red-bean.com-20121001033206-5eja4ztyhs1sjm7q
parent: c...@gnu.org-20121001031702-2mei04wuzv2pk1e7
committer: Karl Fogel <kfo...@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 <kfo...@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 <egg...@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)

[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))

[Drew Adams]

[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

[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 'con...@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.

[Karl Fogel]

"Drew Adams" <drew....@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

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

[Thierry Volpiatto]

"Drew Adams" <drew....@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.