From d3385a3014e173299094133d644763e34e5ecd52 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 5 Apr 2017 13:11:34 -0400 Subject: [PATCH] checkout: Provide useful error with checkout -H and incompat mode Previously we'd assert and dump core if one used `checkout -H` without `-U` on a bare-user repo, because we'd hit the bare-user symlink case. Rework the code to handle this, and add tests. I hit this when I was going to suggest to someone to use `-H` to ensure they were getting hardlinks. Closes: #779 Approved by: jlebon --- src/libostree/ostree-repo-checkout.c | 48 ++++++++++++++++++++++------ tests/basic-test.sh | 44 +++++++++++++++++++++---- 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/src/libostree/ostree-repo-checkout.c b/src/libostree/ostree-repo-checkout.c index 09966a94fd..392e16fda6 100644 --- a/src/libostree/ostree-repo-checkout.c +++ b/src/libostree/ostree-repo-checkout.c @@ -423,6 +423,7 @@ checkout_one_file_at (OstreeRepo *repo, gboolean ret = FALSE; const char *checksum; gboolean is_symlink; + gboolean is_bare_user_symlink = FALSE; gboolean can_cache; gboolean need_copy = TRUE; char loose_path_buf[_OSTREE_LOOSE_PATH_MAX]; @@ -431,7 +432,6 @@ checkout_one_file_at (OstreeRepo *repo, gboolean is_whiteout; is_symlink = g_file_info_get_file_type (source_info) == G_FILE_TYPE_SYMBOLIC_LINK; - checksum = ostree_repo_file_get_checksum ((OstreeRepoFile*)source); is_whiteout = !is_symlink && options->process_whiteouts && @@ -468,20 +468,42 @@ checkout_one_file_at (OstreeRepo *repo, while (current_repo) { - gboolean is_bare = ((current_repo->mode == OSTREE_REPO_MODE_BARE - && options->mode == OSTREE_REPO_CHECKOUT_MODE_NONE) || - (current_repo->mode == OSTREE_REPO_MODE_BARE_USER - && options->mode == OSTREE_REPO_CHECKOUT_MODE_USER - /* NOTE: bare-user symlinks are not stored as symlinks */ - && !is_symlink) || - (current_repo->mode == OSTREE_REPO_MODE_BARE_USER_ONLY - && options->mode == OSTREE_REPO_CHECKOUT_MODE_USER)); + /* TODO - Hoist this up to the toplevel at least for checking out from + * !parent; don't need to compute it for each file. + */ + gboolean repo_is_usermode = + current_repo->mode == OSTREE_REPO_MODE_BARE_USER || + current_repo->mode == OSTREE_REPO_MODE_BARE_USER_ONLY; + /* We're hardlinkable if the checkout mode matches the repo mode */ + gboolean is_hardlinkable = + (current_repo->mode == OSTREE_REPO_MODE_BARE + && options->mode == OSTREE_REPO_CHECKOUT_MODE_NONE) || + (repo_is_usermode && options->mode == OSTREE_REPO_CHECKOUT_MODE_USER); + gboolean is_bare = is_hardlinkable && !is_bare_user_symlink; gboolean current_can_cache = (options->enable_uncompressed_cache && current_repo->enable_uncompressed_cache); gboolean is_archive_z2_with_cache = (current_repo->mode == OSTREE_REPO_MODE_ARCHIVE_Z2 && options->mode == OSTREE_REPO_CHECKOUT_MODE_USER && current_can_cache); + /* NOTE: bare-user symlinks are not stored as symlinks; see + * https://github.com/ostreedev/ostree/commit/47c612e5a0688c3452a125655a245e8f4f01b2b0 + * as well as write_object(). + */ + is_bare_user_symlink = (repo_is_usermode && is_symlink); + + /* Verify if no_copy_fallback is set that we can hardlink, with a + * special exception for bare-user symlinks. + */ + if (options->no_copy_fallback && !is_hardlinkable && !is_bare_user_symlink) + { + glnx_throw (error, + repo_is_usermode ? + "User repository mode requires user checkout mode to hardlink" : + "Bare repository mode cannot hardlink in user checkout mode"); + goto out; + } + /* But only under these conditions */ if (is_bare || is_archive_z2_with_cache) { @@ -596,7 +618,13 @@ checkout_one_file_at (OstreeRepo *repo, /* Fall back to copy if we couldn't hardlink */ if (need_copy) { - g_assert (!options->no_copy_fallback); + /* Bare user mode can't hardlink symlinks, so we need to do a copy for + * those. (Although in the future we could hardlink inside checkouts) This + * assertion is intended to ensure that for regular files at least, we + * succeeded at hardlinking above. + */ + if (options->no_copy_fallback) + g_assert (is_bare_user_symlink); if (!ostree_repo_load_file (repo, checksum, &input, NULL, &xattrs, cancellable, error)) goto out; diff --git a/tests/basic-test.sh b/tests/basic-test.sh index 27b790266f..294854bf44 100644 --- a/tests/basic-test.sh +++ b/tests/basic-test.sh @@ -38,9 +38,45 @@ if grep bare-user-only repo/config; then CHECKOUT_U_ARG="-U" fi +validate_checkout_basic() { + (cd $1; + assert_has_file firstfile + assert_has_file baz/cow + assert_file_has_content baz/cow moo + assert_has_file baz/deeper/ohyeah + ) +} + $OSTREE checkout test2 checkout-test2 +validate_checkout_basic checkout-test2 echo "ok checkout" +# Note this tests bare-user *and* bare-user-only +rm checkout-test2 -rf +if grep bare-user repo/config; then + $OSTREE checkout -U -H test2 checkout-test2 +else + $OSTREE checkout -H test2 checkout-test2 +fi +validate_checkout_basic checkout-test2 +rm checkout-test2 -rf +# Only do these tests on bare-user/bare, not bare-user-only +# since the latter automatically synthesizes -U if it's not passed. +if ! grep -q bare-user-only repo/config; then +if grep -q bare-user repo/config; then + if $OSTREE checkout -H test2 checkout-test2 2>err.txt; then + assert_not_reached "checkout -H worked?" + fi + assert_file_has_content err.txt "User repository.*requires.*user" +else + if $OSTREE checkout -U -H test2 checkout-test2 2>err.txt; then + assert_not_reached "checkout -H worked?" + fi + assert_file_has_content err.txt "Bare repository mode cannot hardlink in user" +fi +fi +echo "ok checkout -H" + $OSTREE rev-parse test2 $OSTREE rev-parse 'test2^' $OSTREE rev-parse 'test2^^' 2>/dev/null && fatal "rev-parse test2^^ unexpectedly succeeded!" @@ -64,13 +100,9 @@ ostree_repo_init test-repo --mode=bare-user rm test-repo -rf echo "ok repo-init on existing repo" +rm checkout-test2 -rf +$OSTREE checkout test2 checkout-test2 cd checkout-test2 -assert_has_file firstfile -assert_has_file baz/cow -assert_file_has_content baz/cow moo -assert_has_file baz/deeper/ohyeah -echo "ok content" - rm firstfile $OSTREE commit ${COMMIT_ARGS} -b test2 -s delete