From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 27 Oct 2025 09:58:09 GMT
Subject: Re: [PATCH v5 06/12] builtin/history: implement "reword" subcommand
Message-ID: <aP9CMa3dDuV_RGnG@pks.im>
In-Reply-To: <xmqqa51kszvc.fsf@gitster.g>

On Tue, Oct 21, 2025 at 02:34:31PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > @@ -9,6 +9,7 @@ SYNOPSIS
> >  --------
> >  [synopsis]
> >  git history [<options>]
> > +git history reword [<options>] <commit>
> 
> It is curious that a command-less form is still listed here,
> especially since this command "requires" a subcommand.  I would have
> expected that there will be a single line here after implementing a
> single subcommand.

True. You can execute `git history` alone, but it doesn't do anything
useful except for giving you an error and all the potential subcommands.
That's in contrast to e.g. git-reflog(1), which supports a command-less
mode that does something useful.

> > +`reword <commit> [--message=<message>]`::
> 
> That should be `reword [--message=<message>] <commit>` no?

Indeed.

> > +	Rewrite the commit message of the specified commit. All the other
> > +	details of this commit remain unchanged. If no commit message is
> > +	provided, then this command will spawn an editor with the current
> > +	message of that commit.
> 
> As long as it takes more than one -m and concatenates them just like
> "git commit -m <message1> -m <message2>" does, I would not complain
> too much that a command line option to give message encourages sloppy
> log messages.

I'll for now defer the discussion around "-m" completely, as the design
isn't entirely clear yet in the first place. So I'll just drop the
option in the next iteration.

> > +		if (!onto) {
> > +			onto = commit;
> > +			result.clean = 1;
> > +			result.tree = repo_get_commit_tree(repo, commit);
> > +		} else {
> > +			onto = replay_pick_regular_commit(repo, commit, replayed_commits,
> > +							  onto, &merge_opts, &result);
> > +			if (!onto)
> > +				break;
> > +		}
> 
> Hmph, I would have expected that the overall flow of this command
> would be
> 
>  * find the commits above and including the <commit> in question,
>    making sure there is no merge.
> 
>  * read metadata of <commit> like the parent (as we do not allow
>    merges), tree, author ident & time.
> 
>  * create a new commit object that has the same metadata as <commit>
>    on top of the parent of <commit>, but with the updated message
>    and new committer ident & time.
> 
>  * initialize a variable Current to point at the rewritten <commit>
> 
>  * loop for each commit C in <commit>..HEAD range in reverse order
>    (we know we have a single strand of pearls):
> 
>    - read metadata of C
> 
>    - create a new commit object C' that has the same metadata and
>      message as C on top of the Current commit, with new committer
>      ident & time.
> 
>    - make Current point at the resulting C'
> 
>  * Point the Current with HEAD.
> 
> without having to touch any "pick" machinery.  Why do we need to go
> down to the merge machinery for a mere "reword" operation?

You're exactly right, we don't need the "pick" machinery at all right
now. I think this is still a leftover from previous iterations, where I
was also driving things like an "edit" command that _do_ require merges.
But neither splitting a commit nor rewording it does require a merge at
all.

Will simplify. We can reintroduce the heavier machinery at a later point
in time as needed.

Thanks!

Patrick