libarchive: Add support for translating paths during commit #1105
Conversation
src/libostree/ostree-repo.h
Outdated
| * Since: 2017.11 | ||
| */ | ||
| typedef char *(*OstreeRepoImportArchiveTranslatePathname) (OstreeRepo *repo, | ||
| GFileType file_type, |
There was a problem hiding this comment.
Let's just pass the stat buffer directly?
There was a problem hiding this comment.
I did that originally, but then we'd need to #include <sys/stat.h> in ostree.h. Which...is probably fine but takes us a bit farther down the unix-only path. OK, so libglnx already makes us linux-specific...yeah, I'll make that change back.
| * will be freed via `g_free()`. | ||
| * | ||
| * This pathname translation will be performed *before* any processing from an | ||
| * active `OstreeRepoCommitModifier`. Will be invoked for all directory and file |
There was a problem hiding this comment.
We should document how this relates to use_ostree_convention here.
src/ostree/ot-builtin-commit.c
Outdated
| @@ -95,6 +97,7 @@ static GOptionEntry options[] = { | |||
| { "no-xattrs", 0, 0, G_OPTION_ARG_NONE, &opt_no_xattrs, "Do not import extended attributes", NULL }, | |||
| { "link-checkout-speedup", 0, 0, G_OPTION_ARG_NONE, &opt_link_checkout_speedup, "Optimize for commits of trees composed of hardlinks into the repository", NULL }, | |||
| { "tar-autocreate-parents", 0, 0, G_OPTION_ARG_NONE, &opt_tar_autocreate_parents, "When loading tar archives, automatically create parent directories as needed", NULL }, | |||
| { "tar-pathname-filter", 0, 0, G_OPTION_ARG_STRING, &opt_tar_pathname_filter, "When loading tar archives, use REGEX,REPLACEMENT against path names", "REGEX,REPLACEMENT" }, | |||
There was a problem hiding this comment.
This feels like it should be a G_OPTION_ARG_STRING_ARRAY instead, where the first filter that applies wins. Though since it's fully backwards compatible, we can do that later if we feel there's a use case for having CLI support for it.
There was a problem hiding this comment.
Agreed; let's consider this later, in the end I think we want to encourage use of the API.
src/ostree/ot-builtin-commit.c
Outdated
| { | ||
| g_assert_no_error (tmp_error); | ||
| g_assert_not_reached (); | ||
| } |
There was a problem hiding this comment.
If the replacement didn't make any changes, we should return NULL, no? At least so we test the fallback behaviour.
| if (ret) | ||
| return ret; | ||
| /* Fall through */ | ||
| } |
There was a problem hiding this comment.
I'd say we add an else here. I.e. if translate_pathname is active, use_ostree_convention has no effect. Otherwise, it's odd thinking that there's a difference between returning NULL and returning the exact same string.
For rpm-ostree, I want to move RPM files in `/boot` to `/usr/lib/ostree-boot`. This is currently impossible without forking the libarchive code. Supporting this is pretty straightforward; we already had pathname translation in the libarchive code, we just need to expose it as an option. On the command line side, I chose to wrap this as a regexp. That should be good enough for a lot of use cases; sophisticated users should as always be making use of the API. Note that this required some new `#ifdef LIBARCHIVE` bits to use the new API. Following previous patterns here, we use the new API only if a relevant option is enabled, ensuring unit test coverage of both paths. For the test cases, I ended up changing the accounting to avoid having to multiply the test count.
cgwalters
force-pushed
the
archive-path-xlate
branch
from
486d8a4
to
49f55c7
Compare
|
Updated for comments ⬆️ |
|
☀️ Test successful - status-atomicjenkins |
For rpm-ostree, I want to move RPM files in
/bootto/usr/lib/ostree-boot.This is currently impossible without forking the libarchive code. Supporting
this is pretty straightforward; we already had pathname translation in
the libarchive code, we just need to expose it as an option.
On the command line side, I chose to wrap this as a regexp. That should be good
enough for a lot of use cases; sophisticated users should as always be making
use of the API. Note that this required some new
#ifdef LIBARCHIVEbits to usethe new API. Following previous patterns here, we use the new API only if a
relevant option is enabled, ensuring unit test coverage of both paths.
For the test cases, I ended up changing the accounting to avoid having to
multiply the test count.