Fix two rebase bugs related to total_nr

I recently picked up work where I regularly rebase large branch thickets consisting of thousands of commits. During those rebases, I could not fail to notice that the progress initially showed a total number around 2,100, when the actual number of commands was more like 1,850. And indeed, when resuming the rebase after being interrupted due to a break command or due to a merge conflict, the progress showed the correct number!

So I set out to fix this, stumbling over an incorrect use of total_nr in the --update-refs code, so I fixed that, too.

Note: These patches apply cleanly on top of ds/rebase-update-ref as well as on top of the current main branch.

Johannes Schindelin (2):
rebase --update-refs: fix loops
rebase -r: fix the total number shown in the progress

 sequencer.c              | 16 ++++++++++------
 t/t3430-rebase-merges.sh |  7 +++++++
 2 files changed, 17 insertions(+), 6 deletions(-)

base-commit: 4611884ea883908a9638cafbd824c401c41cf7f6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1531%2Fdscho%2Ffix-rebase-i-progress-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1531/dscho/fix-rebase-i-progress-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1531

gitgitgadget

From: Johannes Schindelin <[email protected]>

The total_nr field in the todo_list structure merely serves display purposes, and should only be used when generating the progress message.

In these two instances, however, we want to loop over all of the commands in the parsed rebase script. The loop limit therefore needs to be nr, which refers to the count of commands in the current todo_list.

This is important because the two numbers, nr and total_nr can differ wildly, e.g. due to total_nr _not_ counting comments or empty lines, while nr skips any commands that already moved from the git-rebase-todo file to the done file.

