From: Elijah Newren Date: Sun, 23 Nov 2025 02:30:39 GMT Subject: Re: [PATCH v6 00/11] Introduce git-history(1) command for easy history editing Message-ID: In-Reply-To: <3fb47b15-ed43-4137-95f8-cee97ab5e44c@gmail.com> 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/ . > 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. 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. 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".) > > The other problems are independent of whether you try to limit the > > scope initially in such a manner: > > > > Are the testcases and the code requiring something for the feature > > (ensuring the index and worktree are preserved) doing something that > > is incompatible with the capabilities given to the user (allowing them > > to edit the patch while splitting, so that they stage stuff that > > wasn't part of the original commit)? Or...is it assumed that the > > split commits always "sum" to the changes in the original commit, > > meaning the "other" patch immediately undoes those extra changes? > > Yes that's what's implemented. I think that makes sense for the "split" > command. Often when splitting a commit one needs to make small changes > to the diff in order for the result to compile but you still want the > same end state from the sum of the split commits. Makes sense; thanks for confirming. I just didn't realize this was the case while reviewing the patches until my response above. > > 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. > > 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.