[PATCH 0/2] status: improve rebase todo list parsing

[PATCH 0/2] status: improve rebase todo list parsing

[Phillip Wood]

When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this is tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to.

This series fixes that. The first patch factors out the sequencer
code that parses the command names in the todo list. The second patch
uses that function in "git status" to parse the command names so that
it knows whether the line may contain "-C" and whether there is an
object id that should be abbreviated.

Base-Commit: 8c9303b1ffae5b745d1b0a1f98330cf7944d8db0
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fimprove-status-todo-list-parsing%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/8c9303b1f...d20dc1f65
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/improve-status-todo-list-parsing/v1


Phillip Wood (2):
  sequencer: factor out parsing of todo commands
  status: improve rebase todo list parsing

 sequencer.c            |  45 ++++++++++-----
 sequencer.h            |   1 +
 t/t7512-status-help.sh |  74 ++++++++++++++++---------
 wt-status.c            | 121 ++++++++++++++++++++++++++++++++---------
 4 files changed, 174 insertions(+), 67 deletions(-)

-- 
2.54.0.rc1.174.gd833f386ac5.dirty

[PATCH 1/2] sequencer: factor out parsing of todo commands

[Phillip Wood]

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

Move the code that parses todo commands into a separate function so that
it can be shared with "git status" in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 45 ++++++++++++++++++++++++++++++---------------
 sequencer.h |  1 +
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b7d8dca47f..b8e860434a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2625,6 +2625,27 @@ static int is_command(enum todo_command command, const char **bol)
 		return 1;
 	}
 	return 0;
+}
+
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
+{
+	const char *s = *p;
+
+	for (int i = 0; i < TODO_COMMENT; i++)
+		if (is_command(i, p)) {
+			*cmd = i;
+			return true;
+		}
+
+	if (starts_with(s, comment_line_str)) {
+		*cmd = TODO_COMMENT;
+		return true;
+	} else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
+		*cmd = TODO_COMMENT;
+		return true;
+	}
+
+	return false;
 }
 
 static int check_label_or_ref_arg(enum todo_command command, const char *arg)
@@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
 {
 	struct object_id commit_oid;
 	char *end_of_object_name;
-	int i, saved, status, padding;
+	int saved, status, padding;
 
 	item->flags = 0;
 
 	/* left-trim */
 	bol += strspn(bol, " \t");
 
-	if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
-		item->command = TODO_COMMENT;
-		item->commit = NULL;
-		item->arg_offset = bol - buf;
-		item->arg_len = eol - bol;
-		return 0;
-	}
-
-	for (i = 0; i < TODO_COMMENT; i++)
-		if (is_command(i, &bol)) {
-			item->command = i;
-			break;
-		}
-	if (i >= TODO_COMMENT)
+	if (!sequencer_parse_todo_command(&bol, &item->command))
 		return error(_("invalid command '%.*s'"),
 			     (int)strcspn(bol, " \t\r\n"), bol);
+
+	if (item->command == TODO_COMMENT) {
+		item->commit = NULL;
+		item->arg_offset = bol - buf;
+		item->arg_len = eol - bol;
+		return 0;
+	}
 
 	/* Eat up extra spaces/ tabs before object name */
 	padding = strspn(bol, " \t");
diff --git a/sequencer.h b/sequencer.h
index a6fa670c7c..20f6fac48a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -262,6 +262,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,
 int write_basic_state(struct replay_opts *opts, const char *head_name,
 		      struct commit *onto, const struct object_id *orig_head);
 void sequencer_post_commit_cleanup(struct repository *r, int verbose);
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd);
 int sequencer_get_last_command(struct repository* r,
 			       enum replay_action *action);
 int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
-- 
2.54.0.rc1.174.gd833f386ac5.dirty

[PATCH 2/2] status: improve rebase todo list parsing

[Phillip Wood]

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

When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this is tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to. Use
the function added in the last commit to parse the command name and
only try to abbreviate the argument for commands that take an object
id. When trying to abbreviate an object id, only replace the object
name if it starts with the abbreviated object id so that labels or
branch names that contain only hex digits are left unchanged.

Comments are now processed after stripping any leading
whitespace from the line. This matches what the sequencer does in
parse_insn_line(). The existing test cases are updated to test a
wider variety of commands. Only the pending commands in the tests
are changed to avoid removing existing coverage.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t7512-status-help.sh |  74 ++++++++++++++++---------
 wt-status.c            | 121 ++++++++++++++++++++++++++++++++---------
 2 files changed, 143 insertions(+), 52 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 08e82f7914..aca4b6d332 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -224,7 +224,7 @@ test_expect_success 'status when splitting a commit' '
 	COMMIT3=$(git rev-parse --short split_commit) &&
 	test_commit four_split main.txt four &&
 	COMMIT4=$(git rev-parse --short split_commit) &&
-	FAKE_LINES="1 edit 2 3" &&
+	FAKE_LINES="reword 1 edit 2 fixup_-C 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
@@ -233,10 +233,10 @@ test_expect_success 'status when splitting a commit' '
 	cat >expected <<EOF &&
 interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
-   pick $COMMIT2 # two_split
+   reword $COMMIT2 # two_split
    edit $COMMIT3 # three_split
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_split
+   fixup -C $COMMIT4 # four_split
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''split_commit'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -297,7 +297,7 @@ test_expect_success 'prepare for several edits' '
 
 
 test_expect_success 'status: (continue first edit) second edit' '
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 drop 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -312,7 +312,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   drop $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -327,7 +327,7 @@ EOF
 
 test_expect_success 'status: (continue first edit) second edit and split' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 squash 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -343,7 +343,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   squash $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -362,7 +362,7 @@ EOF
 
 test_expect_success 'status: (continue first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 fixup 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -378,7 +378,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   fixup $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -393,7 +393,7 @@ EOF
 
 test_expect_success 'status: (amend first edit) second edit' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 fixup_-c 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -409,7 +409,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   fixup -c $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -460,14 +460,20 @@ EOF
 
 test_expect_success 'status: (amend first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
 	COMMIT4=$(git rev-parse --short several_edits) &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	merge $(git rev-parse main) $(git rev-parse several_edits)
+	EOF
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git commit --amend -m "c" &&
 	git rebase --continue &&
 	git commit --amend -m "d" &&
@@ -477,7 +483,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   merge $(git rev-parse --short main) $COMMIT4
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -525,14 +531,21 @@ EOF
 
 test_expect_success 'status: (split first edit) second edit and split' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
 	COMMIT4=$(git rev-parse --short several_edits) &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	reset $(git rev-parse main)
+	merge -C several_edits topic # title
+	EOF
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git reset HEAD^ &&
 	git add main.txt &&
 	git commit --amend -m "f" &&
@@ -543,8 +556,9 @@ interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+   reset $(git rev-parse --short main)
+   merge -C $COMMIT4 topic # title
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -563,14 +577,21 @@ EOF
 
 test_expect_success 'status: (split first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
+	git branch cafe main &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
-	COMMIT4=$(git rev-parse --short several_edits) &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	update-ref refs/heads/main
+	reset cafe
+	EOF
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git reset HEAD^ &&
 	git add main.txt &&
 	git commit --amend -m "g" &&
@@ -581,8 +602,9 @@ interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+   update-ref refs/heads/main
+   reset cafe
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
diff --git a/wt-status.c b/wt-status.c
index 479ccc3304..ba93a18fc2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1363,6 +1363,51 @@ static int split_commit_in_progress(struct wt_status *s)
 	free(rebase_orig_head);
 
 	return split_in_progress;
+}
+
+static void abbrev_oid_in_line(struct repository *r,
+			       struct strbuf *line, char **pp)
+{
+	char *p = *pp;
+	char *end_of_object_name, saved;
+	const char *abbrev;
+	struct object_id oid;
+	bool have_oid;
+
+	p += strspn(p, " \t");
+	end_of_object_name = p + strcspn(p, " \t");
+	/*
+	 * The for "merge" and "reset" the object name may be a label or
+	 * ref rather than a hex object id. Only abbreviate the object
+	 * name if it is a hex object id.
+	 */
+	for (const char *q = p; q < end_of_object_name; q++) {
+		if (!isxdigit(*q))
+			goto out;
+	}
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	have_oid = !repo_get_oid(r, p, &oid);
+	*end_of_object_name = saved;
+	if (!have_oid)
+		goto out; /* object name was a label */
+	abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
+	if (!starts_with(p, abbrev))
+		goto out; /* object name was a refname containing only xdigits */
+	p += strlen(abbrev);
+	strbuf_remove(line, p - line->buf, end_of_object_name - p);
+	end_of_object_name = p;
+out:
+	*pp = end_of_object_name;
+}
+
+static void skip_dash_c(char **pp) {
+	char *p = *pp;
+
+	p += strspn(p, " \t");
+	/* The (void) cast is required to silence -Wunused_value */
+	(void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
+	*pp = p;
 }
 
 /*
@@ -1371,29 +1416,57 @@ static int split_commit_in_progress(struct wt_status *s)
  * into
  * "pick d6a2f03 some message"
  *
- * The function assumes that the line does not contain useless spaces
- * before or after the command.
+ * Returns false on comment lines, true otherwise
  */
-static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
+static bool format_todo_line(struct repository *r, struct strbuf *line)
 {
-	struct string_list split = STRING_LIST_INIT_DUP;
-	struct object_id oid;
-
-	if (starts_with(line->buf, "exec ") ||
-	    starts_with(line->buf, "x ") ||
-	    starts_with(line->buf, "label ") ||
-	    starts_with(line->buf, "l "))
-		return;
-
-	if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
-	    !repo_get_oid(r, split.items[1].string, &oid)) {
-		strbuf_reset(line);
-		strbuf_addf(line, "%s ", split.items[0].string);
-		strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
-		for (size_t i = 2; i < split.nr; i++)
-			strbuf_addf(line, " %s", split.items[i].string);
+	enum todo_command cmd;
+	char *p = line->buf;
+
+	if (!sequencer_parse_todo_command((const char**)&p, &cmd))
+		return true; /* keep invalid lines */
+
+	switch (cmd) {
+	case TODO_COMMENT:
+		return false;
+
+	case TODO_MERGE:
+		skip_dash_c(&p);
+		while (true) {
+			p += strspn(p, " \t");
+			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
+				break;
+			abbrev_oid_in_line(r, line, &p);
+		}
+		break;
+
+	case TODO_FIXUP:
+		skip_dash_c(&p);
+		/* fallthrough */
+	case TODO_DROP:
+	case TODO_EDIT:
+	case TODO_PICK:
+	case TODO_RESET:
+	case TODO_REVERT:
+	case TODO_REWORD:
+	case TODO_SQUASH:
+		abbrev_oid_in_line(r, line, &p);
+		break;
+
+	/*
+	 * Avoid "default" and instead list all the other commands so
+	 * that -Wswitch warns if a new command is added without handling
+	 * it in this function.
+	 */
+	case TODO_BREAK:
+	case TODO_EXEC:
+	case TODO_LABEL:
+	case TODO_NOOP:
+	case TODO_UPDATE_REF:
+		break;
 	}
-	string_list_clear(&split, 0);
+
+	return true;
 }
 
 static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
@@ -1411,13 +1484,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
 			  repo_git_path_replace(r, &buf, "%s", fname));
 	}
 	while (!strbuf_getline_lf(&buf, f)) {
-		if (starts_with(buf.buf, comment_line_str))
-			continue;
 		strbuf_trim(&buf);
-		if (!buf.len)
-			continue;
-		abbrev_oid_in_line(r, &buf);
-		string_list_append(lines, buf.buf);
+		if (format_todo_line(r, &buf))
+			string_list_append(lines, buf.buf);
 	}
 	fclose(f);
 
-- 
2.54.0.rc1.174.gd833f386ac5.dirty

Re: [PATCH 2/2] status: improve rebase todo list parsing

[Tian Yuchen]

Hi Phillip,

On 4/20/26 23:04, Phillip Wood wrote:

> +	if (!starts_with(p, abbrev))
> +		goto out; /* object name was a refname containing only xdigits */
> +	p += strlen(abbrev);
> +	strbuf_remove(line, p - line->buf, end_of_object_name - p);
> +	end_of_object_name = p;


