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

[Derrick Stolee <derrickstolee@github.com>]

On 2/20/2023 9:19 AM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

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

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