bugQuilt - Bugs: bug #63994, Hardlinks not broken when popping...

 
 

bug #63994: Hardlinks not broken when popping patches

Submitter:  Daniel Richard G. <iskunk>
Submitted:  Mon 03 Apr 2023 09:09:16 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Need Info
Privacy:  Public Assigned to:  None
Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Wed 12 Apr 2023 06:49:11 AM UTC, comment #3: 

Hi Jean,

Note that both these approaches (using hardlinks or symlinks) are fundamentally about having quilt follow copy-on-write semantics. The motivation for this is the scenario of a source tree that is very large---as in, large enough that unpacking or copying it is expensive in time and space---and yet has a quilt patch series that patches a minority of files.

(The specific case of this that I am working with is the Debian packaging of the Chromium Web browser. It is a gargantuan source tree, about 5.3 GB unpacked, >140K source files. It has a slew of patches managed via quilt, which modify a bit over 500 source files.)

Cloning the work tree to see how things look at a different point in the patch series (without modifying the original tree) is reasonable, in principle. However, if the full source tree is 5 GB, and only 1 MB of files are ever modified by the series, then one might question why 10 GB of working space should be needed---not least when making a full copy is slow, and especially when there is only 4 GB free disk space. Without going to the overhead of a full CoW filesystem, there should be a way to make that work.

Creating a hard-linked copy of the tree and working with that may be questionable in itself, sure. You have to be very careful that you don't inadvertently modify files in one tree that shouldn't be reflected in the other, including under .pc/. There's plenty that can go wrong. But there are situations where this can be a useful technique, and this request is about making quilt not be one of those things that go wrong. If all you are doing in the hard-linked copy is quilt operations, then you should be able to assume that the tool will do the right thing.

I don't see how ensuring that hygiene would meaningfully affect performance. A couple techniques come to mind:

1. When a file needs to be modified, don't operate directly on the file; create a new temporary file, and then move it into place. This breaks the hardlink, same as what e.g. "perl -i" does.

2. If avoiding the temp copy would speed things up (e.g. a large file in which only a small part is modified), then stat(3) the file, check st_nlink, and only do the temp copy if it's >1. This should only need to be done once in most scenarios.

Quilt is already a good way there. It just needs the remaining hardlink flubs to be addressed.

As for symlinks... there is certainly potential for mischief if quilt is operating on a tree that one didn't put together. (An option to enable symlink handling would be wise.) That is why I brought up lndir(1), which specifically avoids symlinks to directories and thus yields a tree whose semantics are similar to a hard-linked one.

Now, I can't speak to the scenario of patches specifically dealing with symlinks; I don't have experience with those. My suspicion is that symlinks between trees should be distinguishable from those within one. But the use case might be too niche to be worth addressing edge cases like those. I only bring it up due to the parallels with the hardlink case, and the general (ab)usefulness of the lndir(1) tool itself.

Daniel Richard G. <iskunk>
Tue 11 Apr 2023 11:03:17 AM UTC, comment #2: 

About working on a recursively-symlinked copy: that's not going to fly either. How to deal with symbolic links is a difficult topic, and also has security implications.

Traditional context and unified patch formats can't deal with symbolic links, so we could assume that the intent is to follow them and patch their target. Or create a copy of their target and modify that copy (which is what you are asking for). There's no way to guess what the user actually wants.

Git-style patch format can deal with symbolic links though, so following or breaking the links would be wrong there. If such a patch expects a symbolic link and finds a regular file instead, or the other way around, then the expected outcome is that the patch should fail to apply.

Plus following the links outside of the working tree is considered dangerous. There has been several security bugs reported against GNU patch in that area, and it stopped following symbolic links outside the working directory in 2015.

This is a can of worm I definitely don't want to open. In my opinion, your proposal is an abusive use of symbolic links.

Jean Delvare <khali>
Group administrator
Tue 11 Apr 2023 09:41:41 AM UTC, comment #1: 

Quilt corrupting hard-linked copies created before ever calling quilt was indeed a bug that got fixed at some point (although I currently can't find by which commit).

Quilt not handling the scenario you describe, however, is not a bug. It is a case of a user misusing the tool. The manual page states:

"The .pc/ directory contains some metadata about the current state of your patch serie. Changing its content is not advised."

By creating a hard copy of files under .pc/, you actually changed its contents, in a way which quilt does not expect, so whatever breaks as a consequence of that is your responsibility.

Supporting this scenario may be possible, but at the cost of performance (basically we'd have to check for unexpected hard links every time we modify file .pc/applied-patches, so every time we create, push or pop a patch, which are very common quilt commands).

I'm not willing to do this as performance does matter and your scenario doesn't look particularly useful. Why do you want to do this in the first place? Why don't you clone your working tree before applying the patches as everybody else is doing?

Jean Delvare <khali>
Group administrator
Mon 03 Apr 2023 09:09:16 PM UTC, original submission:  

Issue confirmed to exist in quilt 0.67.

When files under .pc/ are hard-linked elsewhere and patches are popped, quilt does not break the links, resulting in the hard-linked copies of said files (potentially outside the source tree) being modified.

Say that "tree" is a project source tree that uses quilt, and has all patches applied. The issue can be reproduced as follows:

cp -a  tree   tree.a
cp -al tree.a tree.b

(cd tree.b && quilt pop -a -q)

rsync -av --dry-run tree/ tree.a


(I am [ab]using rsync(1) here as a means of comparing two trees, including file timestamps, as "diff -ru" only looks at file content. The comparison is equal if no files/dirs are listed in the output. Note that this invocation will not modify any files.)

The correct result is for "tree" and "tree.a" to remain precise copies of each other, unaffected by the quilt operations in "tree.b". Unfortunately, the above will show that this is currently not the case.

I did observe that the inverse test, where "tree" starts with no patches applied and the patches are pushed in "tree.b", yielded the correct result. Earlier testing with quilt 0.66 had not shown this, so the issue appears to have been at least partially addressed in 0.67.

On a related note, it would be a plus if quilt could handle correctly the case of "tree.b" being a recursively-symlinked copy (as created by lndir(1)). If the original tree has all patches applied, then quilt will pop the patches, and only tree.a/.pc/applied-patches is unduly modified. But if the original tree has no patches applied, and you attempt to push them in "tree.b", then quilt errors out with "File <filename> is not a regular file -- refusing to patch". Quilt should know to copy the file from the symlink target, and patch the copy.

Daniel Richard G. <iskunk>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by khali (Posted a comment)
  • -email is unavailable- added by iskunk (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-04-11 khali StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-04b1.
    Corresponding source code