{"thread":{"id":"51918","subject":"[PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list","startedAt":"2019-09-25T20:13:52Z","lastAt":"2019-11-27T20:09:17Z","messageCount":50,"participants":["Alban Gruin","Phillip Wood","Junio C Hamano","Johannes Schindelin","René Scharfe","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"382936","messageId":"20190925201315.19722-2-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"[PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-09-25T20:13:11Z","receivedAt":"2019-09-25T20:13:52Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"`total_nr' is the total amount of items, done and toto, that are in a\ntodo list. But unlike `nr', it was not updated when an item was\nappended to the list.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like).\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d648aaf416..575b852a5a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2070,6 +2070,7 @@ 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-- \n2.23.0\n\n"},{"id":"382937","messageId":"20190925201315.19722-3-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"[PATCH v1 2/5] sequencer: update `done_nr' when skipping commands in a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-09-25T20:13:12Z","receivedAt":"2019-09-25T20:13:53Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"In a todo list, `done_nr' is the amount of commands that were executed\nor skipped, but skip_unnecessary_picks() did not update it.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like).\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 575b852a5a..42313f8de6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,\n \t\tMOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);\n \t\ttodo_list->nr -= i;\n \t\ttodo_list->current = 0;\n+\t\ttodo_list->done_nr += i;\n \n \t\tif (is_fixup(peek_command(todo_list, 0)))\n \t\t\trecord_in_rewritten(base_oid, peek_command(todo_list, 0));\n-- \n2.23.0\n\n"},{"id":"382938","messageId":"20190925201315.19722-4-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"[PATCH v1 3/5] sequencer: move the code writing total_nr on the disk to a new function","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-09-25T20:13:13Z","receivedAt":"2019-09-25T20:13:55Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"The total amount of commands can be used to show the progression of the\nrebasing in a shell. It is written to the disk by read_populate_todo()\nwhen the todo list is loaded from sequencer_continue() or\npick_commits(), but not by complete_action().\n\nThis moves the part writing total_nr to a new function so it can be\ncalled from complete_action().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 42313f8de6..ec7ea8d9e5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n \tsequencer_remove_state(&opts);\n }\n \n+static void todo_list_write_total_nr(struct todo_list *todo_list)\n+{\n+\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n+\n+\tif (f) {\n+\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n+\t\tfclose(f);\n+\t}\n+}\n+\n static int read_populate_todo(struct repository *r,\n \t\t\t struct todo_list *todo_list,\n \t\t\t struct replay_opts *opts)\n@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,\n \n \tif (is_rebase_i(opts)) {\n \t\tstruct todo_list done = TODO_LIST_INIT;\n-\t\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n \t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,\n \t\t\t+ count_commands(todo_list);\n \t\ttodo_list_release(&done);\n \n-\t\tif (f) {\n-\t\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n-\t\t\tfclose(f);\n-\t\t}\n+\t\ttodo_list_write_total_nr(todo_list);\n \t}\n \n \treturn 0;\n-- \n2.23.0\n\n"},{"id":"382939","messageId":"20190925201315.19722-5-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"[PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-09-25T20:13:14Z","receivedAt":"2019-09-25T20:13:55Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"get_replay_opts() did not fill `squash_onto' if possible, meaning that\nthis field should be read from the disk by the sequencer through\nread_populate_opts(). Without this, calling `pick_commits()' directly\nwill result in incorrect results with `rebase --root'.\n\nLet’s change that.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e8319d5946..2097d41edc 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \tif (opts->strategy_opts)\n \t\tparse_strategy_opts(&replay, opts->strategy_opts);\n \n+\tif (opts->squash_onto) {\n+\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n+\t\treplay.have_squash_onto = 1;\n+\t}\n+\n \treturn replay;\n }\n \n-- \n2.23.0\n\n"},{"id":"382940","messageId":"20190925201315.19722-6-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"[PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-09-25T20:13:15Z","receivedAt":"2019-09-25T20:14:01Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Currently, complete_action() calls sequencer_continue() to do the\nrebase. Even though the former already has the todo list, the latter\nloads it from the disk and parses it. Calling directly pick_commits()\nfrom complete_action() avoids this unnecessary round trip.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ec7ea8d9e5..b395dd6e11 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \t}\n \n-\ttodo_list_release(&new_todo);\n-\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n \t\treturn -1;\n \n \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n \t\treturn -1;\n \n-\treturn sequencer_continue(r, opts);\n+\ttodo_list_write_total_nr(&new_todo);\n+\tres = pick_commits(r, &new_todo, opts);\n+\ttodo_list_release(&new_todo);\n+\n+\treturn res;\n }\n \n struct subject2item_entry {\n-- \n2.23.0\n\n"},{"id":"382941","messageId":"20190925202054.20416-1-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"[PATCH v1 0/5] Use complete_action's todo list to do the rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-09-25T20:20:54Z","receivedAt":"2019-09-25T20:21:09Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"[Sorry, I made a mistake and this message was not transmitted :/]\n\nThis can be seen as a continuation of ag/reduce-rewriting-todo.\n\nCurrently, complete_action() releases its todo list before calling\nsequencer_continue(), which reloads the todo list from the disk. This\nseries removes this useless round trip.\n\nPatches 1, 2, and 3 originally come from a series meaning to improve\nrebase.missingCommitsCheck[0]. In the original series, I wanted to\ncheck for missing commits in read_populate_todo(), so a warning could be\nissued after a `rebase --continue' or an `exec' commands. But, in the\ncase of the initial edit, it is already checked in complete_action(),\nand would be checked a second time in sequencer_continue() (a caller of\nread_populate_todo()). So I hacked up sequencer_continue() to accept a\npointer to a todo list, and if not null, would skip the call to\nread_populate_todo(). (This was really ugly, to be honest.) Some\nissues arose with git-prompt.sh[1], hence 1, 2 and 3.\n\nPatch 5 is a new approach to what I did first. Instead of bolting a new\nparameter to sequencer_continue(), this makes complete_action() calling\ndirectly pick_commits().\n\nThis is based on master (4c86140027, \"Third batch\").\n\nThe tip of this series is tagged as \"reduce-todo-list-cont-v1\" at\nhttps://github.com/agrn/git.\n\n[0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n[1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n\nAlban Gruin (5):\n sequencer: update `total_nr' when adding an item to a todo list\n sequencer: update `done_nr' when skipping commands in a todo list\n sequencer: move the code writing total_nr on the disk to a new\n function\n rebase: fill `squash_onto' in get_replay_opts()\n sequencer: directly call pick_commits() from complete_action()\n\n builtin/rebase.c | 5 +++++\n sequencer.c | 26 ++++++++++++++++++--------\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\n-- \n2.23.0\n\n"},{"id":"383065","messageId":"212cdc0d-8cf3-9172-d405-39b3868e6ca4@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-6-alban.gruin@gmail.com","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-09-27T13:26:26Z","receivedAt":"2019-09-27T13:26:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alban\n\nThanks for removing some more unnecessary work reloading the the todo list.\n\nOn 25/09/2019 21:13, Alban Gruin wrote:\n> Currently, complete_action() calls sequencer_continue() to do the\n> rebase. Even though the former already has the todo list, the latter\n> loads it from the disk and parses it. Calling directly pick_commits()\n> from complete_action() avoids this unnecessary round trip.\n> Signed-off-by: Alban Gruin \n> ---\n> sequencer.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index ec7ea8d9e5..b395dd6e11 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n> \t}\n> \n> -\ttodo_list_release(&new_todo);\n> -\n> \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n> \t\treturn -1;\n> \n> \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n> \t\treturn -1;\n> \n> -\treturn sequencer_continue(r, opts);\n\nsequencer_continue does a number of things before calling pick_commits(). It\n - calls read_and_refresh_cache() - this is unnecessary here as we've \njust called require_clean_work_tree()\n - calls read_populate_opts() - this is unnecessary as we're staring a \nnew rebase so opts is fully populated\n - loads the todo list - this is unnecessary as we've just populated \nthe todo list\n - commits any staged changes - this is unnecessary as we're staring a \nnew rebase so there are no staged changes\n - calls record_in_rewritten() - this is unnecessary as we're starting \na new rebase\n\nSo I agree that this patch is correct.\n\nThanks\n\nPhillip\n\n> +\ttodo_list_write_total_nr(&new_todo);\n> +\tres = pick_commits(r, &new_todo, opts);\n> +\ttodo_list_release(&new_todo);\n> +\n> +\treturn res;\n> }\n> \n> struct subject2item_entry {\n> \n"},{"id":"383066","messageId":"66fcd66b-fad2-e5a1-cdd8-fd7b37c4abbf@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-5-alban.gruin@gmail.com","subject":"Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-09-27T13:30:58Z","receivedAt":"2019-09-27T13:31:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alban\n\nOn 25/09/2019 21:13, Alban Gruin wrote:\n> get_replay_opts() did not fill `squash_onto' if possible, meaning that\n\nI'm not sure what you mean by 'if possible' here, I think the sentance \nmakes sense without that.\n\n> this field should be read from the disk by the sequencer through\n> read_populate_opts(). Without this, calling `pick_commits()' directly\n> will result in incorrect results with `rebase --root'.\n> \n> Let’s change that.\n\nGood catch\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Alban Gruin \n> ---\n> builtin/rebase.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index e8319d5946..2097d41edc 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> \tif (opts->strategy_opts)\n> \t\tparse_strategy_opts(&replay, opts->strategy_opts);\n> \n> +\tif (opts->squash_onto) {\n> +\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n> +\t\treplay.have_squash_onto = 1;\n> +\t}\n> +\n> \treturn replay;\n> }\n> \n> \n"},{"id":"383067","messageId":"a8e6ba74-8a7a-495a-41fb-b3183f49e070@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"Re: [PATCH v1 0/5] Use complete_action’s todo list to do the rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-09-27T13:32:30Z","receivedAt":"2019-09-27T13:32:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alban\n\nOn 25/09/2019 21:13, Alban Gruin wrote:\n> This can be seen as a continuation of ag/reduce-rewriting-todo.\n> \n> Currently, complete_action() releases its todo list before calling\n> sequencer_continue(), which reloads the todo list from the disk. This\n> series removes this useless round trip.\n> \n> Patches 1, 2, and 3 originally come from a series meaning to improve\n> rebase.missingCommitsCheck[0]. In the original series, I wanted to\n> check for missing commits in read_populate_todo(), so a warning could be\n> issued after a `rebase --continue' or an `exec' commands. But, in the\n> case of the initial edit, it is already checked in complete_action(),\n> and would be checked a second time in sequencer_continue() (a caller of\n> read_populate_todo()). So I hacked up sequencer_continue() to accept a\n> pointer to a todo list, and if not null, would skip the call to\n> read_populate_todo(). (This was really ugly, tbh.) Some issues arose\n> with git-prompt.sh[1], hence 1, 2 and 3.\n> \n> Patch 5 is a new approach to what I did first. Instead of bolting a new\n> parameter to sequencer_continue(), this makes complete_action() calling\n> directly pick_commits().\n\nThanks for working on this, these patches all look fine to me modulo the \nconfusing wording in the commit message on patch 4\n\nBest Wishes\n\nPhillip\n\n> \n> This is based on master (4c86140027, \"Third batch\").\n> \n> The tip of this series is tagged as \"reduce-todo-list-cont-v1\" at\n> https://github.com/agrn/git.\n> \n> [0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n> [1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n> \n> Alban Gruin (5):\n> sequencer: update `total_nr' when adding an item to a todo list\n> sequencer: update `done_nr' when skipping commands in a todo list\n> sequencer: move the code writing total_nr on the disk to a new\n> function\n> rebase: fill `squash_onto' in get_replay_opts()\n> sequencer: directly call pick_commits() from complete_action()\n> \n> builtin/rebase.c | 5 +++++\n> sequencer.c | 26 ++++++++++++++++++--------\n> 2 files changed, 23 insertions(+), 8 deletions(-)\n> \n"},{"id":"383236","messageId":"xmqqbluzhp8g.fsf@gitster-ct.c.googlers.com","threadId":"51918","inReplyTo":"20190925201315.19722-2-alban.gruin@gmail.com","subject":"Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T02:10:07Z","receivedAt":"2019-10-02T02:10:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> `total_nr' is the total amount of items, done and toto, that are in a\n> todo list. But unlike `nr', it was not updated when an item was\n> appended to the list.\n\ns/amount/number/, as amount is specifically for something\nthat cannot be counted.\n\nPerhaps a stupid language question but what is \"toto\"?\n\n\n> This variable is mostly used by command prompts (ie. git-prompt.sh and\n> the like).\n>\n> Signed-off-by: Alban Gruin \n> ---\n> sequencer.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index d648aaf416..575b852a5a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2070,6 +2070,7 @@ 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"},{"id":"383237","messageId":"xmqq5zl7hnwe.fsf@gitster-ct.c.googlers.com","threadId":"51918","inReplyTo":"20190925201315.19722-6-alban.gruin@gmail.com","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T02:38:57Z","receivedAt":"2019-10-02T02:43:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> Currently, complete_action() calls sequencer_continue() to do the\n> rebase. Even though the former already has the todo list, the latter\n> loads it from the disk and parses it. Calling directly pick_commits()\n> from complete_action() avoids this unnecessary round trip.\n>\n> Signed-off-by: Alban Gruin \n> ---\n> sequencer.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n\nAll the earlier steps in this series are necessary because the\ninconsistencies caused by not doing things the code updated by the\nearlier steps do (e.g. leaving number of commits recorded in\ntotal_nr and done_nr stale) were masked by re-reading the info from\nthe on-disk file. And this step exposes these inconsistencies\nbecause the extra reading from the file no longer happens.\n\nThat is my reading of the series---did I get it right?\n\nI wonder if that can be stated clearly in the log message of the\nearlier steps.\n\nFor example, by reading 1/5 or 2/5 alone, it would be natural for\nreaders to wonder why the original code that did not update done_nr\ncorrectly terminated after going through the todo list (you would\nexpect that when done reaches total you are finished, so if one of\nthese variables are not maintained correctly, your termination\ncondition may be borked), and then by knowing that the current code\nmore-or-less works correctly by not terminating too early or barfing\nto say it ran out of things to do prematurely, the next thing\nreaders would wonder is if these patches introduce new bugs by\nincrementing these variables twice (iow, \"does the author of the\npatch missed other places where these variables are correctly\nupdated?\")\n\n1/5 for example talks about the variable mostly being used by\nprompts, which may be useful to reduce worries by readers about\ncorrectness (iow, \"well, if it is only showing wrong number in the\nprompts and does not affect correctness otherwise, perhaps that was\nwhy the current code that is buggy did not behave incorrectly\").\nBut I suspect that it is not why these changes in the earlier steps\nare acceptable or are right things to do. So, perhaps replace the\n\"these are used only in prompts and the numbers being wrong does not\naffect correctness\" comments from them with:\n\n By forgetting to update the variable, the original code made it\n not reflect the reality, but this flaw was masked by the code\n calling (sometimes unnecessarily) read_todo_list() again to\n update the variable to its correct value before the next action\n happens. At the end of this series, the unnecessary call will\n be removed and the inconsistency addressed by this patch would\n start to matter.\n\nor something like that (assuming that the answer to the first\nquestion I asked in this message is \"yes\", that is), or a more\nconcise version of it?\n\nThanks.\n"},{"id":"383260","messageId":"nycvar.QRO.7.76.6.1910021005050.46@tvgsbejvaqbjf.bet","threadId":"51918","inReplyTo":"xmqqbluzhp8g.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-02T08:06:04Z","receivedAt":"2019-10-02T08:06:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 2 Oct 2019, Junio C Hamano wrote:\n\n> Alban Gruin writes:\n>\n> > `total_nr' is the total amount of items, done and toto, that are in a\n> > todo list. But unlike `nr', it was not updated when an item was\n> > appended to the list.\n>\n> s/amount/number/, as amount is specifically for something\n> that cannot be counted.\n>\n> Perhaps a stupid language question but what is \"toto\"?\n\n\"in toto\" is Latin for \"in total\", if I remember correctly.\n\nBut in this instance, I think it is merely a typo and should have been\n\"todo\" instead. That is what the \"total_nr\" is about: the number of\n\"done\" and \"todo\" items, added together.\n\nCiao,\nDscho\n\n>\n>\n> > This variable is mostly used by command prompts (ie. git-prompt.sh and\n> > the like).\n> >\n> > Signed-off-by: Alban Gruin \n> > ---\n> > sequencer.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index d648aaf416..575b852a5a 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -2070,6 +2070,7 @@ 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"},{"id":"383261","messageId":"nycvar.QRO.7.76.6.1910021011010.46@tvgsbejvaqbjf.bet","threadId":"51918","inReplyTo":"66fcd66b-fad2-e5a1-cdd8-fd7b37c4abbf@gmail.com","subject":"Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-02T08:16:52Z","receivedAt":"2019-10-02T08:17:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 27 Sep 2019, Phillip Wood wrote:\n\n> Hi Alban\n>\n> On 25/09/2019 21:13, Alban Gruin wrote:\n> > get_replay_opts() did not fill `squash_onto' if possible, meaning that\n>\n> I'm not sure what you mean by 'if possible' here, I think the sentance makes\n> sense without that.\n>\n> > this field should be read from the disk by the sequencer through\n> > read_populate_opts(). Without this, calling `pick_commits()' directly\n> > will result in incorrect results with `rebase --root'.\n\nI guess I would have an easier time reading the commit message if this\nparagraph read like this:\n\n\tThe `get_replay_opts()` function currently does not initialize\n\tthe `squash_onto` field (which is used for the `--root` mode),\n\tonly `read_populate_opts()` does.\n\n\tThat would lead to incorrect results when calling\n\t`pick_commits()` without reading the options from disk first.\n\n> >\n> > Let’s change that.\n>\n> Good catch\n>\n> Best Wishes\n>\n> Phillip\n>\n> > Signed-off-by: Alban Gruin \n> > ---\n> > builtin/rebase.c | 5 +++++\n> > 1 file changed, 5 insertions(+)\n> >\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index e8319d5946..2097d41edc 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct\n> > rebase_options *opts)\n> > if (opts->strategy_opts)\n> > parse_strategy_opts(&replay, opts->strategy_opts);\n> > +\tif (opts->squash_onto) {\n\nI guess it does not matter much, but shouldn't this be guarded against\nthe case where `replay.squash_onto` was already initialized? Like, `if\n(opts->squash_onto && !replay.have_squash_onto)`?\n\nOther than that, this patch makes sense to me.\n\nCiao,\nDscho\n\n> > +\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n> > +\t\treplay.have_squash_onto = 1;\n> > +\t}\n> > +\n> > \treturn replay;\n> > }\n> >\n> >\n>\n>\n"},{"id":"383262","messageId":"nycvar.QRO.7.76.6.1910021018360.46@tvgsbejvaqbjf.bet","threadId":"51918","inReplyTo":"212cdc0d-8cf3-9172-d405-39b3868e6ca4@gmail.com","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-02T08:20:33Z","receivedAt":"2019-10-02T08:21:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 27 Sep 2019, Phillip Wood wrote:\n\n> Hi Alban\n>\n> Thanks for removing some more unnecessary work reloading the the todo list.\n>\n> On 25/09/2019 21:13, Alban Gruin wrote:\n> > Currently, complete_action() calls sequencer_continue() to do the\n> > rebase. Even though the former already has the todo list, the latter\n> > loads it from the disk and parses it. Calling directly pick_commits()\n> > from complete_action() avoids this unnecessary round trip.\n> > Signed-off-by: Alban Gruin \n> > ---\n> > sequencer.c | 8 +++++---\n> > 1 file changed, 5 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index ec7ea8d9e5..b395dd6e11 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct\n> > replay_opts *opts, unsigned fla\n> > \treturn error_errno(_(\"could not write '%s'\"), todo_file);\n> > }\n> > -\ttodo_list_release(&new_todo);\n> > -\n> > if (checkout_onto(r, opts, onto_name, &oid, orig_head))\n> > return -1;\n> >\n> > if (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n> > return -1;\n> > -\treturn sequencer_continue(r, opts);\n>\n> sequencer_continue does a number of things before calling pick_commits(). It\n> - calls read_and_refresh_cache() - this is unnecessary here as we've just\n> called require_clean_work_tree()\n> - calls read_populate_opts() - this is unnecessary as we're staring a new\n> rebase so opts is fully populated\n> - loads the todo list - this is unnecessary as we've just populated the todo\n> list\n> - commits any staged changes - this is unnecessary as we're staring a new\n> rebase so there are no staged changes\n> - calls record_in_rewritten() - this is unnecessary as we're starting a new\n> rebase\n>\n> So I agree that this patch is correct.\n\nAll true. Could this careful analysis maybe be included in the commit\nmessage (with `s/staring/starting/`)?\n\nThanks,\nDscho\n\n>\n> Thanks\n>\n> Phillip\n>\n> > +\ttodo_list_write_total_nr(&new_todo);\n> > +\tres = pick_commits(r, &new_todo, opts);\n> > +\ttodo_list_release(&new_todo);\n> > +\n> > +\treturn res;\n> > }\n> >\n> > struct subject2item_entry {\n> >\n>\n"},{"id":"383264","messageId":"xmqqh84red54.fsf@gitster-ct.c.googlers.com","threadId":"51918","inReplyTo":"nycvar.QRO.7.76.6.1910021005050.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T08:59:35Z","receivedAt":"2019-10-02T08:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Wed, 2 Oct 2019, Junio C Hamano wrote:\n>\n>> Alban Gruin writes:\n>>\n>> > `total_nr' is the total amount of items, done and toto, that are in a\n>> > todo list. But unlike `nr', it was not updated when an item was\n>> > appended to the list.\n>>\n>> s/amount/number/, as amount is specifically for something\n>> that cannot be counted.\n>>\n>> Perhaps a stupid language question but what is \"toto\"?\n>\n> \"in toto\" is Latin for \"in total\", if I remember correctly.\n\nAnd \"Toto\" can also be \"Toyo Toki\", one of the two large and well\nknown Japanese manufacturers of porcelain things you see in\nbathrooms--oh how appropriate in this project ;-).\n\n> But in this instance, I think it is merely a typo and should have been\n> \"todo\" instead. That is what the \"total_nr\" is about: the number of\n> \"done\" and \"todo\" items, added together.\n\nIf I were writing this, I would probably say \"... the total number\nof items, counting both done and todo,...\" and with 'counting both'\nI wouldn't have been so puzzled.\n\nThanks.\n\n>\n> Ciao,\n> Dscho\n>\n>>\n>>\n>> > This variable is mostly used by command prompts (ie. git-prompt.sh and\n>> > the like).\n>> >\n>> > Signed-off-by: Alban Gruin \n>> > ---\n>> > sequencer.c | 1 +\n>> > 1 file changed, 1 insertion(+)\n>> >\n>> > diff --git a/sequencer.c b/sequencer.c\n>> > index d648aaf416..575b852a5a 100644\n>> > --- a/sequencer.c\n>> > +++ b/sequencer.c\n>> > @@ -2070,6 +2070,7 @@ 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"},{"id":"383266","messageId":"94dbb49e-2c11-ce30-5d50-05be19ebb3bb@gmail.com","threadId":"51918","inReplyTo":"nycvar.QRO.7.76.6.1910021011010.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-10-02T09:32:33Z","receivedAt":"2019-10-02T09:32:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 02/10/2019 09:16, Johannes Schindelin wrote:\n> Hi,\n> \n> On Fri, 27 Sep 2019, Phillip Wood wrote:\n> \n>> Hi Alban\n>>\n>> On 25/09/2019 21:13, Alban Gruin wrote:\n >>> [...]\n>>> builtin/rebase.c | 5 +++++\n>>> 1 file changed, 5 insertions(+)\n>>>\n>>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>>> index e8319d5946..2097d41edc 100644\n>>> --- a/builtin/rebase.c\n>>> +++ b/builtin/rebase.c\n>>> @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct\n>>> rebase_options *opts)\n>>> if (opts->strategy_opts)\n>>> parse_strategy_opts(&replay, opts->strategy_opts);\n>>> +\tif (opts->squash_onto) {\n> \n> I guess it does not matter much, but shouldn't this be guarded against\n> the case where `replay.squash_onto` was already initialized? Like, `if\n> (opts->squash_onto && !replay.have_squash_onto)`?\n\nreplay is uninitialized as this function takes a `struct rebase_opitons` \nand returns a new `struct replay_options` based on that.\n\nBest Wishes\n\nPhillip\n\n> \n> Other than that, this patch makes sense to me.\n> \n> Ciao,\n> Dscho\n> \n>>> +\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n>>> +\t\treplay.have_squash_onto = 1;\n>>> +\t}\n>>> +\n>>> \treturn replay;\n>>> }\n>>>\n>>>\n>>\n>>\n"},{"id":"383268","messageId":"nycvar.QRO.7.76.6.1910021147390.46@tvgsbejvaqbjf.bet","threadId":"51918","inReplyTo":"xmqqh84red54.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-02T09:48:50Z","receivedAt":"2019-10-02T09:49:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 2 Oct 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > On Wed, 2 Oct 2019, Junio C Hamano wrote:\n> >\n> >> Alban Gruin writes:\n> >>\n> >> > `total_nr' is the total amount of items, done and toto, that are in a\n> >> > todo list. But unlike `nr', it was not updated when an item was\n> >> > appended to the list.\n> >>\n> >> s/amount/number/, as amount is specifically for something\n> >> that cannot be counted.\n> >>\n> >> Perhaps a stupid language question but what is \"toto\"?\n> >\n> > \"in toto\" is Latin for \"in total\", if I remember correctly.\n>\n> And \"Toto\" can also be \"Toyo Toki\", one of the two large and well\n> known Japanese manufacturers of porcelain things you see in\n> bathrooms--oh how appropriate in this project ;-).\n\nYou made me laugh out loud! :-)\n\n> > But in this instance, I think it is merely a typo and should have been\n> > \"todo\" instead. That is what the \"total_nr\" is about: the number of\n> > \"done\" and \"todo\" items, added together.\n>\n> If I were writing this, I would probably say \"... the total number\n> of items, counting both done and todo,...\" and with 'counting both'\n> I wouldn't have been so puzzled.\n\nI also like it better the way you put it.\n\nCiao,\nDscho\n\n>\n> Thanks.\n>\n> >\n> > Ciao,\n> > Dscho\n> >\n> >>\n> >>\n> >> > This variable is mostly used by command prompts (ie. git-prompt.sh and\n> >> > the like).\n> >> >\n> >> > Signed-off-by: Alban Gruin \n> >> > ---\n> >> > sequencer.c | 1 +\n> >> > 1 file changed, 1 insertion(+)\n> >> >\n> >> > diff --git a/sequencer.c b/sequencer.c\n> >> > index d648aaf416..575b852a5a 100644\n> >> > --- a/sequencer.c\n> >> > +++ b/sequencer.c\n> >> > @@ -2070,6 +2070,7 @@ 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>\n"},{"id":"383270","messageId":"nycvar.QRO.7.76.6.1910021405391.46@tvgsbejvaqbjf.bet","threadId":"51918","inReplyTo":"94dbb49e-2c11-ce30-5d50-05be19ebb3bb@gmail.com","subject":"Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-02T12:06:18Z","receivedAt":"2019-10-02T12:06:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 2 Oct 2019, Phillip Wood wrote:\n\n> On 02/10/2019 09:16, Johannes Schindelin wrote:\n> >\n> > On Fri, 27 Sep 2019, Phillip Wood wrote:\n> >\n> > > Hi Alban\n> > >\n> > > On 25/09/2019 21:13, Alban Gruin wrote:\n> > > > [...]\n> > > > builtin/rebase.c | 5 +++++\n> > > > 1 file changed, 5 insertions(+)\n> > > >\n> > > > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > > > index e8319d5946..2097d41edc 100644\n> > > > --- a/builtin/rebase.c\n> > > > +++ b/builtin/rebase.c\n> > > > @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const\n> > > > struct\n> > > > rebase_options *opts)\n> > > > if (opts->strategy_opts)\n> > > > parse_strategy_opts(&replay, opts->strategy_opts);\n> > > > +\tif (opts->squash_onto) {\n> >\n> > I guess it does not matter much, but shouldn't this be guarded against\n> > the case where `replay.squash_onto` was already initialized? Like, `if\n> > (opts->squash_onto && !replay.have_squash_onto)`?\n>\n> replay is uninitialized as this function takes a `struct rebase_opitons` and\n> returns a new `struct replay_options` based on that.\n\nOoops, you're right. The `.` in `replay.have_squash_onto` should have\nbeen a strong hint, eh?\n\nThanks,\nDscho\n\n>\n> Best Wishes\n>\n> Phillip\n>\n> >\n> > Other than that, this patch makes sense to me.\n> >\n> > Ciao,\n> > Dscho\n> >\n> > > > +\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n> > > > +\t\treplay.have_squash_onto = 1;\n> > > > +\t}\n> > > > +\n> > > > \treturn replay;\n> > > > }\n> > > >\n> > > >\n> > >\n> > >\n>\n"},{"id":"383287","messageId":"88ea97e2-664f-33f9-5f94-fe66b5e4220c@gmail.com","threadId":"51918","inReplyTo":"xmqqh84red54.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-02T18:03:04Z","receivedAt":"2019-10-02T18:03:18Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio and Johannes,\n\nLe 02/10/2019 à 10:59, Junio C Hamano a écrit :\n> Johannes Schindelin writes:\n> \n>> Hi Junio,\n>>\n>> On Wed, 2 Oct 2019, Junio C Hamano wrote:\n>>\n>>> Alban Gruin writes:\n>>>\n>>>> `total_nr' is the total amount of items, done and toto, that are in a\n>>>> todo list. But unlike `nr', it was not updated when an item was\n>>>> appended to the list.\n>>>\n>>> s/amount/number/, as amount is specifically for something\n>>> that cannot be counted.\n>>>\n\nThank you for these corrections, I really appreciate it :)\n\n>>> Perhaps a stupid language question but what is \"toto\"?\n>>\n>> \"in toto\" is Latin for \"in total\", if I remember correctly.\n> \n> And \"Toto\" can also be \"Toyo Toki\", one of the two large and well\n> known Japanese manufacturers of porcelain things you see in\n> bathrooms--oh how appropriate in this project ;-).\n> \n\nIn French, it’s the name of a recurring character in children’s jokes.\nIt’s also used sometimes as a dummy variable name like foo or bar.\n\n>> But in this instance, I think it is merely a typo and should have been\n>> \"todo\" instead. That is what the \"total_nr\" is about: the number of\n>> \"done\" and \"todo\" items, added together.\n> \n\nCorrect.\n\n> If I were writing this, I would probably say \"... the total number\n> of items, counting both done and todo,...\" and with 'counting both'\n> I wouldn't have been so puzzled.\n> \n\nI will change this.\n\nCheers,\nAlban\n\n\n\n> Thanks.\n> \n>>\n>> Ciao,\n>> Dscho\n>>\n>>>\n>>>\n>>>> This variable is mostly used by command prompts (ie. git-prompt.sh and\n>>>> the like).\n>>>>\n>>>> Signed-off-by: Alban Gruin \n>>>> ---\n>>>> sequencer.c | 1 +\n>>>> 1 file changed, 1 insertion(+)\n>>>>\n>>>> diff --git a/sequencer.c b/sequencer.c\n>>>> index d648aaf416..575b852a5a 100644\n>>>> --- a/sequencer.c\n>>>> +++ b/sequencer.c\n>>>> @@ -2070,6 +2070,7 @@ 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\n"},{"id":"383288","messageId":"e37012e1-09dd-41b9-aa51-b6dd2b28a9c4@gmail.com","threadId":"51918","inReplyTo":"nycvar.QRO.7.76.6.1910021018360.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-02T18:03:29Z","receivedAt":"2019-10-02T18:03:34Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Johannes,\n\nLe 02/10/2019 à 10:20, Johannes Schindelin a écrit :\n> Hi,\n> \n> On Fri, 27 Sep 2019, Phillip Wood wrote:\n> \n>> Hi Alban\n>>\n>> Thanks for removing some more unnecessary work reloading the the todo list.\n>>\n>> On 25/09/2019 21:13, Alban Gruin wrote:\n>>> Currently, complete_action() calls sequencer_continue() to do the\n>>> rebase. Even though the former already has the todo list, the latter\n>>> loads it from the disk and parses it. Calling directly pick_commits()\n>>> from complete_action() avoids this unnecessary round trip.\n>>> Signed-off-by: Alban Gruin \n>>> ---\n>>> sequencer.c | 8 +++++---\n>>> 1 file changed, 5 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index ec7ea8d9e5..b395dd6e11 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct\n>>> replay_opts *opts, unsigned fla\n>>> \treturn error_errno(_(\"could not write '%s'\"), todo_file);\n>>> }\n>>> -\ttodo_list_release(&new_todo);\n>>> -\n>>> if (checkout_onto(r, opts, onto_name, &oid, orig_head))\n>>> return -1;\n>>>\n>>> if (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n>>> return -1;\n>>> -\treturn sequencer_continue(r, opts);\n>>\n>> sequencer_continue does a number of things before calling pick_commits(). It\n>> - calls read_and_refresh_cache() - this is unnecessary here as we've just\n>> called require_clean_work_tree()\n>> - calls read_populate_opts() - this is unnecessary as we're staring a new\n>> rebase so opts is fully populated\n>> - loads the todo list - this is unnecessary as we've just populated the todo\n>> list\n>> - commits any staged changes - this is unnecessary as we're staring a new\n>> rebase so there are no staged changes\n>> - calls record_in_rewritten() - this is unnecessary as we're starting a new\n>> rebase\n>>\n>> So I agree that this patch is correct.\n> \n> All true. Could this careful analysis maybe be included in the commit\n> message (with `s/staring/starting/`)?\n> \n\nI will do so (same for your comment on 4/5) and resend this series as\nsoon as possible.\n\nCheers,\nAlban\n\n\n\n> Thanks,\n> Dscho\n> \n>>\n>> Thanks\n>>\n>> Phillip\n>>\n>>> +\ttodo_list_write_total_nr(&new_todo);\n>>> +\tres = pick_commits(r, &new_todo, opts);\n>>> +\ttodo_list_release(&new_todo);\n>>> +\n>>> +\treturn res;\n>>> }\n>>>\n>>> struct subject2item_entry {\n>>>\n>>\n\n"},{"id":"383292","messageId":"c276de00-fffb-c483-e1b9-526052544fe8@gmail.com","threadId":"51918","inReplyTo":"xmqq5zl7hnwe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-02T18:37:03Z","receivedAt":"2019-10-02T18:37:10Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio,\n\nLe 02/10/2019 à 04:38, Junio C Hamano a écrit :\n> Alban Gruin writes:\n> \n>> Currently, complete_action() calls sequencer_continue() to do the\n>> rebase. Even though the former already has the todo list, the latter\n>> loads it from the disk and parses it. Calling directly pick_commits()\n>> from complete_action() avoids this unnecessary round trip.\n>>\n>> Signed-off-by: Alban Gruin \n>> ---\n>> sequencer.c | 8 +++++---\n>> 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> All the earlier steps in this series are necessary because the\n> inconsistencies caused by not doing things the code updated by the\n> earlier steps do (e.g. leaving number of commits recorded in\n> total_nr and done_nr stale) were masked by re-reading the info from\n> the on-disk file. And this step exposes these inconsistencies\n> because the extra reading from the file no longer happens.\n> \n> That is my reading of the series---did I get it right?\n> \n\nYes.\n\n> I wonder if that can be stated clearly in the log message of the\n> earlier steps.\n> \n> For example, by reading 1/5 or 2/5 alone, it would be natural for\n> readers to wonder why the original code that did not update done_nr\n> correctly terminated after going through the todo list (you would\n> expect that when done reaches total you are finished, so if one of\n> these variables are not maintained correctly, your termination\n> condition may be borked), and then by knowing that the current code\n> more-or-less works correctly by not terminating too early or barfing\n> to say it ran out of things to do prematurely, the next thing\n> readers would wonder is if these patches introduce new bugs by\n> incrementing these variables twice (iow, \"does the author of the\n> patch missed other places where these variables are correctly\n> updated?\")\n> \n> 1/5 for example talks about the variable mostly being used by\n> prompts, which may be useful to reduce worries by readers about\n> correctness (iow, \"well, if it is only showing wrong number in the\n> prompts and does not affect correctness otherwise, perhaps that was\n> why the current code that is buggy did not behave incorrectly\").\n> But I suspect that it is not why these changes in the earlier steps\n> are acceptable or are right things to do.\n\nInterestingly, it is the only reason for these two patches. If you skip\nthem both, t34??-*.sh will not fail, only t9903-bash-prompt.sh will.\nThis is because `total_nr' does not reflect the real number of items in\na todo list, but the number of steps, done *and* remaining. When a\nrebase is resumed, some commands may already have been run, and are no\nlonger part of the todo list. So, it’s not used to determine whether or\nnot the rebase is done. `nr' is used for this purpose. `done_nr' and\n`total_nr' are just used for user interaction.\n\n> So, perhaps replace the\n> \"these are used only in prompts and the numbers being wrong does not\n> affect correctness\" comments from them with:\n> \n> By forgetting to update the variable, the original code made it\n> not reflect the reality, but this flaw was masked by the code\n> calling (sometimes unnecessarily) read_todo_list() again to\n> update the variable to its correct value before the next action\n> happens. At the end of this series, the unnecessary call will\n> be removed and the inconsistency addressed by this patch would\n> start to matter.\n> \n> or something like that (assuming that the answer to the first\n> question I asked in this message is \"yes\", that is), or a more\n> concise version of it?\n> \n\nI will do so.\n\nCheers,\nAlban\n\n\n\n> Thanks.\n> \n\n"},{"id":"383564","messageId":"20191007092641.12661-1-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20190925201315.19722-1-alban.gruin@gmail.com","subject":"[PATCH v2 0/5] Use complete_action's todo list to do the rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-07T09:26:36Z","receivedAt":"2019-10-07T09:27:00Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This can be seen as a continuation of ag/reduce-rewriting-todo.\n\nCurrently, complete_action() releases its todo list before calling\nsequencer_continue(), which reloads the todo list from the disk. This\nseries removes this useless round trip.\n\nPatches 1, 2, and 3 originally come from a series meaning to improve\nrebase.missingCommitsCheck[0]. In the original series, I wanted to\ncheck for missing commits in read_populate_todo(), so a warning could be\nissued after a `rebase --continue' or an `exec' commands. But, in the\ncase of the initial edit, it is already checked in complete_action(),\nand would be checked a second time in sequencer_continue() (a caller of\nread_populate_todo()). So I hacked up sequencer_continue() to accept a\npointer to a todo list, and if not null, would skip the call to\nread_populate_todo(). (This was really ugly, to be honest.) Some\nissues arose with git-prompt.sh[1], hence 1, 2 and 3.\n\nPatch 5 is a new approach to what I did first. Instead of bolting a new\nparameter to sequencer_continue(), this makes complete_action() calling\ndirectly pick_commits().\n\nThis is based on 4c86140027 (\"Third batch\").\n\nChanges since v1:\n\n - Rewording of patches 1, 2, 4 and 5 according to comments made by\n Phillip Wood, Junio C Hamano and Johannes Schindelin.\n\nThe tip of this series is tagged as \"reduce-todo-list-cont-v2\" at\nhttps://github.com/agrn/git.\n\n[0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n[1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n\nAlban Gruin (5):\n sequencer: update `total_nr' when adding an item to a todo list\n sequencer: update `done_nr' when skipping commands in a todo list\n sequencer: move the code writing total_nr on the disk to a new\n function\n rebase: fill `squash_onto' in get_replay_opts()\n sequencer: directly call pick_commits() from complete_action()\n\n builtin/rebase.c | 5 +++++\n sequencer.c | 26 ++++++++++++++++++--------\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\nDiff-intervalle contre v1 :\n1: d177b0de1a ! 1: 9215b191c7 sequencer: update `total_nr' when adding an item to a todo list\n @@ Metadata\n ## Commit message ##\n sequencer: update `total_nr' when adding an item to a todo list\n \n - `total_nr' is the total amount of items, done and toto, that are in a\n - todo list. But unlike `nr', it was not updated when an item was\n - appended to the list.\n + `total_nr' is the total number of items, counting both done and todo,\n + that are in a todo list. But unlike `nr', it was not updated when an\n + item was appended to the list.\n \n This variable is mostly used by command prompts (ie. git-prompt.sh and\n - the like).\n + the like). By forgetting to update it, the original code made it not\n + reflect the reality, but this flaw was masked by the code calling\n + unnecessarily read_todo_list() again to update the variable to its\n + correct value. At the end of this series, the unnecessary call will be\n + removed, and the inconsistency addressed by this patch would start to\n + matter.\n \n Signed-off-by: Alban Gruin \n \n2: 09fcbe159b ! 2: 7cad541092 sequencer: update `done_nr' when skipping commands in a todo list\n @@ Commit message\n or skipped, but skip_unnecessary_picks() did not update it.\n \n This variable is mostly used by command prompts (ie. git-prompt.sh and\n - the like).\n + the like). As in the previous commit, this inconsistent behaviour is\n + not a problem yet, but it would start to matter at the end of this\n + series the same reason.\n \n Signed-off-by: Alban Gruin \n \n3: 26a18cd1a9 = 3: 7c9c4ddd30 sequencer: move the code writing total_nr on the disk to a new function\n4: 5d74903cfe ! 4: cd44fb4e10 rebase: fill `squash_onto' in get_replay_opts()\n @@ Metadata\n ## Commit message ##\n rebase: fill `squash_onto' in get_replay_opts()\n \n - get_replay_opts() did not fill `squash_onto' if possible, meaning that\n - this field should be read from the disk by the sequencer through\n - read_populate_opts(). Without this, calling `pick_commits()' directly\n - will result in incorrect results with `rebase --root'.\n + Currently, the get_replay_opts() function does not initialise the\n + `squash_onto' field (which is used for the `--root' mode), only\n + read_populate_opts() does. That would lead to incorrect results when\n + calling pick_commits() without reading the options from the disk first.\n \n Let’s change that.\n \n5: dc803c671f ! 5: 523fdd35a1 sequencer: directly call pick_commits() from complete_action()\n @@ Commit message\n sequencer: directly call pick_commits() from complete_action()\n \n Currently, complete_action() calls sequencer_continue() to do the\n - rebase. Even though the former already has the todo list, the latter\n - loads it from the disk and parses it. Calling directly pick_commits()\n - from complete_action() avoids this unnecessary round trip.\n + rebase. Before the former calls pick_commits(), it\n +\n + - calls read_and_refresh_cache() -- this is unnecessary here as we've\n + just called require_clean_work_tree()\n + - calls read_populate_opts() -- this is unnecessary as we're starting a\n + new rebase, so opts is fully populated\n + - loads the todo list -- this is unnecessary as we've just populated\n + the todo list\n + - commits any staged changes -- this is unnecessary as we're starting a\n + new rebase, so there are no staged changes\n + - calls record_in_rewritten() -- this is unnecessary as we're starting\n + a new rebase.\n +\n + This changes complete_action() to directly call pick_commits() to avoid\n + these unnecessary steps.\n \n Signed-off-by: Alban Gruin \n \n-- \n2.23.0\n\n"},{"id":"383565","messageId":"20191007092641.12661-2-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"[PATCH v2 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-07T09:26:37Z","receivedAt":"2019-10-07T09:27:00Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"`total_nr' is the total number of items, counting both done and todo,\nthat are in a todo list. But unlike `nr', it was not updated when an\nitem was appended to the list.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like). By forgetting to update it, the original code made it not\nreflect the reality, but this flaw was masked by the code calling\nunnecessarily read_todo_list() again to update the variable to its\ncorrect value. At the end of this series, the unnecessary call will be\nremoved, and the inconsistency addressed by this patch would start to\nmatter.\n\nSigned-off-by: Alban Gruin \n---\nReworded commit.\n\nsequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d648aaf416..575b852a5a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2070,6 +2070,7 @@ 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-- \n2.23.0\n\n"},{"id":"383566","messageId":"20191007092641.12661-3-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"[PATCH v2 2/5] sequencer: update `done_nr' when skipping commands in a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-07T09:26:38Z","receivedAt":"2019-10-07T09:27:04Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"In a todo list, `done_nr' is the amount of commands that were executed\nor skipped, but skip_unnecessary_picks() did not update it.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like). As in the previous commit, this inconsistent behaviour is\nnot a problem yet, but it would start to matter at the end of this\nseries the same reason.\n\nSigned-off-by: Alban Gruin \n---\nReworded commit.\n\nsequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 575b852a5a..42313f8de6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,\n \t\tMOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);\n \t\ttodo_list->nr -= i;\n \t\ttodo_list->current = 0;\n+\t\ttodo_list->done_nr += i;\n \n \t\tif (is_fixup(peek_command(todo_list, 0)))\n \t\t\trecord_in_rewritten(base_oid, peek_command(todo_list, 0));\n-- \n2.23.0\n\n"},{"id":"383567","messageId":"20191007092641.12661-4-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"[PATCH v2 3/5] sequencer: move the code writing total_nr on the disk to a new function","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-07T09:26:39Z","receivedAt":"2019-10-07T09:27:05Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"The total amount of commands can be used to show the progression of the\nrebasing in a shell. It is written to the disk by read_populate_todo()\nwhen the todo list is loaded from sequencer_continue() or\npick_commits(), but not by complete_action().\n\nThis moves the part writing total_nr to a new function so it can be\ncalled from complete_action().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 42313f8de6..ec7ea8d9e5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n \tsequencer_remove_state(&opts);\n }\n \n+static void todo_list_write_total_nr(struct todo_list *todo_list)\n+{\n+\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n+\n+\tif (f) {\n+\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n+\t\tfclose(f);\n+\t}\n+}\n+\n static int read_populate_todo(struct repository *r,\n \t\t\t struct todo_list *todo_list,\n \t\t\t struct replay_opts *opts)\n@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,\n \n \tif (is_rebase_i(opts)) {\n \t\tstruct todo_list done = TODO_LIST_INIT;\n-\t\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n \t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,\n \t\t\t+ count_commands(todo_list);\n \t\ttodo_list_release(&done);\n \n-\t\tif (f) {\n-\t\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n-\t\t\tfclose(f);\n-\t\t}\n+\t\ttodo_list_write_total_nr(todo_list);\n \t}\n \n \treturn 0;\n-- \n2.23.0\n\n"},{"id":"383568","messageId":"20191007092641.12661-5-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"[PATCH v2 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-07T09:26:40Z","receivedAt":"2019-10-07T09:27:07Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Currently, the get_replay_opts() function does not initialise the\n`squash_onto' field (which is used for the `--root' mode), only\nread_populate_opts() does. That would lead to incorrect results when\ncalling pick_commits() without reading the options from the disk first.\n\nLet’s change that.\n\nSigned-off-by: Alban Gruin \n---\nReworded commit.\n\nbuiltin/rebase.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e8319d5946..2097d41edc 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \tif (opts->strategy_opts)\n \t\tparse_strategy_opts(&replay, opts->strategy_opts);\n \n+\tif (opts->squash_onto) {\n+\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n+\t\treplay.have_squash_onto = 1;\n+\t}\n+\n \treturn replay;\n }\n \n-- \n2.23.0\n\n"},{"id":"383569","messageId":"20191007092641.12661-6-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"[PATCH v2 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-07T09:26:41Z","receivedAt":"2019-10-07T09:27:08Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Currently, complete_action() calls sequencer_continue() to do the\nrebase. Before the former calls pick_commits(), it\n\n - calls read_and_refresh_cache() -- this is unnecessary here as we've\n just called require_clean_work_tree()\n - calls read_populate_opts() -- this is unnecessary as we're starting a\n new rebase, so opts is fully populated\n - loads the todo list -- this is unnecessary as we've just populated\n the todo list\n - commits any staged changes -- this is unnecessary as we're starting a\n new rebase, so there are no staged changes\n - calls record_in_rewritten() -- this is unnecessary as we're starting\n a new rebase.\n\nThis changes complete_action() to directly call pick_commits() to avoid\nthese unnecessary steps.\n\nSigned-off-by: Alban Gruin \n---\nReworded commit.\n\nsequencer.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ec7ea8d9e5..b395dd6e11 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \t}\n \n-\ttodo_list_release(&new_todo);\n-\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n \t\treturn -1;\n \n \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n \t\treturn -1;\n \n-\treturn sequencer_continue(r, opts);\n+\ttodo_list_write_total_nr(&new_todo);\n+\tres = pick_commits(r, &new_todo, opts);\n+\ttodo_list_release(&new_todo);\n+\n+\treturn res;\n }\n \n struct subject2item_entry {\n-- \n2.23.0\n\n"},{"id":"383644","messageId":"xmqqmuecnefe.fsf@gitster-ct.c.googlers.com","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"Re: [PATCH v2 0/5] Use complete_action's todo list to do the rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-08T02:45:09Z","receivedAt":"2019-10-08T02:45:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> This can be seen as a continuation of ag/reduce-rewriting-todo.\n>\n> Currently, complete_action() releases its todo list before calling\n> sequencer_continue(), which reloads the todo list from the disk. This\n> series removes this useless round trip.\n>\n> Patches 1, 2, and 3 originally come from a series meaning to improve\n> rebase.missingCommitsCheck[0]. In the original series, I wanted to\n> check for missing commits in read_populate_todo(), so a warning could be\n> issued after a `rebase --continue' or an `exec' commands. But, in the\n> case of the initial edit, it is already checked in complete_action(),\n> and would be checked a second time in sequencer_continue() (a caller of\n> read_populate_todo()). So I hacked up sequencer_continue() to accept a\n> pointer to a todo list, and if not null, would skip the call to\n> read_populate_todo(). (This was really ugly, to be honest.) Some\n> issues arose with git-prompt.sh[1], hence 1, 2 and 3.\n>\n> Patch 5 is a new approach to what I did first. Instead of bolting a new\n> parameter to sequencer_continue(), this makes complete_action() calling\n> directly pick_commits().\n>\n> This is based on 4c86140027 (\"Third batch\").\n>\n> Changes since v1:\n>\n> - Rewording of patches 1, 2, 4 and 5 according to comments made by\n> Phillip Wood, Junio C Hamano and Johannes Schindelin.\n>\n> The tip of this series is tagged as \"reduce-todo-list-cont-v2\" at\n> https://github.com/agrn/git.\n>\n> [0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n> [1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n>\n> Alban Gruin (5):\n> sequencer: update `total_nr' when adding an item to a todo list\n> sequencer: update `done_nr' when skipping commands in a todo list\n> sequencer: move the code writing total_nr on the disk to a new\n> function\n> rebase: fill `squash_onto' in get_replay_opts()\n> sequencer: directly call pick_commits() from complete_action()\n>\n> builtin/rebase.c | 5 +++++\n> sequencer.c | 26 ++++++++++++++++++--------\n> 2 files changed, 23 insertions(+), 8 deletions(-)\n>\n> Diff-intervalle contre v1 :\n> 1: d177b0de1a ! 1: 9215b191c7 sequencer: update `total_nr' when adding an item to a todo list\n> @@ Metadata\n> ## Commit message ##\n> sequencer: update `total_nr' when adding an item to a todo list\n> \n> - `total_nr' is the total amount of items, done and toto, that are in a\n> - todo list. But unlike `nr', it was not updated when an item was\n> - appended to the list.\n> + `total_nr' is the total number of items, counting both done and todo,\n\nThe same s/amount/number/ needs to be done to the log message of\npatches 2/5 and 3/5. Other than that, updated log messages looked\nmuch more understandable. Thanks.\n"},{"id":"383688","messageId":"49b2fa05-5954-fa17-3f5a-5c4e11e11192@gmail.com","threadId":"51918","inReplyTo":"xmqqmuecnefe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/5] Use complete_action's todo list to do the rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-08T16:18:18Z","receivedAt":"2019-10-08T16:18:41Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio,\n\nLe 08/10/2019 à 04:45, Junio C Hamano a écrit :\n> Alban Gruin writes:\n> \n>> This can be seen as a continuation of ag/reduce-rewriting-todo.\n>>\n>> Currently, complete_action() releases its todo list before calling\n>> sequencer_continue(), which reloads the todo list from the disk. This\n>> series removes this useless round trip.\n>>\n>> Patches 1, 2, and 3 originally come from a series meaning to improve\n>> rebase.missingCommitsCheck[0]. In the original series, I wanted to\n>> check for missing commits in read_populate_todo(), so a warning could be\n>> issued after a `rebase --continue' or an `exec' commands. But, in the\n>> case of the initial edit, it is already checked in complete_action(),\n>> and would be checked a second time in sequencer_continue() (a caller of\n>> read_populate_todo()). So I hacked up sequencer_continue() to accept a\n>> pointer to a todo list, and if not null, would skip the call to\n>> read_populate_todo(). (This was really ugly, to be honest.) Some\n>> issues arose with git-prompt.sh[1], hence 1, 2 and 3.\n>>\n>> Patch 5 is a new approach to what I did first. Instead of bolting a new\n>> parameter to sequencer_continue(), this makes complete_action() calling\n>> directly pick_commits().\n>>\n>> This is based on 4c86140027 (\"Third batch\").\n>>\n>> Changes since v1:\n>>\n>> - Rewording of patches 1, 2, 4 and 5 according to comments made by\n>> Phillip Wood, Junio C Hamano and Johannes Schindelin.\n>>\n>> The tip of this series is tagged as \"reduce-todo-list-cont-v2\" at\n>> https://github.com/agrn/git.\n>>\n>> [0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n>> [1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n>>\n>> Alban Gruin (5):\n>> sequencer: update `total_nr' when adding an item to a todo list\n>> sequencer: update `done_nr' when skipping commands in a todo list\n>> sequencer: move the code writing total_nr on the disk to a new\n>> function\n>> rebase: fill `squash_onto' in get_replay_opts()\n>> sequencer: directly call pick_commits() from complete_action()\n>>\n>> builtin/rebase.c | 5 +++++\n>> sequencer.c | 26 ++++++++++++++++++--------\n>> 2 files changed, 23 insertions(+), 8 deletions(-)\n>>\n>> Diff-intervalle contre v1 :\n>> 1: d177b0de1a ! 1: 9215b191c7 sequencer: update `total_nr' when adding an item to a todo list\n>> @@ Metadata\n>> ## Commit message ##\n>> sequencer: update `total_nr' when adding an item to a todo list\n>> \n>> - `total_nr' is the total amount of items, done and toto, that are in a\n>> - todo list. But unlike `nr', it was not updated when an item was\n>> - appended to the list.\n>> + `total_nr' is the total number of items, counting both done and todo,\n> \n> The same s/amount/number/ needs to be done to the log message of\n> patches 2/5 and 3/5. Other than that, updated log messages looked\n> much more understandable. Thanks.\n> \n\nOuch, sorry about that. Thank you for fixing them.\n\nCheers,\nAlban\n\n"},{"id":"384014","messageId":"nycvar.QRO.7.76.6.1910141445250.46@tvgsbejvaqbjf.bet","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"Re: [PATCH v2 0/5] Use complete_action's todo list to do the rebase","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-14T12:49:09Z","receivedAt":"2019-10-14T12:49:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Mon, 7 Oct 2019, Alban Gruin wrote:\n\n> This can be seen as a continuation of ag/reduce-rewriting-todo.\n>\n> Currently, complete_action() releases its todo list before calling\n> sequencer_continue(), which reloads the todo list from the disk. This\n> series removes this useless round trip.\n>\n> Patches 1, 2, and 3 originally come from a series meaning to improve\n> rebase.missingCommitsCheck[0]. In the original series, I wanted to\n> check for missing commits in read_populate_todo(), so a warning could be\n> issued after a `rebase --continue' or an `exec' commands. But, in the\n> case of the initial edit, it is already checked in complete_action(),\n> and would be checked a second time in sequencer_continue() (a caller of\n> read_populate_todo()). So I hacked up sequencer_continue() to accept a\n> pointer to a todo list, and if not null, would skip the call to\n> read_populate_todo(). (This was really ugly, to be honest.) Some\n> issues arose with git-prompt.sh[1], hence 1, 2 and 3.\n>\n> Patch 5 is a new approach to what I did first. Instead of bolting a new\n> parameter to sequencer_continue(), this makes complete_action() calling\n> directly pick_commits().\n>\n> This is based on 4c86140027 (\"Third batch\").\n>\n> Changes since v1:\n>\n> - Rewording of patches 1, 2, 4 and 5 according to comments made by\n> Phillip Wood, Junio C Hamano and Johannes Schindelin.\n>\n> The tip of this series is tagged as \"reduce-todo-list-cont-v2\" at\n> https://github.com/agrn/git.\n>\n> [0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n> [1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n>\n> Alban Gruin (5):\n> sequencer: update `total_nr' when adding an item to a todo list\n> sequencer: update `done_nr' when skipping commands in a todo list\n> sequencer: move the code writing total_nr on the disk to a new\n> function\n> rebase: fill `squash_onto' in get_replay_opts()\n> sequencer: directly call pick_commits() from complete_action()\n>\n> builtin/rebase.c | 5 +++++\n> sequencer.c | 26 ++++++++++++++++++--------\n> 2 files changed, 23 insertions(+), 8 deletions(-)\n>\n> Diff-intervalle contre v1 :\n> 1: d177b0de1a ! 1: 9215b191c7 sequencer: update `total_nr' when adding an item to a todo list\n> @@ Metadata\n> ## Commit message ##\n> sequencer: update `total_nr' when adding an item to a todo list\n>\n> - `total_nr' is the total amount of items, done and toto, that are in a\n> - todo list. But unlike `nr', it was not updated when an item was\n> - appended to the list.\n> + `total_nr' is the total number of items, counting both done and todo,\n> + that are in a todo list. But unlike `nr', it was not updated when an\n> + item was appended to the list.\n>\n> This variable is mostly used by command prompts (ie. git-prompt.sh and\n> - the like).\n> + the like). By forgetting to update it, the original code made it not\n> + reflect the reality, but this flaw was masked by the code calling\n> + unnecessarily read_todo_list() again to update the variable to its\n> + correct value. At the end of this series, the unnecessary call will be\n> + removed, and the inconsistency addressed by this patch would start to\n> + matter.\n>\n> Signed-off-by: Alban Gruin \n>\n> 2: 09fcbe159b ! 2: 7cad541092 sequencer: update `done_nr' when skipping commands in a todo list\n> @@ Commit message\n> or skipped, but skip_unnecessary_picks() did not update it.\n>\n> This variable is mostly used by command prompts (ie. git-prompt.sh and\n> - the like).\n> + the like). As in the previous commit, this inconsistent behaviour is\n> + not a problem yet, but it would start to matter at the end of this\n> + series the same reason.\n>\n> Signed-off-by: Alban Gruin \n>\n> 3: 26a18cd1a9 = 3: 7c9c4ddd30 sequencer: move the code writing total_nr on the disk to a new function\n> 4: 5d74903cfe ! 4: cd44fb4e10 rebase: fill `squash_onto' in get_replay_opts()\n> @@ Metadata\n> ## Commit message ##\n> rebase: fill `squash_onto' in get_replay_opts()\n>\n> - get_replay_opts() did not fill `squash_onto' if possible, meaning that\n> - this field should be read from the disk by the sequencer through\n> - read_populate_opts(). Without this, calling `pick_commits()' directly\n> - will result in incorrect results with `rebase --root'.\n> + Currently, the get_replay_opts() function does not initialise the\n> + `squash_onto' field (which is used for the `--root' mode), only\n> + read_populate_opts() does. That would lead to incorrect results when\n> + calling pick_commits() without reading the options from the disk first.\n>\n> Let’s change that.\n>\n> 5: dc803c671f ! 5: 523fdd35a1 sequencer: directly call pick_commits() from complete_action()\n> @@ Commit message\n> sequencer: directly call pick_commits() from complete_action()\n>\n> Currently, complete_action() calls sequencer_continue() to do the\n> - rebase. Even though the former already has the todo list, the latter\n> - loads it from the disk and parses it. Calling directly pick_commits()\n> - from complete_action() avoids this unnecessary round trip.\n> + rebase. Before the former calls pick_commits(), it\n> +\n> + - calls read_and_refresh_cache() -- this is unnecessary here as we've\n> + just called require_clean_work_tree()\n> + - calls read_populate_opts() -- this is unnecessary as we're starting a\n> + new rebase, so opts is fully populated\n> + - loads the todo list -- this is unnecessary as we've just populated\n> + the todo list\n> + - commits any staged changes -- this is unnecessary as we're starting a\n> + new rebase, so there are no staged changes\n> + - calls record_in_rewritten() -- this is unnecessary as we're starting\n> + a new rebase.\n> +\n> + This changes complete_action() to directly call pick_commits() to avoid\n> + these unnecessary steps.\n>\n> Signed-off-by: Alban Gruin \n\nThis range-diff looks good to me!\n\nI just verified that b2 addresses all the concerns I offered for v1.\n\nThanks,\nDscho\n"},{"id":"384905","messageId":"67c98856-55dc-df95-c4a6-61f1af85c480@web.de","threadId":"51918","inReplyTo":"20190925201315.19722-6-alban.gruin@gmail.com","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-10-26T07:47:29Z","receivedAt":"2019-10-26T07:48:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.09.19 um 22:13 schrieb Alban Gruin:\n> Currently, complete_action() calls sequencer_continue() to do the\n> rebase. Even though the former already has the todo list, the latter\n> loads it from the disk and parses it. Calling directly pick_commits()\n> from complete_action() avoids this unnecessary round trip.\n>\n> Signed-off-by: Alban Gruin \n> ---\n> sequencer.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index ec7ea8d9e5..b395dd6e11 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n> \t}\n>\n> -\ttodo_list_release(&new_todo);\n> -\n\nMoving this call down leaks new_todo on error...\n\n> \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n> \t\treturn -1;\n\n... here ...\n\n>\n> \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n> \t\treturn -1;\n... and here. Perhaps set res to -1 and jump to the end?\n\n>\n> -\treturn sequencer_continue(r, opts);\n> +\ttodo_list_write_total_nr(&new_todo);\n> +\tres = pick_commits(r, &new_todo, opts);\n> +\ttodo_list_release(&new_todo);\n> +\n> +\treturn res;\n> }\n>\n> struct subject2item_entry {\n>\n\n"},{"id":"384910","messageId":"9b81e879-091b-bbf7-2d1b-51236d19ee9a@gmail.com","threadId":"51918","inReplyTo":"67c98856-55dc-df95-c4a6-61f1af85c480@web.de","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-10-26T11:27:21Z","receivedAt":"2019-10-26T11:27:38Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi René,\n\nLe 26/10/2019 à 09:47, René Scharfe a écrit :\n> Am 25.09.19 um 22:13 schrieb Alban Gruin:\n>> Currently, complete_action() calls sequencer_continue() to do the\n>> rebase. Even though the former already has the todo list, the latter\n>> loads it from the disk and parses it. Calling directly pick_commits()\n>> from complete_action() avoids this unnecessary round trip.\n>>\n>> Signed-off-by: Alban Gruin \n>> ---\n>> sequencer.c | 8 +++++---\n>> 1 file changed, 5 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index ec7ea8d9e5..b395dd6e11 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>> \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n>> \t}\n>>\n>> -\ttodo_list_release(&new_todo);\n>> -\n> \n> Moving this call down leaks new_todo on error...\n> \n>> \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n>> \t\treturn -1;\n> \n> ... here ...\n> \n>>\n>> \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n>> \t\treturn -1;\n> ... and here. Perhaps set res to -1 and jump to the end?\n> \n>>\n>> -\treturn sequencer_continue(r, opts);\n>> +\ttodo_list_write_total_nr(&new_todo);\n>> +\tres = pick_commits(r, &new_todo, opts);\n>> +\ttodo_list_release(&new_todo);\n>> +\n>> +\treturn res;\n>> }\n>>\n>> struct subject2item_entry {\n>>\n> \n\nThank you for the heads up.\n\nJunio, how do you want me to fix that? Should I reroll the series\naltogether, send a \"fixup!\" commit, or send a standalone patch?\n\nCheers,\nAlban\n\n"},{"id":"384957","messageId":"xmqqeeyxbqc6.fsf@gitster-ct.c.googlers.com","threadId":"51918","inReplyTo":"9b81e879-091b-bbf7-2d1b-51236d19ee9a@gmail.com","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-28T01:39:53Z","receivedAt":"2019-10-28T01:39:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> Junio, how do you want me to fix that? Should I reroll the series\n> altogether, send a \"fixup!\" commit, or send a standalone patch?\n\nNormally a topic that is not yet in 'next' or more stable tracks can\nand should be rerolled (or in a rare case like changing just a\nsingle typo on a line in the last patch in a series, can be\ncorrected with a \"fixup!\" squashed in); once the topic is in 'next'\nor more stable integration branch, there needs a normal freestanding\npatch that may say \"earlier we did X, which is broken for such and\nsuch reasons. correct it this way.\"\n\nBut during a pre-release feature freeze period, the rules can be a\nbit different ;-) Typically a topic that is not in 'master' that is\nnot a bugfix will never graduate from 'next' until the release\nhappens, and after the release, the tip of 'next' gets rebuilt from\nthe newly cut release, at which point a topic that wants a fresh\nstart (in order to avoid \"oops, this was wrong, and here is a fix\"\nfollow-up patch) can be granted one.\n\nSo I'd probably just send a \"fixup!\"to be queued on top of the\nseries to fix a leak in 'next' for now, remind the maintainer not to\nmerge it to 'master' before the release, and once the upcoming\nrelease is made, send another reminder to the maintainer to squash\nthe \"fixup!\" in before rebuilding 'next', if I owned this series.\n\nThanks.\n"},{"id":"384964","messageId":"xmqqftjda745.fsf@gitster-ct.c.googlers.com","threadId":"51918","inReplyTo":"xmqqeeyxbqc6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-28T03:20:26Z","receivedAt":"2019-10-28T03:20:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So I'd probably just send a \"fixup!\"to be queued on top of the\n> series to fix a leak in 'next' for now, remind the maintainer not to\n> merge it to 'master' before the release, and once the upcoming\n> release is made, send another reminder to the maintainer to squash\n> the \"fixup!\" in before rebuilding 'next', if I owned this series.\n\nThis is based on René's suggestion, but seeing the pre-context of\nthis fix, many of the error code path, after an error return from\nedit_todo_list() has been handled, can follow the pattern of this\nfix to jump to the cleanup label after losing their own calls to\ntodo_list_release(). An obvious advantage of doing so is that it\nfuture-proofs the codepath---the todo-list structure may not stay to\nbe the only thing that holds resources that need cleaned up.\n\n sequencer.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b395dd6e11..ec0b793fc5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5140,14 +5140,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \t}\n \n+\tres = -1;\n+\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n-\t\treturn -1;\n+\t\tgoto cleanup;\n \n \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n-\t\treturn -1;\n+\t\tgoto cleanup;\n \n \ttodo_list_write_total_nr(&new_todo);\n \tres = pick_commits(r, &new_todo, opts);\n+\n+cleanup:\n \ttodo_list_release(&new_todo);\n \n \treturn res;\n"},{"id":"386902","messageId":"20191123143705.17280-2-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191123143705.17280-1-alban.gruin@gmail.com","subject":"[PATCH v3 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-23T14:37:01Z","receivedAt":"2019-11-23T14:45:57Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"`total_nr' is the total number of items, counting both done and todo,\nthat are in a todo list. But unlike `nr', it was not updated when an\nitem was appended to the list.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like). By forgetting to update it, the original code made it not\nreflect the reality, but this flaw was masked by the code calling\nunnecessarily read_populate_todo() again to update the variable to its\ncorrect value. At the end of this series, the unnecessary call will be\nremoved, and the inconsistency addressed by this patch would start to\nmatter.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d648aaf416..575b852a5a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2070,6 +2070,7 @@ 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-- \n2.24.0\n\n"},{"id":"386903","messageId":"20191123143705.17280-5-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191123143705.17280-1-alban.gruin@gmail.com","subject":"[PATCH v3 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-23T14:37:04Z","receivedAt":"2019-11-23T14:46:35Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"When sequencer_continue() is called by complete_action(), `opts' has\nbeen filled by get_replay_opts(). Currently, it does not initialise the\n`squash_onto' field (used by the `--root' mode), only\nread_populate_opts() does. It’s not a problem yet since\nsequencer_continue() calls it before pick_commits(), but it would lead\nto incorrect results once complete_action() is modified to call\npick_commits() directly.\n\nLet’s change that.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e8319d5946..2097d41edc 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \tif (opts->strategy_opts)\n \t\tparse_strategy_opts(&replay, opts->strategy_opts);\n \n+\tif (opts->squash_onto) {\n+\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n+\t\treplay.have_squash_onto = 1;\n+\t}\n+\n \treturn replay;\n }\n \n-- \n2.24.0\n\n"},{"id":"386904","messageId":"20191123143705.17280-1-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191007092641.12661-1-alban.gruin@gmail.com","subject":"[PATCH v3 0/5] Use complete_action's todo list to do the rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-23T14:37:00Z","receivedAt":"2019-11-23T14:46:57Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This can be seen as a continuation of ag/reduce-rewriting-todo.\n\nCurrently, complete_action() releases its todo list before calling\nsequencer_continue(), which reloads the todo list from the disk. This\nseries removes this useless round trip.\n\nPatches 1, 2, and 3 originally come from a series meaning to improve\nrebase.missingCommitsCheck[0]. In the original series, I wanted to\ncheck for missing commits in read_populate_todo(), so a warning could be\nissued after a `rebase --continue' or an `exec' commands. But, in the\ncase of the initial edit, it is already checked in complete_action(),\nand would be checked a second time in sequencer_continue() (a caller of\nread_populate_todo()). So I hacked up sequencer_continue() to accept a\npointer to a todo list, and if not null, would skip the call to\nread_populate_todo(). (This was really ugly, to be honest.) Some\nissues arose with git-prompt.sh[1], hence 1, 2 and 3.\n\nPatch 5 is a new approach to what I did first. Instead of bolting a new\nparameter to sequencer_continue(), this makes complete_action() calling\ndirectly pick_commits().\n\nThis is based on 4c86140027 (\"Third batch\").\n\nChanges since v2:\n\n - Patch 1 has been reworded to fix a mistake in read_populate_todo()'s\n name, reported by Jonathan Tan.\n - Patches 4 and 5 has been reworded to improve descriptions of the code\n paths, according to comments made by Jonathan Tan.\n - Squashed b0537b0ec3 (\"SQUASH??? tentative leakfix\") into the 5th\n patch to fix a memory leak reported by René Sharfe.\n\nThe tip of this series is tagged as \"reduce-todo-list-cont-v3\" at\nhttps://github.com/agrn/git.\n\n[0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n[1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n\nAlban Gruin (5):\n sequencer: update `total_nr' when adding an item to a todo list\n sequencer: update `done_nr' when skipping commands in a todo list\n sequencer: move the code writing total_nr on the disk to a new\n function\n rebase: fill `squash_onto' in get_replay_opts()\n sequencer: directly call pick_commits() from complete_action()\n\n builtin/rebase.c | 5 +++++\n sequencer.c | 32 +++++++++++++++++++++++---------\n 2 files changed, 28 insertions(+), 9 deletions(-)\n\nDiff-intervalle contre v2 :\n1: 9215b191c7 ! 1: 11a221e82e sequencer: update `total_nr' when adding an item to a todo list\n @@ Commit message\n This variable is mostly used by command prompts (ie. git-prompt.sh and\n the like). By forgetting to update it, the original code made it not\n reflect the reality, but this flaw was masked by the code calling\n - unnecessarily read_todo_list() again to update the variable to its\n + unnecessarily read_populate_todo() again to update the variable to its\n correct value. At the end of this series, the unnecessary call will be\n removed, and the inconsistency addressed by this patch would start to\n matter.\n2: 7cad541092 = 2: 76a3af70b6 sequencer: update `done_nr' when skipping commands in a todo list\n3: 7c9c4ddd30 = 3: 9c5bd30465 sequencer: move the code writing total_nr on the disk to a new function\n4: cd44fb4e10 ! 4: bc3d69a10e rebase: fill `squash_onto' in get_replay_opts()\n @@ Metadata\n ## Commit message ##\n rebase: fill `squash_onto' in get_replay_opts()\n \n - Currently, the get_replay_opts() function does not initialise the\n - `squash_onto' field (which is used for the `--root' mode), only\n - read_populate_opts() does. That would lead to incorrect results when\n - calling pick_commits() without reading the options from the disk first.\n + When sequencer_continue() is called by complete_action(), `opts' has\n + been filled by get_replay_opts(). Currently, it does not initialise the\n + `squash_onto' field (used by the `--root' mode), only\n + read_populate_opts() does. It’s not a problem yet since\n + sequencer_continue() calls it before pick_commits(), but it would lead\n + to incorrect results once complete_action() is modified to call\n + pick_commits() directly.\n \n Let’s change that.\n \n5: 523fdd35a1 ! 5: e7691db66b sequencer: directly call pick_commits() from complete_action()\n @@ Metadata\n ## Commit message ##\n sequencer: directly call pick_commits() from complete_action()\n \n - Currently, complete_action() calls sequencer_continue() to do the\n - rebase. Before the former calls pick_commits(), it\n + Currently, complete_action(), used by builtin/rebase.c to start a new\n + rebase, calls sequencer_continue() to do it. Before the former calls\n + pick_commits(), it\n \n - calls read_and_refresh_cache() -- this is unnecessary here as we've\n - just called require_clean_work_tree()\n + just called require_clean_work_tree() in complete_action()\n - calls read_populate_opts() -- this is unnecessary as we're starting a\n - new rebase, so opts is fully populated\n + new rebase, so `opts' is fully populated\n - loads the todo list -- this is unnecessary as we've just populated\n - the todo list\n + the todo list in complete_action()\n - commits any staged changes -- this is unnecessary as we're starting a\n new rebase, so there are no staged changes\n - calls record_in_rewritten() -- this is unnecessary as we're starting\n @@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts,\n \t}\n \n -\ttodo_list_release(&new_todo);\n --\n ++\tres = -1;\n + \n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n - \t\treturn -1;\n +-\t\treturn -1;\n ++\t\tgoto cleanup;\n \n \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n - \t\treturn -1;\n +-\t\treturn -1;\n ++\t\tgoto cleanup;\n \n -\treturn sequencer_continue(r, opts);\n +\ttodo_list_write_total_nr(&new_todo);\n +\tres = pick_commits(r, &new_todo, opts);\n ++\n ++cleanup:\n +\ttodo_list_release(&new_todo);\n +\n +\treturn res;\n-- \n2.24.0\n\n"},{"id":"386905","messageId":"20191123143705.17280-6-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191123143705.17280-1-alban.gruin@gmail.com","subject":"[PATCH v3 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-23T14:37:05Z","receivedAt":"2019-11-23T14:47:33Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Currently, complete_action(), used by builtin/rebase.c to start a new\nrebase, calls sequencer_continue() to do it. Before the former calls\npick_commits(), it\n\n - calls read_and_refresh_cache() -- this is unnecessary here as we've\n just called require_clean_work_tree() in complete_action()\n - calls read_populate_opts() -- this is unnecessary as we're starting a\n new rebase, so `opts' is fully populated\n - loads the todo list -- this is unnecessary as we've just populated\n the todo list in complete_action()\n - commits any staged changes -- this is unnecessary as we're starting a\n new rebase, so there are no staged changes\n - calls record_in_rewritten() -- this is unnecessary as we're starting\n a new rebase.\n\nThis changes complete_action() to directly call pick_commits() to avoid\nthese unnecessary steps.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ec7ea8d9e5..ec0b793fc5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5140,15 +5140,21 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \t}\n \n-\ttodo_list_release(&new_todo);\n+\tres = -1;\n \n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n-\t\treturn -1;\n+\t\tgoto cleanup;\n \n \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n-\t\treturn -1;\n+\t\tgoto cleanup;\n \n-\treturn sequencer_continue(r, opts);\n+\ttodo_list_write_total_nr(&new_todo);\n+\tres = pick_commits(r, &new_todo, opts);\n+\n+cleanup:\n+\ttodo_list_release(&new_todo);\n+\n+\treturn res;\n }\n \n struct subject2item_entry {\n-- \n2.24.0\n\n"},{"id":"386906","messageId":"20191123143705.17280-4-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191123143705.17280-1-alban.gruin@gmail.com","subject":"[PATCH v3 3/5] sequencer: move the code writing total_nr on the disk to a new function","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-23T14:37:03Z","receivedAt":"2019-11-23T14:47:36Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"The total amount of commands can be used to show the progression of the\nrebasing in a shell. It is written to the disk by read_populate_todo()\nwhen the todo list is loaded from sequencer_continue() or\npick_commits(), but not by complete_action().\n\nThis moves the part writing total_nr to a new function so it can be\ncalled from complete_action().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 42313f8de6..ec7ea8d9e5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n \tsequencer_remove_state(&opts);\n }\n \n+static void todo_list_write_total_nr(struct todo_list *todo_list)\n+{\n+\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n+\n+\tif (f) {\n+\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n+\t\tfclose(f);\n+\t}\n+}\n+\n static int read_populate_todo(struct repository *r,\n \t\t\t struct todo_list *todo_list,\n \t\t\t struct replay_opts *opts)\n@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,\n \n \tif (is_rebase_i(opts)) {\n \t\tstruct todo_list done = TODO_LIST_INIT;\n-\t\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n \t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,\n \t\t\t+ count_commands(todo_list);\n \t\ttodo_list_release(&done);\n \n-\t\tif (f) {\n-\t\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n-\t\t\tfclose(f);\n-\t\t}\n+\t\ttodo_list_write_total_nr(todo_list);\n \t}\n \n \treturn 0;\n-- \n2.24.0\n\n"},{"id":"386907","messageId":"20191123143705.17280-3-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191123143705.17280-1-alban.gruin@gmail.com","subject":"[PATCH v3 2/5] sequencer: update `done_nr' when skipping commands in a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-23T14:37:02Z","receivedAt":"2019-11-23T14:49:02Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"In a todo list, `done_nr' is the amount of commands that were executed\nor skipped, but skip_unnecessary_picks() did not update it.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like). As in the previous commit, this inconsistent behaviour is\nnot a problem yet, but it would start to matter at the end of this\nseries the same reason.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 575b852a5a..42313f8de6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,\n \t\tMOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);\n \t\ttodo_list->nr -= i;\n \t\ttodo_list->current = 0;\n+\t\ttodo_list->done_nr += i;\n \n \t\tif (is_fixup(peek_command(todo_list, 0)))\n \t\t\trecord_in_rewritten(base_oid, peek_command(todo_list, 0));\n-- \n2.24.0\n\n"},{"id":"386943","messageId":"20191124174332.30887-1-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191123143705.17280-1-alban.gruin@gmail.com","subject":"[PATCH v4 0/5] Use complete_action's todo list to do the rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-24T17:43:27Z","receivedAt":"2019-11-24T17:45:40Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This can be seen as a continuation of ag/reduce-rewriting-todo.\n\nCurrently, complete_action() releases its todo list before calling\nsequencer_continue(), which reloads the todo list from the disk. This\nseries removes this useless round trip.\n\nPatches 1, 2, and 3 originally come from a series meaning to improve\nrebase.missingCommitsCheck[0]. In the original series, I wanted to\ncheck for missing commits in read_populate_todo(), so a warning could be\nissued after a `rebase --continue' or an `exec' commands. But, in the\ncase of the initial edit, it is already checked in complete_action(),\nand would be checked a second time in sequencer_continue() (a caller of\nread_populate_todo()). So I hacked up sequencer_continue() to accept a\npointer to a todo list, and if not null, would skip the call to\nread_populate_todo(). (This was really ugly, to be honest.) Some\nissues arose with git-prompt.sh[1], hence 1, 2 and 3.\n\nPatch 5 is a new approach to what I did first. Instead of bolting a new\nparameter to sequencer_continue(), this makes complete_action() calling\ndirectly pick_commits().\n\nThis is based on 4c86140027 (\"Third batch\").\n\nChanges since v3:\n\n - s/amount/number/ on patches 2 and 3, according to a comment from\n Junio[2] that I had forgotten before I sent the v3 X-(\n\nThe tip of this series is tagged as reduce-todo-list-cont-v4 at\nhttps://github.com/agrn/git.\n\n[0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@gmail.com/\n[1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/\n[2] http://public-inbox.org/git/xmqqmuecnefe.fsf@gitster-ct.c.googlers.com/\n\nAlban Gruin (5):\n sequencer: update `total_nr' when adding an item to a todo list\n sequencer: update `done_nr' when skipping commands in a todo list\n sequencer: move the code writing total_nr on the disk to a new\n function\n rebase: fill `squash_onto' in get_replay_opts()\n sequencer: directly call pick_commits() from complete_action()\n\n builtin/rebase.c | 5 +++++\n sequencer.c | 32 +++++++++++++++++++++++---------\n 2 files changed, 28 insertions(+), 9 deletions(-)\n\nDiff-intervalle contre v3 :\n1: 11a221e82e = 1: 11a221e82e sequencer: update `total_nr' when adding an item to a todo list\n2: 76a3af70b6 ! 2: 6b402a3070 sequencer: update `done_nr' when skipping commands in a todo list\n @@ Metadata\n ## Commit message ##\n sequencer: update `done_nr' when skipping commands in a todo list\n \n - In a todo list, `done_nr' is the amount of commands that were executed\n + In a todo list, `done_nr' is the number of commands that were executed\n or skipped, but skip_unnecessary_picks() did not update it.\n \n This variable is mostly used by command prompts (ie. git-prompt.sh and\n3: 9c5bd30465 ! 3: 0171db4fba sequencer: move the code writing total_nr on the disk to a new function\n @@ Metadata\n ## Commit message ##\n sequencer: move the code writing total_nr on the disk to a new function\n \n - The total amount of commands can be used to show the progression of the\n + The total number of commands can be used to show the progression of the\n rebasing in a shell. It is written to the disk by read_populate_todo()\n when the todo list is loaded from sequencer_continue() or\n pick_commits(), but not by complete_action().\n4: bc3d69a10e = 4: 88f6335c37 rebase: fill `squash_onto' in get_replay_opts()\n5: e7691db66b = 5: 53586b1bed sequencer: directly call pick_commits() from complete_action()\n-- \n2.24.0\n\n"},{"id":"386944","messageId":"20191124174332.30887-2-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191124174332.30887-1-alban.gruin@gmail.com","subject":"[PATCH v4 1/5] sequencer: update `total_nr' when adding an item to a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-24T17:43:28Z","receivedAt":"2019-11-24T17:45:41Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"`total_nr' is the total number of items, counting both done and todo,\nthat are in a todo list. But unlike `nr', it was not updated when an\nitem was appended to the list.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like). By forgetting to update it, the original code made it not\nreflect the reality, but this flaw was masked by the code calling\nunnecessarily read_populate_todo() again to update the variable to its\ncorrect value. At the end of this series, the unnecessary call will be\nremoved, and the inconsistency addressed by this patch would start to\nmatter.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d648aaf416..575b852a5a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2070,6 +2070,7 @@ 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-- \n2.24.0\n\n"},{"id":"386945","messageId":"20191124174332.30887-3-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191124174332.30887-1-alban.gruin@gmail.com","subject":"[PATCH v4 2/5] sequencer: update `done_nr' when skipping commands in a todo list","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-24T17:43:29Z","receivedAt":"2019-11-24T17:45:41Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"In a todo list, `done_nr' is the number of commands that were executed\nor skipped, but skip_unnecessary_picks() did not update it.\n\nThis variable is mostly used by command prompts (ie. git-prompt.sh and\nthe like). As in the previous commit, this inconsistent behaviour is\nnot a problem yet, but it would start to matter at the end of this\nseries the same reason.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 575b852a5a..42313f8de6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,\n \t\tMOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);\n \t\ttodo_list->nr -= i;\n \t\ttodo_list->current = 0;\n+\t\ttodo_list->done_nr += i;\n \n \t\tif (is_fixup(peek_command(todo_list, 0)))\n \t\t\trecord_in_rewritten(base_oid, peek_command(todo_list, 0));\n-- \n2.24.0\n\n"},{"id":"386946","messageId":"20191124174332.30887-5-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191124174332.30887-1-alban.gruin@gmail.com","subject":"[PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-24T17:43:31Z","receivedAt":"2019-11-24T17:45:44Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"When sequencer_continue() is called by complete_action(), `opts' has\nbeen filled by get_replay_opts(). Currently, it does not initialise the\n`squash_onto' field (used by the `--root' mode), only\nread_populate_opts() does. It’s not a problem yet since\nsequencer_continue() calls it before pick_commits(), but it would lead\nto incorrect results once complete_action() is modified to call\npick_commits() directly.\n\nLet’s change that.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e8319d5946..2097d41edc 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \tif (opts->strategy_opts)\n \t\tparse_strategy_opts(&replay, opts->strategy_opts);\n \n+\tif (opts->squash_onto) {\n+\t\toidcpy(&replay.squash_onto, opts->squash_onto);\n+\t\treplay.have_squash_onto = 1;\n+\t}\n+\n \treturn replay;\n }\n \n-- \n2.24.0\n\n"},{"id":"386947","messageId":"20191124174332.30887-4-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191124174332.30887-1-alban.gruin@gmail.com","subject":"[PATCH v4 3/5] sequencer: move the code writing total_nr on the disk to a new function","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-24T17:43:30Z","receivedAt":"2019-11-24T17:45:47Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"The total number of commands can be used to show the progression of the\nrebasing in a shell. It is written to the disk by read_populate_todo()\nwhen the todo list is loaded from sequencer_continue() or\npick_commits(), but not by complete_action().\n\nThis moves the part writing total_nr to a new function so it can be\ncalled from complete_action().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 42313f8de6..ec7ea8d9e5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n \tsequencer_remove_state(&opts);\n }\n \n+static void todo_list_write_total_nr(struct todo_list *todo_list)\n+{\n+\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n+\n+\tif (f) {\n+\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n+\t\tfclose(f);\n+\t}\n+}\n+\n static int read_populate_todo(struct repository *r,\n \t\t\t struct todo_list *todo_list,\n \t\t\t struct replay_opts *opts)\n@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,\n \n \tif (is_rebase_i(opts)) {\n \t\tstruct todo_list done = TODO_LIST_INIT;\n-\t\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n \t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,\n \t\t\t+ count_commands(todo_list);\n \t\ttodo_list_release(&done);\n \n-\t\tif (f) {\n-\t\t\tfprintf(f, \"%d\\n\", todo_list->total_nr);\n-\t\t\tfclose(f);\n-\t\t}\n+\t\ttodo_list_write_total_nr(todo_list);\n \t}\n \n \treturn 0;\n-- \n2.24.0\n\n"},{"id":"386948","messageId":"20191124174332.30887-6-alban.gruin@gmail.com","threadId":"51918","inReplyTo":"20191124174332.30887-1-alban.gruin@gmail.com","subject":"[PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-24T17:43:32Z","receivedAt":"2019-11-24T17:45:47Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Currently, complete_action(), used by builtin/rebase.c to start a new\nrebase, calls sequencer_continue() to do it. Before the former calls\npick_commits(), it\n\n - calls read_and_refresh_cache() -- this is unnecessary here as we've\n just called require_clean_work_tree() in complete_action()\n - calls read_populate_opts() -- this is unnecessary as we're starting a\n new rebase, so `opts' is fully populated\n - loads the todo list -- this is unnecessary as we've just populated\n the todo list in complete_action()\n - commits any staged changes -- this is unnecessary as we're starting a\n new rebase, so there are no staged changes\n - calls record_in_rewritten() -- this is unnecessary as we're starting\n a new rebase.\n\nThis changes complete_action() to directly call pick_commits() to avoid\nthese unnecessary steps.\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ec7ea8d9e5..ec0b793fc5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5140,15 +5140,21 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \t}\n \n-\ttodo_list_release(&new_todo);\n+\tres = -1;\n \n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n-\t\treturn -1;\n+\t\tgoto cleanup;\n \n \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n-\t\treturn -1;\n+\t\tgoto cleanup;\n \n-\treturn sequencer_continue(r, opts);\n+\ttodo_list_write_total_nr(&new_todo);\n+\tres = pick_commits(r, &new_todo, opts);\n+\n+cleanup:\n+\ttodo_list_release(&new_todo);\n+\n+\treturn res;\n }\n \n struct subject2item_entry {\n-- \n2.24.0\n\n"},{"id":"387098","messageId":"20191126182734.142503-1-jonathantanmy@google.com","threadId":"51918","inReplyTo":"20191124174332.30887-5-alban.gruin@gmail.com","subject":"Re: [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-26T18:27:34Z","receivedAt":"2019-11-26T18:27:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for the better commit message! Just one note:\n\n> When sequencer_continue() is called by complete_action(), `opts' has\n> been filled by get_replay_opts().\n\nI searched for \"get_replay_opts\" in complete_action() but couldn't find\nit, and had to do some searching to realize that what you mean is that\nwhenever complete_action() is called by do_interactive_rebase() in\nbuiltin/rebase.c (its only caller), the \"opts\" argument was filled by\nget_replay_opts(). Maybe reword as:\n\n When do_interactive_rebase() in builtin/rebase.c calls\n complete_action(), it passes an \"opts\" argument generated by\n get_replay_opts(). complete_action() then passes it to\n sequencer_continue().\n\n> Currently, it does not initialise the\n> `squash_onto' field (used by the `--root' mode), only\n> read_populate_opts() does. It’s not a problem yet since\n> sequencer_continue() calls it before pick_commits(), but it would lead\n> to incorrect results once complete_action() is modified to call\n> pick_commits() directly.\n> \n> Let’s change that.\n\nThe rest is clear; thanks. I would like to make it explicit that\npick_commits() uses \"squash_onto\", and make the antecedents of all the\n\"it\"s clear, so I would write it like this:\n\n Currently, get_replay_opts() does not initialize the \"squash_onto\"\n field (used by the \"--root\" mode); only read_populate_opts() does.\n This field is used by pick_commits(), called by sequencer_continue().\n It's not a problem yet since sequencer_continue() currently calls\n read_populate_opts() before pick_commits(), but it would lead to\n incorrect results once complete_action() is modified to call\n pick_commits() directly (bypassing sequencer_continue() and, hence,\n read_populate_opts()).\n\n Let's change that.\n\nAlso, I think it's better to use the regular quote ' instead of the\nsmart quote ’.\n"},{"id":"387099","messageId":"20191126184103.146421-1-jonathantanmy@google.com","threadId":"51918","inReplyTo":"20191124174332.30887-6-alban.gruin@gmail.com","subject":"Re: [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-26T18:41:03Z","receivedAt":"2019-11-26T18:41:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Currently, complete_action(), used by builtin/rebase.c to start a new\n> rebase, calls sequencer_continue() to do it. Before the former calls\n> pick_commits(), it\n> \n> - calls read_and_refresh_cache() -- this is unnecessary here as we've\n> just called require_clean_work_tree() in complete_action()\n\nrequire_clean_work_tree() and read_and_refresh_cache() seem to be\ndifferent functions - can you explain why running the former is a good\nsubstitute for running the latter?\n\n> - calls read_populate_opts() -- this is unnecessary as we're starting a\n> new rebase, so `opts' is fully populated\n\nMy comment from [1] has not been addressed. Quoting it here:\n\n> So complete_action() (the function modified in this commit) is called\n> only by do_interactive_rebase() (in builtin/rebase.c), which is only\n> called by run_rebase_interactive() (in builtin/rebase.c) when command is\n> ACTION_NONE, so indeed, we're starting a new rebase. But where the\n> options fully populated? I see that in do_interactive_rebase(), it is\n> initialized with get_replay_opts(), but that seems different from\n> read_populate_opts().\n\n[1] https://lore.kernel.org/git/20191119204146.168001-1-jonathantanmy@google.com/\n\n> - loads the todo list -- this is unnecessary as we've just populated\n> the todo list in complete_action()\n\nBoth functions indeed have their own todo lists that they pass to\npick_commits(), but I don't see (at least, by glancing at the code) that\nboth these todo lists are identical.\n\n> - commits any staged changes -- this is unnecessary as we're starting a\n> new rebase, so there are no staged changes\n> - calls record_in_rewritten() -- this is unnecessary as we're starting\n> a new rebase.\n\nOK - I don't know enough about the rebase mechanism to verify these, but\nthese seem reasonable to me.\n"},{"id":"387239","messageId":"aa162f9c-9ae8-9844-6829-7fc0d9c50f9c@gmail.com","threadId":"51918","inReplyTo":"20191126184103.146421-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-27T19:56:03Z","receivedAt":"2019-11-27T19:56:16Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Jonathan,\n\nLe 26/11/2019 à 19:41, Jonathan Tan a écrit :\n>> Currently, complete_action(), used by builtin/rebase.c to start a new\n>> rebase, calls sequencer_continue() to do it. Before the former calls\n>> pick_commits(), it\n>>\n>> - calls read_and_refresh_cache() -- this is unnecessary here as we've\n>> just called require_clean_work_tree() in complete_action()\n> \n> require_clean_work_tree() and read_and_refresh_cache() seem to be\n> different functions - can you explain why running the former is a good\n> substitute for running the latter?\n> \n\nThey both refresh the index.\n\nrequire_clean_work_tree(), called when starting a new rebase, will also\ncheck if there are any unstaged or uncommitted changes, in which case we\ndo not want to start a rebase.\n\nThis is not what we want when resuming a rebase (with `rebase\n--continue'), because the changes might be the result of a conflict\nresolution. In this case, the last commit is amended, and the rebase is\nresumed.\n\n>> - calls read_populate_opts() -- this is unnecessary as we're starting a\n>> new rebase, so `opts' is fully populated\n> \n> My comment from [1] has not been addressed. Quoting it here:\n> \n>> So complete_action() (the function modified in this commit) is called\n>> only by do_interactive_rebase() (in builtin/rebase.c), which is only\n>> called by run_rebase_interactive() (in builtin/rebase.c) when command is\n>> ACTION_NONE, so indeed, we're starting a new rebase. But where the\n>> options fully populated? I see that in do_interactive_rebase(), it is\n>> initialized with get_replay_opts(), but that seems different from\n>> read_populate_opts().\n> \n> [1] https://lore.kernel.org/git/20191119204146.168001-1-jonathantanmy@google.com/\n> \n\nSorry.\n\nFor the first part of your comment, I added a comment at the beginning\nof the message, although I did _not_ include an analysis on when\ncomplete_action() is used.\n\nget_replay_opts() converts a `struct rebase_options' (which contains the\narguments passed to `git rebase') into a `struct replay_opts' which can\nbe used by the sequencer, whereas read_populate_opts() loads the options\nfrom the disk.\n\nSo, when are they written to the disk? In do_interactive_rebase()\n(builtin/rebase.c), after using get_replay_opts() to convert `opts' to\n`replay', init_basic_state() is called, which calls write_basic_state(),\nwhich write the options to the disk.\n\nThen, until complete_action() is called, `opts' is not modified.\n\n>> - loads the todo list -- this is unnecessary as we've just populated\n>> the todo list in complete_action()\n> \n> Both functions indeed have their own todo lists that they pass to\n> pick_commits(), but I don't see (at least, by glancing at the code) that\n> both these todo lists are identical.\n> \n\nNear the end of complete_action(), the todo list is written to the disk.\n The destination is obtained with rebase_path_todo().\n\nread_populate_todo() will read a file and parse it. In the case of\n`rebase -i', the location is obtained with rebase_path_todo(), and only\n`total_nr' will be modified to contain the number of commands done and todo.\n\nIn the case of a new rebase, the done list might not be empty after\ntajjimh skip_unnecessary_picks() from complete_action(). Skipped\ncommands are moved from the todo list to the done list. As `total_nr'\nis not changed by skip_unnecessary_picks(), it is also equal to the\nnumber of commands remaining in the todo list and in the done list. So,\nwhen read_populate_todo() reads the list and the done list from the\ndisk, as they should not have been modified, `total_nr' should remain\nthe same, too.\n\nThe only thing that can change is the internal buffer (`buf'), because\nskip_unnecessary_picks() don’t change it. Since\nag/sequencer-reduce-rewriting-todo, it is no longer a textual\nrepresentation of the todo list. Each command contains a pointer to a\nlocation in the buffer and a length to represent its argument.\n\n>> - commits any staged changes -- this is unnecessary as we're starting a\n>> new rebase, so there are no staged changes\n>> - calls record_in_rewritten() -- this is unnecessary as we're starting\n>> a new rebase.\n> \n> OK - I don't know enough about the rebase mechanism to verify these, but\n> these seem reasonable to me.\n> \n\nCheers,\nAlban\n\n"},{"id":"387243","messageId":"2ee284b6-461e-dbba-2583-cffc38e95df2@gmail.com","threadId":"51918","inReplyTo":"aa162f9c-9ae8-9844-6829-7fc0d9c50f9c@gmail.com","subject":"Re: [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-11-27T20:09:12Z","receivedAt":"2019-11-27T20:09:17Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Le 27/11/2019 à 20:56, Alban Gruin a écrit :\n> Near the end of complete_action(), the todo list is written to the disk.\n> The destination is obtained with rebase_path_todo().\n> \n> read_populate_todo() will read a file and parse it. In the case of\n> `rebase -i', the location is obtained with rebase_path_todo(), and only\n> `total_nr' will be modified to contain the number of commands done and todo.\n> \n> In the case of a new rebase, the done list might not be empty after\n> tajjimh skip_unnecessary_picks() from complete_action().\n\ns/tajjimh/calling/\n\n"}]}