commit c3c995ae6f30b762ec75f5ef4831e6108ee60ae0
from: Stefan Sperling <stsp@stsp.name>
date: Tue Jan 28 09:00:54 2025 UTC

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

commit - ed016e87863a316dcfbc8a0bc09e2b4574e3934a
commit + c3c995ae6f30b762ec75f5ef4831e6108ee60ae0
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
@@ -276,6 +276,14 @@ got_pack_paint_commits(int *ncolored, struct got_objec
 		case COLOR_KEEP:
 			if (got_object_idset_contains(drop, &qid->id)) {
 				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
@@ -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
@@ -411,6 +411,14 @@ got_pack_paint_commits(int *ncolored, struct got_objec
 		case COLOR_KEEP:
 			if (got_object_idset_contains(drop, &qid->id)) {
 				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
@@ -422,6 +430,14 @@ got_pack_paint_commits(int *ncolored, struct got_objec
 		case COLOR_DROP:
 			if (got_object_idset_contains(keep, &qid->id)) {
 				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
blob - 6b8fdb74f13c5157983c8ba15522d812f4865a83
blob + 5038d17342275e2c833422660801b878be6e5f4e
--- regress/cmdline/pack.sh
+++ regress/cmdline/pack.sh
@@ -704,6 +704,67 @@ test_pack_tagged_tag() {
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "gotadmin pack failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	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
@@ -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