> -	if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
> -	    !repo_get_oid(r, split.items[1].string, &oid)) {
> -		strbuf_reset(line);
> -		strbuf_addf(line, "%s ", split.items[0].string);
> -		strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
> -		for (size_t i = 2; i < split.nr; i++)
> -			strbuf_addf(line, " %s", split.items[i].string);

I noticed that after this patch, refnames shorter than seven characters 
are no longer standardised to the standard seven-character length, 
because the 'start_with()' function always returns FALSE. The code jumps 
directly to 'out', without completing or cutting the refname.

I’m not sure if this was your intention, but I just want to point it out 
for your information.

(Also noted that there is a very rare scenario where the OID of a 
refname longer than 7 characters happens to begin with the refname 
itself; in this case, 'start_with' returns TRUE and the string is cut 
incorrectly. However, I think we can safely ignore this.)

Regards, Yuchen

Re: [PATCH 2/2] status: improve rebase todo list parsing

[Phillip Wood]

Hi Tian

On 20/04/2026 17:38, Tian Yuchen wrote:
> On 4/20/26 23:04, Phillip Wood wrote:
> 
>> +    if (!starts_with(p, abbrev))
>> +        goto out; /* object name was a refname containing only 
>> xdigits */
>> +    p += strlen(abbrev);
>> +    strbuf_remove(line, p - line->buf, end_of_object_name - p);
>> +    end_of_object_name = p;
> 
> 
>> -    if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
>> -        !repo_get_oid(r, split.items[1].string, &oid)) {
>> -        strbuf_reset(line);
>> -        strbuf_addf(line, "%s ", split.items[0].string);
>> -        strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
>> -        for (size_t i = 2; i < split.nr; i++)
>> -            strbuf_addf(line, " %s", split.items[i].string);
> 
> I noticed that after this patch, refnames shorter than seven characters 
> are no longer standardised to the standard seven-character length, 
> because the 'start_with()' function always returns FALSE. The code jumps 
> directly to 'out', without completing or cutting the refname.

Do you mean object id's rather than refnames? We do not want to alter 
refnames, but we do want to abbreviate object ids. You're right that if 
the todo list contains an object id that is shorter than the default 
abbreviation length it will not be expanded but "git rebase" rewrites 
the todo list to contain full length object ids after the user has 
edited it. If a user may edit the todo list directly without using "git 
rebase --edit-todo" and then run "git status" before "git rebase 
--continue" the todo list could contain shorter object ids but that 
seems pretty unlikely.
> 
> I’m not sure if this was your intention, but I just want to point it out 
> for your information.
> 
> (Also noted that there is a very rare scenario where the OID of a 
> refname longer than 7 characters happens to begin with the refname 
> itself; in this case, 'start_with' returns TRUE and the string is cut 
> incorrectly. However, I think we can safely ignore this.)

Yes, I think that's unlikely to happen in practice.

Thanks

Phillip
> Regards, Yuchen

Re: [PATCH 1/2] sequencer: factor out parsing of todo commands

[Elijah Newren]

On Mon, Apr 20, 2026 at 8:41 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Move the code that parses todo commands into a separate function so that
> it can be shared with "git status" in the next commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c | 45 ++++++++++++++++++++++++++++++---------------
>  sequencer.h |  1 +
>  2 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b7d8dca47f..b8e860434a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2625,6 +2625,27 @@ static int is_command(enum todo_command command, const char **bol)
>                 return 1;
>         }
>         return 0;
> +}
> +
> +bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
> +{
> +       const char *s = *p;
> +
> +       for (int i = 0; i < TODO_COMMENT; i++)
> +               if (is_command(i, p)) {
> +                       *cmd = i;
> +                       return true;
> +               }
> +
> +       if (starts_with(s, comment_line_str)) {
> +               *cmd = TODO_COMMENT;
> +               return true;
> +       } else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
> +               *cmd = TODO_COMMENT;
> +               return true;
> +       }
> +
> +       return false;
>  }

Since *p will be advanced by this function, perhaps it's worth a quick
comment before the function explaining how it advances?

>
>  static int check_label_or_ref_arg(enum todo_command command, const char *arg)
> @@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
>  {
>         struct object_id commit_oid;
>         char *end_of_object_name;
> -       int i, saved, status, padding;
> +       int saved, status, padding;
>
>         item->flags = 0;
>
>         /* left-trim */
>         bol += strspn(bol, " \t");
>
> -       if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
> -               item->command = TODO_COMMENT;
> -               item->commit = NULL;
> -               item->arg_offset = bol - buf;
> -               item->arg_len = eol - bol;
> -               return 0;
> -       }
> -
> -       for (i = 0; i < TODO_COMMENT; i++)
> -               if (is_command(i, &bol)) {
> -                       item->command = i;
> -                       break;
> -               }
> -       if (i >= TODO_COMMENT)
> +       if (!sequencer_parse_todo_command(&bol, &item->command))
>                 return error(_("invalid command '%.*s'"),
>                              (int)strcspn(bol, " \t\r\n"), bol);
> +
> +       if (item->command == TODO_COMMENT) {
> +               item->commit = NULL;
> +               item->arg_offset = bol - buf;
> +               item->arg_len = eol - bol;
> +               return 0;
> +       }

The commit message says it's just moving code, but are there some
subtle changes?  It switches the order of parsing commands and
comments, which I think doesn't matter.  I think the previous code
would treat a bare carriage return not immediately followed by a
newline as the start of a comment, whereas the new code only handles
carriage return immediately followed by a newline as the start of a
comment.  I don't think that matters in practice, and was potentially
buggy before, but feels like the commit message isn't quite telling
the whole story.  Maybe it's worth calling out the minor but harmless
tweaks in the commit message?


>         /* Eat up extra spaces/ tabs before object name */
>         padding = strspn(bol, " \t");
> diff --git a/sequencer.h b/sequencer.h
> index a6fa670c7c..20f6fac48a 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -262,6 +262,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,
>  int write_basic_state(struct replay_opts *opts, const char *head_name,
>                       struct commit *onto, const struct object_id *orig_head);
>  void sequencer_post_commit_cleanup(struct repository *r, int verbose);
> +bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd);
>  int sequencer_get_last_command(struct repository* r,
>                                enum replay_action *action);
>  int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
> --
> 2.54.0.rc1.174.gd833f386ac5.dirty

Otherwise, looks good.

Re: [PATCH 2/2] status: improve rebase todo list parsing

[Elijah Newren]

On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When there is rebase in progress "git status" displays the last couple
> of completed and the next couple of pending commands from the todo
> list. When it does this is tries to abbreviate the object ids of

is tries => it tries ?

[...]
> @@ -1363,6 +1363,51 @@ static int split_commit_in_progress(struct wt_status *s)
>         free(rebase_orig_head);
>
>         return split_in_progress;
> +}
> +
> +static void abbrev_oid_in_line(struct repository *r,
> +                              struct strbuf *line, char **pp)
> +{
> +       char *p = *pp;
> +       char *end_of_object_name, saved;
> +       const char *abbrev;
> +       struct object_id oid;
> +       bool have_oid;

I'll put "thinking out loud" text in square brackets below...

> +
> +       p += strspn(p, " \t");
> +       end_of_object_name = p + strcspn(p, " \t");

[Advances p after whitespace, marks the end of the object with the
next whitespace after that.]

> +       /*
> +        * The for "merge" and "reset" the object name may be a label or

The for => For ?

> +        * ref rather than a hex object id. Only abbreviate the object
> +        * name if it is a hex object id.
> +        */
> +       for (const char *q = p; q < end_of_object_name; q++) {
> +               if (!isxdigit(*q))
> +                       goto out;
> +       }



> +       saved = *end_of_object_name;
> +       *end_of_object_name = '\0';
> +       have_oid = !repo_get_oid(r, p, &oid);
> +       *end_of_object_name = saved;

[Tries to resolve the token, doing NUL-termination and restore dance.]

> +       if (!have_oid)
> +               goto out; /* object name was a label */


> +       abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
> +       if (!starts_with(p, abbrev))
> +               goto out; /* object name was a refname containing only xdigits */

[Ensures what we have is an oid rather than a branch name that can be
resolved to an oid]

> +       p += strlen(abbrev);
> +       strbuf_remove(line, p - line->buf, end_of_object_name - p);
> +       end_of_object_name = p;

[Splice out a bunch of characters in the middle?]

> +out:
> +       *pp = end_of_object_name;
> +}

I had a hard time following the logic in the function and trying to
figure out what it was doing.  I went line by line but had no mental
model to follow.  When I got to the comment that is now above
format_todo_line(), I suddenly understood, but without it, all the
code was hard to follow.  Maybe a small comment at the beginning of
the function along the lines of

 /*
  * If the whitespace-delimited token starting at or just after *pp is a
  * full hex object id that resolves uniquely, rewrite it in place to
  * its default abbreviation, shrinking `line` accordingly. On return
  * *pp points one past the (possibly abbreviated) token. Leaves both
  * `line` and *pp-advanced-past-the-token unchanged in all other cases
  * (non-hex token, unresolvable, or a refname that happens to consist
  * only of hex digits).
  */

?  (Assuming I'm understanding correctly, of course.)

> +
> +static void skip_dash_c(char **pp) {

Move the brace to the next line?

> +       char *p = *pp;
> +
> +       p += strspn(p, " \t");
> +       /* The (void) cast is required to silence -Wunused_value */

-Wunused_value => -Wunused-value ?

> +       (void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
> +       *pp = p;
>  }
>
>  /*
> @@ -1371,29 +1416,57 @@ static int split_commit_in_progress(struct wt_status *s)
>   * into
>   * "pick d6a2f03 some message"
>   *
> - * The function assumes that the line does not contain useless spaces
> - * before or after the command.
> + * Returns false on comment lines, true otherwise
>   */
> -static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
> +static bool format_todo_line(struct repository *r, struct strbuf *line)
>  {
> -       struct string_list split = STRING_LIST_INIT_DUP;
> -       struct object_id oid;
> -
> -       if (starts_with(line->buf, "exec ") ||
> -           starts_with(line->buf, "x ") ||
> -           starts_with(line->buf, "label ") ||
> -           starts_with(line->buf, "l "))
> -               return;
> -
> -       if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
> -           !repo_get_oid(r, split.items[1].string, &oid)) {
> -               strbuf_reset(line);
> -               strbuf_addf(line, "%s ", split.items[0].string);
> -               strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
> -               for (size_t i = 2; i < split.nr; i++)
> -                       strbuf_addf(line, " %s", split.items[i].string);
> +       enum todo_command cmd;
> +       char *p = line->buf;
> +
> +       if (!sequencer_parse_todo_command((const char**)&p, &cmd))
> +               return true; /* keep invalid lines */
> +
> +       switch (cmd) {
> +       case TODO_COMMENT:
> +               return false;
> +
> +       case TODO_MERGE:
> +               skip_dash_c(&p);
> +               while (true) {
> +                       p += strspn(p, " \t");
> +                       if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
> +                               break;
> +                       abbrev_oid_in_line(r, line, &p);
> +               }
> +               break;
> +
> +       case TODO_FIXUP:
> +               skip_dash_c(&p);
> +               /* fallthrough */
> +       case TODO_DROP:
> +       case TODO_EDIT:
> +       case TODO_PICK:
> +       case TODO_RESET:
> +       case TODO_REVERT:
> +       case TODO_REWORD:
> +       case TODO_SQUASH:
> +               abbrev_oid_in_line(r, line, &p);
> +               break;
> +
> +       /*
> +        * Avoid "default" and instead list all the other commands so
> +        * that -Wswitch warns if a new command is added without handling
> +        * it in this function.
> +        */

Nice. :-)

> +       case TODO_BREAK:
> +       case TODO_EXEC:
> +       case TODO_LABEL:
> +       case TODO_NOOP:
> +       case TODO_UPDATE_REF:
> +               break;
>         }
> -       string_list_clear(&split, 0);
> +
> +       return true;
>  }
>
>  static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
> @@ -1411,13 +1484,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
>                           repo_git_path_replace(r, &buf, "%s", fname));
>         }
>         while (!strbuf_getline_lf(&buf, f)) {
> -               if (starts_with(buf.buf, comment_line_str))
> -                       continue;
>                 strbuf_trim(&buf);
> -               if (!buf.len)
> -                       continue;
> -               abbrev_oid_in_line(r, &buf);
> -               string_list_append(lines, buf.buf);
> +               if (format_todo_line(r, &buf))
> +                       string_list_append(lines, buf.buf);
>         }
>         fclose(f);
>
> --
> 2.54.0.rc1.174.gd833f386ac5.dirty

Other than the minor comments above, this looks like a nice cleanup.

Re: [PATCH 2/2] status: improve rebase todo list parsing

[Patrick Steinhardt]

