[PATCH 0/2] replay: die descriptively when invalid commit-ish

[PATCH 0/2] replay: die descriptively when invalid commit-ish

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

You get this error when you for example mistype the argument to
`--onto`:

    fatal: Replaying down to root commit is not supported yet!

Consider that you might not know yourself that you have mistyped
something; then this looks even more puzzling. You might have given a
range like `main..topic` but the command says that it would need to
replay down to the root commit.

The only thing that’s happened though is that `NULL` has been
interpreted in the wrong way.

Let’s instead die immediately when the real error happens, in other
words when we can’t find the commit for the given commit-ish.

Also add more regression tests.

Somewhat unrelated to this change—and caveat not a C programmer—I was
confused by `determine_replay_mode`.  Most of the function deals with
three cases:

    if (onto_name) {
    ...
    } else if (*advance_name) {
    ...
    } else {
    ...
    }

But I don’t get the `else` since you now need to provide either `--onto`
or `--advance`.  And both require an argument.  So when can the
fallthrough `else` happen?

I thought that maybe the `else` was old code that predated `--onto` and
`--advance` being mandatory. But that whole method and this check was
added in the same commit, in 22d99f01 (replay: add --advance or
'cherry-pick' mode, 2023-11-24):

    if (!onto_name && !advance_name) {
            error(_("option --onto or --advance is mandatory"));

But just ignore this section if I’m simply confused.

Kristoffer Haugsbakk (2):
  replay: die descriptively when invalid commit-ish
  t3650: add more regression tests for failure conditions

 builtin/replay.c         |  2 +-
 t/t3650-replay-basics.sh | 43 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)


base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
-- 
2.52.0.10.g08704017180

[PATCH 1/2] replay: die descriptively when invalid commit-ish

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Giving an invalid commit-ish to `--onto` or `--advance` makes
git-replay(1) fail with:

    fatal: Replaying down to root commit is not supported yet!

Going backwards from this point:

1. `onto` is `NULL` from `determine_replay_mode`;
2. that function in turn calls `peel_committish`; and
3. here we return `NULL` if `repo_get_oid` fails.

Let’s die immediately with a descriptive error message instead.

Doing this also provides us with a descriptive error if we “forget” to
provide an argument to `--onto` (but we really do unintentionally):[1]

    $ git replay --onto ^main topic1
    fatal: '^main' is not a valid commit-ish

† 1: The argument to `--onto` is mandatory and the option parser accepts
     both `--onto=<name>` (stuck form) and `--onto name`. The latter
     form makes it easy to unintentionally pass something to the option
     when you really meant to pass a positional argument.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/replay.c         |  2 +-
 t/t3650-replay-basics.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 6172c8aacc9..175b64c5335 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -33,7 +33,7 @@ static struct commit *peel_committish(struct repository *repo, const char *name)
 	struct object_id oid;
 
 	if (repo_get_oid(repo, name, &oid))
-		return NULL;
+		die(_("'%s' is not a valid commit-ish"), name);
 	obj = parse_object(repo, &oid);
 	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
 						  OBJ_COMMIT);
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 58b37599357..bfe8e01da49 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -51,6 +51,22 @@ test_expect_success 'setup bare' '
 	git clone --bare . bare
 '
 
+test_expect_success '--onto with invalid commit-ish' '
+	cat >expect <<-EOF &&
+	fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
+	EOF
+	test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--advance with invalid commit-ish' '
+	cat >expect <<-EOF &&
+	fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
+	EOF
+	test_must_fail git replay --advance=refs/not-valid topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'using replay to rebase two branches, one on top of other' '
 	git replay --onto main topic1..topic2 >result &&
 
-- 
2.52.0.10.g08704017180

[PATCH 2/2] t3650: add more regression tests for failure conditions

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

There isn’t much test coverage for basic failure conditions. Let’s add
a few more since these are simple to write and remove if they become
obsolete.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t3650-replay-basics.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index bfe8e01da49..c543d55857b 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -67,6 +67,33 @@ test_expect_success '--advance with invalid commit-ish' '
 	test_cmp expect actual
 '
 
+test_expect_success 'option --onto or --advance is mandatory' '
+	cat >expect <<-\EOF &&
+	error: option --onto or --advance is mandatory
+	EOF
+	# First line is the error; rest is Usage
+	test_must_fail git replay topic1..topic2 >&1 2>&1 |
+		head -1 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'no base or negative ref gives no-replaying down to root error' '
+	cat >expect <<-\EOF &&
+	fatal: replaying down to root commit is not supported yet!
+	EOF
+	test_must_fail git replay --onto=topic1 topic2 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'options --advance and --contained cannot be used together' '
+	cat >expect <<-EOF &&
+	fatal: options ${SQ}--advance${SQ} and ${SQ}--contained${SQ} cannot be used together
+	EOF
+	test_must_fail git replay --advance=main --contained \
+		topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'using replay to rebase two branches, one on top of other' '
 	git replay --onto main topic1..topic2 >result &&
 
-- 
2.52.0.10.g08704017180

Re: [PATCH 1/2] replay: die descriptively when invalid commit-ish

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> diff --git a/builtin/replay.c b/builtin/replay.c
> index 6172c8aacc9..175b64c5335 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -33,7 +33,7 @@ static struct commit *peel_committish(struct repository *repo, const char *name)
>  	struct object_id oid;
>  
>  	if (repo_get_oid(repo, name, &oid))
> -		return NULL;
> +		die(_("'%s' is not a valid commit-ish"), name);

This is after repo_get_oid() fails to turn the "name" into an oid.
The only thing we know about "name" is that it does not name an
object, but we want to get a commit-ish and the new message sounds
like a reasonable way to tell both of these two facts.

>  	obj = parse_object(repo, &oid);
>  	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
>  						  OBJ_COMMIT);

The previous parse_object() can return NULL, in which case
repo_peel_to_type() would also silently return NULL.

If obj is not NULL, repo_peel_to_type() would die with a descriptive
message when the thing does not peel to an object of the expected
type.

So the caller of this function still needs to be prepared for
receiving a NULL from here.

How many callers use this function?  I am wondering if it is better
to give a better message at the caller(s), rather than here, where
we lack context to tell something like "You gave string 'ource' as
the argument to the '--onto' option, but 'ource' does not name any
commit" (in other words, "for what our caller is trying to peel
<name> to a commit").

Re: [PATCH 0/2] replay: die descriptively when invalid commit-ish

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> Subject: Re: [PATCH 0/2] replay: die descriptively when invalid commit-ish

"... is given" or something?

Re: [PATCH 1/2] replay: die descriptively when invalid commit-ish

[Phillip Wood]

On 23/12/2025 03:12, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
> 
> How many callers use this function?  I am wondering if it is better
> to give a better message at the caller(s), rather than here, where
> we lack context to tell something like "You gave string 'ource' as
> the argument to the '--onto' option, but 'ource' does not name any
> commit" (in other words, "for what our caller is trying to peel
> <name> to a commit").

There are only two callers so I think that is a good idea. If you give 
an invalid commit name to "--advance" then it dies with

     fatal: argument to --advance must be a reference

so arguably we only need to check the return value when parsing "--onto"

Thanks

Phillip

Re: [PATCH 2/2] t3650: add more regression tests for failure conditions

[Phillip Wood]

On 22/12/2025 22:04, kristofferhaugsbakk@fastmail.com wrote:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
> There isn’t much test coverage for basic failure conditions. Let’s add
> a few more since these are simple to write and remove if they become
> obsolete.

Sounds like a good idea

>   
> +test_expect_success 'option --onto or --advance is mandatory' '
> +	cat >expect <<-\EOF &&
> +	error: option --onto or --advance is mandatory
> +	EOF
> +	# First line is the error; rest is Usage
> +	test_must_fail git replay topic1..topic2 >&1 2>&1 |
> +		head -1 >actual &&

Using a pipe means we lose the return value of test_must_fail here so 
the test wont fail if the command succeeds. Everything else looks good

Thanks

Phillip

> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'no base or negative ref gives no-replaying down to root error' '
> +	cat >expect <<-\EOF &&
> +	fatal: replaying down to root commit is not supported yet!
> +	EOF
> +	test_must_fail git replay --onto=topic1 topic2 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'options --advance and --contained cannot be used together' '
> +	cat >expect <<-EOF &&
> +	fatal: options ${SQ}--advance${SQ} and ${SQ}--contained${SQ} cannot be used together
> +	EOF
> +	test_must_fail git replay --advance=main --contained \
> +		topic1..topic2 2>actual &&
> +	test_cmp expect actual
> +'
> +
>   test_expect_success 'using replay to rebase two branches, one on top of other' '
>   	git replay --onto main topic1..topic2 >result &&
>   

Re: [PATCH 1/2] replay: die descriptively when invalid commit-ish

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> There are only two callers so I think that is a good idea. If you give 
> an invalid commit name to "--advance" then it dies with
>
>      fatal: argument to --advance must be a reference
>
> so arguably we only need to check the return value when parsing "--onto"

