From: Elijah Newren <newren@gmail.com>
Date: Tue, 30 Dec 2025 22:51:25 GMT
Subject: Re: [PATCH v2 2/5] replay: find *onto only after testing for ref name
Message-ID: <CABPp-BE13K1QB42YLv3mLzB9+jUgkMtHNmbs_XWoTsbv2zSYog@mail.gmail.com>
In-Reply-To: <V2_~axonto_after_ref_test.17d@msgid.xyz>

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.