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

> On Mon, Nov 17, 2025 at 04:27:59PM +0000, Phillip Wood wrote:
>> 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"
> 
> I was trying to avoid using a strvec, but honestly that turned out to be
> more pain than it is worth. We don't have functions like
> `strvec_splice()` for simple arrays, and there is no commit array struct
> that provides similar wrappers, either.

>>   > +{
>>> +	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?
> 
> It's basically a check that the revision arguments have all been
> consumed, except for the initial empty argument. The interface is a bit
> weird.
> 
> [snip]
>>> +		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?
> 
> Good point, yes.
> 
>>> +		}
>>> +	}
>>> +
>>> +	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
> 
> We're not switching branches, true, but we do switch to the rewritten
> commit. Also I'm not sure that printing the commit subject here would
> make sense, as the question becomes which subject to print: the one
> we're moving to, which is the new tip of the branch but may not be the
> commit we have rewritten? Or do we print the subject of the rewritten
> commit?
> 
>>> +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?
> 
> The "Please edit the commit message" thing is taken from other commands
> that phrase it similarly. But it certainly does make sense to note that
> clearing the commit message aborts, will add.
> 
>>> +		  " 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?
> 
> Yeah, some of it is. We don't handle them all yet, but this will be
> backfilled in the future.
> 
>>> +	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.
> 
> Ah, this is a leftover error message from previous iterations.
> 
>>> +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
> 
>>> 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?
> 
> I felt it was dirty to randomly add it to any of the other test suites,
> so I decided to instead have it in its own standalone file. It may also
> become relevant in the future if we ever needed commands like for
> example `git history --continue`, same as the sequencer-based commands
> have.
> 
>>> 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
> 
> True. Will use this:
> 
> diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh
> index 8b353e74dc..4c87953176 100755
> --- a/t/t3451-history-reword.sh
> +++ b/t/t3451-history-reword.sh
> @@ -6,11 +6,11 @@ test_description='tests for git-history reword subcommand'
>   
>   reword_with_message () {
>   	cat >message &&
> -	write_script fake-editor.sh <<-EOF &&
> -	cp "$(pwd)/message" "\$1"
> +	write_script fake-editor.sh <<-\EOF &&
> +	cp "$ORIG_PATH/message" "$1"
>   	EOF
>   	test_set_editor "$(pwd)"/fake-editor.sh &&
> -	git history reword "$@" &&
> +	ORIG_PATH="$(pwd)" git history reword "$@" &&
>   	rm fake-editor.sh message
>   }
>   
> 
>>> +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.
> 
> I prefer that style as it is extremely hard to reason about tests that
> have interdependencies, and not all the state may be removed by a hard
> reset.
> 
>>> +	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.
> 
> Ah, indeed.
> 
>>> +	)
>>> +'
>>> +
>>> +# 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.
> 
> I'll rephrain from doing this as it would require `<<-EOF` instead of
> `<<-\EOF`.
> 
>>> +		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.
> 
> Thanks!
> 
> Patrick
>