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

On 2/17/2023 9:37 AM, Phillip Wood via GitGitGadget wrote:

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

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.

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

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

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;

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

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.

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

Thanks, -Stolee