From 88b16410211fb0741a604736b34531fe3d56b8fd Mon Sep 17 00:00:00 2001 From: Stefan Sperling Date: Tue, 28 Jan 2025 09:00:54 +0000 Subject: [PATCH] fix pack exclusion via an ancestor commit 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 test case, these 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. Add a test which triggers the bug via gotadmin pack. ok jamsek --- lib/got_lib_pack_create.h | 3 ++ lib/pack_create.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++ lib/pack_create_io.c | 16 +++++++++++ lib/pack_create_privsep.c | 16 +++++++++++ regress/cmdline/pack.sh | 62 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 168 insertions(+) diff --git a/lib/got_lib_pack_create.h b/lib/got_lib_pack_create.h index 265d2bf3..ca45026c 100644 --- a/lib/got_lib_pack_create.h +++ b/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); diff --git a/lib/pack_create.c b/lib/pack_create.c index f69fe8b0..fbe87cbd 100644 --- a/lib/pack_create.c +++ b/lib/pack_create.c @@ -1046,6 +1046,77 @@ 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 *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) { diff --git a/lib/pack_create_io.c b/lib/pack_create_io.c index 50fcf48f..67691346 100644 --- a/lib/pack_create_io.c +++ b/lib/pack_create_io.c @@ -277,6 +277,14 @@ got_pack_paint_commits(int *ncolored, struct got_object_id_queue *ids, int nids, 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); @@ -288,6 +296,14 @@ got_pack_paint_commits(int *ncolored, struct got_object_id_queue *ids, int nids, 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); diff --git a/lib/pack_create_privsep.c b/lib/pack_create_privsep.c index db73fef0..47150180 100644 --- a/lib/pack_create_privsep.c +++ b/lib/pack_create_privsep.c @@ -411,6 +411,14 @@ got_pack_paint_commits(int *ncolored, struct got_object_id_queue *ids, int nids, 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); @@ -422,6 +430,14 @@ got_pack_paint_commits(int *ncolored, struct got_object_id_queue *ids, int nids, 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); diff --git a/regress/cmdline/pack.sh b/regress/cmdline/pack.sh index 6b8fdb74..5038d173 100755 --- a/regress/cmdline/pack.sh +++ b/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 -- 2.11.4.GIT