[PATCH v2 0/2] Fix two rebase bugs related to total_nr

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

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
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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