[PATCH v10 3.5/3.7] fixup! history: add squash subcommand to fold a range

From: Phillip Wood <[email protected]>

Refuse to squash if there are branches descended from squashed commits

Rework the detection of branches descended from squashed commits
to protect branches that are not updated (i.e. any branch that is
descended from the squashed range but not the tip of the range). The
current behavior of protecting only those branches that point directly
at a squashed commit but not those descended from the same commit
is inconsistent and confusing. Note that this change means we now
complain when a branch points to the start of the squashed range.

When walking the commits we only need to add those with a parent
outside the squashed range to the ref filter as all the other commits
are descended from those. This patch needs some polishing to print the
branches that are causing us to error out. I've updated the existing
tests to reflect that, though I'm unclear what extra coverage the last
test (squashes a range whose internal merge has a single base) adds.

We do not protect detached HEADs that point to a commit in the
squashed range. I think that's reasonable - if HEAD is detached,
the user is likely experimenting so does not necessarily care that
the commits are being squashed in a different worktree. Protecting
them would require a separate revision walk including the detached
HEADs from all worktrees and excluding the base of the squashed range.

Signed-off-by: Phillip Wood <[email protected]>
---
 builtin/history.c         | 85 +++++++++++++++++----------------------
 t/t3455-history-squash.sh | 67 ++++++++++++++----------------
 2 files changed, 68 insertions(+), 84 deletions(-)

diff --git a/builtin/history.c b/builtin/history.c
index bb4a74ec1a..84e13fd75a 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -17,6 +17,7 @@
 #include "path.h"
 #include "read-cache.h"
 #include "refs.h"
+#include "ref-filter.h"
 #include "replay.h"
 #include "reset.h"
 #include "revision.h"
