Re: [PATCH v8 1/7] builtin/replay: extract core logic to replay revisions

> 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.