So, in determine_replay_mode(), ...

	if (onto_name) {
		*onto = peel_committish(repo, onto_name);

... here is where we must see *onto is NULL and barf and then ...

		if (rinfo.positive_refexprs <
		    strset_get_size(&rinfo.positive_refs))
			die(_("all positive revisions given must be references"));
	} else if (*advance_name) {
		struct object_id oid;
		char *fullname = NULL;

		*onto = peel_committish(repo, *advance_name);

... for symmetry, we would want to do the same.  

In addition, we probably would want to let the following code that
uses the same *advance_name (and requires that it just not names a
commit-ish object, but is actually a ref, which is a different
requirement that is probably a bit tighter) ...

		if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
			     &oid, &fullname, 0) == 1) {
			free(*advance_name);
			*advance_name = fullname;
		} else {
			die(_("argument to --advance must be a reference"));
		}

... first, and then compute *onto after that by moving code a bit,
perhaps?

Thanks.

Re: [PATCH 0/2] replay: die descriptively when invalid commit-ish

[Elijah Newren]

On Mon, Dec 22, 2025 at 2:04 PM <kristofferhaugsbakk@fastmail.com> wrote:
>
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> You get this error when you for example mistype the argument to
> `--onto`:
>
>     fatal: Replaying down to root commit is not supported yet!
>
> Consider that you might not know yourself that you have mistyped
> something; then this looks even more puzzling. You might have given a
> range like `main..topic` but the command says that it would need to
> replay down to the root commit.
>
> The only thing that’s happened though is that `NULL` has been
> interpreted in the wrong way.
>
> Let’s instead die immediately when the real error happens, in other
> words when we can’t find the commit for the given commit-ish.
>
> Also add more regression tests.

Thanks for working on this.  I have nothing to add beyond what others
have already commented on the series, except that I can maybe answer
one question...

> Somewhat unrelated to this change—and caveat not a C programmer—I was
> confused by `determine_replay_mode`.  Most of the function deals with
> three cases:
>
>     if (onto_name) {
>     ...
>     } else if (*advance_name) {
>     ...
>     } else {
>     ...
>     }
>
> But I don’t get the `else` since you now need to provide either `--onto`
> or `--advance`.  And both require an argument.  So when can the
> fallthrough `else` happen?
>
> I thought that maybe the `else` was old code that predated `--onto` and
> `--advance` being mandatory. But that whole method and this check was
> added in the same commit, in 22d99f01 (replay: add --advance or
> 'cherry-pick' mode, 2023-11-24):
>
>     if (!onto_name && !advance_name) {
>             error(_("option --onto or --advance is mandatory"));
>
> But just ignore this section if I’m simply confused.

Your assumption was right -- that the final `else` was old code that
predated `--onto` and `--advance` being mandatory.  git-replay was
originally developed in my private branch, but I let other folks know
I was experimenting with it.  When life pulled me away from git,
Christian and Johannes expressed interest in the command, and
Christian cleaned up my rough patches, stripping out the clearly
half-baked stuff, and also removing some additional things that didn't
need to be in the initial version, and sent it upstream.  None of us
noticed this `else` had turned into dead code in the review.

The original had the idea that it would be able to sometimes guess the
mode and allow for shorter command lines, but I was still playing
around with it and haven't particularly felt the need to push that
idea.

Even if we eventually decide we want that other handling, removing
this `else` clause for now and allowing it to be reintroduced later
would probably make for a more coherent patch anyway.

Re: [PATCH 1/2] replay: die descriptively when invalid commit-ish

[Kristoffer Haugsbakk]

On Tue, Dec 23, 2025, at 11:52, Phillip Wood wrote:
> On 23/12/2025 03:12, Junio C Hamano wrote:
>> kristofferhaugsbakk@fastmail.com writes:
>>
>> How many callers use this function?  I am wondering if it is better
>> to give a better message at the caller(s), rather than here, where
>> we lack context to tell something like "You gave string 'ource' as
>> the argument to the '--onto' option, but 'ource' does not name any
>> commit" (in other words, "for what our caller is trying to peel
>> <name> to a commit").
>
> There are only two callers so I think that is a good idea. If you give
> an invalid commit name to "--advance" then it dies with
>
>      fatal: argument to --advance must be a reference
>
> so arguably we only need to check the return value when parsing "--onto"

Well spotted. My change would give a worse error message
for `--advance`.

I’ve made the move-line change that Junio suggested in version 2.

Thanks.

Re: [PATCH 0/2] replay: die descriptively when invalid commit-ish

[Kristoffer Haugsbakk]

On Wed, Dec 24, 2025, at 04:03, Elijah Newren wrote:
> On Mon, Dec 22, 2025 at 2:04 PM <kristofferhaugsbakk@fastmail.com> wrote:
>>[snip]
>
> Thanks for working on this.  I have nothing to add beyond what others
> have already commented on the series, except that I can maybe answer
> one question...
>
>[snip]

Thanks for explaining! I’ve removed this code in version 2.

Re: [PATCH 0/2] replay: die descriptively when invalid commit-ish

[Kristoffer Haugsbakk]

On Tue, Dec 23, 2025, at 04:16, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> Subject: Re: [PATCH 0/2] replay: die descriptively when invalid commit-ish
>
> "... is given" or something?

Sure thing.

I’m a bit paranoid of the soft 50 character max that git-commit(1)
recommends (and that might practically matter for email-based projects).

Re: [PATCH 2/2] t3650: add more regression tests for failure conditions

[Kristoffer Haugsbakk]

On Tue, Dec 23, 2025, at 11:58, Phillip Wood wrote:
>>[snip]
>> +test_expect_success 'option --onto or --advance is mandatory' '
>> +	cat >expect <<-\EOF &&
>> +	error: option --onto or --advance is mandatory
>> +	EOF
>> +	# First line is the error; rest is Usage
>> +	test_must_fail git replay topic1..topic2 >&1 2>&1 |
>> +		head -1 >actual &&
>
> Using a pipe means we lose the return value of test_must_fail here so
> the test wont fail if the command succeeds. Everything else looks good

Thanks. That’s kind of subtle for me. I’ll fix.

I’ll just test for the full output instead of the first line.

[PATCH v2 0/5] replay: die descriptively when invalid commit-ish

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

You get this error when you for example mistype the argument to
`--onto`:

    fatal: Replaying down to root commit is not supported yet!

Consider that you might not know yourself that you have mistyped
something; then this looks even more puzzling. You might have given a
range like `main..topic` but the command says that it would need to
replay down to the root commit.

The only thing that’s happened though is that `NULL` has been
interpreted in the wrong way.

Let’s instead die immediately when the real error happens, in other
words when we can’t find the commit for the given commit-ish.

Also:

• Add more regression tests
• Remove dead code
• Slightly more robust object parsing (parse_object_or_die)

§ Dropped section (see previous)

Somewhat unrelated to this change ...

§ Changes in v2

In this version we remove some dead code (see previous section), improve
tests and add a couple more, and improve the main “die descriptively”
part, with two additional patches/commits to that end:

• replay: find *onto only after testing for ref name
• replay: die if we cannot parse object

See the notes on each patch/commit for change details.

§ Link to v1

https://lore.kernel.org/git/CV_replay_die_descr.13f@msgid.xyz/

❦ ❦ ❦ ❦ ❦

(this means you can stop reading here)

§ How I Present Patch Series

Version: 2

The cover letter:

• Problem statement and solution
• (optional) Summary of what each commit/patch does
• Changes compared to the previous version
  • Note that all versions are not included
• (optional) “CC”/“Cc” which explains all or parts of the CC list
  • But some things are not noteworthy, like the standard practice of
    including previous-round respondents
• Link to the previous version
  • This is new.  I thought it was redundant but some people (like me!)
    use webapp email clients which are not that great for navigating
    among tree-like email threads.  So I was inspired by this practice
    which many others already use.

Then each commit/patch might have Git notes with a `series` namespace.
These contain:

• Comments/questions to reviewers about my approach or statements made
  in the commit message.
• May quote the commit message or emails (with links)
• Lines starting with `v<version>:` which introduce a changelog for that
  version.  Might have `v<num>: [new]` if this version is 2+ and the
  patch/commit is new. Now, lately (Sep 2025) these have been written in
  the “imperative mood”, like what is done for a commit message in this
  (Git) project.  A bit strange, given that most others seem to use the
  more immediately natural past-tense.  But on the other hand: how many
  new contributors to this project use the wrong tense/mood in their
  commit messages?  The Git project rule is not “natural”.  But I think
  it’s better nonetheless and worth the effort.
• ... But this kind of changelog might also be conducive to a bullet
  list of changes.  So I might either skip the previous point, do only a
  bullet point, or do both: a presentation and then the bullet points
  summarizing the presentation.
• Note that all `v<version>:` are kept between versions, which is not
  consistent with how I only have the “Changes” part for the previous
  version in the cover letter.
• `v<version>:` are ordered newest-to-oldest.

Kristoffer Haugsbakk (5):
  replay: remove dead code and rearrange
  replay: find *onto only after testing for ref name
  replay: die descriptively when invalid commit-ish is given
  replay: die if we cannot parse object
  t3650: add more regression tests for failure conditions

 builtin/replay.c         | 87 +++++++++++-----------------------------
 t/t3650-replay-basics.sh | 54 +++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 63 deletions(-)

Interdiff against v1:
diff --git a/builtin/replay.c b/builtin/replay.c
index 175b64c5335..ca5a14de4c7 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -25,18 +25,20 @@ static const char *short_commit_name(struct repository *repo,
 {
 	return repo_find_unique_abbrev(repo, &commit->object.oid,
 				       DEFAULT_ABBREV);
 }
 
-static struct commit *peel_committish(struct repository *repo, const char *name)
+static struct commit *peel_committish(struct repository *repo,
+				      const char *name,
+				      const char *mode)
 {
 	struct object *obj;
 	struct object_id oid;
 
 	if (repo_get_oid(repo, name, &oid))
-		die(_("'%s' is not a valid commit-ish"), name);
-	obj = parse_object(repo, &oid);
+		die(_("'%s' is not a valid commit-ish for %s"), name, mode);
+	obj = parse_object_or_die(repo, &oid, name);
 	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
 						  OBJ_COMMIT);
 }
 
 static char *get_author(const char *message)
@@ -154,89 +156,50 @@ static void get_ref_information(struct repository *repo,
 
 		free(fullname);
 	}
 }
 
