Re: [Quilt-dev] Another shell re-write of backup-files.

[Jean Delvare]

Hi Kaz,

On Monday 21 March 2011 11:07:11 pm Kaz Kylheku wrote:
> On Mon, 21 Mar 2011 13:40:37 +0100, Jean Delvare <address@hidden>
> 
> wrote:
> > Hi Kaz,
> >
> >> The command
> >>
> >>   backup-files -L -s -B $QUILT_PC/$patch/ -
> >>
> >> means "walk the backup directory $QUILT_PC/$patch,
> >> and replace any file that has a link count > 1
> >> with a clone of that file."
> >
> > No, sorry, it doesn't mean that. See for yourself (with the
> > original C implementation of backup-files, which is the reference):
> 
> Ah wow, it does ensure that even when walking the backup
> tree to determine which files to operate on, the semantics
> of "backup" and "file" is still consistent within
> the process_file function!

Yes, it does. 

> So it is actually orthogonal in this regard also
> (more trivial than I thought).

I have no idea what you mean with "orthogonal" here, sorry.

> > As you can see, backup-files -L broke the link of the original
> > file, and left the backup file untouched.
> 
> I see, and from this you are inferring that there is
> some requirement which makes it important to preserve
> the original inode in the backup, so that the ultimate
> restore will put everything back.

Yes, this is exactly the point.

> Superficially it does look as if the program is
> obsessive in insisting that this be done on the
> original tree, suggesting that there may be some
> deeper reason.
> 
> But in fact, this aspect of the behavior is
> just an "onion in the varnish" type of thing,
> which is revealed when we look at the history.
> 
> Firstly, if you do link-breaking restore (-r -L), this
> is exactly the same as just copying the file"
> 
>       if (link_or_copy(backup, &st, file))
>               return 1;
> 
>       if (opt_nolinks) {
>               if (ensure_nolinks(file))
>                       return 1;
>       }

Your analysis is technically correct, yes.

> Now if you go back in the history of quilt, you will
> find that quilt pop originally did this type of
> restore at every level. I.e. quilt restored files by
> a copy.
>
> That changed in the following commit, which also
> added the "noop" as well as the - argument feature
> to make backup-files walk the backup directory:
> 
> http://git.savannah.gnu.org/cgit/quilt.git/commit/?id=eb1a6de33c1b631
> e4636ff26dde2945a0db8db74
> 
> The newly added noop case breaks the links on the
> original tree side most likely for the sake of consistency with
> backup and restore. The other two existing
> ensure_nolinks calls take an argument of "file" not backup,
> so this is just cut and paste.

I'm not sure what you are up to here. As you noticed before, "file" and 
"backup" are consistently used for the original tree and the backup 
directory, respectively, in the whole backup-files code. There is 
nothing wrong here. 

> Prior to this commit, quilt did not restore hard links.
> All restored files were new objects with a link count of 1
> and a fresh time stamp.

Good point, I admit I hadn't noticed.

> This commit failed to preserve that behavior, so it can
> be regarded as buggy.

The change in behavior was definitely not good, at least not good to 
commit without a detailed description and justification of why it was 
deemed desirable. The commit description sounds like it was a simple 
performance optimization, when it is much more than this. That being 
said, the quilt project was still relatively young back then, so it's 
difficult to tell now which of the old or new behavior was considered 
to be correct, and which was considered to be buggy.

One thing I think is worth mentioning at this point is that the old 
behavior did not cause the "change to one tree causes rebuilds in other 
trees" issue which we discussed a couple days ago. So, in this respect, 
the commit we are discussing indeed caused a regression, which survived 
until now. In fact, at first it was even worse than this, as it no 
longer changed the timestamp of restored files at all, so partial 
rebuilds of the source tree would be incorrect, until commit 
423903f9095b4e447b738715e460cb9319a44c2e, 4 months later.

I'm glad we have a dedicated test case for backup-files now, so that 
hopefully this kind of change doesn't go unnoticed and undocumented in 
the future.

However, taking some distance with the history, I think of the problem 
as follows: there are two use cases of quilt. Either you care about disk 
space and speed of "patch navigation", or you don't.

If you don't, then you will not be using hard-linked trees in the first 
place, which means that the change in behavior described above does 
_not_ affect you: the ensure_nolinks call will always be a no-op for 
you.

If you do care, then you certainly prefer the new behavior (always 
preserve links), because with the old behavior (break links on quilt 
pop), after navigating the patch series you lose most of the benefits of 
the original hard-linking (the amount of benefit left is proportional to 
the ratio of files in your tree which aren't modified by any patch.)

Of course there may be specific use cases with huge trees and only a few 
files being modified, where "unlink on quilt pop" would be desirable, so 
you could argue that we need a way to let the user select which way 
he/she wants quilt to behave. Me, I am happy with the current behavior, 
but I wouldn't object to changing it. But I have no idea what other 
users of quilt on hard-linked trees prefer.

Thanks for the discussion Kaz, it was quite interesting for me to dive 
in the history of backup-files to better understand its limitations. 
That being said, I think we should concentrate on the initial task for 
now, that is, the conversion of backup-files to bash. Behavioral changes 
are out of scope and can be discussed separately at a later point.

-- 
Jean Delvare
Suse L3