@@ -1019,16 +1020,18 @@ static int cmd_history_split(int argc,
  * but the range must have a single base and must not reach a root commit.
  */
 static int resolve_squash_range(struct repository *repo,
+				bool update_branches,
 				int argc, const char **argv,
 				struct commit **base_out,
 				struct commit **oldest_out,
-				struct commit **tip_out,
-				struct oidset *interior_out)
+				struct commit **tip_out)
 {
 	struct rev_info revs;
 	struct commit *commit, *base = NULL, *oldest = NULL, *tip = NULL;
 	size_t i;
 	int ret, tip_count = 0;
+	struct ref_filter filter = REF_FILTER_INIT;
+	struct ref_array refs = { 0 };
 
 	repo_init_revisions(repo, &revs, NULL);
 	revs.reverse = 1;
@@ -1101,8 +1104,12 @@ static int resolve_squash_range(struct repository *repo,
 			 * Allow parents that match the parents of the
 			 * squashed commit.
 			 */
-			for (q = oldest->parents; !seen && q; q = q->next)
-				seen = p->item == q->item;
+			for (q = oldest->parents; !seen && q; q = q->next) {
+				if (p->item == q->item) {
+					seen = true;
+					commit_list_insert(commit, &filter.with_commit);
+				}
+			}
 			if (!seen) {
 				ret = error(_("parent %s of commit %s is "
 					      "outside the revision range"),
@@ -1117,14 +1124,16 @@ static int resolve_squash_range(struct repository *repo,
 				o->flags &= ~SQUASH_TIP;
 			}
 		}
-		if (!oldest)
+		if (!oldest) {
+			commit_list_insert(commit, &filter.with_commit);
 			oldest = commit;
-		if (tip)
-			oidset_insert(interior_out, &tip->object.oid);
+		}
 		tip = commit;
 		tip->object.flags |= SQUASH_SEEN | SQUASH_TIP;
 		tip_count++;
 	}
+	clear_object_flags(repo, SQUASH_SEEN | SQUASH_TIP);
+	reset_revision_walk();
 	if (!tip_count) {
 		ret = error(_("the revision range is empty"));
 		goto out;
@@ -1138,6 +1147,24 @@ static int resolve_squash_range(struct repository *repo,
 		goto out;
 	} else if (!oldest->parents) {
 		BUG("an in-range commit must have a parent");
+	}
+	commit_list_insert(tip, &filter.no_commit);
+	filter.kind = FILTER_REFS_BRANCHES;
+	if (update_branches &&
+	    filter_refs(&refs, &filter, filter.kind)) {
+		ret = error(_("could not filter refs"));
+		goto out;
+	}
+	if (refs.nr) {
+		/*
+		 * TODO: list the branches and also check HEADS from other worktrees
+		 */
+		ret = error(_("a branch points to a commit that is being squashed"));
+		advise_if_enabled(ADVICE_HISTORY_UPDATE_REFS,
+				  _("Use --update-refs=head to rewrite only "
+				    "the current branch and leave such refs "
+				    "untouched."));
+		goto out;
 	}
 	base = oldest->parents->item;
 
@@ -1147,9 +1174,9 @@ static int resolve_squash_range(struct repository *repo,
 	ret = 0;
 
 out:
-	clear_object_flags(repo, SQUASH_SEEN | SQUASH_TIP);
-	reset_revision_walk();
 	release_revisions(&revs);
+	ref_filter_clear(&filter);
+	ref_array_clear(&refs);
 	return ret;
 }
 
@@ -1261,23 +1288,6 @@ static int reject_dangling_fixups(struct repository *repo,
 	release_revisions(&revs);
 	strvec_clear(&args);
 	return ret;
-}
-
-struct interior_ref_cb {
-	const struct oidset *interior;
-	const char *name;
-};
-
-static int find_interior_ref(const struct reference *ref, void *cb_data)
-{
-	struct interior_ref_cb *data = cb_data;
-
-	if (oidset_contains(data->interior, ref->oid)) {
-		data->name = xstrdup(ref->name);
-		return 1;
-	}
-
-	return 0;
 }
 
 static int cmd_history_squash(int argc,
@@ -1305,7 +1315,6 @@ static int cmd_history_squash(int argc,
 	};
 	struct strbuf reflog_msg = STRBUF_INIT;
 	struct strbuf message = STRBUF_INIT;
-	struct oidset interior = OIDSET_INIT;
 	struct commit *base, *oldest, *tip, *rewritten, *msg_source,
 		*amend_source;
 	const struct object_id *base_tree_oid, *tip_tree_oid;
@@ -1328,8 +1337,8 @@ static int cmd_history_squash(int argc,
 	strbuf_addstr(&reflog_msg, "squash: updating ");
 	strbuf_join_argv(&reflog_msg, argc - 1, argv + 1, ' ');
 
-	ret = resolve_squash_range(repo, argc, argv, &base, &oldest, &tip,
-				   &interior);
+	ret = resolve_squash_range(repo, action == REF_ACTION_BRANCHES,
+				   argc, argv, &base, &oldest, &tip);
 	if (ret < 0)
 		goto out;
 
@@ -1347,23 +1356,6 @@ static int cmd_history_squash(int argc,
 		strbuf_addstr(&message, body);
 		message_template = message.buf;
 		repo_unuse_commit_buffer(repo, amend_source, amend_message);
-	}
-
-	if (action == REF_ACTION_BRANCHES) {
-		struct interior_ref_cb cb = { .interior = &interior };
-
-		refs_for_each_ref(get_main_ref_store(repo),
-				  find_interior_ref, &cb);
-		if (cb.name) {
-			ret = error(_("'%s' points into the squashed range"),
-				    cb.name);
-			advise_if_enabled(ADVICE_HISTORY_UPDATE_REFS,
-					  _("Use --update-refs=head to rewrite only "
-					    "the current branch and leave such refs "
-					    "untouched."));
-			free((char *)cb.name);
-			goto out;
-		}
 	}
 
 	ret = setup_revwalk(repo, action, tip, &revs);
@@ -1395,7 +1387,6 @@ static int cmd_history_squash(int argc,
 out:
 	strbuf_release(&reflog_msg);
 	strbuf_release(&message);
-	oidset_clear(&interior);
 	commit_list_free(parents);
 	release_revisions(&revs);
 	return ret;
diff --git a/t/t3455-history-squash.sh b/t/t3455-history-squash.sh
index ba826df592..d7697489a0 100755
--- a/t/t3455-history-squash.sh
+++ b/t/t3455-history-squash.sh
@@ -305,17 +305,36 @@ test_expect_success '--update-refs=head only moves HEAD' '
 	test_cmp_rev "$other_before" other
 '
 
-test_expect_success 'refuses to fold a range a ref points into' '
-	git reset --hard three &&
-	git branch -f mid HEAD~1 &&
-	head_before=$(git rev-parse HEAD) &&
+test_expect_success 'refuses to fold a range a branch points into' '
+	test_when_finished \
+		"git switch -f $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME; \
+		 git branch -D feature" &&
+	git checkout -f -b feature start &&
+	test_commit C1 &&
+	test_commit C2 &&
+	git checkout -b topic-1 start &&
+	test_commit C3 &&
+	test_commit C4 &&
+	git checkout C3 &&
+	test_commit C5 &&
+	git checkout feature &&
+	git merge C5 &&
+	test_commit C6 &&
+	git checkout -b topic-2 C2 &&
+	test_commit C7 &&
+	git checkout feature &&
 
 	test_must_fail git history squash start.. 2>err &&
-	test_grep "error: .* points into the squashed range" err &&
-	test_grep "hint: .*--update-refs=head" err &&
-	test_cmp_rev "$head_before" HEAD &&
+	# TODO: check the branch names when we print them (topic-1 & topic-2)
+	test_grep "^error: a branch points to" err &&
+	test_grep "^hint: .* --update-refs=head" err &&
+	test_cmp_rev C6 HEAD &&
 
-	git branch -D mid
+	# squash succeeds with --update-refs=head
+	git history squash --update-refs=head start.. &&
+	test_cmp_rev start HEAD^ &&
+	test_cmp_rev C6^{tree} HEAD^{tree} &&
+	test_cmp_rev C6 HEAD@{1}
 '
 
 test_expect_success 'advice.historyUpdateRefs silences the hint' '
@@ -325,37 +344,11 @@ test_expect_success 'advice.historyUpdateRefs silences the hint' '
 
 	test_must_fail git -c advice.historyUpdateRefs=false \
 		history squash start.. 2>err &&
-	test_grep "points into the squashed range" err &&
+	test_grep "^error: a branch points to" err &&
 	test_grep ! "hint:" err &&
 	test_cmp_rev "$head_before" HEAD &&
 
 	git branch -D mid
-'
-
-test_expect_success '--update-refs=head folds past a ref pointing into the range' '
-	git reset --hard three &&
-	git branch -f mid HEAD~1 &&
-	mid_before=$(git rev-parse mid) &&
-
-	git history squash --update-refs=head start.. &&
-
-	check_commit_count start..HEAD 1 &&
-	test_cmp_rev "$mid_before" mid &&
-
-	git branch -D mid
-'
-
-test_expect_success 'refuses to fold a range a tag points into' '
-	git reset --hard three &&
-	git tag -f mark HEAD~1 &&
-	head_before=$(git rev-parse HEAD) &&
-
-	test_must_fail git history squash start.. 2>err &&
-	test_grep "refs/tags/mark" err &&
-	test_grep "points into the squashed range" err &&
-	test_cmp_rev "$head_before" HEAD &&
-
-	git tag -d mark
 '
 
 test_expect_success 'squashes a range whose internal merge has a single base' '
@@ -582,8 +575,8 @@ test_expect_success 'refuses to fold a range a ref points into at a merge' '
 	head_before=$(git rev-parse HEAD) &&
 
 	test_must_fail git history squash start.. 2>err &&
-	test_grep "at-merge" err &&
-	test_grep "points into the squashed range" err &&
+	# TODO: test for branch nome "at-merge"
+	test_grep "a branch points to a commit" err &&
 	test_cmp_rev "$head_before" HEAD &&
 
 	git branch -D at-merge
-- 
2.54.0.200.gfd8d68259e3