-static void determine_replay_mode(struct repository *repo,
-				  struct rev_cmdline_info *cmd_info,
-				  const char *onto_name,
-				  char **advance_name,
-				  struct commit **onto,
-				  struct strset **update_refs)
+static void populate_for_onto_or_advance_mode(struct repository *repo,
+					      struct rev_cmdline_info *cmd_info,
+					      const char *onto_name,
+					      char **advance_name,
+					      struct commit **onto,
+					      struct strset **update_refs)
 {
 	struct ref_info rinfo;
 
 	get_ref_information(repo, cmd_info, &rinfo);
 	if (!rinfo.positive_refexprs)
 		die(_("need some commits to replay"));
 
 	die_for_incompatible_opt2(!!onto_name, "--onto",
 				  !!*advance_name, "--advance");
 	if (onto_name) {
-		*onto = peel_committish(repo, onto_name);
+		*onto = peel_committish(repo, onto_name, "--onto");
 		if (rinfo.positive_refexprs <
 		    strset_get_size(&rinfo.positive_refs))
 			die(_("all positive revisions given must be references"));
-	} else if (*advance_name) {
+		*update_refs = xcalloc(1, sizeof(**update_refs));
+		**update_refs = rinfo.positive_refs;
+		memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
+	} else {
 		struct object_id oid;
 		char *fullname = NULL;
 
-		*onto = peel_committish(repo, *advance_name);
+		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);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
 		}
+		*onto = peel_committish(repo, *advance_name, "--advance");
 		if (rinfo.positive_refexprs > 1)
 			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
-	} else {
-		int positive_refs_complete = (
-			rinfo.positive_refexprs ==
-			strset_get_size(&rinfo.positive_refs));
-		int negative_refs_complete = (
-			rinfo.negative_refexprs ==
-			strset_get_size(&rinfo.negative_refs));
-		/*
-		 * We need either positive_refs_complete or
-		 * negative_refs_complete, but not both.
-		 */
-		if (rinfo.negative_refexprs > 0 &&
-		    positive_refs_complete == negative_refs_complete)
-			die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
-		if (negative_refs_complete) {
-			struct hashmap_iter iter;
-			struct strmap_entry *entry;
-			const char *last_key = NULL;
-
-			if (rinfo.negative_refexprs == 0)
-				die(_("all positive revisions given must be references"));
-			else if (rinfo.negative_refexprs > 1)
-				die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
-			else if (rinfo.positive_refexprs > 1)
-				die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
-
-			/* Only one entry, but we have to loop to get it */
-			strset_for_each_entry(&rinfo.negative_refs,
-					      &iter, entry) {
-				last_key = entry->key;
-			}
-
-			free(*advance_name);
-			*advance_name = xstrdup_or_null(last_key);
-		} else { /* positive_refs_complete */
-			if (rinfo.negative_refexprs > 1)
-				die(_("cannot implicitly determine correct base for --onto"));
-			if (rinfo.negative_refexprs == 1)
-				*onto = rinfo.onto;
-		}
-	}
-	if (!*advance_name) {
-		*update_refs = xcalloc(1, sizeof(**update_refs));
-		**update_refs = rinfo.positive_refs;
-		memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
 	}
 	strset_clear(&rinfo.negative_refs);
 	strset_clear(&rinfo.positive_refs);
 }
 
@@ -384,15 +347,13 @@ int cmd_replay(int argc,
 			  "'%s' bit in 'struct rev_info' will be forced"),
 			"simplify_history");
 		revs.simplify_history = 0;
 	}
 
-	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
-			      &onto, &update_refs);
-
-	if (!onto) /* FIXME: Should handle replaying down to root commit */
-		die("Replaying down to root commit is not supported yet!");
+	populate_for_onto_or_advance_mode(repo, &revs.cmdline,
+					  onto_name, &advance_name,
+					  &onto, &update_refs);
 
 	if (prepare_revision_walk(&revs) < 0) {
 		ret = error(_("error preparing revisions"));
 		goto cleanup;
 	}
