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.

Sign up for GitHub

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

checkout: Provide useful error with checkout -H and incompat mode #779

Closed
wants to merge 3 commits into from
Closed

checkout: Provide useful error with checkout -H and incompat mode #779

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a806e30
checkout: Provide useful error with checkout -H and incompat mode
cgwalters Apr 5, 2017
5d48399
fixup! checkout: Provide useful error with checkout -H and incompat mode
cgwalters Apr 5, 2017
054fd76
fixup! checkout: Provide useful error with checkout -H and incompat mode
cgwalters Apr 12, 2017
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
43 changes: 33 additions & 10 deletions src/libostree/ostree-repo-checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,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 &&
Expand Down Expand Up @@ -468,20 +467,38 @@ 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);
/* NOTE: bare-user symlinks are not stored as symlinks */
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a e.g. (see comment in write_object()) here? It's not intuitive offhand why we can't hardlink symlinks in bare-user mode. It took me some digging through git logs to find 47c612e and the associated comment in write_object().

gboolean is_bare_symlink = (repo_is_usermode && is_symlink);
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 renamed is_bare_user_symlink ?

gboolean is_bare = is_hardlinkable && !is_bare_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);

/* 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_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)
{
Expand Down Expand Up @@ -596,7 +613,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 (!is_symlink)
Copy link
Member

Choose a reason for hiding this comment

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

Would the intent here be better expressed as:

if (options->no_copy_fallback)
  g_assert (is_bare_user_symlink);

?

g_assert (!options->no_copy_fallback);
if (!ostree_repo_load_file (repo, checksum, &input, NULL, &xattrs,
cancellable, error))
goto out;
Expand Down
44 changes: 38 additions & 6 deletions tests/basic-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!"
Expand All @@ -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

Expand Down