| Message ID | pull.1531.v2.git.1683965487.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix two rebase bugs related to total_nr | expand |
Hi Dscho On 13/05/2023 09:11, Johannes Schindelin via GitGitGadget wrote: > 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. This version looks good to me, thanks for re-rolling Phillip > 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 > +' > + >
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 +' +