On Tue, Apr 21, 2026 at 05:32:21PM -0700, Elijah Newren wrote:
> On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > +       /*
> > +        * Avoid "default" and instead list all the other commands so
> > +        * that -Wswitch warns if a new command is added without handling
> > +        * it in this function.
> > +        */
> 
> Nice. :-)

Do we actually use -Wswitch anywhere? A quick grep in our code base
didn't surface it, so I'm a bit sceptical that we would actually detect
any missing cases via CI.

Thanks!

Patrick

Re: [PATCH 2/2] status: improve rebase todo list parsing

[Phillip Wood]

On 22/04/2026 14:28, Patrick Steinhardt wrote:
> On Tue, Apr 21, 2026 at 05:32:21PM -0700, Elijah Newren wrote:
>> On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> +       /*
>>> +        * Avoid "default" and instead list all the other commands so
>>> +        * that -Wswitch warns if a new command is added without handling
>>> +        * it in this function.
>>> +        */
>>
>> Nice. :-)
> 
> Do we actually use -Wswitch anywhere? A quick grep in our code base
> didn't surface it, so I'm a bit sceptical that we would actually detect
> any missing cases via CI.

I've just double checked the gcc docs and it's included in -Wall (I'm 
pretty sure I tried deleting one of the case statements to check before 
I submitted the patch)

Thanks

Phillip

Re: [PATCH 2/2] status: improve rebase todo list parsing

[Phillip Wood]

Hi Elijah

Thanks for the review, all you suggestions look sensible to me, I'll 
send a re-roll.

Phillip

On 22/04/2026 01:32, Elijah Newren wrote:
> On Mon, Apr 20, 2026 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When there is rebase in progress "git status" displays the last couple
>> of completed and the next couple of pending commands from the todo
>> list. When it does this is tries to abbreviate the object ids of
> 
> is tries => it tries ?
> 
> [...]
>> @@ -1363,6 +1363,51 @@ static int split_commit_in_progress(struct wt_status *s)
>>          free(rebase_orig_head);
>>
>>          return split_in_progress;
>> +}
>> +
>> +static void abbrev_oid_in_line(struct repository *r,
>> +                              struct strbuf *line, char **pp)
>> +{
>> +       char *p = *pp;
>> +       char *end_of_object_name, saved;
>> +       const char *abbrev;
>> +       struct object_id oid;
>> +       bool have_oid;
> 
> I'll put "thinking out loud" text in square brackets below...
> 
>> +
>> +       p += strspn(p, " \t");
>> +       end_of_object_name = p + strcspn(p, " \t");
> 
> [Advances p after whitespace, marks the end of the object with the
> next whitespace after that.]
> 
>> +       /*
>> +        * The for "merge" and "reset" the object name may be a label or
> 
> The for => For ?
> 
>> +        * ref rather than a hex object id. Only abbreviate the object
>> +        * name if it is a hex object id.
>> +        */
>> +       for (const char *q = p; q < end_of_object_name; q++) {
>> +               if (!isxdigit(*q))
>> +                       goto out;
>> +       }
> 
> 
> 
>> +       saved = *end_of_object_name;
>> +       *end_of_object_name = '\0';
>> +       have_oid = !repo_get_oid(r, p, &oid);
>> +       *end_of_object_name = saved;
> 
> [Tries to resolve the token, doing NUL-termination and restore dance.]
> 
>> +       if (!have_oid)
>> +               goto out; /* object name was a label */
> 
> 
>> +       abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
>> +       if (!starts_with(p, abbrev))
>> +               goto out; /* object name was a refname containing only xdigits */
> 
> [Ensures what we have is an oid rather than a branch name that can be
> resolved to an oid]
> 
>> +       p += strlen(abbrev);
>> +       strbuf_remove(line, p - line->buf, end_of_object_name - p);
>> +       end_of_object_name = p;
> 
> [Splice out a bunch of characters in the middle?]
> 
>> +out:
>> +       *pp = end_of_object_name;
>> +}
> 
> I had a hard time following the logic in the function and trying to
> figure out what it was doing.  I went line by line but had no mental
> model to follow.  When I got to the comment that is now above
> format_todo_line(), I suddenly understood, but without it, all the
> code was hard to follow.  Maybe a small comment at the beginning of
> the function along the lines of
> 
>   /*
>    * If the whitespace-delimited token starting at or just after *pp is a
>    * full hex object id that resolves uniquely, rewrite it in place to
>    * its default abbreviation, shrinking `line` accordingly. On return
>    * *pp points one past the (possibly abbreviated) token. Leaves both
>    * `line` and *pp-advanced-past-the-token unchanged in all other cases
>    * (non-hex token, unresolvable, or a refname that happens to consist
>    * only of hex digits).
>    */
> 
> ?  (Assuming I'm understanding correctly, of course.)
> 
>> +
>> +static void skip_dash_c(char **pp) {
> 
> Move the brace to the next line?
> 
>> +       char *p = *pp;
>> +
>> +       p += strspn(p, " \t");
>> +       /* The (void) cast is required to silence -Wunused_value */
> 
> -Wunused_value => -Wunused-value ?
> 
>> +       (void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
>> +       *pp = p;
>>   }
>>
>>   /*
>> @@ -1371,29 +1416,57 @@ static int split_commit_in_progress(struct wt_status *s)
>>    * into
>>    * "pick d6a2f03 some message"
>>    *
>> - * The function assumes that the line does not contain useless spaces
>> - * before or after the command.
>> + * Returns false on comment lines, true otherwise
>>    */
>> -static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
>> +static bool format_todo_line(struct repository *r, struct strbuf *line)
>>   {
>> -       struct string_list split = STRING_LIST_INIT_DUP;
>> -       struct object_id oid;
>> -
>> -       if (starts_with(line->buf, "exec ") ||
>> -           starts_with(line->buf, "x ") ||
>> -           starts_with(line->buf, "label ") ||
>> -           starts_with(line->buf, "l "))
>> -               return;
>> -
>> -       if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
>> -           !repo_get_oid(r, split.items[1].string, &oid)) {
>> -               strbuf_reset(line);
>> -               strbuf_addf(line, "%s ", split.items[0].string);
>> -               strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
>> -               for (size_t i = 2; i < split.nr; i++)
>> -                       strbuf_addf(line, " %s", split.items[i].string);
>> +       enum todo_command cmd;
>> +       char *p = line->buf;
>> +
>> +       if (!sequencer_parse_todo_command((const char**)&p, &cmd))
>> +               return true; /* keep invalid lines */
>> +
>> +       switch (cmd) {
>> +       case TODO_COMMENT:
>> +               return false;
>> +
>> +       case TODO_MERGE:
>> +               skip_dash_c(&p);
>> +               while (true) {
>> +                       p += strspn(p, " \t");
>> +                       if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
>> +                               break;
>> +                       abbrev_oid_in_line(r, line, &p);
>> +               }
>> +               break;
>> +
>> +       case TODO_FIXUP:
>> +               skip_dash_c(&p);
>> +               /* fallthrough */
>> +       case TODO_DROP:
>> +       case TODO_EDIT:
>> +       case TODO_PICK:
>> +       case TODO_RESET:
>> +       case TODO_REVERT:
>> +       case TODO_REWORD:
>> +       case TODO_SQUASH:
>> +               abbrev_oid_in_line(r, line, &p);
>> +               break;
>> +
>> +       /*
>> +        * Avoid "default" and instead list all the other commands so
>> +        * that -Wswitch warns if a new command is added without handling
>> +        * it in this function.
>> +        */
> 
> Nice. :-)
> 
>> +       case TODO_BREAK:
>> +       case TODO_EXEC:
>> +       case TODO_LABEL:
>> +       case TODO_NOOP:
>> +       case TODO_UPDATE_REF:
>> +               break;
>>          }
>> -       string_list_clear(&split, 0);
>> +
>> +       return true;
>>   }
>>
>>   static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
>> @@ -1411,13 +1484,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
>>                            repo_git_path_replace(r, &buf, "%s", fname));
>>          }
>>          while (!strbuf_getline_lf(&buf, f)) {
>> -               if (starts_with(buf.buf, comment_line_str))
>> -                       continue;
>>                  strbuf_trim(&buf);
>> -               if (!buf.len)
>> -                       continue;
>> -               abbrev_oid_in_line(r, &buf);
>> -               string_list_append(lines, buf.buf);
>> +               if (format_todo_line(r, &buf))
>> +                       string_list_append(lines, buf.buf);
>>          }
>>          fclose(f);
>>
>> --
>> 2.54.0.rc1.174.gd833f386ac5.dirty
> 
> Other than the minor comments above, this looks like a nice cleanup.

[PATCH v2 0/2] status: improve rebase todo list parsing

[Phillip Wood]

When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this is tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to.

This series fixes that. The first patch factors out the sequencer
code that parses the command names in the todo list. The second patch
uses that function in "git status" to parse the command names so that
it knows whether the line may contain "-C" and whether there is an
object id that should be abbreviated.

Thanks to Elijah and Patrick for their comments in V1.

Changes since V1:

Patch 1 - Expanded commit message and added a code comment.

Patch 2 - Fixed some typos, added a code comment and clarified that -Wswitch
          is included by -Wall.

Base-Commit: 8c9303b1ffae5b745d1b0a1f98330cf7944d8db0
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fimprove-status-todo-list-parsing%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/8c9303b1f...b80bc1e0a
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/improve-status-todo-list-parsing/v2


Phillip Wood (2):
  sequencer: factor out parsing of todo commands
  status: improve rebase todo list parsing

 sequencer.c            |  45 +++++++++-----
 sequencer.h            |   8 +++
 t/t7512-status-help.sh |  74 +++++++++++++++--------
 wt-status.c            | 131 +++++++++++++++++++++++++++++++++--------
 4 files changed, 191 insertions(+), 67 deletions(-)

Range-diff against v1:
1:  3d5135a719 ! 1:  d27dddff93 sequencer: factor out parsing of todo commands
    @@ Metadata
      ## Commit message ##
         sequencer: factor out parsing of todo commands
     
    -    Move the code that parses todo commands into a separate function so that
    -    it can be shared with "git status" in the next commit.
    +    Move the code that parses todo commands into a separate function so
    +    that it can be shared with "git status" in the next commit. As we
    +    know the input is NUL terminated we do not pass a pointer to the end
    +    of the line and instead test for a blank line by looking for NUL, CR
    +    LF, or LF. We use starts_with() instead of starts_with_mem() for the
    +    same reason. This results in slightly different behavior when there
    +    a CR at the start of the line that is not followed by LF. Previously
    +    such a line was treated as a comment rather than an invalid line.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    @@ sequencer.h: int read_author_script(const char *path, char **name, char **email,
      int write_basic_state(struct replay_opts *opts, const char *head_name,
      		      struct commit *onto, const struct object_id *orig_head);
      void sequencer_post_commit_cleanup(struct repository *r, int verbose);
    ++
    ++/*
    ++ * Try to parse the todo command pointed to by *p. On success sets cmd,
    ++ * advances p and returns true. On failure returns false, leaves p and
    ++ * cmd unchanged.
    ++ */
     +bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd);
    ++
      int sequencer_get_last_command(struct repository* r,
      			       enum replay_action *action);
      int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
