{"thread":{"id":"56464","subject":"[PATCH 0/5] rebase -i: a couple of small improvements","startedAt":"2021-09-08T13:41:17Z","lastAt":"2021-09-28T10:21:00Z","messageCount":31,"participants":["Phillip Wood via GitGitGadget","Eric Sunshine","Philippe Blain","Phillip Wood","Johannes Schindelin","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"435083","messageId":"pull.1034.git.1631108472.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":null,"subject":"[PATCH 0/5] rebase -i: a couple of small improvements","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T13:41:07Z","receivedAt":"2021-09-08T13:41:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Fix the re-reading of the todo list after an exec or reword command and stop\nforking \"git checkout\" when checking out \"onto\"\n\nPhillip Wood (5):\n sequencer.c: factor out a function\n rebase: fix todo-list rereading\n reset_head(): mark oid parameter as const\n rebase -i: don't fork git checkout\n rebase: remove unused parameter\n\n builtin/rebase.c | 3 +-\n reset.c | 4 +--\n reset.h | 4 +--\n sequencer.c | 88 ++++++++++++++++++++----------------------------\n sequencer.h | 6 ++--\n 5 files changed, 43 insertions(+), 62 deletions(-)\n\n\nbase-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1034%2Fphillipwood%2Fwip%2Frebase-reread-todo-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1034/phillipwood/wip/rebase-reread-todo-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1034\n-- \ngitgitgadget\n"},{"id":"435084","messageId":"53cde4825b408e5fb893bbd9a222e7387d69a408.1631108472.git.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.git.1631108472.gitgitgadget@gmail.com","subject":"[PATCH 1/5] sequencer.c: factor out a function","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T13:41:08Z","receivedAt":"2021-09-08T13:41:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis code is heavily indented and obscures the high level logic within\nthe loop. Lets move it to its own function before modifying it in the\nnext commit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7f07cd00f3f..a248c886c27 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4254,6 +4254,27 @@ static int stopped_at_head(struct repository *r)\n \n }\n \n+static int reread_todo_if_changed(struct repository *r,\n+\t\t\t\t struct todo_list *todo_list,\n+\t\t\t\t struct replay_opts *opts)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(get_todo_path(opts), &st)) {\n+\t\treturn error_errno(_(\"could not stat '%s'\"),\n+\t\t\t\t get_todo_path(opts));\n+\t} else if (match_stat_data(&todo_list->stat, &st)) {\n+\t\t/* Reread the todo file if it has changed. */\n+\t\ttodo_list_release(todo_list);\n+\t\tif (read_populate_todo(r, todo_list, opts))\n+\t\t\treturn -1; /* message was printed */\n+\t\t/* `current` will be incremented on return */\n+\t\ttodo_list->current = -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\n@@ -4433,20 +4454,9 @@ static int pick_commits(struct repository *r,\n \t\t\t\t\t\t\titem->commit,\n \t\t\t\t\t\t\targ, item->arg_len,\n \t\t\t\t\t\t\topts, res, 0);\n-\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n-\t\t\tstruct stat st;\n-\n-\t\t\tif (stat(get_todo_path(opts), &st)) {\n-\t\t\t\tres = error_errno(_(\"could not stat '%s'\"),\n-\t\t\t\t\t\t get_todo_path(opts));\n-\t\t\t} else if (match_stat_data(&todo_list->stat, &st)) {\n-\t\t\t\t/* Reread the todo file if it has changed. */\n-\t\t\t\ttodo_list_release(todo_list);\n-\t\t\t\tif (read_populate_todo(r, todo_list, opts))\n-\t\t\t\t\tres = -1; /* message was printed */\n-\t\t\t\t/* `current` will be incremented below */\n-\t\t\t\ttodo_list->current = -1;\n-\t\t\t}\n+\t\t} else if (is_rebase_i(opts) && check_todo && !res &&\n+\t\t\t reread_todo_if_changed(r, todo_list, opts)) {\n+\t\t\treturn -1;\n \t\t}\n \n \t\ttodo_list->current++;\n-- \ngitgitgadget\n\n"},{"id":"435086","messageId":"3b17a4e3d3fa620638299294a56adb9237fd5c56.1631108472.git.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.git.1631108472.gitgitgadget@gmail.com","subject":"[PATCH 2/5] rebase: fix todo-list rereading","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T13:41:09Z","receivedAt":"2021-09-08T13:41:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n54fd3243da (\"rebase -i: reread the todo list if `exec` touched it\",\n2017-04-26) sought to reread the todo list after running an exec\ncommand only if it had been changed. To accomplish this it checks the\nstat data of the todo list after running an exec command to see if it\nhas changed. Unfortunately there are two problems, firstly the\nimplementation is buggy we actually reread the list after each exec\nwhich is quadratic in the number of commit lookups and secondly the\ndesign is predicated on using nanosecond time stamps which are not the\ndefault.\n\nThe implementation bug stems from the fact that we write a new todo\nlist to disk before running each command but do not update the stat\ndata to reflect this[1].\n\nThe design problem is that it is possible for the user to edit the\ntodo list without changing its size or inode which means we have to\nrely on the mtime to tell us if it has changed. Unfortunately unless\ngit is built with USE_NSEC it is possible for the original and edited\nlist to share the same mtime.\n\nIdeally \"git rebase --edit-todo\" would set a flag that we would then\ncheck in sequencer.c. Unfortunately this is approach will not work as\nthere are scripts in the wild that write to the todo list directly\nwithout running \"git rebase --edit-todo\". Instead of relying on stat\ndata this patch simply reads the possibly edited todo list and\ncompares it to the original with memcmp(). This is much faster than\nreparsing the todo list each time. This patch reduces the time to run\n\n git rebase -r -xtrue v2.32.0~100 v2.32.0\n\nwhich runs 419 exec commands by 6.6%. For comparison fixing the\nimplementation bug in stat based approach reduces the time by a\nfurther 1.4% and is indistinguishable from never rereading the todo\nlist.\n\n[1] https://lore.kernel.org/git/20191125131833.GD23183@szeder.dev/\n\nReported-by: SZEDER Gábor \nSigned-off-by: Phillip Wood \n---\n sequencer.c | 19 ++++++++-----------\n sequencer.h | 1 -\n 2 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a248c886c27..d51440ddcd9 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2671,7 +2671,6 @@ 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 {\n-\tstruct stat st;\n \tconst char *todo_file = get_todo_path(opts);\n \tint res;\n \n@@ -2679,11 +2678,6 @@ static int read_populate_todo(struct repository *r,\n \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n \t\treturn -1;\n \n-\tres = stat(todo_file, &st);\n-\tif (res)\n-\t\treturn error(_(\"could not stat '%s'\"), todo_file);\n-\tfill_stat_data(&todo_list->stat, &st);\n-\n \tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n \tif (res) {\n \t\tif (is_rebase_i(opts))\n@@ -4258,12 +4252,14 @@ static int reread_todo_if_changed(struct repository *r,\n \t\t\t\t struct todo_list *todo_list,\n \t\t\t\t struct replay_opts *opts)\n {\n-\tstruct stat st;\n+\tint offset;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (stat(get_todo_path(opts), &st)) {\n-\t\treturn error_errno(_(\"could not stat '%s'\"),\n-\t\t\t\t get_todo_path(opts));\n-\t} else if (match_stat_data(&todo_list->stat, &st)) {\n+\tif (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)\n+\t\treturn -1;\n+\toffset = get_item_line_offset(todo_list, todo_list->current + 1);\n+\tif (buf.len != todo_list->buf.len - offset ||\n+\t memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {\n \t\t/* Reread the todo file if it has changed. */\n \t\ttodo_list_release(todo_list);\n \t\tif (read_populate_todo(r, todo_list, opts))\n@@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,\n \t\t/* `current` will be incremented on return */\n \t\ttodo_list->current = -1;\n \t}\n+\tstrbuf_release(&buf);\n \n \treturn 0;\n }\ndiff --git a/sequencer.h b/sequencer.h\nindex d57d8ea23d7..cdeb0c6be47 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -116,7 +116,6 @@ struct todo_list {\n \tstruct todo_item *items;\n \tint nr, alloc, current;\n \tint done_nr, total_nr;\n-\tstruct stat_data stat;\n };\n \n #define TODO_LIST_INIT { STRBUF_INIT }\n-- \ngitgitgadget\n\n"},{"id":"435085","messageId":"614555fc10f336cc6f7681501247209aac879269.1631108472.git.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.git.1631108472.gitgitgadget@gmail.com","subject":"[PATCH 3/5] reset_head(): mark oid parameter as const","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T13:41:10Z","receivedAt":"2021-09-08T13:41:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWe do not write to oid so mark it as const in preparation for the next\ncommit.\n\nSigned-off-by: Phillip Wood \n---\n reset.c | 4 ++--\n reset.h | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reset.c b/reset.c\nindex 4bea758053b..5b578d6bb68 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -8,8 +8,8 @@\n #include \"tree.h\"\n #include \"unpack-trees.h\"\n \n-int reset_head(struct repository *r, struct object_id *oid, const char *action,\n-\t const char *switch_to_branch, unsigned flags,\n+int reset_head(struct repository *r, const struct object_id *oid,\n+\t const char *action, const char *switch_to_branch, unsigned flags,\n \t const char *reflog_orig_head, const char *reflog_head,\n \t const char *default_reflog_action)\n {\ndiff --git a/reset.h b/reset.h\nindex 12f83c78e28..6d2be511b8c 100644\n--- a/reset.h\n+++ b/reset.h\n@@ -12,8 +12,8 @@\n #define RESET_HEAD_REFS_ONLY (1<<3)\n #define RESET_ORIG_HEAD (1<<4)\n \n-int reset_head(struct repository *r, struct object_id *oid, const char *action,\n-\t const char *switch_to_branch, unsigned flags,\n+int reset_head(struct repository *r, const struct object_id *oid,\n+\t const char *action, const char *switch_to_branch, unsigned flags,\n \t const char *reflog_orig_head, const char *reflog_head,\n \t const char *default_reflog_action);\n \n-- \ngitgitgadget\n\n"},{"id":"435087","messageId":"39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.git.1631108472.gitgitgadget@gmail.com","subject":"[PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T13:41:11Z","receivedAt":"2021-09-08T13:41:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe \"apply\" based rebase has avoided forking git checkout since\nac7f467fef (\"builtin/rebase: support running \"git rebase \"\",\n2018-08-07). The code that handles the checkout was moved into libgit\nby b309a97108 (\"reset: extract reset_head() from rebase\", 2020-04-07)\nso lets start using it for the \"merge\" based rebase as well. This\nopens the way for us to stop calling the post-checkout hook in the\nfuture.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 29 ++++-------------------------\n 1 file changed, 4 insertions(+), 25 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d51440ddcd9..1a9dbc70d3c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4192,42 +4192,21 @@ int apply_autostash_oid(const char *stash_oid)\n \treturn apply_save_autostash_oid(stash_oid, 1);\n }\n \n-static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n-\t\t\t const char *commit, const char *action)\n-{\n-\tstruct child_process cmd = CHILD_PROCESS_INIT;\n-\tint ret;\n-\n-\tcmd.git_cmd = 1;\n-\n-\tstrvec_push(&cmd.args, \"checkout\");\n-\tstrvec_push(&cmd.args, commit);\n-\tstrvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n-\n-\tif (opts->verbose)\n-\t\tret = run_command(&cmd);\n-\telse\n-\t\tret = run_command_silent_on_success(&cmd);\n-\n-\tif (!ret)\n-\t\tdiscard_index(r->index);\n-\n-\treturn ret;\n-}\n-\n static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t const char *onto_name, const struct object_id *onto,\n \t\t\t const struct object_id *orig_head)\n {\n \tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n \n-\tif (run_git_checkout(r, opts, oid_to_hex(onto), action)) {\n+\tif (reset_head(r, onto, \"checkout\", NULL, RESET_HEAD_DETACH |\n+\t\t RESET_ORIG_HEAD | RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n+\t\t NULL, action, \"rebase\")) {\n \t\tapply_autostash(rebase_path_autostash());\n \t\tsequencer_remove_state(opts);\n \t\treturn error(_(\"could not detach HEAD\"));\n \t}\n \n-\treturn update_ref(NULL, \"ORIG_HEAD\", orig_head, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+\treturn 0;\n }\n \n static int stopped_at_head(struct repository *r)\n-- \ngitgitgadget\n\n"},{"id":"435088","messageId":"c8a92d4242b4dfad71297d1eec9c3ef658641193.1631108472.git.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.git.1631108472.gitgitgadget@gmail.com","subject":"[PATCH 5/5] rebase: remove unused parameter","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T13:41:12Z","receivedAt":"2021-09-08T13:41:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nNow that we use reset_head() we don't need to pass orig_head around.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 3 +--\n sequencer.c | 12 +++++-------\n sequencer.h | 5 ++---\n 3 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 12f093121d9..1520f75a491 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -377,8 +377,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\tsplit_exec_commands(opts->cmd, &commands);\n \t\tret = complete_action(the_repository, &replay, flags,\n \t\t\tshortrevisions, opts->onto_name, opts->onto,\n-\t\t\t&opts->orig_head, &commands, opts->autosquash,\n-\t\t\t&todo_list);\n+\t\t\t&commands, opts->autosquash, &todo_list);\n \t}\n \n \tstring_list_clear(&commands, 0);\ndiff --git a/sequencer.c b/sequencer.c\nindex 1a9dbc70d3c..a7ea60a7a82 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4193,8 +4193,7 @@ int apply_autostash_oid(const char *stash_oid)\n }\n \n static int checkout_onto(struct repository *r, struct replay_opts *opts,\n-\t\t\t const char *onto_name, const struct object_id *onto,\n-\t\t\t const struct object_id *orig_head)\n+\t\t\t const char *onto_name, const struct object_id *onto)\n {\n \tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n \n@@ -5567,9 +5566,8 @@ static int skip_unnecessary_picks(struct repository *r,\n \n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const struct object_id *orig_head,\n-\t\t struct string_list *commands, unsigned autosquash,\n-\t\t struct todo_list *todo_list)\n+\t\t struct commit *onto, struct string_list *commands,\n+\t\t unsigned autosquash, struct todo_list *todo_list)\n {\n \tchar shortonto[GIT_MAX_HEXSZ + 1];\n \tconst char *todo_file = rebase_path_todo();\n@@ -5616,7 +5614,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \n \t\treturn error(_(\"nothing to do\"));\n \t} else if (res == -4) {\n-\t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n+\t\tcheckout_onto(r, opts, onto_name, &onto->object.oid);\n \t\ttodo_list_release(&new_todo);\n \n \t\treturn -1;\n@@ -5644,7 +5642,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \n \tres = -1;\n \n-\tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n+\tif (checkout_onto(r, opts, onto_name, &oid))\n \t\tgoto cleanup;\n \n \tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\ndiff --git a/sequencer.h b/sequencer.h\nindex cdeb0c6be47..352b24014bb 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -163,9 +163,8 @@ void todo_list_add_exec_commands(struct todo_list *todo_list,\n \t\t\t\t struct string_list *commands);\n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const struct object_id *orig_head,\n-\t\t struct string_list *commands, unsigned autosquash,\n-\t\t struct todo_list *todo_list);\n+\t\t struct commit *onto, struct string_list *commands,\n+\t\t unsigned autosquash, struct todo_list *todo_list);\n int todo_list_rearrange_squash(struct todo_list *todo_list);\n \n /*\n-- \ngitgitgadget\n"},{"id":"435127","messageId":"CAPig+cRL-EOZJVEDefdUgP3-DjrBnXVSiMc2EzimYOoECm5bfw@mail.gmail.com","threadId":"56464","inReplyTo":"53cde4825b408e5fb893bbd9a222e7387d69a408.1631108472.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] sequencer.c: factor out a function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-08T17:51:27Z","receivedAt":"2021-09-08T17:51:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 8, 2021 at 9:41 AM Phillip Wood via GitGitGadget\n wrote:\n> This code is heavily indented and obscures the high level logic within\n> the loop. Lets move it to its own function before modifying it in the\n> next commit.\n\ns/Lets/Let's/ ... or just drop \"Let's\" altogether and start with \"Move\".\n"},{"id":"435130","messageId":"f05dc55f-a7e4-b8f7-7b0c-5000bf48f803@gmail.com","threadId":"56464","inReplyTo":"39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-08T18:14:03Z","receivedAt":"2021-09-08T18:14:10Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Phillip,\n\nLe 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :\n> From: Phillip Wood \n> \n> The \"apply\" based rebase has avoided forking git checkout since\n> ac7f467fef (\"builtin/rebase: support running \"git rebase \"\",\n> 2018-08-07). The code that handles the checkout was moved into libgit\n> by b309a97108 (\"reset: extract reset_head() from rebase\", 2020-04-07)\n> so lets start using it for the \"merge\" based rebase as well. This\n> opens the way for us to stop calling the post-checkout hook in the\n> future.\n> \n\nWhile in general I think it's a good thing to avoid forking, this change\nmight result in behavioral differences. Any config that affects\n'git checkout' but not the internal 'reset.c::reset_head' function might\nplay a role in the rebase UX.\n\nOne that immediately came to mind is 'submodule.recurse'. This initial 'onto'\ncheckout was pretty much the only part of 'git rebase' that did something useful\nfor submodules, so it's kind of sad to see it regress. [That is, until someone\ntakes the time to implement 'git rebase --recurse-submodules' and makes sure *all*\ncode paths that touch the working tree pay attention to this flag, and that will\nprobably necessitate 'git merge --recurse-submodules' first because of the 'merge'\nbackend... as far as I'm aware it's on Emily's list [1], it's also on mine but\nI don't know when I'll get the time.]\n\nAnyway, I'm not saying that we should not do what this patch is proposing, but\nI think caveats such as that should be documented in the commit message, and maybe\nan audit of other configs that might results in behavioural differences should be done.\n\nThanks,\n\nPhilippe.\n\n[1] https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/t/#m0229af9183a84c2367f21e82adfbd21f08aa4437\n"},{"id":"435230","messageId":"c42d4051-59cd-094a-4570-32cf4d38ec27@gmail.com","threadId":"56464","inReplyTo":"f05dc55f-a7e4-b8f7-7b0c-5000bf48f803@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-09T10:09:31Z","receivedAt":"2021-09-09T10:09:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Philippe\n\nOn 08/09/2021 19:14, Philippe Blain wrote:\n> Hi Phillip,\n> \n> Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :\n>> From: Phillip Wood \n>>\n>> The \"apply\" based rebase has avoided forking git checkout since\n>> ac7f467fef (\"builtin/rebase: support running \"git rebase \"\",\n>> 2018-08-07). The code that handles the checkout was moved into libgit\n>> by b309a97108 (\"reset: extract reset_head() from rebase\", 2020-04-07)\n>> so lets start using it for the \"merge\" based rebase as well. This\n>> opens the way for us to stop calling the post-checkout hook in the\n>> future.\n>>\n> \n> While in general I think it's a good thing to avoid forking, this change\n> might result in behavioral differences. Any config that affects\n> 'git checkout' but not the internal 'reset.c::reset_head' function might\n> play a role in the rebase UX.\n> \n> One that immediately came to mind is 'submodule.recurse'. This initial \n> 'onto'\n> checkout was pretty much the only part of 'git rebase' that did \n> something useful\n> for submodules, so it's kind of sad to see it regress. \n\nThanks for pointing that out. As a non-submodule user my question would \nbe is it actually useful for the initial checkout to work that way if \nthe rest of rebase (and the checkout for the am backend) ignores \nsubmodules? reset.c::reset_head() just uses unpack trees like checkout \nso if rebase read 'submodule.recurse' then reset_head() would work like \n'git checkout' and also 'git rebase --abort' and the \"reset\" command in \nthe todo list would start checking out submodules. I'm reluctant to do \nthat until the merge backend also handles submodules unless there is a \ngood reason that such partial submodule support would help submodule users.\n\n[That is, until\n> someone\n> takes the time to implement 'git rebase --recurse-submodules' and makes \n> sure *all*\n> code paths that touch the working tree pay attention to this flag, and \n> that will\n> probably necessitate 'git merge --recurse-submodules' first because of \n> the 'merge'\n> backend... as far as I'm aware it's on Emily's list [1], it's also on \n> mine but\n> I don't know when I'll get the time.]\n> \n> Anyway, I'm not saying that we should not do what this patch is \n> proposing, but\n> I think caveats such as that should be documented in the commit message, \n\nYes I'll update the commit message. It should also mention that this \nfixes the bug reported in [1] where a failing post-checkout hook aborts \na rebase.\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/CAHr-Uu_KeAJZrd+GzymNP47iFi+dZkvVYsWQPtzT_FQrVnWTDg@mail.gmail.com/\n\n> and maybe\n> an audit of other configs that might results in behavioural differences \n> should be done.\n> \n> Thanks,\n> \n> Philippe.\n> \n> [1] \n> https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/t/#m0229af9183a84c2367f21e82adfbd21f08aa4437 \n> \n\n"},{"id":"435231","messageId":"e0a82c28-0fe1-b397-541e-6e394179a63e@gmail.com","threadId":"56464","inReplyTo":"CAPig+cRL-EOZJVEDefdUgP3-DjrBnXVSiMc2EzimYOoECm5bfw@mail.gmail.com","subject":"Re: [PATCH 1/5] sequencer.c: factor out a function","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-09T10:10:29Z","receivedAt":"2021-09-09T10:10:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/09/2021 18:51, Eric Sunshine wrote:\n> On Wed, Sep 8, 2021 at 9:41 AM Phillip Wood via GitGitGadget\n> wrote:\n>> This code is heavily indented and obscures the high level logic within\n>> the loop. Lets move it to its own function before modifying it in the\n>> next commit.\n> \n> s/Lets/Let's/ ... or just drop \"Let's\" altogether and start with \"Move\".\n\nThanks, well spotted as always\n\nPhillip\n"},{"id":"435236","messageId":"nycvar.QRO.7.76.6.2109091241270.59@tvgsbejvaqbjf.bet","threadId":"56464","inReplyTo":"53cde4825b408e5fb893bbd9a222e7387d69a408.1631108472.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] sequencer.c: factor out a function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T10:44:56Z","receivedAt":"2021-09-09T10:45:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood \n>\n> This code is heavily indented and obscures the high level logic within\n> the loop. Lets move it to its own function before modifying it in the\n> next commit.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> sequencer.c | 38 ++++++++++++++++++++++++--------------\n> 1 file changed, 24 insertions(+), 14 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 7f07cd00f3f..a248c886c27 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4254,6 +4254,27 @@ static int stopped_at_head(struct repository *r)\n>\n> }\n>\n> +static int reread_todo_if_changed(struct repository *r,\n> +\t\t\t\t struct todo_list *todo_list,\n> +\t\t\t\t struct replay_opts *opts)\n> +{\n> +\tstruct stat st;\n> +\n> +\tif (stat(get_todo_path(opts), &st)) {\n> +\t\treturn error_errno(_(\"could not stat '%s'\"),\n> +\t\t\t\t get_todo_path(opts));\n> +\t} else if (match_stat_data(&todo_list->stat, &st)) {\n> +\t\t/* Reread the todo file if it has changed. */\n> +\t\ttodo_list_release(todo_list);\n> +\t\tif (read_populate_todo(r, todo_list, opts))\n> +\t\t\treturn -1; /* message was printed */\n> +\t\t/* `current` will be incremented on return */\n> +\t\ttodo_list->current = -1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> static const char rescheduled_advice[] =\n> N_(\"Could not execute the todo command\\n\"\n> \"\\n\"\n> @@ -4433,20 +4454,9 @@ static int pick_commits(struct repository *r,\n> \t\t\t\t\t\t\titem->commit,\n> \t\t\t\t\t\t\targ, item->arg_len,\n> \t\t\t\t\t\t\topts, res, 0);\n> -\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n> -\t\t\tstruct stat st;\n> -\n> -\t\t\tif (stat(get_todo_path(opts), &st)) {\n> -\t\t\t\tres = error_errno(_(\"could not stat '%s'\"),\n> -\t\t\t\t\t\t get_todo_path(opts));\n> -\t\t\t} else if (match_stat_data(&todo_list->stat, &st)) {\n> -\t\t\t\t/* Reread the todo file if it has changed. */\n> -\t\t\t\ttodo_list_release(todo_list);\n> -\t\t\t\tif (read_populate_todo(r, todo_list, opts))\n> -\t\t\t\t\tres = -1; /* message was printed */\n> -\t\t\t\t/* `current` will be incremented below */\n> -\t\t\t\ttodo_list->current = -1;\n> -\t\t\t}\n> +\t\t} else if (is_rebase_i(opts) && check_todo && !res &&\n> +\t\t\t reread_todo_if_changed(r, todo_list, opts)) {\n> +\t\t\treturn -1;\n\nIt definitely looks like a good refactoring, but it does subtly change the\nbehavior. If the `todo` file could not be found, we previously continued\nafter printing out an error, but now we return with an error.\n\nLikewise, if the `todo` file existed but had a parse error, we would\nprint that error _but continue_. This is definitely a bug, if you ask me.\n\nNow, this might be a desirable change in the first place: if an `exec`\ncommand removed the `todo` file, changes are that the user wanted to\nabort, right? And the bug needs to be fixed, too.\n\nIt does need to be called out in the commit message, though.\n\nThank you,\nDscho\n\n> \t\t}\n>\n> \t\ttodo_list->current++;\n> --\n> gitgitgadget\n>\n>\n"},{"id":"435237","messageId":"nycvar.QRO.7.76.6.2109091247370.59@tvgsbejvaqbjf.bet","threadId":"56464","inReplyTo":"3b17a4e3d3fa620638299294a56adb9237fd5c56.1631108472.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] rebase: fix todo-list rereading","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T10:48:03Z","receivedAt":"2021-09-09T10:48:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood \n>\n> 54fd3243da (\"rebase -i: reread the todo list if `exec` touched it\",\n> 2017-04-26) sought to reread the todo list after running an exec\n> command only if it had been changed. To accomplish this it checks the\n> stat data of the todo list after running an exec command to see if it\n> has changed. Unfortunately there are two problems, firstly the\n> implementation is buggy we actually reread the list after each exec\n> which is quadratic in the number of commit lookups and secondly the\n> design is predicated on using nanosecond time stamps which are not the\n> default.\n>\n> The implementation bug stems from the fact that we write a new todo\n> list to disk before running each command but do not update the stat\n> data to reflect this[1].\n>\n> The design problem is that it is possible for the user to edit the\n> todo list without changing its size or inode which means we have to\n> rely on the mtime to tell us if it has changed. Unfortunately unless\n> git is built with USE_NSEC it is possible for the original and edited\n> list to share the same mtime.\n>\n> Ideally \"git rebase --edit-todo\" would set a flag that we would then\n> check in sequencer.c. Unfortunately this is approach will not work as\n> there are scripts in the wild that write to the todo list directly\n> without running \"git rebase --edit-todo\". Instead of relying on stat\n> data this patch simply reads the possibly edited todo list and\n> compares it to the original with memcmp(). This is much faster than\n> reparsing the todo list each time. This patch reduces the time to run\n>\n> git rebase -r -xtrue v2.32.0~100 v2.32.0\n>\n> which runs 419 exec commands by 6.6%. For comparison fixing the\n> implementation bug in stat based approach reduces the time by a\n> further 1.4% and is indistinguishable from never rereading the todo\n> list.\n\nThank you for fixing my bug _and_ for improving performance,\nDscho\n\n>\n> [1] https://lore.kernel.org/git/20191125131833.GD23183@szeder.dev/\n>\n> Reported-by: SZEDER Gábor \n> Signed-off-by: Phillip Wood \n> ---\n> sequencer.c | 19 ++++++++-----------\n> sequencer.h | 1 -\n> 2 files changed, 8 insertions(+), 12 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index a248c886c27..d51440ddcd9 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2671,7 +2671,6 @@ 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> {\n> -\tstruct stat st;\n> \tconst char *todo_file = get_todo_path(opts);\n> \tint res;\n>\n> @@ -2679,11 +2678,6 @@ static int read_populate_todo(struct repository *r,\n> \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n> \t\treturn -1;\n>\n> -\tres = stat(todo_file, &st);\n> -\tif (res)\n> -\t\treturn error(_(\"could not stat '%s'\"), todo_file);\n> -\tfill_stat_data(&todo_list->stat, &st);\n> -\n> \tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n> \tif (res) {\n> \t\tif (is_rebase_i(opts))\n> @@ -4258,12 +4252,14 @@ static int reread_todo_if_changed(struct repository *r,\n> \t\t\t\t struct todo_list *todo_list,\n> \t\t\t\t struct replay_opts *opts)\n> {\n> -\tstruct stat st;\n> +\tint offset;\n> +\tstruct strbuf buf = STRBUF_INIT;\n>\n> -\tif (stat(get_todo_path(opts), &st)) {\n> -\t\treturn error_errno(_(\"could not stat '%s'\"),\n> -\t\t\t\t get_todo_path(opts));\n> -\t} else if (match_stat_data(&todo_list->stat, &st)) {\n> +\tif (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)\n> +\t\treturn -1;\n> +\toffset = get_item_line_offset(todo_list, todo_list->current + 1);\n> +\tif (buf.len != todo_list->buf.len - offset ||\n> +\t memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {\n> \t\t/* Reread the todo file if it has changed. */\n> \t\ttodo_list_release(todo_list);\n> \t\tif (read_populate_todo(r, todo_list, opts))\n> @@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,\n> \t\t/* `current` will be incremented on return */\n> \t\ttodo_list->current = -1;\n> \t}\n> +\tstrbuf_release(&buf);\n>\n> \treturn 0;\n> }\n> diff --git a/sequencer.h b/sequencer.h\n> index d57d8ea23d7..cdeb0c6be47 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -116,7 +116,6 @@ struct todo_list {\n> \tstruct todo_item *items;\n> \tint nr, alloc, current;\n> \tint done_nr, total_nr;\n> -\tstruct stat_data stat;\n> };\n>\n> #define TODO_LIST_INIT { STRBUF_INIT }\n> --\n> gitgitgadget\n>\n>\n"},{"id":"435238","messageId":"nycvar.QRO.7.76.6.2109091250540.59@tvgsbejvaqbjf.bet","threadId":"56464","inReplyTo":"f05dc55f-a7e4-b8f7-7b0c-5000bf48f803@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T10:53:17Z","receivedAt":"2021-09-09T10:53:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Wed, 8 Sep 2021, Philippe Blain wrote:\n\n> Hi Phillip,\n>\n> Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :\n> > From: Phillip Wood \n> >\n> > The \"apply\" based rebase has avoided forking git checkout since\n> > ac7f467fef (\"builtin/rebase: support running \"git rebase \"\",\n> > 2018-08-07). The code that handles the checkout was moved into libgit\n> > by b309a97108 (\"reset: extract reset_head() from rebase\", 2020-04-07)\n> > so lets start using it for the \"merge\" based rebase as well. This\n> > opens the way for us to stop calling the post-checkout hook in the\n> > future.\n> >\n>\n> While in general I think it's a good thing to avoid forking, this change\n> might result in behavioral differences. Any config that affects\n> 'git checkout' but not the internal 'reset.c::reset_head' function might\n> play a role in the rebase UX.\n>\n> One that immediately came to mind is 'submodule.recurse'. This initial\n> 'onto' checkout was pretty much the only part of 'git rebase' that did\n> something useful for submodules, so it's kind of sad to see it regress.\n> [That is, until someone takes the time to implement 'git rebase\n> --recurse-submodules' and makes sure *all* code paths that touch the\n> working tree pay attention to this flag, and that will probably\n> necessitate 'git merge --recurse-submodules' first because of the\n> 'merge' backend... as far as I'm aware it's on Emily's list [1], it's\n> also on mine but I don't know when I'll get the time.]\n\nGood point, there is more in the `git_checkout_config()` function (which\nis `static` in `builtin/checkout.c`, and could easily be moved to\n`checkout.[ch]`). We should probably fall back to calling that function\nrather than `git_default_config()` in `rebase_config()`.\n\n> Anyway, I'm not saying that we should not do what this patch is\n> proposing, but I think caveats such as that should be documented in the\n> commit message, and maybe an audit of other configs that might results\n> in behavioural differences should be done.\n\nSince this is already a bug in the `apply` backend, it would be even\nbetter to follow-up with a fix, hint, hint, nudge, nudge ;-)\n\nCiao,\nDscho\n"},{"id":"435239","messageId":"nycvar.QRO.7.76.6.2109091254010.59@tvgsbejvaqbjf.bet","threadId":"56464","inReplyTo":"c8a92d4242b4dfad71297d1eec9c3ef658641193.1631108472.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] rebase: remove unused parameter","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T10:54:52Z","receivedAt":"2021-09-09T10:54:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood \n>\n> Now that we use reset_head() we don't need to pass orig_head around.\n\nDoes this indicate a change in behavior? When we call `update_ref()` with\nthe original `HEAD`, we get some version of safety in that it will fail if\nanything changed the ref in an unexpected way in the meantime.\n\nCiao,\nDscho\n"},{"id":"435252","messageId":"e7224105-83c6-7f12-f63a-474bd477583a@gmail.com","threadId":"56464","inReplyTo":"c42d4051-59cd-094a-4570-32cf4d38ec27@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-09T12:40:56Z","receivedAt":"2021-09-09T14:18:30Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Phillip,\n\nLe 2021-09-09 à 06:09, Phillip Wood a écrit :\n> Hi Philippe\n> \n> On 08/09/2021 19:14, Philippe Blain wrote:\n>> Hi Phillip,\n>> \n>> Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :\n>>> From: Phillip Wood \n>>> \n>>> The \"apply\" based rebase has avoided forking git checkout since \n>>> ac7f467fef (\"builtin/rebase: support running \"git rebase\n>>> \"\", 2018-08-07). The code that handles the checkout was\n>>> moved into libgit by b309a97108 (\"reset: extract reset_head()\n>>> from rebase\", 2020-04-07) so lets start using it for the \"merge\"\n>>> based rebase as well. This opens the way for us to stop calling\n>>> the post-checkout hook in the future.\n>>> \n>> \n>> While in general I think it's a good thing to avoid forking, this\n>> change might result in behavioral differences. Any config that\n>> affects 'git checkout' but not the internal 'reset.c::reset_head'\n>> function might play a role in the rebase UX.\n>> \n>> One that immediately came to mind is 'submodule.recurse'. This\n>> initial 'onto' checkout was pretty much the only part of 'git\n>> rebase' that did something useful for submodules, so it's kind of\n>> sad to see it regress.\n> \n> Thanks for pointing that out. As a non-submodule user my question\n> would be is it actually useful for the initial checkout to work that\n> way if the rest of rebase (and the checkout for the am backend)\n> ignores submodules? reset.c::reset_head() just uses unpack trees like\n> checkout so if rebase read 'submodule.recurse' then reset_head()\n> would work like 'git checkout' and also 'git rebase --abort' and the\n> \"reset\" command in the todo list would start checking out submodules.\n> I'm reluctant to do that until the merge backend also handles\n> submodules unless there is a good reason that such partial submodule\n> support would help submodule users.\n\nYeah, it's not that useful, I have to admit; it can also be very confusing\nsince some parts of rebase are affected, and some not. For example, any time\nthe rebase stops, like for 'edit', 'break', and when there are conflicts, the\nsubmodules are not updated. So I think a full solution is better than a partial\nsolution; in the meantime I'm thinking the change you are proposing would actually\nbe less confusing, even if it slightly changes behaviour...\n\nAs an aside, I *think* reading submodule.recurse in rebase like it's done in checkout\net al., i.e. something like this:\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 33e0961900..125ec907e4 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -26,6 +26,7 @@\n #include \"rerere.h\"\n #include \"branch.h\"\n #include \"sequencer.h\"\n+#include \"submodule.h\"\n #include \"rebase-interactive.h\"\n #include \"reset.h\"\n \n@@ -1106,6 +1107,9 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t\treturn git_config_string(&opts->default_backend, var, value);\n \t}\n \n+\tif (!strcmp(var, \"submodule.recurse\"))\n+\t\treturn git_default_submodule_config(var, value, data);\n+\n \treturn git_default_config(var, value, data);\n }\n \n\nwould actually also affect the merges\nperformed during the rebase, since that would affect the \"global\" state in submodule.c.\nI hacked exactly that the other day but did not test extensively...\n\nCheers,\nPhilippe.\n"},{"id":"435259","messageId":"a4212a67-ba79-0976-db19-334812137e81@gmail.com","threadId":"56464","inReplyTo":"nycvar.QRO.7.76.6.2109091250540.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-09T12:44:02Z","receivedAt":"2021-09-09T14:19:37Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2021-09-09 à 06:53, Johannes Schindelin a écrit :\n> Hi Philippe,\n> \n> On Wed, 8 Sep 2021, Philippe Blain wrote:\n> \n>> Anyway, I'm not saying that we should not do what this patch is\n>> proposing, but I think caveats such as that should be documented in the\n>> commit message, and maybe an audit of other configs that might results\n>> in behavioural differences should be done.\n> \n> Since this is already a bug in the `apply` backend, it would be even\n> better to follow-up with a fix, hint, hint, nudge, nudge ;-)\n> \n\nI'm not sure I understand what you are saying. The fact that 'rebase'\ndoes not pay attention to 'submodule.recurse' is not a bug in my opinion,\nit's just a limitation of the current code... Or do you mean something else?\n\nThanks,\nPhilippe.\n"},{"id":"435263","messageId":"408dc1d3-44b8-a955-4d7b-94f23fa8a6bc@gmail.com","threadId":"56464","inReplyTo":"e7224105-83c6-7f12-f63a-474bd477583a@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-09T13:57:23Z","receivedAt":"2021-09-09T14:39:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Philippe\n\nOn 09/09/2021 13:40, Philippe Blain wrote:\n>>> While in general I think it's a good thing to avoid forking, this\n>>> change might result in behavioral differences. Any config that\n>>> affects 'git checkout' but not the internal 'reset.c::reset_head'\n>>> function might play a role in the rebase UX.\n>>>\n>>> One that immediately came to mind is 'submodule.recurse'. This\n>>> initial 'onto' checkout was pretty much the only part of 'git\n>>> rebase' that did something useful for submodules, so it's kind of\n>>> sad to see it regress.\n>>\n>> Thanks for pointing that out. As a non-submodule user my question\n>> would be is it actually useful for the initial checkout to work that\n>> way if the rest of rebase (and the checkout for the am backend)\n>> ignores submodules? reset.c::reset_head() just uses unpack trees like\n>> checkout so if rebase read 'submodule.recurse' then reset_head()\n>> would work like 'git checkout' and also 'git rebase --abort' and the\n>> \"reset\" command in the todo list would start checking out submodules.\n\nit would also affect fast-forwards\n\n>> I'm reluctant to do that until the merge backend also handles\n>> submodules unless there is a good reason that such partial submodule\n>> support would help submodule users.\n> \n> Yeah, it's not that useful, I have to admit; it can also be very confusing\n> since some parts of rebase are affected, and some not. For example, any \n> time\n> the rebase stops, like for 'edit', 'break', and when there are \n> conflicts, the\n> submodules are not updated. So I think a full solution is better than a \n> partial\n> solution; in the meantime I'm thinking the change you are proposing \n> would actually\n> be less confusing, even if it slightly changes behaviour...\n> \n> As an aside, I *think* reading submodule.recurse in rebase like it's \n> done in checkout\n> et al., i.e. something like this:\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 33e0961900..125ec907e4 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -26,6 +26,7 @@\n>  #include \"rerere.h\"\n>  #include \"branch.h\"\n>  #include \"sequencer.h\"\n> +#include \"submodule.h\"\n>  #include \"rebase-interactive.h\"\n>  #include \"reset.h\"\n> \n> @@ -1106,6 +1107,9 @@ static int rebase_config(const char *var, const \n> char *value, void *data)\n>          return git_config_string(&opts->default_backend, var, value);\n>      }\n> \n> +    if (!strcmp(var, \"submodule.recurse\"))\n> +        return git_default_submodule_config(var, value, data);\n\nThat looks about right to me though I think it would be safer to call \ngit_default_submodule_config() for submodule.* rather than just \nsubmodule.recurse\n\n>      return git_default_config(var, value, data);\n>  }\n> \n> \n> would actually also affect the merges\n> performed during the rebase, since that would affect the \"global\" state \n> in submodule.c.\n> I hacked exactly that the other day but did not test extensively...\n\nmerge-ort.c:checkout() which is used by merge_switch_to_result() uses \nunpack_trees() so it will pick up the global state and hopefully should \njust work (cc'ing Elijah to check as I didn't look what happens when \nthere are conflicts). merge-recursive.c:update_file_flags() does this \nwhen updating the work tree\n\n if (S_ISGITLINK(contents->mode)) { \n\n /* \n\n * We may later decide to recursively descend into \n\n * the submodule directory and update its index \n\n * and/or work tree, but we do not do that now. \n\n */ \n\n update_wd = 0; \n\n goto update_index; \n\n } \n\n \n\nso it looks like it does not update the submodule directory. Given \nmerge-ort is now the default perhaps it's time for rebase (and \ncherry-pick/revert) to start reading the submodule config settings (we \nparse the config before we know if we'll be using merge-ort so I don't \nknow how easy it would be to only parse the submodule settings if we are \nusing merge-ort).\n\nBest Wishes\n\nPhillip\n"},{"id":"435264","messageId":"632884f6-ff63-23de-c043-0848939b4e15@gmail.com","threadId":"56464","inReplyTo":"nycvar.QRO.7.76.6.2109091254010.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 5/5] rebase: remove unused parameter","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-09T14:04:11Z","receivedAt":"2021-09-09T14:40:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 09/09/2021 11:54, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood \n>>\n>> Now that we use reset_head() we don't need to pass orig_head around.\n> \n> Does this indicate a change in behavior? When we call `update_ref()` with\n> the original `HEAD`, we get some version of safety in that it will fail if\n> anything changed the ref in an unexpected way in the meantime.\n\nGood point, it looks like that was overlooked when the am rebase \nstarting using reset_head(). There are already too many arguments to \nreset_head(), I'll look at changing it (in a separate commit) to take a \nstruct of options instead.\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Dscho\n> \n"},{"id":"435273","messageId":"CABPp-BEbY0BqkBP4r-6XpGk46J+Y+W8+7cVZXQg5fuJXYOntDQ@mail.gmail.com","threadId":"56464","inReplyTo":"408dc1d3-44b8-a955-4d7b-94f23fa8a6bc@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-09T15:01:46Z","receivedAt":"2021-09-09T15:02:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Thu, Sep 9, 2021 at 6:57 AM Phillip Wood wrote:\n>\n> Hi Philippe\n>\n> On 09/09/2021 13:40, Philippe Blain wrote:\n> >>> While in general I think it's a good thing to avoid forking, this\n> >>> change might result in behavioral differences. Any config that\n> >>> affects 'git checkout' but not the internal 'reset.c::reset_head'\n> >>> function might play a role in the rebase UX.\n> >>>\n> >>> One that immediately came to mind is 'submodule.recurse'. This\n> >>> initial 'onto' checkout was pretty much the only part of 'git\n> >>> rebase' that did something useful for submodules, so it's kind of\n> >>> sad to see it regress.\n> >>\n> >> Thanks for pointing that out. As a non-submodule user my question\n> >> would be is it actually useful for the initial checkout to work that\n> >> way if the rest of rebase (and the checkout for the am backend)\n> >> ignores submodules? reset.c::reset_head() just uses unpack trees like\n> >> checkout so if rebase read 'submodule.recurse' then reset_head()\n> >> would work like 'git checkout' and also 'git rebase --abort' and the\n> >> \"reset\" command in the todo list would start checking out submodules.\n>\n> it would also affect fast-forwards\n>\n> >> I'm reluctant to do that until the merge backend also handles\n> >> submodules unless there is a good reason that such partial submodule\n> >> support would help submodule users.\n> >\n> > Yeah, it's not that useful, I have to admit; it can also be very confusing\n> > since some parts of rebase are affected, and some not. For example, any\n> > time\n> > the rebase stops, like for 'edit', 'break', and when there are\n> > conflicts, the\n> > submodules are not updated. So I think a full solution is better than a\n> > partial\n> > solution; in the meantime I'm thinking the change you are proposing\n> > would actually\n> > be less confusing, even if it slightly changes behaviour...\n> >\n> > As an aside, I *think* reading submodule.recurse in rebase like it's\n> > done in checkout\n> > et al., i.e. something like this:\n> >\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 33e0961900..125ec907e4 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -26,6 +26,7 @@\n> > #include \"rerere.h\"\n> > #include \"branch.h\"\n> > #include \"sequencer.h\"\n> > +#include \"submodule.h\"\n> > #include \"rebase-interactive.h\"\n> > #include \"reset.h\"\n> >\n> > @@ -1106,6 +1107,9 @@ static int rebase_config(const char *var, const\n> > char *value, void *data)\n> > return git_config_string(&opts->default_backend, var, value);\n> > }\n> >\n> > + if (!strcmp(var, \"submodule.recurse\"))\n> > + return git_default_submodule_config(var, value, data);\n>\n> That looks about right to me though I think it would be safer to call\n> git_default_submodule_config() for submodule.* rather than just\n> submodule.recurse\n>\n> > return git_default_config(var, value, data);\n> > }\n> >\n> >\n> > would actually also affect the merges\n> > performed during the rebase, since that would affect the \"global\" state\n> > in submodule.c.\n> > I hacked exactly that the other day but did not test extensively...\n>\n> merge-ort.c:checkout() which is used by merge_switch_to_result() uses\n> unpack_trees() so it will pick up the global state and hopefully should\n> just work (cc'ing Elijah to check as I didn't look what happens when\n> there are conflicts).\n\nYep, merge-ort was designed to just piggy back on checkout code. The\ncheckout() function was basically just code lifted from\nbuiltin/checkout.c. Using that code means that merges now also\nbenefit from all the special working tree handling that is encoded\ninto git-checkout -- whether that's parallel checkout, submodule\nhandling, tricky D/F switches or symlink handling, etc. In contrast\nto merge-recursive, it does not need hundreds and hundreds of lines of\nspecial worktree updating code sprayed all over the codebase.\n\nConflicts are not special in this regard; merge-ort creates a tree\nwhich has files that include conflict markers, and then merge-ort\ncalls checkout() to switch the working copy over to that tree.\n\nThe only issue conflicts present for merge-ort, is that AFTER it has\nchecked out that special tree with conflict markers, it then has to go\nand touch up the index afterwards to replace the entries for\nconflicted files with multiple higher order stages. (You could say\nthat merge-recursive is \"index-first\", since its design focuses on the\nindex -- updating it first and then figuring out everything else like\nupdating the working tree with special code afterwards. In contrast,\nmerge-ort ignores the index entirely until the very end -- after a new\nmerge tree is created and after the working tree is updated.)\n\n> merge-recursive.c:update_file_flags() does this\n> when updating the work tree\n>\n> if (S_ISGITLINK(contents->mode)) {\n>\n> /*\n>\n> * We may later decide to recursively descend into\n>\n> * the submodule directory and update its index\n>\n> * and/or work tree, but we do not do that now.\n>\n> */\n>\n> update_wd = 0;\n>\n> goto update_index;\n>\n> }\n>\n>\n>\n> so it looks like it does not update the submodule directory. Given\n> merge-ort is now the default perhaps it's time for rebase (and\n> cherry-pick/revert) to start reading the submodule config settings (we\n> parse the config before we know if we'll be using merge-ort so I don't\n> know how easy it would be to only parse the submodule settings if we are\n> using merge-ort).\n\nI'd just parse any needed config in all cases. The submodule settings\naren't going to hurt merge-recursive; it'll just ignore them. (Or are\nyou worried about a mix-and-match of rebase calling both checkout and\nmerge code doing weird things, and you'd rather not have the checkout\nbits update submodules if the merges won't?)\n"},{"id":"435274","messageId":"CABPp-BG=pWhLpjKKSbgUyuaSEwjdkhwv8Vb1Ut1-n3LziSy_pQ@mail.gmail.com","threadId":"56464","inReplyTo":"39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-09T15:03:27Z","receivedAt":"2021-09-09T15:04:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 8, 2021 at 6:44 AM Phillip Wood via GitGitGadget\n wrote:\n>\n> From: Phillip Wood \n>\n> The \"apply\" based rebase has avoided forking git checkout since\n> ac7f467fef (\"builtin/rebase: support running \"git rebase \"\",\n> 2018-08-07). The code that handles the checkout was moved into libgit\n> by b309a97108 (\"reset: extract reset_head() from rebase\", 2020-04-07)\n> so lets start using it for the \"merge\" based rebase as well. This\n> opens the way for us to stop calling the post-checkout hook in the\n> future.\n\nWahoo! So exciting to see this, and for the future plans mentioned here. :-)\n\n> Signed-off-by: Phillip Wood \n> ---\n> sequencer.c | 29 ++++-------------------------\n> 1 file changed, 4 insertions(+), 25 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index d51440ddcd9..1a9dbc70d3c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4192,42 +4192,21 @@ int apply_autostash_oid(const char *stash_oid)\n> return apply_save_autostash_oid(stash_oid, 1);\n> }\n>\n> -static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n> - const char *commit, const char *action)\n> -{\n> - struct child_process cmd = CHILD_PROCESS_INIT;\n> - int ret;\n> -\n> - cmd.git_cmd = 1;\n> -\n> - strvec_push(&cmd.args, \"checkout\");\n> - strvec_push(&cmd.args, commit);\n> - strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> -\n> - if (opts->verbose)\n> - ret = run_command(&cmd);\n> - else\n> - ret = run_command_silent_on_success(&cmd);\n> -\n> - if (!ret)\n> - discard_index(r->index);\n> -\n> - return ret;\n> -}\n> -\n> static int checkout_onto(struct repository *r, struct replay_opts *opts,\n> const char *onto_name, const struct object_id *onto,\n> const struct object_id *orig_head)\n> {\n> const char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n>\n> - if (run_git_checkout(r, opts, oid_to_hex(onto), action)) {\n> + if (reset_head(r, onto, \"checkout\", NULL, RESET_HEAD_DETACH |\n> + RESET_ORIG_HEAD | RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n> + NULL, action, \"rebase\")) {\n> apply_autostash(rebase_path_autostash());\n> sequencer_remove_state(opts);\n> return error(_(\"could not detach HEAD\"));\n> }\n>\n> - return update_ref(NULL, \"ORIG_HEAD\", orig_head, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n> + return 0;\n> }\n>\n> static int stopped_at_head(struct repository *r)\n> --\n> gitgitgadget\n"},{"id":"435338","messageId":"nycvar.QRO.7.76.6.2109092342110.59@tvgsbejvaqbjf.bet","threadId":"56464","inReplyTo":"a4212a67-ba79-0976-db19-334812137e81@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T21:43:54Z","receivedAt":"2021-09-09T21:44:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Thu, 9 Sep 2021, Philippe Blain wrote:\n\n> Le 2021-09-09 à 06:53, Johannes Schindelin a écrit :\n> >\n> > On Wed, 8 Sep 2021, Philippe Blain wrote:\n> >\n> > > Anyway, I'm not saying that we should not do what this patch is\n> > > proposing, but I think caveats such as that should be documented in the\n> > > commit message, and maybe an audit of other configs that might results\n> > > in behavioural differences should be done.\n> >\n> > Since this is already a bug in the `apply` backend, it would be even\n> > better to follow-up with a fix, hint, hint, nudge, nudge ;-)\n>\n> I'm not sure I understand what you are saying.\n\nI am saying that indeed, you found what I consider a bug, but it is\nalready present in the `apply` backend. And then I am hoping that you\ncould find the time to fix it ;-)\n\n> The fact that 'rebase' does not pay attention to 'submodule.recurse' is\n> not a bug in my opinion, it's just a limitation of the current code...\n\nBut the code that spawned `git checkout` _did_ pay attention to the\n`submodule.*` settings, no?\n\nCiao,\nDscho\n\n> Or do you mean something else?\n>\n> Thanks,\n> Philippe.\n>\n>\n"},{"id":"435383","messageId":"nycvar.QRO.7.76.6.2109101245510.59@tvgsbejvaqbjf.bet","threadId":"56464","inReplyTo":"a4212a67-ba79-0976-db19-334812137e81@gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-10T10:46:30Z","receivedAt":"2021-09-10T10:46:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Thu, 9 Sep 2021, Philippe Blain wrote:\n\n> Le 2021-09-09 à 06:53, Johannes Schindelin a écrit :\n> >\n> > On Wed, 8 Sep 2021, Philippe Blain wrote:\n> >\n> > > Anyway, I'm not saying that we should not do what this patch is\n> > > proposing, but I think caveats such as that should be documented in the\n> > > commit message, and maybe an audit of other configs that might results\n> > > in behavioural differences should be done.\n> >\n> > Since this is already a bug in the `apply` backend, it would be even\n> > better to follow-up with a fix, hint, hint, nudge, nudge ;-)\n> >\n>\n> I'm not sure I understand what you are saying. The fact that 'rebase'\n> does not pay attention to 'submodule.recurse' is not a bug in my opinion,\n> it's just a limitation of the current code... Or do you mean something else?\n\nI must have misunderstood you. I thought you were saying that Phillip's\npatch introduces the regression where `submodule.recurse` is no longer\nrespected.\n\nCiao,\nDscho\n"},{"id":"435404","messageId":"9d643b71-922e-5b61-b8d5-55b9c30c07b5@gmail.com","threadId":"56464","inReplyTo":"nycvar.QRO.7.76.6.2109101245510.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-10T11:58:50Z","receivedAt":"2021-09-10T11:58:55Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2021-09-10 à 06:46, Johannes Schindelin a écrit :\n> Hi Philippe,\n> \n> On Thu, 9 Sep 2021, Philippe Blain wrote:\n> \n>> Le 2021-09-09 à 06:53, Johannes Schindelin a écrit :\n>>>\n>>> On Wed, 8 Sep 2021, Philippe Blain wrote:\n>>>\n>>>> Anyway, I'm not saying that we should not do what this patch is\n>>>> proposing, but I think caveats such as that should be documented in the\n>>>> commit message, and maybe an audit of other configs that might results\n>>>> in behavioural differences should be done.\n>>>\n>>> Since this is already a bug in the `apply` backend, it would be even\n>>> better to follow-up with a fix, hint, hint, nudge, nudge ;-)\n>>>\n>>\n>> I'm not sure I understand what you are saying. The fact that 'rebase'\n>> does not pay attention to 'submodule.recurse' is not a bug in my opinion,\n>> it's just a limitation of the current code... Or do you mean something else?\n> \n> I must have misunderstood you. I thought you were saying that Phillip's\n> patch introduces the regression where `submodule.recurse` is no longer\n> respected.\n> \n> Ciao,\n> Dscho\n> \n\nWell it does, but only for the initial checkout of 'onto'. But as I wrote in\n[1], I think that half respecting 'submodule.recurse' is confusing UX. So\nI would think that it is not *that* bad to keep Phillip's patch as-is in the meantime\ni.e. while we wait for 'git rebase --recurse-submodules' to materialize. I think\nthat it would not be good UI either to have rebase respect 'submodule.recurse',\nbut not having a --recurse-submodules flag like all other commands that honor\nthat config.\n\nThanks,\nPhilippe.\n\nP.S. I did not CC you in [1], should I have? What's the etiquette around this,\ni.e. should I manually add CC's for people involved in \"parallel\" threads in\naddition to everyone I get by doing \"reply-all\" ?\n\n1. https://lore.kernel.org/git/pull.1034.git.1631108472.gitgitgadget@gmail.com/T/#m182b8d5f24b41c2ff8e919819229974d71258cd9\n"},{"id":"435405","messageId":"d4eff812-374a-0523-a490-ea3058db585f@gmail.com","threadId":"56464","inReplyTo":"CABPp-BEbY0BqkBP4r-6XpGk46J+Y+W8+7cVZXQg5fuJXYOntDQ@mail.gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-10T12:07:03Z","receivedAt":"2021-09-10T12:07:08Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Elijah,\n\nLe 2021-09-09 à 11:01, Elijah Newren a écrit :\n> Hi,\n> \n> On Thu, Sep 9, 2021 at 6:57 AM Phillip Wood wrote:\n>>\n\n>> merge-recursive.c:update_file_flags() does this\n>> when updating the work tree\n>>\n>> if (S_ISGITLINK(contents->mode)) {\n>>\n>> /*\n>>\n>> * We may later decide to recursively descend into\n>>\n>> * the submodule directory and update its index\n>>\n>> * and/or work tree, but we do not do that now.\n>>\n>> */\n>>\n>> update_wd = 0;\n>>\n>> goto update_index;\n>>\n>> }\n>>\n>>\n>>\n>> so it looks like it does not update the submodule directory. Given\n>> merge-ort is now the default perhaps it's time for rebase (and\n>> cherry-pick/revert) to start reading the submodule config settings (we\n>> parse the config before we know if we'll be using merge-ort so I don't\n>> know how easy it would be to only parse the submodule settings if we are\n>> using merge-ort).\n> \n> I'd just parse any needed config in all cases. The submodule settings\n> aren't going to hurt merge-recursive; it'll just ignore them. (Or are\n> you worried about a mix-and-match of rebase calling both checkout and\n> merge code doing weird things, and you'd rather not have the checkout\n> bits update submodules if the merges won't?)\n> \n\nThanks for your input. I agree that reading the config in all cases would\nbe simpler. We could even decide that since ort is the new default, the\nsubmodule support will not be \"backported\" to merge recursive (that would\nbe way simpler to implement, I think) This way we can just document it as\nsuch and be done with it. But anyway, I think this is kind of\northogonal to this here series and should be done separately.\n\nCheers,\nPhilippe.\n"},{"id":"435983","messageId":"1311e9e9-6d10-40f8-2073-077313a94301@gmail.com","threadId":"56464","inReplyTo":"CABPp-BEbY0BqkBP4r-6XpGk46J+Y+W8+7cVZXQg5fuJXYOntDQ@mail.gmail.com","subject":"Re: [PATCH 4/5] rebase -i: don't fork git checkout","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-15T15:44:33Z","receivedAt":"2021-09-15T15:44:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 09/09/2021 16:01, Elijah Newren wrote:\n> [...]\n>> merge-ort.c:checkout() which is used by merge_switch_to_result() uses\n>> unpack_trees() so it will pick up the global state and hopefully should\n>> just work (cc'ing Elijah to check as I didn't look what happens when\n>> there are conflicts).\n> \n> Yep, merge-ort was designed to just piggy back on checkout code. The\n> checkout() function was basically just code lifted from\n> builtin/checkout.c. Using that code means that merges now also\n> benefit from all the special working tree handling that is encoded\n> into git-checkout -- whether that's parallel checkout, submodule\n> handling, tricky D/F switches or symlink handling, etc. In contrast\n> to merge-recursive, it does not need hundreds and hundreds of lines of\n> special worktree updating code sprayed all over the codebase.\n> \n> Conflicts are not special in this regard; merge-ort creates a tree\n> which has files that include conflict markers, and then merge-ort\n> calls checkout() to switch the working copy over to that tree.\n> \n> The only issue conflicts present for merge-ort, is that AFTER it has\n> checked out that special tree with conflict markers, it then has to go\n> and touch up the index afterwards to replace the entries for\n> conflicted files with multiple higher order stages. (You could say\n> that merge-recursive is \"index-first\", since its design focuses on the\n> index -- updating it first and then figuring out everything else like\n> updating the working tree with special code afterwards. In contrast,\n> merge-ort ignores the index entirely until the very end -- after a new\n> merge tree is created and after the working tree is updated.)\n\nThanks for explaining, it's a nice design feature that you can just reuse\nthe checkout code to update the working copy with the merge result\n\n>> merge-recursive.c:update_file_flags() does this\n>> when updating the work tree\n>>\n>> if (S_ISGITLINK(contents->mode)) {\n>> /*\n>> * We may later decide to recursively descend into\n>> * the submodule directory and update its index\n>> * and/or work tree, but we do not do that now.\n>> */\n>> update_wd = 0;\n>> goto update_index;\n>> }\n>>\n>> so it looks like it does not update the submodule directory. Given\n>> merge-ort is now the default perhaps it's time for rebase (and\n>> cherry-pick/revert) to start reading the submodule config settings (we\n>> parse the config before we know if we'll be using merge-ort so I don't\n>> know how easy it would be to only parse the submodule settings if we are\n>> using merge-ort).\n> \n> I'd just parse any needed config in all cases. The submodule settings\n> aren't going to hurt merge-recursive; it'll just ignore them. (Or are\n> you worried about a mix-and-match of rebase calling both checkout and\n> merge code doing weird things, and you'd rather not have the checkout\n> bits update submodules if the merges won't?)\n\nI'd rather just parse the config when we know submodules are going to be\nrebased, I think it's confusing if some bit work and others don't. I've\ntried the diff below locally, but t7402-submodule-rebase.sh does not show\nany change (I was hoping some text_expect_failure would be fixed) so I'm\nnot sure if it's working or not and I ran out of time.\n\nBest Wishes\n\nPhillip\n\n--- >8 ---\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex eed70168df..a35a9e3460 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -26,6 +26,7 @@\n #include \"rerere.h\"\n #include \"branch.h\"\n #include \"sequencer.h\"\n+#include \"submodule.h\"\n #include \"rebase-interactive.h\"\n #include \"reset.h\"\n \n@@ -1114,6 +1115,10 @@ static int rebase_config(const char *var, const char *value, void *data)\n return git_config_string(&opts->default_backend, var, value);\n }\n \n+ if (starts_with(var, \"submodule.\")) {\n+ return git_default_submodule_config(var, value, NULL);\n+ }\n+\n return git_default_config(var, value, data);\n }\n \n@@ -1820,6 +1825,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n getenv(\"GIT_TEST_MERGE_ALGORITHM\"))\n options.strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n \n+ /* only the \"ort\" merge strategy handles submodules correctly */\n+ if (!is_merge(&options) ||\n+ (options.strategy && strcmp(options.strategy, \"ort\")))\n+ git_default_submodule_config(\"submodule.recurse\", \"false\",\n+ NULL);\n+\n switch (options.type) {\n case REBASE_MERGE:\n case REBASE_PRESERVE_MERGES:\n \n"},{"id":"436862","messageId":"pull.1034.v2.git.1632410782.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.git.1631108472.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] rebase -i: a couple of small improvements","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-23T15:26:19Z","receivedAt":"2021-09-23T15:26:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks for the feedback on V1. I have decided to split this series so I'm\njust posting a re-roll of the first two patches here. The only change is to\nreword the commit message of the first patch as suggested by Eric and Dscho\n\nCover letter for V1: Fix the re-reading of the todo list after an exec or\nreword command and stop forking \"git checkout\" when checking out \"onto\"\n\nPhillip Wood (2):\n sequencer.c: factor out a function\n rebase: fix todo-list rereading\n\n sequencer.c | 47 +++++++++++++++++++++++++++--------------------\n sequencer.h | 1 -\n 2 files changed, 27 insertions(+), 21 deletions(-)\n\n\nbase-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1034%2Fphillipwood%2Fwip%2Frebase-reread-todo-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1034/phillipwood/wip/rebase-reread-todo-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1034\n\nRange-diff vs v1:\n\n 1: 53cde4825b4 ! 1: 98ebefc140e sequencer.c: factor out a function\n @@ Commit message\n sequencer.c: factor out a function\n \n This code is heavily indented and obscures the high level logic within\n - the loop. Lets move it to its own function before modifying it in the\n - next commit.\n + the loop. Let's move it to its own function before modifying it in the\n + next commit. Note that there is a subtle change in behavior if the\n + todo list cannot be reread. Previously todo_list->current was\n + incremented before returning, now it returns immediately.\n \n Signed-off-by: Phillip Wood \n \n 2: 3b17a4e3d3f = 2: 0d89c506192 rebase: fix todo-list rereading\n 3: 614555fc10f < -: ----------- reset_head(): mark oid parameter as const\n 4: 39ad40c9297 < -: ----------- rebase -i: don't fork git checkout\n 5: c8a92d4242b < -: ----------- rebase: remove unused parameter\n\n-- \ngitgitgadget\n"},{"id":"436863","messageId":"0d89c506192a84822a3fbd6c76befac187129ad4.1632410782.git.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.v2.git.1632410782.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase: fix todo-list rereading","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-23T15:26:21Z","receivedAt":"2021-09-23T15:26:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n54fd3243da (\"rebase -i: reread the todo list if `exec` touched it\",\n2017-04-26) sought to reread the todo list after running an exec\ncommand only if it had been changed. To accomplish this it checks the\nstat data of the todo list after running an exec command to see if it\nhas changed. Unfortunately there are two problems, firstly the\nimplementation is buggy we actually reread the list after each exec\nwhich is quadratic in the number of commit lookups and secondly the\ndesign is predicated on using nanosecond time stamps which are not the\ndefault.\n\nThe implementation bug stems from the fact that we write a new todo\nlist to disk before running each command but do not update the stat\ndata to reflect this[1].\n\nThe design problem is that it is possible for the user to edit the\ntodo list without changing its size or inode which means we have to\nrely on the mtime to tell us if it has changed. Unfortunately unless\ngit is built with USE_NSEC it is possible for the original and edited\nlist to share the same mtime.\n\nIdeally \"git rebase --edit-todo\" would set a flag that we would then\ncheck in sequencer.c. Unfortunately this is approach will not work as\nthere are scripts in the wild that write to the todo list directly\nwithout running \"git rebase --edit-todo\". Instead of relying on stat\ndata this patch simply reads the possibly edited todo list and\ncompares it to the original with memcmp(). This is much faster than\nreparsing the todo list each time. This patch reduces the time to run\n\n git rebase -r -xtrue v2.32.0~100 v2.32.0\n\nwhich runs 419 exec commands by 6.6%. For comparison fixing the\nimplementation bug in stat based approach reduces the time by a\nfurther 1.4% and is indistinguishable from never rereading the todo\nlist.\n\n[1] https://lore.kernel.org/git/20191125131833.GD23183@szeder.dev/\n\nReported-by: SZEDER Gábor \nSigned-off-by: Phillip Wood \n---\n sequencer.c | 19 ++++++++-----------\n sequencer.h | 1 -\n 2 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a248c886c27..d51440ddcd9 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2671,7 +2671,6 @@ 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 {\n-\tstruct stat st;\n \tconst char *todo_file = get_todo_path(opts);\n \tint res;\n \n@@ -2679,11 +2678,6 @@ static int read_populate_todo(struct repository *r,\n \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n \t\treturn -1;\n \n-\tres = stat(todo_file, &st);\n-\tif (res)\n-\t\treturn error(_(\"could not stat '%s'\"), todo_file);\n-\tfill_stat_data(&todo_list->stat, &st);\n-\n \tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n \tif (res) {\n \t\tif (is_rebase_i(opts))\n@@ -4258,12 +4252,14 @@ static int reread_todo_if_changed(struct repository *r,\n \t\t\t\t struct todo_list *todo_list,\n \t\t\t\t struct replay_opts *opts)\n {\n-\tstruct stat st;\n+\tint offset;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (stat(get_todo_path(opts), &st)) {\n-\t\treturn error_errno(_(\"could not stat '%s'\"),\n-\t\t\t\t get_todo_path(opts));\n-\t} else if (match_stat_data(&todo_list->stat, &st)) {\n+\tif (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)\n+\t\treturn -1;\n+\toffset = get_item_line_offset(todo_list, todo_list->current + 1);\n+\tif (buf.len != todo_list->buf.len - offset ||\n+\t memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {\n \t\t/* Reread the todo file if it has changed. */\n \t\ttodo_list_release(todo_list);\n \t\tif (read_populate_todo(r, todo_list, opts))\n@@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,\n \t\t/* `current` will be incremented on return */\n \t\ttodo_list->current = -1;\n \t}\n+\tstrbuf_release(&buf);\n \n \treturn 0;\n }\ndiff --git a/sequencer.h b/sequencer.h\nindex d57d8ea23d7..cdeb0c6be47 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -116,7 +116,6 @@ struct todo_list {\n \tstruct todo_item *items;\n \tint nr, alloc, current;\n \tint done_nr, total_nr;\n-\tstruct stat_data stat;\n };\n \n #define TODO_LIST_INIT { STRBUF_INIT }\n-- \ngitgitgadget\n"},{"id":"436864","messageId":"98ebefc140e5ae61cd415b17fb95ee9184520c83.1632410782.git.gitgitgadget@gmail.com","threadId":"56464","inReplyTo":"pull.1034.v2.git.1632410782.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] sequencer.c: factor out a function","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-23T15:26:20Z","receivedAt":"2021-09-23T15:26:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis code is heavily indented and obscures the high level logic within\nthe loop. Let's move it to its own function before modifying it in the\nnext commit. Note that there is a subtle change in behavior if the\ntodo list cannot be reread. Previously todo_list->current was\nincremented before returning, now it returns immediately.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7f07cd00f3f..a248c886c27 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4254,6 +4254,27 @@ static int stopped_at_head(struct repository *r)\n \n }\n \n+static int reread_todo_if_changed(struct repository *r,\n+\t\t\t\t struct todo_list *todo_list,\n+\t\t\t\t struct replay_opts *opts)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(get_todo_path(opts), &st)) {\n+\t\treturn error_errno(_(\"could not stat '%s'\"),\n+\t\t\t\t get_todo_path(opts));\n+\t} else if (match_stat_data(&todo_list->stat, &st)) {\n+\t\t/* Reread the todo file if it has changed. */\n+\t\ttodo_list_release(todo_list);\n+\t\tif (read_populate_todo(r, todo_list, opts))\n+\t\t\treturn -1; /* message was printed */\n+\t\t/* `current` will be incremented on return */\n+\t\ttodo_list->current = -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\n@@ -4433,20 +4454,9 @@ static int pick_commits(struct repository *r,\n \t\t\t\t\t\t\titem->commit,\n \t\t\t\t\t\t\targ, item->arg_len,\n \t\t\t\t\t\t\topts, res, 0);\n-\t\t} else if (is_rebase_i(opts) && check_todo && !res) {\n-\t\t\tstruct stat st;\n-\n-\t\t\tif (stat(get_todo_path(opts), &st)) {\n-\t\t\t\tres = error_errno(_(\"could not stat '%s'\"),\n-\t\t\t\t\t\t get_todo_path(opts));\n-\t\t\t} else if (match_stat_data(&todo_list->stat, &st)) {\n-\t\t\t\t/* Reread the todo file if it has changed. */\n-\t\t\t\ttodo_list_release(todo_list);\n-\t\t\t\tif (read_populate_todo(r, todo_list, opts))\n-\t\t\t\t\tres = -1; /* message was printed */\n-\t\t\t\t/* `current` will be incremented below */\n-\t\t\t\ttodo_list->current = -1;\n-\t\t\t}\n+\t\t} else if (is_rebase_i(opts) && check_todo && !res &&\n+\t\t\t reread_todo_if_changed(r, todo_list, opts)) {\n+\t\t\treturn -1;\n \t\t}\n \n \t\ttodo_list->current++;\n-- \ngitgitgadget\n\n"},{"id":"436971","messageId":"xmqqilyqvtmh.fsf@gitster.g","threadId":"56464","inReplyTo":"0d89c506192a84822a3fbd6c76befac187129ad4.1632410782.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: fix todo-list rereading","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-24T16:13:58Z","receivedAt":"2021-09-24T16:14:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> .... Instead of relying on stat\n> data this patch simply reads the possibly edited todo list and\n> compares it to the original with memcmp(). This is much faster than\n> reparsing the todo list each time.\n\nNice. Is that an egg of Columbus or what ;-)\n\n> +\tif (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)\n> +\t\treturn -1;\n> +\toffset = get_item_line_offset(todo_list, todo_list->current + 1);\n> +\tif (buf.len != todo_list->buf.len - offset ||\n> +\t memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {\n> \t\t/* Reread the todo file if it has changed. */\n> \t\ttodo_list_release(todo_list);\n> \t\tif (read_populate_todo(r, todo_list, opts))\n\nAs we already have the contents of hte file in the buffer, we could\nfurther refactor the code around read_populate_todo() to tell it not\nto reopen and reread the rebase-todo file (which risks toctou race),\nbut that is OK for now, I would think.\n\n> @@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,\n> \t\t/* `current` will be incremented on return */\n> \t\ttodo_list->current = -1;\n> \t}\n> +\tstrbuf_release(&buf);\n> \n> \treturn 0;\n> }\n> diff --git a/sequencer.h b/sequencer.h\n> index d57d8ea23d7..cdeb0c6be47 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -116,7 +116,6 @@ struct todo_list {\n> \tstruct todo_item *items;\n> \tint nr, alloc, current;\n> \tint done_nr, total_nr;\n> -\tstruct stat_data stat;\n\nGood riddance ;-)\n\nWill queue. Thanks.\n"},{"id":"437021","messageId":"xmqq1r5du68z.fsf@gitster.g","threadId":"56464","inReplyTo":"pull.1034.v2.git.1632410782.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] rebase -i: a couple of small improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-24T19:24:12Z","receivedAt":"2021-09-24T19:24:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> Thanks for the feedback on V1. I have decided to split this series so I'm\n> just posting a re-roll of the first two patches here. The only change is to\n> reword the commit message of the first patch as suggested by Eric and Dscho\n\nOK. Prioritizing the fix and leaving the add-on part that is not\nyet solid for later makes perfect sense.\n\n"},{"id":"437275","messageId":"bed00cb7-2204-668c-dca5-4bc883e80bab@gmail.com","threadId":"56464","inReplyTo":"xmqqilyqvtmh.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] rebase: fix todo-list rereading","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-28T10:20:54Z","receivedAt":"2021-09-28T10:21:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/09/2021 17:13, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> .... Instead of relying on stat\n>> data this patch simply reads the possibly edited todo list and\n>> compares it to the original with memcmp(). This is much faster than\n>> reparsing the todo list each time.\n> \n> Nice. Is that an egg of Columbus or what ;-)\n> \n>> +\tif (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)\n>> +\t\treturn -1;\n>> +\toffset = get_item_line_offset(todo_list, todo_list->current + 1);\n>> +\tif (buf.len != todo_list->buf.len - offset ||\n>> +\t memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {\n>> \t\t/* Reread the todo file if it has changed. */\n>> \t\ttodo_list_release(todo_list);\n>> \t\tif (read_populate_todo(r, todo_list, opts))\n> \n> As we already have the contents of hte file in the buffer, we could\n> further refactor the code around read_populate_todo() to tell it not\n> to reopen and reread the rebase-todo file (which risks toctou race),\n> but that is OK for now, I would think.\n\nI did wonder about doing that but decided to punt it for now, I don't \nthink the race is a concern - if another process is writing to the todo \nlist while rebase is picking commits it is asking for trouble already. I \nsuspect doing this will be easier once git-rebase--preserve-merges.sh is \ngone from master.\n\n>> @@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,\n>> \t\t/* `current` will be incremented on return */\n>> \t\ttodo_list->current = -1;\n>> \t}\n>> +\tstrbuf_release(&buf);\n>> \n>> \treturn 0;\n>> }\n>> diff --git a/sequencer.h b/sequencer.h\n>> index d57d8ea23d7..cdeb0c6be47 100644\n>> --- a/sequencer.h\n>> +++ b/sequencer.h\n>> @@ -116,7 +116,6 @@ struct todo_list {\n>> \tstruct todo_item *items;\n>> \tint nr, alloc, current;\n>> \tint done_nr, total_nr;\n>> -\tstruct stat_data stat;\n> \n> Good riddance ;-)\n\nHear, hear!\n\n> Will queue. Thanks.\n> \n\n"}]}