[PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Karl Fogel]

I'm offering this patch for review before I commit, since I'm not an
expert in the VC code.  Following the patch is a shell script showing
the reproduction recipe.

Summary: this was actually a latent bug between Emacs and bzr branches
created with 'bzr branch --hardlink', but it had been masked by the
default make-backup-files behavior.  As long as Emacs was writing to a
tmp file and then renaming, the hardlink would get broken anyway --
which was desirable, and so the right thing happened by accident.  But
then VC apparently got smart and stopped making backup files for files
under version control.  This was good, but unmasked a bug whereby saving
a file in branch NEW (created with 'bzr branch --hardlink OLD NEW')
would cause the corresponding file in OLD to be modified too.  Oops.

I tested with today's head of Bazaar (1.10dev) and today's head CVS
Emacs (23.0.60.1).  Comments welcome.

Patch first:

[[[
Index: lisp/ChangeLog
===================================================================
RCS file: /sources/emacs/emacs/lisp/ChangeLog,v
retrieving revision 1.14751
diff -u -r1.14751 ChangeLog
--- lisp/ChangeLog	8 Nov 2008 14:23:06 -0000	1.14751
+++ lisp/ChangeLog	9 Nov 2008 03:25:56 -0000
@@ -1,3 +1,8 @@
+2008-11-09  Karl Fogel  <kfogel@red-bean.com>
+
+	* vc-bzr.el (vc-bzr-find-file-hook): Set file-precious-flag to t,
+	to avoid stomping files across bzr hardlink branches.
+
 2008-11-08  Chong Yidong  <cyd@stupidchicken.com>
 
 	* dired.el (dired-read-dir-and-switches): Revert to 2007-11-22

Index: lisp/vc-bzr.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc-bzr.el,v
retrieving revision 1.70
diff -u -r1.70 vc-bzr.el
--- lisp/vc-bzr.el	4 Nov 2008 17:36:43 -0000	1.70
+++ lisp/vc-bzr.el	9 Nov 2008 03:25:57 -0000
@@ -293,6 +293,15 @@
       (remove-hook 'after-save-hook 'vc-bzr-resolve-when-done t))))
 
 (defun vc-bzr-find-file-hook ()
+  ;; The user might have done 'bzr branch --hardlink OLD NEW', and if
+  ;; they did, we definitely want to break the link when saving a file
+  ;; in either OLD or NEW.  Since there's little harm done if the user
+  ;; created a non-hardlink branch, and no easy way to tell if they
+  ;; did or didn't, we just set preciousness unconditionally.  We don't
+  ;; ask (and buffer-file-name (vc-bzr-registered buffer-file-name))
+  ;; because if this hook is being called, the file must be registered.
+  (set (make-local-variable 'file-precious-flag) t)
+  ;; Notice conflicts.
   (when (and buffer-file-name
              ;; FIXME: We should check that "bzr status" says "conflict".
              (file-exists-p (concat buffer-file-name ".BASE"))
]]]

Then reproduction script:

------------------------------------------------------------------------------
#!/bin/sh

echo "### bzr version is:"
bzr --version
echo ""

rm -rf sandbox

mkdir sandbox
cd sandbox/

# Make bzr not use your normal ~/.bazaar/config
BZR_HOME=`pwd`
export BZR_HOME

mkdir old
cd old

echo "### In old, run 'bzr init'..."
bzr init
echo ""

echo "### In old, create hello.txt, add it to bzr, and commit..."
echo hello > hello.txt
bzr add
bzr commit -m "Hello."
echo ""

cd ..
echo "### Do 'bzr branch --hardlink old new'..."
bzr branch --hardlink old new
echo ""

cd new/

echo "### In new, run 'ls -l' to see the hardlink is really there..."
echo ""
ls -l
echo ""

echo "### In new, run 'emacs -q hello.txt' (please modify, save, and exit)..."
emacs -q hello.txt
echo ""

echo "### In new, run 'bzr diff' to see the expected diff..."
bzr diff
echo ""

echo "### In new, commit the change (although I'm not sure this step is"
echo "### strictly necessary to demonstrate the bug)..."
echo ""
bzr commit -m "Added world."
echo ""

cd ../old/

echo "### In old, run 'bzr diff' (we don't want any diff to show)..."
bzr diff
echo ""

echo "### If there was a diff the second time (in old), then Emacs has"
echo "### the hardlink bug."

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Miles Bader]

Karl Fogel <kfogel@red-bean.com> writes:
> But then VC apparently got smart and stopped making backup files for
> files under version control.  This was good

VC has had this behavior for a very long time (and I think it's quite
arguable whether it's actually a good default or not -- the granularity
of vc commits and emacs backup files can be very different).

-Miles

-- 
Do not taunt Happy Fun Ball.

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Karl Fogel]

Miles Bader <miles@gnu.org> writes:
> Karl Fogel <kfogel@red-bean.com> writes:
>> But then VC apparently got smart and stopped making backup files for
>> files under version control.  This was good
>
> VC has had this behavior for a very long time (and I think it's quite
> arguable whether it's actually a good default or not -- the granularity
> of vc commits and emacs backup files can be very different).

Thanks.  I should have added a stronger qualifier than "apparently", as
I wasn't completely sure about that history!

There must be some other reason why the original reporter, Tim Penhey,
only noticed the bug recently.  Perhaps he'd been customizing his
backup-file behavior but then stopped doing so, or something?  Anyway,
the bug exists regardless.

