[v3] rebase -i: check labels and refs when parsing todo list

Commit Message

Phillip Wood Feb. 21, 2023, 5:05 p.m. UTC

From: Phillip Wood <[e-mail]>

Check that the argument to the "label" and "update-ref" commands is a
valid refname when the todo list is parsed rather than waiting until the
command is executed. This means that the user can deal with any errors
at the beginning of the rebase rather than having it stop halfway
through due to a typo in a label name. The "update-ref" command is
changed to reject single level refs as it is all to easy to type
"update-ref branch" which is incorrect rather than "update-ref
refs/heads/branch"

Note that it is not straight forward to check the arguments to "reset"
and "merge" commands as they may be any revision, not just a refname and
we do not have an equivalent of check_refname_format() for revisions.

Helped-by: Derrick Stolee <[e-mail]>
Signed-off-by: Phillip Wood <[e-mail]>
---
    rebase -i: check labels and refs when parsing todo list
    
    Hopefully detecting these errors when the user edits the todo list will
    give a better experience.
    
    Changes from V2:
    
     * Fix some whitespace issues pointed out by Stolee.
    
    Thanks to Stolee for his comments on V1, I've updated the patch based on
    those, there are no functional changes but the code is hopefully
    clearer.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1482%2Fphillipwood%2Frebase-check-labels-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1482/phillipwood/rebase-check-labels-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1482

Range-diff vs v2:

 1:  2cc62fe5986 ! 1:  52a23db3cfe rebase -i: check labels and refs when parsing todo list
     @@ sequencer.c: static int is_command(enum todo_command command, const char **bol)
      +{
      +	switch (command) {
      +	case TODO_LABEL:
     -+	  /*
     -+	   * '#' is not a valid label as the merge command uses it to
     -+	   * separate merge parents from the commit subject.
     -+	   */
     -+	  if (!strcmp(arg, "#") ||
     ++		/*
     ++		 * '#' is not a valid label as the merge command uses it to
     ++		 * separate merge parents from the commit subject.
     ++		 */
     ++		if (!strcmp(arg, "#") ||
      +		    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
      +			return error(_("'%s' is not a valid label"), arg);
      +		break;


 sequencer.c                   | 39 ++++++++++++++++++++++++++++++++++-
 t/t3404-rebase-interactive.sh | 23 +++++++++++++++++++++
 2 files changed, 61 insertions(+), 1 deletion(-)


base-commit: b1485644f936ee83a995ec24d23f713f4230a1ae

Comments

Derrick Stolee Feb. 21, 2023, 5:07 p.m. UTC | #1

On 2/21/2023 12:05 PM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <[e-mail]>
>     Changes from V2:
>     
>      * Fix some whitespace issues pointed out by Stolee.

Thanks for the quick update. LGTM.

-Stolee

Patch

diff --git a/sequencer.c b/sequencer.c
index 3e4a1972897..ff83a55bb36 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2477,6 +2477,34 @@  static int is_command(enum todo_command command, const char **bol)
 		 (*bol = p));
 }
 
+static int check_label_or_ref_arg(enum todo_command command, const char *arg)
+{
+	switch (command) {
+	case TODO_LABEL:
+		/*
+		 * '#' is not a valid label as the merge command uses it to
+		 * separate merge parents from the commit subject.
+		 */
+		if (!strcmp(arg, "#") ||
+		    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
+			return error(_("'%s' is not a valid label"), arg);
+		break;
+
+	case TODO_UPDATE_REF:
+		if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
+			return error(_("'%s' is not a valid refname"), arg);
+		if (check_refname_format(arg, 0))
+			return error(_("update-ref requires a fully qualified "
+				       "refname e.g. refs/heads/%s"), arg);
+		break;
+
+	default:
+		BUG("unexpected todo_command");
+	}
+
+	return 0;
+}
+
 static int parse_insn_line(struct repository *r, struct todo_item *item,
 			   const char *buf, const char *bol, char *eol)
 {
@@ -2525,10 +2553,19 @@  static int parse_insn_line(struct repository *r, struct todo_item *item,
 
 	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
 	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
+		int ret = 0;
+
 		item->commit = NULL;
 		item->arg_offset = bol - buf;
 		item->arg_len = (int)(eol - bol);
-		return 0;
+		if (item->command == TODO_LABEL ||
+		    item->command == TODO_UPDATE_REF) {
+			saved = *eol;
+			*eol = '\0';
+			ret = check_label_or_ref_arg(item->command, bol);
+			*eol = saved;
+		}
+		return ret;
 	}
 
 	if (item->command == TODO_FIXUP) {
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 462cefd25df..efeb74ad50a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2072,6 +2072,7 @@  test_expect_success '--update-refs: --edit-todo with no update-ref lines' '
 '
 
 test_expect_success '--update-refs: check failed ref update' '
+	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -B update-refs-error no-conflict-branch &&
 	git branch -f base HEAD~4 &&
 	git branch -f first HEAD~3 &&
@@ -2123,6 +2124,28 @@  test_expect_success '--update-refs: check failed ref update' '
 	test_cmp expect err.trimmed
 '
 
+test_expect_success 'bad labels and refs rejected when parsing todo list' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	cat >todo <<-\EOF &&
+	exec >execed
+	label #
+	label :invalid
+	update-ref :bad
+	update-ref topic
+	EOF
+	rm -f execed &&
+	(
+		set_replace_editor todo &&
+		test_must_fail git rebase -i HEAD 2>err
+	) &&
+	grep "'\''#'\'' is not a valid label" err &&
+	grep "'\'':invalid'\'' is not a valid label" err &&
+	grep "'\'':bad'\'' is not a valid refname" err &&
+	grep "update-ref requires a fully qualified refname e.g. refs/heads/topic" \
+		err &&
+	test_path_is_missing execed
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged