[PATCH 0/3] Wip/quieter sequencer status parsing

[PATCH 0/3] Wip/quieter sequencer status parsing

[Phillip Wood via GitGitGadget]

If we cannot parse the oid in the sequencer todo file do not show an error
when running git status but instead report that a cherry-pick or revert is
in progress. This addresses a confusing error message reported in 
https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/

These patches are based on maint

Phillip Wood (3):
  sequencer: always allow tab after command name
  sequencer: factor out todo command name parsing
  status: do not report errors in sequencer/todo

 sequencer.c            | 43 +++++++++++++++++++++---------------------
 t/t7512-status-help.sh | 16 ++++++++++++++++
 2 files changed, 37 insertions(+), 22 deletions(-)


base-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-275%2Fphillipwood%2Fwip%2Fquieter%C2%A0sequencer%C2%A0status%C2%A0parsing-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-275/phillipwood/wip/quieter sequencer status parsing-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/275
-- 
gitgitgadget

[PATCH 1/3] sequencer: always allow tab after command name

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 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;
-- 
gitgitgadget

[PATCH 2/3] sequencer: factor out todo command name parsing

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 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) &&
+		 (*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;
 		}
-- 
gitgitgadget

[PATCH 3/3] status: do not report errors in sequencer/todo

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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@www.fastmail.com/

Reported-by: Espen Antonsen <espen@inspired.no>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 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 = STRBUF_INIT;
-	int ret = -1;
+	int ret = 0;
 
 	todo_file = git_path_todo_file();
 	if (strbuf_read_file(&buf, todo_file, 0) < 0) {
-		if (errno == ENOENT)
+		if (errno == ENOENT || errno == ENOTDIR)
 			return -1;
 		else
 			return error_errno("unable to open '%s'", todo_file);
 	}
-	eol = strchrnul(buf.buf, '\n');
-	if (buf.buf != eol && eol[-1] == '\r')
-		eol--; /* strip Carriage Return */
-	if (parse_insn_line(r, &item, buf.buf, buf.buf, eol))
-		goto fail;
-	if (item.command == TODO_PICK)
+	bol = buf.buf + strspn(buf.buf, " \t\r\n");
+	if (is_command(TODO_PICK, &bol) && (*bol == ' ' || *bol == '\t'))
 		*action = REPLAY_PICK;
-	else if (item.command == TODO_REVERT)
+	else if (is_command(TODO_REVERT, &bol) &&
+		 (*bol == ' ' || *bol == '\t'))
 		*action = REPLAY_REVERT;
 	else
-		goto fail;
-
-	ret = 0;
+		ret = -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=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" &&
+	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 &&
-- 
gitgitgadget

Re: [PATCH 3/3] status: do not report errors in sequencer/todo

[Johannes Schindelin]

Hi Phillip,

On Tue, 25 Jun 2019, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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@www.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 <espen@inspired.no>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  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 = STRBUF_INIT;
> -	int ret = -1;
> +	int ret = 0;
>
>  	todo_file = git_path_todo_file();
>  	if (strbuf_read_file(&buf, todo_file, 0) < 0) {
> -		if (errno == ENOENT)
> +		if (errno == ENOENT || errno == ENOTDIR)
>  			return -1;
>  		else
>  			return error_errno("unable to open '%s'", todo_file);
>  	}
> -	eol = strchrnul(buf.buf, '\n');
> -	if (buf.buf != eol && eol[-1] == '\r')
> -		eol--; /* strip Carriage Return */
> -	if (parse_insn_line(r, &item, buf.buf, buf.buf, eol))
> -		goto fail;
> -	if (item.command == TODO_PICK)
> +	bol = buf.buf + strspn(buf.buf, " \t\r\n");
> +	if (is_command(TODO_PICK, &bol) && (*bol == ' ' || *bol == '\t'))
>  		*action = REPLAY_PICK;
> -	else if (item.command == TODO_REVERT)
> +	else if (is_command(TODO_REVERT, &bol) &&
> +		 (*bol == ' ' || *bol == '\t'))
>  		*action = REPLAY_REVERT;
>  	else
> -		goto fail;
> -
> -	ret = 0;
> +		ret = -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=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" &&
> +	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 &&
> --
> gitgitgadget
>

Re: [PATCH 0/3] Wip/quieter sequencer status parsing

[Phillip Wood]

Please ignore the 'Wip' in the title, I forget to edit it on gitgitgadget

Best Wishes

Phillip

On 25/06/2019 11:11, Phillip Wood via GitGitGadget wrote:
> If we cannot parse the oid in the sequencer todo file do not show an error
> when running git status but instead report that a cherry-pick or revert is
> in progress. This addresses a confusing error message reported in
> https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/
> 
> These patches are based on maint
> 
> Phillip Wood (3):
>    sequencer: always allow tab after command name
>    sequencer: factor out todo command name parsing
>    status: do not report errors in sequencer/todo
> 
>   sequencer.c            | 43 +++++++++++++++++++++---------------------
>   t/t7512-status-help.sh | 16 ++++++++++++++++
>   2 files changed, 37 insertions(+), 22 deletions(-)
> 
> 
> base-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-275%2Fphillipwood%2Fwip%2Fquieter%C2%A0sequencer%C2%A0status%C2%A0parsing-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-275/phillipwood/wip/quieter sequencer status parsing-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/275
> 