@@ -405,11 +366,11 @@ int cmd_replay(int argc,
 	while ((commit = get_revision(&revs))) {
 		const struct name_decoration *decoration;
 		khint_t pos;
 		int hr;
 
-		if (!commit->parents)
+		if (!commit->parents) /* FIXME: Should handle replaying down to root commit */
 			die(_("replaying down to root commit is not supported yet!"));
 		if (commit->parents->next)
 			die(_("replaying merge commits is not supported yet!"));
 
 		last_commit = pick_regular_commit(repo, commit, replayed_commits,
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index c543d55857b..c0c59ae6938 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -41,61 +41,72 @@ test_expect_success 'setup' '
 
 	git switch main &&
 	test_commit L &&
 	test_commit M &&
 
+	git switch --detach topic4 &&
+	test_commit N &&
+	test_commit O &&
+	git switch -c topic-with-merge topic4 &&
+	test_merge P O --no-ff &&
+	git switch main &&
+
 	git switch -c conflict B &&
 	test_commit C.conflict C.t conflict
 '
 
 test_expect_success 'setup bare' '
 	git clone --bare . bare
 '
 
-test_expect_success '--onto with invalid commit-ish' '
-	cat >expect <<-EOF &&
-	fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
-	EOF
-	test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
+test_expect_success 'argument to --advance must be a reference' '
+	echo "fatal: argument to --advance must be a reference" >expect &&
+	oid=$(git rev-parse main) &&
+	test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
 	test_cmp expect actual
 '
 
-test_expect_success '--advance with invalid commit-ish' '
-	cat >expect <<-EOF &&
-	fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
-	EOF
-	test_must_fail git replay --advance=refs/not-valid topic1..topic2 2>actual &&
+test_expect_success '--onto with invalid commit-ish' '
+	printf "fatal: ${SQ}refs/not-valid${SQ} is not " >expect &&
+	printf "a valid commit-ish for --onto\n" >>expect &&
+	test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'option --onto or --advance is mandatory' '
-	cat >expect <<-\EOF &&
-	error: option --onto or --advance is mandatory
-	EOF
-	# First line is the error; rest is Usage
-	test_must_fail git replay topic1..topic2 >&1 2>&1 |
-		head -1 >actual &&
+	echo "error: option --onto or --advance is mandatory" >expect &&
+	test_might_fail git replay -h >>expect &&
+	test_must_fail git replay topic1..topic2 2>actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'no base or negative ref gives no-replaying down to root error' '
-	cat >expect <<-\EOF &&
-	fatal: replaying down to root commit is not supported yet!
-	EOF
+	echo "fatal: replaying down to root commit is not supported yet!" >expect &&
 	test_must_fail git replay --onto=topic1 topic2 2>actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'options --advance and --contained cannot be used together' '
-	cat >expect <<-EOF &&
-	fatal: options ${SQ}--advance${SQ} and ${SQ}--contained${SQ} cannot be used together
-	EOF
+	printf "fatal: options ${SQ}--advance${SQ} " >expect &&
+	printf "and ${SQ}--contained${SQ} cannot be used together\n" >>expect &&
 	test_must_fail git replay --advance=main --contained \
 		topic1..topic2 2>actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'cannot advance target ... ordering would be ill-defined' '
+	echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect &&
+	test_must_fail git replay --advance=main main topic1 topic2 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'replaying merge commits is not supported yet' '
+	echo "fatal: replaying merge commits is not supported yet!" >expect &&
+	test_must_fail git replay --advance=main main..topic-with-merge 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'using replay to rebase two branches, one on top of other' '
 	git replay --onto main topic1..topic2 >result &&
 
 	test_line_count = 1 result &&
 
Range-diff against v1:
-:  ----------- > 1:  314ba49dd2f replay: remove dead code and rearrange
-:  ----------- > 2:  976d336adef replay: find *onto only after testing for ref name
1:  d49923de7c5 ! 3:  a7a7e1e720e replay: die descriptively when invalid commit-ish
    @@ Metadata
     Author: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
      ## Commit message ##
    -    replay: die descriptively when invalid commit-ish
    +    replay: die descriptively when invalid commit-ish is given
     
    -    Giving an invalid commit-ish to `--onto` or `--advance` makes
    -    git-replay(1) fail with:
    +    Giving an invalid commit-ish to `--onto` makes git-replay(1) fail with:
     
             fatal: Replaying down to root commit is not supported yet!
     
    @@ Commit message
             $ git replay --onto ^main topic1
             fatal: '^main' is not a valid commit-ish
     
    +    Note that the `--advance` case won’t be triggered in practice because
    +    of the “argument to --advance must be a reference” check (see the
    +    previous test, and commit).
    +
         † 1: The argument to `--onto` is mandatory and the option parser accepts
              both `--onto=<name>` (stuck form) and `--onto name`. The latter
              form makes it easy to unintentionally pass something to the option
    @@ Commit message
     
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
    +
    + ## Notes (series) ##
    +    v2:
    +
    +    Let’s use a slightly longer subject line in the commit message so that it
    +    looks more like a full sentence (no dropped/implied words).[1]
    +
    +    Also remove the test for `--advance` which is now wrong because of the
    +    previous commit/patch. And reword the commit message now that only `--onto`
    +    is relevant in practice.
    +
    +    There was also feedback about *where* to give this error:[2]
    +
    +    > How many callers use this function?  I am wondering if it is better
    +    > to give a better message at the caller(s), rather than here, where
    +    > we lack context to tell something like "You gave string 'ource' as
    +    > the argument to the '--onto' option, but 'ource' does not name any
    +    > commit" (in other words, "for what our caller is trying to peel
    +    > <name> to a commit").
    +
    +    But I opted to keep the check here by using the new `mode` parameter to
    +    provide the context; it is either `--onto` or `--advance`.
    +
    +    Also remove the “not supported yet” now that `*onto` cannot be `NULL` at
    +    this point. I wasn’t confident enough to pull the trigger on that in the
    +    first round. But after Elijah’s comment[3] I feel like I understand the code
    +    well enough.
    +
    +    Also change the test to use printf since it’s only one line. That will be
    +    in line with the later commits/patches here.
    +
    +    🔗 1: https://lore.kernel.org/git/xmqqecolrip7.fsf@gitster.g/
    +    🔗 2: https://lore.kernel.org/git/xmqqikdxriw3.fsf@gitster.g/
    +    🔗 3: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/
    +
      ## builtin/replay.c ##
    -@@ builtin/replay.c: static struct commit *peel_committish(struct repository *repo, const char *name)
    +@@ builtin/replay.c: static const char *short_commit_name(struct repository *repo,
    + 				       DEFAULT_ABBREV);
    + }
    + 
    +-static struct commit *peel_committish(struct repository *repo, const char *name)
    ++static struct commit *peel_committish(struct repository *repo,
    ++				      const char *name,
    ++				      const char *mode)
    + {
    + 	struct object *obj;
      	struct object_id oid;
      
      	if (repo_get_oid(repo, name, &oid))
     -		return NULL;
    -+		die(_("'%s' is not a valid commit-ish"), name);
    ++		die(_("'%s' is not a valid commit-ish for %s"), name, mode);
      	obj = parse_object(repo, &oid);
      	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
      						  OBJ_COMMIT);
    +@@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repository *repo,
    + 	die_for_incompatible_opt2(!!onto_name, "--onto",
    + 				  !!*advance_name, "--advance");
    + 	if (onto_name) {
    +-		*onto = peel_committish(repo, onto_name);
    ++		*onto = peel_committish(repo, onto_name, "--onto");
    + 		if (rinfo.positive_refexprs <
    + 		    strset_get_size(&rinfo.positive_refs))
    + 			die(_("all positive revisions given must be references"));
    +@@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repository *repo,
    + 		} else {
    + 			die(_("argument to --advance must be a reference"));
    + 		}
    +-		*onto = peel_committish(repo, *advance_name);
    ++		*onto = peel_committish(repo, *advance_name, "--advance");
    + 		if (rinfo.positive_refexprs > 1)
    + 			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
    + 	}
    +@@ builtin/replay.c: int cmd_replay(int argc,
    + 					  onto_name, &advance_name,
    + 					  &onto, &update_refs);
    + 
    +-	if (!onto) /* FIXME: Should handle replaying down to root commit */
    +-		die("Replaying down to root commit is not supported yet!");
    +-
    + 	if (prepare_revision_walk(&revs) < 0) {
    + 		ret = error(_("error preparing revisions"));
    + 		goto cleanup;
    +@@ builtin/replay.c: int cmd_replay(int argc,
    + 		khint_t pos;
    + 		int hr;
    + 
    +-		if (!commit->parents)
    ++		if (!commit->parents) /* FIXME: Should handle replaying down to root commit */
    + 			die(_("replaying down to root commit is not supported yet!"));
    + 		if (commit->parents->next)
    + 			die(_("replaying merge commits is not supported yet!"));
     
      ## t/t3650-replay-basics.sh ##
    -@@ t/t3650-replay-basics.sh: test_expect_success 'setup bare' '
    - 	git clone --bare . bare
    +@@ t/t3650-replay-basics.sh: test_expect_success 'argument to --advance must be a reference' '
    + 	test_cmp expect actual
      '
      
     +test_expect_success '--onto with invalid commit-ish' '
    -+	cat >expect <<-EOF &&
    -+	fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
    -+	EOF
    ++	printf "fatal: ${SQ}refs/not-valid${SQ} is not " >expect &&
    ++	printf "a valid commit-ish for --onto\n" >>expect &&
     +	test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
     +	test_cmp expect actual
     +'
    -+
    -+test_expect_success '--advance with invalid commit-ish' '
    -+	cat >expect <<-EOF &&
    -+	fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
    -+	EOF
    -+	test_must_fail git replay --advance=refs/not-valid topic1..topic2 2>actual &&
    -+	test_cmp expect actual
    -+'
     +
      test_expect_success 'using replay to rebase two branches, one on top of other' '
      	git replay --onto main topic1..topic2 >result &&
-:  ----------- > 4:  9700630d95e replay: die if we cannot parse object
2:  5aa5b96950e ! 5:  5e1b9205df5 t3650: add more regression tests for failure conditions
    @@ Commit message
         a few more since these are simple to write and remove if they become
         obsolete.
     
    +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
    +
    + ## Notes (series) ##
    +    v2:
    +
    +    Improve test `option --onto or --advance is mandatory`. Phillip pointed out
    +    that using a pipe loses the return value. Instead let’s test the whole
    +    output by just appending `git replay -h` to `expect`.
    +
    +    Also “normalize” to just using echo/printf for the `expect` since these are
    +    just oneliner errors.
    +
    +    Also add two more tests (at the end).
    +
      ## t/t3650-replay-basics.sh ##
    -@@ t/t3650-replay-basics.sh: test_expect_success '--advance with invalid commit-ish' '
    +@@ t/t3650-replay-basics.sh: test_expect_success 'setup' '
    + 	test_commit L &&
    + 	test_commit M &&
    + 
    ++	git switch --detach topic4 &&
    ++	test_commit N &&
    ++	test_commit O &&
    ++	git switch -c topic-with-merge topic4 &&
    ++	test_merge P O --no-ff &&
    ++	git switch main &&
    ++
    + 	git switch -c conflict B &&
    + 	test_commit C.conflict C.t conflict
    + '
    +@@ t/t3650-replay-basics.sh: test_expect_success '--onto with invalid commit-ish' '
      	test_cmp expect actual
      '
      
     +test_expect_success 'option --onto or --advance is mandatory' '
    -+	cat >expect <<-\EOF &&
    -+	error: option --onto or --advance is mandatory
    -+	EOF
    -+	# First line is the error; rest is Usage
    -+	test_must_fail git replay topic1..topic2 >&1 2>&1 |
    -+		head -1 >actual &&
    ++	echo "error: option --onto or --advance is mandatory" >expect &&
    ++	test_might_fail git replay -h >>expect &&
    ++	test_must_fail git replay topic1..topic2 2>actual &&
     +	test_cmp expect actual
     +'
     +
     +test_expect_success 'no base or negative ref gives no-replaying down to root error' '
    -+	cat >expect <<-\EOF &&
    -+	fatal: replaying down to root commit is not supported yet!
    -+	EOF
    ++	echo "fatal: replaying down to root commit is not supported yet!" >expect &&
     +	test_must_fail git replay --onto=topic1 topic2 2>actual &&
     +	test_cmp expect actual
     +'
     +
     +test_expect_success 'options --advance and --contained cannot be used together' '
    -+	cat >expect <<-EOF &&
    -+	fatal: options ${SQ}--advance${SQ} and ${SQ}--contained${SQ} cannot be used together
    -+	EOF
    ++	printf "fatal: options ${SQ}--advance${SQ} " >expect &&
    ++	printf "and ${SQ}--contained${SQ} cannot be used together\n" >>expect &&
     +	test_must_fail git replay --advance=main --contained \
     +		topic1..topic2 2>actual &&
     +	test_cmp expect actual
     +'
    ++
    ++test_expect_success 'cannot advance target ... ordering would be ill-defined' '
    ++	echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect &&
    ++	test_must_fail git replay --advance=main main topic1 topic2 2>actual &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'replaying merge commits is not supported yet' '
    ++	echo "fatal: replaying merge commits is not supported yet!" >expect &&
    ++	test_must_fail git replay --advance=main main..topic-with-merge 2>actual &&
    ++	test_cmp expect actual
    ++'
     +
      test_expect_success 'using replay to rebase two branches, one on top of other' '
      	git replay --onto main topic1..topic2 >result &&

base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
-- 
2.52.0.10.g08704017180

[PATCH v2 1/5] replay: remove dead code and rearrange

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

22d99f01 (replay: add --advance or 'cherry-pick' mode, 2023-11-24) both
added `--advance` and made one of `--onto` or `--advance` mandatory.
But `determine_replay_mode` claims that there is a third alternative;
neither of `--onto` or `--advance` were given:

    if (onto_name) {
    ...
    } else if (*advance_name) {
    ...
    } else {
    ...
    }

But this is false—the fallthrough else-block is dead code.

Commit 22d99f01 was iterated upon by several people.[1] The initial
author wrote code for a sort of *guess mode*, allowing for shorter
commands when that was possible. But the next person instead made one
of the aforementioned options mandatory. In turn this code was dead on
arrival in git.git.

[1]: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/

Let’s remove this code. We can also join the if-block with the
condition `!*advance_name` into the `*onto` block since we do not set
`*advance_name` in this function. It only looked like we might set it
since the dead code has this line:

    *advance_name = xstrdup_or_null(last_key);

Let’s also rename the function since we do not determine
the replay mode here. We simply populate data structures.

Note that there might be more dead code caused by this *guess mode*.
We only concern ourselves with this function for now.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2: [new]
    
    See the link in the commit message.
    
    Not strictly needed for this series but I think it makes sense to fix it
    here.

 builtin/replay.c | 70 +++++++++++-------------------------------------
 1 file changed, 16 insertions(+), 54 deletions(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 6172c8aacc9..54849f65c87 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -154,16 +154,16 @@ static void get_ref_information(struct repository *repo,
 
 		free(fullname);
 	}
 }
 
-static void determine_replay_mode(struct repository *repo,
-				  struct rev_cmdline_info *cmd_info,
-				  const char *onto_name,
-				  char **advance_name,
-				  struct commit **onto,
-				  struct strset **update_refs)
+static void populate_for_onto_or_advance_mode(struct repository *repo,
+					      struct rev_cmdline_info *cmd_info,
+					      const char *onto_name,
+					      char **advance_name,
+					      struct commit **onto,
+					      struct strset **update_refs)
 {
 	struct ref_info rinfo;
 
 	get_ref_information(repo, cmd_info, &rinfo);
 	if (!rinfo.positive_refexprs)
@@ -174,69 +174,30 @@ static void determine_replay_mode(struct repository *repo,
 	if (onto_name) {
 		*onto = peel_committish(repo, onto_name);
 		if (rinfo.positive_refexprs <
 		    strset_get_size(&rinfo.positive_refs))
 			die(_("all positive revisions given must be references"));
-	} else if (*advance_name) {
+		*update_refs = xcalloc(1, sizeof(**update_refs));
+		**update_refs = rinfo.positive_refs;
+		memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
+	} else {
 		struct object_id oid;
 		char *fullname = NULL;
 
+		if (!*advance_name)
+			BUG("expected either onto_name or *advance_name in this function");
+
 		*onto = peel_committish(repo, *advance_name);
 		if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
 			     &oid, &fullname, 0) == 1) {
 			free(*advance_name);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
 		}
 		if (rinfo.positive_refexprs > 1)
 			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
-	} else {
-		int positive_refs_complete = (
-			rinfo.positive_refexprs ==
-			strset_get_size(&rinfo.positive_refs));
-		int negative_refs_complete = (
-			rinfo.negative_refexprs ==
-			strset_get_size(&rinfo.negative_refs));
-		/*
-		 * We need either positive_refs_complete or
-		 * negative_refs_complete, but not both.
-		 */
-		if (rinfo.negative_refexprs > 0 &&
-		    positive_refs_complete == negative_refs_complete)
-			die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
-		if (negative_refs_complete) {
-			struct hashmap_iter iter;
-			struct strmap_entry *entry;
-			const char *last_key = NULL;
-
-			if (rinfo.negative_refexprs == 0)
-				die(_("all positive revisions given must be references"));
-			else if (rinfo.negative_refexprs > 1)
-				die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
-			else if (rinfo.positive_refexprs > 1)
-				die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
-
-			/* Only one entry, but we have to loop to get it */
-			strset_for_each_entry(&rinfo.negative_refs,
-					      &iter, entry) {
-				last_key = entry->key;
-			}
-
-			free(*advance_name);
-			*advance_name = xstrdup_or_null(last_key);
-		} else { /* positive_refs_complete */
-			if (rinfo.negative_refexprs > 1)
-				die(_("cannot implicitly determine correct base for --onto"));
-			if (rinfo.negative_refexprs == 1)
-				*onto = rinfo.onto;
-		}
-	}
-	if (!*advance_name) {
-		*update_refs = xcalloc(1, sizeof(**update_refs));
-		**update_refs = rinfo.positive_refs;
-		memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
 	}
 	strset_clear(&rinfo.negative_refs);
 	strset_clear(&rinfo.positive_refs);
 }
 
@@ -384,12 +345,13 @@ int cmd_replay(int argc,
 			  "'%s' bit in 'struct rev_info' will be forced"),
 			"simplify_history");
 		revs.simplify_history = 0;
 	}
 
