Commit Diff

Commit:
88b16410211fb0741a604736b34531fe3d56b8fd
From:
Stefan Sperling <stsp@stsp.name>
Via:
Thomas Adam <thomas@xteddy.org>
Date:
Message:
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
Actions:
Patch | Tree
commit - 81a1b9add11a52cde9675b1a2c85cae35e644418
commit + 88b16410211fb0741a604736b34531fe3d56b8fd
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 - f69fe8b0f0d9292efbd835da4ba3669bdceff718
blob + fbe87cbd49638e1213544fd4d21e34e72114ec3b
--- lib/pack_create.c
+++ lib/pack_create.c
@@ -1046,6 +1046,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 - 50fcf48fcebe173324fc948ae4d733cb77564b5d
blob + 676913461ef3b1753061d53ad542d422b6269657
--- lib/pack_create_io.c
+++ lib/pack_create_io.c
@@ -275,6 +275,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
@@ -288,6 +296,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 - db73fef0419f5e2e739953b796b27d540a11c841
blob + 47150180b95579e50c3b719155ffad96a8703195
--- lib/pack_create_privsep.c
+++ lib/pack_create_privsep.c
@@ -409,6 +409,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
@@ -420,6 +428,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