Hi, On Mon, 12 Nov 2018, Johannes Schindelin via GitGitGadget wrote:
> In our tests with large repositories, we noticed a serious regression of the > performance of git rebase when using the built-in vs the shell script > version. It boils down to an incorrect conversion of a git checkout -q: > instead of using a twoway_merge as git checkout does, we used a oneway_merge > as git reset does. The latter, however, calls lstat() on all files listed in > the index, while the former essentially looks only at the files that are > different between the given two revisions. > > Let's reinstate the original behavior by introducing a flag to the > reset_head() function to indicate whether we want to emulate reset --hard > (in which case we use the oneway_merge, otherwise we use twoway_merge). > > Johannes Schindelin (3): > rebase: consolidate clean-up code before leaving reset_head() > rebase: prepare reset_head() for more flags > built-in rebase: reinstate `checkout -q` behavior where appropriate > > builtin/rebase.c | 79 ++++++++++++++++++++++++++++-------------------- > 1 file changed, 46 insertions(+), 33 deletions(-) I forgot to specify the changes vs v1: - More error paths are not consolidated via `goto leave_reset_head`. - The `desc` array is not initialized to all-zero, to avoid bogus addresses being passed to `free()`. - The `detach_head` and `reset_hard` parameters have been consolidated into a `flags` parameter. - The `reset_head()` function once again only initializes `head_oid` when needed. Sorry for the omission, Johannes > > > base-commit: 8858448bb49332d353febc078ce4a3abcc962efe > Published-As: > https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git > pr-72/dscho/builtin-rebase-perf-regression-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/72 > > Range-diff vs v1: > > 1: 64597fe827 ! 1: 28e24d98ab rebase: consolidate clean-up code before > leaving reset_head() > @@ -11,6 +11,33 @@ > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ > + if (switch_to_branch && !starts_with(switch_to_branch, "refs/")) > + BUG("Not a fully qualified branch: '%s'", > switch_to_branch); > + > +- if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) > +- return -1; > ++ if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) { > ++ ret = -1; > ++ goto leave_reset_head; > ++ } > + > + if (!oid) { > + if (get_oid("HEAD", &head_oid)) { > +- rollback_lock_file(&lock); > +- return error(_("could not determine HEAD > revision")); > ++ ret = error(_("could not determine HEAD > revision")); > ++ goto leave_reset_head; > + } > + oid = &head_oid; > + } > +@@ > + unpack_tree_opts.reset = 1; > + > + if (read_index_unmerged(the_repository->index) < 0) { > +- rollback_lock_file(&lock); > +- return error(_("could not read index")); > ++ ret = error(_("could not read index")); > ++ goto leave_reset_head; > } > > if (!fill_tree_descriptor(&desc, oid)) { > @@ -31,15 +58,17 @@ > } > > tree = parse_tree_indirect(oid); > -@@ > + prime_cache_tree(the_repository->index, tree); > > - if (write_locked_index(the_repository->index, &lock, > COMMIT_LOCK) < 0) > +- if (write_locked_index(the_repository->index, &lock, > COMMIT_LOCK) < 0) > ++ if (write_locked_index(the_repository->index, &lock, > COMMIT_LOCK) < 0) { > ret = error(_("could not write index")); > - free((void *)desc.buffer); > - > - if (ret) > +- > +- if (ret) > - return ret; > + goto leave_reset_head; > ++ } > > reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT); > strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : > "rebase"); > -: ---------- > 2: db963b2094 rebase: prepare reset_head() for more flags > 2: 070092b430 ! 3: a7360b856f built-in rebase: reinstate `checkout -q` > behavior where appropriate > @@ -20,15 +20,18 @@ > @@ > #define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION" > > + #define RESET_HEAD_DETACH (1<<0) > ++#define RESET_HEAD_HARD (1<<1) > + > static int reset_head(struct object_id *oid, const char *action, > -- const char *switch_to_branch, int detach_head, > -+ const char *switch_to_branch, > -+ int detach_head, int reset_hard, > + const char *switch_to_branch, unsigned flags, > const char *reflog_orig_head, const char > *reflog_head) > { > + unsigned detach_head = flags & RESET_HEAD_DETACH; > ++ unsigned reset_hard = flags & RESET_HEAD_HARD; > struct object_id head_oid; > - struct tree_desc desc; > -+ struct tree_desc desc[2]; > ++ struct tree_desc desc[2] = { { NULL }, { NULL } }; > struct lock_file lock = LOCK_INIT; > struct unpack_trees_options unpack_tree_opts; > struct tree *tree; > @@ -42,18 +45,18 @@ > if (switch_to_branch && !starts_with(switch_to_branch, "refs/")) > BUG("Not a fully qualified branch: '%s'", > switch_to_branch); > @@ > - if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) > - return -1; > + goto leave_reset_head; > + } > > - if (!oid) { > - if (get_oid("HEAD", &head_oid)) { > -- rollback_lock_file(&lock); > -- return error(_("could not determine HEAD > revision")); > +- ret = error(_("could not determine HEAD > revision")); > +- goto leave_reset_head; > - } > - oid = &head_oid; > -+ if (get_oid("HEAD", &head_oid)) { > -+ rollback_lock_file(&lock); > -+ return error(_("could not determine HEAD revision")); > ++ if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) { > ++ ret = error(_("could not determine HEAD revision")); > ++ goto leave_reset_head; > } > > + if (!oid) > @@ -70,7 +73,7 @@ > unpack_tree_opts.merge = 1; > if (!detach_head) > @@ > - return error(_("could not read index")); > + goto leave_reset_head; > } > > - if (!fill_tree_descriptor(&desc, oid)) { > @@ -104,7 +107,8 @@ > string_list_clear(&merge_rr, 1); > > - if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0) > -+ if (reset_head(NULL, "reset", NULL, 0, 1, NULL, NULL) < > 0) > ++ if (reset_head(NULL, "reset", NULL, RESET_HEAD_HARD, > ++ NULL, NULL) < 0) > die(_("could not discard worktree changes")); > if (read_basic_state(&options)) > exit(1); > @@ -113,7 +117,8 @@ > exit(1); > if (reset_head(&options.orig_head, "reset", > - options.head_name, 0, NULL, NULL) < 0) > -+ options.head_name, 0, 1, NULL, NULL) < 0) > ++ options.head_name, RESET_HEAD_HARD, > ++ NULL, NULL) < 0) > die(_("could not move back to %s"), > oid_to_hex(&options.orig_head)); > ret = finish_rebase(&options); > @@ -122,34 +127,7 @@ > printf(_("Created autostash: %s\n"), buf.buf); > if (reset_head(&head->object.oid, "reset > --hard", > - NULL, 0, NULL, NULL) < 0) > -+ NULL, 0, 1, NULL, NULL) < 0) > ++ NULL, RESET_HEAD_HARD, NULL, > NULL) < 0) > die(_("could not reset --hard")); > printf(_("HEAD is now at %s"), > find_unique_abbrev(&head->object.oid, > -@@ > - strbuf_addf(&buf, "rebase: checkout %s", > - options.switch_to); > - if (reset_head(&oid, "checkout", > -- options.head_name, 0, > -+ options.head_name, 0, 0, > - NULL, NULL) < 0) { > - ret = !!error(_("could not > switch to " > - "%s"), > -@@ > - "it...\n")); > - > - strbuf_addf(&msg, "rebase: checkout %s", options.onto_name); > -- if (reset_head(&options.onto->object.oid, "checkout", NULL, 1, > -+ if (reset_head(&options.onto->object.oid, "checkout", NULL, 1, > 0, > - NULL, msg.buf)) > - die(_("Could not detach HEAD")); > - strbuf_release(&msg); > -@@ > - strbuf_addf(&msg, "rebase finished: %s onto %s", > - options.head_name ? options.head_name : > "detached HEAD", > - oid_to_hex(&options.onto->object.oid)); > -- reset_head(NULL, "Fast-forwarded", options.head_name, 0, > -+ reset_head(NULL, "Fast-forwarded", options.head_name, > 0, 0, > - "HEAD", msg.buf); > - strbuf_release(&msg); > - ret = !!finish_rebase(&options); > > -- > gitgitgadget >
