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

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Jan 7, 2026, 17:53 UTC
Message-ID: <CALnO6CAMX8K6oNzTmcg_stqkU2FCUepdvNfPTGaA-jSaTMzj0g@mail.gmail.com>
In-Reply-To: <20260107-b4-pks-history-builtin-v8-1-18e9779e3a26@pks.im>

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

Show 27 quoted lines

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

-- 
D. Ben Knoble