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

> 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?

> > +		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

> > 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