Re: [PATCH v9 2/7] builtin/replay: move core logic into "libgit.a"

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jan 09, 2026 at 05:16:41PM -0800, Elijah Newren wrote:
> On Fri, Jan 9, 2026 at 12:35 AM Patrick Steinhardt <ps@pks.im> wrote:
> > diff --git a/replay.c b/replay.c
> > new file mode 100644
> > index 0000000000..fc7186ef09
> > --- /dev/null
> > +++ b/replay.c
> > @@ -0,0 +1,355 @@
> > +#define USE_THE_REPOSITORY_VARIABLE
> > +#define DISABLE_SIGN_COMPARE_WARNINGS
> > +
> > +#include "git-compat-util.h"
> > +#include "environment.h"
> > +#include "hex.h"
> > +#include "merge-ort.h"
> > +#include "object-name.h"
> > +#include "oidset.h"
> 
> I don't think oidset is used here?   Although, come to think of it,
> I'm not sure it was used in replay to begin with.  Looks like I added
> this include during the switch from t/helper/test-fast-rebase, but
> never used it even then.  I must have thought I was going to use it,
> and added it, but never did.  Anyway, maybe this is a good time to get
> rid of it?

Yeah, let's.

> > +#include "parse-options.h"
> 
> Why would parse-options be included here?  Oh,
> die_for_incompatible_opt2()?  Feels a little weird that we have option
> parsing logic outside of builtin/, but...maybe it's all fine?

I'm not a huge fan of it, either. We could for example change this so
that we `BUG()` here and instead have the option in "builtin/replay.c"?
Something like this:

diff --git a/builtin/replay.c b/builtin/replay.c
index 4a11ef0f1b..649c93200e 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -177,8 +177,9 @@ static void set_up_replay_mode(struct repository *repo,
 	if (!rinfo.positive_refexprs)
 		die(_("need some commits to replay"));
 
-	die_for_incompatible_opt2(!!onto_name, "--onto",
-				  !!*advance_name, "--advance");
+	if (!(!!onto_name ^ !!*advance_name))
+		BUG("expected either onto_name or *advance_name in this function");
+
 	if (onto_name) {
 		*onto = peel_committish(repo, onto_name, "--onto");
 		if (rinfo.positive_refexprs <
@@ -191,9 +192,6 @@ static void set_up_replay_mode(struct repository *repo,
 		struct object_id oid;
 		char *fullname = NULL;
 
-		if (!*advance_name)
-			BUG("expected either onto_name or *advance_name in this function");
-
 		if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
 			     &oid, &fullname, 0) == 1) {
 			free(*advance_name);
@@ -478,6 +476,8 @@ int cmd_replay(int argc,
 
 	die_for_incompatible_opt2(!!opts.advance, "--advance",
 				  opts.contained, "--contained");
+	die_for_incompatible_opt2(!!opts.advance, "--advance",
+				  !!opts.onto, "--onto");
 
 	/* Parse ref action mode from command line or config */
 	ref_mode = get_ref_action_mode(repo, ref_action);

Yeha, I think that's cleaner, even if it repeats some of the
conditionals.

> > +#include "refs.h"
> > +#include "replay.h"
> > +#include "revision.h"
> 
> Shouldn't strmap.h (for strset) also be included?  I think we get it
> as a side-effect of something else, but since we use it directly, it'd
> make sense to include directly.

Yup, will add.

> > diff --git a/replay.h b/replay.h
> > new file mode 100644
> > index 0000000000..84bc8a7a5b
> > --- /dev/null
> > +++ b/replay.h
> > @@ -0,0 +1,64 @@
> > +#ifndef REPLAY_H
> > +#define REPLAY_H
> > +
> > +#include "hash.h"
> > +
> > +struct repository;
> > +struct rev_info;
> > +
> > +/*
> > + * A set of options that can be passed to `replay_revisions()`.
> > + */
> > +struct replay_revisions_options {
> > +       /*
> > +        * Starting point at which to create the new commits; must be a branch
> > +        * name. The branch will be updated to point to the rewritten commits.
> > +        * This option is mutually exclusive with `onto`.
> > +        */
> > +       const char *advance;
> > +
> > +       /*
> > +        * Starting point at which to create the new commits; must be a
> > +        * committish. References pointing at decendants of `onto` will be
> > +        * updated to point to the new commits.
> > +        */
> > +        const char *onto;
> > +
> > +       /*
> > +        * Update branches that point at commits in the given revision range.
> > +        * Requires `onto` to be set.
> > +        */
> > +       int contained;
> > +};
> > +
> > +/* This struct is used as an out-parameter by `replay_revisions()`. */
> > +struct replay_result {
> > +       /*
> > +        * The set of reference updates that are caused by replaying the
> > +        * commits.
> > +        */
> > +       struct replay_ref_update {
> > +               char *refname;
> > +               struct object_id old_oid;
> > +               struct object_id new_oid;
> > +       } *updates;
> > +       size_t updates_nr, updates_alloc;
> > +
> > +       /* Set to true in case the replay failed with a merge conflict. */
> > +       bool merge_conflict;
> > +};
> > +
> > +void replay_result_release(struct replay_result *result);
> > +
> > +/*
> > + * Replay a set of commits onto a new location. Leaves both the working tree,
> > + * index and references untouched. Reference updates caused by the replay will
> > + * be recorded in the `updates` out pointer.
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
> > + */
> > +int replay_revisions(struct repository *repo, struct rev_info *revs,
> > +                    struct replay_revisions_options *opts,
> > +                    struct replay_result *out);
> > +
> 
> stray extra line?

We typically have an empty line between the last declaration and the
`#endif` in our headers.

> > +#endif
> 
> It feels duplicative to have replay_result include a merge_conflict
> field and to have replay_revisions() return an int which signifies
> whether there's a conflict.  Can we remove one of the two?  (Perhaps
> the merge_conflict field was only a workaround to the weird ret
> setting from the previous patch?)

The idea here is that we have a generic error code that tells the caller
that _something_ happened, whereas `struct replay_result` gives the
caller a bit more context around what exactly has happened. This allows
callers to handle merge conflicts differently from any other error and
makes the different failure modes a lot more explicit.

Some context: at GitLab we actually have the use case to surface more
information around what commits have conflicted, so there will be a
follow-up patch series that extends `struct replay_result` to return
more information about the actual conflict. I'm already planning ahead a
bit in this patch series.

Patrick