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 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? 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

2025-01-27 09:33 Stefan Sperling:
fix pack exclusion via an ancestor commit
- 2025-01-27 09:42 Stefan Sperling:
  fix pack exclusion via an ancestor commit
- - 2025-01-27 12:56 Stefan Sperling:
    fix pack exclusion via an ancestor commit