From: Phillip Wood Date: Wed, 10 Dec 2025 09:52:52 GMT Subject: Re: [PATCH v6 05/11] builtin/history: implement "reword" subcommand Message-ID: <266a9cfc-142f-49c0-a2a0-1d67426479b9@gmail.com> In-Reply-To: On 02/12/2025 18:50, Patrick Steinhardt wrote: > 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. I'm surprised it was such a pain compared to the cost to using a strvec. We're forever converting from a string to a struct commit and back again which bloats the code and obscures the interesting and important parts. That cost will be paid each time we add a new subcommand. An implementation of 'struct commit_vec' that implements commit_vec_push(), commit_vec_splice() and commit_vec_clear() is only going to be 30 or 40 lines of code and gives us a solid foundation for this series. Open coding the array and adding a SPLICE_ARRAY macro would also be pretty simple. Thanks Phillip >> > +{ >>> + 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: >> " 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 >> 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 >