Re: [PATCH v2] rebase -i: check labels and refs when parsing todo list
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Feb 21, 2023, 14:24 UTC
- Message-ID
- <f99d7043-7757-2e19-d1e1-4ee9b887af1f@github.com>
- In-Reply-To
- <pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com>
On 2/20/2023 9:19 AM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
Show 10 quoted lines
> +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.
Show 19 quoted lines
> @@ -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