{"thread":{"id":"50095","subject":"[PATCH 0/4] Let the builtin rebase call the git am command directly","startedAt":"2018-12-21T13:17:39Z","lastAt":"2019-01-29T09:46:39Z","messageCount":27,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin","Alban Gruin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"365730","messageId":"pull.24.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":null,"subject":"[PATCH 0/4] Let the builtin rebase call the git am command directly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-21T13:17:35Z","receivedAt":"2018-12-21T13:17:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Especially on Windows, where Unix shell scripting is a foreign endeavor, and\nan expensive one at that, we really want to avoid running through the Bash.\n\nThis not only makes everything faster, but also more robust, as the Bash we\nuse on Windows relies on a derivative of the Cygwin runtime, which in turn\nhas to jump through a couple of hoops that are sometimes a little too tricky\nto make things work. Read: the less we rely on Unix shell scripting, the\nmore likely Windows users will be able to enjoy our software.\n\nJohannes Schindelin (4):\n  rebase: move `reset_head()` into a better spot\n  rebase: avoid double reflog entry when switching branches\n  rebase: teach `reset_head()` to optionally skip the worktree\n  built-in rebase: call `git am` directly\n\n builtin/rebase.c | 428 ++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 309 insertions(+), 119 deletions(-)\n\n\nbase-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-24%2Fdscho%2Fbuiltin-rebase--am-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-24/dscho/builtin-rebase--am-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/24\n-- \ngitgitgadget\n"},{"id":"365731","messageId":"4c5f87b9dc245bf27785aa5559d4b35d87c4bcbf.1545398254.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.git.gitgitgadget@gmail.com","subject":"[PATCH 2/4] rebase: avoid double reflog entry when switching branches","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-21T13:17:37Z","receivedAt":"2018-12-21T13:17:41Z","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 switching a branch *and* updating said branch to a different\nrevision, let's avoid a double entry by first updating the branch and\nthen adjusting the symbolic ref HEAD.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e1dfa74ca8..768bea0da8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\t\t\t detach_head ? REF_NO_DEREF : 0,\n \t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \telse {\n-\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n+\t\tret = update_ref(reflog_orig_head, switch_to_branch, oid,\n+\t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n \t\tif (!ret)\n-\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n-\t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n+\t\t\tret = create_symref(\"HEAD\", switch_to_branch,\n+\t\t\t\t\t    reflog_head);\n \t}\n \n leave_reset_head:\n-- \ngitgitgadget\n\n"},{"id":"365732","messageId":"175dc7d29a02d4ebf6b964f7821738f4ed2bbe0b.1545398254.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.git.gitgitgadget@gmail.com","subject":"[PATCH 1/4] rebase: move `reset_head()` into a better spot","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-21T13:17:36Z","receivedAt":"2018-12-21T13:17:42Z","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\nOver the next commits, we want to make use of it in `run_am()` (i.e.\nrunning the `--am` backend directly, without detouring to Unix shell\nscript code) which in turn will be called from `run_specific_rebase()`.\n\nSo let's move it before that latter function.\n\nThis commit is best viewed using --color-moved.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 238 +++++++++++++++++++++++------------------------\n 1 file changed, 119 insertions(+), 119 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b5c99ec10c..e1dfa74ca8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -333,6 +333,125 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n \t}\n }\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, 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[2] = { { NULL }, { NULL } };\n+\tstruct lock_file lock = LOCK_INIT;\n+\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct tree *tree;\n+\tconst char *reflog_action;\n+\tstruct strbuf msg = STRBUF_INIT;\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, 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+\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 || !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 = reset_hard ? oneway_merge : twoway_merge;\n+\tunpack_tree_opts.update = 1;\n+\tunpack_tree_opts.merge = 1;\n+\tif (!detach_head)\n+\t\tunpack_tree_opts.reset = 1;\n+\n+\tif (read_index_unmerged(the_repository->index) < 0) {\n+\t\tret = error(_(\"could not read index\"));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n+\tif (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {\n+\t\tret = error(_(\"failed to find tree of %s\"),\n+\t\t\t    oid_to_hex(&head_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(nr, desc, &unpack_tree_opts)) {\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+\t\tret = error(_(\"could not write index\"));\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+\tprefix_len = msg.len;\n+\n+\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n+\t\told_orig = &oid_old_orig;\n+\tif (!get_oid(\"HEAD\", &oid_orig)) {\n+\t\torig = &oid_orig;\n+\t\tif (!reflog_orig_head) {\n+\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n+\t\t\treflog_orig_head = msg.buf;\n+\t\t}\n+\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig, old_orig, 0,\n+\t\t\t   UPDATE_REFS_MSG_ON_ERR);\n+\t} else if (old_orig)\n+\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n+\tif (!reflog_head) {\n+\t\tstrbuf_setlen(&msg, prefix_len);\n+\t\tstrbuf_addstr(&msg, \"updating HEAD\");\n+\t\treflog_head = msg.buf;\n+\t}\n+\tif (!switch_to_branch)\n+\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig,\n+\t\t\t\t detach_head ? REF_NO_DEREF : 0,\n+\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n+\telse {\n+\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n+\t\tif (!ret)\n+\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\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+\twhile (nr)\n+\t\tfree((void *)desc[--nr].buffer);\n+\treturn ret;\n+}\n+\n static const char *resolvemsg =\n N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n \"\\\"git add/rm <conflicted_files>\\\", then run \\\"git rebase --continue\\\".\\n\"\n@@ -526,125 +645,6 @@ static int run_specific_rebase(struct rebase_options *opts)\n \treturn status ? -1 : 0;\n }\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, 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[2] = { { NULL }, { NULL } };\n-\tstruct lock_file lock = LOCK_INIT;\n-\tstruct unpack_trees_options unpack_tree_opts;\n-\tstruct tree *tree;\n-\tconst char *reflog_action;\n-\tstruct strbuf msg = STRBUF_INIT;\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, 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-\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 || !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 = reset_hard ? oneway_merge : twoway_merge;\n-\tunpack_tree_opts.update = 1;\n-\tunpack_tree_opts.merge = 1;\n-\tif (!detach_head)\n-\t\tunpack_tree_opts.reset = 1;\n-\n-\tif (read_index_unmerged(the_repository->index) < 0) {\n-\t\tret = error(_(\"could not read index\"));\n-\t\tgoto leave_reset_head;\n-\t}\n-\n-\tif (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {\n-\t\tret = error(_(\"failed to find tree of %s\"),\n-\t\t\t    oid_to_hex(&head_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(nr, desc, &unpack_tree_opts)) {\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-\t\tret = error(_(\"could not write index\"));\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-\tprefix_len = msg.len;\n-\n-\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n-\t\told_orig = &oid_old_orig;\n-\tif (!get_oid(\"HEAD\", &oid_orig)) {\n-\t\torig = &oid_orig;\n-\t\tif (!reflog_orig_head) {\n-\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n-\t\t\treflog_orig_head = msg.buf;\n-\t\t}\n-\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig, old_orig, 0,\n-\t\t\t   UPDATE_REFS_MSG_ON_ERR);\n-\t} else if (old_orig)\n-\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n-\tif (!reflog_head) {\n-\t\tstrbuf_setlen(&msg, prefix_len);\n-\t\tstrbuf_addstr(&msg, \"updating HEAD\");\n-\t\treflog_head = msg.buf;\n-\t}\n-\tif (!switch_to_branch)\n-\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig,\n-\t\t\t\t detach_head ? REF_NO_DEREF : 0,\n-\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n-\telse {\n-\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n-\t\tif (!ret)\n-\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\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-\twhile (nr)\n-\t\tfree((void *)desc[--nr].buffer);\n-\treturn ret;\n-}\n-\n static int rebase_config(const char *var, const char *value, void *data)\n {\n \tstruct rebase_options *opts = data;\n-- \ngitgitgadget\n\n"},{"id":"365733","messageId":"21939140e00d96cf6f76e4c994638fecd3a95639.1545398254.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.git.gitgitgadget@gmail.com","subject":"[PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-21T13:17:38Z","receivedAt":"2018-12-21T13:17:43Z","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\nThis is what the legacy (scripted) rebase does in\n`move_to_original_branch`, and we will need this functionality in the\nnext commit.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 768bea0da8..303175fdf1 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n \n #define RESET_HEAD_DETACH (1<<0)\n #define RESET_HEAD_HARD (1<<1)\n+#define RESET_HEAD_REFS_ONLY (1<<2)\n \n static int reset_head(struct object_id *oid, const char *action,\n \t\t      const char *switch_to_branch, unsigned flags,\n@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,\n {\n \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n+\tunsigned refs_only = flags & RESET_HEAD_REFS_ONLY;\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n@@ -359,7 +361,7 @@ 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+\tif (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n \t\tret = -1;\n \t\tgoto leave_reset_head;\n \t}\n@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,\n \tif (!oid)\n \t\toid = &head_oid;\n \n+\tif (flags & RESET_HEAD_REFS_ONLY)\n+\t\tgoto reset_head_refs;\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@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\tgoto leave_reset_head;\n \t}\n \n+reset_head_refs:\n \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n \tprefix_len = msg.len;\n-- \ngitgitgadget\n\n"},{"id":"365734","messageId":"2b5ece8263936f0a7dfad864c0de43d784fdaf1f.1545398254.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.git.gitgitgadget@gmail.com","subject":"[PATCH 4/4] built-in rebase: call `git am` directly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-21T13:17:38Z","receivedAt":"2018-12-21T13:17:45Z","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\nWhile the scripted `git rebase` still has to rely on the\n`git-rebase--am.sh` script to implement the glue between the `rebase`\nand the `am` commands, we can go a more direct route in the built-in\nrebase and avoid using a shell script altogether.\n\nThis patch represents a straight-forward port of `git-rebase--am.sh` to\nC, along with the glue code to call it directly from within\n`builtin/rebase.c`.\n\nThis reduces the chances of Git for Windows running into trouble due to\nproblems with the POSIX emulation layer (known as \"MSYS2 runtime\",\nitself a derivative of the Cygwin runtime): when no shell script is\ncalled, the POSIX emulation layer is avoided altogether.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 183 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 303175fdf1..e327c30b6b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -246,6 +246,37 @@ static int read_basic_state(struct rebase_options *opts)\n \treturn 0;\n }\n \n+static int write_basic_state(struct rebase_options *opts)\n+{\n+\twrite_file(state_dir_path(\"head-name\", opts), \"%s\",\n+\t\t   opts->head_name ? opts->head_name : \"detached HEAD\");\n+\twrite_file(state_dir_path(\"onto\", opts), \"%s\",\n+\t\t   opts->onto ? oid_to_hex(&opts->onto->object.oid) : \"\");\n+\twrite_file(state_dir_path(\"orig-head\", opts), \"%s\",\n+\t\t   oid_to_hex(&opts->orig_head));\n+\twrite_file(state_dir_path(\"quiet\", opts), \"%s\",\n+\t\t   opts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n+\tif (opts->flags & REBASE_VERBOSE)\n+\t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n+\tif (opts->strategy)\n+\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n+\t\t\t   opts->strategy);\n+\tif (opts->strategy_opts)\n+\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n+\t\t\t   opts->strategy_opts);\n+\tif (opts->allow_rerere_autoupdate >= 0)\n+\t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n+\t\t\t   \"-%s-rerere-autoupdate\",\n+\t\t\t   opts->allow_rerere_autoupdate ? \"\" : \"-no\");\n+\tif (opts->gpg_sign_opt)\n+\t\twrite_file(state_dir_path(\"gpg_sign_opt\", opts), \"%s\",\n+\t\t\t   opts->gpg_sign_opt);\n+\tif (opts->signoff)\n+\t\twrite_file(state_dir_path(\"strategy\", opts), \"--signoff\");\n+\n+\treturn 0;\n+}\n+\n static int apply_autostash(struct rebase_options *opts)\n {\n \tconst char *path = state_dir_path(\"autostash\", opts);\n@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,\n \treturn ret;\n }\n \n+static int move_to_original_branch(struct rebase_options *opts)\n+{\n+\tstruct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (!opts->head_name)\n+\t\treturn 0; /* nothing to move back to */\n+\n+\tif (!opts->onto)\n+\t\tBUG(\"move_to_original_branch without onto\");\n+\n+\tstrbuf_addf(&orig_head_reflog, \"rebase finished: %s onto %s\",\n+\t\t    opts->head_name, oid_to_hex(&opts->onto->object.oid));\n+\tstrbuf_addf(&head_reflog, \"rebase finished: returning to %s\",\n+\t\t    opts->head_name);\n+\tret = reset_head(NULL, \"checkout\", opts->head_name,\n+\t\t\t RESET_HEAD_REFS_ONLY,\n+\t\t\t orig_head_reflog.buf, head_reflog.buf);\n+\n+\tstrbuf_release(&orig_head_reflog);\n+\tstrbuf_release(&head_reflog);\n+\treturn ret;\n+}\n+\n static const char *resolvemsg =\n N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n \"\\\"git add/rm <conflicted_files>\\\", then run \\\"git rebase --continue\\\".\\n\"\n@@ -466,6 +521,129 @@ N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n \"To abort and get back to the state before \\\"git rebase\\\", run \"\n \"\\\"git rebase --abort\\\".\");\n \n+static int run_am(struct rebase_options *opts)\n+{\n+\tstruct child_process am = CHILD_PROCESS_INIT;\n+\tstruct child_process format_patch = CHILD_PROCESS_INIT;\n+\tstruct strbuf revisions = STRBUF_INIT;\n+\tint status;\n+\tchar *rebased_patches;\n+\n+\tam.git_cmd = 1;\n+\targv_array_push(&am.args, \"am\");\n+\n+\tif (opts->action && !strcmp(\"continue\", opts->action)) {\n+\t\targv_array_push(&am.args, \"--resolved\");\n+\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\t\tif (opts->gpg_sign_opt)\n+\t\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n+\t\tstatus = run_command(&am);\n+\t\tif (status)\n+\t\t\treturn status;\n+\n+\t\tdiscard_cache();\n+\t\treturn move_to_original_branch(opts);\n+\t}\n+\tif (opts->action && !strcmp(\"skip\", opts->action)) {\n+\t\targv_array_push(&am.args, \"--skip\");\n+\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\t\tstatus = run_command(&am);\n+\t\tif (status)\n+\t\t\treturn status;\n+\n+\t\tdiscard_cache();\n+\t\treturn move_to_original_branch(opts);\n+\t}\n+\tif (opts->action && !strcmp(\"show-current-patch\", opts->action)) {\n+\t\targv_array_push(&am.args, \"--show-current-patch\");\n+\t\treturn run_command(&am);\n+\t}\n+\n+\tstrbuf_addf(&revisions, \"%s...%s\",\n+\t\t    oid_to_hex(opts->root ?\n+\t\t\t       /* this is now equivalent to ! -z \"$upstream\" */\n+\t\t\t       &opts->onto->object.oid :\n+\t\t\t       &opts->upstream->object.oid),\n+\t\t    oid_to_hex(&opts->orig_head));\n+\n+\trebased_patches = xstrdup(git_path(\"rebased-patches\"));\n+\tformat_patch.out = open(rebased_patches,\n+\t\t\t\tO_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\tif (format_patch.out < 0) {\n+\t\tstatus = error_errno(_(\"could not write '%s'\"),\n+\t\t\t\t     rebased_patches);\n+\t\tfree(rebased_patches);\n+\t\targv_array_clear(&am.args);\n+\t\treturn status;\n+\t}\n+\n+\tformat_patch.git_cmd = 1;\n+\targv_array_pushl(&format_patch.args, \"format-patch\", \"-k\", \"--stdout\",\n+\t\t\t \"--full-index\", \"--cherry-pick\", \"--right-only\",\n+\t\t\t \"--src-prefix=a/\", \"--dst-prefix=b/\", \"--no-renames\",\n+\t\t\t \"--no-cover-letter\", \"--pretty=mboxrd\", NULL);\n+\tif (opts->git_format_patch_opt.len)\n+\t\targv_array_split(&format_patch.args,\n+\t\t\t\t opts->git_format_patch_opt.buf);\n+\targv_array_push(&format_patch.args, revisions.buf);\n+\tif (opts->restrict_revision)\n+\t\targv_array_pushf(&format_patch.args, \"^%s\",\n+\t\t\t\t oid_to_hex(&opts->restrict_revision->object.oid));\n+\n+\tstatus = run_command(&format_patch);\n+\tif (status) {\n+\t\tunlink(rebased_patches);\n+\t\tfree(rebased_patches);\n+\t\targv_array_clear(&am.args);\n+\n+\t\treset_head(&opts->orig_head, \"checkout\", opts->head_name, 0,\n+\t\t\t   \"HEAD\", NULL);\n+\t\terror(_(\"\\ngit encountered an error while preparing the \"\n+\t\t\t\"patches to replay\\n\"\n+\t\t\t\"these revisions:\\n\"\n+\t\t\t\"\\n    %s\\n\\n\"\n+\t\t\t\"As a result, git cannot rebase them.\"),\n+\t\t      opts->revisions);\n+\n+\t\tstrbuf_release(&revisions);\n+\t\treturn status;\n+\t}\n+\tstrbuf_release(&revisions);\n+\n+\tam.in = open(rebased_patches, O_RDONLY);\n+\tif (am.in < 0) {\n+\t\tstatus = error_errno(_(\"could not read '%s'\"),\n+\t\t\t\t     rebased_patches);\n+\t\tfree(rebased_patches);\n+\t\targv_array_clear(&am.args);\n+\t\treturn status;\n+\t}\n+\n+\targv_array_pushv(&am.args, opts->git_am_opts.argv);\n+\targv_array_push(&am.args, \"--rebasing\");\n+\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\targv_array_push(&am.args, \"--patch-format=mboxrd\");\n+\tif (opts->allow_rerere_autoupdate > 0)\n+\t\targv_array_push(&am.args, \"--rerere-autoupdate\");\n+\telse if (opts->allow_rerere_autoupdate == 0)\n+\t\targv_array_push(&am.args, \"--no-rerere-autoupdate\");\n+\tif (opts->gpg_sign_opt)\n+\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n+\tstatus = run_command(&am);\n+\tunlink(rebased_patches);\n+\tfree(rebased_patches);\n+\n+\tif (!status) {\n+\t\tdiscard_cache();\n+\t\treturn move_to_original_branch(opts);\n+\t}\n+\n+\tif (is_directory(opts->state_dir))\n+\t\twrite_basic_state(opts);\n+\n+\treturn status;\n+}\n+\n static int run_specific_rebase(struct rebase_options *opts)\n {\n \tconst char *argv[] = { NULL, NULL };\n@@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts)\n \t\tgoto finished_rebase;\n \t}\n \n+\tif (opts->type == REBASE_AM) {\n+\t\tstatus = run_am(opts);\n+\t\tgoto finished_rebase;\n+\t}\n+\n \tadd_var(&script_snippet, \"GIT_DIR\", absolute_path(get_git_dir()));\n \tadd_var(&script_snippet, \"state_dir\", opts->state_dir);\n \n-- \ngitgitgadget\n"},{"id":"366150","messageId":"xmqqwonkclpx.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"2b5ece8263936f0a7dfad864c0de43d784fdaf1f.1545398254.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] built-in rebase: call `git am` directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T18:38:02Z","receivedAt":"2019-01-04T18:38:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> +static int write_basic_state(struct rebase_options *opts)\n> +{\n> +\twrite_file(state_dir_path(\"head-name\", opts), \"%s\",\n> +\t\t   opts->head_name ? opts->head_name : \"detached HEAD\");\n> +\twrite_file(state_dir_path(\"onto\", opts), \"%s\",\n> +\t\t   opts->onto ? oid_to_hex(&opts->onto->object.oid) : \"\");\n> +\twrite_file(state_dir_path(\"orig-head\", opts), \"%s\",\n> +\t\t   oid_to_hex(&opts->orig_head));\n> +\twrite_file(state_dir_path(\"quiet\", opts), \"%s\",\n> +\t\t   opts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n> +\tif (opts->flags & REBASE_VERBOSE)\n> +\t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n> +\tif (opts->strategy)\n> +\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n> +\t\t\t   opts->strategy);\n> +\tif (opts->strategy_opts)\n> +\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n> +\t\t\t   opts->strategy_opts);\n> +\tif (opts->allow_rerere_autoupdate >= 0)\n> +\t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n> +\t\t\t   \"-%s-rerere-autoupdate\",\n> +\t\t\t   opts->allow_rerere_autoupdate ? \"\" : \"-no\");\n\nInside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0\n(declined) or 1 (requested), and this code is being consistent with\nthat convention.\n\nThe \"--[no-]rerere-autoupdate\" option that is parsed via\nOPT_RERERE_AUTOUPDATE (used in builtin/rebase--interactive.c among\nother built-in commands) on the other hand is tertially that uses 0\n(unspecified), 1 (requested) and 2 (declined).  This might be a\nticking timebomb to confuse us in the future that may be worth\nfixing but probably outside this series.\n\n> +\tif (opts->gpg_sign_opt)\n> +\t\twrite_file(state_dir_path(\"gpg_sign_opt\", opts), \"%s\",\n> +\t\t\t   opts->gpg_sign_opt);\n> +\tif (opts->signoff)\n> +\t\twrite_file(state_dir_path(\"strategy\", opts), \"--signoff\");\n> +\n> +\treturn 0;\n> +}\n\nThe above looks like a literal translation of a helper function with\nthe same name in git-rebase--common.sh.  Good.\n\n> @@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \treturn ret;\n>  }\n>  \n> +static int move_to_original_branch(struct rebase_options *opts)\n> +{\n> +\tstruct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;\n> +\tint ret;\n> +\n> +\tif (!opts->head_name)\n> +\t\treturn 0; /* nothing to move back to */\n> +\n> +\tif (!opts->onto)\n> +\t\tBUG(\"move_to_original_branch without onto\");\n\nThis check is absent in the scripted version, but from the message\nwe generate here, it is clear that the caller must not call this\nwhen there is no \"onto\" commit.  Good.\n\n> +\tstrbuf_addf(&orig_head_reflog, \"rebase finished: %s onto %s\",\n> +\t\t    opts->head_name, oid_to_hex(&opts->onto->object.oid));\n> +\tstrbuf_addf(&head_reflog, \"rebase finished: returning to %s\",\n> +\t\t    opts->head_name);\n> +\tret = reset_head(NULL, \"checkout\", opts->head_name,\n> +\t\t\t RESET_HEAD_REFS_ONLY,\n> +\t\t\t orig_head_reflog.buf, head_reflog.buf);\n\nThe *action given to reset_head() here is \"checkout\".  Makes me\nwonder about two things:\n\n - The only real use of the parameter in the callee is to prepare\n   the error and advice messages from the unpack_trees machinery,\n   but because we are using it in REFS_ONLY mode, it does not\n   matter.  In fact it might even be misleading; perhaps pass NULL\n   or something, so that a mistaken update to reset_head() later\n   that lets REFS_ONLY request to go to unpack_trees machinery will\n   catch it as a bug?\n\n - Another topic in flight wants to make sure that the post-checkout\n   hook gets called when the RESET_HEAD_RUN_POST_CHECKOUT_HOOK flag\n   is given by the caller, and IIRC, the use of the flag is strongly\n   correlated to *action being \"checkout\".  Do we want to pass\n   REFS_ONLY and RUN_POST_CHECKOUT_HOOK flag for this call, or do we\n   rather keep it silent?  As the original scripted version did not\n   use \"checkout\" here and never triggered post-checkout hook, I am\n   inclined to say that we should not pass that other bit.  That\n   then leads me to suspect that we do not want *action to be\n   \"checkout\" here.\n\n> +\tstrbuf_release(&orig_head_reflog);\n> +\tstrbuf_release(&head_reflog);\n> +\treturn ret;\n> +}\n\nUnlike the scripted version, this does not die() upon failure, so\nthe caller needs to be careful about the returned status.\n\n> @@ -466,6 +521,129 @@ N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n>  \"To abort and get back to the state before \\\"git rebase\\\", run \"\n>  \"\\\"git rebase --abort\\\".\");\n>  \n> +static int run_am(struct rebase_options *opts)\n> +{\n> +\tstruct child_process am = CHILD_PROCESS_INIT;\n> +\tstruct child_process format_patch = CHILD_PROCESS_INIT;\n> +\tstruct strbuf revisions = STRBUF_INIT;\n> +\tint status;\n> +\tchar *rebased_patches;\n> +\n> +\tam.git_cmd = 1;\n> +\targv_array_push(&am.args, \"am\");\n> +\n> +\tif (opts->action && !strcmp(\"continue\", opts->action)) {\n> +\t\targv_array_push(&am.args, \"--resolved\");\n> +\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n> +\t\tif (opts->gpg_sign_opt)\n> +\t\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n> +\t\tstatus = run_command(&am);\n> +\t\tif (status)\n> +\t\t\treturn status;\n> +\n> +\t\tdiscard_cache();\n> +\t\treturn move_to_original_branch(opts);\n\nIt is curious why discard_cache() is placed exacly here, as if we\nwant to preserve the contents of the in-core index when\nrun_command() failed.  But I do not think we care about the in-core\nindex as the only thing that happen after \"return status\" is to\nreturn the control to run_specific_rebase(), let it jump to\nfinished_rebase label to clean things up and rturn control to\ncmd_rebase() and exit based on the status value.\n\nIt's not like move_to_original_branch() wants to call read_cache()\nand get the result from the \"am\" that run_command() executed,\neither.\n\nPuzzled.  Care to explain a bit more in the in-code comment?\n\n> +\t}\n> +\tif (opts->action && !strcmp(\"skip\", opts->action)) {\n> +\t\targv_array_push(&am.args, \"--skip\");\n> +\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n> +\t\tstatus = run_command(&am);\n> +\t\tif (status)\n> +\t\t\treturn status;\n> +\n> +\t\tdiscard_cache();\n> +\t\treturn move_to_original_branch(opts);\n\nDitto.\n\n> +\t}\n> +\tif (opts->action && !strcmp(\"show-current-patch\", opts->action)) {\n> +\t\targv_array_push(&am.args, \"--show-current-patch\");\n> +\t\treturn run_command(&am);\n> +\t}\n\nUp to this point, it is a faithful conversion of the first case/esac\nstatement.  Good.\n\n> +\tstrbuf_addf(&revisions, \"%s...%s\",\n> +\t\t    oid_to_hex(opts->root ?\n> +\t\t\t       /* this is now equivalent to ! -z \"$upstream\" */\n\nDoes \"this\" refer to the \"opts->root being true\" check?\n\nBecause you are flipping the polarity of the test from scripted\nversion, shouldn't the comment be updated to \"-z $upstream\"?\n\n> +\t\t\t       &opts->onto->object.oid :\n> +\t\t\t       &opts->upstream->object.oid),\n> +\t\t    oid_to_hex(&opts->orig_head));\n\n> +\trebased_patches = xstrdup(git_path(\"rebased-patches\"));\n> +\tformat_patch.out = open(rebased_patches,\n> +\t\t\t\tO_WRONLY | O_CREAT | O_TRUNC, 0666);\n\nUnlike scripted version, we do not remove a (possibly) existing file.\nWe give CREAT in case there is no existing one, and TRUNC in case\nthere is an existing one.  Makes sense.  A more faithful translation\nwould have unlink(2)ed a (possibly) existing one, and then because\nwe can afford to, passed O_EXCL to avoid stomping on somebody else\nracing with us, but I do not think it is worth it.\n\n> +\tif (format_patch.out < 0) {\n> +\t\tstatus = error_errno(_(\"could not write '%s'\"),\n> +\t\t\t\t     rebased_patches);\n\ns/write '%s'/open '%s' for writing/?  I dunno.\n\n> +\t\tfree(rebased_patches);\n> +\t\targv_array_clear(&am.args);\n> +\t\treturn status;\n> +\t}\n> +\n> +\tformat_patch.git_cmd = 1;\n> +\targv_array_pushl(&format_patch.args, \"format-patch\", \"-k\", \"--stdout\",\n> +\t\t\t \"--full-index\", \"--cherry-pick\", \"--right-only\",\n> +\t\t\t \"--src-prefix=a/\", \"--dst-prefix=b/\", \"--no-renames\",\n> +\t\t\t \"--no-cover-letter\", \"--pretty=mboxrd\", NULL);\n> +\tif (opts->git_format_patch_opt.len)\n> +\t\targv_array_split(&format_patch.args,\n> +\t\t\t\t opts->git_format_patch_opt.buf);\n> +\targv_array_push(&format_patch.args, revisions.buf);\n> +\tif (opts->restrict_revision)\n> +\t\targv_array_pushf(&format_patch.args, \"^%s\",\n> +\t\t\t\t oid_to_hex(&opts->restrict_revision->object.oid));\n\nIt is kinda surprising to see that we have learned quite a lot of\nfringe \"configurations\" we need to explicitly override like this.\n\nLooks like a quite faithful conversion, anyway.\n\n> +\tstatus = run_command(&format_patch);\n> +\tif (status) {\n> +\t\tunlink(rebased_patches);\n> +\t\tfree(rebased_patches);\n> +\t\targv_array_clear(&am.args);\n> +\n> +\t\treset_head(&opts->orig_head, \"checkout\", opts->head_name, 0,\n> +\t\t\t   \"HEAD\", NULL);\n\nThis one may need to trigger post-checkout hook.  The scripted\nversion does two different things depending on the value of\n$head_name, but we can just use the same code without conditional?\n\n> +\t\terror(_(\"\\ngit encountered an error while preparing the \"\n> +\t\t\t\"patches to replay\\n\"\n> +\t\t\t\"these revisions:\\n\"\n> +\t\t\t\"\\n    %s\\n\\n\"\n> +\t\t\t\"As a result, git cannot rebase them.\"),\n> +\t\t      opts->revisions);\n> +\n> +\t\tstrbuf_release(&revisions);\n> +\t\treturn status;\n> +\t}\n> +\tstrbuf_release(&revisions);\n> +\n> +\tam.in = open(rebased_patches, O_RDONLY);\n> +\tif (am.in < 0) {\n> +\t\tstatus = error_errno(_(\"could not read '%s'\"),\n> +\t\t\t\t     rebased_patches);\n\ns/write '%s'/open '%s' for reading/?  I dunno.\n\n> +\t\tfree(rebased_patches);\n> +\t\targv_array_clear(&am.args);\n> +\t\treturn status;\n> +\t}\n> +\n> +\targv_array_pushv(&am.args, opts->git_am_opts.argv);\n> +\targv_array_push(&am.args, \"--rebasing\");\n> +\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n> +\targv_array_push(&am.args, \"--patch-format=mboxrd\");\n> +\tif (opts->allow_rerere_autoupdate > 0)\n> +\t\targv_array_push(&am.args, \"--rerere-autoupdate\");\n> +\telse if (opts->allow_rerere_autoupdate == 0)\n> +\t\targv_array_push(&am.args, \"--no-rerere-autoupdate\");\n> +\tif (opts->gpg_sign_opt)\n> +\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n> +\tstatus = run_command(&am);\n> +\tunlink(rebased_patches);\n> +\tfree(rebased_patches);\n> +\n> +\tif (!status) {\n> +\t\tdiscard_cache();\n> +\t\treturn move_to_original_branch(opts);\n> +\t}\n> +\n> +\tif (is_directory(opts->state_dir))\n> +\t\twrite_basic_state(opts);\n> +\n> +\treturn status;\n> +}\n> +\n>  static int run_specific_rebase(struct rebase_options *opts)\n>  {\n>  \tconst char *argv[] = { NULL, NULL };\n> @@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts)\n>  \t\tgoto finished_rebase;\n>  \t}\n>  \n> +\tif (opts->type == REBASE_AM) {\n> +\t\tstatus = run_am(opts);\n> +\t\tgoto finished_rebase;\n> +\t}\n> +\n>  \tadd_var(&script_snippet, \"GIT_DIR\", absolute_path(get_git_dir()));\n>  \tadd_var(&script_snippet, \"state_dir\", opts->state_dir);\n\n\nOverall, this was quite a pleasant read and a well constructed\nseries.  Other than two minor points (i.e. interaction with the\n'post-checkout hook' topic, and discard_cache() before calling\nmove_to_original_branch) I did not quite understand, looks good to\nme.\n\nWhen merged to 'pu', I seem to be getting failure from t3425.5, .8\nand .11, by the way.  I haven't dug into the actual breakages any\nfurther than that.\n\nThanks.\n"},{"id":"366151","messageId":"xmqqpntcclot.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"21939140e00d96cf6f76e4c994638fecd3a95639.1545398254.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T18:38:42Z","receivedAt":"2019-01-04T18:38:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> This is what the legacy (scripted) rebase does in\n> `move_to_original_branch`, and we will need this functionality in the\n> next commit.\n\nThe move-to-original-branch helper does:\n\n    - point $head_name to the commit pointed at by HEAD\n    - point HEAD symref to $head_name\n\nwithout touching the index or the working tree files.  It's not\nexactly \"reset --soft\" but more like \"switch-branch --soft\" ;-)\n\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  builtin/rebase.c | 8 +++++++-\n>  1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 768bea0da8..303175fdf1 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n>  \n>  #define RESET_HEAD_DETACH (1<<0)\n>  #define RESET_HEAD_HARD (1<<1)\n> +#define RESET_HEAD_REFS_ONLY (1<<2)\n\nIn the future codebase in 'pu', we have 1<<2 already taken by\nanother topic, so I'll tell my rerere database that the bit\nassignment needs to be adjusted.\n\n>  static int reset_head(struct object_id *oid, const char *action,\n>  \t\t      const char *switch_to_branch, unsigned flags,\n> @@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,\n>  {\n>  \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n>  \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n> +\tunsigned refs_only = flags & RESET_HEAD_REFS_ONLY;\n>  \tstruct object_id head_oid;\n>  \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n>  \tstruct lock_file lock = LOCK_INIT;\n> @@ -359,7 +361,7 @@ 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> +\tif (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n>  \t\tret = -1;\n>  \t\tgoto leave_reset_head;\n>  \t}\n\nNot touching the index, so no need to lock the index.  Makes sense.\n\n> @@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \tif (!oid)\n>  \t\toid = &head_oid;\n>  \n> +\tif (flags & RESET_HEAD_REFS_ONLY)\n> +\t\tgoto reset_head_refs;\n> +\n\nWhy not \"refs_only\" that we already prepared above???  Are we\nmunging that secondary variable before control comes here?\n\nIn any case, not touching the index nor the working tree, so no need\nto call into the unpack_trees machinery.  Makes sense.\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> @@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \t\tgoto leave_reset_head;\n>  \t}\n>  \n> +reset_head_refs:\n>  \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n\nAnd the control continues from the point we update the reflog.\nMakes sense.\n\n>  \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n>  \tprefix_len = msg.len;\n\nThis helper is touched by two other topics in flight, and that was\none of the reason why it took a bit longer than usual for me to\nmerge this topic.  Please sanity-check the result of the conflict\nresolution at the tip of 'pu' branch.\n\nThanks.\n"},{"id":"366152","messageId":"xmqqimz4cloh.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"4c5f87b9dc245bf27785aa5559d4b35d87c4bcbf.1545398254.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] rebase: avoid double reflog entry when switching branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T18:38:54Z","receivedAt":"2019-01-04T18:38:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> When switching a branch *and* updating said branch to a different\n> revision, let's avoid a double entry by first updating the branch and\n> then adjusting the symbolic ref HEAD.\n\nAh, in the original sequence, HEAD is updated twice, leaving two\nreflog entries for HEAD (and one for the underlying \"switch_to\"\nbranch by virtue of REF_UPDATE_VIA_HEAD).  In the new sequence,\nupdate_ref() updates the underlying \"switch_to\" and then HEAD, so\nwe'd get one reflog entry for each of them.\n\nMakes sense.  s/let's avoid a double entry/& in HEAD's reflog/ would\nhave avoided wasting reader's time who needlessly wondered where\nthat redundancy came from, though.\n\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  builtin/rebase.c | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index e1dfa74ca8..768bea0da8 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \t\t\t\t detach_head ? REF_NO_DEREF : 0,\n>  \t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n>  \telse {\n> -\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n> +\t\tret = update_ref(reflog_orig_head, switch_to_branch, oid,\n> +\t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n>  \t\tif (!ret)\n> -\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n> -\t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n> +\t\t\tret = create_symref(\"HEAD\", switch_to_branch,\n> +\t\t\t\t\t    reflog_head);\n>  \t}\n>  \n>  leave_reset_head:\n"},{"id":"366153","messageId":"xmqqbm4wclob.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"175dc7d29a02d4ebf6b964f7821738f4ed2bbe0b.1545398254.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] rebase: move `reset_head()` into a better spot","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T18:39:00Z","receivedAt":"2019-01-04T18:39:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Over the next commits, we want to make use of it in `run_am()` (i.e.\n> running the `--am` backend directly, without detouring to Unix shell\n> script code) which in turn will be called from `run_specific_rebase()`.\n>\n> So let's move it before that latter function.\n\nOK.\n"},{"id":"366154","messageId":"xmqq7efkclm3.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"xmqqpntcclot.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T18:40:20Z","receivedAt":"2019-01-04T18:40:25Z","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 via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n>> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>>\n>> This is what the legacy (scripted) rebase does in\n>> `move_to_original_branch`, and we will need this functionality in the\n>> next commit.\n>\n> The move-to-original-branch helper does:\n>\n>     - point $head_name to the commit pointed at by HEAD\n>     - point HEAD symref to $head_name\n>\n> without touching the index or the working tree files.  It's not\n> exactly \"reset --soft\" but more like \"switch-branch --soft\" ;-)\n>\n>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> ---\n>>  builtin/rebase.c | 8 +++++++-\n>>  1 file changed, 7 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index 768bea0da8..303175fdf1 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n>>  \n>>  #define RESET_HEAD_DETACH (1<<0)\n>>  #define RESET_HEAD_HARD (1<<1)\n>> +#define RESET_HEAD_REFS_ONLY (1<<2)\n>\n> In the future codebase in 'pu', we have 1<<2 already taken by\n> another topic, so I'll tell my rerere database that the bit\n> assignment needs to be adjusted.\n>\n>>  static int reset_head(struct object_id *oid, const char *action,\n>>  \t\t      const char *switch_to_branch, unsigned flags,\n>> @@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,\n>>  {\n>>  \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n>>  \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n>> +\tunsigned refs_only = flags & RESET_HEAD_REFS_ONLY;\n>>  \tstruct object_id head_oid;\n>>  \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n>>  \tstruct lock_file lock = LOCK_INIT;\n>> @@ -359,7 +361,7 @@ 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>> +\tif (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n>>  \t\tret = -1;\n>>  \t\tgoto leave_reset_head;\n>>  \t}\n>\n> Not touching the index, so no need to lock the index.  Makes sense.\n>\n>> @@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,\n>>  \tif (!oid)\n>>  \t\toid = &head_oid;\n>>  \n>> +\tif (flags & RESET_HEAD_REFS_ONLY)\n>> +\t\tgoto reset_head_refs;\n>> +\n>\n> Why not \"refs_only\" that we already prepared above???  Are we\n> munging that secondary variable before control comes here?\n>\n> In any case, not touching the index nor the working tree, so no need\n> to call into the unpack_trees machinery.  Makes sense.\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>> @@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,\n>>  \t\tgoto leave_reset_head;\n>>  \t}\n>>  \n>> +reset_head_refs:\n>>  \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n>\n> And the control continues from the point we update the reflog.\n> Makes sense.\n>\n>>  \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n>>  \tprefix_len = msg.len;\n>\n> This helper is touched by two other topics in flight, and that was\n> one of the reason why it took a bit longer than usual for me to\n> merge this topic.  Please sanity-check the result of the conflict\n> resolution at the tip of 'pu' branch.\n\nThe topics are os/rebase-runs-post-checkout-hook and nd/backup-log\nIIRC.\n"},{"id":"366161","messageId":"xmqqbm4wau51.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"2b5ece8263936f0a7dfad864c0de43d784fdaf1f.1545398254.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] built-in rebase: call `git am` directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T23:19:06Z","receivedAt":"2019-01-04T23:19:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> While the scripted `git rebase` still has to rely on the\n> `git-rebase--am.sh` script to implement the glue between the `rebase`\n> and the `am` commands, we can go a more direct route in the built-in\n> rebase and avoid using a shell script altogether.\n\n ...\n\n>  builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++\n\nNow at some point as a follow-up change, we'd need to remove the\ngit-rebase--am.sh that is no longer used, together with the\nreference to it in the Makefile, no?\n"},{"id":"366268","messageId":"xmqqef9o9yhf.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"xmqqbm4wau51.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/4] built-in rebase: call `git am` directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-07T17:19:40Z","receivedAt":"2019-01-07T17:19:45Z","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 via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n>> While the scripted `git rebase` still has to rely on the\n>> `git-rebase--am.sh` script to implement the glue between the `rebase`\n>> and the `am` commands, we can go a more direct route in the built-in\n>> rebase and avoid using a shell script altogether.\n>\n>  ...\n>\n>>  builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++\n>\n> Now at some point as a follow-up change, we'd need to remove the\n> git-rebase--am.sh that is no longer used, together with the\n> reference to it in the Makefile, no?\n\nAnswering to myself.  That needs to wait until the legacy-rebase is\nretired, so it's not like \"immediately after this series is done\"\nkind of thing.\n\nAnyway, thanks for working on this.\n"},{"id":"367097","messageId":"nycvar.QRO.7.76.6.1901181443350.41@tvgsbejvaqbjf.bet","threadId":"50095","inReplyTo":"xmqqwonkclpx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/4] built-in rebase: call `git am` directly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T14:15:45Z","receivedAt":"2019-01-18T14:16:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 4 Jan 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n> \n> > +static int write_basic_state(struct rebase_options *opts)\n> > +{\n> > +\twrite_file(state_dir_path(\"head-name\", opts), \"%s\",\n> > +\t\t   opts->head_name ? opts->head_name : \"detached HEAD\");\n> > +\twrite_file(state_dir_path(\"onto\", opts), \"%s\",\n> > +\t\t   opts->onto ? oid_to_hex(&opts->onto->object.oid) : \"\");\n> > +\twrite_file(state_dir_path(\"orig-head\", opts), \"%s\",\n> > +\t\t   oid_to_hex(&opts->orig_head));\n> > +\twrite_file(state_dir_path(\"quiet\", opts), \"%s\",\n> > +\t\t   opts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n> > +\tif (opts->flags & REBASE_VERBOSE)\n> > +\t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n> > +\tif (opts->strategy)\n> > +\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n> > +\t\t\t   opts->strategy);\n> > +\tif (opts->strategy_opts)\n> > +\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n> > +\t\t\t   opts->strategy_opts);\n> > +\tif (opts->allow_rerere_autoupdate >= 0)\n> > +\t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n> > +\t\t\t   \"-%s-rerere-autoupdate\",\n> > +\t\t\t   opts->allow_rerere_autoupdate ? \"\" : \"-no\");\n> \n> Inside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0\n> (declined) or 1 (requested), and this code is being consistent with\n> that convention.\n> \n> The \"--[no-]rerere-autoupdate\" option that is parsed via\n> OPT_RERERE_AUTOUPDATE (used in builtin/rebase--interactive.c among\n> other built-in commands) on the other hand is tertially that uses 0\n> (unspecified), 1 (requested) and 2 (declined).  This might be a\n> ticking timebomb to confuse us in the future that may be worth\n> fixing but probably outside this series.\n\nGood point. We use -1 for unspecified in so many places, I think\nOPT_RERERE_AUTOUPDATE needs to be fixed. But yes, I'll leave this as\n#leftoverbits here.\n\n> > @@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  \treturn ret;\n> >  }\n> >  \n> > +static int move_to_original_branch(struct rebase_options *opts)\n> > +{\n> > +\tstruct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;\n> > +\tint ret;\n> > +\n> > +\tif (!opts->head_name)\n> > +\t\treturn 0; /* nothing to move back to */\n> > +\n> > +\tif (!opts->onto)\n> > +\t\tBUG(\"move_to_original_branch without onto\");\n> \n> This check is absent in the scripted version, but from the message\n> we generate here, it is clear that the caller must not call this\n> when there is no \"onto\" commit.  Good.\n> \n> > +\tstrbuf_addf(&orig_head_reflog, \"rebase finished: %s onto %s\",\n> > +\t\t    opts->head_name, oid_to_hex(&opts->onto->object.oid));\n> > +\tstrbuf_addf(&head_reflog, \"rebase finished: returning to %s\",\n> > +\t\t    opts->head_name);\n> > +\tret = reset_head(NULL, \"checkout\", opts->head_name,\n> > +\t\t\t RESET_HEAD_REFS_ONLY,\n> > +\t\t\t orig_head_reflog.buf, head_reflog.buf);\n> \n> The *action given to reset_head() here is \"checkout\".  Makes me\n> wonder about two things:\n> \n>  - The only real use of the parameter in the callee is to prepare\n>    the error and advice messages from the unpack_trees machinery,\n>    but because we are using it in REFS_ONLY mode, it does not\n>    matter.  In fact it might even be misleading; perhaps pass NULL\n>    or something, so that a mistaken update to reset_head() later\n>    that lets REFS_ONLY request to go to unpack_trees machinery will\n>    catch it as a bug?\n> \n>  - Another topic in flight wants to make sure that the post-checkout\n>    hook gets called when the RESET_HEAD_RUN_POST_CHECKOUT_HOOK flag\n>    is given by the caller, and IIRC, the use of the flag is strongly\n>    correlated to *action being \"checkout\".  Do we want to pass\n>    REFS_ONLY and RUN_POST_CHECKOUT_HOOK flag for this call, or do we\n>    rather keep it silent?  As the original scripted version did not\n>    use \"checkout\" here and never triggered post-checkout hook, I am\n>    inclined to say that we should not pass that other bit.  That\n>    then leads me to suspect that we do not want *action to be\n>    \"checkout\" here.\n\nThe only thing for which that the `action` is used, though, is the call to\n`setup_unpack_trees_porcelain()`, which does not accept a `NULL`. I guess\nI could replace it by the empty string. Will do that.\n\n> \n> > +\tstrbuf_release(&orig_head_reflog);\n> > +\tstrbuf_release(&head_reflog);\n> > +\treturn ret;\n> > +}\n> \n> Unlike the scripted version, this does not die() upon failure, so\n> the caller needs to be careful about the returned status.\n\nIndeed. That function is only called from `run_am()`, and returns the\nstatus in every instance. The caller of `run_am()`,\n`run_specific_rebase()` also handles it correctly.\n\n> \n> > @@ -466,6 +521,129 @@ N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> >  \"To abort and get back to the state before \\\"git rebase\\\", run \"\n> >  \"\\\"git rebase --abort\\\".\");\n> >  \n> > +static int run_am(struct rebase_options *opts)\n> > +{\n> > +\tstruct child_process am = CHILD_PROCESS_INIT;\n> > +\tstruct child_process format_patch = CHILD_PROCESS_INIT;\n> > +\tstruct strbuf revisions = STRBUF_INIT;\n> > +\tint status;\n> > +\tchar *rebased_patches;\n> > +\n> > +\tam.git_cmd = 1;\n> > +\targv_array_push(&am.args, \"am\");\n> > +\n> > +\tif (opts->action && !strcmp(\"continue\", opts->action)) {\n> > +\t\targv_array_push(&am.args, \"--resolved\");\n> > +\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n> > +\t\tif (opts->gpg_sign_opt)\n> > +\t\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n> > +\t\tstatus = run_command(&am);\n> > +\t\tif (status)\n> > +\t\t\treturn status;\n> > +\n> > +\t\tdiscard_cache();\n> > +\t\treturn move_to_original_branch(opts);\n> \n> It is curious why discard_cache() is placed exacly here, as if we\n> want to preserve the contents of the in-core index when\n> run_command() failed.  But I do not think we care about the in-core\n> index as the only thing that happen after \"return status\" is to\n> return the control to run_specific_rebase(), let it jump to\n> finished_rebase label to clean things up and rturn control to\n> cmd_rebase() and exit based on the status value.\n> \n> It's not like move_to_original_branch() wants to call read_cache()\n> and get the result from the \"am\" that run_command() executed,\n> either.\n> \n> Puzzled.  Care to explain a bit more in the in-code comment?\n\nI think that this call is just a left-over from a previous version that\ndid not have the REFS_ONLY flag to pass to `move_to_original_branch()`\n(and it caused havoc before that flag was passed). Let me double-check\nwhether the `discard_cache()` even makes sense any longer.\n\n*clicketyclick* indeed that is the case. Will remove all three\n`discard_cache()` calls.\n\n> \n> > +\t}\n> > +\tif (opts->action && !strcmp(\"skip\", opts->action)) {\n> > +\t\targv_array_push(&am.args, \"--skip\");\n> > +\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n> > +\t\tstatus = run_command(&am);\n> > +\t\tif (status)\n> > +\t\t\treturn status;\n> > +\n> > +\t\tdiscard_cache();\n> > +\t\treturn move_to_original_branch(opts);\n> \n> Ditto.\n> \n> > +\t}\n> > +\tif (opts->action && !strcmp(\"show-current-patch\", opts->action)) {\n> > +\t\targv_array_push(&am.args, \"--show-current-patch\");\n> > +\t\treturn run_command(&am);\n> > +\t}\n> \n> Up to this point, it is a faithful conversion of the first case/esac\n> statement.  Good.\n> \n> > +\tstrbuf_addf(&revisions, \"%s...%s\",\n> > +\t\t    oid_to_hex(opts->root ?\n> > +\t\t\t       /* this is now equivalent to ! -z \"$upstream\" */\n> \n> Does \"this\" refer to the \"opts->root being true\" check?\n> \n> Because you are flipping the polarity of the test from scripted\n> version, shouldn't the comment be updated to \"-z $upstream\"?\n\nIt did flip the polarity, you are right, this comment is incorrect. It is\neven more incorrect, though, as it talks about a shell construct that is\nno longer applicable. Will fix.\n\n> \n> > +\t\t\t       &opts->onto->object.oid :\n> > +\t\t\t       &opts->upstream->object.oid),\n> > +\t\t    oid_to_hex(&opts->orig_head));\n> \n> > +\trebased_patches = xstrdup(git_path(\"rebased-patches\"));\n> > +\tformat_patch.out = open(rebased_patches,\n> > +\t\t\t\tO_WRONLY | O_CREAT | O_TRUNC, 0666);\n> \n> Unlike scripted version, we do not remove a (possibly) existing file.\n> We give CREAT in case there is no existing one, and TRUNC in case\n> there is an existing one.  Makes sense.  A more faithful translation\n> would have unlink(2)ed a (possibly) existing one, and then because\n> we can afford to, passed O_EXCL to avoid stomping on somebody else\n> racing with us, but I do not think it is worth it.\n\nOkay.\n\n> > +\tif (format_patch.out < 0) {\n> > +\t\tstatus = error_errno(_(\"could not write '%s'\"),\n> > +\t\t\t\t     rebased_patches);\n> \n> s/write '%s'/open '%s' for writing/?  I dunno.\n\nYep, of course! Will fix.\n\n> > +\t\tfree(rebased_patches);\n> > +\t\targv_array_clear(&am.args);\n> > +\t\treturn status;\n> > +\t}\n> > +\n> > +\tformat_patch.git_cmd = 1;\n> > +\targv_array_pushl(&format_patch.args, \"format-patch\", \"-k\", \"--stdout\",\n> > +\t\t\t \"--full-index\", \"--cherry-pick\", \"--right-only\",\n> > +\t\t\t \"--src-prefix=a/\", \"--dst-prefix=b/\", \"--no-renames\",\n> > +\t\t\t \"--no-cover-letter\", \"--pretty=mboxrd\", NULL);\n> > +\tif (opts->git_format_patch_opt.len)\n> > +\t\targv_array_split(&format_patch.args,\n> > +\t\t\t\t opts->git_format_patch_opt.buf);\n> > +\targv_array_push(&format_patch.args, revisions.buf);\n> > +\tif (opts->restrict_revision)\n> > +\t\targv_array_pushf(&format_patch.args, \"^%s\",\n> > +\t\t\t\t oid_to_hex(&opts->restrict_revision->object.oid));\n> \n> It is kinda surprising to see that we have learned quite a lot of\n> fringe \"configurations\" we need to explicitly override like this.\n> \n> Looks like a quite faithful conversion, anyway.\n> \n> > +\tstatus = run_command(&format_patch);\n> > +\tif (status) {\n> > +\t\tunlink(rebased_patches);\n> > +\t\tfree(rebased_patches);\n> > +\t\targv_array_clear(&am.args);\n> > +\n> > +\t\treset_head(&opts->orig_head, \"checkout\", opts->head_name, 0,\n> > +\t\t\t   \"HEAD\", NULL);\n> \n> This one may need to trigger post-checkout hook.  The scripted\n> version does two different things depending on the value of\n> $head_name, but we can just use the same code without conditional?\n\nYes, because `opts->head_name` is `NULL` in one case, and not `NULL` in\nthe other, and the `reset_head()` function performs the desired operation\nin each case.\n\n> > +\t\terror(_(\"\\ngit encountered an error while preparing the \"\n> > +\t\t\t\"patches to replay\\n\"\n> > +\t\t\t\"these revisions:\\n\"\n> > +\t\t\t\"\\n    %s\\n\\n\"\n> > +\t\t\t\"As a result, git cannot rebase them.\"),\n> > +\t\t      opts->revisions);\n> > +\n> > +\t\tstrbuf_release(&revisions);\n> > +\t\treturn status;\n> > +\t}\n> > +\tstrbuf_release(&revisions);\n> > +\n> > +\tam.in = open(rebased_patches, O_RDONLY);\n> > +\tif (am.in < 0) {\n> > +\t\tstatus = error_errno(_(\"could not read '%s'\"),\n> > +\t\t\t\t     rebased_patches);\n> \n> s/write '%s'/open '%s' for reading/?  I dunno.\n\nYep, will fix.\n\n> \n> > +\t\tfree(rebased_patches);\n> > +\t\targv_array_clear(&am.args);\n> > +\t\treturn status;\n> > +\t}\n> > +\n> > +\targv_array_pushv(&am.args, opts->git_am_opts.argv);\n> > +\targv_array_push(&am.args, \"--rebasing\");\n> > +\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n> > +\targv_array_push(&am.args, \"--patch-format=mboxrd\");\n> > +\tif (opts->allow_rerere_autoupdate > 0)\n> > +\t\targv_array_push(&am.args, \"--rerere-autoupdate\");\n> > +\telse if (opts->allow_rerere_autoupdate == 0)\n> > +\t\targv_array_push(&am.args, \"--no-rerere-autoupdate\");\n> > +\tif (opts->gpg_sign_opt)\n> > +\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n> > +\tstatus = run_command(&am);\n> > +\tunlink(rebased_patches);\n> > +\tfree(rebased_patches);\n> > +\n> > +\tif (!status) {\n> > +\t\tdiscard_cache();\n> > +\t\treturn move_to_original_branch(opts);\n> > +\t}\n> > +\n> > +\tif (is_directory(opts->state_dir))\n> > +\t\twrite_basic_state(opts);\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> >  static int run_specific_rebase(struct rebase_options *opts)\n> >  {\n> >  \tconst char *argv[] = { NULL, NULL };\n> > @@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts)\n> >  \t\tgoto finished_rebase;\n> >  \t}\n> >  \n> > +\tif (opts->type == REBASE_AM) {\n> > +\t\tstatus = run_am(opts);\n> > +\t\tgoto finished_rebase;\n> > +\t}\n> > +\n> >  \tadd_var(&script_snippet, \"GIT_DIR\", absolute_path(get_git_dir()));\n> >  \tadd_var(&script_snippet, \"state_dir\", opts->state_dir);\n> \n> \n> Overall, this was quite a pleasant read and a well constructed\n> series.  Other than two minor points (i.e. interaction with the\n> 'post-checkout hook' topic, and discard_cache() before calling\n> move_to_original_branch) I did not quite understand, looks good to\n> me.\n> \n> When merged to 'pu', I seem to be getting failure from t3425.5, .8\n> and .11, by the way.  I haven't dug into the actual breakages any\n> further than that.\n\nSorry for the trouble, and for my silence (I was heads-down into the Azure\nPipelines support).\n\nI did not see any breakage in `pu` lately, hopefully things resolved\nthemselves?\n\nCiao,\nDscho\n"},{"id":"367098","messageId":"nycvar.QRO.7.76.6.1901181516240.41@tvgsbejvaqbjf.bet","threadId":"50095","inReplyTo":"xmqqpntcclot.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T14:18:02Z","receivedAt":"2019-01-18T14:18:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 4 Jan 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n> \n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > This is what the legacy (scripted) rebase does in\n> > `move_to_original_branch`, and we will need this functionality in the\n> > next commit.\n> \n> The move-to-original-branch helper does:\n> \n>     - point $head_name to the commit pointed at by HEAD\n>     - point HEAD symref to $head_name\n> \n> without touching the index or the working tree files.  It's not\n> exactly \"reset --soft\" but more like \"switch-branch --soft\" ;-)\n\nRight, but it does reset the HEAD softly.\n\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >  builtin/rebase.c | 8 +++++++-\n> >  1 file changed, 7 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 768bea0da8..303175fdf1 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n> >  \n> >  #define RESET_HEAD_DETACH (1<<0)\n> >  #define RESET_HEAD_HARD (1<<1)\n> > +#define RESET_HEAD_REFS_ONLY (1<<2)\n> \n> In the future codebase in 'pu', we have 1<<2 already taken by\n> another topic, so I'll tell my rerere database that the bit\n> assignment needs to be adjusted.\n\nOkay.\n\n> >  static int reset_head(struct object_id *oid, const char *action,\n> >  \t\t      const char *switch_to_branch, unsigned flags,\n> > @@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  {\n> >  \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n> >  \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n> > +\tunsigned refs_only = flags & RESET_HEAD_REFS_ONLY;\n> >  \tstruct object_id head_oid;\n> >  \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n> >  \tstruct lock_file lock = LOCK_INIT;\n> > @@ -359,7 +361,7 @@ 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> > +\tif (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n> >  \t\tret = -1;\n> >  \t\tgoto leave_reset_head;\n> >  \t}\n> \n> Not touching the index, so no need to lock the index.  Makes sense.\n> \n> > @@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  \tif (!oid)\n> >  \t\toid = &head_oid;\n> >  \n> > +\tif (flags & RESET_HEAD_REFS_ONLY)\n> > +\t\tgoto reset_head_refs;\n> > +\n> \n> Why not \"refs_only\" that we already prepared above???  Are we\n> munging that secondary variable before control comes here?\n\nNo, just an oversight. Will fix.\n\n> In any case, not touching the index nor the working tree, so no need\n> to call into the unpack_trees machinery.  Makes sense.\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> > @@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  \t\tgoto leave_reset_head;\n> >  \t}\n> >  \n> > +reset_head_refs:\n> >  \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n> \n> And the control continues from the point we update the reflog.\n> Makes sense.\n> \n> >  \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n> >  \tprefix_len = msg.len;\n> \n> This helper is touched by two other topics in flight, and that was\n> one of the reason why it took a bit longer than usual for me to\n> merge this topic.  Please sanity-check the result of the conflict\n> resolution at the tip of 'pu' branch.\n\nSorry, I *just now* had time to take care of this patch series. I will\ngladly check once you integrate the new patch series iteration.\n\nThanks,\nDscho\n"},{"id":"367099","messageId":"nycvar.QRO.7.76.6.1901181518350.41@tvgsbejvaqbjf.bet","threadId":"50095","inReplyTo":"xmqqimz4cloh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] rebase: avoid double reflog entry when switching branches","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T14:18:50Z","receivedAt":"2019-01-18T14:19:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 4 Jan 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n> \n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > When switching a branch *and* updating said branch to a different\n> > revision, let's avoid a double entry by first updating the branch and\n> > then adjusting the symbolic ref HEAD.\n> \n> Ah, in the original sequence, HEAD is updated twice, leaving two\n> reflog entries for HEAD (and one for the underlying \"switch_to\"\n> branch by virtue of REF_UPDATE_VIA_HEAD).  In the new sequence,\n> update_ref() updates the underlying \"switch_to\" and then HEAD, so\n> we'd get one reflog entry for each of them.\n> \n> Makes sense.  s/let's avoid a double entry/& in HEAD's reflog/ would\n> have avoided wasting reader's time who needlessly wondered where\n> that redundancy came from, though.\n\nWill fix the commit message.\n\nThanks!\nDscho\n\n> \n> >\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >  builtin/rebase.c | 7 ++++---\n> >  1 file changed, 4 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index e1dfa74ca8..768bea0da8 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action,\n> >  \t\t\t\t detach_head ? REF_NO_DEREF : 0,\n> >  \t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n> >  \telse {\n> > -\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n> > +\t\tret = update_ref(reflog_orig_head, switch_to_branch, oid,\n> > +\t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n> >  \t\tif (!ret)\n> > -\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n> > -\t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n> > +\t\t\tret = create_symref(\"HEAD\", switch_to_branch,\n> > +\t\t\t\t\t    reflog_head);\n> >  \t}\n> >  \n> >  leave_reset_head:\n> \n"},{"id":"367103","messageId":"pull.24.v2.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Let the builtin rebase call the git am command directly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-18T15:09:23Z","receivedAt":"2019-01-18T15:09:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Especially on Windows, where Unix shell scripting is a foreign endeavor, and\nan expensive one at that, we really want to avoid running through the Bash.\n\nThis not only makes everything faster, but also more robust, as the Bash we\nuse on Windows relies on a derivative of the Cygwin runtime, which in turn\nhas to jump through a couple of hoops that are sometimes a little too tricky\nto make things work. Read: the less we rely on Unix shell scripting, the\nmore likely Windows users will be able to enjoy our software.\n\nChanges since v1:\n\n * Rebased on top of master to avoid merge conflicts.\n * Adjusted the commit message talking about double entries, to clarify that\n   it talks about HEAD's reflog.\n * Replaced a misleading action parameter \"checkout\" for the reset_head() \n   function by the empty string: we do not check out here, we just update\n   the refs, and certainly do not want any checkout functionality (such as\n   hooks) to be involved.\n * Reused a just-prepared refs_only variable instead of repeating the value\n   assigned to it.\n * Fixed a stale comment about the shell variable \"$upstream\" (which should\n   have been negated to begin with).\n * Fixed error messages when files could not be opened.\n\nJohannes Schindelin (4):\n  rebase: move `reset_head()` into a better spot\n  rebase: avoid double reflog entry when switching branches\n  rebase: teach `reset_head()` to optionally skip the worktree\n  built-in rebase: call `git am` directly\n\n builtin/rebase.c | 424 ++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 305 insertions(+), 119 deletions(-)\n\n\nbase-commit: 77556354bb7ac50450e3b28999e3576969869068\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-24%2Fdscho%2Fbuiltin-rebase--am-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-24/dscho/builtin-rebase--am-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/24\n\nRange-diff vs v1:\n\n 1:  175dc7d29a ! 1:  e886ae3de5 rebase: move `reset_head()` into a better spot\n     @@ -91,7 +91,7 @@\n      +\t}\n      +\n      +\ttree = parse_tree_indirect(oid);\n     -+\tprime_cache_tree(the_repository->index, tree);\n     ++\tprime_cache_tree(the_repository, the_repository->index, tree);\n      +\n      +\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {\n      +\t\tret = error(_(\"could not write index\"));\n     @@ -217,7 +217,7 @@\n      -\t}\n      -\n      -\ttree = parse_tree_indirect(oid);\n     --\tprime_cache_tree(the_repository->index, tree);\n     +-\tprime_cache_tree(the_repository, the_repository->index, tree);\n      -\n      -\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {\n      -\t\tret = error(_(\"could not write index\"));\n 2:  4c5f87b9dc ! 2:  3a68f1c509 rebase: avoid double reflog entry when switching branches\n     @@ -3,8 +3,8 @@\n          rebase: avoid double reflog entry when switching branches\n      \n          When switching a branch *and* updating said branch to a different\n     -    revision, let's avoid a double entry by first updating the branch and\n     -    then adjusting the symbolic ref HEAD.\n     +    revision, let's avoid a double entry in HEAD's reflog by first updating\n     +    the branch and then adjusting the symbolic ref HEAD.\n      \n          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n 3:  21939140e0 ! 3:  387071dcd7 rebase: teach `reset_head()` to optionally skip the worktree\n     @@ -40,7 +40,7 @@\n       \tif (!oid)\n       \t\toid = &head_oid;\n       \n     -+\tif (flags & RESET_HEAD_REFS_ONLY)\n     ++\tif (refs_only)\n      +\t\tgoto reset_head_refs;\n      +\n       \tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n 4:  2b5ece8263 ! 4:  3c40318682 built-in rebase: call `git am` directly\n     @@ -16,6 +16,10 @@\n          itself a derivative of the Cygwin runtime): when no shell script is\n          called, the POSIX emulation layer is avoided altogether.\n      \n     +    Note: we pass an empty action to `reset_head()` here when moving back to\n     +    the original branch, as no other action is applicable, really. This\n     +    parameter is used to initialize `unpack_trees()`' messages.\n     +\n          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n       diff --git a/builtin/rebase.c b/builtin/rebase.c\n     @@ -78,8 +82,7 @@\n      +\t\t    opts->head_name, oid_to_hex(&opts->onto->object.oid));\n      +\tstrbuf_addf(&head_reflog, \"rebase finished: returning to %s\",\n      +\t\t    opts->head_name);\n     -+\tret = reset_head(NULL, \"checkout\", opts->head_name,\n     -+\t\t\t RESET_HEAD_REFS_ONLY,\n     ++\tret = reset_head(NULL, \"\", opts->head_name, RESET_HEAD_REFS_ONLY,\n      +\t\t\t orig_head_reflog.buf, head_reflog.buf);\n      +\n      +\tstrbuf_release(&orig_head_reflog);\n     @@ -114,7 +117,6 @@\n      +\t\tif (status)\n      +\t\t\treturn status;\n      +\n     -+\t\tdiscard_cache();\n      +\t\treturn move_to_original_branch(opts);\n      +\t}\n      +\tif (opts->action && !strcmp(\"skip\", opts->action)) {\n     @@ -124,7 +126,6 @@\n      +\t\tif (status)\n      +\t\t\treturn status;\n      +\n     -+\t\tdiscard_cache();\n      +\t\treturn move_to_original_branch(opts);\n      +\t}\n      +\tif (opts->action && !strcmp(\"show-current-patch\", opts->action)) {\n     @@ -134,7 +135,7 @@\n      +\n      +\tstrbuf_addf(&revisions, \"%s...%s\",\n      +\t\t    oid_to_hex(opts->root ?\n     -+\t\t\t       /* this is now equivalent to ! -z \"$upstream\" */\n     ++\t\t\t       /* this is now equivalent to !opts->upstream */\n      +\t\t\t       &opts->onto->object.oid :\n      +\t\t\t       &opts->upstream->object.oid),\n      +\t\t    oid_to_hex(&opts->orig_head));\n     @@ -143,7 +144,7 @@\n      +\tformat_patch.out = open(rebased_patches,\n      +\t\t\t\tO_WRONLY | O_CREAT | O_TRUNC, 0666);\n      +\tif (format_patch.out < 0) {\n     -+\t\tstatus = error_errno(_(\"could not write '%s'\"),\n     ++\t\tstatus = error_errno(_(\"could not open '%s' for writing\"),\n      +\t\t\t\t     rebased_patches);\n      +\t\tfree(rebased_patches);\n      +\t\targv_array_clear(&am.args);\n     @@ -185,7 +186,7 @@\n      +\n      +\tam.in = open(rebased_patches, O_RDONLY);\n      +\tif (am.in < 0) {\n     -+\t\tstatus = error_errno(_(\"could not read '%s'\"),\n     ++\t\tstatus = error_errno(_(\"could not open '%s' for reading\"),\n      +\t\t\t\t     rebased_patches);\n      +\t\tfree(rebased_patches);\n      +\t\targv_array_clear(&am.args);\n     @@ -207,7 +208,6 @@\n      +\tfree(rebased_patches);\n      +\n      +\tif (!status) {\n     -+\t\tdiscard_cache();\n      +\t\treturn move_to_original_branch(opts);\n      +\t}\n      +\n\n-- \ngitgitgadget\n"},{"id":"367104","messageId":"e886ae3de50cdc200b50781aaa4746535d3a8589.1547824162.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] rebase: move `reset_head()` into a better spot","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-18T15:09:24Z","receivedAt":"2019-01-18T15:09:30Z","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\nOver the next commits, we want to make use of it in `run_am()` (i.e.\nrunning the `--am` backend directly, without detouring to Unix shell\nscript code) which in turn will be called from `run_specific_rebase()`.\n\nSo let's move it before that latter function.\n\nThis commit is best viewed using --color-moved.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 238 +++++++++++++++++++++++------------------------\n 1 file changed, 119 insertions(+), 119 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 00de70365e..efa90ca894 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -333,6 +333,125 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n \t}\n }\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, 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[2] = { { NULL }, { NULL } };\n+\tstruct lock_file lock = LOCK_INIT;\n+\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct tree *tree;\n+\tconst char *reflog_action;\n+\tstruct strbuf msg = STRBUF_INIT;\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, 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+\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 || !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 = reset_hard ? oneway_merge : twoway_merge;\n+\tunpack_tree_opts.update = 1;\n+\tunpack_tree_opts.merge = 1;\n+\tif (!detach_head)\n+\t\tunpack_tree_opts.reset = 1;\n+\n+\tif (read_index_unmerged(the_repository->index) < 0) {\n+\t\tret = error(_(\"could not read index\"));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n+\tif (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {\n+\t\tret = error(_(\"failed to find tree of %s\"),\n+\t\t\t    oid_to_hex(&head_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(nr, desc, &unpack_tree_opts)) {\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, the_repository->index, tree);\n+\n+\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {\n+\t\tret = error(_(\"could not write index\"));\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+\tprefix_len = msg.len;\n+\n+\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n+\t\told_orig = &oid_old_orig;\n+\tif (!get_oid(\"HEAD\", &oid_orig)) {\n+\t\torig = &oid_orig;\n+\t\tif (!reflog_orig_head) {\n+\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n+\t\t\treflog_orig_head = msg.buf;\n+\t\t}\n+\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig, old_orig, 0,\n+\t\t\t   UPDATE_REFS_MSG_ON_ERR);\n+\t} else if (old_orig)\n+\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n+\tif (!reflog_head) {\n+\t\tstrbuf_setlen(&msg, prefix_len);\n+\t\tstrbuf_addstr(&msg, \"updating HEAD\");\n+\t\treflog_head = msg.buf;\n+\t}\n+\tif (!switch_to_branch)\n+\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig,\n+\t\t\t\t detach_head ? REF_NO_DEREF : 0,\n+\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n+\telse {\n+\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n+\t\tif (!ret)\n+\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\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+\twhile (nr)\n+\t\tfree((void *)desc[--nr].buffer);\n+\treturn ret;\n+}\n+\n static const char *resolvemsg =\n N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n \"\\\"git add/rm <conflicted_files>\\\", then run \\\"git rebase --continue\\\".\\n\"\n@@ -526,125 +645,6 @@ static int run_specific_rebase(struct rebase_options *opts)\n \treturn status ? -1 : 0;\n }\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, 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[2] = { { NULL }, { NULL } };\n-\tstruct lock_file lock = LOCK_INIT;\n-\tstruct unpack_trees_options unpack_tree_opts;\n-\tstruct tree *tree;\n-\tconst char *reflog_action;\n-\tstruct strbuf msg = STRBUF_INIT;\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, 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-\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 || !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 = reset_hard ? oneway_merge : twoway_merge;\n-\tunpack_tree_opts.update = 1;\n-\tunpack_tree_opts.merge = 1;\n-\tif (!detach_head)\n-\t\tunpack_tree_opts.reset = 1;\n-\n-\tif (read_index_unmerged(the_repository->index) < 0) {\n-\t\tret = error(_(\"could not read index\"));\n-\t\tgoto leave_reset_head;\n-\t}\n-\n-\tif (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {\n-\t\tret = error(_(\"failed to find tree of %s\"),\n-\t\t\t    oid_to_hex(&head_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(nr, desc, &unpack_tree_opts)) {\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, the_repository->index, tree);\n-\n-\tif (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {\n-\t\tret = error(_(\"could not write index\"));\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-\tprefix_len = msg.len;\n-\n-\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n-\t\told_orig = &oid_old_orig;\n-\tif (!get_oid(\"HEAD\", &oid_orig)) {\n-\t\torig = &oid_orig;\n-\t\tif (!reflog_orig_head) {\n-\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n-\t\t\treflog_orig_head = msg.buf;\n-\t\t}\n-\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig, old_orig, 0,\n-\t\t\t   UPDATE_REFS_MSG_ON_ERR);\n-\t} else if (old_orig)\n-\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n-\tif (!reflog_head) {\n-\t\tstrbuf_setlen(&msg, prefix_len);\n-\t\tstrbuf_addstr(&msg, \"updating HEAD\");\n-\t\treflog_head = msg.buf;\n-\t}\n-\tif (!switch_to_branch)\n-\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig,\n-\t\t\t\t detach_head ? REF_NO_DEREF : 0,\n-\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n-\telse {\n-\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n-\t\tif (!ret)\n-\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\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-\twhile (nr)\n-\t\tfree((void *)desc[--nr].buffer);\n-\treturn ret;\n-}\n-\n static int rebase_config(const char *var, const char *value, void *data)\n {\n \tstruct rebase_options *opts = data;\n-- \ngitgitgadget\n\n"},{"id":"367105","messageId":"3a68f1c509a357447bddf8bae6f1685b47673f89.1547824162.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] rebase: avoid double reflog entry when switching branches","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-18T15:09:25Z","receivedAt":"2019-01-18T15:09:31Z","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 switching a branch *and* updating said branch to a different\nrevision, let's avoid a double entry in HEAD's reflog by first updating\nthe branch and then adjusting the symbolic ref HEAD.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex efa90ca894..5c827b2f03 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\t\t\t detach_head ? REF_NO_DEREF : 0,\n \t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \telse {\n-\t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\n+\t\tret = update_ref(reflog_orig_head, switch_to_branch, oid,\n+\t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n \t\tif (!ret)\n-\t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n-\t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n+\t\t\tret = create_symref(\"HEAD\", switch_to_branch,\n+\t\t\t\t\t    reflog_head);\n \t}\n \n leave_reset_head:\n-- \ngitgitgadget\n\n"},{"id":"367106","messageId":"387071dcd77fde6ceef1daea623b4ae2e0e08166.1547824162.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] rebase: teach `reset_head()` to optionally skip the worktree","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-18T15:09:26Z","receivedAt":"2019-01-18T15:09:32Z","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\nThis is what the legacy (scripted) rebase does in\n`move_to_original_branch`, and we will need this functionality in the\nnext commit.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5c827b2f03..c65493a484 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n \n #define RESET_HEAD_DETACH (1<<0)\n #define RESET_HEAD_HARD (1<<1)\n+#define RESET_HEAD_REFS_ONLY (1<<2)\n \n static int reset_head(struct object_id *oid, const char *action,\n \t\t      const char *switch_to_branch, unsigned flags,\n@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,\n {\n \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n+\tunsigned refs_only = flags & RESET_HEAD_REFS_ONLY;\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n@@ -359,7 +361,7 @@ 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+\tif (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {\n \t\tret = -1;\n \t\tgoto leave_reset_head;\n \t}\n@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,\n \tif (!oid)\n \t\toid = &head_oid;\n \n+\tif (refs_only)\n+\t\tgoto reset_head_refs;\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@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\tgoto leave_reset_head;\n \t}\n \n+reset_head_refs:\n \treflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n \tprefix_len = msg.len;\n-- \ngitgitgadget\n\n"},{"id":"367107","messageId":"3c4031868204862ea4e2a53a03cd8d7ec066a891.1547824162.git.gitgitgadget@gmail.com","threadId":"50095","inReplyTo":"pull.24.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] built-in rebase: call `git am` directly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-18T15:09:27Z","receivedAt":"2019-01-18T15:09: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\nWhile the scripted `git rebase` still has to rely on the\n`git-rebase--am.sh` script to implement the glue between the `rebase`\nand the `am` commands, we can go a more direct route in the built-in\nrebase and avoid using a shell script altogether.\n\nThis patch represents a straight-forward port of `git-rebase--am.sh` to\nC, along with the glue code to call it directly from within\n`builtin/rebase.c`.\n\nThis reduces the chances of Git for Windows running into trouble due to\nproblems with the POSIX emulation layer (known as \"MSYS2 runtime\",\nitself a derivative of the Cygwin runtime): when no shell script is\ncalled, the POSIX emulation layer is avoided altogether.\n\nNote: we pass an empty action to `reset_head()` here when moving back to\nthe original branch, as no other action is applicable, really. This\nparameter is used to initialize `unpack_trees()`' messages.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/rebase.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 179 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex c65493a484..0727ddaf00 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -246,6 +246,37 @@ static int read_basic_state(struct rebase_options *opts)\n \treturn 0;\n }\n \n+static int write_basic_state(struct rebase_options *opts)\n+{\n+\twrite_file(state_dir_path(\"head-name\", opts), \"%s\",\n+\t\t   opts->head_name ? opts->head_name : \"detached HEAD\");\n+\twrite_file(state_dir_path(\"onto\", opts), \"%s\",\n+\t\t   opts->onto ? oid_to_hex(&opts->onto->object.oid) : \"\");\n+\twrite_file(state_dir_path(\"orig-head\", opts), \"%s\",\n+\t\t   oid_to_hex(&opts->orig_head));\n+\twrite_file(state_dir_path(\"quiet\", opts), \"%s\",\n+\t\t   opts->flags & REBASE_NO_QUIET ? \"\" : \"t\");\n+\tif (opts->flags & REBASE_VERBOSE)\n+\t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n+\tif (opts->strategy)\n+\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n+\t\t\t   opts->strategy);\n+\tif (opts->strategy_opts)\n+\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n+\t\t\t   opts->strategy_opts);\n+\tif (opts->allow_rerere_autoupdate >= 0)\n+\t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n+\t\t\t   \"-%s-rerere-autoupdate\",\n+\t\t\t   opts->allow_rerere_autoupdate ? \"\" : \"-no\");\n+\tif (opts->gpg_sign_opt)\n+\t\twrite_file(state_dir_path(\"gpg_sign_opt\", opts), \"%s\",\n+\t\t\t   opts->gpg_sign_opt);\n+\tif (opts->signoff)\n+\t\twrite_file(state_dir_path(\"strategy\", opts), \"--signoff\");\n+\n+\treturn 0;\n+}\n+\n static int apply_autostash(struct rebase_options *opts)\n {\n \tconst char *path = state_dir_path(\"autostash\", opts);\n@@ -459,6 +490,29 @@ static int reset_head(struct object_id *oid, const char *action,\n \treturn ret;\n }\n \n+static int move_to_original_branch(struct rebase_options *opts)\n+{\n+\tstruct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (!opts->head_name)\n+\t\treturn 0; /* nothing to move back to */\n+\n+\tif (!opts->onto)\n+\t\tBUG(\"move_to_original_branch without onto\");\n+\n+\tstrbuf_addf(&orig_head_reflog, \"rebase finished: %s onto %s\",\n+\t\t    opts->head_name, oid_to_hex(&opts->onto->object.oid));\n+\tstrbuf_addf(&head_reflog, \"rebase finished: returning to %s\",\n+\t\t    opts->head_name);\n+\tret = reset_head(NULL, \"\", opts->head_name, RESET_HEAD_REFS_ONLY,\n+\t\t\t orig_head_reflog.buf, head_reflog.buf);\n+\n+\tstrbuf_release(&orig_head_reflog);\n+\tstrbuf_release(&head_reflog);\n+\treturn ret;\n+}\n+\n static const char *resolvemsg =\n N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n \"\\\"git add/rm <conflicted_files>\\\", then run \\\"git rebase --continue\\\".\\n\"\n@@ -466,6 +520,126 @@ N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n \"To abort and get back to the state before \\\"git rebase\\\", run \"\n \"\\\"git rebase --abort\\\".\");\n \n+static int run_am(struct rebase_options *opts)\n+{\n+\tstruct child_process am = CHILD_PROCESS_INIT;\n+\tstruct child_process format_patch = CHILD_PROCESS_INIT;\n+\tstruct strbuf revisions = STRBUF_INIT;\n+\tint status;\n+\tchar *rebased_patches;\n+\n+\tam.git_cmd = 1;\n+\targv_array_push(&am.args, \"am\");\n+\n+\tif (opts->action && !strcmp(\"continue\", opts->action)) {\n+\t\targv_array_push(&am.args, \"--resolved\");\n+\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\t\tif (opts->gpg_sign_opt)\n+\t\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n+\t\tstatus = run_command(&am);\n+\t\tif (status)\n+\t\t\treturn status;\n+\n+\t\treturn move_to_original_branch(opts);\n+\t}\n+\tif (opts->action && !strcmp(\"skip\", opts->action)) {\n+\t\targv_array_push(&am.args, \"--skip\");\n+\t\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\t\tstatus = run_command(&am);\n+\t\tif (status)\n+\t\t\treturn status;\n+\n+\t\treturn move_to_original_branch(opts);\n+\t}\n+\tif (opts->action && !strcmp(\"show-current-patch\", opts->action)) {\n+\t\targv_array_push(&am.args, \"--show-current-patch\");\n+\t\treturn run_command(&am);\n+\t}\n+\n+\tstrbuf_addf(&revisions, \"%s...%s\",\n+\t\t    oid_to_hex(opts->root ?\n+\t\t\t       /* this is now equivalent to !opts->upstream */\n+\t\t\t       &opts->onto->object.oid :\n+\t\t\t       &opts->upstream->object.oid),\n+\t\t    oid_to_hex(&opts->orig_head));\n+\n+\trebased_patches = xstrdup(git_path(\"rebased-patches\"));\n+\tformat_patch.out = open(rebased_patches,\n+\t\t\t\tO_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\tif (format_patch.out < 0) {\n+\t\tstatus = error_errno(_(\"could not open '%s' for writing\"),\n+\t\t\t\t     rebased_patches);\n+\t\tfree(rebased_patches);\n+\t\targv_array_clear(&am.args);\n+\t\treturn status;\n+\t}\n+\n+\tformat_patch.git_cmd = 1;\n+\targv_array_pushl(&format_patch.args, \"format-patch\", \"-k\", \"--stdout\",\n+\t\t\t \"--full-index\", \"--cherry-pick\", \"--right-only\",\n+\t\t\t \"--src-prefix=a/\", \"--dst-prefix=b/\", \"--no-renames\",\n+\t\t\t \"--no-cover-letter\", \"--pretty=mboxrd\", NULL);\n+\tif (opts->git_format_patch_opt.len)\n+\t\targv_array_split(&format_patch.args,\n+\t\t\t\t opts->git_format_patch_opt.buf);\n+\targv_array_push(&format_patch.args, revisions.buf);\n+\tif (opts->restrict_revision)\n+\t\targv_array_pushf(&format_patch.args, \"^%s\",\n+\t\t\t\t oid_to_hex(&opts->restrict_revision->object.oid));\n+\n+\tstatus = run_command(&format_patch);\n+\tif (status) {\n+\t\tunlink(rebased_patches);\n+\t\tfree(rebased_patches);\n+\t\targv_array_clear(&am.args);\n+\n+\t\treset_head(&opts->orig_head, \"checkout\", opts->head_name, 0,\n+\t\t\t   \"HEAD\", NULL);\n+\t\terror(_(\"\\ngit encountered an error while preparing the \"\n+\t\t\t\"patches to replay\\n\"\n+\t\t\t\"these revisions:\\n\"\n+\t\t\t\"\\n    %s\\n\\n\"\n+\t\t\t\"As a result, git cannot rebase them.\"),\n+\t\t      opts->revisions);\n+\n+\t\tstrbuf_release(&revisions);\n+\t\treturn status;\n+\t}\n+\tstrbuf_release(&revisions);\n+\n+\tam.in = open(rebased_patches, O_RDONLY);\n+\tif (am.in < 0) {\n+\t\tstatus = error_errno(_(\"could not open '%s' for reading\"),\n+\t\t\t\t     rebased_patches);\n+\t\tfree(rebased_patches);\n+\t\targv_array_clear(&am.args);\n+\t\treturn status;\n+\t}\n+\n+\targv_array_pushv(&am.args, opts->git_am_opts.argv);\n+\targv_array_push(&am.args, \"--rebasing\");\n+\targv_array_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\targv_array_push(&am.args, \"--patch-format=mboxrd\");\n+\tif (opts->allow_rerere_autoupdate > 0)\n+\t\targv_array_push(&am.args, \"--rerere-autoupdate\");\n+\telse if (opts->allow_rerere_autoupdate == 0)\n+\t\targv_array_push(&am.args, \"--no-rerere-autoupdate\");\n+\tif (opts->gpg_sign_opt)\n+\t\targv_array_push(&am.args, opts->gpg_sign_opt);\n+\tstatus = run_command(&am);\n+\tunlink(rebased_patches);\n+\tfree(rebased_patches);\n+\n+\tif (!status) {\n+\t\treturn move_to_original_branch(opts);\n+\t}\n+\n+\tif (is_directory(opts->state_dir))\n+\t\twrite_basic_state(opts);\n+\n+\treturn status;\n+}\n+\n static int run_specific_rebase(struct rebase_options *opts)\n {\n \tconst char *argv[] = { NULL, NULL };\n@@ -546,6 +720,11 @@ static int run_specific_rebase(struct rebase_options *opts)\n \t\tgoto finished_rebase;\n \t}\n \n+\tif (opts->type == REBASE_AM) {\n+\t\tstatus = run_am(opts);\n+\t\tgoto finished_rebase;\n+\t}\n+\n \tadd_var(&script_snippet, \"GIT_DIR\", absolute_path(get_git_dir()));\n \tadd_var(&script_snippet, \"state_dir\", opts->state_dir);\n \n-- \ngitgitgadget\n"},{"id":"367120","messageId":"xmqqimylludk.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"nycvar.QRO.7.76.6.1901181443350.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] built-in rebase: call `git am` directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-18T17:59:19Z","receivedAt":"2019-01-18T17:59:23Z","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>> Overall, this was quite a pleasant read and a well constructed\n>> series.  Other than two minor points (i.e. interaction with the\n>> 'post-checkout hook' topic, and discard_cache() before calling\n>> move_to_original_branch) I did not quite understand, looks good to\n>> me.\n>> \n>> When merged to 'pu', I seem to be getting failure from t3425.5, .8\n>> and .11, by the way.  I haven't dug into the actual breakages any\n>> further than that.\n>\n> Sorry for the trouble, and for my silence (I was heads-down into the Azure\n> Pipelines support).\n>\n> I did not see any breakage in `pu` lately, hopefully things resolved\n> themselves?\n\nThe (semantic) conflict resolution can be seen in\n\n    $ git show 'origin/pu^{/^Merge branch .js/rebase-am. into}'\n\nwhich is recorded in my rerere database ;-)\n"},{"id":"367121","messageId":"xmqqef99lu2c.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"pull.24.v2.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] Let the builtin rebase call the git am command directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-18T18:06:03Z","receivedAt":"2019-01-18T18:06:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> Especially on Windows, where Unix shell scripting is a foreign endeavor, and\n> an expensive one at that, we really want to avoid running through the Bash.\n>\n> This not only makes everything faster, but also more robust, as the Bash we\n> use on Windows relies on a derivative of the Cygwin runtime, which in turn\n> has to jump through a couple of hoops that are sometimes a little too tricky\n> to make things work. Read: the less we rely on Unix shell scripting, the\n> more likely Windows users will be able to enjoy our software.\n>\n> Changes since v1:\n>\n>  * Rebased on top of master to avoid merge conflicts.\n\nI do not appreciate this very much.  \n\nWe already have a good resolution prepared when merging this to\neither 'next' or 'master', which also resolves conflict with the\nother topic that requires this topic to add \"--topo-order\" in its\ncall.  Rebasing series means invalidating the previous work recorded\nin rerere.\n\n\tside note. The rerere database entry can be recovered from\n\tmaster..pu with contrib/rerere-train.sh).\n\n>  * Adjusted the commit message talking about double entries, to clarify that\n>    it talks about HEAD's reflog.\n\nGood.\n\n>  * Replaced a misleading action parameter \"checkout\" for the reset_head() \n>    function by the empty string: we do not check out here, we just update\n>    the refs, and certainly do not want any checkout functionality (such as\n>    hooks) to be involved.\n\nOK.\n\n>  * Reused a just-prepared refs_only variable instead of repeating the value\n>    assigned to it.\n\nOK.\n\n>  * Fixed a stale comment about the shell variable \"$upstream\" (which should\n>    have been negated to begin with).\n>  * Fixed error messages when files could not be opened.\n\nGood.\n\nWill take a look.  Thanks.\n\n"},{"id":"367123","messageId":"nycvar.QRO.7.76.6.1901181912460.41@tvgsbejvaqbjf.bet","threadId":"50095","inReplyTo":"xmqqimylludk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/4] built-in rebase: call `git am` directly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T18:14:03Z","receivedAt":"2019-01-18T18:14:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 18 Jan 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> Overall, this was quite a pleasant read and a well constructed\n> >> series.  Other than two minor points (i.e. interaction with the\n> >> 'post-checkout hook' topic, and discard_cache() before calling\n> >> move_to_original_branch) I did not quite understand, looks good to\n> >> me.\n> >> \n> >> When merged to 'pu', I seem to be getting failure from t3425.5, .8\n> >> and .11, by the way.  I haven't dug into the actual breakages any\n> >> further than that.\n> >\n> > Sorry for the trouble, and for my silence (I was heads-down into the Azure\n> > Pipelines support).\n> >\n> > I did not see any breakage in `pu` lately, hopefully things resolved\n> > themselves?\n> \n> The (semantic) conflict resolution can be seen in\n> \n>     $ git show 'origin/pu^{/^Merge branch .js/rebase-am. into}'\n> \n> which is recorded in my rerere database ;-)\n\nThat looks all good to me, with the minor nit that you did not wrap at\n<= 80 columns/row:\n\n -                       \"--no-cover-letter\", \"--pretty=mboxrd\", NULL);\n++                       \"--no-cover-letter\", \"--pretty=mboxrd\", \"--topo-order\", NULL);\n\nI am not a stickler to this line length rule, though, as I long switched\naway from the 80x25 screen and can easily resize my text windows.\n\nCiao,\nDscho\n"},{"id":"367137","messageId":"xmqqa7jxk9ls.fsf@gitster-ct.c.googlers.com","threadId":"50095","inReplyTo":"xmqqef99lu2c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/4] Let the builtin rebase call the git am command directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-18T20:13:19Z","receivedAt":"2019-01-18T20:13:25Z","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 via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n>> Especially on Windows, where Unix shell scripting is a foreign endeavor, and\n>> an expensive one at that, we really want to avoid running through the Bash.\n>>\n>> This not only makes everything faster, but also more robust, as the Bash we\n>> use on Windows relies on a derivative of the Cygwin runtime, which in turn\n>> has to jump through a couple of hoops that are sometimes a little too tricky\n>> to make things work. Read: the less we rely on Unix shell scripting, the\n>> more likely Windows users will be able to enjoy our software.\n>>\n>> Changes since v1:\n>>\n>>  * Rebased on top of master to avoid merge conflicts.\n>\n> I do not appreciate this very much.  \n>\n> We already have a good resolution prepared when merging this to\n> either 'next' or 'master', which also resolves conflict with the\n> other topic that requires this topic to add \"--topo-order\" in its\n> call.  Rebasing series means invalidating the previous work recorded\n> in rerere.\n>\n> \tside note. The rerere database entry can be recovered from\n> \tmaster..pu with contrib/rerere-train.sh).\n\nI prepared a set of patches rebased back on top of the previous base\nand applied them on top of the previous base (call the result X).\n\nApplying the posted patches directly on top of master, and merging X\ninto master, produces the same tree.\n\nSo I'll keep the X (i.e. these patches backward-rebased to the\nprevious base) and replace js/rebase-am with it.  That is safer when\nrecreating 'pu' and merging it to 'next', which I hope we can do\nsoonish.\n\n> Will take a look.  Thanks.\n\nThey were pleasant and smooth read.  Crafted nicely.\n\nThanks.\n\n\n"},{"id":"367138","messageId":"nycvar.QRO.7.76.6.1901182128040.41@tvgsbejvaqbjf.bet","threadId":"50095","inReplyTo":"xmqqef99lu2c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/4] Let the builtin rebase call the git am command directly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T20:31:05Z","receivedAt":"2019-01-18T20:31:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 18 Jan 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n> \n> > Especially on Windows, where Unix shell scripting is a foreign endeavor, and\n> > an expensive one at that, we really want to avoid running through the Bash.\n> >\n> > This not only makes everything faster, but also more robust, as the Bash we\n> > use on Windows relies on a derivative of the Cygwin runtime, which in turn\n> > has to jump through a couple of hoops that are sometimes a little too tricky\n> > to make things work. Read: the less we rely on Unix shell scripting, the\n> > more likely Windows users will be able to enjoy our software.\n> >\n> > Changes since v1:\n> >\n> >  * Rebased on top of master to avoid merge conflicts.\n> \n> I do not appreciate this very much.  \n> \n> We already have a good resolution prepared when merging this to\n> either 'next' or 'master', which also resolves conflict with the\n> other topic that requires this topic to add \"--topo-order\" in its\n> call.  Rebasing series means invalidating the previous work recorded\n> in rerere.\n\nYou misunderstand. The PR had merge conflicts with `master`. As such, the\nAzure Pipeline that tests it on Windows, macOS and Linux could not give me\nenough confidence. So I *had* to rebase.\n\nBesides, I think it is a terrible idea to leave the resolution of merge\nconflicts to you. You are not in the best position to judge how to resolve\nthem, and as a consequence you spend more time and effort on this than\nnecessary. Of course, it's your call if you want to do it, as a\ncontributor I'd rather be certain that *I* resolved the conflicts.\n\nCiao,\nDscho\n\n> \tside note. The rerere database entry can be recovered from\n> \tmaster..pu with contrib/rerere-train.sh).\n> \n> >  * Adjusted the commit message talking about double entries, to clarify that\n> >    it talks about HEAD's reflog.\n> \n> Good.\n> \n> >  * Replaced a misleading action parameter \"checkout\" for the reset_head() \n> >    function by the empty string: we do not check out here, we just update\n> >    the refs, and certainly do not want any checkout functionality (such as\n> >    hooks) to be involved.\n> \n> OK.\n> \n> >  * Reused a just-prepared refs_only variable instead of repeating the value\n> >    assigned to it.\n> \n> OK.\n> \n> >  * Fixed a stale comment about the shell variable \"$upstream\" (which should\n> >    have been negated to begin with).\n> >  * Fixed error messages when files could not be opened.\n> \n> Good.\n> \n> Will take a look.  Thanks.\n> \n> \n"},{"id":"367719","messageId":"dc6830ab-d31c-fb9b-4f8d-24a79a978764@gmail.com","threadId":"50095","inReplyTo":"3c4031868204862ea4e2a53a03cd8d7ec066a891.1547824162.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] built-in rebase: call `git am` directly","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-01-26T21:13:47Z","receivedAt":"2019-01-26T21:13:52Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Johannes,\n\nLe 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :\n> -%<-\n> +static int run_am(struct rebase_options *opts)\n> +{\n> -%<-\n> +\n> +\tif (!status) {\n> +\t\treturn move_to_original_branch(opts);\n> +\t}\n> +\n> +\tif (is_directory(opts->state_dir))\n> +\t\twrite_basic_state(opts);\n> +\n> +\treturn status;\n> +}\n> +\n\nThis means that the state directory will be created only if there is a\nproblem (ie. a conflict), not if the user cancels the rebase by hitting\n^C.  In this case, the user will be left in a corrupted state where the\nbuiltin rebase cannot abort the rebase properly.\n\n-- Alban\n\n"},{"id":"367937","messageId":"nycvar.QRO.7.76.6.1901291033180.41@tvgsbejvaqbjf.bet","threadId":"50095","inReplyTo":"dc6830ab-d31c-fb9b-4f8d-24a79a978764@gmail.com","subject":"Re: [PATCH v2 4/4] built-in rebase: call `git am` directly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-29T09:46:13Z","receivedAt":"2019-01-29T09:46:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Sat, 26 Jan 2019, Alban Gruin wrote:\n\n> Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :\n> > -%<-\n> > +static int run_am(struct rebase_options *opts)\n> > +{\n> > -%<-\n> > +\n> > +\tif (!status) {\n> > +\t\treturn move_to_original_branch(opts);\n> > +\t}\n> > +\n> > +\tif (is_directory(opts->state_dir))\n> > +\t\twrite_basic_state(opts);\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> \n> This means that the state directory will be created only if there is a\n> problem (ie. a conflict), not if the user cancels the rebase by hitting\n> ^C.  In this case, the user will be left in a corrupted state where the\n> builtin rebase cannot abort the rebase properly.\n\nThis is an almost literal translation of\nhttps://github.com/git/git/blob/v2.20.1/git-rebase--am.sh#L77-L83:\n\n```sh\nif test 0 != $ret\nthen\n        test -d \"$state_dir\" && write_basic_state\n        return $ret\nfi\n\nmove_to_original_branch\n```\n\nAll I did was to switch the order to handle the simple (and common) case\nfirst.\n\nSo I do not think that this changes the behavior of the shell-scripted\n`am` backend.\n\nLooking at the code, for a moment I thought that I had introduced a bug\nwhere the state_dir is not cleaned up after move_to_original_branch() is\ncalled, but I call run_am() in run_specific_rebase() and skip directly to\nthe finished_rebase label after that, which does take care of calling\nfinish_rebase() (which removes the state_dir, among other things such as\napplying the autostash).\n\nCiao,\nDscho"}]}