Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repo: Add a "force copy" flag to checkout #804

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libostree/ostree-repo-checkout.c
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ checkout_one_file_at (OstreeRepo *repo,

need_copy = FALSE;
}
else
else if (!options->force_copy)
{
HardlinkResult hardlink_res = HARDLINK_RESULT_NOT_SUPPORTED;
/* Try to do a hardlink first, if it's a regular file. This also
Expand Down
3 changes: 2 additions & 1 deletion src/libostree/ostree-repo.h
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,8 @@ typedef struct {
gboolean enable_fsync; /* Deprecated */
gboolean process_whiteouts;
gboolean no_copy_fallback;
gboolean unused_bools[7];
gboolean force_copy; /* Since: 2017.6 */
gboolean unused_bools[6];

const char *subpath;

Expand Down
10 changes: 9 additions & 1 deletion src/ostree/ot-builtin-checkout.c
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ static gboolean opt_from_stdin;
static char *opt_from_file;
static gboolean opt_disable_fsync;
static gboolean opt_require_hardlinks;
static gboolean opt_force_copy;

static gboolean
parse_fsync_cb (const char *option_name,
Expand Down Expand Up @@ -71,6 +72,7 @@ static GOptionEntry options[] = {
{ "from-file", 0, 0, G_OPTION_ARG_STRING, &opt_from_file, "Process many checkouts from input file", "FILE" },
{ "fsync", 0, 0, G_OPTION_ARG_CALLBACK, parse_fsync_cb, "Specify how to invoke fsync()", "POLICY" },
{ "require-hardlinks", 'H', 0, G_OPTION_ARG_NONE, &opt_require_hardlinks, "Do not fall back to full copies if hardlinking fails", NULL },
{ "force-copy", 'C', 0, G_OPTION_ARG_NONE, &opt_force_copy, "Never hardlink (but may reflink if available)", NULL },
{ NULL }
};

Expand All @@ -89,7 +91,7 @@ process_one_checkout (OstreeRepo *repo,
* `ostree_repo_checkout_at` until such time as we have a more
* convenient infrastructure for testing C APIs with data.
*/
if (opt_disable_cache || opt_whiteouts || opt_require_hardlinks || opt_union_add)
if (opt_disable_cache || opt_whiteouts || opt_require_hardlinks || opt_union_add || opt_force_copy)
{
OstreeRepoCheckoutAtOptions options = { 0, };

Expand All @@ -102,6 +104,11 @@ process_one_checkout (OstreeRepo *repo,
"Cannot specify both --union and --union-add");
goto out;
}
if (opt_require_hardlinks && opt_force_copy)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be checked at the API level too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ⬇️

{
glnx_throw (error, "Cannot specify both --require-hardlinks and --force-copy");
goto out;
}
else if (opt_union)
options.overwrite_mode = OSTREE_REPO_CHECKOUT_OVERWRITE_UNION_FILES;
else if (opt_union_add)
Expand All @@ -111,6 +118,7 @@ process_one_checkout (OstreeRepo *repo,
if (subpath)
options.subpath = subpath;
options.no_copy_fallback = opt_require_hardlinks;
options.force_copy = opt_force_copy;

if (!ostree_repo_checkout_at (repo, &options,
AT_FDCWD, destination,
Expand Down
11 changes: 10 additions & 1 deletion tests/basic-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

set -euo pipefail

echo "1..65"
echo "1..66"

$CMD_PREFIX ostree --version > version.yaml
python -c 'import yaml; yaml.safe_load(open("version.yaml"))'
Expand All @@ -41,6 +41,7 @@ fi
validate_checkout_basic() {
(cd $1;
assert_has_file firstfile
assert_not_streq $(stat -c '%h' firstfile) 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, this is failing in test-basic-user.sh. Surprisingly, not in the -U -H case, but just in the vanilla $OSTREE checkout test2 checkout-test2 test. Is there a bug there? Seems like it's hardlinking from a bare-user repo even without -U.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I read this backwards. The fact that it's failing means that it is copying rather than hardlinking, which is what we want in user-mode without -U. So I think this just needs some conditionals around it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this only works in bare right now. This whole suite really needs rewriting.

assert_has_file baz/cow
assert_file_has_content baz/cow moo
assert_has_file baz/deeper/ohyeah
Expand Down Expand Up @@ -78,6 +79,14 @@ fi
fi
echo "ok checkout -H"

rm checkout-test2 -rf
$OSTREE checkout -C test2 checkout-test2
for file in firstfile baz/cow baz/alink; do
assert_streq $(stat -c '%h' checkout-test2/$file) 1
done

echo "ok checkout -C"

$OSTREE rev-parse test2
$OSTREE rev-parse 'test2^'
$OSTREE rev-parse 'test2^^' 2>/dev/null && fatal "rev-parse test2^^ unexpectedly succeeded!"
Expand Down