Wip/quieter sequencer status parsing

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/[email protected]/

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(-)

sequencer.c +8 -16

index 793f86bf9a..f8eab1697e 100644
@@ -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;

t/t7512-status-help.sh +16 -1

index c1eb72555d..3c9308709a 100755
@@ -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

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/[email protected]/

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

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.

Signed-off-by Phillip Wood <[email protected]>

---
 sequencer.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

sequencer.c +0 -0

index 919e3153f5..793f86bf9a 100644
@@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,
 	return todo_list->buf.buf + item->arg_offset;
 }

sequencer.c +3 -0

+		 (*bol = p));
+}
+
 static int parse_insn_line(struct repository *r, struct todo_item *item,
 			   const char *buf, const char *bol, char *eol)
 {

sequencer.c +1 -2

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

Am 25.06.19 um 12:11 schrieb Phillip Wood via GitGitGadget:

Quoted 5 lines

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

Quoted 13 lines

Signed-off-by Phillip Wood <[email protected]>

---
 sequencer.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

sequencer.c +0 -0

index 919e3153f5..793f86bf9a 100644
@@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,
 	return todo_list->buf.buf + item->arg_offset;
 }

sequencer.c const char *todo_item_get_arg(struct todo_list *todo_list, · line 2087

Quoted 10 lines

sequencer.c +9 -0

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

Quoted 8 lines

sequencer.c +3 -0

+		 (*bol = p));
+}
+
 static int parse_insn_line(struct repository *r, struct todo_item *item,
 			   const char *buf, const char *bol, char *eol)
 {

sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2103

Quoted 7 lines

sequencer.c +0 -4

@@ -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?

Quoted 8 lines

sequencer.c +1 -2

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

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).

Also avoid printing an error message if for some reason the user has a file called sequencer in $GIT_DIR.

t/t7512-status-help.sh +5 -0

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

Quoted 10 lines · Phillip Wood via GitGitGadget

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

Quoted 3 lines

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?

Quoted 2 lines

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

t/t7512-status-help.sh EOF · line 811

Quoted 11 lines

t/t7512-status-help.sh +11 -0

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

Quoted 8 lines

t/t7512-status-help.sh +5 -0

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

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.

Signed-off-by Phillip Wood <[email protected]>

---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sequencer.c +1 -1

index f88a97fb10..919e3153f5 100644
@@ -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;

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

---
 sequencer.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

sequencer.c +0 -0

index 919e3153f5..793f86bf9a 100644

sequencer.c +5 -6

+		 (*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;
 		}

Quoted 15 lines · Junio C Hamano

Quoted 10 lines · Phillip Wood via GitGitGadget

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

Quoted 5 lines

Quoted 3 lines

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.