2:  d20dc1f655 ! 2:  b80bc1e0a2 status: improve rebase todo list parsing
    @@ Commit message
     
         When there is rebase in progress "git status" displays the last couple
         of completed and the next couple of pending commands from the todo
    -    list. When it does this is tries to abbreviate the object ids of
    +    list. When it does this it tries to abbreviate the object ids of
         the commits to be picked. Unfortunately it does not abbreviate the
         object ids when the line starts with "fixup -C" or "merge -C". It
         also mistakenly replaces the refname in "reset main" and "update-ref
    @@ Commit message
         wider variety of commands. Only the pending commands in the tests
         are changed to avoid removing existing coverage.
     
    +    Helped-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## t/t7512-status-help.sh ##
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
      	return split_in_progress;
     +}
     +
    ++/*
    ++ * If the whitespace-delimited token starting at or just after *pp *
    ++ * is a hex object id that is longer than its default abbreviation, *
    ++ * abbreviate it in-place, shrinking `line` accordingly. On return
    ++ * *pp points one past the (possibly abbreviated) token. Leaves both
    ++ * `line` and *pp-advanced-past-the-token unchanged in all other cases
    ++ * (non-hex token, unresolvable, or a refname that happens to consist
    ++ * only of hex digits).
    ++ */
     +static void abbrev_oid_in_line(struct repository *r,
     +			       struct strbuf *line, char **pp)
     +{
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +	p += strspn(p, " \t");
     +	end_of_object_name = p + strcspn(p, " \t");
     +	/*
    -+	 * The for "merge" and "reset" the object name may be a label or
    ++	 * For "merge" and "reset" the object name may be a label or
     +	 * ref rather than a hex object id. Only abbreviate the object
     +	 * name if it is a hex object id.
     +	 */
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +	*pp = end_of_object_name;
     +}
     +
    -+static void skip_dash_c(char **pp) {
    ++static void skip_dash_c(char **pp)
    ++{
     +	char *p = *pp;
     +
     +	p += strspn(p, " \t");
    -+	/* The (void) cast is required to silence -Wunused_value */
    ++	/* The (void) cast is required to silence -Wunused-value */
     +	(void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
     +	*pp = p;
      }
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +
     +	/*
     +	 * Avoid "default" and instead list all the other commands so
    -+	 * that -Wswitch warns if a new command is added without handling
    -+	 * it in this function.
    ++	 * that -Wswitch (which is included in -Wall) warns if a new
    ++	 * command is added without handling it in this function.
     +	 */
     +	case TODO_BREAK:
     +	case TODO_EXEC:
-- 
2.54.0.rc1.174.gd833f386ac5.dirty

[PATCH v2 1/2] sequencer: factor out parsing of todo commands

[Phillip Wood]

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

Move the code that parses todo commands into a separate function so
that it can be shared with "git status" in the next commit. As we
know the input is NUL terminated we do not pass a pointer to the end
of the line and instead test for a blank line by looking for NUL, CR
LF, or LF. We use starts_with() instead of starts_with_mem() for the
same reason. This results in slightly different behavior when there
a CR at the start of the line that is not followed by LF. Previously
such a line was treated as a comment rather than an invalid line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 45 ++++++++++++++++++++++++++++++---------------
 sequencer.h |  8 ++++++++
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b7d8dca47f..b8e860434a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2625,6 +2625,27 @@ static int is_command(enum todo_command command, const char **bol)
 		return 1;
 	}
 	return 0;
+}
+
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
+{
+	const char *s = *p;
+
+	for (int i = 0; i < TODO_COMMENT; i++)
+		if (is_command(i, p)) {
+			*cmd = i;
+			return true;
+		}
+
+	if (starts_with(s, comment_line_str)) {
+		*cmd = TODO_COMMENT;
+		return true;
+	} else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
+		*cmd = TODO_COMMENT;
+		return true;
+	}
+
+	return false;
 }
 
 static int check_label_or_ref_arg(enum todo_command command, const char *arg)
@@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
 {
 	struct object_id commit_oid;
 	char *end_of_object_name;
-	int i, saved, status, padding;
+	int saved, status, padding;
 
 	item->flags = 0;
 
 	/* left-trim */
 	bol += strspn(bol, " \t");
 
-	if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
-		item->command = TODO_COMMENT;
-		item->commit = NULL;
-		item->arg_offset = bol - buf;
-		item->arg_len = eol - bol;
-		return 0;
-	}
-
-	for (i = 0; i < TODO_COMMENT; i++)
-		if (is_command(i, &bol)) {
-			item->command = i;
-			break;
-		}
-	if (i >= TODO_COMMENT)
+	if (!sequencer_parse_todo_command(&bol, &item->command))
 		return error(_("invalid command '%.*s'"),
 			     (int)strcspn(bol, " \t\r\n"), bol);
+
+	if (item->command == TODO_COMMENT) {
+		item->commit = NULL;
+		item->arg_offset = bol - buf;
+		item->arg_len = eol - bol;
+		return 0;
+	}
 
 	/* Eat up extra spaces/ tabs before object name */
 	padding = strspn(bol, " \t");
diff --git a/sequencer.h b/sequencer.h
index a6fa670c7c..28fabef926 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -262,6 +262,14 @@ int read_author_script(const char *path, char **name, char **email, char **date,
 int write_basic_state(struct replay_opts *opts, const char *head_name,
 		      struct commit *onto, const struct object_id *orig_head);
 void sequencer_post_commit_cleanup(struct repository *r, int verbose);
+
+/*
+ * Try to parse the todo command pointed to by *p. On success sets cmd,
+ * advances p and returns true. On failure returns false, leaves p and
+ * cmd unchanged.
+ */
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd);
+
 int sequencer_get_last_command(struct repository* r,
 			       enum replay_action *action);
 int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
-- 
2.54.0.rc1.174.gd833f386ac5.dirty

[PATCH v2 2/2] status: improve rebase todo list parsing

[Phillip Wood]

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

When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this it tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to. Use
the function added in the last commit to parse the command name and
only try to abbreviate the argument for commands that take an object
id. When trying to abbreviate an object id, only replace the object
name if it starts with the abbreviated object id so that labels or
branch names that contain only hex digits are left unchanged.

Comments are now processed after stripping any leading
whitespace from the line. This matches what the sequencer does in
parse_insn_line(). The existing test cases are updated to test a
wider variety of commands. Only the pending commands in the tests
are changed to avoid removing existing coverage.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t7512-status-help.sh |  74 +++++++++++++++--------
 wt-status.c            | 131 +++++++++++++++++++++++++++++++++--------
 2 files changed, 153 insertions(+), 52 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 08e82f7914..aca4b6d332 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -224,7 +224,7 @@ test_expect_success 'status when splitting a commit' '
 	COMMIT3=$(git rev-parse --short split_commit) &&
 	test_commit four_split main.txt four &&
 	COMMIT4=$(git rev-parse --short split_commit) &&
-	FAKE_LINES="1 edit 2 3" &&
+	FAKE_LINES="reword 1 edit 2 fixup_-C 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
@@ -233,10 +233,10 @@ test_expect_success 'status when splitting a commit' '
 	cat >expected <<EOF &&
 interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
-   pick $COMMIT2 # two_split
+   reword $COMMIT2 # two_split
    edit $COMMIT3 # three_split
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_split
+   fixup -C $COMMIT4 # four_split
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''split_commit'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -297,7 +297,7 @@ test_expect_success 'prepare for several edits' '
 
 
 test_expect_success 'status: (continue first edit) second edit' '
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 drop 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -312,7 +312,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   drop $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -327,7 +327,7 @@ EOF
 
 test_expect_success 'status: (continue first edit) second edit and split' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 squash 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -343,7 +343,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   squash $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -362,7 +362,7 @@ EOF
 
 test_expect_success 'status: (continue first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 fixup 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -378,7 +378,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   fixup $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -393,7 +393,7 @@ EOF
 
 test_expect_success 'status: (amend first edit) second edit' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 fixup_-c 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -409,7 +409,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   fixup -c $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -460,14 +460,20 @@ EOF
 
 test_expect_success 'status: (amend first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
 	COMMIT4=$(git rev-parse --short several_edits) &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	merge $(git rev-parse main) $(git rev-parse several_edits)
+	EOF
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git commit --amend -m "c" &&
 	git rebase --continue &&
 	git commit --amend -m "d" &&
@@ -477,7 +483,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   merge $(git rev-parse --short main) $COMMIT4
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -525,14 +531,21 @@ EOF
 
 test_expect_success 'status: (split first edit) second edit and split' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
 	COMMIT4=$(git rev-parse --short several_edits) &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	reset $(git rev-parse main)
+	merge -C several_edits topic # title
+	EOF
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git reset HEAD^ &&
 	git add main.txt &&
 	git commit --amend -m "f" &&
@@ -543,8 +556,9 @@ interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+   reset $(git rev-parse --short main)
+   merge -C $COMMIT4 topic # title
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -563,14 +577,21 @@ EOF
 
 test_expect_success 'status: (split first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
+	git branch cafe main &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
-	COMMIT4=$(git rev-parse --short several_edits) &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	update-ref refs/heads/main
+	reset cafe
+	EOF
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git reset HEAD^ &&
 	git add main.txt &&
 	git commit --amend -m "g" &&
@@ -581,8 +602,9 @@ interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+   update-ref refs/heads/main
+   reset cafe
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
diff --git a/wt-status.c b/wt-status.c
index 479ccc3304..94c159d9d4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1363,6 +1363,61 @@ static int split_commit_in_progress(struct wt_status *s)
 	free(rebase_orig_head);
 
 	return split_in_progress;
+}
+
+/*
+ * If the whitespace-delimited token starting at or just after *pp *
+ * is a hex object id that is longer than its default abbreviation, *
+ * abbreviate it in-place, shrinking `line` accordingly. On return
+ * *pp points one past the (possibly abbreviated) token. Leaves both
+ * `line` and *pp-advanced-past-the-token unchanged in all other cases
+ * (non-hex token, unresolvable, or a refname that happens to consist
+ * only of hex digits).
+ */
+static void abbrev_oid_in_line(struct repository *r,
+			       struct strbuf *line, char **pp)
+{
+	char *p = *pp;
+	char *end_of_object_name, saved;
+	const char *abbrev;
+	struct object_id oid;
+	bool have_oid;
+
+	p += strspn(p, " \t");
+	end_of_object_name = p + strcspn(p, " \t");
+	/*
+	 * For "merge" and "reset" the object name may be a label or
+	 * ref rather than a hex object id. Only abbreviate the object
+	 * name if it is a hex object id.
+	 */
+	for (const char *q = p; q < end_of_object_name; q++) {
+		if (!isxdigit(*q))
+			goto out;
+	}
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	have_oid = !repo_get_oid(r, p, &oid);
+	*end_of_object_name = saved;
+	if (!have_oid)
+		goto out; /* object name was a label */
+	abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
+	if (!starts_with(p, abbrev))
+		goto out; /* object name was a refname containing only xdigits */
+	p += strlen(abbrev);
+	strbuf_remove(line, p - line->buf, end_of_object_name - p);
+	end_of_object_name = p;
+out:
+	*pp = end_of_object_name;
+}
+
+static void skip_dash_c(char **pp)
+{
+	char *p = *pp;
+
+	p += strspn(p, " \t");
+	/* The (void) cast is required to silence -Wunused-value */
+	(void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
+	*pp = p;
 }
 
 /*
@@ -1371,29 +1426,57 @@ static int split_commit_in_progress(struct wt_status *s)
  * into
  * "pick d6a2f03 some message"
  *
- * The function assumes that the line does not contain useless spaces
- * before or after the command.
+ * Returns false on comment lines, true otherwise
  */
-static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
+static bool format_todo_line(struct repository *r, struct strbuf *line)
 {
-	struct string_list split = STRING_LIST_INIT_DUP;
-	struct object_id oid;
-
-	if (starts_with(line->buf, "exec ") ||
-	    starts_with(line->buf, "x ") ||
-	    starts_with(line->buf, "label ") ||
-	    starts_with(line->buf, "l "))
-		return;
-
-	if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
-	    !repo_get_oid(r, split.items[1].string, &oid)) {
-		strbuf_reset(line);
-		strbuf_addf(line, "%s ", split.items[0].string);
-		strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
-		for (size_t i = 2; i < split.nr; i++)
-			strbuf_addf(line, " %s", split.items[i].string);
+	enum todo_command cmd;
+	char *p = line->buf;
+
+	if (!sequencer_parse_todo_command((const char**)&p, &cmd))
+		return true; /* keep invalid lines */
+
+	switch (cmd) {
+	case TODO_COMMENT:
+		return false;
+
+	case TODO_MERGE:
+		skip_dash_c(&p);
+		while (true) {
+			p += strspn(p, " \t");
+			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
+				break;
+			abbrev_oid_in_line(r, line, &p);
+		}
+		break;
+
+	case TODO_FIXUP:
+		skip_dash_c(&p);
+		/* fallthrough */
+	case TODO_DROP:
+	case TODO_EDIT:
+	case TODO_PICK:
+	case TODO_RESET:
+	case TODO_REVERT:
+	case TODO_REWORD:
+	case TODO_SQUASH:
+		abbrev_oid_in_line(r, line, &p);
+		break;
+
+	/*
+	 * Avoid "default" and instead list all the other commands so
+	 * that -Wswitch (which is included in -Wall) warns if a new
+	 * command is added without handling it in this function.
+	 */
+	case TODO_BREAK:
+	case TODO_EXEC:
+	case TODO_LABEL:
+	case TODO_NOOP:
+	case TODO_UPDATE_REF:
+		break;
 	}
-	string_list_clear(&split, 0);
+
+	return true;
 }
 
 static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
@@ -1411,13 +1494,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
 			  repo_git_path_replace(r, &buf, "%s", fname));
 	}
 	while (!strbuf_getline_lf(&buf, f)) {
-		if (starts_with(buf.buf, comment_line_str))
-			continue;
 		strbuf_trim(&buf);
-		if (!buf.len)
-			continue;
-		abbrev_oid_in_line(r, &buf);
-		string_list_append(lines, buf.buf);
+		if (format_todo_line(r, &buf))
+			string_list_append(lines, buf.buf);
 	}
 	fclose(f);
 