-	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
-			      &onto, &update_refs);
+	populate_for_onto_or_advance_mode(repo, &revs.cmdline,
+					  onto_name, &advance_name,
+					  &onto, &update_refs);
 
 	if (!onto) /* FIXME: Should handle replaying down to root commit */
 		die("Replaying down to root commit is not supported yet!");
 
 	if (prepare_revision_walk(&revs) < 0) {
-- 
2.52.0.10.g08704017180

[PATCH v2 2/5] replay: find *onto only after testing for ref name

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

We are about to make `peel_committish` die when it cannot find
a commit-ish instead of returning `NULL`. But that would make e.g.
`git replay --advance=refs/non-existent` die with a less descriptive
error message; the highest-level error message is that the name does
not exist as a ref, not that we cannot find a commit-ish based on
the name.

Let’s try to find the ref and only after that try to peel to
as a commit-ish.

Also add a regression test to protect this error-order from future
modifications.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2: [new]
    
    Fallout of v1. Needs to be moved so that the new error message does not
    “shadow” this one.
    
    See: https://lore.kernel.org/git/xmqqpl85pb7k.fsf@gitster.g/

 builtin/replay.c         | 2 +-
 t/t3650-replay-basics.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 54849f65c87..35813140e99 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -184,18 +184,18 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
 		char *fullname = NULL;
 
 		if (!*advance_name)
 			BUG("expected either onto_name or *advance_name in this function");
 
-		*onto = peel_committish(repo, *advance_name);
 		if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
 			     &oid, &fullname, 0) == 1) {
 			free(*advance_name);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
 		}
+		*onto = peel_committish(repo, *advance_name);
 		if (rinfo.positive_refexprs > 1)
 			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
 	}
 	strset_clear(&rinfo.negative_refs);
 	strset_clear(&rinfo.positive_refs);
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 58b37599357..7dea62f064f 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -49,10 +49,17 @@ test_expect_success 'setup' '
 
 test_expect_success 'setup bare' '
 	git clone --bare . bare
 '
 
+test_expect_success 'argument to --advance must be a reference' '
+	echo "fatal: argument to --advance must be a reference" >expect &&
+	oid=$(git rev-parse main) &&
+	test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'using replay to rebase two branches, one on top of other' '
 	git replay --onto main topic1..topic2 >result &&
 
 	test_line_count = 1 result &&
 
-- 
2.52.0.10.g08704017180

[PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Giving an invalid commit-ish to `--onto` makes git-replay(1) fail with:

    fatal: Replaying down to root commit is not supported yet!

Going backwards from this point:

1. `onto` is `NULL` from `determine_replay_mode`;
2. that function in turn calls `peel_committish`; and
3. here we return `NULL` if `repo_get_oid` fails.

Let’s die immediately with a descriptive error message instead.

Doing this also provides us with a descriptive error if we “forget” to
provide an argument to `--onto` (but we really do unintentionally):[1]

    $ git replay --onto ^main topic1
    fatal: '^main' is not a valid commit-ish

Note that the `--advance` case won’t be triggered in practice because
of the “argument to --advance must be a reference” check (see the
previous test, and commit).

† 1: The argument to `--onto` is mandatory and the option parser accepts
     both `--onto=<name>` (stuck form) and `--onto name`. The latter
     form makes it easy to unintentionally pass something to the option
     when you really meant to pass a positional argument.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    
    Let’s use a slightly longer subject line in the commit message so that it
    looks more like a full sentence (no dropped/implied words).[1]
    
    Also remove the test for `--advance` which is now wrong because of the
    previous commit/patch. And reword the commit message now that only `--onto`
    is relevant in practice.
    
    There was also feedback about *where* to give this error:[2]
    
    > How many callers use this function?  I am wondering if it is better
    > to give a better message at the caller(s), rather than here, where
    > we lack context to tell something like "You gave string 'ource' as
    > the argument to the '--onto' option, but 'ource' does not name any
    > commit" (in other words, "for what our caller is trying to peel
    > <name> to a commit").
    
    But I opted to keep the check here by using the new `mode` parameter to
    provide the context; it is either `--onto` or `--advance`.
    
    Also remove the “not supported yet” now that `*onto` cannot be `NULL` at
    this point. I wasn’t confident enough to pull the trigger on that in the
    first round. But after Elijah’s comment[3] I feel like I understand the code
    well enough.
    
    Also change the test to use printf since it’s only one line. That will be
    in line with the later commits/patches here.
    
    🔗 1: https://lore.kernel.org/git/xmqqecolrip7.fsf@gitster.g/
    🔗 2: https://lore.kernel.org/git/xmqqikdxriw3.fsf@gitster.g/
    🔗 3: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/

 builtin/replay.c         | 15 +++++++--------
 t/t3650-replay-basics.sh |  7 +++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 35813140e99..07a6767ade1 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -25,17 +25,19 @@ static const char *short_commit_name(struct repository *repo,
 {
 	return repo_find_unique_abbrev(repo, &commit->object.oid,
 				       DEFAULT_ABBREV);
 }
 
-static struct commit *peel_committish(struct repository *repo, const char *name)
+static struct commit *peel_committish(struct repository *repo,
+				      const char *name,
+				      const char *mode)
 {
 	struct object *obj;
 	struct object_id oid;
 
 	if (repo_get_oid(repo, name, &oid))
-		return NULL;
+		die(_("'%s' is not a valid commit-ish for %s"), name, mode);
 	obj = parse_object(repo, &oid);
 	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
 						  OBJ_COMMIT);
 }
 
@@ -170,11 +172,11 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
 		die(_("need some commits to replay"));
 
 	die_for_incompatible_opt2(!!onto_name, "--onto",
 				  !!*advance_name, "--advance");
 	if (onto_name) {
-		*onto = peel_committish(repo, onto_name);
+		*onto = peel_committish(repo, onto_name, "--onto");
 		if (rinfo.positive_refexprs <
 		    strset_get_size(&rinfo.positive_refs))
 			die(_("all positive revisions given must be references"));
 		*update_refs = xcalloc(1, sizeof(**update_refs));
 		**update_refs = rinfo.positive_refs;
@@ -191,11 +193,11 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
 			free(*advance_name);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
 		}
-		*onto = peel_committish(repo, *advance_name);
+		*onto = peel_committish(repo, *advance_name, "--advance");
 		if (rinfo.positive_refexprs > 1)
 			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
 	}
 	strset_clear(&rinfo.negative_refs);
 	strset_clear(&rinfo.positive_refs);
