| Message ID | pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] rebase -i: check labels and refs when parsing todo list | expand |
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
On 21/02/2023 14:24, Derrick Stolee wrote: > 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.) Yes you're right, I'm not sure what happened there. I'll re-roll Thanks Phillip > 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
Phillip Wood <phillip.wood123@gmail.com> writes: >> (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.
diff --git a/sequencer.c b/sequencer.c index 3e4a1972897..33bdc8bca43 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