---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 5f22b7cd377..f5d89abdc5e 100644
@@ -4202,7 +4202,7 @@ void todo_list_filter_update_refs(struct repository *r,
 		if (!is_null_oid(&rec->after))
 			continue;
 
-		for (j = 0; !found && j < todo_list->total_nr; j++) {
+		for (j = 0; !found && j < todo_list->nr; j++) {
 			struct todo_item *item = &todo_list->items[j];
 			const char *arg = todo_list->buf.buf + item->arg_offset;
 

@@ -4232,7 +4232,7 @@ void todo_list_filter_update_refs(struct repository *r,
 	 * For each todo_item, check if its ref is in the update_refs list.
 	 * If not, then add it as an un-updated ref.
 	 */
-	for (i = 0; i < todo_list->total_nr; i++) {
+	for (i = 0; i < todo_list->nr; i++) {
 		struct todo_item *item = &todo_list->items[i];
 		const char *arg = todo_list->buf.buf + item->arg_offset;
 		int j, found = 0;

gitgitgadget

From: Johannes Schindelin <[email protected]>

For regular, non---rebase-merges runs, there is very little work to do for the parser when determining the total number of commands in a rebase
script: it is simply the number of lines after stripping the commented lines and then trimming the trailing empty line, if any.

The --rebase-merges mode complicates things by introducing empty lines and comments in the middle of the script. These should _not_ be counted as commands, and indeed, when an interactive rebase is interrupted and subsequently resumed, the total number of commands can magically shrink, sometimes dramatically.

The reason for this strange behavior is that empty lines _are_ counted in edit_todo_list() (but not the comments, as they are stripped via strbuf_stripspace(..., 1), which is a bug.

Let's fix this so that the correct total number is shown from the get-go, by carefully adjusting it according to what's in the rebase script. Extra care needs to be taken in case the user edits the script:
the number of commands might be different after the user edited than beforehand.

Note: Even though commented lines are skipped in edit_todo_list(), we still need to handle TODO_COMMENT items by decrementing the already-incremented total_nr again: empty lines are also marked as TODO_COMMENT.

---
 sequencer.c              | 12 ++++++++----
 t/t3430-rebase-merges.sh |  7 +++++++
 2 files changed, 15 insertions(+), 4 deletions(-)

index f5d89abdc5e..46dd07df0f2 100644
@@ -2609,7 +2609,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
 	char *p = buf, *next_p;
 	int i, res = 0, fixup_okay = file_exists(rebase_path_done());
 
-	todo_list->current = todo_list->nr = 0;
+	todo_list->current = todo_list->nr = todo_list->total_nr = 0;
 
 	for (i = 1; *p; i++, p = next_p) {
 		char *eol = strchrnul(p, '\n');

@@ -2628,7 +2628,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
 			item->arg_offset = p - buf;
 			item->arg_len = (int)(eol - p);
 			item->commit = NULL;
-		}
+		} else if (item->command == TODO_COMMENT)
+			todo_list->total_nr--;
 
 		if (fixup_okay)
 			; /* do nothing */

@@ -6039,7 +6040,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	struct todo_list new_todo = TODO_LIST_INIT;
 	struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
 	struct object_id oid = onto->object.oid;
-	int res;
+	int new_total_nr, res;
 
 	find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
 

@@ -6066,6 +6067,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error(_("nothing to do"));
 	}
 
+	new_total_nr = todo_list->total_nr - count_commands(todo_list);
 	res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
 			     shortonto, flags);
 	if (res == -1)

@@ -6088,11 +6090,13 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return -1;
 	}
 
+	new_total_nr += count_commands(&new_todo);
+	new_todo.total_nr = new_total_nr;
+
 	/* Expand the commit IDs */
 	todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
 	strbuf_swap(&new_todo.buf, &buf2);
 	strbuf_release(&buf2);
-	new_todo.total_nr -= new_todo.nr;
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
 		BUG("invalid todo list after expanding IDs:\n%s",
 		    new_todo.buf.buf);

index f351701fec2..8da99a075dc 100755
@@ -517,4 +517,11 @@ test_expect_success '--rebase-merges with message matched with onto label' '
 	EOF
 '
 
+test_expect_success 'progress shows the correct total' '
+	git checkout -b progress H &&
+	git rebase --rebase-merges --force-rebase --verbose A 2> err &&
+	grep "^Rebasing.*14.$" err >progress &&
+	test_line_count = 14 progress
+'
+
 test_done

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

This is a good distinction to keep in mind.

I notice that sequencer.h does not do a very good job at giving guidance on how these members are to be used.

        struct todo_list {
                struct strbuf buf;
                struct todo_item *items;
                int nr, alloc, current;
                int done_nr, total_nr;
        };

The <nr,alloc> tuple lets readers to guess they are tied to the items[] array, so perhaps it is sufficient to give a comment to total_nr member and probably done_nr while we are at it.

Yes.

OK. The items[] array has not just executable insn but also holds comments and NOOP, and <nr,alloc> tuple is used to control its sizing in the usual ALLOC_GROW() way. Because total_nr is used only for progress, it naturally excludes the no-ops. Elements of items[] array are consumed in core by incrementing the current pointer and nr will not update while that is happening, but when the sequencer gives control the user and then takes the control back upon resuming, items[] would contain only the insns that have not been moved to the done list, meaning that 'nr' would shrink. total_nr is compensated by reading the done list and adds its size to 'nr'.

OK, that all makes sense. The whole arrangement sounds like a bit more error prone than necessary (an obvious alternative is to just always keep the whole todo list with "done up to here" pointner) but changing that is not in the scope of these fixes, because such an arrangement wouldn't have prevented this particular bug from happening, as total_nr and nr could still be different due to no-ops.

"Johannes Schindelin via GitGitGadget" <[email protected]>

    "^Rebasing.*/14)$"

    "^Rebasing ([0-9]*/14)$"

Hi Dscho

	if (item->command != TODO_COMMENT)
		todo_list->total_nr++;

	new_todo.total_nr = 0;

OK.

Thanks both for working on the fix and reviewing the patch.

I recently picked up work where I regularly rebase large branch thickets consisting of thousands of commits. During those rebases, I could not fail to notice that the progress initially showed a total number around 2,100, when the actual number of commands was more like 1,850. And indeed, when resuming the rebase after being interrupted due to a break command or due to a merge conflict, the progress showed the correct number!

So I set out to fix this, stumbling over an incorrect use of total_nr in the --update-refs code, so I fixed that, too.

Note: These patches apply cleanly on top of ds/rebase-update-ref as well as on top of the current main branch.

Changes since v1:

* Clarified the pattern expected by the test case in the progress output, as suggested by Junio.
* Simplified the code as suggested by Phillipp, based on the insight that complete_action() (naming is hard!) is only called at the very beginning of a rebase, and therefore there cannot be any already-done commands in the todo list.

Johannes Schindelin (2):
rebase --update-refs: fix loops
rebase -r: fix the total number shown in the progress

 sequencer.c              | 13 ++++++++-----
 t/t3430-rebase-merges.sh |  8 ++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

base-commit: 4611884ea883908a9638cafbd824c401c41cf7f6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1531%2Fdscho%2Ffix-rebase-i-progress-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1531/dscho/fix-rebase-i-progress-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1531

Range-diff vs v1:

 1:  2ac7c7a7c61 = 1:  2ac7c7a7c61 rebase --update-refs: fix loops
 2:  d12d5469f8c ! 2:  cac809bcffd rebase -r: fix the total number shown in the progress

@@ Commit message

## sequencer.c ##
+@@ sequencer.c: void todo_list_release(struct todo_list *todo_list)
+ static struct todo_item *append_new_todo(struct todo_list *todo_list)
+ {
+ ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
+- todo_list->total_nr++;
+ return todo_list->items + todo_list->nr++;
+ }
+
@@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,
char *p = buf, *next_p;
int i, res = 0, fixup_okay = file_exists(rebase_path_done());
@@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,
for (i = 1; *p; i++, p = next_p) {
char *eol = strchrnul(p, '\n');
@@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,
- item->arg_offset = p - buf;
- item->arg_len = (int)(eol - p);
item->commit = NULL;
-- }
-+ } else if (item->command == TODO_COMMENT)
-+ todo_list->total_nr--;
+ }

++ if (item->command != TODO_COMMENT)
++ todo_list->total_nr++;
++
if (fixup_okay)
; /* do nothing */
+ else if (is_fixup(item->command))
@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
- struct todo_list new_todo = TODO_LIST_INIT;
- struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
- struct object_id oid = onto->object.oid;
-- int res;
-+ int new_total_nr, res;
-
- find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
-
-@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
- return error(_("nothing to do"));
- }
-
-+ new_total_nr = todo_list->total_nr - count_commands(todo_list);
- res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
- shortonto, flags);
- if (res == -1)
-@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
- return -1;
- }
-
-+ new_total_nr += count_commands(&new_todo);
-+ new_todo.total_nr = new_total_nr;
-+
- /* Expand the commit IDs */
todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
strbuf_swap(&new_todo.buf, &buf2);
strbuf_release(&buf2);
- new_todo.total_nr -= new_todo.nr;
++ /* Nothing is done yet, and we're reparsing, so let's reset the count */
++ new_todo.total_nr = 0;
if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
BUG("invalid todo list after expanding IDs:\n%s",
new_todo.buf.buf);
@@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges with message matc
+test_expect_success 'progress shows the correct total' '
+ git checkout -b progress H &&
+ git rebase --rebase-merges --force-rebase --verbose A 2> err &&
-+ grep "^Rebasing.*14.$" err >progress &&
++ # Expecting "Rebasing (N/14)" here, no bogus total number
++ grep "^Rebasing.*/14.$" err >progress &&
+ test_line_count = 14 progress
+'
+

