Download raw body.
fix pack exclusion via an ancestor commit
Stefan Sperling <stsp@stsp.name> wrote:
> When a commit is first discovered as a commit which should be included
> in the pack, but is later found to be a parent of a commit which should
> be excluded from the pack, gotadmin pack correctly excluded the commit
> itself but failed to exclude this commit's parents.
>
> This bug is the reason why our test suite did not notice that gotd
> was not protecting references when clients did not send a pack file.
> In our gotd test case, the parents are in the 'keep' set already and
> were never added to the 'skip' set. A useless pack was built which
> included those parents and nothing else.
>
> Extend test coverage to cover both code paths in gotd, and add a test
> which triggers the bug via gotadmin pack.
>
> ok?
ok (I didn't review the regress/gotd component of the diff
based on your follow-up mail)
> M lib/got_lib_pack_create.h | 3+ 0-
> M lib/pack_create.c | 71+ 0-
> M lib/pack_create_io.c | 16+ 0-
> M lib/pack_create_privsep.c | 16+ 0-
> M regress/cmdline/pack.sh | 62+ 0-
> M regress/gotd/repo_write_protected.sh | 124+ 0-
>
> 6 files changed, 292 insertions(+), 0 deletions(-)
>
> commit - 6ba505793ee92f3acf299c8d06b1409066094261
> commit + 0294cf74d9140a748615f1a7f127291acde3e139
> blob - 265d2bf372d90b0b2c3c124b8d9885056f6f1250
> blob + ca45026cc97b8e4d57488732f016db3ca87aaf80
> --- lib/got_lib_pack_create.h
> +++ lib/got_lib_pack_create.h
> @@ -55,6 +55,9 @@ enum got_pack_findtwixt_color {
>
> const struct got_error *got_pack_paint_commit(struct got_object_qid *qid,
> intptr_t color);
> +const struct got_error *got_pack_repaint_parent_commits(
> + struct got_object_id *commit_id, int color, struct got_object_idset *set,
> + struct got_object_idset *skip, struct got_repository *repo);
> const struct got_error *got_pack_queue_commit_id(
> struct got_object_id_queue *ids, struct got_object_id *id, intptr_t color,
> struct got_repository *repo);
> blob - 7cbe1bb9a78f7c06fed0b49bd3658873b9882862
> blob + 3bd5a64f277590883cfa6d747219b4c5509e10fd
> --- lib/pack_create.c
> +++ lib/pack_create.c
> @@ -1049,6 +1049,77 @@ got_pack_paint_commit(struct got_object_qid *qid, intp
> }
>
> const struct got_error *
> +got_pack_repaint_parent_commits(struct got_object_id *commit_id, int color,
> + struct got_object_idset *set, struct got_object_idset *skip,
> + struct got_repository *repo)
> +{
> + const struct got_error *err;
> + struct got_object_id_queue ids;
> + struct got_object_qid *qid;
> + struct got_commit_object *commit;
> + const struct got_object_id_queue *parents;
> +
> + STAILQ_INIT(&ids);
> +
> + err = got_object_open_as_commit(&commit, repo, commit_id);
> + if (err)
> + return err;
> +
> + while (commit) {
> + parents = got_object_commit_get_parent_ids(commit);
> + if (parents) {
> + struct got_object_qid *pid;
> + STAILQ_FOREACH(pid, parents, entry) {
> + /*
> + * No need to traverse parents which are
> + * already in the desired set or are
> + * marked for skipping already.
> + */
> + if (got_object_idset_contains(set, &pid->id))
> + continue;
> + if (skip != set &&
> + got_object_idset_contains(skip, &pid->id))
> + continue;
> +
> + err = got_pack_queue_commit_id(&ids, &pid->id,
> + color, repo);
> + if (err)
> + break;
> + }
> + }
> + got_object_commit_close(commit);
> + commit = NULL;
> +
> + qid = STAILQ_FIRST(&ids);
> + if (qid) {
> + STAILQ_REMOVE_HEAD(&ids, entry);
> + if (!got_object_idset_contains(set, &qid->id)) {
> + err = got_object_idset_add(set, &qid->id,
> + NULL);
> + if (err)
> + break;
> + }
> +
> + err = got_object_open_as_commit(&commit, repo,
> + &qid->id);
> + if (err)
> + break;
> +
> + got_object_qid_free(qid);
> + qid = NULL;
> + }
> + }
> +
> + if (commit)
> + got_object_commit_close(commit);
> + if (qid)
> + got_object_qid_free(qid);
> + got_object_id_queue_free(&ids);
> +
> + return err;
> +}
> +
> +const struct got_error *
> got_pack_queue_commit_id(struct got_object_id_queue *ids,
> struct got_object_id *id, intptr_t color, struct got_repository *repo)
> {
> blob - 0d826389e2105b3dffcab6046a757b0fdb83a8ad
> blob + b8011c9c9311767fc79faf56bc73ca31e59ebf37
> --- lib/pack_create_io.c
> +++ lib/pack_create_io.c
> @@ -278,6 +278,14 @@ got_pack_paint_commits(int *ncolored, struct got_objec
> err = got_pack_paint_commit(qid, COLOR_SKIP);
> if (err)
> goto done;
> + err = got_object_idset_add(skip, &qid->id,
> + NULL);
> + if (err)
> + goto done;
> + err = got_pack_repaint_parent_commits(&qid->id,
> + COLOR_SKIP, skip, skip, repo);
> + if (err)
> + goto done;
> } else
> (*ncolored)++;
> err = got_object_idset_add(keep, &qid->id, NULL);
> @@ -289,6 +297,14 @@ got_pack_paint_commits(int *ncolored, struct got_objec
> err = got_pack_paint_commit(qid, COLOR_SKIP);
> if (err)
> goto done;
> + err = got_object_idset_add(skip, &qid->id,
> + NULL);
> + if (err)
> + goto done;
> + err = got_pack_repaint_parent_commits(&qid->id,
> + COLOR_SKIP, skip, skip, repo);
> + if (err)
> + goto done;
> } else
> (*ncolored)++;
> err = got_object_idset_add(drop, &qid->id, NULL);
> blob - ea20b9c4b08199a59e5e822d3319990fd6f175fc
> blob + 05666f0e8981ce94a832dd0fd1ae61c5a4fdcb24
> --- lib/pack_create_privsep.c
> +++ lib/pack_create_privsep.c
> @@ -413,6 +413,14 @@ got_pack_paint_commits(int *ncolored, struct got_objec
> err = got_pack_paint_commit(qid, COLOR_SKIP);
> if (err)
> goto done;
> + err = got_object_idset_add(skip, &qid->id,
> + NULL);
> + if (err)
> + goto done;
> + err = got_pack_repaint_parent_commits(&qid->id,
> + COLOR_SKIP, skip, skip, repo);
> + if (err)
> + goto done;
> } else
> (*ncolored)++;
> err = got_object_idset_add(keep, &qid->id, NULL);
> @@ -424,6 +432,14 @@ got_pack_paint_commits(int *ncolored, struct got_objec
> err = got_pack_paint_commit(qid, COLOR_SKIP);
> if (err)
> goto done;
> + err = got_object_idset_add(skip, &qid->id,
> + NULL);
> + if (err)
> + goto done;
> + err = got_pack_repaint_parent_commits(&qid->id,
> + COLOR_SKIP, skip, skip, repo);
> + if (err)
> + goto done;
> } else
> (*ncolored)++;
> err = got_object_idset_add(drop, &qid->id, NULL);
> blob - 6b8fdb74f13c5157983c8ba15522d812f4865a83
> blob + 5038d17342275e2c833422660801b878be6e5f4e
> --- regress/cmdline/pack.sh
> +++ regress/cmdline/pack.sh
> @@ -711,6 +711,67 @@ test_pack_tagged_tag() {
> test_done "$testroot" "$ret"
> }
>
> +test_pack_exclude_via_ancestor_commit() {
> + local testroot=`test_init pack_exclude`
> + local commit0=`git_show_head $testroot/repo`
> +
> + # no pack files should exist yet
> + ls $testroot/repo/.git/objects/pack/ > $testroot/stdout
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + test_done "$testroot" "$ret"
> + return 1
> + fi
> + echo -n > $testroot/stdout.expected
> + cmp -s $testroot/stdout.expected $testroot/stdout
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + diff -u $testroot/stdout.expected $testroot/stdout
> + test_done "$testroot" "$ret"
> + return 1
> + fi
> +
> + got checkout $testroot/repo $testroot/wt > /dev/null
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + test_done "$testroot" "$ret"
> + return 1
> + fi
> +
> + for i in 1 2 3; do
> + echo a new line >> $testroot/wt/alpha
> + (cd $testroot/wt && got commit -m "edit alpha" >/dev/null)
> + done
> + local parent_commit=`git_show_head $testroot/repo`
> +
> + echo a new line >> $testroot/wt/alpha
> + (cd $testroot/wt && got commit -m "edit alpha" >/dev/null)
> +
> + got ref -r $testroot/repo -c $parent_commit refs/heads/pleasepackthis
> +
> + # Packing the 'pleasepackthis' branch while exluding commits
> + # reachable via 'master' should result in an empty pack file.
> + gotadmin pack -a -r $testroot/repo -x master pleasepackthis \
> + > $testroot/stdout 2> $testroot/stderr
> + ret=$?
> + if [ $ret -eq 0 ]; then
> + echo "gotadmin pack succeeded unexpectedly" >&2
> + test_done "$testroot" "1"
> + return 1
> + fi
> +
> + echo "gotadmin: not enough objects to pack" > $testroot/stderr.expected
> + cmp -s $testroot/stderr.expected $testroot/stderr
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + diff -u $testroot/stderr.expected $testroot/stderr
> + test_done "$testroot" "$ret"
> + return 1
> + fi
> +
> + test_done "$testroot" "$ret"
> +}
> +
> test_parseargs "$@"
> run_test test_pack_all_loose_objects
> run_test test_pack_exclude
> @@ -721,3 +782,4 @@ run_test test_pack_loose_only
> run_test test_pack_all_objects
> run_test test_pack_bad_ref
> run_test test_pack_tagged_tag
> +run_test test_pack_exclude_via_ancestor_commit
> blob - d1e70129d2724139bccbc574d043f24c9ec5c7cd
> blob + e4fbb5fd04e0bbf35f12d2246856ca38c8564fd3
> --- regress/gotd/repo_write_protected.sh
> +++ regress/gotd/repo_write_protected.sh
> @@ -309,8 +309,132 @@ test_modify_protected_branch() {
> test_done "$testroot" $ret
> }
>
> +test_modify_protected_branch_with_deep_history() {
> + local testroot=`test_init modify_protected_branch`
> +
> + got clone -a -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + echo "got clone failed unexpectedly" >&2
> + test_done "$testroot" 1
> + return 1
> + fi
> +
> + got checkout $testroot/repo-clone $testroot/wt >/dev/null
> +
> + for i in 1 2 3 4 5 6 7 8 9; do
> + echo "more alpha" >> $testroot/wt/alpha
> + (cd $testroot/wt && got commit -m "more" >/dev/null)
> + done
> + local commit_id=`git_show_head $testroot/repo-clone`
> + local parent_commit_id=`git_show_parent_commit $testroot/repo-clone \
> + "$commit_id"`
> +
> + # Modifying the branch by adding new commits on top should succeed.
> + got send -q -r $testroot/repo-clone 2> $testroot/stderr
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + echo "got send failed unexpectedly" >&2
> + test_done "$testroot" 1
> + return 1
> + fi
> +
> + # Verify that the send operation worked fine.
> + got clone -l ${GOTD_TEST_REPO_URL} | grep main > $testroot/stdout
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + echo "got clone -l failed unexpectedly" >&2
> + test_done "$testroot" "1"
> + return 1
> + fi
> +
> + echo "HEAD: refs/heads/main" > $testroot/stdout.expected
> + echo "refs/heads/main: $commit_id" >> $testroot/stdout.expected
> + cmp -s $testroot/stdout.expected $testroot/stdout
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + diff -u $testroot/stdout.expected $testroot/stdout
> + test_done "$testroot" $ret
> + return 1
> + fi
> +
> + # Attempt to remove the tip commit
> + (cd $testroot/wt && got update -c "$parent_commit_id" >/dev/null)
> + (cd $testroot/wt && got histedit -d >/dev/null)
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + echo "got histedit failed unexpectedly" >&2
> + test_done "$testroot" 1
> + return 1
> + fi
> +
> + # The client should reject sending without -f.
> + got send -q -r $testroot/repo-clone 2> $testroot/stderr
> + ret=$?
> + if [ $ret -eq 0 ]; then
> + echo "got send succeeded unexpectedly" >&2
> + test_done "$testroot" 1
> + return 1
> + fi
> +
> + echo -n 'got: refs/heads/main: branch on server has' \
> + > $testroot/stderr.expected
> + echo -n ' a different ancestry; either fetch changes' \
> + >> $testroot/stderr.expected
> + echo -n ' from server and then rebase or merge local' \
> + >> $testroot/stderr.expected
> + echo -n ' branch before sending, or ignore ancestry' \
> + >> $testroot/stderr.expected
> + echo -n ' with send -f (can lead to data loss on' \
> + >> $testroot/stderr.expected
> + echo ' server)' >> $testroot/stderr.expected
> +
> + if ! cmp -s $testroot/stderr.expected $testroot/stderr; then
> + diff -u $testroot/stderr.expected $testroot/stderr
> + test_done "$testroot" 1
> + return 1
> + fi
> +
> + # Try again with -f.
> + got send -q -r $testroot/repo-clone -f 2> $testroot/stderr
> + ret=$?
> + if [ $ret -eq 0 ]; then
> + echo "got send succeeded unexpectedly" >&2
> + test_done "$testroot" 1
> + return 1
> + fi
> +
> + if ! egrep -q '(gotsh|got-send-pack): refs/heads/main: reference is protected' \
> + $testroot/stderr; then
> + echo -n "error message unexpected or missing: " >&2
> + cat $testroot/stderr >&2
> + test_done "$testroot" 1
> + return 1
> + fi
> +
> + # Verify that the send -f operation did not have any effect.
> + got clone -l ${GOTD_TEST_REPO_URL} | grep main > $testroot/stdout
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + echo "got clone -l failed unexpectedly" >&2
> + test_done "$testroot" "1"
> + return 1
> + fi
> +
> + echo "HEAD: refs/heads/main" > $testroot/stdout.expected
> + echo "refs/heads/main: $commit_id" >> $testroot/stdout.expected
> + cmp -s $testroot/stdout.expected $testroot/stdout
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + diff -u $testroot/stdout.expected $testroot/stdout
> + fi
> +
> + test_done "$testroot" $ret
> +}
> +
> test_parseargs "$@"
> run_test test_create_protected_branch
> run_test test_modify_protected_tag_namespace
> run_test test_delete_protected_branch
> run_test test_modify_protected_branch
> +run_test test_modify_protected_branch_with_deep_history
--
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68
fix pack exclusion via an ancestor commit