-- 
2.54.0.rc1.174.gd833f386ac5.dirty

Re: [PATCH v2 0/2] status: improve rebase todo list parsing

[Phillip Wood]

On 01/05/2026 16:16, Phillip Wood wrote:
> When there is rebase in progress "git status" displays the last couple
> of completed and the next couple of pending commands from the todo
> list. When it does this is tries to abbreviate the object ids of
> the commits to be picked. Unfortunately it does not abbreviate the
> object ids when the line starts with "fixup -C" or "merge -C". It
> also mistakenly replaces the refname in "reset main" and "update-ref
> refs/heads/main" with the object id that the ref points to.
> 
> This series fixes that. The first patch factors out the sequencer
> code that parses the command names in the todo list. The second patch
> uses that function in "git status" to parse the command names so that
> it knows whether the line may contain "-C" and whether there is an
> object id that should be abbreviated.
> 
> Thanks to Elijah and Patrick for their comments in V1.

Sorry Tian, I'd forgotten that you'd commented on these patches as well 
- thanks for your comments too.

Phillip

> 
> Changes since V1:
> 
> Patch 1 - Expanded commit message and added a code comment.
> 
> Patch 2 - Fixed some typos, added a code comment and clarified that -Wswitch
>            is included by -Wall.
> 
> Base-Commit: 8c9303b1ffae5b745d1b0a1f98330cf7944d8db0
> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fimprove-status-todo-list-parsing%2Fv2
> View-Changes-At: https://github.com/phillipwood/git/compare/8c9303b1f...b80bc1e0a
> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/improve-status-todo-list-parsing/v2
> 
> 
> Phillip Wood (2):
>    sequencer: factor out parsing of todo commands
>    status: improve rebase todo list parsing
> 
>   sequencer.c            |  45 +++++++++-----
>   sequencer.h            |   8 +++
>   t/t7512-status-help.sh |  74 +++++++++++++++--------
>   wt-status.c            | 131 +++++++++++++++++++++++++++++++++--------
>   4 files changed, 191 insertions(+), 67 deletions(-)
> 
> Range-diff against v1:
> 1:  3d5135a719 ! 1:  d27dddff93 sequencer: factor out parsing of todo commands
>      @@ Metadata
>        ## Commit message ##
>           sequencer: factor out parsing of todo commands
>       
>      -    Move the code that parses todo commands into a separate function so that
>      -    it can be shared with "git status" in the next commit.
>      +    Move the code that parses todo commands into a separate function so
>      +    that it can be shared with "git status" in the next commit. As we
>      +    know the input is NUL terminated we do not pass a pointer to the end
>      +    of the line and instead test for a blank line by looking for NUL, CR
>      +    LF, or LF. We use starts_with() instead of starts_with_mem() for the
>      +    same reason. This results in slightly different behavior when there
>      +    a CR at the start of the line that is not followed by LF. Previously
>      +    such a line was treated as a comment rather than an invalid line.
>       
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>       
>      @@ sequencer.h: int read_author_script(const char *path, char **name, char **email,
>        int write_basic_state(struct replay_opts *opts, const char *head_name,
>        		      struct commit *onto, const struct object_id *orig_head);
>        void sequencer_post_commit_cleanup(struct repository *r, int verbose);
>      ++
>      ++/*
>      ++ * Try to parse the todo command pointed to by *p. On success sets cmd,
>      ++ * advances p and returns true. On failure returns false, leaves p and
>      ++ * cmd unchanged.
>      ++ */
>       +bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd);
>      ++
>        int sequencer_get_last_command(struct repository* r,
>        			       enum replay_action *action);
>        int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
> 2:  d20dc1f655 ! 2:  b80bc1e0a2 status: improve rebase todo list parsing
>      @@ Commit message
>       
>           When there is rebase in progress "git status" displays the last couple
>           of completed and the next couple of pending commands from the todo
>      -    list. When it does this is tries to abbreviate the object ids of
>      +    list. When it does this it tries to abbreviate the object ids of
>           the commits to be picked. Unfortunately it does not abbreviate the
>           object ids when the line starts with "fixup -C" or "merge -C". It
>           also mistakenly replaces the refname in "reset main" and "update-ref
>      @@ Commit message
>           wider variety of commands. Only the pending commands in the tests
>           are changed to avoid removing existing coverage.
>       
>      +    Helped-by: Elijah Newren <newren@gmail.com>
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>       
>        ## t/t7512-status-help.sh ##
>      @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
>        	return split_in_progress;
>       +}
>       +
>      ++/*
>      ++ * If the whitespace-delimited token starting at or just after *pp *
>      ++ * is a hex object id that is longer than its default abbreviation, *
>      ++ * abbreviate it in-place, shrinking `line` accordingly. On return
>      ++ * *pp points one past the (possibly abbreviated) token. Leaves both
>      ++ * `line` and *pp-advanced-past-the-token unchanged in all other cases
>      ++ * (non-hex token, unresolvable, or a refname that happens to consist
>      ++ * only of hex digits).
>      ++ */
>       +static void abbrev_oid_in_line(struct repository *r,
>       +			       struct strbuf *line, char **pp)
>       +{
>      @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
>       +	p += strspn(p, " \t");
>       +	end_of_object_name = p + strcspn(p, " \t");
>       +	/*
>      -+	 * The for "merge" and "reset" the object name may be a label or
>      ++	 * For "merge" and "reset" the object name may be a label or
>       +	 * ref rather than a hex object id. Only abbreviate the object
>       +	 * name if it is a hex object id.
>       +	 */
>      @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
>       +	*pp = end_of_object_name;
>       +}
>       +
>      -+static void skip_dash_c(char **pp) {
>      ++static void skip_dash_c(char **pp)
>      ++{
>       +	char *p = *pp;
>       +
>       +	p += strspn(p, " \t");
>      -+	/* The (void) cast is required to silence -Wunused_value */
>      ++	/* The (void) cast is required to silence -Wunused-value */
>       +	(void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
>       +	*pp = p;
>        }
>      @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
>       +
>       +	/*
>       +	 * Avoid "default" and instead list all the other commands so
>      -+	 * that -Wswitch warns if a new command is added without handling
>      -+	 * it in this function.
>      ++	 * that -Wswitch (which is included in -Wall) warns if a new
>      ++	 * command is added without handling it in this function.
>       +	 */
>       +	case TODO_BREAK:
>       +	case TODO_EXEC:

Re: [PATCH v2 2/2] status: improve rebase todo list parsing

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> +static void abbrev_oid_in_line(struct repository *r,
> +			       struct strbuf *line, char **pp)
> +{
> ...
> +	have_oid = !repo_get_oid(r, p, &oid);
> +	*end_of_object_name = saved;
> +	if (!have_oid)
> +		goto out; /* object name was a label */

Can there be a label "deadbeef123" that is unrelated to an object whose
object name happens to abbreviate to "deadbeef123"?

> +	case TODO_MERGE:
> +		skip_dash_c(&p);
> +		while (true) {
> +			p += strspn(p, " \t");
> +			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
> +				break;
> +			abbrev_oid_in_line(r, line, &p);
> +		}
> +		break;

What does this loop do?  A "merge" command may look like "merge
[[-C|-c] <commit>] <label>", and we give each whitespace-separated
token to abbrev_oid_in_line()?  Would "<label>" that is ambiguous
cause an issue?  You may want to limit the scope of what the loop
does a bit, e.g., massage only the token after -C/-c, or something?

> +	case TODO_FIXUP:
> +		skip_dash_c(&p);
> +		/* fallthrough */
> +	case TODO_DROP:
> +	case TODO_EDIT:
> +	case TODO_PICK:
> +	case TODO_RESET:

Doesn't RESET also take a <label>?  And if it happens to be the same
as an abbreviated object name, e.g., "deadbeef123", of an unrelated
object, would wt-status say "reset deadbeef1", causing a mismatch?
If this is indeed an issue, would moving this to the "no-op" section
below, next to TODO_LABEL, solve it?

> +	case TODO_REVERT:
> +	case TODO_REWORD:
> +	case TODO_SQUASH:
> +		abbrev_oid_in_line(r, line, &p);
> +		break;
> +
> +	/*
> +	 * Avoid "default" and instead list all the other commands so
> +	 * that -Wswitch (which is included in -Wall) warns if a new
> +	 * command is added without handling it in this function.
> +	 */
> +	case TODO_BREAK:
> +	case TODO_EXEC:
> +	case TODO_LABEL:
> +	case TODO_NOOP:
> +	case TODO_UPDATE_REF:
> +		break;
>  	}
> -	string_list_clear(&split, 0);
> +
> +	return true;
>  }

Re: [PATCH v2 2/2] status: improve rebase todo list parsing

[Phillip Wood]

Hi Junio

On 31/05/2026 01:46, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> +static void abbrev_oid_in_line(struct repository *r,
>> +			       struct strbuf *line, char **pp)
>> +{
>> ...
>> +	have_oid = !repo_get_oid(r, p, &oid);
>> +	*end_of_object_name = saved;
>> +	if (!have_oid)
>> +		goto out; /* object name was a label */
> 
> Can there be a label "deadbeef123" that is unrelated to an object whose
> object name happens to abbreviate to "deadbeef123"?

In theory yes, but I had assumed it was so unlikely to happen that we 
could ignore it. If we want to be more careful then we could add a "bool 
maybe_label" argument for commands that accept a label or a revision and 
check if "refs/rewritten/$object_name" exists before trying repo_get_oid().

>> +	case TODO_MERGE:
>> +		skip_dash_c(&p);
>> +		while (true) {
>> +			p += strspn(p, " \t");
>> +			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
>> +				break;
>> +			abbrev_oid_in_line(r, line, &p);
>> +		}
>> +		break;
> 
> What does this loop do?  A "merge" command may look like "merge
> [[-C|-c] <commit>] <label>", and we give each whitespace-separated
> token to abbrev_oid_in_line()?  Would "<label>" that is ambiguous
> cause an issue?  You may want to limit the scope of what the loop
> does a bit, e.g., massage only the token after -C/-c, or something?

The parents can be a label or any revision so we want to abbreviate the 
parent if it is a hex object id. The same is true for "reset" below.

Thanks

Phillip

> 
>> +	case TODO_FIXUP:
>> +		skip_dash_c(&p);
>> +		/* fallthrough */
>> +	case TODO_DROP:
>> +	case TODO_EDIT:
>> +	case TODO_PICK:
>> +	case TODO_RESET:
> 
> Doesn't RESET also take a <label>?  And if it happens to be the same
> as an abbreviated object name, e.g., "deadbeef123", of an unrelated
> object, would wt-status say "reset deadbeef1", causing a mismatch?
> If this is indeed an issue, would moving this to the "no-op" section
> below, next to TODO_LABEL, solve it?
> 
>> +	case TODO_REVERT:
>> +	case TODO_REWORD:
>> +	case TODO_SQUASH:
>> +		abbrev_oid_in_line(r, line, &p);
>> +		break;
>> +
>> +	/*
>> +	 * Avoid "default" and instead list all the other commands so
>> +	 * that -Wswitch (which is included in -Wall) warns if a new
>> +	 * command is added without handling it in this function.
>> +	 */
>> +	case TODO_BREAK:
>> +	case TODO_EXEC:
>> +	case TODO_LABEL:
>> +	case TODO_NOOP:
>> +	case TODO_UPDATE_REF:
>> +		break;
>>   	}
>> -	string_list_clear(&split, 0);
>> +
>> +	return true;
>>   }
> 

