[PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction

["SZEDER Gábor" <szeder.dev@gmail.com>]

Users can add their own 'update-ref <ref>' instructions to the rebase
todo list, which also gives them the possibility to specify an invalid
ref as argument.  Now, while git does catch any invalid ref and errors
out, it does so at the very end of the rebase process, when the
invalid ref causes the transaction updating all involved refs to fail,
leaving users on their own to figure out where each of those refs
should point now and to update them themselves.

Let's do better, and catch invalid refs early on by calling
check_refname_format() for the argument of each 'update-ref'
instruction while parsing the todo file.  This way 'git rebase' would
error out right after the user finished editing the todo file, and
would show the same generic advice to rectify the situation that is
shown e.g.  after an unknown instruction or a missing argument for a
'pick' instruction, etc.

Furthermore, require that all refs given to 'update-ref' instructions
live under the "refs/" hierarchy.  The argument of the 'update-ref'
instruction is treated as a fully qualified ref, so if the todo list
were to contain the 'update-ref foo' instruction, then 'git rebase'
would happily create the ref file '.git/foo' containing the
appropriate object id.  This is most likely not what the user wanted
and will cause confusion.  I assume it's much more probable that some
users simply forgot about the "refs/heads/" prefix than that they have
a use-case for using 'git rebase' to create/update a ref outside the
"refs/" hierarchy.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 sequencer.c                   | 19 ++++++++++++++++++-
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index f1732f88f3..ababfa6352 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2522,7 +2522,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 			     command_to_string(item->command));
 
 	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
-	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
+	    item->command == TODO_RESET) {
 		item->commit = NULL;
 		item->arg_offset = bol - buf;
 		item->arg_len = (int)(eol - bol);
@@ -2556,6 +2556,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 		}
 	}
 
+	if (item->command == TODO_UPDATE_REF) {
+		struct strbuf ref = STRBUF_INIT;
+		int ret = 0;
+
+		item->commit = NULL;
+		item->arg_offset = bol - buf;
+		item->arg_len = (int)(eol - bol);
+
+		strbuf_add(&ref, bol, item->arg_len);
+		if (!starts_with(ref.buf, "refs/") ||
+		    check_refname_format(ref.buf, 0))
+			ret = error(_("invalid ref for update-ref instruction: %s"), ref.buf);
+
+		strbuf_release(&ref);
+		return ret;
+	}
+
 	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
 	saved = *end_of_object_name;
 	*end_of_object_name = '\0';
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 2e081b3914..b97f1e8b31 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' '
 	test_cmp_rev HEAD refs/heads/no-conflict-branch
 '
 
+test_expect_success 'update-refs with invalid refs' '
+	cat >fake-todo-4 <<-EOF &&
+	update-ref refs/heads/foo..bar
+	update-ref refs/heads/foo.lock
+	update-ref foo
+	update-ref foo/bar
+	pick $(git rev-parse HEAD)
+	EOF
+	cat >expect.err <<-EOF &&
+	error: invalid ref for update-ref instruction: refs/heads/foo..bar
+	error: invalid line 1: update-ref refs/heads/foo..bar
+	error: invalid ref for update-ref instruction: refs/heads/foo.lock
+	error: invalid line 2: update-ref refs/heads/foo.lock
+	error: invalid ref for update-ref instruction: foo
+	error: invalid line 3: update-ref foo
+	error: invalid ref for update-ref instruction: foo/bar
+	error: invalid line 4: update-ref foo/bar
+	You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
+	Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
+	EOF
+	test_when_finished "test_might_fail git rebase --abort" &&
+	(
+		set_replace_editor fake-todo-4 &&
+		test_must_fail git rebase -i HEAD^ 2>err
+	) &&
+	test_cmp expect.err err
+'
+
 test_expect_success REFFILES '--update-refs: check failed ref update' '
 	git checkout -B update-refs-error no-conflict-branch &&
 	git branch -f base HEAD~4 &&
-- 
2.38.0.rc2.542.g9b62912f7f