Quieter sequencer status parsing #275
Closed
Conversation
This file contains hidden or 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
The code that parses the todo list allows an unabbreviated command name to be followed by a space or a tab, but if the command name is abbreviated it only allows a space after it. Fix this inconsistency. Signed-off-by: Phillip Wood <[email protected]>
phillipwood
force-pushed
the
wip/quieter sequencer status parsing
branch
from
8429c4b to
af4b823
Compare
|
/submit |
|
Submitted as [email protected] |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list, | |||
| return todo_list->buf.buf + item->arg_offset; | |||
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Tue, 25 Jun 2019, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <[email protected]>
>
> commit 4a72486de9 ("fix cherry-pick/revert status after commit",
> 2019-04-16) used parse_insn_line() to parse the first line of the todo
> list to check if it was a pick or revert. However if the todo list is
> left over from an old cherry-pick or revert and references a commit that
> no longer exists then parse_insn_line() prints an error message which is
> confusing for users [1]. Instead parse just the command name so that the
> user is alerted to the presence of stale sequencer state by status
> reporting that a cherry-pick or revert is in progress.
>
> Note that we should not be leaving stale sequencer state lying around
> (or at least not as often) after commit b07d9bfd17 ("commit/reset: try
> to clean up sequencer state", 2019-04-16). However the user may still
> have stale state that predates that commit.
>
> Also avoid printing an error message if for some reason the user has a
> file called `sequencer` in $GIT_DIR.
>
> [1] https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@ww=
w.fastmail.com/
Very nice, I particularly like the simplicity of the code in `sequencer.c`
after this patch.
The entire patch series looks good to me.
Thanks,
Dscho
>
> Reported-by: Espen Antonsen <[email protected]>
> Signed-off-by: Phillip Wood <[email protected]>
> ---
> sequencer.c | 24 ++++++++----------------
> t/t7512-status-help.sh | 16 ++++++++++++++++
> 2 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 793f86bf9a..f8eab1697e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2177,34 +2177,26 @@ static int parse_insn_line(struct repository *r,=
struct todo_item *item,
>
> int sequencer_get_last_command(struct repository *r, enum replay_action=
*action)
> {
> - struct todo_item item;
> - char *eol;
> - const char *todo_file;
> + const char *todo_file, *bol;
> struct strbuf buf =3D STRBUF_INIT;
> - int ret =3D -1;
> + int ret =3D 0;
>
> todo_file =3D git_path_todo_file();
> if (strbuf_read_file(&buf, todo_file, 0) < 0) {
> - if (errno =3D=3D ENOENT)
> + if (errno =3D=3D ENOENT || errno =3D=3D ENOTDIR)
> return -1;
> else
> return error_errno("unable to open '%s'", todo_file);
> }
> - eol =3D strchrnul(buf.buf, '\n');
> - if (buf.buf !=3D eol && eol[-1] =3D=3D '\r')
> - eol--; /* strip Carriage Return */
> - if (parse_insn_line(r, &item, buf.buf, buf.buf, eol))
> - goto fail;
> - if (item.command =3D=3D TODO_PICK)
> + bol =3D buf.buf + strspn(buf.buf, " \t\r\n");
> + if (is_command(TODO_PICK, &bol) && (*bol =3D=3D ' ' || *bol =3D=3D '\t=
'))
> *action =3D REPLAY_PICK;
> - else if (item.command =3D=3D TODO_REVERT)
> + else if (is_command(TODO_REVERT, &bol) &&
> + (*bol =3D=3D ' ' || *bol =3D=3D '\t'))
> *action =3D REPLAY_REVERT;
> else
> - goto fail;
> -
> - ret =3D 0;
> + ret =3D -1;
>
> - fail:
> strbuf_release(&buf);
>
> return ret;
> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index c1eb72555d..3c9308709a 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -798,6 +798,22 @@ EOF
> test_i18ncmp expected actual
> '
>
> +test_expect_success 'status shows cherry-pick with invalid oid' '
> + mkdir .git/sequencer &&
> + test_write_lines "pick invalid-oid" >.git/sequencer/todo &&
> + git status --untracked-files=3Dno >actual 2>err &&
> + git cherry-pick --quit &&
> + test_must_be_empty err &&
> + test_i18ncmp expected actual
> +'
> +
> +test_expect_success 'status does not show error if .git/sequencer is a =
file' '
> + test_when_finished "rm .git/sequencer" &&
> + test_write_lines hello >.git/sequencer &&
> + git status --untracked-files=3Dno 2>err &&
> + test_must_be_empty err
> +'
> +
> test_expect_success 'status showing detached at and from a tag' '
> test_commit atag tagging &&
> git checkout atag &&
> --
> gitgitgadget
>
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
| @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list, | |||
| return todo_list->buf.buf + item->arg_offset; | |||
There was a problem hiding this comment.
On the Git mailing list, René Scharfe wrote (reply to this):
Am 25.06.19 um 12:11 schrieb Phillip Wood via GitGitGadget:
> From: Phillip Wood <[email protected]>
>
> Factor out the code that parses the name of the command at the start of
> each line in the todo file into it's own function so that it can be used
> in the next commit.
"Factor out" sounds like functionality is intended to not be changed...
>
> Signed-off-by: Phillip Wood <[email protected]>
> ---
> sequencer.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 919e3153f5..793f86bf9a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *t=
odo_list,
> return todo_list->buf.buf + item->arg_offset;
> }
>
> +static int is_command(enum todo_command command, const char **bol)
> +{
> + const char *str =3D todo_command_info[command].str;
> + const char nick =3D todo_command_info[command].c;
> + const char *p =3D *bol + 1;
> +
> + return skip_prefix(*bol, str, bol) ||
> + ((nick && **bol =3D=3D nick) &&
> + (*p =3D=3D ' ' || *p =3D=3D '\t' || *p =3D=3D '\n' || *p =3D=3D '\r'=
|| !*p) &&
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^=
^^
... but this adds support for LF, CR and NUL as separators after short
commands...
> + (*bol =3D p));
> +}
> +
> static int parse_insn_line(struct repository *r, struct todo_item *item=
,
> const char *buf, const char *bol, char *eol)
> {
> @@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, =
struct todo_item *item,
> }
>
> for (i =3D 0; i < TODO_COMMENT; i++)
> - if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
> - item->command =3D i;
> - break;
> - } else if ((bol + 1 =3D=3D eol || bol[1] =3D=3D ' ' || bol[1] =3D=3D =
'\t') &&
^^^^^^^^^^^^^^
... while this removes the check against the string's length.
Is this safe? It probably (hopefully?) is, as skip_prefix() requires bol
to point to a NUL-terminated string already.
But is_command() could do the old (and shorter) eol check just as well,
so the next question is: Why this change?
> - *bol =3D=3D todo_command_info[i].c) {
> - bol++;
> + if (is_command(i, &bol)) {
> item->command =3D i;
> break;
> }
>
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi René
On 25/06/2019 18:03, René Scharfe wrote:
> Am 25.06.19 um 12:11 schrieb Phillip Wood via GitGitGadget:
>> From: Phillip Wood <[email protected]>
>>
>> Factor out the code that parses the name of the command at the start of
>> each line in the todo file into it's own function so that it can be used
>> in the next commit.
>
> "Factor out" sounds like functionality is intended to not be changed...
Indeed, I thought I should have explained a bit more in the commit
message after I sent it. I think it is unchanged but it's not
immediately obvious.
>>
>> Signed-off-by: Phillip Wood <[email protected]>
>> ---
>> sequencer.c | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 919e3153f5..793f86bf9a 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,
>> return todo_list->buf.buf + item->arg_offset;
>> }
>>
>> +static int is_command(enum todo_command command, const char **bol)
>> +{
>> + const char *str = todo_command_info[command].str;
>> + const char nick = todo_command_info[command].c;
>> + const char *p = *bol + 1;
>> +
>> + return skip_prefix(*bol, str, bol) ||
>> + ((nick && **bol == nick) &&
>> + (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ... but this adds support for LF, CR and NUL as separators after short
> commands...
They're there to detect the end of the line, as I don't want to have to
pass in eol from other callers. The check that the nick name is not '\0'
means that if we pass in an empty string we do not read past the end as
the test will fail before it gets to the "*p == ..." part.
>
>> + (*bol = p));
>> +}
>> +
>> static int parse_insn_line(struct repository *r, struct todo_item *item,
>> const char *buf, const char *bol, char *eol)
>> {
>> @@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>> }
>>
>> for (i = 0; i < TODO_COMMENT; i++)
>> - if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
>> - item->command = i;
>> - break;
>> - } else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\t') &&
> ^^^^^^^^^^^^^^
> ... while this removes the check against the string's length.
>
> Is this safe? It probably (hopefully?) is, as skip_prefix() requires bol
> to point to a NUL-terminated string already.
Yes the string is NUL-terminated
> But is_command() could do the old (and shorter) eol check just as well,
> so the next question is: Why this change?
For the next patch so other callers don't have to find the end of the
line if they don't need it. The new checks for '\n' etc. replace the
'bol + 1 == eol' part.
I'll reroll with a better commit message
Best Wishes
Phillip
>
>> - *bol == todo_command_info[i].c) {
>> - bol++;
>> + if (is_command(i, &bol)) {
>> item->command = i;
>> break;
>> }
>>
>
| @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list, | |||
| return todo_list->buf.buf + item->arg_offset; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <[email protected]> writes:
> From: Phillip Wood <[email protected]>
>
> commit 4a72486de9 ("fix cherry-pick/revert status after commit",
> 2019-04-16) used parse_insn_line() to parse the first line of the todo
> list to check if it was a pick or revert. However if the todo list is
> left over from an old cherry-pick or revert and references a commit that
> no longer exists then parse_insn_line() prints an error message which is
> confusing for users [1]. Instead parse just the command name so that the
> user is alerted to the presence of stale sequencer state by status
> reporting that a cherry-pick or revert is in progress.
I have to wonder what would happen when "git cherry-pick --continue"
is given from such a stale state. It would have to fail as it would
not know the shape of the change to be replayed, no?
Is it easy to detect and say "there is an abandoned state file from
stale cherry-pick" and optionally offer to remove it (as we have no
chance of continuing anyway)?
Or is it likely that such an effort would end up being wasted, as...
> Note that we should not be leaving stale sequencer state lying around
> (or at least not as often) after commit b07d9bfd17 ("commit/reset: try
> to clean up sequencer state", 2019-04-16).
...this already happened?
> Also avoid printing an error message if for some reason the user has a
> file called `sequencer` in $GIT_DIR.
I agree that it is not a good place for giving diagnostics, but
getting ENOTDIR would be an indication that next time when the user
wants to do a rebase, range pick or range revert, it would not work
unless somebody removes that `sequencer` file, right? Are our users
sufficiently covered on that front (e.g. giving "we cannot do this
operation as $GIT_DIR/sequencer is in the way. please remove that
file" would be OK, but silently removing and possibly losing info we
didn't even look at may be bad)?
In any case, the "unable to open 'sequencer/todo'" you are removing
with this patch was *not* about helping the issue above which is an
unrelated tangent, so let me not be distracted by that ;-)
> +test_expect_success 'status shows cherry-pick with invalid oid' '
> + mkdir .git/sequencer &&
> + test_write_lines "pick invalid-oid" >.git/sequencer/todo &&
> + git status --untracked-files=no >actual 2>err &&
> + git cherry-pick --quit &&
> + test_must_be_empty err &&
> + test_i18ncmp expected actual
> +'
> +
> +test_expect_success 'status does not show error if .git/sequencer is a file' '
> + test_when_finished "rm .git/sequencer" &&
In short, I was wondering what happens if we lose this line, and
then we later had a command that needs to use .git/sequencer/todo or
something.
> + test_write_lines hello >.git/sequencer &&
> + git status --untracked-files=no 2>err &&
> + test_must_be_empty err
> +'
> +
> test_expect_success 'status showing detached at and from a tag' '
> test_commit atag tagging &&
> git checkout atag &&
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Junio
Thanks for the comments
On 25/06/2019 21:44, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[email protected]> writes:
>
>> From: Phillip Wood <[email protected]>
>>
>> commit 4a72486de9 ("fix cherry-pick/revert status after commit",
>> 2019-04-16) used parse_insn_line() to parse the first line of the todo
>> list to check if it was a pick or revert. However if the todo list is
>> left over from an old cherry-pick or revert and references a commit that
>> no longer exists then parse_insn_line() prints an error message which is
>> confusing for users [1]. Instead parse just the command name so that the
>> user is alerted to the presence of stale sequencer state by status
>> reporting that a cherry-pick or revert is in progress.
>
> I have to wonder what would happen when "git cherry-pick --continue"
> is given from such a stale state. It would have to fail as it would
> not know the shape of the change to be replayed, no?
Yes it would fail with
error: invalid line <line-no>: <line>
error: unusable instruction sheet: '.git/sequencer/todo'
fatal: cherry-pick failed
> Is it easy to detect and say "there is an abandoned state file from
> stale cherry-pick" and optionally offer to remove it (as we have no
> chance of continuing anyway)?
I guess we could have a specific error return if get_oid() failed and
suggest that then
> Or is it likely that such an effort would end up being wasted, as...
>
>> Note that we should not be leaving stale sequencer state lying around
>> (or at least not as often) after commit b07d9bfd17 ("commit/reset: try
>> to clean up sequencer state", 2019-04-16).
>
> ...this already happened?
Probably. It is still possible for the user to run checkout in the
middle of a cherry-pick and forget to finish it but if they're using a
prompt with git support it should tell them that a cherry-pick is in
progress as `git status` detects that it is.
>> Also avoid printing an error message if for some reason the user has a
>> file called `sequencer` in $GIT_DIR.
>
> I agree that it is not a good place for giving diagnostics, but
> getting ENOTDIR would be an indication that next time when the user
> wants to do a rebase, range pick or range revert, it would not work
> unless somebody removes that `sequencer` file, right? Are our users
> sufficiently covered on that front (e.g. giving "we cannot do this
> operation as $GIT_DIR/sequencer is in the way. please remove that
> file" would be OK, but silently removing and possibly losing info we
> didn't even look at may be bad)?
mkdir() will fail with ENOTDIR which should be reported with
error_errno() I think.
Best Wishes
Phillip
> In any case, the "unable to open 'sequencer/todo'" you are removing
> with this patch was *not* about helping the issue above which is an
> unrelated tangent, so let me not be distracted by that ;-)
>
>> +test_expect_success 'status shows cherry-pick with invalid oid' '
>> + mkdir .git/sequencer &&
>> + test_write_lines "pick invalid-oid" >.git/sequencer/todo &&
>> + git status --untracked-files=no >actual 2>err &&
>> + git cherry-pick --quit &&
>> + test_must_be_empty err &&
>> + test_i18ncmp expected actual
>> +'
>> +
>> +test_expect_success 'status does not show error if .git/sequencer is a file' '
>> + test_when_finished "rm .git/sequencer" &&
>
> In short, I was wondering what happens if we lose this line, and
> then we later had a command that needs to use .git/sequencer/todo or
> something.
>
>> + test_write_lines hello >.git/sequencer &&
>> + git status --untracked-files=no 2>err &&
>> + test_must_be_empty err
>> +'
>> +
>> test_expect_success 'status showing detached at and from a tag' '
>> test_commit atag tagging &&
>> git checkout atag &&
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 26/06/2019 09:57, Phillip Wood wrote:
> On 25/06/2019 21:44, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <[email protected]> writes:
>>
>>> From: Phillip Wood <[email protected]>
>>>
>>> commit 4a72486de9 ("fix cherry-pick/revert status after commit",
>>> 2019-04-16) used parse_insn_line() to parse the first line of the todo
>>> list to check if it was a pick or revert. However if the todo list is
>>> left over from an old cherry-pick or revert and references a commit that
>>> no longer exists then parse_insn_line() prints an error message which is
>>> confusing for users [1]. Instead parse just the command name so that the
>>> user is alerted to the presence of stale sequencer state by status
>>> reporting that a cherry-pick or revert is in progress.
>>
>> Or is it likely that such an effort would end up being wasted, as...
>>
>>> Note that we should not be leaving stale sequencer state lying around
>>> (or at least not as often) after commit b07d9bfd17 ("commit/reset: try
>>> to clean up sequencer state", 2019-04-16).
>>
>> ...this already happened?
>
> Probably. It is still possible for the user to run checkout in the
> middle of a cherry-pick and forget to finish it but if they're using a
> prompt with git support it should tell them that a cherry-pick is in
> progress as `git status` detects that it is.
Unfortunately it is not true that the prompt will see the in-progress
cherry-pick as it does not use git status - see
https://public-inbox.org/git/0f04fa2930d5cc7dfd2a5c5185573f7ecefa6055.1561990865.git.gitgitgadget@gmail.com/T/#u
for a fix
Best Wishes
Phillip
|
This branch is now known as |
|
This patch series was integrated into pu via git@bd05c02. |
phillipwood
force-pushed
the
wip/quieter sequencer status parsing
branch
from
af4b823 to
7db0e8e
Compare
phillipwood
changed the title
|
This patch series was integrated into pu via git@72fa5b9. |
|
This patch series was integrated into pu via git@d650402. |
Factor out the code that parses the name of the command at the start of each line in the todo file into its own function so that it can be used in the next commit. As I don't want to burden other callers with having to pass in a pointer to the end of the line the test for an abbreviated command is changed. This change should not affect the behavior. Instead of testing `eol == bol + 1` the new code checks for the end of the line by testing for '\n', '\r' or '\0' following the abbreviated name. To avoid reading past the end of an empty string it also checks that there is actually a single character abbreviation before testing if it matches. This prevents it from matching '\0' as the abbreviated name and then trying to read another character. Signed-off-by: Phillip Wood <[email protected]>
commit 4a72486 ("fix cherry-pick/revert status after commit", 2019-04-16) used parse_insn_line() to parse the first line of the todo list to check if it was a pick or revert. However if the todo list is left over from an old cherry-pick or revert and references a commit that no longer exists then parse_insn_line() prints an error message which is confusing for users [1]. Instead parse just the command name so that the user is alerted to the presence of stale sequencer state by status reporting that a cherry-pick or revert is in progress. Note that we should not be leaving stale sequencer state lying around (or at least not as often) after commit b07d9bf ("commit/reset: try to clean up sequencer state", 2019-04-16). However the user may still have stale state that predates that commit. Also avoid printing an error message if for some reason the user has a file called `sequencer` in $GIT_DIR. [1] https://public-inbox.org/git/[email protected]/ Reported-by: Espen Antonsen <[email protected]> Signed-off-by: Phillip Wood <[email protected]>
phillipwood
force-pushed
the
wip/quieter sequencer status parsing
branch
from
7db0e8e to
3e8fb61
Compare
|
/submit |
|
Submitted as [email protected] |
sequencer.c
Outdated
| @@ -2100,7 +2100,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, | |||
| if (skip_prefix(bol, todo_command_info[i].str, &bol)) { | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <[email protected]> writes:
> From: Phillip Wood <[email protected]>
>
> The code that parses the todo list allows an unabbreviated command name
> to be followed by a space or a tab, but if the command name is
> abbreviated it only allows a space after it. Fix this inconsistency.
Makes sense.
>
> Signed-off-by: Phillip Wood <[email protected]>
> ---
> sequencer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index f88a97fb10..919e3153f5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2100,7 +2100,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
> item->command = i;
> break;
> - } else if ((bol + 1 == eol || bol[1] == ' ') &&
> + } else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\t') &&
> *bol == todo_command_info[i].c) {
> bol++;
> item->command = i;
| @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list, | |||
| return todo_list->buf.buf + item->arg_offset; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <[email protected]> writes:
> As I don't want to burden other callers with having to pass in a pointer
> to the end of the line the test for an abbreviated command is
> changed.
A comma missing somewhere between "As <<REASON>>, <<CONSEQUENCE>>",
perhaps after "end of the line"?
> This change should not affect the behavior. Instead of testing
> `eol == bol + 1` the new code checks for the end of the line by testing
> for '\n', '\r' or '\0' following the abbreviated name. To avoid reading
> past the end of an empty string it also checks that there is actually a
> single character abbreviation before testing if it matches. This
> prevents it from matching '\0' as the abbreviated name and then trying
> to read another character.
>
> Signed-off-by: Phillip Wood <[email protected]>
> ---
> sequencer.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 919e3153f5..793f86bf9a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,
> return todo_list->buf.buf + item->arg_offset;
> }
>
> +static int is_command(enum todo_command command, const char **bol)
> +{
This is a tangent, but the reason why the caller of this function is
named parse_insn_line() (and not parse_command_line()) is because a
"command" often refers "rebase", "cherry-pick" etc., and we need a
word to differenciate these from what is to be done as an individual
step. Once the codebase stabilizes (read: I am excluding this kind
of change outside the scope of a series like this one), we'd need to
clean up the names in this file, I think.
> + const char *str = todo_command_info[command].str;
> + const char nick = todo_command_info[command].c;
> + const char *p = *bol + 1;
> +
> + return skip_prefix(*bol, str, bol) ||
> + ((nick && **bol == nick) &&
OK, making sure that nick is not NUL is the key to avoid stepping
past the NUL after the line that begins at *bol, as explained in the
additional paragraph in the proposed log message.
Looking good.
> + (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
> + (*bol = p));
> +}
> +
> static int parse_insn_line(struct repository *r, struct todo_item *item,
> const char *buf, const char *bol, char *eol)
> {
> @@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> }
>
> for (i = 0; i < TODO_COMMENT; i++)
> - if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
> - item->command = i;
> - break;
> - } else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\t') &&
> - *bol == todo_command_info[i].c) {
> - bol++;
> + if (is_command(i, &bol)) {
> item->command = i;
> break;
> }
|
This patch series was integrated into pu via git@d0c3b91. |
|
This patch series was integrated into pu via git@1783aca. |
|
This patch series was integrated into pu via git@f2edf80. |
|
This patch series was integrated into pu via git@2271da2. |
|
This patch series was integrated into pu via git@6b142f7. |
|
This patch series was integrated into pu via git@f872097. |
|
This patch series was integrated into pu via git@7a445cf. |
|
This patch series was integrated into next via git@273aee6. |
|
This patch series was integrated into pu via git@f616784. |
|
This patch series was integrated into pu via git@776924e. |
|
This patch series was integrated into pu via git@8759d81. |
|
This patch series was integrated into pu via git@1f16ac1. |
|
This patch series was integrated into pu via git@bd48ccf. |
|
This patch series was integrated into next via git@bd48ccf. |
|
This patch series was integrated into master via git@bd48ccf. |
|
Closed via bd48ccf. |
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.
Thanks for the comments on v1. I've updated the commit message of the second patch to try and explain why the new function is not a straight-forward copy of the old code
If we cannot parse the oid in the sequencer todo file do not show an error when running
git statusbut instead report that a cherry-pick or revert is in progress. This addresses a confusing error message reported in https://public-inbox.org/git/[email protected]/These patches are based on maint
Cc: Johannes Schindelin [email protected]
Cc: René Scharfe [email protected]