{"thread":{"id":"49785","subject":"[PATCH 0/2] Fix built-in rebase perf regression","startedAt":"2018-11-09T09:34:19Z","lastAt":"2018-11-13T06:07:42Z","messageCount":18,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"362803","messageId":"pull.72.git.gitgitgadget@gmail.com","threadId":"49785","inReplyTo":null,"subject":"[PATCH 0/2] Fix built-in rebase perf regression","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-09T09:34:16Z","receivedAt":"2018-11-09T09:34:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In our tests with large repositories, we noticed a serious regression of the\nperformance of git rebase when using the built-in vs the shell script\nversion. It boils down to an incorrect conversion of a git checkout -q:\ninstead of using a twoway_merge as git checkout does, we used a oneway_merge \nas git reset does. The latter, however, calls lstat() on all files listed in\nthe index, while the former essentially looks only at the files that are\ndifferent between the given two revisions.\n\nLet's reinstate the original behavior by introducing a flag to the \nreset_head() function to indicate whether we want to emulate reset --hard \n(in which case we use the oneway_merge, otherwise we use twoway_merge).\n\nJohannes Schindelin (2):\n  rebase: consolidate clean-up code before leaving reset_head()\n  built-in rebase: reinstate `checkout -q` behavior where appropriate\n\n builtin/rebase.c | 60 ++++++++++++++++++++++++++----------------------\n 1 file changed, 33 insertions(+), 27 deletions(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-72/dscho/builtin-rebase-perf-regression-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/72\n-- \ngitgitgadget\n"},{"id":"362804","messageId":"64597fe827021383ab68cfb247de61fcf104a961.1541756054.git.gitgitgadget@gmail.com","threadId":"49785","inReplyTo":"pull.72.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] rebase: consolidate clean-up code before leaving reset_head()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-09T09:34:17Z","receivedAt":"2018-11-09T09:34:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe same clean-up code is repeated quite a few times; Let's DRY up the\ncode some.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..6f6d7de156 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -569,16 +569,13 @@ static int reset_head(struct object_id *oid, const char *action,\n \t}\n \n \tif (!fill_tree_descriptor(&desc, oid)) {\n-\t\terror(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\treturn -1;\n+\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n \t}\n \n \tif (unpack_trees(1, &desc, &unpack_tree_opts)) {\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto leave_reset_head;\n \t}\n \n \ttree = parse_tree_indirect(oid);\n@@ -586,10 +583,9 @@ static int reset_head(struct object_id *oid, const char *action,\n \n \tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n \t\tret = error(_(\"could not write index\"));\n-\tfree((void *)desc.buffer);\n \n \tif (ret)\n-\t\treturn ret;\n+\t\tgoto leave_reset_head;\n \n \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n@@ -622,7 +618,10 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \t}\n \n+leave_reset_head:\n \tstrbuf_release(&msg);\n+\trollback_lock_file(&lock);\n+\tfree((void *)desc.buffer);\n \treturn ret;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"362805","messageId":"070092b4309e5e74e3a1b3be54613cccf26e97da.1541756054.git.gitgitgadget@gmail.com","threadId":"49785","inReplyTo":"pull.72.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-09T09:34:19Z","receivedAt":"2018-11-09T09:34:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWhen we converted a `git checkout -q $onto^0` call to use\n`reset_head()`, we inadvertently incurred a change from a twoway_merge\nto a oneway_merge, as if we wanted a `git reset --hard` instead.\n\nThis has performance ramifications under certain, though, as the\noneway_merge needs to lstat() every single index entry whereas\ntwoway_merge does not.\n\nSo let's go back to the old behavior.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 45 ++++++++++++++++++++++++++-------------------\n 1 file changed, 26 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6f6d7de156..c1cc50f3f8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -523,11 +523,12 @@ finished_rebase:\n #define GIT_REFLOG_ACTION_ENVIRONMENT \"GIT_REFLOG_ACTION\"\n \n static int reset_head(struct object_id *oid, const char *action,\n-\t\t      const char *switch_to_branch, int detach_head,\n+\t\t      const char *switch_to_branch,\n+\t\t      int detach_head, int reset_hard,\n \t\t      const char *reflog_orig_head, const char *reflog_head)\n {\n \tstruct object_id head_oid;\n-\tstruct tree_desc desc;\n+\tstruct tree_desc desc[2];\n \tstruct lock_file lock = LOCK_INIT;\n \tstruct unpack_trees_options unpack_tree_opts;\n \tstruct tree *tree;\n@@ -536,7 +537,7 @@ static int reset_head(struct object_id *oid, const char *action,\n \tsize_t prefix_len;\n \tstruct object_id *orig = NULL, oid_orig,\n \t\t*old_orig = NULL, oid_old_orig;\n-\tint ret = 0;\n+\tint ret = 0, nr = 0;\n \n \tif (switch_to_branch && !starts_with(switch_to_branch, \"refs/\"))\n \t\tBUG(\"Not a fully qualified branch: '%s'\", switch_to_branch);\n@@ -544,20 +545,20 @@ static int reset_head(struct object_id *oid, const char *action,\n \tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n \n-\tif (!oid) {\n-\t\tif (get_oid(\"HEAD\", &head_oid)) {\n-\t\t\trollback_lock_file(&lock);\n-\t\t\treturn error(_(\"could not determine HEAD revision\"));\n-\t\t}\n-\t\toid = &head_oid;\n+\tif (get_oid(\"HEAD\", &head_oid)) {\n+\t\trollback_lock_file(&lock);\n+\t\treturn error(_(\"could not determine HEAD revision\"));\n \t}\n \n+\tif (!oid)\n+\t\toid = &head_oid;\n+\n \tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, action);\n \tunpack_tree_opts.head_idx = 1;\n \tunpack_tree_opts.src_index = the_repository->index;\n \tunpack_tree_opts.dst_index = the_repository->index;\n-\tunpack_tree_opts.fn = oneway_merge;\n+\tunpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.merge = 1;\n \tif (!detach_head)\n@@ -568,12 +569,17 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\treturn error(_(\"could not read index\"));\n \t}\n \n-\tif (!fill_tree_descriptor(&desc, oid)) {\n+\tif (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {\n+\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n+\tif (!fill_tree_descriptor(&desc[nr++], oid)) {\n \t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n \t\tgoto leave_reset_head;\n \t}\n \n-\tif (unpack_trees(1, &desc, &unpack_tree_opts)) {\n+\tif (unpack_trees(nr, desc, &unpack_tree_opts)) {\n \t\tret = -1;\n \t\tgoto leave_reset_head;\n \t}\n@@ -621,7 +627,8 @@ static int reset_head(struct object_id *oid, const char *action,\n leave_reset_head:\n \tstrbuf_release(&msg);\n \trollback_lock_file(&lock);\n-\tfree((void *)desc.buffer);\n+\twhile (nr)\n+\t\tfree((void *)desc[--nr].buffer);\n \treturn ret;\n }\n \n@@ -999,7 +1006,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\trerere_clear(&merge_rr);\n \t\tstring_list_clear(&merge_rr, 1);\n \n-\t\tif (reset_head(NULL, \"reset\", NULL, 0, NULL, NULL) < 0)\n+\t\tif (reset_head(NULL, \"reset\", NULL, 0, 1, NULL, NULL) < 0)\n \t\t\tdie(_(\"could not discard worktree changes\"));\n \t\tif (read_basic_state(&options))\n \t\t\texit(1);\n@@ -1015,7 +1022,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (read_basic_state(&options))\n \t\t\texit(1);\n \t\tif (reset_head(&options.orig_head, \"reset\",\n-\t\t\t       options.head_name, 0, NULL, NULL) < 0)\n+\t\t\t       options.head_name, 0, 1, NULL, NULL) < 0)\n \t\t\tdie(_(\"could not move back to %s\"),\n \t\t\t    oid_to_hex(&options.orig_head));\n \t\tret = finish_rebase(&options);\n@@ -1379,7 +1386,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n \t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n-\t\t\t\t       NULL, 0, NULL, NULL) < 0)\n+\t\t\t\t       NULL, 0, 1, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n \t\t\tprintf(_(\"HEAD is now at %s\"),\n \t\t\t       find_unique_abbrev(&head->object.oid,\n@@ -1433,7 +1440,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrbuf_addf(&buf, \"rebase: checkout %s\",\n \t\t\t\t\t    options.switch_to);\n \t\t\t\tif (reset_head(&oid, \"checkout\",\n-\t\t\t\t\t       options.head_name, 0,\n+\t\t\t\t\t       options.head_name, 0, 0,\n \t\t\t\t\t       NULL, NULL) < 0) {\n \t\t\t\t\tret = !!error(_(\"could not switch to \"\n \t\t\t\t\t\t\t\"%s\"),\n@@ -1499,7 +1506,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t \"it...\\n\"));\n \n \tstrbuf_addf(&msg, \"rebase: checkout %s\", options.onto_name);\n-\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n+\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1, 0,\n \t    NULL, msg.buf))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n@@ -1515,7 +1522,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n \t\t\toid_to_hex(&options.onto->object.oid));\n-\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n+\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0, 0,\n \t\t\t   \"HEAD\", msg.buf);\n \t\tstrbuf_release(&msg);\n \t\tret = !!finish_rebase(&options);\n-- \ngitgitgadget\n"},{"id":"362806","messageId":"20181109100357.GA7410@sigill.intra.peff.net","threadId":"49785","inReplyTo":"64597fe827021383ab68cfb247de61fcf104a961.1541756054.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] rebase: consolidate clean-up code before leaving reset_head()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-09T10:03:57Z","receivedAt":"2018-11-09T10:04:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 09, 2018 at 01:34:17AM -0800, Johannes Schindelin via GitGitGadget wrote:\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 0ee06aa363..6f6d7de156 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -569,16 +569,13 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \t}\n>  \n>  \tif (!fill_tree_descriptor(&desc, oid)) {\n> -\t\terror(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n> -\t\trollback_lock_file(&lock);\n> -\t\tfree((void *)desc.buffer);\n> -\t\treturn -1;\n> +\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n> +\t\tgoto leave_reset_head;\n>  \t}\n\nIf fill_tree_descriptor() fails, what is left in desc.buffer? Looking at\nthe implementation, I think it's always NULL or a valid buffer. But I\nthink all code paths actually die() unless we pass a NULL oid (and in\nthat case desc.buffer would be NULL, too).\n\nSo I think the original here that calls free() doesn't ever do anything\nbut it did not hurt. After your patch, the leave_reset_head code would\ncontinue to call free(), and that's OK.\n\nThere are a few earlier conditionals in reset_head() that do only\nrollback_lock_file() that could similarly be converted to use the goto.\nBut they would need desc.buffer to be initialized to NULL. I could go\neither way on converting them or not.\n\n> @@ -586,10 +583,9 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \n>  \tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n>  \t\tret = error(_(\"could not write index\"));\n> -\tfree((void *)desc.buffer);\n>  \n>  \tif (ret)\n> -\t\treturn ret;\n> +\t\tgoto leave_reset_head;\n>  \n>  \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n>  \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n> @@ -622,7 +618,10 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n>  \t}\n>  \n> +leave_reset_head:\n>  \tstrbuf_release(&msg);\n> +\trollback_lock_file(&lock);\n> +\tfree((void *)desc.buffer);\n>  \treturn ret;\n\nWe get here on success, too. So we may call rollback_lock_file() on an\nalready-committed lock. This is explicitly documented as a no-op by the\nlock code, so that's OK.\n\nSo overall looks good to me.\n\n-Peff\n"},{"id":"362807","messageId":"20181109101148.GB7410@sigill.intra.peff.net","threadId":"49785","inReplyTo":"070092b4309e5e74e3a1b3be54613cccf26e97da.1541756054.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-09T10:11:48Z","receivedAt":"2018-11-09T10:11:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 09, 2018 at 01:34:19AM -0800, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> When we converted a `git checkout -q $onto^0` call to use\n> `reset_head()`, we inadvertently incurred a change from a twoway_merge\n> to a oneway_merge, as if we wanted a `git reset --hard` instead.\n> \n> This has performance ramifications under certain, though, as the\n> oneway_merge needs to lstat() every single index entry whereas\n> twoway_merge does not.\n> \n> So let's go back to the old behavior.\n\nMakes sense. I didn't think too hard about any possible gotchas with the\ntwoway/oneway switch, but if that's what git-checkout was doing before,\nit seems obviously safe.\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 6f6d7de156..c1cc50f3f8 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -523,11 +523,12 @@ finished_rebase:\n>  #define GIT_REFLOG_ACTION_ENVIRONMENT \"GIT_REFLOG_ACTION\"\n>  \n>  static int reset_head(struct object_id *oid, const char *action,\n> -\t\t      const char *switch_to_branch, int detach_head,\n> +\t\t      const char *switch_to_branch,\n> +\t\t      int detach_head, int reset_hard,\n\nIt might be worth switching to a single flag variable here. It would\nmake calls like this:\n\n> -\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n> +\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1, 0,\n>  \t    NULL, msg.buf))\n\na little more self-documenting (if a little more verbose).\n\n> -\tif (!oid) {\n> -\t\tif (get_oid(\"HEAD\", &head_oid)) {\n> -\t\t\trollback_lock_file(&lock);\n> -\t\t\treturn error(_(\"could not determine HEAD revision\"));\n> -\t\t}\n> -\t\toid = &head_oid;\n> +\tif (get_oid(\"HEAD\", &head_oid)) {\n> +\t\trollback_lock_file(&lock);\n> +\t\treturn error(_(\"could not determine HEAD revision\"));\n>  \t}\n\nThis one could actually turn into:\n\n  ret = error(...);\n  goto leave_reset_head;\n\nnow. We don't have to worry about an uninitialized desc.buffer anymore\n(as I mentioned in the previous email), because \"nr\" would be 0.\n\nIt doesn't save any lines, though (but maybe having a single\ncleanup/exit point would make things easier to read; I dunno).\n\nTake all my comments as observations, not objections. This looks OK to\nme either way.\n\n-Peff\n"},{"id":"362833","messageId":"nycvar.QRO.7.76.6.1811091750330.39@tvgsbejvaqbjf.bet","threadId":"49785","inReplyTo":"20181109100357.GA7410@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] rebase: consolidate clean-up code before leaving reset_head()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-09T17:13:08Z","receivedAt":"2018-11-09T17:13:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 9 Nov 2018, Jeff King wrote:\n\n> On Fri, Nov 09, 2018 at 01:34:17AM -0800, Johannes Schindelin via GitGitGadget wrote:\n> \n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 0ee06aa363..6f6d7de156 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -569,16 +569,13 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  \t}\n> >  \n> >  \tif (!fill_tree_descriptor(&desc, oid)) {\n> > -\t\terror(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n> > -\t\trollback_lock_file(&lock);\n> > -\t\tfree((void *)desc.buffer);\n> > -\t\treturn -1;\n> > +\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n> > +\t\tgoto leave_reset_head;\n> >  \t}\n> \n> If fill_tree_descriptor() fails, what is left in desc.buffer? Looking at\n> the implementation, I think it's always NULL or a valid buffer. But I\n> think all code paths actually die() unless we pass a NULL oid (and in\n> that case desc.buffer would be NULL, too).\n> \n> So I think the original here that calls free() doesn't ever do anything\n> but it did not hurt. After your patch, the leave_reset_head code would\n> continue to call free(), and that's OK.\n\nRight, that was my thinking, too.\n\n> There are a few earlier conditionals in reset_head() that do only\n> rollback_lock_file() that could similarly be converted to use the goto.\n> But they would need desc.buffer to be initialized to NULL. I could go\n> either way on converting them or not.\n\nWhoops. I should have checked more carefully?\n\n> > @@ -586,10 +583,9 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  \n> >  \tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n> >  \t\tret = error(_(\"could not write index\"));\n> > -\tfree((void *)desc.buffer);\n> >  \n> >  \tif (ret)\n> > -\t\treturn ret;\n> > +\t\tgoto leave_reset_head;\n> >  \n> >  \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n> >  \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n> > @@ -622,7 +618,10 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  \t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n> >  \t}\n> >  \n> > +leave_reset_head:\n> >  \tstrbuf_release(&msg);\n> > +\trollback_lock_file(&lock);\n> > +\tfree((void *)desc.buffer);\n> >  \treturn ret;\n> \n> We get here on success, too. So we may call rollback_lock_file() on an\n> already-committed lock. This is explicitly documented as a no-op by the\n> lock code, so that's OK.\n\nIndeed. I did not check the documentation, but the code, and came to the\nsame conclusion.\n\n> So overall looks good to me.\n\nThanks!\nDscho\n"},{"id":"362834","messageId":"nycvar.QRO.7.76.6.1811091813140.39@tvgsbejvaqbjf.bet","threadId":"49785","inReplyTo":"20181109101148.GB7410@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-09T17:21:41Z","receivedAt":"2018-11-09T17:21:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 9 Nov 2018, Jeff King wrote:\n\n> On Fri, Nov 09, 2018 at 01:34:19AM -0800, Johannes Schindelin via GitGitGadget wrote:\n> \n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > \n> > When we converted a `git checkout -q $onto^0` call to use\n> > `reset_head()`, we inadvertently incurred a change from a twoway_merge\n> > to a oneway_merge, as if we wanted a `git reset --hard` instead.\n> > \n> > This has performance ramifications under certain, though, as the\n> > oneway_merge needs to lstat() every single index entry whereas\n> > twoway_merge does not.\n> > \n> > So let's go back to the old behavior.\n> \n> Makes sense. I didn't think too hard about any possible gotchas with the\n> twoway/oneway switch, but if that's what git-checkout was doing before,\n> it seems obviously safe.\n\nRight.\n\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 6f6d7de156..c1cc50f3f8 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -523,11 +523,12 @@ finished_rebase:\n> >  #define GIT_REFLOG_ACTION_ENVIRONMENT \"GIT_REFLOG_ACTION\"\n> >  \n> >  static int reset_head(struct object_id *oid, const char *action,\n> > -\t\t      const char *switch_to_branch, int detach_head,\n> > +\t\t      const char *switch_to_branch,\n> > +\t\t      int detach_head, int reset_hard,\n> \n> It might be worth switching to a single flag variable here. It would\n> make calls like this:\n> \n> > -\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n> > +\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1, 0,\n> >  \t    NULL, msg.buf))\n> \n> a little more self-documenting (if a little more verbose).\n\nI thought about that, but for just two flags? Well, let me try it and see\nwhether I like the result ;-)\n\n> > -\tif (!oid) {\n> > -\t\tif (get_oid(\"HEAD\", &head_oid)) {\n> > -\t\t\trollback_lock_file(&lock);\n> > -\t\t\treturn error(_(\"could not determine HEAD revision\"));\n> > -\t\t}\n> > -\t\toid = &head_oid;\n> > +\tif (get_oid(\"HEAD\", &head_oid)) {\n> > +\t\trollback_lock_file(&lock);\n> > +\t\treturn error(_(\"could not determine HEAD revision\"));\n> >  \t}\n> \n> This one could actually turn into:\n> \n>   ret = error(...);\n>   goto leave_reset_head;\n> \n> now. We don't have to worry about an uninitialized desc.buffer anymore\n> (as I mentioned in the previous email), because \"nr\" would be 0.\n> \n> It doesn't save any lines, though (but maybe having a single\n> cleanup/exit point would make things easier to read; I dunno).\n\nBut you're right, of course. Consistency makes for easier-to-read code.\n\n> Take all my comments as observations, not objections. This looks OK to\n> me either way.\n\nActually, you got me thinking about the desc.buffer. And I think there is\none corner case where it could cause a problem: `struct tree_desc desc[2]`\ndoes not initialize the buffers to NULL. And what if\nfill_tree_descriptor() function returns NULL? Then the buffer is still\nuninitialized.\n\nIn practice, our *current* version of fill_tree_descriptor() never returns\nNULL if the oid parameter is non-NULL. It would die() in the error case\ninstead (bad!). So to prepare for a less bad version, I'd rather\ninitialize the `desc` array and be on the safe (and easier to reason\nabout) side.\n\nThank you for your helpful review,\nDscho\n"},{"id":"362837","messageId":"20181109195934.GC4938@sigill.intra.peff.net","threadId":"49785","inReplyTo":"nycvar.QRO.7.76.6.1811091813140.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-09T19:59:34Z","receivedAt":"2018-11-09T19:59:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 09, 2018 at 06:21:41PM +0100, Johannes Schindelin wrote:\n\n> Actually, you got me thinking about the desc.buffer. And I think there is\n> one corner case where it could cause a problem: `struct tree_desc desc[2]`\n> does not initialize the buffers to NULL. And what if\n> fill_tree_descriptor() function returns NULL? Then the buffer is still\n> uninitialized.\n> \n> In practice, our *current* version of fill_tree_descriptor() never returns\n> NULL if the oid parameter is non-NULL. It would die() in the error case\n> instead (bad!). So to prepare for a less bad version, I'd rather\n> initialize the `desc` array and be on the safe (and easier to reason\n> about) side.\n\nYeah, I agree with all of that.\n\nOne solution would just be to increment only after success:\n\n  if (fill_tree_descriptor(&desc[nr], ..) < 0)\n\tgoto error;\n  nr++; /* now we know it's valid! */\n\nBut there are lots of alternatives.  :)\n\n-Peff\n"},{"id":"362960","messageId":"xmqq1s7r3pqy.fsf@gitster-ct.c.googlers.com","threadId":"49785","inReplyTo":"nycvar.QRO.7.76.6.1811091813140.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T04:04:53Z","receivedAt":"2018-11-12T04:05:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> >  static int reset_head(struct object_id *oid, const char *action,\n>> > -\t\t      const char *switch_to_branch, int detach_head,\n>> > +\t\t      const char *switch_to_branch,\n>> > +\t\t      int detach_head, int reset_hard,\n>> \n>> It might be worth switching to a single flag variable here. It would\n>> make calls like this:\n>> \n>> > -\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n>> > +\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1, 0,\n>> >  \t    NULL, msg.buf))\n>> \n>> a little more self-documenting (if a little more verbose).\n>\n> I thought about that, but for just two flags? Well, let me try it and see\n> whether I like the result ;-)\n\nMy rule of thumb is that repeating three times is definitely when we\nshould consolidate separate ones into a single flag word, and twice\nis a borderline.\n\nFor two-time repetition, it is often worth fixing when somebody\nnotices it during the review, as that is a sign that repetition,\neven a minor one, disturbed a reader enough to point out.  On the\nother hand, for a file-scope static that is likely to stay as a\nnon-API function but a local helper, it may not be worth it.\n\nSo I am OK either way, slightly in favor of fixing it while we\nremember.\n\n\n>> This one could actually turn into:\n>> \n>>   ret = error(...);\n>>   goto leave_reset_head;\n>> \n>> now. We don't have to worry about an uninitialized desc.buffer anymore\n>> (as I mentioned in the previous email), because \"nr\" would be 0.\n>> \n>> It doesn't save any lines, though (but maybe having a single\n>> cleanup/exit point would make things easier to read; I dunno).\n>\n> But you're right, of course. Consistency makes for easier-to-read code.\n\nYup, that does sound good.\n\nThanks.\n"},{"id":"362987","messageId":"nycvar.QRO.7.76.6.1811121211040.39@tvgsbejvaqbjf.bet","threadId":"49785","inReplyTo":"20181109195934.GC4938@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-12T11:11:36Z","receivedAt":"2018-11-12T11:11:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 9 Nov 2018, Jeff King wrote:\n\n> On Fri, Nov 09, 2018 at 06:21:41PM +0100, Johannes Schindelin wrote:\n> \n> > Actually, you got me thinking about the desc.buffer. And I think there is\n> > one corner case where it could cause a problem: `struct tree_desc desc[2]`\n> > does not initialize the buffers to NULL. And what if\n> > fill_tree_descriptor() function returns NULL? Then the buffer is still\n> > uninitialized.\n> > \n> > In practice, our *current* version of fill_tree_descriptor() never returns\n> > NULL if the oid parameter is non-NULL. It would die() in the error case\n> > instead (bad!). So to prepare for a less bad version, I'd rather\n> > initialize the `desc` array and be on the safe (and easier to reason\n> > about) side.\n> \n> Yeah, I agree with all of that.\n> \n> One solution would just be to increment only after success:\n> \n>   if (fill_tree_descriptor(&desc[nr], ..) < 0)\n> \tgoto error;\n>   nr++; /* now we know it's valid! */\n> \n> But there are lots of alternatives.  :)\n\nTrue. I simply prefer to initialize it and be done with it. ;-)\n\nCiao,\nDscho\n"},{"id":"362989","messageId":"nycvar.QRO.7.76.6.1811121235120.39@tvgsbejvaqbjf.bet","threadId":"49785","inReplyTo":"xmqq1s7r3pqy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-12T11:42:45Z","receivedAt":"2018-11-12T11:43:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 12 Nov 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> >  static int reset_head(struct object_id *oid, const char *action,\n> >> > -\t\t      const char *switch_to_branch, int detach_head,\n> >> > +\t\t      const char *switch_to_branch,\n> >> > +\t\t      int detach_head, int reset_hard,\n> >> \n> >> It might be worth switching to a single flag variable here. It would\n> >> make calls like this:\n> >> \n> >> > -\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n> >> > +\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1, 0,\n> >> >  \t    NULL, msg.buf))\n> >> \n> >> a little more self-documenting (if a little more verbose).\n> >\n> > I thought about that, but for just two flags? Well, let me try it and see\n> > whether I like the result ;-)\n> \n> My rule of thumb is that repeating three times is definitely when we\n> should consolidate separate ones into a single flag word, and twice\n> is a borderline.\n> \n> For two-time repetition, it is often worth fixing when somebody\n> notices it during the review, as that is a sign that repetition,\n> even a minor one, disturbed a reader enough to point out.\n\nThat's my thought exactly, hence I looked into it. The end result is\nactually easier to read, in particular the commit that re-introduces the\n`reset --hard` behavior: it no longer touches *all* callsites of\n`reset_head()` but only the relevant ones.\n\n> On the other hand, for a file-scope static that is likely to stay as a\n> non-API function but a local helper, it may not be worth it.\n\nOh, do you think that `reset_head()` is likely to stay as non-API\nfunction? I found myself in the need of repeating this tedious\nunpack_trees() dance quite a few times over the past two years, and it is\n*always* daunting because the API is *that* unintuitive.\n\nSo I *do* hope that `reset_head()` will actually be moved to reset.[ch]\neventually, and callsites e.g. in `sequencer.c` will be converted from\ncalling `unpack_trees()` to calling `reset_head()` instead.\n\nv2 on the way,\nDscho\n\n> So I am OK either way, slightly in favor of fixing it while we\n> remember.\n> \n> \n> >> This one could actually turn into:\n> >> \n> >>   ret = error(...);\n> >>   goto leave_reset_head;\n> >> \n> >> now. We don't have to worry about an uninitialized desc.buffer anymore\n> >> (as I mentioned in the previous email), because \"nr\" would be 0.\n> >> \n> >> It doesn't save any lines, though (but maybe having a single\n> >> cleanup/exit point would make things easier to read; I dunno).\n> >\n> > But you're right, of course. Consistency makes for easier-to-read code.\n> \n> Yup, that does sound good.\n> \n> Thanks.\n> \n"},{"id":"362990","messageId":"pull.72.v2.git.gitgitgadget@gmail.com","threadId":"49785","inReplyTo":"pull.72.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix built-in rebase perf regression","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T11:44:28Z","receivedAt":"2018-11-12T11:44:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In our tests with large repositories, we noticed a serious regression of the\nperformance of git rebase when using the built-in vs the shell script\nversion. It boils down to an incorrect conversion of a git checkout -q:\ninstead of using a twoway_merge as git checkout does, we used a oneway_merge \nas git reset does. The latter, however, calls lstat() on all files listed in\nthe index, while the former essentially looks only at the files that are\ndifferent between the given two revisions.\n\nLet's reinstate the original behavior by introducing a flag to the \nreset_head() function to indicate whether we want to emulate reset --hard \n(in which case we use the oneway_merge, otherwise we use twoway_merge).\n\nJohannes Schindelin (3):\n  rebase: consolidate clean-up code before leaving reset_head()\n  rebase: prepare reset_head() for more flags\n  built-in rebase: reinstate `checkout -q` behavior where appropriate\n\n builtin/rebase.c | 79 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 46 insertions(+), 33 deletions(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-72/dscho/builtin-rebase-perf-regression-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/72\n\nRange-diff vs v1:\n\n 1:  64597fe827 ! 1:  28e24d98ab rebase: consolidate clean-up code before leaving reset_head()\n     @@ -11,6 +11,33 @@\n      --- a/builtin/rebase.c\n      +++ b/builtin/rebase.c\n      @@\n     + \tif (switch_to_branch && !starts_with(switch_to_branch, \"refs/\"))\n     + \t\tBUG(\"Not a fully qualified branch: '%s'\", switch_to_branch);\n     + \n     +-\tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)\n     +-\t\treturn -1;\n     ++\tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n     ++\t\tret = -1;\n     ++\t\tgoto leave_reset_head;\n     ++\t}\n     + \n     + \tif (!oid) {\n     + \t\tif (get_oid(\"HEAD\", &head_oid)) {\n     +-\t\t\trollback_lock_file(&lock);\n     +-\t\t\treturn error(_(\"could not determine HEAD revision\"));\n     ++\t\t\tret = error(_(\"could not determine HEAD revision\"));\n     ++\t\t\tgoto leave_reset_head;\n     + \t\t}\n     + \t\toid = &head_oid;\n     + \t}\n     +@@\n     + \t\tunpack_tree_opts.reset = 1;\n     + \n     + \tif (read_index_unmerged(the_repository->index) < 0) {\n     +-\t\trollback_lock_file(&lock);\n     +-\t\treturn error(_(\"could not read index\"));\n     ++\t\tret = error(_(\"could not read index\"));\n     ++\t\tgoto leave_reset_head;\n       \t}\n       \n       \tif (!fill_tree_descriptor(&desc, oid)) {\n     @@ -31,15 +58,17 @@\n       \t}\n       \n       \ttree = parse_tree_indirect(oid);\n     -@@\n     + \tprime_cache_tree(the_repository->index, tree);\n       \n     - \tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n     +-\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n     ++\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {\n       \t\tret = error(_(\"could not write index\"));\n      -\tfree((void *)desc.buffer);\n     - \n     - \tif (ret)\n     +-\n     +-\tif (ret)\n      -\t\treturn ret;\n      +\t\tgoto leave_reset_head;\n     ++\t}\n       \n       \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n       \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n -:  ---------- > 2:  db963b2094 rebase: prepare reset_head() for more flags\n 2:  070092b430 ! 3:  a7360b856f built-in rebase: reinstate `checkout -q` behavior where appropriate\n     @@ -20,15 +20,18 @@\n      @@\n       #define GIT_REFLOG_ACTION_ENVIRONMENT \"GIT_REFLOG_ACTION\"\n       \n     + #define RESET_HEAD_DETACH (1<<0)\n     ++#define RESET_HEAD_HARD (1<<1)\n     + \n       static int reset_head(struct object_id *oid, const char *action,\n     --\t\t      const char *switch_to_branch, int detach_head,\n     -+\t\t      const char *switch_to_branch,\n     -+\t\t      int detach_head, int reset_hard,\n     + \t\t      const char *switch_to_branch, unsigned flags,\n       \t\t      const char *reflog_orig_head, const char *reflog_head)\n       {\n     + \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n     ++\tunsigned reset_hard = flags & RESET_HEAD_HARD;\n       \tstruct object_id head_oid;\n      -\tstruct tree_desc desc;\n     -+\tstruct tree_desc desc[2];\n     ++\tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n       \tstruct lock_file lock = LOCK_INIT;\n       \tstruct unpack_trees_options unpack_tree_opts;\n       \tstruct tree *tree;\n     @@ -42,18 +45,18 @@\n       \tif (switch_to_branch && !starts_with(switch_to_branch, \"refs/\"))\n       \t\tBUG(\"Not a fully qualified branch: '%s'\", switch_to_branch);\n      @@\n     - \tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)\n     - \t\treturn -1;\n     + \t\tgoto leave_reset_head;\n     + \t}\n       \n      -\tif (!oid) {\n      -\t\tif (get_oid(\"HEAD\", &head_oid)) {\n     --\t\t\trollback_lock_file(&lock);\n     --\t\t\treturn error(_(\"could not determine HEAD revision\"));\n     +-\t\t\tret = error(_(\"could not determine HEAD revision\"));\n     +-\t\t\tgoto leave_reset_head;\n      -\t\t}\n      -\t\toid = &head_oid;\n     -+\tif (get_oid(\"HEAD\", &head_oid)) {\n     -+\t\trollback_lock_file(&lock);\n     -+\t\treturn error(_(\"could not determine HEAD revision\"));\n     ++\tif ((!oid || !reset_hard) && get_oid(\"HEAD\", &head_oid)) {\n     ++\t\tret = error(_(\"could not determine HEAD revision\"));\n     ++\t\tgoto leave_reset_head;\n       \t}\n       \n      +\tif (!oid)\n     @@ -70,7 +73,7 @@\n       \tunpack_tree_opts.merge = 1;\n       \tif (!detach_head)\n      @@\n     - \t\treturn error(_(\"could not read index\"));\n     + \t\tgoto leave_reset_head;\n       \t}\n       \n      -\tif (!fill_tree_descriptor(&desc, oid)) {\n     @@ -104,7 +107,8 @@\n       \t\tstring_list_clear(&merge_rr, 1);\n       \n      -\t\tif (reset_head(NULL, \"reset\", NULL, 0, NULL, NULL) < 0)\n     -+\t\tif (reset_head(NULL, \"reset\", NULL, 0, 1, NULL, NULL) < 0)\n     ++\t\tif (reset_head(NULL, \"reset\", NULL, RESET_HEAD_HARD,\n     ++\t\t\t       NULL, NULL) < 0)\n       \t\t\tdie(_(\"could not discard worktree changes\"));\n       \t\tif (read_basic_state(&options))\n       \t\t\texit(1);\n     @@ -113,7 +117,8 @@\n       \t\t\texit(1);\n       \t\tif (reset_head(&options.orig_head, \"reset\",\n      -\t\t\t       options.head_name, 0, NULL, NULL) < 0)\n     -+\t\t\t       options.head_name, 0, 1, NULL, NULL) < 0)\n     ++\t\t\t       options.head_name, RESET_HEAD_HARD,\n     ++\t\t\t       NULL, NULL) < 0)\n       \t\t\tdie(_(\"could not move back to %s\"),\n       \t\t\t    oid_to_hex(&options.orig_head));\n       \t\tret = finish_rebase(&options);\n     @@ -122,34 +127,7 @@\n       \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n       \t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n      -\t\t\t\t       NULL, 0, NULL, NULL) < 0)\n     -+\t\t\t\t       NULL, 0, 1, NULL, NULL) < 0)\n     ++\t\t\t\t       NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n       \t\t\t\tdie(_(\"could not reset --hard\"));\n       \t\t\tprintf(_(\"HEAD is now at %s\"),\n       \t\t\t       find_unique_abbrev(&head->object.oid,\n     -@@\n     - \t\t\t\tstrbuf_addf(&buf, \"rebase: checkout %s\",\n     - \t\t\t\t\t    options.switch_to);\n     - \t\t\t\tif (reset_head(&oid, \"checkout\",\n     --\t\t\t\t\t       options.head_name, 0,\n     -+\t\t\t\t\t       options.head_name, 0, 0,\n     - \t\t\t\t\t       NULL, NULL) < 0) {\n     - \t\t\t\t\tret = !!error(_(\"could not switch to \"\n     - \t\t\t\t\t\t\t\"%s\"),\n     -@@\n     - \t\t\t \"it...\\n\"));\n     - \n     - \tstrbuf_addf(&msg, \"rebase: checkout %s\", options.onto_name);\n     --\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n     -+\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1, 0,\n     - \t    NULL, msg.buf))\n     - \t\tdie(_(\"Could not detach HEAD\"));\n     - \tstrbuf_release(&msg);\n     -@@\n     - \t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n     - \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n     - \t\t\toid_to_hex(&options.onto->object.oid));\n     --\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n     -+\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0, 0,\n     - \t\t\t   \"HEAD\", msg.buf);\n     - \t\tstrbuf_release(&msg);\n     - \t\tret = !!finish_rebase(&options);\n\n-- \ngitgitgadget\n"},{"id":"362991","messageId":"28e24d98abad95996b78f346a505171bccfdf547.1542023066.git.gitgitgadget@gmail.com","threadId":"49785","inReplyTo":"pull.72.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] rebase: consolidate clean-up code before leaving reset_head()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T11:44:30Z","receivedAt":"2018-11-12T11:44:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe same clean-up code is repeated quite a few times; Let's DRY up the\ncode some.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..e173654d56 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -541,13 +541,15 @@ static int reset_head(struct object_id *oid, const char *action,\n \tif (switch_to_branch && !starts_with(switch_to_branch, \"refs/\"))\n \t\tBUG(\"Not a fully qualified branch: '%s'\", switch_to_branch);\n \n-\tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)\n-\t\treturn -1;\n+\tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n+\t\tret = -1;\n+\t\tgoto leave_reset_head;\n+\t}\n \n \tif (!oid) {\n \t\tif (get_oid(\"HEAD\", &head_oid)) {\n-\t\t\trollback_lock_file(&lock);\n-\t\t\treturn error(_(\"could not determine HEAD revision\"));\n+\t\t\tret = error(_(\"could not determine HEAD revision\"));\n+\t\t\tgoto leave_reset_head;\n \t\t}\n \t\toid = &head_oid;\n \t}\n@@ -564,32 +566,27 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\tunpack_tree_opts.reset = 1;\n \n \tif (read_index_unmerged(the_repository->index) < 0) {\n-\t\trollback_lock_file(&lock);\n-\t\treturn error(_(\"could not read index\"));\n+\t\tret = error(_(\"could not read index\"));\n+\t\tgoto leave_reset_head;\n \t}\n \n \tif (!fill_tree_descriptor(&desc, oid)) {\n-\t\terror(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\treturn -1;\n+\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n \t}\n \n \tif (unpack_trees(1, &desc, &unpack_tree_opts)) {\n-\t\trollback_lock_file(&lock);\n-\t\tfree((void *)desc.buffer);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto leave_reset_head;\n \t}\n \n \ttree = parse_tree_indirect(oid);\n \tprime_cache_tree(the_repository->index, tree);\n \n-\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n+\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {\n \t\tret = error(_(\"could not write index\"));\n-\tfree((void *)desc.buffer);\n-\n-\tif (ret)\n-\t\treturn ret;\n+\t\tgoto leave_reset_head;\n+\t}\n \n \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n@@ -622,7 +619,10 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \t}\n \n+leave_reset_head:\n \tstrbuf_release(&msg);\n+\trollback_lock_file(&lock);\n+\tfree((void *)desc.buffer);\n \treturn ret;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"362992","messageId":"db963b20941b524199e51b9da0b59269db57c860.1542023066.git.gitgitgadget@gmail.com","threadId":"49785","inReplyTo":"pull.72.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] rebase: prepare reset_head() for more flags","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T11:44:31Z","receivedAt":"2018-11-12T11:44:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nCurrently, we only accept the flag indicating whether the HEAD should be\ndetached not. In the next commit, we want to introduce another flag: to\ntoggle between emulating `reset --hard` vs `checkout -q`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e173654d56..074594cf10 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -522,10 +522,13 @@ finished_rebase:\n \n #define GIT_REFLOG_ACTION_ENVIRONMENT \"GIT_REFLOG_ACTION\"\n \n+#define RESET_HEAD_DETACH (1<<0)\n+\n static int reset_head(struct object_id *oid, const char *action,\n-\t\t      const char *switch_to_branch, int detach_head,\n+\t\t      const char *switch_to_branch, unsigned flags,\n \t\t      const char *reflog_orig_head, const char *reflog_head)\n {\n+\tunsigned detach_head = flags & RESET_HEAD_DETACH;\n \tstruct object_id head_oid;\n \tstruct tree_desc desc;\n \tstruct lock_file lock = LOCK_INIT;\n@@ -1500,8 +1503,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t \"it...\\n\"));\n \n \tstrbuf_addf(&msg, \"rebase: checkout %s\", options.onto_name);\n-\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n-\t    NULL, msg.buf))\n+\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL,\n+\t\t       RESET_HEAD_DETACH, NULL, msg.buf))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n \n-- \ngitgitgadget\n\n"},{"id":"362993","messageId":"a7360b856f05169bad094c60e92571035e9d96a7.1542023067.git.gitgitgadget@gmail.com","threadId":"49785","inReplyTo":"pull.72.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T11:44:32Z","receivedAt":"2018-11-12T11:44:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWhen we converted a `git checkout -q $onto^0` call to use\n`reset_head()`, we inadvertently incurred a change from a twoway_merge\nto a oneway_merge, as if we wanted a `git reset --hard` instead.\n\nThis has performance ramifications under certain, though, as the\noneway_merge needs to lstat() every single index entry whereas\ntwoway_merge does not.\n\nSo let's go back to the old behavior.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 40 +++++++++++++++++++++++++---------------\n 1 file changed, 25 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 074594cf10..dc78c1497d 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -523,14 +523,16 @@ finished_rebase:\n #define GIT_REFLOG_ACTION_ENVIRONMENT \"GIT_REFLOG_ACTION\"\n \n #define RESET_HEAD_DETACH (1<<0)\n+#define RESET_HEAD_HARD (1<<1)\n \n static int reset_head(struct object_id *oid, const char *action,\n \t\t      const char *switch_to_branch, unsigned flags,\n \t\t      const char *reflog_orig_head, const char *reflog_head)\n {\n \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n+\tunsigned reset_hard = flags & RESET_HEAD_HARD;\n \tstruct object_id head_oid;\n-\tstruct tree_desc desc;\n+\tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n \tstruct unpack_trees_options unpack_tree_opts;\n \tstruct tree *tree;\n@@ -539,7 +541,7 @@ static int reset_head(struct object_id *oid, const char *action,\n \tsize_t prefix_len;\n \tstruct object_id *orig = NULL, oid_orig,\n \t\t*old_orig = NULL, oid_old_orig;\n-\tint ret = 0;\n+\tint ret = 0, nr = 0;\n \n \tif (switch_to_branch && !starts_with(switch_to_branch, \"refs/\"))\n \t\tBUG(\"Not a fully qualified branch: '%s'\", switch_to_branch);\n@@ -549,20 +551,20 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\tgoto leave_reset_head;\n \t}\n \n-\tif (!oid) {\n-\t\tif (get_oid(\"HEAD\", &head_oid)) {\n-\t\t\tret = error(_(\"could not determine HEAD revision\"));\n-\t\t\tgoto leave_reset_head;\n-\t\t}\n-\t\toid = &head_oid;\n+\tif ((!oid || !reset_hard) && get_oid(\"HEAD\", &head_oid)) {\n+\t\tret = error(_(\"could not determine HEAD revision\"));\n+\t\tgoto leave_reset_head;\n \t}\n \n+\tif (!oid)\n+\t\toid = &head_oid;\n+\n \tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, action);\n \tunpack_tree_opts.head_idx = 1;\n \tunpack_tree_opts.src_index = the_repository->index;\n \tunpack_tree_opts.dst_index = the_repository->index;\n-\tunpack_tree_opts.fn = oneway_merge;\n+\tunpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.merge = 1;\n \tif (!detach_head)\n@@ -573,12 +575,17 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\tgoto leave_reset_head;\n \t}\n \n-\tif (!fill_tree_descriptor(&desc, oid)) {\n+\tif (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {\n+\t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n+\tif (!fill_tree_descriptor(&desc[nr++], oid)) {\n \t\tret = error(_(\"failed to find tree of %s\"), oid_to_hex(oid));\n \t\tgoto leave_reset_head;\n \t}\n \n-\tif (unpack_trees(1, &desc, &unpack_tree_opts)) {\n+\tif (unpack_trees(nr, desc, &unpack_tree_opts)) {\n \t\tret = -1;\n \t\tgoto leave_reset_head;\n \t}\n@@ -625,7 +632,8 @@ static int reset_head(struct object_id *oid, const char *action,\n leave_reset_head:\n \tstrbuf_release(&msg);\n \trollback_lock_file(&lock);\n-\tfree((void *)desc.buffer);\n+\twhile (nr)\n+\t\tfree((void *)desc[--nr].buffer);\n \treturn ret;\n }\n \n@@ -1003,7 +1011,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\trerere_clear(&merge_rr);\n \t\tstring_list_clear(&merge_rr, 1);\n \n-\t\tif (reset_head(NULL, \"reset\", NULL, 0, NULL, NULL) < 0)\n+\t\tif (reset_head(NULL, \"reset\", NULL, RESET_HEAD_HARD,\n+\t\t\t       NULL, NULL) < 0)\n \t\t\tdie(_(\"could not discard worktree changes\"));\n \t\tif (read_basic_state(&options))\n \t\t\texit(1);\n@@ -1019,7 +1028,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (read_basic_state(&options))\n \t\t\texit(1);\n \t\tif (reset_head(&options.orig_head, \"reset\",\n-\t\t\t       options.head_name, 0, NULL, NULL) < 0)\n+\t\t\t       options.head_name, RESET_HEAD_HARD,\n+\t\t\t       NULL, NULL) < 0)\n \t\t\tdie(_(\"could not move back to %s\"),\n \t\t\t    oid_to_hex(&options.orig_head));\n \t\tret = finish_rebase(&options);\n@@ -1383,7 +1393,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n \t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n-\t\t\t\t       NULL, 0, NULL, NULL) < 0)\n+\t\t\t\t       NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n \t\t\tprintf(_(\"HEAD is now at %s\"),\n \t\t\t       find_unique_abbrev(&head->object.oid,\n-- \ngitgitgadget\n"},{"id":"363024","messageId":"nycvar.QRO.7.76.6.1811121521300.39@tvgsbejvaqbjf.bet","threadId":"49785","inReplyTo":"pull.72.v2.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Fix built-in rebase perf regression","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-12T14:26:30Z","receivedAt":"2018-11-12T14:26:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Nov 2018, Johannes Schindelin via GitGitGadget wrote:\n\n> In our tests with large repositories, we noticed a serious regression of the\n> performance of git rebase when using the built-in vs the shell script\n> version. It boils down to an incorrect conversion of a git checkout -q:\n> instead of using a twoway_merge as git checkout does, we used a oneway_merge \n> as git reset does. The latter, however, calls lstat() on all files listed in\n> the index, while the former essentially looks only at the files that are\n> different between the given two revisions.\n> \n> Let's reinstate the original behavior by introducing a flag to the \n> reset_head() function to indicate whether we want to emulate reset --hard \n> (in which case we use the oneway_merge, otherwise we use twoway_merge).\n> \n> Johannes Schindelin (3):\n>   rebase: consolidate clean-up code before leaving reset_head()\n>   rebase: prepare reset_head() for more flags\n>   built-in rebase: reinstate `checkout -q` behavior where appropriate\n> \n>  builtin/rebase.c | 79 ++++++++++++++++++++++++++++--------------------\n>  1 file changed, 46 insertions(+), 33 deletions(-)\n\nI forgot to specify the changes vs v1:\n\n- More error paths are not consolidated via `goto leave_reset_head`.\n- The `desc` array is not initialized to all-zero, to avoid bogus\n  addresses being passed to `free()`.\n- The `detach_head` and `reset_hard` parameters have been consolidated\n  into a `flags` parameter.\n- The `reset_head()` function once again only initializes `head_oid`\n  when needed.\n\nSorry for the omission,\nJohannes\n\n> \n> \n> base-commit: 8858448bb49332d353febc078ce4a3abcc962efe\n> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-72/dscho/builtin-rebase-perf-regression-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/72\n> \n> Range-diff vs v1:\n> \n>  1:  64597fe827 ! 1:  28e24d98ab rebase: consolidate clean-up code before leaving reset_head()\n>      @@ -11,6 +11,33 @@\n>       --- a/builtin/rebase.c\n>       +++ b/builtin/rebase.c\n>       @@\n>      + \tif (switch_to_branch && !starts_with(switch_to_branch, \"refs/\"))\n>      + \t\tBUG(\"Not a fully qualified branch: '%s'\", switch_to_branch);\n>      + \n>      +-\tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)\n>      +-\t\treturn -1;\n>      ++\tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n>      ++\t\tret = -1;\n>      ++\t\tgoto leave_reset_head;\n>      ++\t}\n>      + \n>      + \tif (!oid) {\n>      + \t\tif (get_oid(\"HEAD\", &head_oid)) {\n>      +-\t\t\trollback_lock_file(&lock);\n>      +-\t\t\treturn error(_(\"could not determine HEAD revision\"));\n>      ++\t\t\tret = error(_(\"could not determine HEAD revision\"));\n>      ++\t\t\tgoto leave_reset_head;\n>      + \t\t}\n>      + \t\toid = &head_oid;\n>      + \t}\n>      +@@\n>      + \t\tunpack_tree_opts.reset = 1;\n>      + \n>      + \tif (read_index_unmerged(the_repository->index) < 0) {\n>      +-\t\trollback_lock_file(&lock);\n>      +-\t\treturn error(_(\"could not read index\"));\n>      ++\t\tret = error(_(\"could not read index\"));\n>      ++\t\tgoto leave_reset_head;\n>        \t}\n>        \n>        \tif (!fill_tree_descriptor(&desc, oid)) {\n>      @@ -31,15 +58,17 @@\n>        \t}\n>        \n>        \ttree = parse_tree_indirect(oid);\n>      -@@\n>      + \tprime_cache_tree(the_repository->index, tree);\n>        \n>      - \tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n>      +-\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)\n>      ++\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {\n>        \t\tret = error(_(\"could not write index\"));\n>       -\tfree((void *)desc.buffer);\n>      - \n>      - \tif (ret)\n>      +-\n>      +-\tif (ret)\n>       -\t\treturn ret;\n>       +\t\tgoto leave_reset_head;\n>      ++\t}\n>        \n>        \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n>        \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n>  -:  ---------- > 2:  db963b2094 rebase: prepare reset_head() for more flags\n>  2:  070092b430 ! 3:  a7360b856f built-in rebase: reinstate `checkout -q` behavior where appropriate\n>      @@ -20,15 +20,18 @@\n>       @@\n>        #define GIT_REFLOG_ACTION_ENVIRONMENT \"GIT_REFLOG_ACTION\"\n>        \n>      + #define RESET_HEAD_DETACH (1<<0)\n>      ++#define RESET_HEAD_HARD (1<<1)\n>      + \n>        static int reset_head(struct object_id *oid, const char *action,\n>      --\t\t      const char *switch_to_branch, int detach_head,\n>      -+\t\t      const char *switch_to_branch,\n>      -+\t\t      int detach_head, int reset_hard,\n>      + \t\t      const char *switch_to_branch, unsigned flags,\n>        \t\t      const char *reflog_orig_head, const char *reflog_head)\n>        {\n>      + \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n>      ++\tunsigned reset_hard = flags & RESET_HEAD_HARD;\n>        \tstruct object_id head_oid;\n>       -\tstruct tree_desc desc;\n>      -+\tstruct tree_desc desc[2];\n>      ++\tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n>        \tstruct lock_file lock = LOCK_INIT;\n>        \tstruct unpack_trees_options unpack_tree_opts;\n>        \tstruct tree *tree;\n>      @@ -42,18 +45,18 @@\n>        \tif (switch_to_branch && !starts_with(switch_to_branch, \"refs/\"))\n>        \t\tBUG(\"Not a fully qualified branch: '%s'\", switch_to_branch);\n>       @@\n>      - \tif (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)\n>      - \t\treturn -1;\n>      + \t\tgoto leave_reset_head;\n>      + \t}\n>        \n>       -\tif (!oid) {\n>       -\t\tif (get_oid(\"HEAD\", &head_oid)) {\n>      --\t\t\trollback_lock_file(&lock);\n>      --\t\t\treturn error(_(\"could not determine HEAD revision\"));\n>      +-\t\t\tret = error(_(\"could not determine HEAD revision\"));\n>      +-\t\t\tgoto leave_reset_head;\n>       -\t\t}\n>       -\t\toid = &head_oid;\n>      -+\tif (get_oid(\"HEAD\", &head_oid)) {\n>      -+\t\trollback_lock_file(&lock);\n>      -+\t\treturn error(_(\"could not determine HEAD revision\"));\n>      ++\tif ((!oid || !reset_hard) && get_oid(\"HEAD\", &head_oid)) {\n>      ++\t\tret = error(_(\"could not determine HEAD revision\"));\n>      ++\t\tgoto leave_reset_head;\n>        \t}\n>        \n>       +\tif (!oid)\n>      @@ -70,7 +73,7 @@\n>        \tunpack_tree_opts.merge = 1;\n>        \tif (!detach_head)\n>       @@\n>      - \t\treturn error(_(\"could not read index\"));\n>      + \t\tgoto leave_reset_head;\n>        \t}\n>        \n>       -\tif (!fill_tree_descriptor(&desc, oid)) {\n>      @@ -104,7 +107,8 @@\n>        \t\tstring_list_clear(&merge_rr, 1);\n>        \n>       -\t\tif (reset_head(NULL, \"reset\", NULL, 0, NULL, NULL) < 0)\n>      -+\t\tif (reset_head(NULL, \"reset\", NULL, 0, 1, NULL, NULL) < 0)\n>      ++\t\tif (reset_head(NULL, \"reset\", NULL, RESET_HEAD_HARD,\n>      ++\t\t\t       NULL, NULL) < 0)\n>        \t\t\tdie(_(\"could not discard worktree changes\"));\n>        \t\tif (read_basic_state(&options))\n>        \t\t\texit(1);\n>      @@ -113,7 +117,8 @@\n>        \t\t\texit(1);\n>        \t\tif (reset_head(&options.orig_head, \"reset\",\n>       -\t\t\t       options.head_name, 0, NULL, NULL) < 0)\n>      -+\t\t\t       options.head_name, 0, 1, NULL, NULL) < 0)\n>      ++\t\t\t       options.head_name, RESET_HEAD_HARD,\n>      ++\t\t\t       NULL, NULL) < 0)\n>        \t\t\tdie(_(\"could not move back to %s\"),\n>        \t\t\t    oid_to_hex(&options.orig_head));\n>        \t\tret = finish_rebase(&options);\n>      @@ -122,34 +127,7 @@\n>        \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n>        \t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n>       -\t\t\t\t       NULL, 0, NULL, NULL) < 0)\n>      -+\t\t\t\t       NULL, 0, 1, NULL, NULL) < 0)\n>      ++\t\t\t\t       NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n>        \t\t\t\tdie(_(\"could not reset --hard\"));\n>        \t\t\tprintf(_(\"HEAD is now at %s\"),\n>        \t\t\t       find_unique_abbrev(&head->object.oid,\n>      -@@\n>      - \t\t\t\tstrbuf_addf(&buf, \"rebase: checkout %s\",\n>      - \t\t\t\t\t    options.switch_to);\n>      - \t\t\t\tif (reset_head(&oid, \"checkout\",\n>      --\t\t\t\t\t       options.head_name, 0,\n>      -+\t\t\t\t\t       options.head_name, 0, 0,\n>      - \t\t\t\t\t       NULL, NULL) < 0) {\n>      - \t\t\t\t\tret = !!error(_(\"could not switch to \"\n>      - \t\t\t\t\t\t\t\"%s\"),\n>      -@@\n>      - \t\t\t \"it...\\n\"));\n>      - \n>      - \tstrbuf_addf(&msg, \"rebase: checkout %s\", options.onto_name);\n>      --\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1,\n>      -+\tif (reset_head(&options.onto->object.oid, \"checkout\", NULL, 1, 0,\n>      - \t    NULL, msg.buf))\n>      - \t\tdie(_(\"Could not detach HEAD\"));\n>      - \tstrbuf_release(&msg);\n>      -@@\n>      - \t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n>      - \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n>      - \t\t\toid_to_hex(&options.onto->object.oid));\n>      --\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n>      -+\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0, 0,\n>      - \t\t\t   \"HEAD\", msg.buf);\n>      - \t\tstrbuf_release(&msg);\n>      - \t\tret = !!finish_rebase(&options);\n> \n> -- \n> gitgitgadget\n> \n"},{"id":"363119","messageId":"xmqq1s7pzqzl.fsf@gitster-ct.c.googlers.com","threadId":"49785","inReplyTo":"nycvar.QRO.7.76.6.1811121235120.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T01:49:18Z","receivedAt":"2018-11-13T01:49:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> On the other hand, for a file-scope static that is likely to stay as a\n>> non-API function but a local helper, it may not be worth it.\n>\n> Oh, do you think that `reset_head()` is likely to stay as non-API\n> function? I found myself in the need of repeating this tedious\n> unpack_trees() dance quite a few times over the past two years, and it is\n> *always* daunting because the API is *that* unintuitive.\n>\n> So I *do* hope that `reset_head()` will actually be moved to reset.[ch]\n> eventually, and callsites e.g. in `sequencer.c` will be converted from\n> calling `unpack_trees()` to calling `reset_head()` instead.\n\nAs long as the public API function is well thought out, of course it\nis OK.  Ideally, builtin/reset.c can detect when it is making a hard\nreset to HEAD and call that same function.  Which may or may not\nmean you would also want to tell it to record ORIG_HEAD and remove\nMERGE_HEAD and others), perhaps with another bit in that flag word.\n"},{"id":"363132","messageId":"xmqq8t1xwlw6.fsf@gitster-ct.c.googlers.com","threadId":"49785","inReplyTo":"xmqq1s7pzqzl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T06:07:37Z","receivedAt":"2018-11-13T06:07:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n>>> On the other hand, for a file-scope static that is likely to stay as a\n>>> non-API function but a local helper, it may not be worth it.\n>>\n>> Oh, do you think that `reset_head()` is likely to stay as non-API\n>> function? I found myself in the need of repeating this tedious\n>> unpack_trees() dance quite a few times over the past two years, and it is\n>> *always* daunting because the API is *that* unintuitive.\n>>\n>> So I *do* hope that `reset_head()` will actually be moved to reset.[ch]\n>> eventually, and callsites e.g. in `sequencer.c` will be converted from\n>> calling `unpack_trees()` to calling `reset_head()` instead.\n>\n> As long as the public API function is well thought out, of course it\n> is OK.  Ideally, builtin/reset.c can detect when it is making a hard\n> reset to HEAD and call that same function.  Which may or may not\n> mean you would also want to tell it to record ORIG_HEAD and remove\n> MERGE_HEAD and others), perhaps with another bit in that flag word.\n\nNah, forget about that one.  It sometimes does branch switching and\nsometimes does hard reset, which looks like a strange chimera.  We\nshouldn't burden it further by adding \"while at it remove cruft, as\n'reset --hard' needs to do that\" to it.\n"}]}