(I agree with you about VC's backup-file behavior being arguable, but
it's a separate topic -- this bug would exist, and my patch would apply,
no matter what VC's default backup-file behavior were and no matter what
customizations a user happened to make to that behavior.)

-Karl

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Stefan Monnier]

> I'm offering this patch for review before I commit, since I'm not an
> expert in the VC code.  Following the patch is a shell script showing
> the reproduction recipe.

I don't really like this, because links can also be used for other
purposes than "copy on write".  Most importantly your patch breaks
symlinks, which is usually the wrong thing to do.

In my opinion, the use of hardlinked trees is just risky business that
requires a lot of care, so I think priority should be given to handle
the "non-hardlinked tree" case correctly.


        Stefan

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Tim Penhey]

On Mon, 10 Nov 2008 04:03:05 Stefan Monnier wrote:
> > I'm offering this patch for review before I commit, since I'm not
> > an expert in the VC code.  Following the patch is a shell script
> > showing the reproduction recipe.
>
> I don't really like this, because links can also be used for other
> purposes than "copy on write".  Most importantly your patch breaks
> symlinks, which is usually the wrong thing to do.
>
> In my opinion, the use of hardlinked trees is just risky business
> that requires a lot of care, so I think priority should be given to
> handle the "non-hardlinked tree" case correctly.

Can we at least have an option to have emacs break hard links on edit?

This bit me when my ubuntu laptop was upgraded to intrepid, which 
upgraded my emacs which brought with it a newer vc module which 
automagically detected files that were in a bzr branch.  Previously 
backup copies were being made, which caused the had links to be broken.  
When the files were detected to be in source control, emacs (correctly in 
my opinion) stopped attempting to create backup files.  My issue was that 
my workflow suddenly got screwed up.

I'm not suggesting that the default behaviour necessarily be changed for 
everything, but I'd like the ability to have emacs brake my hardlinks on 
edit for my bzr hosted files.

Tim

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Stefan Monnier]

> Can we at least have an option to have emacs break hard links on edit?

Yes, that would be good.  It should work regardless of VC.  People using
hardlinked trees would know to use it.


        Stefan

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Karl Fogel]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Can we at least have an option to have emacs break hard links on edit?
>
> Yes, that would be good.  It should work regardless of VC.  People using
> hardlinked trees would know to use it.

So a variable, something like this?:

   break-hardlinks-on-edit

   "*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 edits.

    If `buffer-file-name' refers to a symlink, do not break the
    symlink."

I'm happy to write this, just want to make sure I'm headed down the
right design path.

-Karl

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Stefan Monnier]

>>> Can we at least have an option to have emacs break hard links on edit?
>> Yes, that would be good.  It should work regardless of VC.  People using
>> hardlinked trees would know to use it.
> So a variable, something like this?:

>    break-hardlinks-on-edit

>    "*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 edits.
>     If `buffer-file-name' refers to a symlink, do not break the
>     symlink."

> I'm happy to write this, just want to make sure I'm headed down the
> right design path.

That sounds good.


        Stefan

Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.

[Karl Fogel]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>> Can we at least have an option to have emacs break hard links on edit?
>>> Yes, that would be good.  It should work regardless of VC.  People using
>>> hardlinked trees would know to use it.
>> So a variable, something like this?:
>
>>    break-hardlinks-on-edit
>
>>    "*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 edits.
>>     If `buffer-file-name' refers to a symlink, do not break the
>>     symlink."
>
>> I'm happy to write this, just want to make sure I'm headed down the
>> right design path.
>
> That sounds good.

This is done now, in rev 1.14881 of lisp/files.el.  (Except I went with
the singular `break-hardlink-on-save', since it only breaks one hardlink
at a time.)

-Karl

vc-dir header for bzr (was: Re: [PATCH] vc-bzr.el: avoid stomping files across hardlink branches.)

[Dan Nicolaescu]

Since you seem to be interested in VC with bzr, can you please look at
the following issue too?

In vc-dir it is possible to add custom headers for each VC backend. 
Currently for bzr only a single one is used "Parent branch". 
But it seems that "bzr info" offers more information that can be useful:

$ bzr info
Standalone tree (format: pack-0.92)
Location:
  branch root: FOOBAR

Related branches:
    push branch: FOO
  parent branch: BAR


Is there anything else that could be added to the headers here?
Thanks.
                --dan

Re: vc-dir header for bzr

[Karl Fogel]

Dan Nicolaescu <dann@ics.uci.edu> writes:
> Since you seem to be interested in VC with bzr, can you please look at
> the following issue too?

I'm actually not much of a bzr user yet.

However, I'm becoming one soon, thanks to a fortuitous intersection of
work and interest.  So I'll try to remember to look at this once I'm
more expert -- but feel free to poke me again in month or so too.

-Karl

> In vc-dir it is possible to add custom headers for each VC backend. 
> Currently for bzr only a single one is used "Parent branch". 
> But it seems that "bzr info" offers more information that can be useful:
>
> $ bzr info
> Standalone tree (format: pack-0.92)
> Location:
>   branch root: FOOBAR
>
> Related branches:
>     push branch: FOO
>   parent branch: BAR
>
>
> Is there anything else that could be added to the headers here?
> Thanks.
>                 --dan