gitgitgadget

From: Johannes Schindelin <[email protected]>

The total_nr field in the todo_list structure merely serves display purposes, and should only be used when generating the progress message.

In these two instances, however, we want to loop over all of the commands in the parsed rebase script. The loop limit therefore needs to be nr, which refers to the count of commands in the current todo_list.

This is important because the two numbers, nr and total_nr can differ wildly, e.g. due to total_nr _not_ counting comments or empty lines, while nr skips any commands that already moved from the git-rebase-todo file to the done file.

---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 5f22b7cd377..f5d89abdc5e 100644
@@ -4202,7 +4202,7 @@ void todo_list_filter_update_refs(struct repository *r,
 		if (!is_null_oid(&rec->after))
 			continue;
 
-		for (j = 0; !found && j < todo_list->total_nr; j++) {
+		for (j = 0; !found && j < todo_list->nr; j++) {
 			struct todo_item *item = &todo_list->items[j];
 			const char *arg = todo_list->buf.buf + item->arg_offset;
 

@@ -4232,7 +4232,7 @@ void todo_list_filter_update_refs(struct repository *r,
 	 * For each todo_item, check if its ref is in the update_refs list.
 	 * If not, then add it as an un-updated ref.
 	 */
-	for (i = 0; i < todo_list->total_nr; i++) {
+	for (i = 0; i < todo_list->nr; i++) {
 		struct todo_item *item = &todo_list->items[i];
 		const char *arg = todo_list->buf.buf + item->arg_offset;
 		int j, found = 0;

gitgitgadget

From: Johannes Schindelin <[email protected]>

For regular, non---rebase-merges runs, there is very little work to do for the parser when determining the total number of commands in a rebase
script: it is simply the number of lines after stripping the commented lines and then trimming the trailing empty line, if any.

The --rebase-merges mode complicates things by introducing empty lines and comments in the middle of the script. These should _not_ be counted as commands, and indeed, when an interactive rebase is interrupted and subsequently resumed, the total number of commands can magically shrink, sometimes dramatically.

The reason for this strange behavior is that empty lines _are_ counted in edit_todo_list() (but not the comments, as they are stripped via strbuf_stripspace(..., 1), which is a bug.

Let's fix this so that the correct total number is shown from the get-go, by carefully adjusting it according to what's in the rebase script. Extra care needs to be taken in case the user edits the script:
the number of commands might be different after the user edited than beforehand.

Note: Even though commented lines are skipped in edit_todo_list(), we still need to handle TODO_COMMENT items by decrementing the already-incremented total_nr again: empty lines are also marked as TODO_COMMENT.

---
 sequencer.c              | 9 ++++++---
 t/t3430-rebase-merges.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

index f5d89abdc5e..9fb8ef17618 100644
@@ -2453,7 +2453,6 @@ void todo_list_release(struct todo_list *todo_list)
 static struct todo_item *append_new_todo(struct todo_list *todo_list)
 {
 	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
-	todo_list->total_nr++;
 	return todo_list->items + todo_list->nr++;
 }
 

@@ -2609,7 +2608,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
 	char *p = buf, *next_p;
 	int i, res = 0, fixup_okay = file_exists(rebase_path_done());
 
-	todo_list->current = todo_list->nr = 0;
+	todo_list->current = todo_list->nr = todo_list->total_nr = 0;
 
 	for (i = 1; *p; i++, p = next_p) {
 		char *eol = strchrnul(p, '\n');

@@ -2630,6 +2629,9 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
 			item->commit = NULL;
 		}
 
+		if (item->command != TODO_COMMENT)
+			todo_list->total_nr++;
+
 		if (fixup_okay)
 			; /* do nothing */
 		else if (is_fixup(item->command))

@@ -6092,7 +6094,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
 	strbuf_swap(&new_todo.buf, &buf2);
 	strbuf_release(&buf2);
-	new_todo.total_nr -= new_todo.nr;
+	/* Nothing is done yet, and we're reparsing, so let's reset the count */
+	new_todo.total_nr = 0;
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
 		BUG("invalid todo list after expanding IDs:\n%s",
 		    new_todo.buf.buf);

index f351701fec2..499c31a8d9a 100755
@@ -517,4 +517,12 @@ test_expect_success '--rebase-merges with message matched with onto label' '
 	EOF
 '
 
+test_expect_success 'progress shows the correct total' '
+	git checkout -b progress H &&
+	git rebase --rebase-merges --force-rebase --verbose A 2> err &&
+	# Expecting "Rebasing (N/14)" here, no bogus total number
+	grep "^Rebasing.*/14.$" err >progress &&
+	test_line_count = 14 progress
+'
+
 test_done

gitgitgadget

Hi Dscho

This version looks good to me, thanks for re-rolling

Phillip