Re: [PATCH 2/3] sequencer: factor out todo command name parsing

[René Scharfe]

Am 25.06.19 um 12:11 schrieb Phillip Wood via GitGitGadget:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> ---
>  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...

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

But is_command() could do the old (and shorter) eol check just as well,
so the next question is: Why this change?

> -			   *bol == todo_command_info[i].c) {
> -			bol++;
> +		if (is_command(i, &bol)) {
>  			item->command = i;
>  			break;
>  		}
>

Re: [PATCH 2/3] sequencer: factor out todo command name parsing

[Phillip Wood]

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 <phillip.wood@dunelm.org.uk>
>>
>> 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 <phillip.wood@dunelm.org.uk>
>> ---
>>  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;
>>  		}
>>
> 

Re: [PATCH 3/3] status: do not report errors in sequencer/todo

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 &&

Re: [PATCH 3/3] status: do not report errors in sequencer/todo

[Phillip Wood]

Hi Junio

Thanks for the comments

On 25/06/2019 21:44, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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 &&

[PATCH v2 0/3] Quieter sequencer status parsing

[Phillip Wood via GitGitGadget]

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 status but instead report that a cherry-pick or revert is
in progress. This addresses a confusing error message reported in 
https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/

These patches are based on maint

Cc: Johannes Schindelin Johannes.Schindelin@gmx.de
[Johannes.Schindelin@gmx.de]

Phillip Wood (3):
  sequencer: always allow tab after command name
  sequencer: factor out todo command name parsing
  status: do not report errors in sequencer/todo

 sequencer.c            | 43 +++++++++++++++++++++---------------------
 t/t7512-status-help.sh | 16 ++++++++++++++++
 2 files changed, 37 insertions(+), 22 deletions(-)


base-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-275%2Fphillipwood%2Fwip%2Fquieter%C2%A0sequencer%C2%A0status%C2%A0parsing-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-275/phillipwood/wip/quieter sequencer status parsing-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/275

Range-diff vs v1:

 1:  a5bede1cf9 = 1:  a5bede1cf9 sequencer: always allow tab after command name
 2:  ebe8a2b3a1 ! 2:  c3885c0b8f sequencer: factor out todo command name parsing
     @@ -3,9 +3,19 @@
          sequencer: factor out todo command name parsing
      
          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
     +    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 <phillip.wood@dunelm.org.uk>
      
       diff --git a/sequencer.c b/sequencer.c
 3:  af4b823caa = 3:  3e8fb61fb8 status: do not report errors in sequencer/todo

-- 
gitgitgadget

[PATCH v2 1/3] sequencer: always allow tab after command name

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 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;
-- 
gitgitgadget

[PATCH v2 2/3] sequencer: factor out todo command name parsing

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 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) &&
+		 (*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;
 		}
-- 
gitgitgadget

[PATCH v2 3/3] status: do not report errors in sequencer/todo

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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@www.fastmail.com/

Reported-by: Espen Antonsen <espen@inspired.no>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 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 = STRBUF_INIT;
-	int ret = -1;
+	int ret = 0;
 
 	todo_file = git_path_todo_file();
 	if (strbuf_read_file(&buf, todo_file, 0) < 0) {
-		if (errno == ENOENT)
+		if (errno == ENOENT || errno == ENOTDIR)
 			return -1;
 		else
 			return error_errno("unable to open '%s'", todo_file);
 	}
-	eol = strchrnul(buf.buf, '\n');
-	if (buf.buf != eol && eol[-1] == '\r')
-		eol--; /* strip Carriage Return */
-	if (parse_insn_line(r, &item, buf.buf, buf.buf, eol))
-		goto fail;
-	if (item.command == TODO_PICK)
+	bol = buf.buf + strspn(buf.buf, " \t\r\n");
+	if (is_command(TODO_PICK, &bol) && (*bol == ' ' || *bol == '\t'))
 		*action = REPLAY_PICK;
-	else if (item.command == TODO_REVERT)
+	else if (is_command(TODO_REVERT, &bol) &&
+		 (*bol == ' ' || *bol == '\t'))
 		*action = REPLAY_REVERT;
 	else
-		goto fail;
-
-	ret = 0;
+		ret = -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=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" &&
+	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 &&
-- 
gitgitgadget

Re: [PATCH v2 1/3] sequencer: always allow tab after command name

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> ---
>  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;

Re: [PATCH v2 2/3] sequencer: factor out todo command name parsing

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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 <phillip.wood@dunelm.org.uk>
> ---
>  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;
>  		}

Re: [PATCH 3/3] status: do not report errors in sequencer/todo

[Phillip Wood]

On 26/06/2019 09:57, Phillip Wood wrote:
> On 25/06/2019 21:44, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> 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