bug#13801: [PATCH] Trivial fix for files.el

bug#13801: [PATCH] Trivial fix for files.el

[Xue Fuqiao]

I made a minor patch for files.el.  The patch does this:

1. Improve the doc string for `break-hardlink-on-save';

2. Remove an unnecessary `buffer-auto-save-file-name'.


*** trunk/lisp/files.el	2013-02-24 06:49:38.358835000 +0800
--- trunk/lisp/files.el.new	2013-02-24 14:23:28.681175258 +0800
*************** See also: `break-hardlink-on-save'."
*** 248,259 ****
    :group 'backup)
  
  (defcustom break-hardlink-on-save nil
!   "Non-nil means when saving a file that exists under several names
! \(i.e., has multiple hardlinks), break the hardlink associated with
! `buffer-file-name' and write to a new file, so that the other
! instances of the file are not affected by the save.
  
! If `buffer-file-name' refers to a symlink, do not break the symlink.
  
  Unlike `file-precious-flag', `break-hardlink-on-save' is not advisory.
  For example, if the directory in which a file is being saved is not
--- 248,262 ----
    :group 'backup)
  
  (defcustom break-hardlink-on-save nil
!   "This variable affect the relation between hardlink(s) and the file.
! Non-nil means when saving a file that exists under several names
! \(i.e., has multiple hardlinks), break the hardlink associated
! with the variable `buffer-file-name' and write to a new file, so
! that the other instances of the file are not affected by the
! save.
  
! If the variable`buffer-file-name' refers to a symlink, do not
! break the symlink.
  
  Unlike `file-precious-flag', `break-hardlink-on-save' is not advisory.
  For example, if the directory in which a file is being saved is not
*************** non-nil, it is called instead of rereadi
*** 5333,5339 ****
        (let* ((revert-buffer-in-progress-p t)
               (auto-save-p (and (not ignore-auto)
  			       (recent-auto-save-p)
- 			       buffer-auto-save-file-name
  			       (file-readable-p buffer-auto-save-file-name)
  			       (y-or-n-p
       "Buffer has been auto-saved recently.  Revert from auto-save file? ")))
--- 5336,5341 ----



*** trunk/lisp/ChangeLog	2013-02-24 06:49:38.358835000 +0800
--- trunk/lisp/ChangeLog.new	2013-02-24 14:27:41.126427073 +0800
***************
*** 1,7 ****
  2013-02-23  Peter Kleiweg  <p.c.j.kleiweg@rug.nl>
  
  	* progmodes/ps-mode.el (ps-mode-version): Bump to 1.1i.
! 	(ps-mode-octal-region): Use string-make-unibyte.
  
  2013-02-23  Glenn Morris  <rgm@gnu.org>
  
--- 1,12 ----
+ 2013-02-24  Xue Fuqiao  <xfq.free@gmail.com>
+ 
+ 	* files.el (break-hardlink-on-save): Doc fix.
+ 	(revert-buffer): Remove the unnecessary `buffer-auto-save-file-name'.
+ 
  2013-02-23  Peter Kleiweg  <p.c.j.kleiweg@rug.nl>
  
  	* progmodes/ps-mode.el (ps-mode-version): Bump to 1.1i.
! 	(ps-mode-octal-region): Use `string-make-unibyte'.
  
  2013-02-23  Glenn Morris  <rgm@gnu.org>
  


-- 
Best regards, Xue Fuqiao.
http://www.emacswiki.org/emacs/XueFuqiao

bug#13801: [PATCH] Trivial fix for files.el

[Eli Zaretskii]

> Date: Sun, 24 Feb 2013 14:33:32 +0800
> From: Xue Fuqiao <xfq.free@gmail.com>
> 
> *** trunk/lisp/files.el	2013-02-24 06:49:38.358835000 +0800
> --- trunk/lisp/files.el.new	2013-02-24 14:23:28.681175258 +0800
> *************** See also: `break-hardlink-on-save'."
> *** 248,259 ****
>     :group 'backup)
>   
>   (defcustom break-hardlink-on-save nil
> !   "Non-nil means when saving a file that exists under several names
> ! \(i.e., has multiple hardlinks), break the hardlink associated with
> ! `buffer-file-name' and write to a new file, so that the other
> ! instances of the file are not affected by the save.
>   
> ! If `buffer-file-name' refers to a symlink, do not break the symlink.
>   
>   Unlike `file-precious-flag', `break-hardlink-on-save' is not advisory.
>   For example, if the directory in which a file is being saved is not
> --- 248,262 ----
>     :group 'backup)
>   
>   (defcustom break-hardlink-on-save nil
> !   "This variable affect the relation between hardlink(s) and the file.
> ! Non-nil means when saving a file that exists under several names
> ! \(i.e., has multiple hardlinks), break the hardlink associated
> ! with the variable `buffer-file-name' and write to a new file, so
> ! that the other instances of the file are not affected by the
> ! save.

The first line of the doc string should concisely say what the option
does.  The old doc string did it, albeit not perfectly; the one you
suggest does not.

bug#13801: [PATCH] Trivial fix for files.el

[Drew Adams]

> The first line of the doc string should concisely say what
> the option does.

Correct.

Including, if possible, what a nil or non-nil value does, if the option is
Boolean.  And the first line should (must) be a full sentence.

> The old doc string did it, albeit not perfectly;

Incorrect.  The first line did not do that at all.  At all.

> the one you suggest does not.

Correct.  My guess is that it was an attempt to provide a short full sentence
(good), but it does not say what the option does (not good).

What is important in both the old and new doc strings, and can serve as the
first line, is that the option does this (77 chars):

"Non-nil means break a hard link for the visited file and write to a new file."

If you feel that 77 chars is too much, you can drop "a" before "hard link" or
"the" before "visited file".  The rest of the doc string can clarify things
further (e.g., mentioning `buffer-file-name').

Whatever the wording chosen, the point is that non-nil means Emacs writes a new
file, bypassing any hard link for `buffer-file-name'.

In fact, I'm not sure about "break" here.  Is the effect that the hard link no
longer exists, or simply that it is ignored by Emacs when saving?  Depending on
the answer, the doc might need to be tweaked a little more.

The old first line was not even a complete sentence.  And the old first sentence
was four lines!

And unless I'm mistaken, the old first sentence was incorrect or at least
misleading regarding the condition where the option applies: "has multiple
hardlinks".

I think the correct criterion is simply having a hard link for the visited file
(`buffer-file-name'), regardless of whether there are any other hard links,
i.e., not necessarily multiple such.

It is not easy to write a short summary sentence, especially when English is not
your maternal language.  This is a welcome initiative from Fuqiao.  He clearly
takes an interest in the doc (as do Eli and Drew), and that is a fairly rare
resource.  Thank you.

bug#13801: [PATCH] Trivial fix for files.el

[Stephen Berman]

On Sun, 24 Feb 2013 09:47:44 -0800 "Drew Adams" <drew.adams@oracle.com> wrote:

>> The first line of the doc string should concisely say what
>> the option does.
>
> Correct.
>
> Including, if possible, what a nil or non-nil value does, if the option is
> Boolean.  And the first line should (must) be a full sentence.
>
>> The old doc string did it, albeit not perfectly;
>
> Incorrect.  The first line did not do that at all.  At all.
>
>> the one you suggest does not.
>
> Correct.  My guess is that it was an attempt to provide a short full sentence
> (good), but it does not say what the option does (not good).
>
> What is important in both the old and new doc strings, and can serve as the
> first line, is that the option does this (77 chars):
>
> "Non-nil means break a hard link for the visited file and write to a new file."

To keep it under 68 characters (as per (elisp) Documentation Tips), how
about:

Non-nil means write visited file to a new file, breaking hard links.

Steve Berman

bug#13801: [PATCH] Trivial fix for files.el

[Drew Adams]

> > "Non-nil means break a hard link for the visited file and 
> write to a new file."
> 
> To keep it under 68 characters (as per (elisp) Documentation 
> Tips), how about:
> 
> Non-nil means write visited file to a new file, breaking hard links.

OK by me.  (I was thinking the guideline limit is more than 68.)

But that text suggests that a new file is always used.  Using "any hard link" or
"a hard link" is a bit better, suggesting that the new file thing is conditional
on there being a hard link.

Maybe this (66 chars)?

Non-nil means write new file if `buffer-file-name' is hard-linked.

bug#13801: [PATCH] Trivial fix for files.el

[Xue Fuqiao]

On Sun, 24 Feb 2013 14:27:06 -0800
"Drew Adams" <drew.adams@oracle.com> wrote:

> > > "Non-nil means break a hard link for the visited file and 
> > write to a new file."
> > To keep it under 68 characters (as per (elisp) Documentation 
> > Tips), how about:
> > Non-nil means write visited file to a new file, breaking hard links.
> OK by me.  (I was thinking the guideline limit is more than 68.)
> But that text suggests that a new file is always used.  Using "any hard link" or
> "a hard link" is a bit better, suggesting that the new file thing is conditional
> on there being a hard link.
> Maybe this (66 chars)?
> Non-nil means write new file if `buffer-file-name' is hard-linked.

I think there are three problems (to me) about this version:

1. "write new file" is confusing.  I don't know what kind of thing will be write to which file;

2. `buffer-file-name' has both a function cell and a value cell, a little confusing;

2. `buffer-file-name' is a string (or returns a string), not a file, so the file cannot be hard-linked.

But I can't give a better version, sorry.

-- 
Best regards, Xue Fuqiao.
http://www.emacswiki.org/emacs/XueFuqiao

bug#13801: [PATCH] Trivial fix for files.el

[Chong Yidong]

Xue Fuqiao <xfq.free@gmail.com> writes:

> I made a minor patch for files.el.  The patch does this:
>
> 1. Improve the doc string for `break-hardlink-on-save';

Thanks, I committed a slightly different fix.

> 2. Remove an unnecessary `buffer-auto-save-file-name'.
>
> - 			       buffer-auto-save-file-name
>   			       (file-readable-p buffer-auto-save-file-name)

This is not unnecessary.  If buffer-auto-save-file-name is nil, the next
line would signal an error, which is undesireable.