Re: [PATCH v3 3/4] history: add squash subcommand to fold a range

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jun 18, 2026 at 07:17:05PM +0000, Harald Nordgren via GitGitGadget wrote:
> diff --git a/builtin/history.c b/builtin/history.c
> index 305bde3102..9d9416870f 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -973,6 +975,156 @@ out:
>  	return ret;
>  }
>  
> +/*
> + * Resolve a "<base>..<tip>" revision range into the base commit just outside
> + * the range (which becomes the parent of the squashed commit), the oldest
> + * commit contained in the range (whose message the squash reuses), and the
> + * range tip (whose tree becomes the result). A merge inside the range is fine,
> + * but the range must have a single base and must not reach a root commit.
> + */
> +static int resolve_squash_range(struct repository *repo,
> +				const char *range,
> +				struct commit **base_out,
> +				struct commit **oldest_out,
> +				struct commit **tip_out)
> +{
> +	struct rev_info revs;
> +	struct commit *commit, *base = NULL, *oldest = NULL, *tip = NULL;
> +	struct strvec args = STRVEC_INIT;
> +	int ret;
> +
> +	repo_init_revisions(repo, &revs, NULL);
> +	strvec_push(&args, "ignored");
> +	strvec_push(&args, "--reverse");
> +	strvec_push(&args, "--topo-order");
> +	strvec_push(&args, "--boundary");
> +	strvec_push(&args, range);

We don't have any kind of input verification for "range". So in theory,
the user could pass whatever string here, and this may or may not work.

Also, should we use "--ancestry-path" with the first commit of the range
here? Otherwise we may incldue commits that aren't descendants of A in a
range "A..B". If not I wonder whether we might see multiple boundaries
even though we would be able to resolve the boundary unambiguously in
some cases.

> +	setup_revisions_from_strvec(&args, &revs, NULL);
> +	if (args.nr != 1) {
> +		ret = error(_("'%s' does not name a revision range"), range);
> +		goto out;
> +	}
> +
> +	if (prepare_revision_walk(&revs) < 0) {
> +		ret = error(_("error preparing revisions"));
> +		goto out;
> +	}
> +
> +	while ((commit = get_revision(&revs))) {
> +		if (commit->object.flags & BOUNDARY) {
> +			if (base) {
> +				ret = error(_("range '%s' has more than one base; "
> +					      "cannot squash"), range);
> +				goto out;
> +			}
> +			base = commit;
> +			continue;
> +		}
> +		if (!oldest)
> +			oldest = commit;
> +		tip = commit;
> +	}

Hmm. I really wonder whether we should also restrict merges. It might be
somewhat obvious that intermediate merge commits should just be
discarded. But is that equally obvious for HEAD and the base commit?

> +	if (!oldest) {
> +		ret = error(_("the range '%s' is empty"), range);
> +		goto out;
> +	}
> +
> +	if (!base) {
> +		ret = error(_("cannot squash the root commit"));
> +		goto out;
> +	}

In theory we can by squashing onto an empty tree. But it's fine to not
care about this edge case, we can still address it at a later point in
time if we ever feel the need to.

> +	*base_out = base;
> +	*oldest_out = oldest;
> +	*tip_out = tip;
> +	ret = 0;
> +
> +out:
> +	reset_revision_walk();
> +	release_revisions(&revs);
> +	strvec_clear(&args);
> +	return ret;
> +}
> +
> +static int cmd_history_squash(int argc,
> +			      const char **argv,
> +			      const char *prefix,
> +			      struct repository *repo)
> +{
> +	const char * const usage[] = {
> +		GIT_HISTORY_SQUASH_USAGE,
> +		NULL,
> +	};
> +	enum ref_action action = REF_ACTION_DEFAULT;
> +	enum commit_tree_flags flags = 0;
> +	int dry_run = 0;
> +	struct option options[] = {
> +		OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
> +			       N_("control which refs should be updated"),
> +			       PARSE_OPT_NONEG, parse_ref_action),
> +		OPT_BOOL('n', "dry-run", &dry_run,
> +			 N_("perform a dry-run without updating any refs")),
> +		OPT_BIT(0, "reedit-message", &flags,
> +			N_("open an editor to modify the commit message"),
> +			COMMIT_TREE_EDIT_MESSAGE),
> +		OPT_END(),
> +	};
> +	struct strbuf reflog_msg = STRBUF_INIT;
> +	struct commit *base, *oldest, *tip, *rewritten;
> +	const struct object_id *base_tree_oid, *tip_tree_oid;
> +	struct commit_list *parents = NULL;
> +	struct rev_info revs = { 0 };
> +	int ret;
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +	if (argc != 1) {
> +		ret = error(_("command expects a single revision range"));
> +		goto out;
> +	}
> +	repo_config(repo, git_default_config, NULL);
> +
> +	if (action == REF_ACTION_DEFAULT)
> +		action = REF_ACTION_BRANCHES;
> +
> +	ret = resolve_squash_range(repo, argv[0], &base, &oldest, &tip);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = setup_revwalk(repo, action, tip, &revs);
> +	if (ret < 0)
> +		goto out;

Oh, you already use `setup_revwalk()` here. Wouldn't that keep us from
accepting merge commits?

Patrick