@@ -349,13 +351,10 @@ int cmd_replay(int argc,
 
 	populate_for_onto_or_advance_mode(repo, &revs.cmdline,
 					  onto_name, &advance_name,
 					  &onto, &update_refs);
 
-	if (!onto) /* FIXME: Should handle replaying down to root commit */
-		die("Replaying down to root commit is not supported yet!");
-
 	if (prepare_revision_walk(&revs) < 0) {
 		ret = error(_("error preparing revisions"));
 		goto cleanup;
 	}
 
@@ -367,11 +366,11 @@ int cmd_replay(int argc,
 	while ((commit = get_revision(&revs))) {
 		const struct name_decoration *decoration;
 		khint_t pos;
 		int hr;
 
-		if (!commit->parents)
+		if (!commit->parents) /* FIXME: Should handle replaying down to root commit */
 			die(_("replaying down to root commit is not supported yet!"));
 		if (commit->parents->next)
 			die(_("replaying merge commits is not supported yet!"));
 
 		last_commit = pick_regular_commit(repo, commit, replayed_commits,
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 7dea62f064f..d4399aa1662 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -56,10 +56,17 @@ test_expect_success 'argument to --advance must be a reference' '
 	oid=$(git rev-parse main) &&
 	test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
 	test_cmp expect actual
 '
 
+test_expect_success '--onto with invalid commit-ish' '
+	printf "fatal: ${SQ}refs/not-valid${SQ} is not " >expect &&
+	printf "a valid commit-ish for --onto\n" >>expect &&
+	test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'using replay to rebase two branches, one on top of other' '
 	git replay --onto main topic1..topic2 >result &&
 
 	test_line_count = 1 result &&
 
-- 
2.52.0.10.g08704017180

[PATCH v2 4/5] replay: die if we cannot parse object

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`parse_object` can return `NULL`. That will in turn make
`repo_peel_to_type` return the same.

Let’s die fast and descriptively with the `*_or_die` variant.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2: [new]
    
    See: https://lore.kernel.org/git/xmqqikdxriw3.fsf@gitster.g/
    
    With the `*_or_die` function we don’t have to check it at the call site.

 builtin/replay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 07a6767ade1..ca5a14de4c7 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -34,11 +34,11 @@ static struct commit *peel_committish(struct repository *repo,
 	struct object *obj;
 	struct object_id oid;
 
 	if (repo_get_oid(repo, name, &oid))
 		die(_("'%s' is not a valid commit-ish for %s"), name, mode);
-	obj = parse_object(repo, &oid);
+	obj = parse_object_or_die(repo, &oid, name);
 	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
 						  OBJ_COMMIT);
 }
 
 static char *get_author(const char *message)
-- 
2.52.0.10.g08704017180

[PATCH v2 5/5] t3650: add more regression tests for failure conditions

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

There isn’t much test coverage for basic failure conditions. Let’s add
a few more since these are simple to write and remove if they become
obsolete.

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    
    Improve test `option --onto or --advance is mandatory`. Phillip pointed out
    that using a pipe loses the return value. Instead let’s test the whole
    output by just appending `git replay -h` to `expect`.
    
    Also “normalize” to just using echo/printf for the `expect` since these are
    just oneliner errors.
    
    Also add two more tests (at the end).

 t/t3650-replay-basics.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index d4399aa1662..c0c59ae6938 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -41,10 +41,17 @@ test_expect_success 'setup' '
 
 	git switch main &&
 	test_commit L &&
 	test_commit M &&
 
+	git switch --detach topic4 &&
+	test_commit N &&
+	test_commit O &&
+	git switch -c topic-with-merge topic4 &&
+	test_merge P O --no-ff &&
+	git switch main &&
+
 	git switch -c conflict B &&
 	test_commit C.conflict C.t conflict
 '
 
 test_expect_success 'setup bare' '
@@ -63,10 +70,43 @@ test_expect_success '--onto with invalid commit-ish' '
 	printf "a valid commit-ish for --onto\n" >>expect &&
 	test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'option --onto or --advance is mandatory' '
+	echo "error: option --onto or --advance is mandatory" >expect &&
+	test_might_fail git replay -h >>expect &&
+	test_must_fail git replay topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'no base or negative ref gives no-replaying down to root error' '
+	echo "fatal: replaying down to root commit is not supported yet!" >expect &&
+	test_must_fail git replay --onto=topic1 topic2 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'options --advance and --contained cannot be used together' '
+	printf "fatal: options ${SQ}--advance${SQ} " >expect &&
+	printf "and ${SQ}--contained${SQ} cannot be used together\n" >>expect &&
+	test_must_fail git replay --advance=main --contained \
+		topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cannot advance target ... ordering would be ill-defined' '
+	echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect &&
+	test_must_fail git replay --advance=main main topic1 topic2 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'replaying merge commits is not supported yet' '
+	echo "fatal: replaying merge commits is not supported yet!" >expect &&
+	test_must_fail git replay --advance=main main..topic-with-merge 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'using replay to rebase two branches, one on top of other' '
 	git replay --onto main topic1..topic2 >result &&
 
 	test_line_count = 1 result &&
 
-- 
2.52.0.10.g08704017180

Re: [PATCH v2 1/5] replay: remove dead code and rearrange

[Elijah Newren]

On Tue, Dec 30, 2025 at 7:03 AM <kristofferhaugsbakk@fastmail.com> wrote:
>
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> 22d99f01 (replay: add --advance or 'cherry-pick' mode, 2023-11-24) both
> added `--advance` and made one of `--onto` or `--advance` mandatory.
> But `determine_replay_mode` claims that there is a third alternative;
> neither of `--onto` or `--advance` were given:
>
>     if (onto_name) {
>     ...
>     } else if (*advance_name) {
>     ...
>     } else {
>     ...
>     }
>
> But this is false—the fallthrough else-block is dead code.
>
> Commit 22d99f01 was iterated upon by several people.[1] The initial
> author wrote code for a sort of *guess mode*, allowing for shorter
> commands when that was possible. But the next person instead made one
> of the aforementioned options mandatory. In turn this code was dead on
> arrival in git.git.
>
> [1]: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/
>
> Let’s remove this code. We can also join the if-block with the
> condition `!*advance_name` into the `*onto` block since we do not set
> `*advance_name` in this function. It only looked like we might set it
> since the dead code has this line:
>
>     *advance_name = xstrdup_or_null(last_key);
>
> Let’s also rename the function since we do not determine
> the replay mode here. We simply populate data structures.
>
> Note that there might be more dead code caused by this *guess mode*.
> We only concern ourselves with this function for now.
>
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>
> Notes (series):
>     v2: [new]
>
>     See the link in the commit message.
>
>     Not strictly needed for this series but I think it makes sense to fix it
>     here.
>
>  builtin/replay.c | 70 +++++++++++-------------------------------------
>  1 file changed, 16 insertions(+), 54 deletions(-)
>
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 6172c8aacc9..54849f65c87 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -154,16 +154,16 @@ static void get_ref_information(struct repository *repo,
>
>                 free(fullname);
>         }
>  }
>
> -static void determine_replay_mode(struct repository *repo,
> -                                 struct rev_cmdline_info *cmd_info,
> -                                 const char *onto_name,
> -                                 char **advance_name,
> -                                 struct commit **onto,
> -                                 struct strset **update_refs)
> +static void populate_for_onto_or_advance_mode(struct repository *repo,
> +                                             struct rev_cmdline_info *cmd_info,
> +                                             const char *onto_name,
> +                                             char **advance_name,
> +                                             struct commit **onto,
> +                                             struct strset **update_refs)

Renaming makes sense, but the new name is quite the mouthful, and it
feels slightly odd because "onto" is both a command line flag and a
variable -- and the variable value is used regardless of which command
line flag is used.  Since the variable is used either way, there's a
risk someone might be confused by this function name.  Maybe just
setup_replay_mode() ?  Or maybe others have other suggestions?

>  {
>         struct ref_info rinfo;
>
>         get_ref_information(repo, cmd_info, &rinfo);
>         if (!rinfo.positive_refexprs)
> @@ -174,69 +174,30 @@ static void determine_replay_mode(struct repository *repo,
>         if (onto_name) {
>                 *onto = peel_committish(repo, onto_name);
>                 if (rinfo.positive_refexprs <
>                     strset_get_size(&rinfo.positive_refs))
>                         die(_("all positive revisions given must be references"));
> -       } else if (*advance_name) {
> +               *update_refs = xcalloc(1, sizeof(**update_refs));
> +               **update_refs = rinfo.positive_refs;
> +               memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
> +       } else {
>                 struct object_id oid;
>                 char *fullname = NULL;
>
> +               if (!*advance_name)
> +                       BUG("expected either onto_name or *advance_name in this function");
> +
>                 *onto = peel_committish(repo, *advance_name);
>                 if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
>                              &oid, &fullname, 0) == 1) {
>                         free(*advance_name);
>                         *advance_name = fullname;
>                 } else {
>                         die(_("argument to --advance must be a reference"));
>                 }
>                 if (rinfo.positive_refexprs > 1)
>                         die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
> -       } else {
> -               int positive_refs_complete = (
> -                       rinfo.positive_refexprs ==
> -                       strset_get_size(&rinfo.positive_refs));
> -               int negative_refs_complete = (
> -                       rinfo.negative_refexprs ==
> -                       strset_get_size(&rinfo.negative_refs));
> -               /*
> -                * We need either positive_refs_complete or
> -                * negative_refs_complete, but not both.
> -                */
> -               if (rinfo.negative_refexprs > 0 &&
> -                   positive_refs_complete == negative_refs_complete)
> -                       die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
> -               if (negative_refs_complete) {
> -                       struct hashmap_iter iter;
> -                       struct strmap_entry *entry;
> -                       const char *last_key = NULL;
> -
> -                       if (rinfo.negative_refexprs == 0)
> -                               die(_("all positive revisions given must be references"));
> -                       else if (rinfo.negative_refexprs > 1)
> -                               die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
> -                       else if (rinfo.positive_refexprs > 1)
> -                               die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
> -
> -                       /* Only one entry, but we have to loop to get it */
> -                       strset_for_each_entry(&rinfo.negative_refs,
> -                                             &iter, entry) {
> -                               last_key = entry->key;
> -                       }
> -
> -                       free(*advance_name);
> -                       *advance_name = xstrdup_or_null(last_key);
> -               } else { /* positive_refs_complete */
> -                       if (rinfo.negative_refexprs > 1)
> -                               die(_("cannot implicitly determine correct base for --onto"));
> -                       if (rinfo.negative_refexprs == 1)
> -                               *onto = rinfo.onto;
> -               }
> -       }
> -       if (!*advance_name) {
> -               *update_refs = xcalloc(1, sizeof(**update_refs));
> -               **update_refs = rinfo.positive_refs;
> -               memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
>         }
>         strset_clear(&rinfo.negative_refs);
>         strset_clear(&rinfo.positive_refs);
>  }
>
> @@ -384,12 +345,13 @@ int cmd_replay(int argc,
>                           "'%s' bit in 'struct rev_info' will be forced"),
>                         "simplify_history");
>                 revs.simplify_history = 0;
>         }
>
> -       determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
> -                             &onto, &update_refs);
> +       populate_for_onto_or_advance_mode(repo, &revs.cmdline,
> +                                         onto_name, &advance_name,
> +                                         &onto, &update_refs);
>
>         if (!onto) /* FIXME: Should handle replaying down to root commit */
>                 die("Replaying down to root commit is not supported yet!");
>
>         if (prepare_revision_walk(&revs) < 0) {
> --
> 2.52.0.10.g08704017180

Looks fine otherwise.

Re: [PATCH v2 2/5] replay: find *onto only after testing for ref name

[Elijah Newren]

On Tue, Dec 30, 2025 at 7:03 AM <kristofferhaugsbakk@fastmail.com> wrote:
>
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> We are about to make `peel_committish` die when it cannot find
> a commit-ish instead of returning `NULL`. But that would make e.g.
> `git replay --advance=refs/non-existent` die with a less descriptive
> error message; the highest-level error message is that the name does
> not exist as a ref, not that we cannot find a commit-ish based on
> the name.
>
> Let’s try to find the ref and only after that try to peel to
> as a commit-ish.
>
> Also add a regression test to protect this error-order from future
> modifications.

"error-order" looked like a typo and took a while for me to parse.
Maybe drop the hyphen or replace with "order of errors"?


>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>
> Notes (series):
>     v2: [new]
>
>     Fallout of v1. Needs to be moved so that the new error message does not
>     “shadow” this one.
>
>     See: https://lore.kernel.org/git/xmqqpl85pb7k.fsf@gitster.g/
>
>  builtin/replay.c         | 2 +-
>  t/t3650-replay-basics.sh | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 54849f65c87..35813140e99 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -184,18 +184,18 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
>                 char *fullname = NULL;
>
>                 if (!*advance_name)
>                         BUG("expected either onto_name or *advance_name in this function");
>
> -               *onto = peel_committish(repo, *advance_name);
>                 if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
>                              &oid, &fullname, 0) == 1) {
>                         free(*advance_name);
>                         *advance_name = fullname;
>                 } else {
>                         die(_("argument to --advance must be a reference"));
>                 }
> +               *onto = peel_committish(repo, *advance_name);
>                 if (rinfo.positive_refexprs > 1)
>                         die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
>         }
>         strset_clear(&rinfo.negative_refs);
>         strset_clear(&rinfo.positive_refs);
> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index 58b37599357..7dea62f064f 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -49,10 +49,17 @@ test_expect_success 'setup' '
>
>  test_expect_success 'setup bare' '
>         git clone --bare . bare
>  '
>
> +test_expect_success 'argument to --advance must be a reference' '
> +       echo "fatal: argument to --advance must be a reference" >expect &&
> +       oid=$(git rev-parse main) &&
> +       test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
> +       test_cmp expect actual
> +'
> +
>  test_expect_success 'using replay to rebase two branches, one on top of other' '
>         git replay --onto main topic1..topic2 >result &&
>
>         test_line_count = 1 result &&
>
> --
> 2.52.0.10.g08704017180

