From 76eab50f756fedfa28388213d7fea209f86dfae6 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:17 +0100 Subject: [PATCH 1/6] replay: remove dead code and rearrange MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 just set up `*onto` and refs to update. 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 Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 70 +++++++++++------------------------------------- 1 file changed, 16 insertions(+), 54 deletions(-) diff --git a/builtin/replay.c b/builtin/replay.c index 69c4c55129..524bf96ffd 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -162,12 +162,12 @@ static void get_ref_information(struct repository *repo, } } -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 set_up_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) { struct ref_info rinfo; @@ -182,10 +182,16 @@ static void determine_replay_mode(struct repository *repo, 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) { @@ -196,51 +202,6 @@ static void determine_replay_mode(struct repository *repo, } 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); @@ -451,8 +412,9 @@ int cmd_replay(int argc, revs.simplify_history = 0; } - determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name, - &onto, &update_refs); + set_up_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!"); From 17b7965a03bd38215cb78ae1c4b9646d0ee73a40 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:18 +0100 Subject: [PATCH 2/6] replay: find *onto only after testing for ref name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- 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 524bf96ffd..9265ebcd05 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -192,7 +192,6 @@ static void set_up_replay_mode(struct repository *repo, 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); @@ -200,6 +199,7 @@ static void set_up_replay_mode(struct repository *repo, } 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")); } diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index cf3aacf355..8ef0b1984d 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -51,6 +51,13 @@ 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 --ref-action=print --onto main topic1..topic2 >result && From 3074d08cfa1bfb75f96fa4a240c575fad4cb8060 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:19 +0100 Subject: [PATCH 3/6] replay: die descriptively when invalid commit-ish is given MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `set_up_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=` (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 Signed-off-by: Junio C Hamano --- builtin/replay.c | 13 +++++++------ t/t3650-replay-basics.sh | 7 +++++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/builtin/replay.c b/builtin/replay.c index 9265ebcd05..1899ccc7cc 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -33,13 +33,15 @@ 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 for %s"), name, mode); obj = parse_object(repo, &oid); return (struct commit *)repo_peel_to_type(repo, name, 0, obj, OBJ_COMMIT); @@ -178,7 +180,7 @@ static void set_up_replay_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")); @@ -199,7 +201,7 @@ static void set_up_replay_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")); } @@ -416,8 +418,7 @@ 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!"); + /* FIXME: Should handle replaying down to root commit */ /* Build reflog message */ if (advance_name_opt) diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index 8ef0b1984d..8d82dad714 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -58,6 +58,13 @@ test_expect_success 'argument to --advance must be a reference' ' 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 --ref-action=print --onto main topic1..topic2 >result && From f67f7ddbbd03634318d54b1d1ad7ed8df4a2b292 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:20 +0100 Subject: [PATCH 4/6] replay: improve code comment and die message Suggested-by: Elijah Newren Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/replay.c b/builtin/replay.c index 1899ccc7cc..402db44af2 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -418,7 +418,7 @@ int cmd_replay(int argc, onto_name, &advance_name, &onto, &update_refs); - /* FIXME: Should handle replaying down to root commit */ + /* FIXME: Should allow replaying commits with the first as a root commit */ /* Build reflog message */ if (advance_name_opt) @@ -454,7 +454,7 @@ int cmd_replay(int argc, int hr; if (!commit->parents) - die(_("replaying down to root commit is not supported yet!")); + die(_("replaying down from root commit is not supported yet!")); if (commit->parents->next) die(_("replaying merge commits is not supported yet!")); From 6f693364cc183ea5a8296c9ce2ff515f47206f92 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:21 +0100 Subject: [PATCH 5/6] replay: die if we cannot parse object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/replay.c b/builtin/replay.c index 402db44af2..1960bbbee8 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -42,7 +42,7 @@ static struct commit *peel_committish(struct repository *repo, 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); } From 56b77a687eaf9c48482e9f59ab7077e442e85ff5 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:22 +0100 Subject: [PATCH 6/6] t3650: add more regression tests for failure conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- 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 8d82dad714..307101eeb9 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -43,6 +43,13 @@ 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 ' @@ -65,6 +72,39 @@ test_expect_success '--onto with invalid commit-ish' ' 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 from 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 --ref-action=print --onto main topic1..topic2 >result &&