From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 09 Jan 2026 07:37:25 GMT
Subject: Re: [PATCH v8 1/7] builtin/replay: extract core logic to replay revisions
Message-ID: <aWCwNZrJl1w-Vibw@pks.im>
In-Reply-To: <CALnO6CAMX8K6oNzTmcg_stqkU2FCUepdvNfPTGaA-jSaTMzj0g@mail.gmail.com>

On Wed, Jan 07, 2026 at 12:53:59PM -0500, D. Ben Knoble wrote:
> On Wed, Jan 7, 2026 at 5:10 AM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > diff --git a/builtin/replay.c b/builtin/replay.c
> > index 1960bbbee8..d7523fdbc2 100644
> > --- a/builtin/replay.c
> > +++ b/builtin/replay.c
> 
> > @@ -517,24 +568,13 @@ int cmd_replay(int argc,
> >                 }
> >         }
> >
> > -       merge_finalize(&merge_opt, &result);
> > -       kh_destroy_oid_map(replayed_commits);
> > -       if (update_refs) {
> > -               strset_clear(update_refs);
> > -               free(update_refs);
> > -       }
> > -       ret = result.clean;
> > -
> >  cleanup:
> >         if (transaction)
> >                 ref_transaction_free(transaction);
> > +       replay_ref_updates_release(&updates);
> >         strbuf_release(&transaction_err);
> >         strbuf_release(&reflog_msg);
> >         release_revisions(&revs);
> > -       free(advance_name);
> >
> > -       /* Return */
> > -       if (ret < 0)
> > -               exit(128);
> > -       return ret ? 0 : 1;
> > +       return ret ? 1 : 0;
> >  }
> 
> I tried checking the tree after applying this patch, too, and it looks
> to me like the return code flipped here? In particular, some callsites
> that assign ret = error(…) are untouched, so I don't think the meaning
> of ret has changed. Now, error() returns -1, which is truthy, so
> returning 1 instead of 0 makes sense here… was this a bug in the
> original? I can't quite tell, but that seems unlikely.
> 
> The original blames to 81613be31e (replay: make it a minimal server
> side command, 2023-11-24), but there it seems like ret is
> "result.clean" (except for some error cases? which are handled by the
> negative conditional), and "result.clean == 0" is the success
> indicator (in other words, _falsey_ means success here).
> 
> So overall this flip _seems_ correct, but it was hard for me to follow
> at a glance. Hm.

I think you're onto something here. The intent seems to be that:

  - We exit with 128 in case there was any generic error.

  - We exit with 1 in case there was a merge conflict.

  - We exit with 0 in case the command was successful.

But the extracted `replay_revisions()` command always returns negative
on error now. I was initially returning that value directly, which has
caused a test failure. I fixed that with the above condition, but I
didn't realize that we explicitly wanted to tell apart those two error
cases.

I'll fix this code and refactor it a bit to make it more explicit,
thanks!

Patrick