rebase -i: check labels and refs when parsing todo list #1482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1482.git.1676644675638.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 2/17/2023 9:37 AM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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.
> 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.
> +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
> + 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.
> + 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;
> @@ -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);
(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?
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 462cefd25df..2cf2d2b8a24 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -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.
> +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
> '
Again, the `git rebase --abort` seems like protection for
future tests, so a test_when_finished would help.
Thanks,
-Stolee |
phillipwood
force-pushed
the
rebase-check-labels
branch
from
5a31d88
to
2cc62fe
Compare
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Stolee
On 17/02/2023 15:32, Derrick Stolee wrote:
> On 2/17/2023 9:37 AM, Phillip Wood via GitGitGadget wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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.
> >> 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.
> >> +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)
> 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.
>> + 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.
> >> + 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;
That's definitely clearer, thanks for the suggestion
>> @@ -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);
> > (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?
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.
> 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?
I'll update as you suggest
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 462cefd25df..2cf2d2b8a24 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -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.
That's a good idea (and for the next test as well)
Thanks for you're thoughtful comments and suggestions
Best Wishes
Phillip
>> +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
>> '
> > Again, the `git rebase --abort` seems like protection for
> future tests, so a test_when_finished would help.
> > Thanks,
> -Stolee |
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 20/02/2023 09:24, Phillip Wood wrote:
> 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 |
|
/submit |
|
Submitted as pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): 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 |
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. Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
phillipwood
force-pushed
the
rebase-check-labels
branch
from
2cc62fe
to
52a23db
Compare
|
On the Git mailing list, Phillip Wood wrote (reply to this): 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 |
|
/submit |
|
Submitted as pull.1482.v3.git.1676999137195.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 2/21/2023 12:05 PM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> Changes from V2:
>
> * Fix some whitespace issues pointed out by Stolee.
Thanks for the quick update. LGTM.
-Stolee |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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.
|
|
This branch is now known as |
|
This patch series was integrated into seen via git@72c1ec2. |
|
This patch series was integrated into seen via git@a20c22c. |
|
This patch series was integrated into next via git@9b37d42. |
|
There was a status update in the "New Topics" section about the branch An invalid label or ref in the "rebase -i" todo file used to trigger an runtime error. SUch an error is now diagnosed while the todo file is parsed. Will merge to 'master'. source: <pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@f6d30c2. |
|
This patch series was integrated into seen via git@7b4c040. |
|
There was a status update in the "Cooking" section about the branch An invalid label or ref in the "rebase -i" todo file used to trigger an runtime error. SUch an error is now diagnosed while the todo file is parsed. Will merge to 'master'. source: <pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3de0794. |
|
This patch series was integrated into seen via git@37030a3. |
|
This patch series was integrated into seen via git@605915f. |
|
This patch series was integrated into seen via git@21522cf. |
|
This patch series was integrated into master via git@21522cf. |
|
This patch series was integrated into next via git@21522cf. |
|
Closed via 21522cf. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hopefully detecting these errors when the user edits the todo list will give a better experience.
Changes from V2:
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.
Cc: Derrick Stolee derrickstolee@github.com
Cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Phillip Wood phillip.wood123@gmail.com