Re: [PATCH v6 05/11] builtin/history: implement "reword" subcommand

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Patrick
On 27/10/2025 11:33, Patrick Steinhardt wrote:

> +static int collect_commits(struct repository *repo,
> +			   struct commit *old_commit,
> +			   struct commit *new_commit,
> +			   struct strvec *out)

Now that we're not using the sequencer it would be nice to stop messing 
about converting object ids to and from strings and return an array of 
"struct commit" instead of "struct strvec"
  > +{
> +	struct setup_revision_opt revision_opts = {
> +		.assume_dashdash = 1,
> +	};
> +	struct strvec revisions = STRVEC_INIT;
> +	struct commit *child;
> +	struct rev_info rev = { 0 };
> +	int ret;
> +
> +	repo_init_revisions(repo, &rev, NULL);
> +	strvec_push(&revisions, "");
> +	strvec_push(&revisions, oid_to_hex(&new_commit->object.oid));
> +	if (old_commit)
> +		strvec_pushf(&revisions, "^%s", oid_to_hex(&old_commit->object.oid));
> +
> +	setup_revisions_from_strvec(&revisions, &rev, &revision_opts);
> +	if (revisions.nr != 1 || prepare_revision_walk(&rev)) {

I'm not that familiar with the revision walking api, what 'revisions.nr 
!= 1' check for here?

> +static int apply_commits(struct repository *repo,
> +			 const struct strvec *commits,
> +			 struct commit *onto,
> +			 struct commit *orig_head,
> +			 const char *action)
> +{
> +	struct reset_head_opts reset_opts = { 0 };
> +	struct strbuf buf = STRBUF_INIT;
> +	int ret;
> +
> +	for (size_t i = 0; i < commits->nr; i++) {
> +		struct object_id commit_id;
> +		struct commit *commit;
> +		const char *end;
> +
> +		if (parse_oid_hex_algop(commits->v[i], &commit_id, &end,
> +					repo->hash_algo)) {
> +			ret = error(_("invalid object ID: %s"), commits->v[i]);
> +			goto out;
> +		}
> +		commit = lookup_commit(repo, &commit_id);
> +		if (!commit || repo_parse_commit(repo, commit)) {
> +			ret = error(_("failed to look up commit: %s"), oid_to_hex(&commit_id));
> +			goto out;
> +		}


Using an array of "struct commit" rather than "struct strvec" would mean 
we could delete everything in the loop body up to here.

> +		if (!onto) {
> +			onto = commit;
> +		} else {
> +			struct tree *tree = repo_get_commit_tree(repo, commit);
> +			onto = replay_create_commit(repo, tree, commit, onto);
> +			if (!onto)
> +				break;

Don't we want to avoid updating HEAD if replay_create_commit() fails?

> +		}
> +	}
> +
> +	reset_opts.oid = &onto->object.oid;
> +	strbuf_addf(&buf, "%s: switch to rewritten %s", action, oid_to_hex(reset_opts.oid));

We're not switching branches so I wonder if saying "history: <action> 
<oid> <commit subject>" might be a more useful reflog entry

> +static int fill_commit_message(struct repository *repo,
> +			       const struct object_id *old_tree,
> +			       const struct object_id *new_tree,
> +			       const char *default_message,
> +			       const char *action,
> +			       struct strbuf *out)
> +{
> +	const char *path = git_path_commit_editmsg();
> +	const char *hint =
> +		_("Please enter the commit message for the %s changes."

Maybe "Please edit the commit message"? Also do we want to tell the user 
they can abort by clearing the commit message?

> +		  " Lines starting\nwith '%s' will be ignored.\n");
> +	struct wt_status s;
> +
> +	strbuf_addstr(out, default_message);
> +	strbuf_addch(out, '\n');
> +	strbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);
> +	write_file_buf(path, out->buf, out->len);
> +
> +	wt_status_prepare(repo, &s);
> +	FREE_AND_NULL(s.branch);
> +	s.ahead_behind_flags = AHEAD_BEHIND_QUICK;
> +	s.commit_template = 1;
> +	s.colopts = 0;
> +	s.display_comment_prefix = 1;
> +	s.hints = 0;
> +	s.use_color = 0;
> +	s.whence = FROM_COMMIT;
> +	s.committable = 1;

"git commit" reads a load of status related config settings, is any of 
that relevant here?

> +	s.fp = fopen(git_path_commit_editmsg(), "a");
> +	if (!s.fp)
> +		return error_errno(_("could not open '%s'"), git_path_commit_editmsg());
> +
> +	wt_status_collect_changes_trees(&s, old_tree, new_tree);
> +	wt_status_print(&s);
> +	wt_status_collect_free_buffers(&s);
> +	string_list_clear_func(&s.change, change_data_free);
> +
> +	strbuf_reset(out);
> +	if (launch_editor(path, out, NULL)) {
> +		fprintf(stderr, _("Please supply the message using the -m option.\n"));

I'm not sure that's a very helpful suggestion as we don't support "-m" 
(it's not very helpful when "git commit --amend" suggests it either). We 
should just give up if the editor fails.

> +static int cmd_history_reword(int argc,
> +			      const char **argv,
> +			      const char *prefix,
> +			      struct repository *repo)
> +{
> +	const char * const usage[] = {
> +		GIT_HISTORY_REWORD_USAGE,
> +		NULL,
> +	};
> +	struct option options[] = {
> +		OPT_END(),
> +	};
> +	struct strbuf final_message = STRBUF_INIT;
> +	struct commit *original_commit, *parent, *head;
> +	struct strvec commits = STRVEC_INIT;
> +	struct object_id parent_tree_oid, original_commit_tree_oid;
> +	struct object_id rewritten_commit;
> +	struct commit_list *from_list = NULL;
> +	const char *original_message, *original_body, *ptr;
> +	char *original_author = NULL;
> +	size_t len;
> +	int ret;
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +	if (argc != 1) {
> +		ret = error(_("command expects a single revision"));
> +		goto out;
> +	}
> +	repo_config(repo, git_default_config, NULL);
> +
> +	original_commit = lookup_commit_reference_by_name(argv[0]);
> +	if (!original_commit) {
> +		ret = error(_("commit to be reworded cannot be found: %s"), argv[0]);
> +		goto out;
> +	}
> +	original_commit_tree_oid = repo_get_commit_tree(repo, original_commit)->object.oid;

Looking at the implementation of repo_get_commit_tree() it can return NULL

> +
> +	parent = original_commit->parents ? original_commit->parents->item : NULL;
> +	if (parent) {
> +		if (repo_parse_commit(repo, parent)) {
> +			ret = error(_("unable to parse commit %s"),
> +				    oid_to_hex(&parent->object.oid));
> +			goto out;
> +		}
> +		parent_tree_oid = repo_get_commit_tree(repo, parent)->object.oid;
> +	} else {
> +		oidcpy(&parent_tree_oid, repo->hash_algo->empty_tree);
> +	}
> +
> +	head = lookup_commit_reference_by_name("HEAD");
> +	if (!head) {
> +		ret = error(_("could not resolve HEAD to a commit"));
> +		goto out;
> +	}
> +
> +	commit_list_append(original_commit, &from_list);
> +	if (!repo_is_descendant_of(repo, head, from_list)) {
> +		ret = error (_("split commit must be reachable from current HEAD commit"));

s/split/reword/

> +		goto out;
> +	}
> +
> +	/*
> +	 * Collect the list of commits that we'll have to reapply now already.
> +	 * This ensures that we'll abort early on in case the range of commits
> +	 * contains merges, which we do not yet handle.
> +	 */

Ah, I wondered if we should be checking that the reworded commit wasn't 
a merge when we look at the parents above but we don't need to because 
we will error out here if it is.

> diff --git a/t/t3450-history.sh b/t/t3450-history.sh
> index 417c343d43b..f513463b92b 100755
> --- a/t/t3450-history.sh
> +++ b/t/t3450-history.sh
> @@ -5,13 +5,13 @@ test_description='tests for git-history command'
>   . ./test-lib.sh
>   
>   test_expect_success 'does nothing without any arguments' '
> -	git history >out 2>&1 &&
> -	test_must_be_empty out
> +	test_must_fail git history 2>err &&
> +	test_grep "need a subcommand" err
>   '
>   
>   test_expect_success 'raises an error with unknown argument' '
>   	test_must_fail git history garbage 2>err &&
> -	test_grep "unrecognized argument: garbage" err
> +	test_grep "unknown subcommand: .garbage." err
>   '
>   
>   test_done

Do we really need a separate test file just for a couple of tests. I can 
see that having a separate test file for each subcommand makes sense but 
can't we just add these two tests to one of those?

> diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh
> new file mode 100755
> index 00000000000..09dbc463c59
> --- /dev/null
> +++ b/t/t3451-history-reword.sh
> @@ -0,0 +1,237 @@
> +#!/bin/sh
> +
> +test_description='tests for git-history reword subcommand'
> +
> +. ./test-lib.sh
> +
> +reword_with_message () {
> +	cat >message &&
> +	write_script fake-editor.sh <<-EOF &&
> +	cp "$(pwd)/message" "\$1"

Let's hope $(pwd) doesn't contain any dollar signs, backticks, 
backslashes or double quotes. Doing

	export MSG_PATH="$(pwd)/message"
	write_script fake-editor.sh <<-\EOF &&
	cp "$MSG_PATH" "$1"
	EOF

would be safer

> +test_expect_success 'refuses to work with merge commits' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(

Do we really need to set up a separate repo for each test? The test 
suite is slow enough already without running "git init" followed by a 
bunch calls to test_commit() in each test. Can we instead run "git reset 
--hard <known-starting-point> at the beginning of each test? That 
removes any interdependence between tests but saves a bunch of processes.
> +test_expect_success 'can reword root commit' '

It's nice that unlike "git rebase" this works without much effort from 
the implementation.

> +test_expect_success 'can use editor to rewrite commit message' '

Don't all the other tests check that? This test is checking what's 
presented to the user which is a good idea but I wouldn't have guessed 
that from the test name.

> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit first &&
> +
> +		write_script fake-editor.sh <<-\EOF &&
> +		cp "$1" . &&
> +		printf "\namend a comment\n" >>"$1"
> +		EOF
> +		test_set_editor "$(pwd)"/fake-editor.sh &&
> +		git history reword HEAD &&
> +
> +		cat >expect <<-EOF &&
> +		first
> +
> +		# Please enter the commit message for the reworded changes. Lines starting
> +		# with ${SQ}#${SQ} will be ignored.
> +		# Changes to be committed:
> +		#	new file:   first.t
> +		#
> +		EOF
> +		test_cmp expect COMMIT_EDITMSG &&
> +
> +		cat >expect <<-EOF &&
> +		first
> +
> +		amend a comment
> +
> +		EOF
> +		git log --format=%B >actual &&
> +		test_cmp expect actual

We have test_commit_message() to do this which will accept the expected 
message on stdin.

> +	)
> +'
> +
> +# For now, git-history(1) does not yet execute any hooks. This is subject to
> +# change in the future, and if it does this test here is expected to start
> +# failing. In other words, this test is not an endorsement of the current
> +# status quo.
> +test_expect_success 'hooks are not executed for rewritten commits' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit first &&
> +		test_commit second &&
> +		test_commit third &&
> +
> +		write_script .git/hooks/prepare-commit-msg <<-EOF &&
> +		touch "$(pwd)/hooks.log

This has the same problem of expanding $(pwd) as fake-editor.sh. For 
debugging it would be nicer if the hook scripts did

	echo "$hook_name" >>hooks.log

so we can easily see which hooks are causing the test to fail.

> +		EOF
> +		write_script .git/hooks/post-commit <<-EOF &&
> +		touch "$(pwd)/hooks.log
> +		EOF
> +		write_script .git/hooks/post-rewrite <<-EOF &&
> +		touch "$(pwd)/hooks.log
> +		EOF

This is good idea. We should add tests for the "pre-commit" and 
"commit-msg" hooks as well.

Overall the test coverage looks good, the only thing we might want to 
add is a check for the reflog message. Thanks for working on this, I'll 
try and look at the rest of the patches sometime this week.

Phillip