Skip to content

Quieter sequencer status parsing #275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Quieter sequencer status parsing #275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5bede1
sequencer: always allow tab after command name
phillipwood Jun 23, 2019
c3885c0
sequencer: factor out todo command name parsing
phillipwood Jun 23, 2019
3e8fb61
status: do not report errors in sequencer/todo
phillipwood Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 21 additions & 22 deletions sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,
return todo_list->buf.buf + item->arg_offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
>  		}
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
>>  		}
>>
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &&

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &&

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
>  		}

}

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)
{
Expand All @@ -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 == todo_command_info[i].c) {
bol++;
if (is_command(i, &bol)) {
item->command = i;
break;
}
Expand Down Expand Up @@ -2170,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;
Expand Down
16 changes: 16 additions & 0 deletions t/t7512-status-help.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down