Looks good otherwise.

Re: [PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given

[Elijah Newren]

On Tue, Dec 30, 2025 at 7:03 AM <kristofferhaugsbakk@fastmail.com> wrote:
>
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Giving an invalid commit-ish to `--onto` makes git-replay(1) fail with:
>
>     fatal: Replaying down to root commit is not supported yet!
>
> Going backwards from this point:
>
> 1. `onto` is `NULL` from `determine_replay_mode`;

`determine_replay_mode` no longer exists due to your new patch 1.

> 2. that function in turn calls `peel_committish`; and
> 3. here we return `NULL` if `repo_get_oid` fails.
>
> Let’s die immediately with a descriptive error message instead.
>
> Doing this also provides us with a descriptive error if we “forget” to
> provide an argument to `--onto` (but we really do unintentionally):[1]
>
>     $ git replay --onto ^main topic1
>     fatal: '^main' is not a valid commit-ish
>
> Note that the `--advance` case won’t be triggered in practice because
> of the “argument to --advance must be a reference” check (see the
> previous test, and commit).
>
> † 1: The argument to `--onto` is mandatory and the option parser accepts
>      both `--onto=<name>` (stuck form) and `--onto name`. The latter
>      form makes it easy to unintentionally pass something to the option
>      when you really meant to pass a positional argument.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>
> Notes (series):
>     v2:
>
>     Let’s use a slightly longer subject line in the commit message so that it
>     looks more like a full sentence (no dropped/implied words).[1]
>
>     Also remove the test for `--advance` which is now wrong because of the
>     previous commit/patch. And reword the commit message now that only `--onto`
>     is relevant in practice.
>
>     There was also feedback about *where* to give this error:[2]
>
>     > How many callers use this function?  I am wondering if it is better
>     > to give a better message at the caller(s), rather than here, where
>     > we lack context to tell something like "You gave string 'ource' as
>     > the argument to the '--onto' option, but 'ource' does not name any
>     > commit" (in other words, "for what our caller is trying to peel
>     > <name> to a commit").
>
>     But I opted to keep the check here by using the new `mode` parameter to
>     provide the context; it is either `--onto` or `--advance`.
>
>     Also remove the “not supported yet” now that `*onto` cannot be `NULL` at
>     this point. I wasn’t confident enough to pull the trigger on that in the
>     first round. But after Elijah’s comment[3] I feel like I understand the code
>     well enough.
>
>     Also change the test to use printf since it’s only one line. That will be
>     in line with the later commits/patches here.
>
>     🔗 1: https://lore.kernel.org/git/xmqqecolrip7.fsf@gitster.g/
>     🔗 2: https://lore.kernel.org/git/xmqqikdxriw3.fsf@gitster.g/
>     🔗 3: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/
>
>  builtin/replay.c         | 15 +++++++--------
>  t/t3650-replay-basics.sh |  7 +++++++
>  2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 35813140e99..07a6767ade1 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -25,17 +25,19 @@ static const char *short_commit_name(struct repository *repo,
>  {
>         return repo_find_unique_abbrev(repo, &commit->object.oid,
>                                        DEFAULT_ABBREV);
>  }
>
> -static struct commit *peel_committish(struct repository *repo, const char *name)
> +static struct commit *peel_committish(struct repository *repo,
> +                                     const char *name,
> +                                     const char *mode)
>  {
>         struct object *obj;
>         struct object_id oid;
>
>         if (repo_get_oid(repo, name, &oid))
> -               return NULL;
> +               die(_("'%s' is not a valid commit-ish for %s"), name, mode);
>         obj = parse_object(repo, &oid);
>         return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
>                                                   OBJ_COMMIT);
>  }
>
> @@ -170,11 +172,11 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
>                 die(_("need some commits to replay"));
>
>         die_for_incompatible_opt2(!!onto_name, "--onto",
>                                   !!*advance_name, "--advance");
>         if (onto_name) {
> -               *onto = peel_committish(repo, onto_name);
> +               *onto = peel_committish(repo, onto_name, "--onto");
>                 if (rinfo.positive_refexprs <
>                     strset_get_size(&rinfo.positive_refs))
>                         die(_("all positive revisions given must be references"));
>                 *update_refs = xcalloc(1, sizeof(**update_refs));
>                 **update_refs = rinfo.positive_refs;
> @@ -191,11 +193,11 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
>                         free(*advance_name);
>                         *advance_name = fullname;
>                 } else {
>                         die(_("argument to --advance must be a reference"));
>                 }
> -               *onto = peel_committish(repo, *advance_name);
> +               *onto = peel_committish(repo, *advance_name, "--advance");
>                 if (rinfo.positive_refexprs > 1)
>                         die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
>         }
>         strset_clear(&rinfo.negative_refs);
>         strset_clear(&rinfo.positive_refs);
> @@ -349,13 +351,10 @@ int cmd_replay(int argc,
>
>         populate_for_onto_or_advance_mode(repo, &revs.cmdline,
>                                           onto_name, &advance_name,
>                                           &onto, &update_refs);
>
> -       if (!onto) /* FIXME: Should handle replaying down to root commit */
> -               die("Replaying down to root commit is not supported yet!");
> -

Removing the `if` makes sense given the current code, but I wonder if
we should keep a corrected FIXME here:
    /* FIXME: Should allow replaying commits with the first as a root commit */

This is out-of-scope for this series, but behind that FIXME...

 I'm guessing the user would specify to cherry-pick onto NULL via something like
   git replay --root A..B
which would translate into making `onto` be NULL, and mean that the
first commit after A would be a root commit.

Similarly the user could be allowed to do something like
  git replay --advance new-empty-branch A..B
where new-empty-branch doesn't yet point to a commit, this would also
result in `onto` being NULL, and start new-empty-branch by
cherry-picking some commits into it.

>         if (prepare_revision_walk(&revs) < 0) {
>                 ret = error(_("error preparing revisions"));
>                 goto cleanup;
>         }
>
> @@ -367,11 +366,11 @@ int cmd_replay(int argc,
>         while ((commit = get_revision(&revs))) {
>                 const struct name_decoration *decoration;
>                 khint_t pos;
>                 int hr;
>
> -               if (!commit->parents)
> +               if (!commit->parents) /* FIXME: Should handle replaying down to root commit */
>                         die(_("replaying down to root commit is not supported yet!"));

I wonder if I should have written s/to/from/ here ?


>                 if (commit->parents->next)
>                         die(_("replaying merge commits is not supported yet!"));
>
>                 last_commit = pick_regular_commit(repo, commit, replayed_commits,
> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index 7dea62f064f..d4399aa1662 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -56,10 +56,17 @@ test_expect_success 'argument to --advance must be a reference' '
>         oid=$(git rev-parse main) &&
>         test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
>         test_cmp expect actual
>  '
>
> +test_expect_success '--onto with invalid commit-ish' '
> +       printf "fatal: ${SQ}refs/not-valid${SQ} is not " >expect &&
> +       printf "a valid commit-ish for --onto\n" >>expect &&
> +       test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
> +       test_cmp expect actual
> +'
> +
>  test_expect_success 'using replay to rebase two branches, one on top of other' '
>         git replay --onto main topic1..topic2 >result &&
>
>         test_line_count = 1 result &&
>
> --
> 2.52.0.10.g08704017180

Re: [PATCH v2 0/5] replay: die descriptively when invalid commit-ish

[Elijah Newren]

On Tue, Dec 30, 2025 at 7:02 AM <kristofferhaugsbakk@fastmail.com> wrote:
>
[...]
> § Changes in v2
>
> In this version we remove some dead code (see previous section), improve
> tests and add a couple more, and improve the main “die descriptively”
> part, with two additional patches/commits to that end:
>
> • replay: find *onto only after testing for ref name
> • replay: die if we cannot parse object
>
> See the notes on each patch/commit for change details.

I had a few minor comments on a few of the patches, but otherwise it
looked good to me.

Re: [PATCH v2 1/5] replay: remove dead code and rearrange

[Junio C Hamano]

> > -static void determine_replay_mode(struct repository *repo,
> > ...
> > +static void populate_for_onto_or_advance_mode(struct repository *repo,
> > ...
> Renaming makes sense, but the new name is quite the mouthful, and it
> feels slightly odd because "onto" is both a command line flag and a
> variable -- and the variable value is used regardless of which command
> line flag is used.  Since the variable is used either way, there's a
> risk someone might be confused by this function name.  Maybe just
> setup_replay_mode() ?  Or maybe others have other suggestions?

After reading the above, the name that came to my mind is (curiously)
determine_replay_mode() ;-).

> Looks fine otherwise.

Thanks.