Re: [PATCH v2 2/2] status: improve rebase todo list parsing

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Junio
>
> On 31/05/2026 01:46, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> +static void abbrev_oid_in_line(struct repository *r,
>>> +			       struct strbuf *line, char **pp)
>>> +{
>>> ...
>>> +	have_oid = !repo_get_oid(r, p, &oid);
>>> +	*end_of_object_name = saved;
>>> +	if (!have_oid)
>>> +		goto out; /* object name was a label */
>> 
>> Can there be a label "deadbeef123" that is unrelated to an object whose
>> object name happens to abbreviate to "deadbeef123"?
>
> In theory yes, but I had assumed it was so unlikely to happen that we 
> could ignore it. If we want to be more careful then we could add a "bool 
> maybe_label" argument for commands that accept a label or a revision and 
> check if "refs/rewritten/$object_name" exists before trying repo_get_oid().

To me, how rare the possibility of such a bug happening is of
secondary importance.  What affects the decision more is when the
"rare" failure happens, if it is immediately obvious to the user,
and if the user may be further harmed badly if they used the wrong
information given by the tool due to such a "rare" failure.

It would be a huge plus if the workaround, when such a "rare"
failure triggers, would be immediately obvious to the user.

What we do not want to see is that the tool to create a wrong
result, cascading into more problems, silently.  In a sense, it is
even worse if such a bug triggers only rarely, because it would mean
that the users always have to be on the lookout.

Having said all that.

I suspect that the OID in the output generated by "status" after it
parses rebase "todo list" is merely meant as an eye candy, and the
users do not _use_ it to decide further actions based on them.

Or do people stare at "git status" output, find an interesting
object name and go "git show" on it or something?  If not, then even
if such a failure were not rare, it would be OK.  We may however
want to record i as a limitation of the current implementation in
the end-user facing documentation, though.

Thanks.

[PATCH v3 0/2] status: improve rebase todo list parsing

[Phillip Wood]

When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this is tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to.

This series fixes that. The first patch factors out the sequencer
code that parses the command names in the todo list. The second patch
uses that function in "git status" to parse the command names so that
it knows whether the line may contain "-C" and whether there is an
object id that should be abbreviated.

Thanks to Junio for his comments on V2.

Changes since V2:

Patch 2 - Check if the object name is a label before trying to
          abbreviate it.

Note that a number of the CI jobs fail[1] due to the rather old base,
but a test merge of this branch with "master" passes[2]

[1] https://github.com/phillipwood/git/actions/runs/27906115900
[2] https://github.com/phillipwood/git/actions/runs/27908204055

Changes since V1:

Patch 1 - Expanded commit message and added a code comment.

Patch 2 - Fixed some typos, added a code comment and clarified that -Wswitch
          is included by -Wall.

Base-Commit: 8c9303b1ffae5b745d1b0a1f98330cf7944d8db0
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fimprove-status-todo-list-parsing%2Fv3
View-Changes-At: https://github.com/phillipwood/git/compare/8c9303b1f...b3514e9b1
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/improve-status-todo-list-parsing/v3


Phillip Wood (2):
  sequencer: factor out parsing of todo commands
  status: improve rebase todo list parsing

 sequencer.c            |  45 ++++++++----
 sequencer.h            |   8 +++
 t/t7512-status-help.sh |  74 +++++++++++++-------
 wt-status.c            | 154 +++++++++++++++++++++++++++++++++--------
 4 files changed, 213 insertions(+), 68 deletions(-)

Range-diff against v2:
1:  d27dddff931 = 1:  d27dddff931 sequencer: factor out parsing of todo commands
2:  b80bc1e0a29 ! 2:  b3514e9b1c9 status: improve rebase todo list parsing
    @@ Commit message
         the commits to be picked. Unfortunately it does not abbreviate the
         object ids when the line starts with "fixup -C" or "merge -C". It
         also mistakenly replaces the refname in "reset main" and "update-ref
    -    refs/heads/main" with the object id that the ref points to. Use
    -    the function added in the last commit to parse the command name and
    -    only try to abbreviate the argument for commands that take an object
    -    id. When trying to abbreviate an object id, only replace the object
    -    name if it starts with the abbreviated object id so that labels or
    -    branch names that contain only hex digits are left unchanged.
    +    refs/heads/main" with the object id that the ref points to.
    +
    +    Fix this by using the function added in the last commit to parse the
    +    command name and only try to abbreviate the argument for commands that
    +    take an object id. If a command accepts a label then try to resolve the
    +    object name as a label first and only if that fails try to resolve it
    +    as an object_id. When trying to abbreviate an object id, only replace
    +    the object name if it starts with the abbreviated object id so that
    +    tag or branch names that contain only hex digits are left unchanged.
     
         Comments are now processed after stripping any leading
         whitespace from the line. This matches what the sequencer does in
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +}
     +
     +/*
    -+ * If the whitespace-delimited token starting at or just after *pp *
    -+ * is a hex object id that is longer than its default abbreviation, *
    ++ * If the whitespace-delimited token starting at or just after *pp
    ++ * is a hex object id that is longer than its default abbreviation,
     + * abbreviate it in-place, shrinking `line` accordingly. On return
     + * *pp points one past the (possibly abbreviated) token. Leaves both
     + * `line` and *pp-advanced-past-the-token unchanged in all other cases
    -+ * (non-hex token, unresolvable, or a refname that happens to consist
    -+ * only of hex digits).
    ++ * (non-hex token, label name, unresolvable, or a refname that happens
    ++ * to consist only of hex digits).
     + */
    -+static void abbrev_oid_in_line(struct repository *r,
    -+			       struct strbuf *line, char **pp)
    ++static void abbrev_oid_in_line(struct repository *r, struct strbuf *scratch,
    ++			       struct strbuf *line, bool maybe_label, char **pp)
     +{
     +	char *p = *pp;
     +	char *end_of_object_name, saved;
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +	for (const char *q = p; q < end_of_object_name; q++) {
     +		if (!isxdigit(*q))
     +			goto out;
    ++	}
    ++	if (maybe_label) {
    ++		strbuf_reset(scratch);
    ++		strbuf_addf(scratch, "refs/rewritten/%.*s",
    ++			    (int)(end_of_object_name - p), p);
    ++		if (refs_ref_exists(get_main_ref_store(r), scratch->buf))
    ++			goto out; /* object name was a label */
     +	}
     +	saved = *end_of_object_name;
     +	*end_of_object_name = '\0';
     +	have_oid = !repo_get_oid(r, p, &oid);
     +	*end_of_object_name = saved;
     +	if (!have_oid)
    -+		goto out; /* object name was a label */
    ++		goto out; /* invalid object name */
     +	abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
     +	if (!starts_with(p, abbrev))
     +		goto out; /* object name was a refname containing only xdigits */
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +	*pp = end_of_object_name;
     +}
     +
    -+static void skip_dash_c(char **pp)
    ++/* Skip "[ \t]*(-[cC])?", returns true if "-c/-C" was skipped. */
    ++static bool skip_dash_c(char **pp)
     +{
    ++	bool ret;
     +	char *p = *pp;
     +
     +	p += strspn(p, " \t");
    -+	/* The (void) cast is required to silence -Wunused-value */
    -+	(void)(skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p));
    ++	ret = skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p);
     +	*pp = p;
    ++
    ++	return ret;
      }
      
      /*
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     -		strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
     -		for (size_t i = 2; i < split.nr; i++)
     -			strbuf_addf(line, " %s", split.items[i].string);
    +-	}
    +-	string_list_clear(&split, 0);
     +	enum todo_command cmd;
    ++	struct strbuf scratch = STRBUF_INIT;
     +	char *p = line->buf;
     +
     +	if (!sequencer_parse_todo_command((const char**)&p, &cmd))
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +	case TODO_COMMENT:
     +		return false;
     +
    -+	case TODO_MERGE:
    -+		skip_dash_c(&p);
    ++	case TODO_MERGE: {
    ++		/*
    ++		 * The argument to -C cannot be a label, but the parents
    ++		 * can be labels.
    ++		 */
    ++		bool maybe_label = !skip_dash_c(&p);
    ++
     +		while (true) {
     +			p += strspn(p, " \t");
     +			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
     +				break;
    -+			abbrev_oid_in_line(r, line, &p);
    ++			abbrev_oid_in_line(r, &scratch, line, maybe_label, &p);
    ++			maybe_label = true;
     +		}
     +		break;
    ++	}
     +
     +	case TODO_FIXUP:
     +		skip_dash_c(&p);
     +		/* fallthrough */
     +	case TODO_DROP:
     +	case TODO_EDIT:
     +	case TODO_PICK:
    -+	case TODO_RESET:
     +	case TODO_REVERT:
     +	case TODO_REWORD:
     +	case TODO_SQUASH:
    -+		abbrev_oid_in_line(r, line, &p);
    ++		abbrev_oid_in_line(r, &scratch, line, false, &p);
     +		break;
     +
    ++	case TODO_RESET:
    ++		abbrev_oid_in_line(r, &scratch, line, true, &p);
    ++		break;
     +	/*
     +	 * Avoid "default" and instead list all the other commands so
     +	 * that -Wswitch (which is included in -Wall) warns if a new
    @@ wt-status.c: static int split_commit_in_progress(struct wt_status *s)
     +	case TODO_NOOP:
     +	case TODO_UPDATE_REF:
     +		break;
    - 	}
    --	string_list_clear(&split, 0);
    ++	}
     +
    ++	strbuf_release(&scratch);
     +	return true;
      }
      
-- 
2.54.0.200.gfd8d68259e3

[PATCH v3 1/2] sequencer: factor out parsing of todo commands

[Phillip Wood]

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

Move the code that parses todo commands into a separate function so
that it can be shared with "git status" in the next commit. As we
know the input is NUL terminated we do not pass a pointer to the end
of the line and instead test for a blank line by looking for NUL, CR
LF, or LF. We use starts_with() instead of starts_with_mem() for the
same reason. This results in slightly different behavior when there
a CR at the start of the line that is not followed by LF. Previously
such a line was treated as a comment rather than an invalid line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 45 ++++++++++++++++++++++++++++++---------------
 sequencer.h |  8 ++++++++
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b7d8dca47f4..b8e860434a8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2625,6 +2625,27 @@ static int is_command(enum todo_command command, const char **bol)
 		return 1;
 	}
 	return 0;
+}
+
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
+{
+	const char *s = *p;
+
+	for (int i = 0; i < TODO_COMMENT; i++)
+		if (is_command(i, p)) {
+			*cmd = i;
+			return true;
+		}
+
+	if (starts_with(s, comment_line_str)) {
+		*cmd = TODO_COMMENT;
+		return true;
+	} else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
+		*cmd = TODO_COMMENT;
+		return true;
+	}
+
+	return false;
 }
 
 static int check_label_or_ref_arg(enum todo_command command, const char *arg)
@@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
 {
 	struct object_id commit_oid;
 	char *end_of_object_name;
-	int i, saved, status, padding;
+	int saved, status, padding;
 
 	item->flags = 0;
 
 	/* left-trim */
 	bol += strspn(bol, " \t");
 
-	if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
-		item->command = TODO_COMMENT;
-		item->commit = NULL;
-		item->arg_offset = bol - buf;
-		item->arg_len = eol - bol;
-		return 0;
-	}
-
-	for (i = 0; i < TODO_COMMENT; i++)
-		if (is_command(i, &bol)) {
-			item->command = i;
-			break;
-		}
-	if (i >= TODO_COMMENT)
+	if (!sequencer_parse_todo_command(&bol, &item->command))
 		return error(_("invalid command '%.*s'"),
 			     (int)strcspn(bol, " \t\r\n"), bol);
+
+	if (item->command == TODO_COMMENT) {
+		item->commit = NULL;
+		item->arg_offset = bol - buf;
+		item->arg_len = eol - bol;
+		return 0;
+	}
 
 	/* Eat up extra spaces/ tabs before object name */
 	padding = strspn(bol, " \t");
diff --git a/sequencer.h b/sequencer.h
index a6fa670c7c1..28fabef926f 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -262,6 +262,14 @@ int read_author_script(const char *path, char **name, char **email, char **date,
 int write_basic_state(struct replay_opts *opts, const char *head_name,
 		      struct commit *onto, const struct object_id *orig_head);
 void sequencer_post_commit_cleanup(struct repository *r, int verbose);
+
+/*
+ * Try to parse the todo command pointed to by *p. On success sets cmd,
+ * advances p and returns true. On failure returns false, leaves p and
+ * cmd unchanged.
+ */
+bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd);
+
 int sequencer_get_last_command(struct repository* r,
 			       enum replay_action *action);
 int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
-- 
2.54.0.200.gfd8d68259e3

[PATCH v3 2/2] status: improve rebase todo list parsing

[Phillip Wood]

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

When there is rebase in progress "git status" displays the last couple
of completed and the next couple of pending commands from the todo
list. When it does this it tries to abbreviate the object ids of
the commits to be picked. Unfortunately it does not abbreviate the
object ids when the line starts with "fixup -C" or "merge -C". It
also mistakenly replaces the refname in "reset main" and "update-ref
refs/heads/main" with the object id that the ref points to.

