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

[Ben Knoble <ben.knoble@gmail.com>]

> 
> Le 7 mai 2026 à 11:06, Johannes Schindelin <johannes.schindelin@gmx.de> a écrit :
> 
> 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;

Thanks, super helpful.

(Perhaps later) if we can say _which_ descendants weren’t rewritten, that might be good.

>> 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 seem to recall Patrick making a similar argument, but don’t let me put words in anyone’s mouth. 

> 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

Makes sense, thanks.