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

PATCH Superseded Phillip Wood via GitGitGadget 4 participants 10 messages Feb 17, 2023 – Feb 21, 2023

lore

Share & tools

lore

Phillip Wood via GitGitGadget

author

2023-02-17 14:38 UTC #0 lore raw

PATCH 1/1 rebase -i: check labels and refs when parsing todo list

From: Phillip Wood <[email protected]>

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.

Signed-off-by Phillip Wood <[email protected]>

---
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.

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

 sequencer.c                   | 37 ++++++++++++++++++++++++++++++++++-
 t/t3404-rebase-interactive.sh | 25 ++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 2 deletions(-)

sequencer.c +36 -1

index 3e4a1972897..b4b5c3f331d 100644
@@ -2477,6 +2477,26 @@ 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)
+{
+	int allow_onelevel =
+		command == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;
+
+	if ((command == TODO_LABEL && !strcmp(arg, "#")) ||
+	    check_refname_format(arg, allow_onelevel)) {
+		if (command == TODO_LABEL)
+			error(_("'%s' is not a valid label"), arg);
+		else if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
+			error(_("'%s' is not a valid refname"), arg);
+		else
+			error(_("update-ref requires a fully qualified refname e.g. refs/heads/%s"),
+			      arg);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int parse_insn_line(struct repository *r, struct todo_item *item,
 			   const char *buf, const char *bol, char *eol)
 {

@@ -2523,8 +2543,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 		return error(_("missing arguments for %s"),
 			     command_to_string(item->command));
 
-	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
+	if (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);
+		if (item->command != TODO_RESET) {
+			saved = *eol;
+			*eol = '\0';
+			ret = check_label_or_ref_arg(item->command, bol);
+			*eol = saved;
+		}
+		return ret;
+	}
+
+	if (item->command == TODO_EXEC) {
 		item->commit = NULL;
 		item->arg_offset = bol - buf;
 		item->arg_len = (int)(eol - bol);

t/t3404-rebase-interactive.sh +24 -1

index 462cefd25df..2cf2d2b8a24 100755
@@ -2120,7 +2120,30 @@ test_expect_success '--update-refs: check failed ref update' '
 	tail -n 6 err >err.last &&
 	sed -e "s/Rebasing.*Successfully/Successfully/g" -e "s/^\t//g" \
 		<err.last >err.trimmed &&
-	test_cmp expect err.trimmed
+	test_cmp expect err.trimmed &&
+	git rebase --abort
+'
+
+test_expect_success 'bad labels and refs rejected when parsing todo list' '
+	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 &&
+	git rebase --abort
 '
 
 # This must be the last test in this file

base-commit: b1485644f936ee83a995ec24d23f713f4230a1ae

Signature · 1 line

gitgitgadget

Derrick Stolee

2023-02-17 15:32 UTC #1 lore raw

Quoted 7 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>
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.

Thanks for thinking about this user experience. This is a good improvement to the user interaction.

Quoted 4 lines

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"

I think it's good to start by adding the restriction in this check, but we could revisit this requirement in the future to see if it is worth allowing the user to drop "refs/heads/" and let it be implied. It just adds some complexity to the parsing, so this patch adds helpful scaffolding (in checks and tests) such that we could do that later in a safer way.

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2485

Quoted 6 lines

sequencer.c +6 -0

+static int check_label_or_ref_arg(enum todo_command command, const char *arg)
+{
+	int allow_onelevel =
+		command == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;
+
+	if ((command == TODO_LABEL && !strcmp(arg, "#")) ||

Interesting that "#" means something special for the label, and it's not limited to just the start of the label name, but must be the entire string. Is this not something that check_refname_format() would catch? Is the motivation that users might add what they think is a comment, such as:

label # make a label here

but oddly, this doesn't include something strange like

label #make a label here

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2488

Quoted 3 lines

sequencer.c +3 -0

+	    check_refname_format(arg, allow_onelevel)) {
+		if (command == TODO_LABEL)
+			error(_("'%s' is not a valid label"), arg);

If we have any kind of error and we are in TODO_LABEL, then we can use a label message. Good.

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2493

Quoted 5 lines

sequencer.c +5 -0

+		else if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
+			error(_("'%s' is not a valid refname"), arg);
+		else
+			error(_("update-ref requires a fully qualified refname e.g. refs/heads/%s"),
+			      arg);

This took me a little while to grok, but I think I have it
now: when in the update-ref mode, it could fail because of a one-level ref (the else case) or it could fail because the ref name uses forbidden characters (the else if case).

This nesting of conditions seems a bit fragile if we were to add a new todo_command to check here. Perhaps reorganize it to switch on the command?

	switch (command) {
	case TODO_LABEL:
		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);
		else if (check_refname_format(arg, 0))
			return error(_("update-ref reqruies a fully qualified refname (e.g. refs/heads/%s)",
				     arg);
		break;

	default:
		BUG("unexpected todo_command");
	}

	return 0;

Quoted 15 lines

sequencer.c +9 -1

@@ -2523,8 +2543,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 		return error(_("missing arguments for %s"),
 			     command_to_string(item->command));
 
-	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
+	if (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);
+		if (item->command != TODO_RESET) {
+			saved = *eol;
+			*eol = '\0';

sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2565

Quoted 10 lines

sequencer.c +7 -0

+			ret = check_label_or_ref_arg(item->command, bol);
+			*eol = saved;
+		}
+		return ret;
+	}
+
+	if (item->command == TODO_EXEC) {
 		item->commit = NULL;
 		item->arg_offset = bol - buf;
 		item->arg_len = (int)(eol - bol);

(What's missing from this context is "return 0;")

Is there an important reason why you separated TODO_EXEC and its identical item->arg_(offset|len) parsing into its own block? It seems like we could modify your change to look like this:

	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);
		if (item->command == TODO_RESET ||
		    item->command == TODO_UPDATE_REF) {
			saved = *eol;
			*eol = '\0';
			ret = check_label_or_ref_arg(item->command, bol);
			*eol = saved;
		}
		return ret;
	}

and the diff will have fewer new lines as well as fewer duplicate lines in the post-image. Am I missing something about TODO_EXEC being special?

t/t3404-rebase-interactive.sh test_expect_success '--update-refs: check failed ref update' ' · line 2125

Quoted 12 lines

t/t3404-rebase-interactive.sh +3 -1

index 462cefd25df..2cf2d2b8a24 100755
@@ -2120,7 +2120,30 @@ test_expect_success '--update-refs: check failed ref update' '
 	tail -n 6 err >err.last &&
 	sed -e "s/Rebasing.*Successfully/Successfully/g" -e "s/^\t//g" \
 		<err.last >err.trimmed &&
-	test_cmp expect err.trimmed
+	test_cmp expect err.trimmed &&
+	git rebase --abort
+'

Perhaps this git rebase --abort should be part of a test_when_finished test_may_fail git rebase --abort at the start of the test so that your new test can succeed even if an earlier test step caused the test to fail.

Quoted 11 lines

t/t3404-rebase-interactive.sh +11 -0

+test_expect_success 'bad labels and refs rejected when parsing todo list' '
+	cat >todo <<-\EOF &&
+	exec >execed
+	label #
+	label :invalid
+	update-ref :bad
+	update-ref topic
+	EOF
+	rm -f execed &&
+	(
+		set_replace_editor todo &&

t/t3404-rebase-interactive.sh test_expect_success '--update-refs: check failed ref update' ' · line 2147

Quoted 10 lines

t/t3404-rebase-interactive.sh +9 -0

+		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 &&
+	git rebase --abort
 '

Again, the git rebase --abort seems like protection for future tests, so a test_when_finished would help.

Thanks,
-Stolee

Phillip Wood

2023-02-20 09:25 UTC #2 lore raw

Hi Stolee

Quoted 24 lines · Derrick Stolee

Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
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.
Thanks for thinking about this user experience. This is a good improvement to the user interaction.
Quoted 4 lines
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"
I think it's good to start by adding the restriction in this check, but we could revisit this requirement in the future to see if it is worth allowing the user to drop "refs/heads/" and let it be implied. It just adds some complexity to the parsing, so this patch adds helpful scaffolding (in checks and tests) such that we could do that later in a safer way.

Quoted 11 lines

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2485
Quoted 6 lines
sequencer.c +6 -0
+static int check_label_or_ref_arg(enum todo_command command, const char *arg)
+{
+	int allow_onelevel =
+		command == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;
+
+	if ((command == TODO_LABEL && !strcmp(arg, "#")) ||
Interesting that "#" means something special for the label, and it's not limited to just the start of the label name, but must be the entire string. Is this not something that check_refname_format() would catch?

"#" is a valid refname, but the merge command uses it to separate the merge parents from the commit summary so it cannot be used as a label. It is rejected at the start of do_label() (arguably that check could be removed with this series but I'm tempted to leave it alone)

Quoted 8 lines

Is the motivation that users
might add what they think is a comment, such as:
label # make a label here
but oddly, this doesn't include something strange like
label #make a label here

I've actually got some patches based on this that add support for comments like this, but they are not the reason that "#" is forbidden as a label here.

Quoted 34 lines

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2488
Quoted 3 lines
sequencer.c +3 -0
+	    check_refname_format(arg, allow_onelevel)) {
+		if (command == TODO_LABEL)
+			error(_("'%s' is not a valid label"), arg);
If we have any kind of error and we are in TODO_LABEL, then we can use a label message. Good.
sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2493
Quoted 5 lines
sequencer.c +5 -0
+		else if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
+			error(_("'%s' is not a valid refname"), arg);
+		else
+			error(_("update-ref requires a fully qualified refname e.g. refs/heads/%s"),
+			      arg);
This took me a little while to grok, but I think I have it
now: when in the update-ref mode, it could fail because of a one-level ref (the else case) or it could fail because the ref name uses forbidden characters (the else if case).
This nesting of conditions seems a bit fragile if we were to add a new todo_command to check here. Perhaps reorganize it to switch on the command?
	switch (command) {
	case TODO_LABEL:
		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);
		else if (check_refname_format(arg, 0))
			return error(_("update-ref reqruies a fully qualified refname (e.g. refs/heads/%s)",

Quoted 8 lines

				     arg);
		break;

	default:
		BUG("unexpected todo_command");
	}

	return 0;

That's definitely clearer, thanks for the suggestion

Quoted 28 lines

Quoted 15 lines

sequencer.c +9 -1

@@ -2523,8 +2543,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
  		return error(_("missing arguments for %s"),
  			     command_to_string(item->command));
 
-	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
+	if (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);
+		if (item->command != TODO_RESET) {
+			saved = *eol;
+			*eol = '\0';

sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2565

Quoted 10 lines

sequencer.c +7 -0

+			ret = check_label_or_ref_arg(item->command, bol);
+			*eol = saved;
+		}
+		return ret;
+	}
+
+	if (item->command == TODO_EXEC) {
  		item->commit = NULL;
  		item->arg_offset = bol - buf;
  		item->arg_len = (int)(eol - bol);

(What's missing from this context is "return 0;")

Quoted 3 lines

Is there an important reason why you separated TODO_EXEC and its identical item->arg_(offset|len) parsing into its own block?

In a word no! This patch was pulled out from the series I mentioned above that adds support for comments. There it makes sense to treat exec commands separately as they cannot have comments but you're right that there is no need to do that in this patch.

Quoted 21 lines

It seems like we could modify your change to look like this:

	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);
		if (item->command == TODO_RESET ||
		    item->command == TODO_UPDATE_REF) {
			saved = *eol;
			*eol = '\0';
			ret = check_label_or_ref_arg(item->command, bol);
			*eol = saved;
		}
		return ret;
	}

Quoted 3 lines

and the diff will have fewer new lines as well as fewer duplicate lines in the post-image. Am I missing something about TODO_EXEC being special?

I'll update as you suggest

Quoted 9 lines

t/t3404-rebase-interactive.sh +0 -1

index 462cefd25df..2cf2d2b8a24 100755
@@ -2120,7 +2120,30 @@ test_expect_success '--update-refs: check failed ref update' '
  	tail -n 6 err >err.last &&
  	sed -e "s/Rebasing.*Successfully/Successfully/g" -e "s/^\t//g" \
  		<err.last >err.trimmed &&
-	test_cmp expect err.trimmed

Quoted 8 lines

t/t3404-rebase-interactive.sh test_expect_success '--update-refs: check failed ref update' ' · line 2125
Quoted 3 lines
t/t3404-rebase-interactive.sh +3 -0
+	test_cmp expect err.trimmed &&
+	git rebase --abort
+'
Perhaps this git rebase --abort should be part of a test_when_finished test_may_fail git rebase --abort at the start of the test so that your new test can succeed even if an earlier test step caused the test to fail.

That's a good idea (and for the next test as well)

Thanks for you're thoughtful comments and suggestions

Best Wishes

Phillip

Quoted 27 lines

Quoted 11 lines
t/t3404-rebase-interactive.sh +11 -0
+test_expect_success 'bad labels and refs rejected when parsing todo list' '
+	cat >todo <<-\EOF &&
+	exec >execed
+	label #
+	label :invalid
+	update-ref :bad
+	update-ref topic
+	EOF
+	rm -f execed &&
+	(
+		set_replace_editor todo &&
t/t3404-rebase-interactive.sh test_expect_success '--update-refs: check failed ref update' ' · line 2147
Quoted 10 lines
t/t3404-rebase-interactive.sh +9 -0
+		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 &&
+	git rebase --abort
  '
Again, the git rebase --abort seems like protection for future tests, so a test_when_finished would help.
Thanks,
-Stolee

Phillip Wood

2023-02-20 10:04 UTC #3 lore raw

Quoted 1 line · Phillip Wood

Thanks for you're thoughtful comments and suggestions

Hmm, perhaps I'm not quite as awake as I thought I was. I meant "your thoughtful comments" of course.

Best Wishes

Phillip

Phillip Wood via GitGitGadget

author

2023-02-20 14:19 UTC #4 lore raw

PATCH v2 1/1 rebase -i: check labels and refs when parsing todo list

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Helped-by Derrick Stolee <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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.

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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1482/phillipwood/rebase-check-labels-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1482

Range-diff vs v1:

1: 5a31d880064 ! 1: 2cc62fe5986 rebase -i: check labels and refs when parsing todo list

     @@ Commit message
          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 <[email protected]>
          Signed-off-by: Phillip Wood <[email protected]>
      
       ## sequencer.c ##
     @@ sequencer.c: static int is_command(enum todo_command command, const char **bol)
       
      +static int check_label_or_ref_arg(enum todo_command command, const char *arg)
      +{
     -+	int allow_onelevel =
     -+		command == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;
     ++	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;
      +
     -+	if ((command == TODO_LABEL && !strcmp(arg, "#")) ||
     -+	    check_refname_format(arg, allow_onelevel)) {
     -+		if (command == TODO_LABEL)
     -+			error(_("'%s' is not a valid label"), arg);
     -+		else if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
     -+			error(_("'%s' is not a valid refname"), arg);
     -+		else
     -+			error(_("update-ref requires a fully qualified refname e.g. refs/heads/%s"),
     -+			      arg);
     -+		return -1;
     ++	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;
     @@ sequencer.c: static int is_command(enum todo_command command, const char **bol)
       			   const char *buf, const char *bol, char *eol)
       {
      @@ sequencer.c: static int parse_insn_line(struct repository *r, struct todo_item *item,
     - 		return error(_("missing arguments for %s"),
     - 			     command_to_string(item->command));
       
     --	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
     -+	if (item->command == TODO_LABEL ||
     + 	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);
     -+		if (item->command != TODO_RESET) {
     + 		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_EXEC) {
     - 		item->commit = NULL;
     - 		item->arg_offset = bol - buf;
     - 		item->arg_len = (int)(eol - bol);
     + 	}
     + 
     + 	if (item->command == TODO_FIXUP) {
      
       ## t/t3404-rebase-interactive.sh ##
     +@@ t/t3404-rebase-interactive.sh: 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 &&
      @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs: check failed ref update' '
     - 	tail -n 6 err >err.last &&
     - 	sed -e "s/Rebasing.*Successfully/Successfully/g" -e "s/^\t//g" \
     - 		<err.last >err.trimmed &&
     --	test_cmp expect err.trimmed
     -+	test_cmp expect err.trimmed &&
     -+	git rebase --abort
     -+'
     -+
     + 	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 #
     @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs: check failed
      +	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 &&
     -+	git rebase --abort
     - '
     - 
     ++	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

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

sequencer.c +38 -1

index 3e4a1972897..33bdc8bca43 100644
@@ -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) {

t/t3404-rebase-interactive.sh +23 -0

index 462cefd25df..efeb74ad50a 100755
@@ -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

base-commit: b1485644f936ee83a995ec24d23f713f4230a1ae

Signature · 1 line

gitgitgadget

Derrick Stolee

2023-02-21 14:25 UTC #5 lore raw

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2489

Quoted 12 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

sequencer.c +10 -0

+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))

Tabbing is strange here. Within the case there seems to be "\t " to the left of each line. Then the conditional has strange spacing. I think it should be:

		if (!strcmp(arg, "#") ||
		    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))

(The "check_refname_format()" line is correct in your patch, but the lines above it are not, for some reason.)

The rest of the switch statement is correctly tabbed.

Quoted 9 lines

sequencer.c +2 -0

@@ -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);

sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2569

Quoted 10 lines

sequencer.c +8 -1

-		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;
 	}

This diff is much simpler to understand for the purpose of this patch. I saw your comment about splitting out TODO_EXEC for a future change, and that would be fine when it happens, too.

Thanks for the updates. Outside of that strange whitespace issue, this patch LGTM.

Thanks,
-Stolee

Phillip Wood

2023-02-21 16:30 UTC #6 lore raw

Quoted 18 lines · Derrick Stolee

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2489
Quoted 12 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
sequencer.c +10 -0
+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))
Tabbing is strange here. Within the case there seems to be "\t " to the left of each line. Then the conditional has strange spacing. I think it should be:

Quoted 5 lines

		if (!strcmp(arg, "#") ||
		    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
(The "check_refname_format()" line is correct in your patch, but the lines above it are not, for some reason.)

Yes you're right, I'm not sure what happened there. I'll re-roll

Thanks

Phillip

Quoted 31 lines

The rest of the switch statement is correctly tabbed.
Quoted 9 lines
sequencer.c +2 -0
@@ -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);
sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2569
Quoted 10 lines
sequencer.c +8 -1
-		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;
  	}
This diff is much simpler to understand for the purpose of this patch. I saw your comment about splitting out TODO_EXEC for a future change, and that would be fine when it happens, too.
Thanks for the updates. Outside of that strange whitespace issue, this patch LGTM.
Thanks,
-Stolee

Junio C Hamano

2023-02-21 17:27 UTC #7 lore raw

Quoted 4 lines · Phillip Wood

Quoted 2 lines
(The "check_refname_format()" line is correct in your patch, but the lines above it are not, for some reason.)
Yes you're right, I'm not sure what happened there. I'll re-roll

Nah, thanks, both. I've applied with reformatting plus Derrick's Acked-by.

Phillip Wood via GitGitGadget

author

2023-02-21 17:05 UTC #8 lore raw

PATCH v3 1/1 rebase -i: check labels and refs when parsing todo list

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Helped-by Derrick Stolee <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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(-)

sequencer.c +38 -1

index 3e4a1972897..ff83a55bb36 100644
@@ -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) {

t/t3404-rebase-interactive.sh +23 -0

index 462cefd25df..efeb74ad50a 100755
@@ -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

base-commit: b1485644f936ee83a995ec24d23f713f4230a1ae

Signature · 1 line

gitgitgadget

Derrick Stolee

2023-02-21 17:07 UTC #9 lore raw

Quoted 4 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>
Changes from V2:
* Fix some whitespace issues pointed out by Stolee.

Thanks for the quick update. LGTM.

-Stolee