checkout: Merge union/add logic for copies during checkout #801
Conversation
|
☔ The latest upstream changes (presumably 50ca653) made this pull request unmergeable. Please resolve the merge conflicts. |
We really have an astonishing variety of similar functions which write files and symlinks. I was working on a different PR and the duplication between the union-mode and add-mode/none-mode checkout functions bothered me. I realized that the "handle EEXIST" tri-state maps directly to the `GLnxLinkTmpfileReplaceMode`, so deduping things makes even more sense.
cgwalters
force-pushed
the
checkout-dedup
branch
from
a3a2b35 to
4e70f66
Compare
|
🛫 Rebased 🛬 |
src/libostree/ostree-repo-checkout.c
Outdated
| cancellable, error)) | ||
| return FALSE; | ||
|
|
||
| /* Process any xattrs now that we made the link */ | ||
| if (xattrs) |
There was a problem hiding this comment.
This is not the same behaviour though, right? I.e. if we're in ADD_FILES mode, then we should only apply xattrs if it didn't already exist, right? Also, it seems like the chmod bits conditional on user mode got snipped (not that it matters much for symlinks, though I suppose ostree diff would notice). Actually, even applying xattrs should be conditional on user mode.
But now, I'm surprised the testsuite didn't catch this. So either I'm missing something, or we should add a few more tests for these :)
There was a problem hiding this comment.
Ah, good catch. Yes. In general though, I'm not sure anyone uses xattrs on symlinks aside from SELinux (and possibly SMACK).
Yeah, no test coverage for this bit right now. Hmm. Filed #806
There was a problem hiding this comment.
We still need the chown bit too, right? Otherwise, I see ostree diff immediately reporting something after a fresh checkout:
# ostree ls symlink-chown-test
d00755 0 0 0 /
l00777 1000 1000 0 /mylink -> mytarget
# mkdir tmp/copy-checkout && mount --bind tmp/copy-checkout tmp/copy-checkout
# ostree checkout symlink-chown-test tmp/copy-checkout/checkout
# ostree diff symlink-chown-test ./tmp/copy-checkout/checkout
M /mylink
There was a problem hiding this comment.
Yes, fixed. I spent a little bit trying to add a test but it needs 3a794ea
We can add it after this all comes together.
|
☀️ Test successful - status-atomicjenkins |
We really have an astonishing variety of similar functions which write files and
symlinks. I was working on a different PR and the duplication between the
union-mode and add-mode/none-mode checkout functions bothered me.
I realized that the "handle EEXIST" tri-state maps directly to the
GLnxLinkTmpfileReplaceMode, so deduping things makes even more sense.