Fix this by using the function added in the last commit to parse the
command name and only try to abbreviate the argument for commands that
take an object id. If a command accepts a label then try to resolve the
object name as a label first and only if that fails try to resolve it
as an object_id. When trying to abbreviate an object id, only replace
the object name if it starts with the abbreviated object id so that
tag or branch names that contain only hex digits are left unchanged.

Comments are now processed after stripping any leading
whitespace from the line. This matches what the sequencer does in
parse_insn_line(). The existing test cases are updated to test a
wider variety of commands. Only the pending commands in the tests
are changed to avoid removing existing coverage.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t7512-status-help.sh |  74 +++++++++++++-------
 wt-status.c            | 154 +++++++++++++++++++++++++++++++++--------
 2 files changed, 175 insertions(+), 53 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 08e82f79140..aca4b6d3326 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -224,7 +224,7 @@ test_expect_success 'status when splitting a commit' '
 	COMMIT3=$(git rev-parse --short split_commit) &&
 	test_commit four_split main.txt four &&
 	COMMIT4=$(git rev-parse --short split_commit) &&
-	FAKE_LINES="1 edit 2 3" &&
+	FAKE_LINES="reword 1 edit 2 fixup_-C 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
@@ -233,10 +233,10 @@ test_expect_success 'status when splitting a commit' '
 	cat >expected <<EOF &&
 interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
-   pick $COMMIT2 # two_split
+   reword $COMMIT2 # two_split
    edit $COMMIT3 # three_split
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_split
+   fixup -C $COMMIT4 # four_split
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''split_commit'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -297,7 +297,7 @@ test_expect_success 'prepare for several edits' '
 
 
 test_expect_success 'status: (continue first edit) second edit' '
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 drop 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -312,7 +312,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   drop $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -327,7 +327,7 @@ EOF
 
 test_expect_success 'status: (continue first edit) second edit and split' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 squash 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -343,7 +343,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   squash $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -362,7 +362,7 @@ EOF
 
 test_expect_success 'status: (continue first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 fixup 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -378,7 +378,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   fixup $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -393,7 +393,7 @@ EOF
 
 test_expect_success 'status: (amend first edit) second edit' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
+	FAKE_LINES="edit 1 edit 2 fixup_-c 3" &&
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
@@ -409,7 +409,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   fixup -c $COMMIT4 # four_edits
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -460,14 +460,20 @@ EOF
 
 test_expect_success 'status: (amend first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
 	COMMIT4=$(git rev-parse --short several_edits) &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	merge $(git rev-parse main) $(git rev-parse several_edits)
+	EOF
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git commit --amend -m "c" &&
 	git rebase --continue &&
 	git commit --amend -m "d" &&
@@ -477,7 +483,7 @@ Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
 Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+   merge $(git rev-parse --short main) $COMMIT4
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
@@ -525,14 +531,21 @@ EOF
 
 test_expect_success 'status: (split first edit) second edit and split' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
 	COMMIT4=$(git rev-parse --short several_edits) &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	reset $(git rev-parse main)
+	merge -C several_edits topic # title
+	EOF
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git reset HEAD^ &&
 	git add main.txt &&
 	git commit --amend -m "f" &&
@@ -543,8 +556,9 @@ interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+   reset $(git rev-parse --short main)
+   merge -C $COMMIT4 topic # title
   (use "git rebase --edit-todo" to view and edit)
 You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (Once your working directory is clean, run "git rebase --continue")
@@ -563,14 +577,21 @@ EOF
 
 test_expect_success 'status: (split first edit) second edit and amend' '
 	git reset --hard several_edits &&
-	FAKE_LINES="edit 1 edit 2 3" &&
-	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
+	git branch cafe main &&
 	COMMIT2=$(git rev-parse --short several_edits^^) &&
 	COMMIT3=$(git rev-parse --short several_edits^) &&
-	COMMIT4=$(git rev-parse --short several_edits) &&
+	cat >todo <<-EOF &&
+	edit several_edits^^ # two_edits
+	edit several_edits^ # three_edits
+	update-ref refs/heads/main
+	reset cafe
+	EOF
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	git rebase -i HEAD~3 &&
+	(
+		set_replace_editor todo &&
+		git rebase -i HEAD~3
+	) &&
 	git reset HEAD^ &&
 	git add main.txt &&
 	git commit --amend -m "g" &&
@@ -581,8 +602,9 @@ interactive rebase in progress; onto $ONTO
 Last commands done (2 commands done):
    edit $COMMIT2 # two_edits
    edit $COMMIT3 # three_edits
-Next command to do (1 remaining command):
-   pick $COMMIT4 # four_edits
+Next commands to do (2 remaining commands):
+   update-ref refs/heads/main
+   reset cafe
   (use "git rebase --edit-todo" to view and edit)
 You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
   (use "git commit --amend" to amend the current commit)
diff --git a/wt-status.c b/wt-status.c
index 479ccc3304b..4b15bda76f4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1363,6 +1363,71 @@ static int split_commit_in_progress(struct wt_status *s)
 	free(rebase_orig_head);
 
 	return split_in_progress;
+}
+
+/*
+ * If the whitespace-delimited token starting at or just after *pp
+ * is a hex object id that is longer than its default abbreviation,
+ * abbreviate it in-place, shrinking `line` accordingly. On return
+ * *pp points one past the (possibly abbreviated) token. Leaves both
+ * `line` and *pp-advanced-past-the-token unchanged in all other cases
+ * (non-hex token, label name, unresolvable, or a refname that happens
+ * to consist only of hex digits).
+ */
+static void abbrev_oid_in_line(struct repository *r, struct strbuf *scratch,
+			       struct strbuf *line, bool maybe_label, char **pp)
+{
+	char *p = *pp;
+	char *end_of_object_name, saved;
+	const char *abbrev;
+	struct object_id oid;
+	bool have_oid;
+
+	p += strspn(p, " \t");
+	end_of_object_name = p + strcspn(p, " \t");
+	/*
+	 * For "merge" and "reset" the object name may be a label or
+	 * ref rather than a hex object id. Only abbreviate the object
+	 * name if it is a hex object id.
+	 */
+	for (const char *q = p; q < end_of_object_name; q++) {
+		if (!isxdigit(*q))
+			goto out;
+	}
+	if (maybe_label) {
+		strbuf_reset(scratch);
+		strbuf_addf(scratch, "refs/rewritten/%.*s",
+			    (int)(end_of_object_name - p), p);
+		if (refs_ref_exists(get_main_ref_store(r), scratch->buf))
+			goto out; /* object name was a label */
+	}
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	have_oid = !repo_get_oid(r, p, &oid);
+	*end_of_object_name = saved;
+	if (!have_oid)
+		goto out; /* invalid object name */
+	abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
+	if (!starts_with(p, abbrev))
+		goto out; /* object name was a refname containing only xdigits */
+	p += strlen(abbrev);
+	strbuf_remove(line, p - line->buf, end_of_object_name - p);
+	end_of_object_name = p;
+out:
+	*pp = end_of_object_name;
+}
+
+/* Skip "[ \t]*(-[cC])?", returns true if "-c/-C" was skipped. */
+static bool skip_dash_c(char **pp)
+{
+	bool ret;
+	char *p = *pp;
+
+	p += strspn(p, " \t");
+	ret = skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p);
+	*pp = p;
+
+	return ret;
 }
 
 /*
@@ -1371,29 +1436,68 @@ static int split_commit_in_progress(struct wt_status *s)
  * into
  * "pick d6a2f03 some message"
  *
- * The function assumes that the line does not contain useless spaces
- * before or after the command.
+ * Returns false on comment lines, true otherwise
  */
-static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
+static bool format_todo_line(struct repository *r, struct strbuf *line)
 {
-	struct string_list split = STRING_LIST_INIT_DUP;
-	struct object_id oid;
-
-	if (starts_with(line->buf, "exec ") ||
-	    starts_with(line->buf, "x ") ||
-	    starts_with(line->buf, "label ") ||
-	    starts_with(line->buf, "l "))
-		return;
-
-	if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
-	    !repo_get_oid(r, split.items[1].string, &oid)) {
-		strbuf_reset(line);
-		strbuf_addf(line, "%s ", split.items[0].string);
-		strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
-		for (size_t i = 2; i < split.nr; i++)
-			strbuf_addf(line, " %s", split.items[i].string);
-	}
-	string_list_clear(&split, 0);
+	enum todo_command cmd;
+	struct strbuf scratch = STRBUF_INIT;
+	char *p = line->buf;
+
+	if (!sequencer_parse_todo_command((const char**)&p, &cmd))
+		return true; /* keep invalid lines */
+
+	switch (cmd) {
+	case TODO_COMMENT:
+		return false;
+
+	case TODO_MERGE: {
+		/*
+		 * The argument to -C cannot be a label, but the parents
+		 * can be labels.
+		 */
+		bool maybe_label = !skip_dash_c(&p);
+
+		while (true) {
+			p += strspn(p, " \t");
+			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
+				break;
+			abbrev_oid_in_line(r, &scratch, line, maybe_label, &p);
+			maybe_label = true;
+		}
+		break;
+	}
+
+	case TODO_FIXUP:
+		skip_dash_c(&p);
+		/* fallthrough */
+	case TODO_DROP:
+	case TODO_EDIT:
+	case TODO_PICK:
+	case TODO_REVERT:
+	case TODO_REWORD:
+	case TODO_SQUASH:
+		abbrev_oid_in_line(r, &scratch, line, false, &p);
+		break;
+
+	case TODO_RESET:
+		abbrev_oid_in_line(r, &scratch, line, true, &p);
+		break;
+	/*
+	 * Avoid "default" and instead list all the other commands so
+	 * that -Wswitch (which is included in -Wall) warns if a new
+	 * command is added without handling it in this function.
+	 */
+	case TODO_BREAK:
+	case TODO_EXEC:
+	case TODO_LABEL:
+	case TODO_NOOP:
+	case TODO_UPDATE_REF:
+		break;
+	}
+
+	strbuf_release(&scratch);
+	return true;
 }
 
 static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
@@ -1411,13 +1515,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
 			  repo_git_path_replace(r, &buf, "%s", fname));
 	}
 	while (!strbuf_getline_lf(&buf, f)) {
-		if (starts_with(buf.buf, comment_line_str))
-			continue;
 		strbuf_trim(&buf);
-		if (!buf.len)
-			continue;
-		abbrev_oid_in_line(r, &buf);
-		string_list_append(lines, buf.buf);
+		if (format_todo_line(r, &buf))
+			string_list_append(lines, buf.buf);
 	}
 	fclose(f);
 
-- 
2.54.0.200.gfd8d68259e3

Re: [PATCH v2 2/2] status: improve rebase todo list parsing

[Phillip Wood]

