From: Phillip Wood Date: Mon, 24 Nov 2025 16:31:44 GMT Subject: Re: [PATCH v6 00/11] Introduce git-history(1) command for easy history editing Message-ID: In-Reply-To: On 23/11/2025 02:30, Elijah Newren wrote: > On Fri, Nov 21, 2025 at 6:31 AM Phillip Wood wrote: >> > [...] >>> So, you are referring to the single branch, HEAD-centric piece of the >>> feedback. The funny thing there is that operating on a more limited >>> case, without checking and verifying that you are indeed in the more >>> limited case (and erroring out if not), risks painting us into a >>> corner or providing some really buggy behavior when we aren't actually >>> in that case. To me, it opens a can of worms and makes the problem >>> scope bigger instead of smaller. Funnily enough, the single branch >>> thing is also the one piece of this that I think could be solved by a >>> fairly small change in the reroll (and I pointed out how in the >>> comments), so the limited view really didn't buy anything here IMO. >> >> I can't find that comment. Are you referring to reusing more of the >> replay machinery? > > Yeah, what's needed is the equivalent of running "git replay --onto > ${NEW_COMMIT_ID} --ancestry-path ^${OLD_COMMIT_ID} --branches", as > noted in more detail over at > https://lore.kernel.org/git/CABPp-BEm1QBP+CuSOn5FaE3XJVFg+Qbfzdp560u00ZERbNm6qQ@mail.gmail.com/ Thanks, I'd somehow missed that when I read that message the first time >> If so we have the problem that the user gives a single >> commit to "git history" so we don't have a handy revision range to pass >> to the replay machinery unless we assume we're rewriting an ancestor of >> HEAD or we go and find all the branches descended from the commit the >> user gave us. > > The range is included in the command above: "--ancestry-path > ^${OLD_COMMIT_ID} --branches" > > And because of this, we don't even really need to "find" all the > branches as a separate step, it's just part of the same revision walk > for rewriting commits. Oh, so --branches means we consider all the branches and --ancestry-path excludes the those that are not descended from the commit we're rewriting - nice. We'd need to be careful about modifying the commit at the tip of a branch though as in that case we'd exclude the branch from the set of commits with ^{OLD_COMMIT_ID} and so "git replay" would not update that branch. In general rewriting multiple branches can be confusing if those branches are checked out elsewhere and the HEAD of that worktree suddenly changes but for rewording and splitting commits as implemented here the final tree is the same after the rewrite so it should be fine. The other potential problem with rewriting multiple branches is that we need to ensure two separate "git history" processes running at the same time in two different worktrees don't try to update the same branch. "git rebase --update-refs" has some logic to prevent that. > Whereas if we do want to only handle a single branch as the current > implementation does, then we *need* to do an extra revision walk to > ensure that the commit is not also part of any other branch and error > out if it is, because disconnecting the histories would be very > counterintuitive in most cases. Oh - I had not understood what the "extra work" you were talking about before was - that makes it clear. > If users really do want to disconnect > histories of two branches sharing a commit, we should require the user > to provide some flag to explicitly specify such to signal that it is > okay for us to bypass such a check and just rewrite one branch. Such > a check is missing from the current code. > >> Long term we should certainly do the latter but depending >> on how much work it is to implement that we may want to go with the >> single branch case at first > > I showed the implementation of the latter, and it's actually (much) > less code than what's already in this series; see the > replay_descendants() function I posted at the same link above. > > My replay-edit work used a just slightly modified form of that > function, because editing a commit and replaying all commits from all > branches that reached the OLD_COMMIT_ID, to now be replayed on top of > NEW_COMMIT_ID, is exactly what was needed there too. (If you're > curious about the modifications: I had an extra --brief-stats option > because I found it nice to provide some user feedback about what was > updated, and I pulled the "--branches" portion of the command from a > ${GIT_DIR}/REPLAY_EDIT file, because that allowed me to give users the > opportunity to disconnect histories via some mechanism that would put > a single branch name in that file instead of "--branches".) Interesting - I watched you're git merge talk about it recently and it looked quite impressive. >>> I'm also worried about extended header handling for the edited >>> (reworded or split) commits. That seems to have been overlooked in >>> this series, despite the fact that in early versions extended headers >>> were explicitly called out for the remainder of the commits being >>> replayed/rebased, so it seems interesting that they weren't considered >>> for the commits explicitly being edited. >> >> What headers does it make sense to copy when splitting a commit? When >> rewording it is more likely that copying the extended headers is what >> the user wants but the example of the "encoding" header you gave does >> not make sense to me as we re-encode the commit message and author data >> when the user edit's the message so we're not preserving the original >> encoding. > > I agree that when rewording we probably want to copy most extended > headers, but you make a good point about encoding. For splitting, I > agree it's less clear, and I'm not sure I know the answer. But I > expected the topic to at least be discussed and mentioned in the > relevant commit messages. It appears to have been silently > overlooked, and I'm worried it's the kind of topic that doesn't come > up often, meaning that if we don't discuss now and just pick whatever > behavior we get from implementation side-effects, then people will > come back in a year or two and point out we got it buggy but it's too > late to change it. It would certainly be worth adding a comment about commit headers in the commit message. Thanks Phillip >>> And I'm a bit surprised that the original commit message for a split >>> commit is automatically associated with the second commit; if I had >>> been forced to choose, I would have assumed it should be associated >>> with the first. >> >> I don't think it is safe to assume either - we should prompt the user to >> edit the message when creating both commits and seed the editor with the >> original message. > > That sounds like a better solution to me for that particular issue, > and probably wouldn't be hard to implement.