replay: die descriptively when invalid commit-ish is given

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=<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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/replay.c b/builtin/replay.c
index 9265ebcd05d569024e381a0f6726a5e08d010ae0..1899ccc7cc3ff53eac1afcf07ab9f4f5bed991f2 100644 (file)
--- 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 8ef0b1984d73246f1eb875952d7259d51709f3cb..8d82dad71486ee35eca3ce713a0063e6c2ef6c23 100755 (executable)
--- 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 &&