On 11/06/2026 17:08, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Hi Junio
>>
>> On 31/05/2026 01:46, Junio C Hamano wrote:
>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>
>>>> +static void abbrev_oid_in_line(struct repository *r,
>>>> +			       struct strbuf *line, char **pp)
>>>> +{
>>>> ...
>>>> +	have_oid = !repo_get_oid(r, p, &oid);
>>>> +	*end_of_object_name = saved;
>>>> +	if (!have_oid)
>>>> +		goto out; /* object name was a label */
>>>
>>> Can there be a label "deadbeef123" that is unrelated to an object whose
>>> object name happens to abbreviate to "deadbeef123"?
>>
>> In theory yes, but I had assumed it was so unlikely to happen that we
>> could ignore it. If we want to be more careful then we could add a "bool
>> maybe_label" argument for commands that accept a label or a revision and
>> check if "refs/rewritten/$object_name" exists before trying repo_get_oid().
> 
> To me, how rare the possibility of such a bug happening is of
> secondary importance.  What affects the decision more is when the
> "rare" failure happens, if it is immediately obvious to the user,
> and if the user may be further harmed badly if they used the wrong
> information given by the tool due to such a "rare" failure.

That's a good point - I should have been clearer that I thought the 
consequences were not serious so and while we wouldn't want to 
misinterpret labels on a regular basis it didn't matter we did so very 
occasionally.

> It would be a huge plus if the workaround, when such a "rare"
> failure triggers, would be immediately obvious to the user.
> 
> What we do not want to see is that the tool to create a wrong
> result, cascading into more problems, silently.  In a sense, it is
> even worse if such a bug triggers only rarely, because it would mean
> that the users always have to be on the lookout.
> 
> Having said all that.
> 
> I suspect that the OID in the output generated by "status" after it
> parses rebase "todo list" is merely meant as an eye candy, and the
> users do not _use_ it to decide further actions based on them.

That's my suspicion as well

> Or do people stare at "git status" output, find an interesting
> object name and go "git show" on it or something?  If not, then even
> if such a failure were not rare, it would be OK.  We may however
> want to record i as a limitation of the current implementation in
> the end-user facing documentation, though.

I've updated the implementation to check for a label before trying to 
abbreviate the object name.

Thanks

Phillip

> 
> Thanks.
> 
> 

Re: [PATCH v3 1/2] sequencer: factor out parsing of todo commands

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Move the code that parses todo commands into a separate function so
> that it can be shared with "git status" in the next commit. As we
> know the input is NUL terminated we do not pass a pointer to the end
> of the line and instead test for a blank line by looking for NUL, CR
> LF, or LF. We use starts_with() instead of starts_with_mem() for the
> same reason. This results in slightly different behavior when there
> a CR at the start of the line that is not followed by LF. Previously
> such a line was treated as a comment rather than an invalid line.

Meaning that the input validation is tighter than before?  I think
it is fine in this case, as I do not see a reason why anybody wants
to use a lone CR as comment introducer.

> +bool sequencer_parse_todo_command(const char **p, enum todo_command *cmd)
> +{
> +	const char *s = *p;
> +
> +	for (int i = 0; i < TODO_COMMENT; i++)
> +		if (is_command(i, p)) {
> +			*cmd = i;
> +			return true;
> +		}
> +
> +	if (starts_with(s, comment_line_str)) {
> +		*cmd = TODO_COMMENT;
> +		return true;
> +	} else if (s[0] == '\n' || (s[0] == '\r' && s[1] == '\n') || !s[0]) {
> +		*cmd = TODO_COMMENT;
> +		return true;
> +	}
> +
> +	return false;
>  }

I notice that the order of noticing concrete comments and comment
lines are swapped relative to the original.  There is no inherently
"natural" order between them, so the change is perfectly OK.  I just
got confused slightly while reading it until I realized that is what
you did.

>  static int check_label_or_ref_arg(enum todo_command command, const char *arg)
> @@ -2716,29 +2737,23 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
>  {
>  	struct object_id commit_oid;
>  	char *end_of_object_name;
> -	int i, saved, status, padding;
> +	int saved, status, padding;
>  
>  	item->flags = 0;
>  
>  	/* left-trim */
>  	bol += strspn(bol, " \t");
>  
> -	if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
> -		item->command = TODO_COMMENT;
> -		item->commit = NULL;
> -		item->arg_offset = bol - buf;
> -		item->arg_len = eol - bol;
> -		return 0;
> -	}
> -
> -	for (i = 0; i < TODO_COMMENT; i++)
> -		if (is_command(i, &bol)) {
> -			item->command = i;
> -			break;
> -		}
> -	if (i >= TODO_COMMENT)
> +	if (!sequencer_parse_todo_command(&bol, &item->command))
>  		return error(_("invalid command '%.*s'"),
>  			     (int)strcspn(bol, " \t\r\n"), bol);
> +
> +	if (item->command == TODO_COMMENT) {
> +		item->commit = NULL;
> +		item->arg_offset = bol - buf;
> +		item->arg_len = eol - bol;
> +		return 0;
> +	}

And the extra stuff that are only relevant to a comment line is
naturally processed by the caller.  OK.

Thanks.  Looking good so far.

Re: [PATCH v3 2/2] status: improve rebase todo list parsing

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When there is rebase in progress "git status" displays the last couple
> of completed and the next couple of pending commands from the todo
> list. When it does this it tries to abbreviate the object ids of
> the commits to be picked. Unfortunately it does not abbreviate the
> object ids when the line starts with "fixup -C" or "merge -C". It
> also mistakenly replaces the refname in "reset main" and "update-ref
> refs/heads/main" with the object id that the ref points to.
>
> Fix this by using the function added in the last commit to parse the
> command name and only try to abbreviate the argument for commands that
> take an object id. If a command accepts a label then try to resolve the
> object name as a label first and only if that fails try to resolve it
> as an object_id. When trying to abbreviate an object id, only replace
> the object name if it starts with the abbreviated object id so that
> tag or branch names that contain only hex digits are left unchanged.

;-)  

Strictly speaking, the original that said "if begins with
exed, x, label, or l, then don't bother" style can be extended
without using the function added in the last commit to do this, but
it certainly is much more pleasant to read the resulting code
presented here that uses parsed command enum and switches on it.

> Comments are now processed after stripping any leading
> whitespace from the line. This matches what the sequencer does in
> parse_insn_line(). The existing test cases are updated to test a
> wider variety of commands. Only the pending commands in the tests
> are changed to avoid removing existing coverage.
>
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> diff --git a/wt-status.c b/wt-status.c
> index 479ccc3304b..4b15bda76f4 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1363,6 +1363,71 @@ static int split_commit_in_progress(struct wt_status *s)
>  	free(rebase_orig_head);
>  
>  	return split_in_progress;
> +}
> +
> +/*
> + * If the whitespace-delimited token starting at or just after *pp
> + * is a hex object id that is longer than its default abbreviation,
> + * abbreviate it in-place, shrinking `line` accordingly. On return
> + * *pp points one past the (possibly abbreviated) token. Leaves both
> + * `line` and *pp-advanced-past-the-token unchanged in all other cases
> + * (non-hex token, label name, unresolvable, or a refname that happens
> + * to consist only of hex digits).
> + */
> +static void abbrev_oid_in_line(struct repository *r, struct strbuf *scratch,
> +			       struct strbuf *line, bool maybe_label, char **pp)
> +{
> +	char *p = *pp;
> +	char *end_of_object_name, saved;
> +	const char *abbrev;
> +	struct object_id oid;
> +	bool have_oid;
> +
> +	p += strspn(p, " \t");
> +	end_of_object_name = p + strcspn(p, " \t");
> +	/*
> +	 * For "merge" and "reset" the object name may be a label or
> +	 * ref rather than a hex object id. Only abbreviate the object
> +	 * name if it is a hex object id.
> +	 */
> +	for (const char *q = p; q < end_of_object_name; q++) {
> +		if (!isxdigit(*q))
> +			goto out;
> +	}

OK.  If the string has non hexdigit, it cannot be a raw object name
so there is no point in rewriting.  OK.

> +	if (maybe_label) {
> +		strbuf_reset(scratch);
> +		strbuf_addf(scratch, "refs/rewritten/%.*s",
> +			    (int)(end_of_object_name - p), p);
> +		if (refs_ref_exists(get_main_ref_store(r), scratch->buf))
> +			goto out; /* object name was a label */
> +	}

If it could be a label, then we check if such a label exists, and if
so, we won't modify it.  OK.

> +	saved = *end_of_object_name;
> +	*end_of_object_name = '\0';
> +	have_oid = !repo_get_oid(r, p, &oid);
> +	*end_of_object_name = saved;
> +	if (!have_oid)
> +		goto out; /* invalid object name */

We obviously cannot abbreviate if we cannot even recognize it as an
object name.  OK.

> +	abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
> +	if (!starts_with(p, abbrev))
> +		goto out; /* object name was a refname containing only xdigits */

OK, nice to see sufficient paranoia ;-)

> +	p += strlen(abbrev);
> +	strbuf_remove(line, p - line->buf, end_of_object_name - p);
> +	end_of_object_name = p;

By abbreviating, line->buf only shrinks so we won't risk getting
confused by a realloc() happening under the hood.  Upon entry to
this helper, *pp must be pointing into line->buf, or everything will
go awry but for a file-scope static helper function like this, it
probably is too obvious to anybody that it does not have to be
spelled out.  OK.

> +out:
> +	*pp = end_of_object_name;
> +}


> +/* Skip "[ \t]*(-[cC])?", returns true if "-c/-C" was skipped. */
> +static bool skip_dash_c(char **pp)
> +{
> +	bool ret;
> +	char *p = *pp;
> +
> +	p += strspn(p, " \t");
> +	ret = skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p);
> +	*pp = p;
> +
> +	return ret;
>  }

OK.

> @@ -1371,29 +1436,68 @@ static int split_commit_in_progress(struct wt_status *s)
>   * into
>   * "pick d6a2f03 some message"
>   *
> - * The function assumes that the line does not contain useless spaces
> - * before or after the command.
> + * Returns false on comment lines, true otherwise
>   */
> -static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
> +static bool format_todo_line(struct repository *r, struct strbuf *line)
>  {
> -	struct string_list split = STRING_LIST_INIT_DUP;
> -	struct object_id oid;
> -
> -	if (starts_with(line->buf, "exec ") ||
> -	    starts_with(line->buf, "x ") ||
> -	    starts_with(line->buf, "label ") ||
> -	    starts_with(line->buf, "l "))
> -		return;
> -
> -	if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
> -	    !repo_get_oid(r, split.items[1].string, &oid)) {
> -		strbuf_reset(line);
> -		strbuf_addf(line, "%s ", split.items[0].string);
> -		strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
> -		for (size_t i = 2; i < split.nr; i++)
> -			strbuf_addf(line, " %s", split.items[i].string);
> -	}
> -	string_list_clear(&split, 0);

We essentially said, "do not molest exec and label, but everything
else, as long as there are two (or more) tokens and the second token
looks like an object name, replace it with its abbreviation",
regardless of what the actual command was.  Now we do the right
thing by ...

> +	enum todo_command cmd;
> +	struct strbuf scratch = STRBUF_INIT;
> +	char *p = line->buf;
> +
> +	if (!sequencer_parse_todo_command((const char**)&p, &cmd))
> +		return true; /* keep invalid lines */

... parsing out what the line is about, and ...

> +	switch (cmd) {

... switching on it, to make it clear that we cover all the cases
known to us (and the code will be maintained like so, by not having
a "default" arm).

> +	case TODO_COMMENT:
> +		return false;
> +
> +	case TODO_MERGE: {
> +		/*
> +		 * The argument to -C cannot be a label, but the parents
> +		 * can be labels.
> +		 */
> +		bool maybe_label = !skip_dash_c(&p);
> +
> +		while (true) {
> +			p += strspn(p, " \t");
> +			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
> +				break;
> +			abbrev_oid_in_line(r, &scratch, line, maybe_label, &p);
> +			maybe_label = true;
> +		}
> +		break;
> +	}
> +
> +	case TODO_FIXUP:
> +		skip_dash_c(&p);
> +		/* fallthrough */

Fixup always refers to raw object ID and never a label, so it
would be OK to just skip -c/-C here ...

> +	case TODO_DROP:
> +	case TODO_EDIT:
> +	case TODO_PICK:
> +	case TODO_REVERT:
> +	case TODO_REWORD:
> +	case TODO_SQUASH:

... and pass "false" for "maybe_label".  OK.

> +		abbrev_oid_in_line(r, &scratch, line, false, &p);
> +		break;
> +
> +	case TODO_RESET:
> +		abbrev_oid_in_line(r, &scratch, line, true, &p);
> +		break;
> +	/*
> +	 * Avoid "default" and instead list all the other commands so
> +	 * that -Wswitch (which is included in -Wall) warns if a new
> +	 * command is added without handling it in this function.
> +	 */
> +	case TODO_BREAK:
> +	case TODO_EXEC:
> +	case TODO_LABEL:
> +	case TODO_NOOP:
> +	case TODO_UPDATE_REF:
> +		break;
> +	}
> +
> +	strbuf_release(&scratch);
> +	return true;
>  }
>  
>  static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
> @@ -1411,13 +1515,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
>  			  repo_git_path_replace(r, &buf, "%s", fname));
>  	}
>  	while (!strbuf_getline_lf(&buf, f)) {
> -		if (starts_with(buf.buf, comment_line_str))
> -			continue;
>  		strbuf_trim(&buf);
> -		if (!buf.len)
> -			continue;
> -		abbrev_oid_in_line(r, &buf);
> -		string_list_append(lines, buf.buf);
> +		if (format_todo_line(r, &buf))
> +			string_list_append(lines, buf.buf);
>  	}
>  	fclose(f);

This loop got much nicer than the original.

Looks good.

Thanks.

Re: [PATCH v3 2/2] status: improve rebase todo list parsing

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> +	if (!sequencer_parse_todo_command((const char**)&p, &cmd))

Style.  Missing SP between "char" and "**".