{"thread":{"id":"59727","subject":"[PATCH 0/2] Fix two rebase bugs related to total_nr","startedAt":"2023-05-10T22:55:59Z","lastAt":"2023-05-14T18:06:20Z","messageCount":11,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"477002","messageId":"pull.1531.git.1683759338.gitgitgadget@gmail.com","threadId":"59727","inReplyTo":null,"subject":"[PATCH 0/2] Fix two rebase bugs related to total_nr","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-10T22:55:36Z","receivedAt":"2023-05-10T22:55:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently picked up work where I regularly rebase large branch thickets\nconsisting of thousands of commits. During those rebases, I could not fail\nto notice that the progress initially showed a total number around 2,100,\nwhen the actual number of commands was more like 1,850. And indeed, when\nresuming the rebase after being interrupted due to a break command or due to\na merge conflict, the progress showed the correct number!\n\nSo I set out to fix this, stumbling over an incorrect use of total_nr in the\n--update-refs code, so I fixed that, too.\n\nNote: These patches apply cleanly on top of ds/rebase-update-ref as well as\non top of the current main branch.\n\nJohannes Schindelin (2):\n  rebase --update-refs: fix loops\n  rebase -r: fix the total number shown in the progress\n\n sequencer.c              | 16 ++++++++++------\n t/t3430-rebase-merges.sh |  7 +++++++\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\n\nbase-commit: 4611884ea883908a9638cafbd824c401c41cf7f6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1531%2Fdscho%2Ffix-rebase-i-progress-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1531/dscho/fix-rebase-i-progress-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1531\n-- \ngitgitgadget\n"},{"id":"477003","messageId":"2ac7c7a7c615db75a46076b58a51d363bc2daf2e.1683759338.git.gitgitgadget@gmail.com","threadId":"59727","inReplyTo":"pull.1531.git.1683759338.gitgitgadget@gmail.com","subject":"[PATCH 1/2] rebase --update-refs: fix loops","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-10T22:55:37Z","receivedAt":"2023-05-10T22:56:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe `total_nr` field in the `todo_list` structure merely serves display\npurposes, and should only be used when generating the progress message.\n\nIn these two instances, however, we want to loop over all of the\ncommands in the parsed rebase script. The loop limit therefore needs to\nbe `nr`, which refers to the count of commands in the current\n`todo_list`.\n\nThis is important because the two numbers, `nr` and `total_nr` can\ndiffer wildly, e.g. due to `total_nr` _not_ counting comments or empty\nlines, while `nr` skips any commands that already moved from the\n`git-rebase-todo` file to the `done` file.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5f22b7cd377..f5d89abdc5e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4202,7 +4202,7 @@ void todo_list_filter_update_refs(struct repository *r,\n \t\tif (!is_null_oid(&rec->after))\n \t\t\tcontinue;\n \n-\t\tfor (j = 0; !found && j < todo_list->total_nr; j++) {\n+\t\tfor (j = 0; !found && j < todo_list->nr; j++) {\n \t\t\tstruct todo_item *item = &todo_list->items[j];\n \t\t\tconst char *arg = todo_list->buf.buf + item->arg_offset;\n \n@@ -4232,7 +4232,7 @@ void todo_list_filter_update_refs(struct repository *r,\n \t * For each todo_item, check if its ref is in the update_refs list.\n \t * If not, then add it as an un-updated ref.\n \t */\n-\tfor (i = 0; i < todo_list->total_nr; i++) {\n+\tfor (i = 0; i < todo_list->nr; i++) {\n \t\tstruct todo_item *item = &todo_list->items[i];\n \t\tconst char *arg = todo_list->buf.buf + item->arg_offset;\n \t\tint j, found = 0;\n-- \ngitgitgadget\n\n"},{"id":"477004","messageId":"d12d5469f8cbc21ce1efbffc8e7569c534b5a305.1683759339.git.gitgitgadget@gmail.com","threadId":"59727","inReplyTo":"pull.1531.git.1683759338.gitgitgadget@gmail.com","subject":"[PATCH 2/2] rebase -r: fix the total number shown in the progress","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-10T22:55:38Z","receivedAt":"2023-05-10T22:56:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nFor regular, non-`--rebase-merges` runs, there is very little work to do\nfor the parser when determining the total number of commands in a rebase\nscript: it is simply the number of lines after stripping the commented\nlines and then trimming the trailing empty line, if any.\n\nThe `--rebase-merges` mode complicates things by introducing empty lines\nand comments in the middle of the script. These should _not_ be counted\nas commands, and indeed, when an interactive rebase is interrupted and\nsubsequently resumed, the total number of commands can magically shrink,\nsometimes dramatically.\n\nThe reason for this strange behavior is that empty lines _are_ counted\nin `edit_todo_list()` (but not the comments, as they are stripped via\n`strbuf_stripspace(..., 1)`, which is a bug.\n\nLet's fix this so that the correct total number is shown from the\nget-go, by carefully adjusting it according to what's in the rebase\nscript. Extra care needs to be taken in case the user edits the script:\nthe number of commands might be different after the user edited than\nbeforehand.\n\nNote: Even though commented lines are skipped in `edit_todo_list()`, we\nstill need to handle `TODO_COMMENT` items by decrementing the\nalready-incremented `total_nr` again: empty lines are also marked as\n`TODO_COMMENT`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c              | 12 ++++++++----\n t/t3430-rebase-merges.sh |  7 +++++++\n 2 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f5d89abdc5e..46dd07df0f2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2609,7 +2609,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \tchar *p = buf, *next_p;\n \tint i, res = 0, fixup_okay = file_exists(rebase_path_done());\n \n-\ttodo_list->current = todo_list->nr = 0;\n+\ttodo_list->current = todo_list->nr = todo_list->total_nr = 0;\n \n \tfor (i = 1; *p; i++, p = next_p) {\n \t\tchar *eol = strchrnul(p, '\\n');\n@@ -2628,7 +2628,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\t\titem->arg_offset = p - buf;\n \t\t\titem->arg_len = (int)(eol - p);\n \t\t\titem->commit = NULL;\n-\t\t}\n+\t\t} else if (item->command == TODO_COMMENT)\n+\t\t\ttodo_list->total_nr--;\n \n \t\tif (fixup_okay)\n \t\t\t; /* do nothing */\n@@ -6039,7 +6040,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tstruct todo_list new_todo = TODO_LIST_INIT;\n \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n \tstruct object_id oid = onto->object.oid;\n-\tint res;\n+\tint new_total_nr, res;\n \n \tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n \n@@ -6066,6 +6067,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error(_(\"nothing to do\"));\n \t}\n \n+\tnew_total_nr = todo_list->total_nr - count_commands(todo_list);\n \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n \t\t\t     shortonto, flags);\n \tif (res == -1)\n@@ -6088,11 +6090,13 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn -1;\n \t}\n \n+\tnew_total_nr += count_commands(&new_todo);\n+\tnew_todo.total_nr = new_total_nr;\n+\n \t/* Expand the commit IDs */\n \ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n \tstrbuf_swap(&new_todo.buf, &buf2);\n \tstrbuf_release(&buf2);\n-\tnew_todo.total_nr -= new_todo.nr;\n \tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n \t\t    new_todo.buf.buf);\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex f351701fec2..8da99a075dc 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -517,4 +517,11 @@ test_expect_success '--rebase-merges with message matched with onto label' '\n \tEOF\n '\n \n+test_expect_success 'progress shows the correct total' '\n+\tgit checkout -b progress H &&\n+\tgit rebase --rebase-merges --force-rebase --verbose A 2> err &&\n+\tgrep \"^Rebasing.*14.$\" err >progress &&\n+\ttest_line_count = 14 progress\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"477009","messageId":"xmqqcz3722gx.fsf@gitster.g","threadId":"59727","inReplyTo":"2ac7c7a7c615db75a46076b58a51d363bc2daf2e.1683759338.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] rebase --update-refs: fix loops","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-10T23:25:50Z","receivedAt":"2023-05-10T23:25:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> The `total_nr` field in the `todo_list` structure merely serves display\n> purposes, and should only be used when generating the progress message.\n\nThis is a good distinction to keep in mind.\n\nI notice that sequencer.h does not do a very good job at giving\nguidance on how these members are to be used.\n\n        struct todo_list {\n                struct strbuf buf;\n                struct todo_item *items;\n                int nr, alloc, current;\n                int done_nr, total_nr;\n        };\n\nThe <nr,alloc> tuple lets readers to guess they are tied to the\nitems[] array, so perhaps it is sufficient to give a comment to\ntotal_nr member and probably done_nr while we are at it.\n\n> In these two instances, however, we want to loop over all of the\n> commands in the parsed rebase script. The loop limit therefore needs to\n> be `nr`, which refers to the count of commands in the current\n> `todo_list`.\n\nYes.\n\n> This is important because the two numbers, `nr` and `total_nr` can\n> differ wildly, e.g. due to `total_nr` _not_ counting comments or empty\n> lines, while `nr` skips any commands that already moved from the\n> `git-rebase-todo` file to the `done` file.\n\nOK.  The items[] array has not just executable insn but also holds\ncomments and NOOP, and <nr,alloc> tuple is used to control its\nsizing in the usual ALLOC_GROW() way.  Because total_nr is used only\nfor progress, it naturally excludes the no-ops.  Elements of items[]\narray are consumed in core by incrementing the current pointer and\nnr will not update while that is happening, but when the sequencer\ngives control the user and then takes the control back upon resuming,\nitems[] would contain only the insns that have not been moved to the\ndone list, meaning that 'nr' would shrink.  total_nr is compensated\nby reading the done list and adds its size to 'nr'.\n\nOK, that all makes sense.  The whole arrangement sounds like a bit\nmore error prone than necessary (an obvious alternative is to just\nalways keep the whole todo list with \"done up to here\" pointner) but\nchanging that is not in the scope of these fixes, because such an\narrangement wouldn't have prevented this particular bug from\nhappening, as total_nr and nr could still be different due to\nno-ops.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 5f22b7cd377..f5d89abdc5e 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4202,7 +4202,7 @@ void todo_list_filter_update_refs(struct repository *r,\n>  \t\tif (!is_null_oid(&rec->after))\n>  \t\t\tcontinue;\n>  \n> -\t\tfor (j = 0; !found && j < todo_list->total_nr; j++) {\n> +\t\tfor (j = 0; !found && j < todo_list->nr; j++) {\n>  \t\t\tstruct todo_item *item = &todo_list->items[j];\n\nThe .total_nr member could be smaller (because it does not count\nnoops) or larger (because it counts already done steps that are not\nin the items[]) than the .nr member, and the old code could have\nmade out-of-bounds access into the items[] array.  It is now\ncorrected.  Excellent.\n\n>  \t\t\tconst char *arg = todo_list->buf.buf + item->arg_offset;\n>  \n> @@ -4232,7 +4232,7 @@ void todo_list_filter_update_refs(struct repository *r,\n>  \t * For each todo_item, check if its ref is in the update_refs list.\n>  \t * If not, then add it as an un-updated ref.\n>  \t */\n> -\tfor (i = 0; i < todo_list->total_nr; i++) {\n> +\tfor (i = 0; i < todo_list->nr; i++) {\n>  \t\tstruct todo_item *item = &todo_list->items[i];\n>  \t\tconst char *arg = todo_list->buf.buf + item->arg_offset;\n>  \t\tint j, found = 0;\n\nDitto.  Will queue.\n\nThanks.\n"},{"id":"477012","messageId":"xmqq8rdv21uz.fsf@gitster.g","threadId":"59727","inReplyTo":"d12d5469f8cbc21ce1efbffc8e7569c534b5a305.1683759339.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] rebase -r: fix the total number shown in the progress","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-10T23:39:00Z","receivedAt":"2023-05-10T23:39:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index f351701fec2..8da99a075dc 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -517,4 +517,11 @@ test_expect_success '--rebase-merges with message matched with onto label' '\n>  \tEOF\n>  '\n>  \n> +test_expect_success 'progress shows the correct total' '\n> +\tgit checkout -b progress H &&\n> +\tgit rebase --rebase-merges --force-rebase --verbose A 2> err &&\n> +\tgrep \"^Rebasing.*14.$\" err >progress &&\n> +\ttest_line_count = 14 progress\n> +'\n\nThis is trying to find output lines that say \"Rebasing (NN/14)\";\nClarifying that we mean the denominator by matching\n\n    \"^Rebasing.*/14)$\"\n\nor even\n\n    \"^Rebasing ([0-9]*/14)$\"\n\nwould have made it easier to grok (even though I know we do not have\n114 steps to rebase in this one ;-).\n\nLooks good.  Will queue.  Thanks.\n"},{"id":"477033","messageId":"b481d212-89c5-5c8e-c99f-6df3909523c7@gmail.com","threadId":"59727","inReplyTo":"d12d5469f8cbc21ce1efbffc8e7569c534b5a305.1683759339.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] rebase -r: fix the total number shown in the progress","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-11T14:13:44Z","receivedAt":"2023-05-11T14:14:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 10/05/2023 23:55, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> For regular, non-`--rebase-merges` runs, there is very little work to do\n> for the parser when determining the total number of commands in a rebase\n> script: it is simply the number of lines after stripping the commented\n> lines and then trimming the trailing empty line, if any.\n> \n> The `--rebase-merges` mode complicates things by introducing empty lines\n> and comments in the middle of the script. These should _not_ be counted\n> as commands, and indeed, when an interactive rebase is interrupted and\n> subsequently resumed, the total number of commands can magically shrink,\n> sometimes dramatically.\n> \n> The reason for this strange behavior is that empty lines _are_ counted\n> in `edit_todo_list()` (but not the comments, as they are stripped via\n> `strbuf_stripspace(..., 1)`, which is a bug.\n> \n> Let's fix this so that the correct total number is shown from the\n> get-go, by carefully adjusting it according to what's in the rebase\n> script. Extra care needs to be taken in case the user edits the script:\n> the number of commands might be different after the user edited than\n> beforehand.\n> \n> Note: Even though commented lines are skipped in `edit_todo_list()`, we\n> still need to handle `TODO_COMMENT` items by decrementing the\n> already-incremented `total_nr` again: empty lines are also marked as\n> `TODO_COMMENT`.\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>   sequencer.c              | 12 ++++++++----\n>   t/t3430-rebase-merges.sh |  7 +++++++\n>   2 files changed, 15 insertions(+), 4 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index f5d89abdc5e..46dd07df0f2 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2609,7 +2609,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>   \tchar *p = buf, *next_p;\n>   \tint i, res = 0, fixup_okay = file_exists(rebase_path_done());\n>   \n> -\ttodo_list->current = todo_list->nr = 0;\n> +\ttodo_list->current = todo_list->nr = todo_list->total_nr = 0;\n>   \n>   \tfor (i = 1; *p; i++, p = next_p) {\n>   \t\tchar *eol = strchrnul(p, '\\n');\n> @@ -2628,7 +2628,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>   \t\t\titem->arg_offset = p - buf;\n>   \t\t\titem->arg_len = (int)(eol - p);\n>   \t\t\titem->commit = NULL;\n> -\t\t}\n> +\t\t} else if (item->command == TODO_COMMENT)\n> +\t\t\ttodo_list->total_nr--;\n\nThis feels a bit fragile, I think it would be better to count the \ncommands properly in the first place rather than adjusting the total \nhere. We could do that by not incrementing \"todo_list->total_nr\" in \nappend_new_todo() and then doing\n\n\tif (item->command != TODO_COMMENT)\n\t\ttodo_list->total_nr++;\n\nhere. We may want to stop counting invalid commands as well by only \ncounting commands whre \"item->command < TODO_COMMENT\".\n\n>   \n>   \t\tif (fixup_okay)\n>   \t\t\t; /* do nothing */\n> @@ -6039,7 +6040,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>   \tstruct todo_list new_todo = TODO_LIST_INIT;\n>   \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n>   \tstruct object_id oid = onto->object.oid;\n> -\tint res;\n> +\tint new_total_nr, res;\n>   \n>   \tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n>   \n> @@ -6066,6 +6067,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>   \t\treturn error(_(\"nothing to do\"));\n>   \t}\n>   \n> +\tnew_total_nr = todo_list->total_nr - count_commands(todo_list);\n>   \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n>   \t\t\t     shortonto, flags);\n>   \tif (res == -1)\n> @@ -6088,11 +6090,13 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>   \t\treturn -1;\n>   \t}\n>   \n> +\tnew_total_nr += count_commands(&new_todo);\n> +\tnew_todo.total_nr = new_total_nr;\n> +\n>   \t/* Expand the commit IDs */\n>   \ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n>   \tstrbuf_swap(&new_todo.buf, &buf2);\n>   \tstrbuf_release(&buf2);\n> -\tnew_todo.total_nr -= new_todo.nr;\n\nI think if we just change this line to\n\t\n\tnew_todo.total_nr = 0;\n\nwe don't need any other changes to this function. This is because \ncomplete_action() is only called at the start of a rebase so we don't \nneed to worry about \"total_nr\" including commands that have already been \nexecuted. The reason we need to set it to zero here is that we re-parse \nthe todo list below which increments \"total_nr\" by the number of \ncommands parsed.\n\nThanks for working on this.\nBest Wishes\n\nPhillip\n\n>   \tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n>   \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n>   \t\t    new_todo.buf.buf);\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index f351701fec2..8da99a075dc 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -517,4 +517,11 @@ test_expect_success '--rebase-merges with message matched with onto label' '\n>   \tEOF\n>   '\n>   \n> +test_expect_success 'progress shows the correct total' '\n> +\tgit checkout -b progress H &&\n> +\tgit rebase --rebase-merges --force-rebase --verbose A 2> err &&\n> +\tgrep \"^Rebasing.*14.$\" err >progress &&\n> +\ttest_line_count = 14 progress\n> +'\n> +\n>   test_done\n"},{"id":"477055","messageId":"xmqqsfc2wxjx.fsf@gitster.g","threadId":"59727","inReplyTo":"b481d212-89c5-5c8e-c99f-6df3909523c7@gmail.com","subject":"Re: [PATCH 2/2] rebase -r: fix the total number shown in the progress","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-11T18:08:34Z","receivedAt":"2023-05-11T18:09:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>> @@ -2609,7 +2609,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>>   \tchar *p = buf, *next_p;\n>>   \tint i, res = 0, fixup_okay = file_exists(rebase_path_done());\n>>   -\ttodo_list->current = todo_list->nr = 0;\n>> +\ttodo_list->current = todo_list->nr = todo_list->total_nr = 0;\n>>     \tfor (i = 1; *p; i++, p = next_p) {\n>>   \t\tchar *eol = strchrnul(p, '\\n');\n>> @@ -2628,7 +2628,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>>   \t\t\titem->arg_offset = p - buf;\n>>   \t\t\titem->arg_len = (int)(eol - p);\n>>   \t\t\titem->commit = NULL;\n>> -\t\t}\n>> +\t\t} else if (item->command == TODO_COMMENT)\n>> +\t\t\ttodo_list->total_nr--;\n>\n> This feels a bit fragile, I think it would be better to count the\n> commands properly in the first place rather than adjusting the total\n> here. We could do that by not incrementing \"todo_list->total_nr\" in\n> append_new_todo() and then doing\n>\n> \tif (item->command != TODO_COMMENT)\n> \t\ttodo_list->total_nr++;\n>\n> here. We may want to stop counting invalid commands as well by only\n> counting commands whre \"item->command < TODO_COMMENT\".\n\nOK.\n\n>> @@ -6088,11 +6090,13 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>>   \t\treturn -1;\n>>   \t}\n>>   +\tnew_total_nr += count_commands(&new_todo);\n>> +\tnew_todo.total_nr = new_total_nr;\n>> +\n>>   \t/* Expand the commit IDs */\n>>   \ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n>>   \tstrbuf_swap(&new_todo.buf, &buf2);\n>>   \tstrbuf_release(&buf2);\n>> -\tnew_todo.total_nr -= new_todo.nr;\n>\n> I think if we just change this line to\n> \t\n> \tnew_todo.total_nr = 0;\n>\n> we don't need any other changes to this function. This is because\n> complete_action() is only called at the start of a rebase so we don't\n> need to worry about \"total_nr\" including commands that have already\n> been executed. The reason we need to set it to zero here is that we\n> re-parse the todo list below which increments \"total_nr\" by the number\n> of commands parsed.\n>\n> Thanks for working on this.\n> Best Wishes\n>\n> Phillip\n\nThanks both for working on the fix and reviewing the patch.\n"},{"id":"477247","messageId":"pull.1531.v2.git.1683965487.gitgitgadget@gmail.com","threadId":"59727","inReplyTo":"pull.1531.git.1683759338.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Fix two rebase bugs related to total_nr","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-13T08:11:24Z","receivedAt":"2023-05-13T08:11:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently picked up work where I regularly rebase large branch thickets\nconsisting of thousands of commits. During those rebases, I could not fail\nto notice that the progress initially showed a total number around 2,100,\nwhen the actual number of commands was more like 1,850. And indeed, when\nresuming the rebase after being interrupted due to a break command or due to\na merge conflict, the progress showed the correct number!\n\nSo I set out to fix this, stumbling over an incorrect use of total_nr in the\n--update-refs code, so I fixed that, too.\n\nNote: These patches apply cleanly on top of ds/rebase-update-ref as well as\non top of the current main branch.\n\nChanges since v1:\n\n * Clarified the pattern expected by the test case in the progress output,\n   as suggested by Junio.\n * Simplified the code as suggested by Phillipp, based on the insight that\n   complete_action() (naming is hard!) is only called at the very beginning\n   of a rebase, and therefore there cannot be any already-done commands in\n   the todo list.\n\nJohannes Schindelin (2):\n  rebase --update-refs: fix loops\n  rebase -r: fix the total number shown in the progress\n\n sequencer.c              | 13 ++++++++-----\n t/t3430-rebase-merges.sh |  8 ++++++++\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\n\nbase-commit: 4611884ea883908a9638cafbd824c401c41cf7f6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1531%2Fdscho%2Ffix-rebase-i-progress-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1531/dscho/fix-rebase-i-progress-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1531\n\nRange-diff vs v1:\n\n 1:  2ac7c7a7c61 = 1:  2ac7c7a7c61 rebase --update-refs: fix loops\n 2:  d12d5469f8c ! 2:  cac809bcffd rebase -r: fix the total number shown in the progress\n     @@ Commit message\n          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n       ## sequencer.c ##\n     +@@ sequencer.c: void todo_list_release(struct todo_list *todo_list)\n     + static struct todo_item *append_new_todo(struct todo_list *todo_list)\n     + {\n     + \tALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);\n     +-\ttodo_list->total_nr++;\n     + \treturn todo_list->items + todo_list->nr++;\n     + }\n     + \n      @@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n       \tchar *p = buf, *next_p;\n       \tint i, res = 0, fixup_okay = file_exists(rebase_path_done());\n     @@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n       \tfor (i = 1; *p; i++, p = next_p) {\n       \t\tchar *eol = strchrnul(p, '\\n');\n      @@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n     - \t\t\titem->arg_offset = p - buf;\n     - \t\t\titem->arg_len = (int)(eol - p);\n       \t\t\titem->commit = NULL;\n     --\t\t}\n     -+\t\t} else if (item->command == TODO_COMMENT)\n     -+\t\t\ttodo_list->total_nr--;\n     + \t\t}\n       \n     ++\t\tif (item->command != TODO_COMMENT)\n     ++\t\t\ttodo_list->total_nr++;\n     ++\n       \t\tif (fixup_okay)\n       \t\t\t; /* do nothing */\n     + \t\telse if (is_fixup(item->command))\n      @@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n     - \tstruct todo_list new_todo = TODO_LIST_INIT;\n     - \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n     - \tstruct object_id oid = onto->object.oid;\n     --\tint res;\n     -+\tint new_total_nr, res;\n     - \n     - \tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n     - \n     -@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n     - \t\treturn error(_(\"nothing to do\"));\n     - \t}\n     - \n     -+\tnew_total_nr = todo_list->total_nr - count_commands(todo_list);\n     - \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n     - \t\t\t     shortonto, flags);\n     - \tif (res == -1)\n     -@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n     - \t\treturn -1;\n     - \t}\n     - \n     -+\tnew_total_nr += count_commands(&new_todo);\n     -+\tnew_todo.total_nr = new_total_nr;\n     -+\n     - \t/* Expand the commit IDs */\n       \ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n       \tstrbuf_swap(&new_todo.buf, &buf2);\n       \tstrbuf_release(&buf2);\n      -\tnew_todo.total_nr -= new_todo.nr;\n     ++\t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n     ++\tnew_todo.total_nr = 0;\n       \tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n       \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n       \t\t    new_todo.buf.buf);\n     @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges with message matc\n      +test_expect_success 'progress shows the correct total' '\n      +\tgit checkout -b progress H &&\n      +\tgit rebase --rebase-merges --force-rebase --verbose A 2> err &&\n     -+\tgrep \"^Rebasing.*14.$\" err >progress &&\n     ++\t# Expecting \"Rebasing (N/14)\" here, no bogus total number\n     ++\tgrep \"^Rebasing.*/14.$\" err >progress &&\n      +\ttest_line_count = 14 progress\n      +'\n      +\n\n-- \ngitgitgadget\n"},{"id":"477248","messageId":"2ac7c7a7c615db75a46076b58a51d363bc2daf2e.1683965487.git.gitgitgadget@gmail.com","threadId":"59727","inReplyTo":"pull.1531.v2.git.1683965487.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] rebase --update-refs: fix loops","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-13T08:11:25Z","receivedAt":"2023-05-13T08:11:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe `total_nr` field in the `todo_list` structure merely serves display\npurposes, and should only be used when generating the progress message.\n\nIn these two instances, however, we want to loop over all of the\ncommands in the parsed rebase script. The loop limit therefore needs to\nbe `nr`, which refers to the count of commands in the current\n`todo_list`.\n\nThis is important because the two numbers, `nr` and `total_nr` can\ndiffer wildly, e.g. due to `total_nr` _not_ counting comments or empty\nlines, while `nr` skips any commands that already moved from the\n`git-rebase-todo` file to the `done` file.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5f22b7cd377..f5d89abdc5e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4202,7 +4202,7 @@ void todo_list_filter_update_refs(struct repository *r,\n \t\tif (!is_null_oid(&rec->after))\n \t\t\tcontinue;\n \n-\t\tfor (j = 0; !found && j < todo_list->total_nr; j++) {\n+\t\tfor (j = 0; !found && j < todo_list->nr; j++) {\n \t\t\tstruct todo_item *item = &todo_list->items[j];\n \t\t\tconst char *arg = todo_list->buf.buf + item->arg_offset;\n \n@@ -4232,7 +4232,7 @@ void todo_list_filter_update_refs(struct repository *r,\n \t * For each todo_item, check if its ref is in the update_refs list.\n \t * If not, then add it as an un-updated ref.\n \t */\n-\tfor (i = 0; i < todo_list->total_nr; i++) {\n+\tfor (i = 0; i < todo_list->nr; i++) {\n \t\tstruct todo_item *item = &todo_list->items[i];\n \t\tconst char *arg = todo_list->buf.buf + item->arg_offset;\n \t\tint j, found = 0;\n-- \ngitgitgadget\n\n"},{"id":"477249","messageId":"cac809bcffd8ac6905ff823e1ab3503160ac54fd.1683965487.git.gitgitgadget@gmail.com","threadId":"59727","inReplyTo":"pull.1531.v2.git.1683965487.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase -r: fix the total number shown in the progress","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-13T08:11:26Z","receivedAt":"2023-05-13T08:11:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nFor regular, non-`--rebase-merges` runs, there is very little work to do\nfor the parser when determining the total number of commands in a rebase\nscript: it is simply the number of lines after stripping the commented\nlines and then trimming the trailing empty line, if any.\n\nThe `--rebase-merges` mode complicates things by introducing empty lines\nand comments in the middle of the script. These should _not_ be counted\nas commands, and indeed, when an interactive rebase is interrupted and\nsubsequently resumed, the total number of commands can magically shrink,\nsometimes dramatically.\n\nThe reason for this strange behavior is that empty lines _are_ counted\nin `edit_todo_list()` (but not the comments, as they are stripped via\n`strbuf_stripspace(..., 1)`, which is a bug.\n\nLet's fix this so that the correct total number is shown from the\nget-go, by carefully adjusting it according to what's in the rebase\nscript. Extra care needs to be taken in case the user edits the script:\nthe number of commands might be different after the user edited than\nbeforehand.\n\nNote: Even though commented lines are skipped in `edit_todo_list()`, we\nstill need to handle `TODO_COMMENT` items by decrementing the\nalready-incremented `total_nr` again: empty lines are also marked as\n`TODO_COMMENT`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c              | 9 ++++++---\n t/t3430-rebase-merges.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f5d89abdc5e..9fb8ef17618 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2453,7 +2453,6 @@ void todo_list_release(struct todo_list *todo_list)\n static struct todo_item *append_new_todo(struct todo_list *todo_list)\n {\n \tALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);\n-\ttodo_list->total_nr++;\n \treturn todo_list->items + todo_list->nr++;\n }\n \n@@ -2609,7 +2608,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \tchar *p = buf, *next_p;\n \tint i, res = 0, fixup_okay = file_exists(rebase_path_done());\n \n-\ttodo_list->current = todo_list->nr = 0;\n+\ttodo_list->current = todo_list->nr = todo_list->total_nr = 0;\n \n \tfor (i = 1; *p; i++, p = next_p) {\n \t\tchar *eol = strchrnul(p, '\\n');\n@@ -2630,6 +2629,9 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\t\titem->commit = NULL;\n \t\t}\n \n+\t\tif (item->command != TODO_COMMENT)\n+\t\t\ttodo_list->total_nr++;\n+\n \t\tif (fixup_okay)\n \t\t\t; /* do nothing */\n \t\telse if (is_fixup(item->command))\n@@ -6092,7 +6094,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n \tstrbuf_swap(&new_todo.buf, &buf2);\n \tstrbuf_release(&buf2);\n-\tnew_todo.total_nr -= new_todo.nr;\n+\t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n+\tnew_todo.total_nr = 0;\n \tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n \t\t    new_todo.buf.buf);\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex f351701fec2..499c31a8d9a 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -517,4 +517,12 @@ test_expect_success '--rebase-merges with message matched with onto label' '\n \tEOF\n '\n \n+test_expect_success 'progress shows the correct total' '\n+\tgit checkout -b progress H &&\n+\tgit rebase --rebase-merges --force-rebase --verbose A 2> err &&\n+\t# Expecting \"Rebasing (N/14)\" here, no bogus total number\n+\tgrep \"^Rebasing.*/14.$\" err >progress &&\n+\ttest_line_count = 14 progress\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"477266","messageId":"42b3c24a-8c3f-df03-18b8-312520d6c963@gmail.com","threadId":"59727","inReplyTo":"pull.1531.v2.git.1683965487.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Fix two rebase bugs related to total_nr","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-14T17:59:07Z","receivedAt":"2023-05-14T18:06:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 13/05/2023 09:11, Johannes Schindelin via GitGitGadget wrote:\n> I recently picked up work where I regularly rebase large branch thickets\n> consisting of thousands of commits. During those rebases, I could not fail\n> to notice that the progress initially showed a total number around 2,100,\n> when the actual number of commands was more like 1,850. And indeed, when\n> resuming the rebase after being interrupted due to a break command or due to\n> a merge conflict, the progress showed the correct number!\n> \n> So I set out to fix this, stumbling over an incorrect use of total_nr in the\n> --update-refs code, so I fixed that, too.\n> \n> Note: These patches apply cleanly on top of ds/rebase-update-ref as well as\n> on top of the current main branch.\n> \n> Changes since v1:\n> \n>   * Clarified the pattern expected by the test case in the progress output,\n>     as suggested by Junio.\n>   * Simplified the code as suggested by Phillipp, based on the insight that\n>     complete_action() (naming is hard!) is only called at the very beginning\n>     of a rebase, and therefore there cannot be any already-done commands in\n>     the todo list.\n\nThis version looks good to me, thanks for re-rolling\n\nPhillip\n\n> Johannes Schindelin (2):\n>    rebase --update-refs: fix loops\n>    rebase -r: fix the total number shown in the progress\n> \n>   sequencer.c              | 13 ++++++++-----\n>   t/t3430-rebase-merges.sh |  8 ++++++++\n>   2 files changed, 16 insertions(+), 5 deletions(-)\n> \n> \n> base-commit: 4611884ea883908a9638cafbd824c401c41cf7f6\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1531%2Fdscho%2Ffix-rebase-i-progress-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1531/dscho/fix-rebase-i-progress-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1531\n> \n> Range-diff vs v1:\n> \n>   1:  2ac7c7a7c61 = 1:  2ac7c7a7c61 rebase --update-refs: fix loops\n>   2:  d12d5469f8c ! 2:  cac809bcffd rebase -r: fix the total number shown in the progress\n>       @@ Commit message\n>            Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>        \n>         ## sequencer.c ##\n>       +@@ sequencer.c: void todo_list_release(struct todo_list *todo_list)\n>       + static struct todo_item *append_new_todo(struct todo_list *todo_list)\n>       + {\n>       + \tALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);\n>       +-\ttodo_list->total_nr++;\n>       + \treturn todo_list->items + todo_list->nr++;\n>       + }\n>       +\n>        @@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>         \tchar *p = buf, *next_p;\n>         \tint i, res = 0, fixup_okay = file_exists(rebase_path_done());\n>       @@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>         \tfor (i = 1; *p; i++, p = next_p) {\n>         \t\tchar *eol = strchrnul(p, '\\n');\n>        @@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>       - \t\t\titem->arg_offset = p - buf;\n>       - \t\t\titem->arg_len = (int)(eol - p);\n>         \t\t\titem->commit = NULL;\n>       --\t\t}\n>       -+\t\t} else if (item->command == TODO_COMMENT)\n>       -+\t\t\ttodo_list->total_nr--;\n>       + \t\t}\n>         \n>       ++\t\tif (item->command != TODO_COMMENT)\n>       ++\t\t\ttodo_list->total_nr++;\n>       ++\n>         \t\tif (fixup_okay)\n>         \t\t\t; /* do nothing */\n>       + \t\telse if (is_fixup(item->command))\n>        @@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>       - \tstruct todo_list new_todo = TODO_LIST_INIT;\n>       - \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n>       - \tstruct object_id oid = onto->object.oid;\n>       --\tint res;\n>       -+\tint new_total_nr, res;\n>       -\n>       - \tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n>       -\n>       -@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>       - \t\treturn error(_(\"nothing to do\"));\n>       - \t}\n>       -\n>       -+\tnew_total_nr = todo_list->total_nr - count_commands(todo_list);\n>       - \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n>       - \t\t\t     shortonto, flags);\n>       - \tif (res == -1)\n>       -@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>       - \t\treturn -1;\n>       - \t}\n>       -\n>       -+\tnew_total_nr += count_commands(&new_todo);\n>       -+\tnew_todo.total_nr = new_total_nr;\n>       -+\n>       - \t/* Expand the commit IDs */\n>         \ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n>         \tstrbuf_swap(&new_todo.buf, &buf2);\n>         \tstrbuf_release(&buf2);\n>        -\tnew_todo.total_nr -= new_todo.nr;\n>       ++\t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n>       ++\tnew_todo.total_nr = 0;\n>         \tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n>         \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n>         \t\t    new_todo.buf.buf);\n>       @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges with message matc\n>        +test_expect_success 'progress shows the correct total' '\n>        +\tgit checkout -b progress H &&\n>        +\tgit rebase --rebase-merges --force-rebase --verbose A 2> err &&\n>       -+\tgrep \"^Rebasing.*14.$\" err >progress &&\n>       ++\t# Expecting \"Rebasing (N/14)\" here, no bogus total number\n>       ++\tgrep \"^Rebasing.*/14.$\" err >progress &&\n>        +\ttest_line_count = 14 progress\n>        +'\n>        +\n> \n"}]}