Re: [PATCH v6 00/11] Introduce git-history(1) command for easy history editing

[Patrick Steinhardt <ps@pks.im>]

On Sat, Nov 22, 2025 at 06:54:00PM -0800, Elijah Newren wrote:
> On Fri, Nov 21, 2025 at 8:01 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> > >> Granted, I think good progress is being made and perhaps the changes
> > >> needed for the rest aren't that huge (and maybe there's more pieces
> > >> I'm not quite understanding yet similar to the
> > >> two-split-patches-always-summing-to-the-original), I was just a little
> > >> surprised that my comments are summarized by "expecting a small and
> > >> final reroll".  :-)
> >
> > It was because I didn't think (and I still do not think) your
> > comments are something for the immediate future, for a tool that
> > wants to present its minimum-serviceable experimental version to
> > users so that the users can experiment, extend it more and fix its
> > behaviour incrementally.  I may have been probably wrong, but I was
> > getting an impression from the reviews that it is getting to there,
> > not the feature-perfect version that needs only maintenance from now
> > on, but a minimum-serviceable one.
> >
> > We could instead of collect all the I want moon and I want cheeze
> > comments and iterate until the tool has all of them before it hits
> > 'next', but I do not think it is often what we do to a new feature.
> 
> You're reading my feedback as feature requests rather than as bugs
> and/or possible paint-ourselves-in-a-corner situations in the
> presented implementation?  I must have described things rather poorly;
> if they were just feature requests, I'd agree we could just implement
> them later.

The command is explicitly marked as experimental so that we can iterate
on its behaviour as needed. So I don't think we're painting us into a
corner yet.

> But maybe I see where the confusion arises, since you were focusing
> solely on the single branch thing; that's the one issue where it's
> perhaps not as clear whether I was discussing a bug or a new feature.
> Let me try to explain that case another way:
> 
> 
> The early cover letters said they focused on a case where just a
> single branch was involved, yet they don't check whether there really
> is only one branch involved for safe operation.

I think this depends on the definition. We _do_ verify that the commit
that is to be edited is part of the current branch. What we _don't_
check is that the commit is _only_ part of that current branch.

I'm not sure whether that is something we want. I myself have the same
commit on multiple branches quite regularly, as I tend to queue up
multiple dependent patch series. But I wouldn't ever want a history edit
to affect all of these branches myself, I really only want it to modify
the branch I'm currently on.

> If a user tries to reword or split a commit that is in the history of
> multiple branches, the current implementation does not check and makes
> the branches diverge.  Some users may want that, though I suspect most
> would be negatively surprised.  The commit messages and code do not
> even discuss this case.  If we merge the code as-is and then later
> notice and fix this problem soon enough, maybe we'd be fine, but I
> always worry a bit about a git-switch/git-restore kind of case where
> it sits long enough and people depend on side-effects in a way that
> prevents us from fixing it.  Besides, since the bug has been
> identified and there are multiple not-too-hard ways to fix, I think we
> should do something.  Some possibilities:
> 
>   * Document this case and warn users to check on their own (not that
> friendly, but might be good enough for the first cut).
>   * Check if the user-specified commit is part of multiple branches
> and error out, unless the user provides a flag verifying that they
> want histories to diverge.
>   * Just rewrite all relevant branches
> 
> The third of those could sound like a feature request in isolation,
> but I raised it primarily because it's a potential fix to this
> overlooked bug.  I mentioned all three possible fixes, but assumed
> others didn't realize how simple that third option was, so I pointed
> out how easy it was with some code (~12 lines, which also replace many
> more existing lines of code).  I personally think the third option is
> *less work* than the second option, and that the focus on trying to
> limit to a single branch is creating more work rather than simplifying
> the problem.  But if folks really do want to limit to a single branch
> despite the code existing to handle the more general case, then we can
> implement one of the other solutions.  (If we do so, I still think
> choice three is more friendly to users, to cpus, and to future
> extension of these features while also simultaneously simplifying the
> existing code; so I'll volunteer to investigate and post patches on
> top of this series if others decide to go with one of the other
> choices for the initial version of the feature.)  My main point here
> is just that there is a clear, un-discussed bug, and it should be
> addressed somehow in the initial version of the feature.

So with the above clarification I wouldn't call any of this a bug, but
rather working as designed. We could of course still print a warning in
that case to protect the user, but one problem I see is that generating
this warning could be quite expensive as we'd now have to walk all
references. That might be cheap in case the user only has short-lived
feature branchs. But it may very well not be cheap in case they for
example have old release branches checked out, as we'd now have to
potentially walk a significant portiion of history.

Consequently I'm leaning more into the direction of doing nothing. It's
not really clear to me that this is a bug, and we still can introduce a
flag in the future that opts into the behaviour of rewriting relevant
branches. That behaviour certainly can be useful, but I'd claim that
it would be rather surprising to the user if that was the default.

Patrick