Re: [PATCH/RFC 0/5] replay: support replaying 2-parent merges

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

Hi Ben,

On Thu, 7 May 2026, D. Ben Knoble wrote:

> On Wed, May 6, 2026 at 6:44 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > [...]
> >
> > While I was at it, git history reword had a pre-existing
> > silent-success bug: a positive return from replay_revisions() (which
> > means "conflict, no updates queued") was treated as success. Obviously
> > this should never occur, as a reword simply does not change any of the
> > file contents, but bugs do happen. The merge-replay work is complex
> > enough to make that class of bugs more likely, therefore I introduce
> > error messages for those instances.
> 
> Fixing this bug sounded interesting; I had a hard time spotting it
> while skimming the first 2 patches.

It's this part:

@@ -482,6 +482,9 @@ static int cmd_history_reword(int argc,
 	if (ret < 0) {
 		ret = error(_("failed replaying descendants"));
 		goto out;
+	} else if (ret) {
+		ret = error(_("conflict during replay; some descendants were not rewritten"));
+		goto out;
 	}
 
 	ret = 0;
@@ -721,6 +724,9 @@ static int cmd_history_split(int argc,
 	if (ret < 0) {
 		ret = error(_("failed replaying descendants"));
 		goto out;
+	} else if (ret) {
+		ret = error(_("conflict during replay; some descendants were not rewritten"));
+		goto out;
 	}
 
 	ret = 0;

> Did I just miss it? Is it worth splitting that fix out to a separate patch?

Well, you _could_ argue that they were not bugs at all: a `git history
reword` isn't supposed to be able to result in merge conflicts, nor is
`git history split` because they leave the respective commits tree-same
(in the `split` case, the second commit).

I could see the point were anybody to suggest using `BUG()` instead of
`error()` here, but erred on the "nicer to the user" side.

The only way this _might_ be triggered before this patch series is most
likely by playing games with replace objects. Or maybe you cannot trigger
it at all.

With the changes in this here patch series, I wasn't so certain that I had
covered all the edge cases (an early iteration of the quick short-cut in
patch 2/5 keyed only on the parent commits' trees, and forgot to verify
the merge _bases_' trees, for example). That's why I think it matters more
now than it did before.

Ciao,
Johannes