[PATCH 0/4] Let the builtin rebase call the git am command directly

Especially on Windows, where Unix shell scripting is a foreign endeavor, and
an expensive one at that, we really want to avoid running through the Bash.

This not only makes everything faster, but also more robust, as the Bash we
use on Windows relies on a derivative of the Cygwin runtime, which in turn
has to jump through a couple of hoops that are sometimes a little too tricky
to make things work. Read: the less we rely on Unix shell scripting, the
more likely Windows users will be able to enjoy our software.

Johannes Schindelin (4):
  rebase: move `reset_head()` into a better spot
  rebase: avoid double reflog entry when switching branches
  rebase: teach `reset_head()` to optionally skip the worktree
  built-in rebase: call `git am` directly

 builtin/rebase.c | 428 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 309 insertions(+), 119 deletions(-)


base-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-24%2Fdscho%2Fbuiltin-rebase--am-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-24/dscho/builtin-rebase--am-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/24
-- 
gitgitgadget

From: Johannes Schindelin <redacted>

When switching a branch *and* updating said branch to a different
revision, let's avoid a double entry by first updating the branch and
then adjusting the symbolic ref HEAD.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/rebase.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e1dfa74ca8..768bea0da8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action,
 				 detach_head ? REF_NO_DEREF : 0,
 				 UPDATE_REFS_MSG_ON_ERR);
 	else {
-		ret = create_symref("HEAD", switch_to_branch, msg.buf);
+		ret = update_ref(reflog_orig_head, switch_to_branch, oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
 		if (!ret)
-			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
-					 UPDATE_REFS_MSG_ON_ERR);
+			ret = create_symref("HEAD", switch_to_branch,
+					    reflog_head);
 	}
 
 leave_reset_head:

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Over the next commits, we want to make use of it in `run_am()` (i.e.
running the `--am` backend directly, without detouring to Unix shell
script code) which in turn will be called from `run_specific_rebase()`.

So let's move it before that latter function.

This commit is best viewed using --color-moved.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/rebase.c | 238 +++++++++++++++++++++++------------------------
 1 file changed, 119 insertions(+), 119 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b5c99ec10c..e1dfa74ca8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -333,6 +333,125 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 	}
 }
 
+#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+
+#define RESET_HEAD_DETACH (1<<0)
+#define RESET_HEAD_HARD (1<<1)
+
+static int reset_head(struct object_id *oid, const char *action,
+		      const char *switch_to_branch, unsigned flags,
+		      const char *reflog_orig_head, const char *reflog_head)
+{
+	unsigned detach_head = flags & RESET_HEAD_DETACH;
+	unsigned reset_hard = flags & RESET_HEAD_HARD;
+	struct object_id head_oid;
+	struct tree_desc desc[2] = { { NULL }, { NULL } };
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	const char *reflog_action;
+	struct strbuf msg = STRBUF_INIT;
+	size_t prefix_len;
+	struct object_id *orig = NULL, oid_orig,
+		*old_orig = NULL, oid_old_orig;
+	int ret = 0, nr = 0;
+
+	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
+		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+		ret = -1;
+		goto leave_reset_head;
+	}
+
+	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
+		ret = error(_("could not determine HEAD revision"));
+		goto leave_reset_head;
+	}
+
+	if (!oid)
+		oid = &head_oid;
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = the_repository->index;
+	unpack_tree_opts.dst_index = the_repository->index;
+	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
+	unpack_tree_opts.update = 1;
+	unpack_tree_opts.merge = 1;
+	if (!detach_head)
+		unpack_tree_opts.reset = 1;
+
+	if (read_index_unmerged(the_repository->index) < 0) {
+		ret = error(_("could not read index"));
+		goto leave_reset_head;
+	}
+
+	if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {
+		ret = error(_("failed to find tree of %s"),
+			    oid_to_hex(&head_oid));
+		goto leave_reset_head;
+	}
+
+	if (!fill_tree_descriptor(&desc[nr++], oid)) {
+		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+		goto leave_reset_head;
+	}
+
+	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
+		ret = -1;
+		goto leave_reset_head;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(the_repository->index, tree);
+
+	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
+		ret = error(_("could not write index"));
+		goto leave_reset_head;
+	}
+
+	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
+	prefix_len = msg.len;
+
+	if (!get_oid("ORIG_HEAD", &oid_old_orig))
+		old_orig = &oid_old_orig;
+	if (!get_oid("HEAD", &oid_orig)) {
+		orig = &oid_orig;
+		if (!reflog_orig_head) {
+			strbuf_addstr(&msg, "updating ORIG_HEAD");
+			reflog_orig_head = msg.buf;
+		}
+		update_ref(reflog_orig_head, "ORIG_HEAD", orig, old_orig, 0,
+			   UPDATE_REFS_MSG_ON_ERR);
+	} else if (old_orig)
+		delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
+	if (!reflog_head) {
+		strbuf_setlen(&msg, prefix_len);
+		strbuf_addstr(&msg, "updating HEAD");
+		reflog_head = msg.buf;
+	}
+	if (!switch_to_branch)
+		ret = update_ref(reflog_head, "HEAD", oid, orig,
+				 detach_head ? REF_NO_DEREF : 0,
+				 UPDATE_REFS_MSG_ON_ERR);
+	else {
+		ret = create_symref("HEAD", switch_to_branch, msg.buf);
+		if (!ret)
+			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
+					 UPDATE_REFS_MSG_ON_ERR);
+	}
+
+leave_reset_head:
+	strbuf_release(&msg);
+	rollback_lock_file(&lock);
+	while (nr)
+		free((void *)desc[--nr].buffer);
+	return ret;
+}
+
 static const char *resolvemsg =
 N_("Resolve all conflicts manually, mark them as resolved with\n"
 "\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"

@@ -526,125 +645,6 @@ static int run_specific_rebase(struct rebase_options *opts)
 	return status ? -1 : 0;
 }
 
-#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
-
-#define RESET_HEAD_DETACH (1<<0)
-#define RESET_HEAD_HARD (1<<1)
-
-static int reset_head(struct object_id *oid, const char *action,
-		      const char *switch_to_branch, unsigned flags,
-		      const char *reflog_orig_head, const char *reflog_head)
-{
-	unsigned detach_head = flags & RESET_HEAD_DETACH;
-	unsigned reset_hard = flags & RESET_HEAD_HARD;
-	struct object_id head_oid;
-	struct tree_desc desc[2] = { { NULL }, { NULL } };
-	struct lock_file lock = LOCK_INIT;
-	struct unpack_trees_options unpack_tree_opts;
-	struct tree *tree;
-	const char *reflog_action;
-	struct strbuf msg = STRBUF_INIT;
-	size_t prefix_len;
-	struct object_id *orig = NULL, oid_orig,
-		*old_orig = NULL, oid_old_orig;
-	int ret = 0, nr = 0;
-
-	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
-		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
-
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
-		ret = -1;
-		goto leave_reset_head;
-	}
-
-	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
-		ret = error(_("could not determine HEAD revision"));
-		goto leave_reset_head;
-	}
-
-	if (!oid)
-		oid = &head_oid;
-
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
-	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
-	unpack_tree_opts.head_idx = 1;
-	unpack_tree_opts.src_index = the_repository->index;
-	unpack_tree_opts.dst_index = the_repository->index;
-	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
-	unpack_tree_opts.update = 1;
-	unpack_tree_opts.merge = 1;
-	if (!detach_head)
-		unpack_tree_opts.reset = 1;
-
-	if (read_index_unmerged(the_repository->index) < 0) {
-		ret = error(_("could not read index"));
-		goto leave_reset_head;
-	}
-
-	if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {
-		ret = error(_("failed to find tree of %s"),
-			    oid_to_hex(&head_oid));
-		goto leave_reset_head;
-	}
-
-	if (!fill_tree_descriptor(&desc[nr++], oid)) {
-		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
-		goto leave_reset_head;
-	}
-
-	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
-		ret = -1;
-		goto leave_reset_head;
-	}
-
-	tree = parse_tree_indirect(oid);
-	prime_cache_tree(the_repository->index, tree);
-
-	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
-		ret = error(_("could not write index"));
-		goto leave_reset_head;
-	}
-
-	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
-	prefix_len = msg.len;
-
-	if (!get_oid("ORIG_HEAD", &oid_old_orig))
-		old_orig = &oid_old_orig;
-	if (!get_oid("HEAD", &oid_orig)) {
-		orig = &oid_orig;
-		if (!reflog_orig_head) {
-			strbuf_addstr(&msg, "updating ORIG_HEAD");
-			reflog_orig_head = msg.buf;
-		}
-		update_ref(reflog_orig_head, "ORIG_HEAD", orig, old_orig, 0,
-			   UPDATE_REFS_MSG_ON_ERR);
-	} else if (old_orig)
-		delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
-	if (!reflog_head) {
-		strbuf_setlen(&msg, prefix_len);
-		strbuf_addstr(&msg, "updating HEAD");
-		reflog_head = msg.buf;
-	}
-	if (!switch_to_branch)
-		ret = update_ref(reflog_head, "HEAD", oid, orig,
-				 detach_head ? REF_NO_DEREF : 0,
-				 UPDATE_REFS_MSG_ON_ERR);
-	else {
-		ret = create_symref("HEAD", switch_to_branch, msg.buf);
-		if (!ret)
-			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
-					 UPDATE_REFS_MSG_ON_ERR);
-	}
-
-leave_reset_head:
-	strbuf_release(&msg);
-	rollback_lock_file(&lock);
-	while (nr)
-		free((void *)desc[--nr].buffer);
-	return ret;
-}
-
 static int rebase_config(const char *var, const char *value, void *data)
 {
 	struct rebase_options *opts = data;

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

This is what the legacy (scripted) rebase does in
`move_to_original_branch`, and we will need this functionality in the
next commit.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 768bea0da8..303175fdf1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 
 #define RESET_HEAD_DETACH (1<<0)
 #define RESET_HEAD_HARD (1<<1)
+#define RESET_HEAD_REFS_ONLY (1<<2)
 
 static int reset_head(struct object_id *oid, const char *action,
 		      const char *switch_to_branch, unsigned flags,

@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,
 {
 	unsigned detach_head = flags & RESET_HEAD_DETACH;
 	unsigned reset_hard = flags & RESET_HEAD_HARD;
+	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
 	struct object_id head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;

@@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,
 	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
 		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
 
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
 		ret = -1;
 		goto leave_reset_head;
 	}

@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,
 	if (!oid)
 		oid = &head_oid;
 
+	if (flags & RESET_HEAD_REFS_ONLY)
+		goto reset_head_refs;
+
 	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;

@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,
 		goto leave_reset_head;
 	}
 
+reset_head_refs:
 	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
 	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
 	prefix_len = msg.len;

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

While the scripted `git rebase` still has to rely on the
`git-rebase--am.sh` script to implement the glue between the `rebase`
and the `am` commands, we can go a more direct route in the built-in
rebase and avoid using a shell script altogether.

This patch represents a straight-forward port of `git-rebase--am.sh` to
C, along with the glue code to call it directly from within
`builtin/rebase.c`.

This reduces the chances of Git for Windows running into trouble due to
problems with the POSIX emulation layer (known as "MSYS2 runtime",
itself a derivative of the Cygwin runtime): when no shell script is
called, the POSIX emulation layer is avoided altogether.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 303175fdf1..e327c30b6b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -246,6 +246,37 @@ static int read_basic_state(struct rebase_options *opts)
 	return 0;
 }
 
+static int write_basic_state(struct rebase_options *opts)
+{
+	write_file(state_dir_path("head-name", opts), "%s",
+		   opts->head_name ? opts->head_name : "detached HEAD");
+	write_file(state_dir_path("onto", opts), "%s",
+		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
+	write_file(state_dir_path("orig-head", opts), "%s",
+		   oid_to_hex(&opts->orig_head));
+	write_file(state_dir_path("quiet", opts), "%s",
+		   opts->flags & REBASE_NO_QUIET ? "" : "t");
+	if (opts->flags & REBASE_VERBOSE)
+		write_file(state_dir_path("verbose", opts), "%s", "");
+	if (opts->strategy)
+		write_file(state_dir_path("strategy", opts), "%s",
+			   opts->strategy);
+	if (opts->strategy_opts)
+		write_file(state_dir_path("strategy_opts", opts), "%s",
+			   opts->strategy_opts);
+	if (opts->allow_rerere_autoupdate >= 0)
+		write_file(state_dir_path("allow_rerere_autoupdate", opts),
+			   "-%s-rerere-autoupdate",
+			   opts->allow_rerere_autoupdate ? "" : "-no");
+	if (opts->gpg_sign_opt)
+		write_file(state_dir_path("gpg_sign_opt", opts), "%s",
+			   opts->gpg_sign_opt);
+	if (opts->signoff)
+		write_file(state_dir_path("strategy", opts), "--signoff");
+
+	return 0;
+}
+
 static int apply_autostash(struct rebase_options *opts)
 {
 	const char *path = state_dir_path("autostash", opts);

@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,
 	return ret;
 }
 
+static int move_to_original_branch(struct rebase_options *opts)
+{
+	struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
+	int ret;
+
+	if (!opts->head_name)
+		return 0; /* nothing to move back to */
+
+	if (!opts->onto)
+		BUG("move_to_original_branch without onto");
+
+	strbuf_addf(&orig_head_reflog, "rebase finished: %s onto %s",
+		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
+	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
+		    opts->head_name);
+	ret = reset_head(NULL, "checkout", opts->head_name,
+			 RESET_HEAD_REFS_ONLY,
+			 orig_head_reflog.buf, head_reflog.buf);
+
+	strbuf_release(&orig_head_reflog);
+	strbuf_release(&head_reflog);
+	return ret;
+}
+
 static const char *resolvemsg =
 N_("Resolve all conflicts manually, mark them as resolved with\n"
 "\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"

@@ -466,6 +521,129 @@ N_("Resolve all conflicts manually, mark them as resolved with\n"
 "To abort and get back to the state before \"git rebase\", run "
 "\"git rebase --abort\".");
 
+static int run_am(struct rebase_options *opts)
+{
+	struct child_process am = CHILD_PROCESS_INIT;
+	struct child_process format_patch = CHILD_PROCESS_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+	int status;
+	char *rebased_patches;
+
+	am.git_cmd = 1;
+	argv_array_push(&am.args, "am");
+
+	if (opts->action && !strcmp("continue", opts->action)) {
+		argv_array_push(&am.args, "--resolved");
+		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+		if (opts->gpg_sign_opt)
+			argv_array_push(&am.args, opts->gpg_sign_opt);
+		status = run_command(&am);
+		if (status)
+			return status;
+
+		discard_cache();
+		return move_to_original_branch(opts);
+	}
+	if (opts->action && !strcmp("skip", opts->action)) {
+		argv_array_push(&am.args, "--skip");
+		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+		status = run_command(&am);
+		if (status)
+			return status;
+
+		discard_cache();
+		return move_to_original_branch(opts);
+	}
+	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+		argv_array_push(&am.args, "--show-current-patch");
+		return run_command(&am);
+	}
+
+	strbuf_addf(&revisions, "%s...%s",
+		    oid_to_hex(opts->root ?
+			       /* this is now equivalent to ! -z "$upstream" */
+			       &opts->onto->object.oid :
+			       &opts->upstream->object.oid),
+		    oid_to_hex(&opts->orig_head));
+
+	rebased_patches = xstrdup(git_path("rebased-patches"));
+	format_patch.out = open(rebased_patches,
+				O_WRONLY | O_CREAT | O_TRUNC, 0666);
+	if (format_patch.out < 0) {
+		status = error_errno(_("could not write '%s'"),
+				     rebased_patches);
+		free(rebased_patches);
+		argv_array_clear(&am.args);
+		return status;
+	}
+
+	format_patch.git_cmd = 1;
+	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
+			 "--full-index", "--cherry-pick", "--right-only",
+			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
+			 "--no-cover-letter", "--pretty=mboxrd", NULL);
+	if (opts->git_format_patch_opt.len)
+		argv_array_split(&format_patch.args,
+				 opts->git_format_patch_opt.buf);
+	argv_array_push(&format_patch.args, revisions.buf);
+	if (opts->restrict_revision)
+		argv_array_pushf(&format_patch.args, "^%s",
+				 oid_to_hex(&opts->restrict_revision->object.oid));
+
+	status = run_command(&format_patch);
+	if (status) {
+		unlink(rebased_patches);
+		free(rebased_patches);
+		argv_array_clear(&am.args);
+
+		reset_head(&opts->orig_head, "checkout", opts->head_name, 0,
+			   "HEAD", NULL);
+		error(_("\ngit encountered an error while preparing the "
+			"patches to replay\n"
+			"these revisions:\n"
+			"\n    %s\n\n"
+			"As a result, git cannot rebase them."),
+		      opts->revisions);
+
+		strbuf_release(&revisions);
+		return status;
+	}
+	strbuf_release(&revisions);
+
+	am.in = open(rebased_patches, O_RDONLY);
+	if (am.in < 0) {
+		status = error_errno(_("could not read '%s'"),
+				     rebased_patches);
+		free(rebased_patches);
+		argv_array_clear(&am.args);
+		return status;
+	}
+
+	argv_array_pushv(&am.args, opts->git_am_opts.argv);
+	argv_array_push(&am.args, "--rebasing");
+	argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+	argv_array_push(&am.args, "--patch-format=mboxrd");
+	if (opts->allow_rerere_autoupdate > 0)
+		argv_array_push(&am.args, "--rerere-autoupdate");
+	else if (opts->allow_rerere_autoupdate == 0)
+		argv_array_push(&am.args, "--no-rerere-autoupdate");
+	if (opts->gpg_sign_opt)
+		argv_array_push(&am.args, opts->gpg_sign_opt);
+	status = run_command(&am);
+	unlink(rebased_patches);
+	free(rebased_patches);
+
+	if (!status) {
+		discard_cache();
+		return move_to_original_branch(opts);
+	}
+
+	if (is_directory(opts->state_dir))
+		write_basic_state(opts);
+
+	return status;
+}
+
 static int run_specific_rebase(struct rebase_options *opts)
 {
 	const char *argv[] = { NULL, NULL };

@@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts)
 		goto finished_rebase;
 	}
 
+	if (opts->type == REBASE_AM) {
+		status = run_am(opts);
+		goto finished_rebase;
+	}
+
 	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
 	add_var(&script_snippet, "state_dir", opts->state_dir);
 

-- 
gitgitgadget

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

+static int write_basic_state(struct rebase_options *opts)
+{
+	write_file(state_dir_path("head-name", opts), "%s",
+		   opts->head_name ? opts->head_name : "detached HEAD");
+	write_file(state_dir_path("onto", opts), "%s",
+		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
+	write_file(state_dir_path("orig-head", opts), "%s",
+		   oid_to_hex(&opts->orig_head));
+	write_file(state_dir_path("quiet", opts), "%s",
+		   opts->flags & REBASE_NO_QUIET ? "" : "t");
+	if (opts->flags & REBASE_VERBOSE)
+		write_file(state_dir_path("verbose", opts), "%s", "");
+	if (opts->strategy)
+		write_file(state_dir_path("strategy", opts), "%s",
+			   opts->strategy);
+	if (opts->strategy_opts)
+		write_file(state_dir_path("strategy_opts", opts), "%s",
+			   opts->strategy_opts);
+	if (opts->allow_rerere_autoupdate >= 0)
+		write_file(state_dir_path("allow_rerere_autoupdate", opts),
+			   "-%s-rerere-autoupdate",
+			   opts->allow_rerere_autoupdate ? "" : "-no");

Inside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0
(declined) or 1 (requested), and this code is being consistent with
that convention.

The "--[no-]rerere-autoupdate" option that is parsed via
OPT_RERERE_AUTOUPDATE (used in builtin/rebase--interactive.c among
other built-in commands) on the other hand is tertially that uses 0
(unspecified), 1 (requested) and 2 (declined).  This might be a
ticking timebomb to confuse us in the future that may be worth
fixing but probably outside this series.

+	if (opts->gpg_sign_opt)
+		write_file(state_dir_path("gpg_sign_opt", opts), "%s",
+			   opts->gpg_sign_opt);
+	if (opts->signoff)
+		write_file(state_dir_path("strategy", opts), "--signoff");
+
+	return 0;
+}

The above looks like a literal translation of a helper function with
the same name in git-rebase--common.sh.  Good.

This check is absent in the scripted version, but from the message
we generate here, it is clear that the caller must not call this
when there is no "onto" commit.  Good.

+	strbuf_addf(&orig_head_reflog, "rebase finished: %s onto %s",
+		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
+	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
+		    opts->head_name);
+	ret = reset_head(NULL, "checkout", opts->head_name,
+			 RESET_HEAD_REFS_ONLY,
+			 orig_head_reflog.buf, head_reflog.buf);

The *action given to reset_head() here is "checkout".  Makes me
wonder about two things:

 - The only real use of the parameter in the callee is to prepare
   the error and advice messages from the unpack_trees machinery,
   but because we are using it in REFS_ONLY mode, it does not
   matter.  In fact it might even be misleading; perhaps pass NULL
   or something, so that a mistaken update to reset_head() later
   that lets REFS_ONLY request to go to unpack_trees machinery will
   catch it as a bug?

 - Another topic in flight wants to make sure that the post-checkout
   hook gets called when the RESET_HEAD_RUN_POST_CHECKOUT_HOOK flag
   is given by the caller, and IIRC, the use of the flag is strongly
   correlated to *action being "checkout".  Do we want to pass
   REFS_ONLY and RUN_POST_CHECKOUT_HOOK flag for this call, or do we
   rather keep it silent?  As the original scripted version did not
   use "checkout" here and never triggered post-checkout hook, I am
   inclined to say that we should not pass that other bit.  That
   then leads me to suspect that we do not want *action to be
   "checkout" here.

+	strbuf_release(&orig_head_reflog);
+	strbuf_release(&head_reflog);
+	return ret;
+}

Unlike the scripted version, this does not die() upon failure, so
the caller needs to be careful about the returned status.

It is curious why discard_cache() is placed exacly here, as if we
want to preserve the contents of the in-core index when
run_command() failed.  But I do not think we care about the in-core
index as the only thing that happen after "return status" is to
return the control to run_specific_rebase(), let it jump to
finished_rebase label to clean things up and rturn control to
cmd_rebase() and exit based on the status value.

It's not like move_to_original_branch() wants to call read_cache()
and get the result from the "am" that run_command() executed,
either.

Puzzled.  Care to explain a bit more in the in-code comment?

+	}
+	if (opts->action && !strcmp("skip", opts->action)) {
+		argv_array_push(&am.args, "--skip");
+		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+		status = run_command(&am);
+		if (status)
+			return status;
+
+		discard_cache();
+		return move_to_original_branch(opts);

Ditto.

+	}
+	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+		argv_array_push(&am.args, "--show-current-patch");
+		return run_command(&am);
+	}

Up to this point, it is a faithful conversion of the first case/esac
statement.  Good.

+	strbuf_addf(&revisions, "%s...%s",
+		    oid_to_hex(opts->root ?
+			       /* this is now equivalent to ! -z "$upstream" */

Does "this" refer to the "opts->root being true" check?

Because you are flipping the polarity of the test from scripted
version, shouldn't the comment be updated to "-z $upstream"?

+			       &opts->onto->object.oid :
+			       &opts->upstream->object.oid),
+		    oid_to_hex(&opts->orig_head));

+	rebased_patches = xstrdup(git_path("rebased-patches"));
+	format_patch.out = open(rebased_patches,
+				O_WRONLY | O_CREAT | O_TRUNC, 0666);

Unlike scripted version, we do not remove a (possibly) existing file.
We give CREAT in case there is no existing one, and TRUNC in case
there is an existing one.  Makes sense.  A more faithful translation
would have unlink(2)ed a (possibly) existing one, and then because
we can afford to, passed O_EXCL to avoid stomping on somebody else
racing with us, but I do not think it is worth it.

+	if (format_patch.out < 0) {
+		status = error_errno(_("could not write '%s'"),
+				     rebased_patches);

s/write '%s'/open '%s' for writing/?  I dunno.

+		free(rebased_patches);
+		argv_array_clear(&am.args);
+		return status;
+	}
+
+	format_patch.git_cmd = 1;
+	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
+			 "--full-index", "--cherry-pick", "--right-only",
+			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
+			 "--no-cover-letter", "--pretty=mboxrd", NULL);
+	if (opts->git_format_patch_opt.len)
+		argv_array_split(&format_patch.args,
+				 opts->git_format_patch_opt.buf);
+	argv_array_push(&format_patch.args, revisions.buf);
+	if (opts->restrict_revision)
+		argv_array_pushf(&format_patch.args, "^%s",
+				 oid_to_hex(&opts->restrict_revision->object.oid));

It is kinda surprising to see that we have learned quite a lot of
fringe "configurations" we need to explicitly override like this.

Looks like a quite faithful conversion, anyway.

+	status = run_command(&format_patch);
+	if (status) {
+		unlink(rebased_patches);
+		free(rebased_patches);
+		argv_array_clear(&am.args);
+
+		reset_head(&opts->orig_head, "checkout", opts->head_name, 0,
+			   "HEAD", NULL);

This one may need to trigger post-checkout hook.  The scripted
version does two different things depending on the value of
$head_name, but we can just use the same code without conditional?

+		error(_("\ngit encountered an error while preparing the "
+			"patches to replay\n"
+			"these revisions:\n"
+			"\n    %s\n\n"
+			"As a result, git cannot rebase them."),
+		      opts->revisions);
+
+		strbuf_release(&revisions);
+		return status;
+	}
+	strbuf_release(&revisions);
+
+	am.in = open(rebased_patches, O_RDONLY);
+	if (am.in < 0) {
+		status = error_errno(_("could not read '%s'"),
+				     rebased_patches);

s/write '%s'/open '%s' for reading/?  I dunno.

Overall, this was quite a pleasant read and a well constructed
series.  Other than two minor points (i.e. interaction with the
'post-checkout hook' topic, and discard_cache() before calling
move_to_original_branch) I did not quite understand, looks good to
me.

When merged to 'pu', I seem to be getting failure from t3425.5, .8
and .11, by the way.  I haven't dug into the actual breakages any
further than that.

Thanks.

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

From: Johannes Schindelin <redacted>

This is what the legacy (scripted) rebase does in
`move_to_original_branch`, and we will need this functionality in the
next commit.

The move-to-original-branch helper does:

    - point $head_name to the commit pointed at by HEAD
    - point HEAD symref to $head_name

without touching the index or the working tree files.  It's not
exactly "reset --soft" but more like "switch-branch --soft" ;-)

In the future codebase in 'pu', we have 1<<2 already taken by
another topic, so I'll tell my rerere database that the bit
assignment needs to be adjusted.

Not touching the index, so no need to lock the index.  Makes sense.

Why not "refs_only" that we already prepared above???  Are we
munging that secondary variable before control comes here?

In any case, not touching the index nor the working tree, so no need
to call into the unpack_trees machinery.  Makes sense.

And the control continues from the point we update the reflog.
Makes sense.

 	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
 	prefix_len = msg.len;

This helper is touched by two other topics in flight, and that was
one of the reason why it took a bit longer than usual for me to
merge this topic.  Please sanity-check the result of the conflict
resolution at the tip of 'pu' branch.

Thanks.

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

From: Johannes Schindelin <redacted>

When switching a branch *and* updating said branch to a different
revision, let's avoid a double entry by first updating the branch and
then adjusting the symbolic ref HEAD.

Ah, in the original sequence, HEAD is updated twice, leaving two
reflog entries for HEAD (and one for the underlying "switch_to"
branch by virtue of REF_UPDATE_VIA_HEAD).  In the new sequence,
update_ref() updates the underlying "switch_to" and then HEAD, so
we'd get one reflog entry for each of them.

Makes sense.  s/let's avoid a double entry/& in HEAD's reflog/ would
have avoided wasting reader's time who needlessly wondered where
that redundancy came from, though.

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

From: Johannes Schindelin <redacted>

Over the next commits, we want to make use of it in `run_am()` (i.e.
running the `--am` backend directly, without detouring to Unix shell
script code) which in turn will be called from `run_specific_rebase()`.

So let's move it before that latter function.

OK.

Hi Junio,

On Fri, 4 Jan 2019, Junio C Hamano wrote:

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

+static int write_basic_state(struct rebase_options *opts)
+{
+	write_file(state_dir_path("head-name", opts), "%s",
+		   opts->head_name ? opts->head_name : "detached HEAD");
+	write_file(state_dir_path("onto", opts), "%s",
+		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
+	write_file(state_dir_path("orig-head", opts), "%s",
+		   oid_to_hex(&opts->orig_head));
+	write_file(state_dir_path("quiet", opts), "%s",
+		   opts->flags & REBASE_NO_QUIET ? "" : "t");
+	if (opts->flags & REBASE_VERBOSE)
+		write_file(state_dir_path("verbose", opts), "%s", "");
+	if (opts->strategy)
+		write_file(state_dir_path("strategy", opts), "%s",
+			   opts->strategy);
+	if (opts->strategy_opts)
+		write_file(state_dir_path("strategy_opts", opts), "%s",
+			   opts->strategy_opts);
+	if (opts->allow_rerere_autoupdate >= 0)
+		write_file(state_dir_path("allow_rerere_autoupdate", opts),
+			   "-%s-rerere-autoupdate",
+			   opts->allow_rerere_autoupdate ? "" : "-no");

Inside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0
(declined) or 1 (requested), and this code is being consistent with
that convention.

The "--[no-]rerere-autoupdate" option that is parsed via
OPT_RERERE_AUTOUPDATE (used in builtin/rebase--interactive.c among
other built-in commands) on the other hand is tertially that uses 0
(unspecified), 1 (requested) and 2 (declined).  This might be a
ticking timebomb to confuse us in the future that may be worth
fixing but probably outside this series.

Good point. We use -1 for unspecified in so many places, I think
OPT_RERERE_AUTOUPDATE needs to be fixed. But yes, I'll leave this as
#leftoverbits here.

quoted

@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,
 	return ret;
 }
 
+static int move_to_original_branch(struct rebase_options *opts)
+{
+	struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
+	int ret;
+
+	if (!opts->head_name)
+		return 0; /* nothing to move back to */
+
+	if (!opts->onto)
+		BUG("move_to_original_branch without onto");

This check is absent in the scripted version, but from the message
we generate here, it is clear that the caller must not call this
when there is no "onto" commit.  Good.

quoted

+	strbuf_addf(&orig_head_reflog, "rebase finished: %s onto %s",
+		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
+	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
+		    opts->head_name);
+	ret = reset_head(NULL, "checkout", opts->head_name,
+			 RESET_HEAD_REFS_ONLY,
+			 orig_head_reflog.buf, head_reflog.buf);

The *action given to reset_head() here is "checkout".  Makes me
wonder about two things:

 - The only real use of the parameter in the callee is to prepare
   the error and advice messages from the unpack_trees machinery,
   but because we are using it in REFS_ONLY mode, it does not
   matter.  In fact it might even be misleading; perhaps pass NULL
   or something, so that a mistaken update to reset_head() later
   that lets REFS_ONLY request to go to unpack_trees machinery will
   catch it as a bug?

 - Another topic in flight wants to make sure that the post-checkout
   hook gets called when the RESET_HEAD_RUN_POST_CHECKOUT_HOOK flag
   is given by the caller, and IIRC, the use of the flag is strongly
   correlated to *action being "checkout".  Do we want to pass
   REFS_ONLY and RUN_POST_CHECKOUT_HOOK flag for this call, or do we
   rather keep it silent?  As the original scripted version did not
   use "checkout" here and never triggered post-checkout hook, I am
   inclined to say that we should not pass that other bit.  That
   then leads me to suspect that we do not want *action to be
   "checkout" here.

The only thing for which that the `action` is used, though, is the call to
`setup_unpack_trees_porcelain()`, which does not accept a `NULL`. I guess
I could replace it by the empty string. Will do that.

quoted

+	strbuf_release(&orig_head_reflog);
+	strbuf_release(&head_reflog);
+	return ret;
+}

Unlike the scripted version, this does not die() upon failure, so
the caller needs to be careful about the returned status.

Indeed. That function is only called from `run_am()`, and returns the
status in every instance. The caller of `run_am()`,
`run_specific_rebase()` also handles it correctly.

quoted

@@ -466,6 +521,129 @@ N_("Resolve all conflicts manually, mark them as resolved with\n"
 "To abort and get back to the state before \"git rebase\", run "
 "\"git rebase --abort\".");
 
+static int run_am(struct rebase_options *opts)
+{
+	struct child_process am = CHILD_PROCESS_INIT;
+	struct child_process format_patch = CHILD_PROCESS_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+	int status;
+	char *rebased_patches;
+
+	am.git_cmd = 1;
+	argv_array_push(&am.args, "am");
+
+	if (opts->action && !strcmp("continue", opts->action)) {
+		argv_array_push(&am.args, "--resolved");
+		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+		if (opts->gpg_sign_opt)
+			argv_array_push(&am.args, opts->gpg_sign_opt);
+		status = run_command(&am);
+		if (status)
+			return status;
+
+		discard_cache();
+		return move_to_original_branch(opts);

It is curious why discard_cache() is placed exacly here, as if we
want to preserve the contents of the in-core index when
run_command() failed.  But I do not think we care about the in-core
index as the only thing that happen after "return status" is to
return the control to run_specific_rebase(), let it jump to
finished_rebase label to clean things up and rturn control to
cmd_rebase() and exit based on the status value.

It's not like move_to_original_branch() wants to call read_cache()
and get the result from the "am" that run_command() executed,
either.

Puzzled.  Care to explain a bit more in the in-code comment?

I think that this call is just a left-over from a previous version that
did not have the REFS_ONLY flag to pass to `move_to_original_branch()`
(and it caused havoc before that flag was passed). Let me double-check
whether the `discard_cache()` even makes sense any longer.

*clicketyclick* indeed that is the case. Will remove all three
`discard_cache()` calls.

quoted

+	}
+	if (opts->action && !strcmp("skip", opts->action)) {
+		argv_array_push(&am.args, "--skip");
+		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+		status = run_command(&am);
+		if (status)
+			return status;
+
+		discard_cache();
+		return move_to_original_branch(opts);

Ditto.

quoted

+	}
+	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+		argv_array_push(&am.args, "--show-current-patch");
+		return run_command(&am);
+	}

Up to this point, it is a faithful conversion of the first case/esac
statement.  Good.

quoted

+	strbuf_addf(&revisions, "%s...%s",
+		    oid_to_hex(opts->root ?
+			       /* this is now equivalent to ! -z "$upstream" */

Does "this" refer to the "opts->root being true" check?

Because you are flipping the polarity of the test from scripted
version, shouldn't the comment be updated to "-z $upstream"?

It did flip the polarity, you are right, this comment is incorrect. It is
even more incorrect, though, as it talks about a shell construct that is
no longer applicable. Will fix.

quoted

+			       &opts->onto->object.oid :
+			       &opts->upstream->object.oid),
+		    oid_to_hex(&opts->orig_head));

quoted

+	rebased_patches = xstrdup(git_path("rebased-patches"));
+	format_patch.out = open(rebased_patches,
+				O_WRONLY | O_CREAT | O_TRUNC, 0666);

Unlike scripted version, we do not remove a (possibly) existing file.
We give CREAT in case there is no existing one, and TRUNC in case
there is an existing one.  Makes sense.  A more faithful translation
would have unlink(2)ed a (possibly) existing one, and then because
we can afford to, passed O_EXCL to avoid stomping on somebody else
racing with us, but I do not think it is worth it.

Okay.

quoted

+	if (format_patch.out < 0) {
+		status = error_errno(_("could not write '%s'"),
+				     rebased_patches);

s/write '%s'/open '%s' for writing/?  I dunno.

Yep, of course! Will fix.

quoted

+		free(rebased_patches);
+		argv_array_clear(&am.args);
+		return status;
+	}
+
+	format_patch.git_cmd = 1;
+	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
+			 "--full-index", "--cherry-pick", "--right-only",
+			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
+			 "--no-cover-letter", "--pretty=mboxrd", NULL);
+	if (opts->git_format_patch_opt.len)
+		argv_array_split(&format_patch.args,
+				 opts->git_format_patch_opt.buf);
+	argv_array_push(&format_patch.args, revisions.buf);
+	if (opts->restrict_revision)
+		argv_array_pushf(&format_patch.args, "^%s",
+				 oid_to_hex(&opts->restrict_revision->object.oid));

It is kinda surprising to see that we have learned quite a lot of
fringe "configurations" we need to explicitly override like this.

Looks like a quite faithful conversion, anyway.

quoted

+	status = run_command(&format_patch);
+	if (status) {
+		unlink(rebased_patches);
+		free(rebased_patches);
+		argv_array_clear(&am.args);
+
+		reset_head(&opts->orig_head, "checkout", opts->head_name, 0,
+			   "HEAD", NULL);

This one may need to trigger post-checkout hook.  The scripted
version does two different things depending on the value of
$head_name, but we can just use the same code without conditional?

Yes, because `opts->head_name` is `NULL` in one case, and not `NULL` in
the other, and the `reset_head()` function performs the desired operation
in each case.

quoted

+		error(_("\ngit encountered an error while preparing the "
+			"patches to replay\n"
+			"these revisions:\n"
+			"\n    %s\n\n"
+			"As a result, git cannot rebase them."),
+		      opts->revisions);
+
+		strbuf_release(&revisions);
+		return status;
+	}
+	strbuf_release(&revisions);
+
+	am.in = open(rebased_patches, O_RDONLY);
+	if (am.in < 0) {
+		status = error_errno(_("could not read '%s'"),
+				     rebased_patches);

s/write '%s'/open '%s' for reading/?  I dunno.

Yep, will fix.

quoted

+		free(rebased_patches);
+		argv_array_clear(&am.args);
+		return status;
+	}
+
+	argv_array_pushv(&am.args, opts->git_am_opts.argv);
+	argv_array_push(&am.args, "--rebasing");
+	argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+	argv_array_push(&am.args, "--patch-format=mboxrd");
+	if (opts->allow_rerere_autoupdate > 0)
+		argv_array_push(&am.args, "--rerere-autoupdate");
+	else if (opts->allow_rerere_autoupdate == 0)
+		argv_array_push(&am.args, "--no-rerere-autoupdate");
+	if (opts->gpg_sign_opt)
+		argv_array_push(&am.args, opts->gpg_sign_opt);
+	status = run_command(&am);
+	unlink(rebased_patches);
+	free(rebased_patches);
+
+	if (!status) {
+		discard_cache();
+		return move_to_original_branch(opts);
+	}
+
+	if (is_directory(opts->state_dir))
+		write_basic_state(opts);
+
+	return status;
+}
+
 static int run_specific_rebase(struct rebase_options *opts)
 {
 	const char *argv[] = { NULL, NULL };

@@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts)
 		goto finished_rebase;
 	}
 
+	if (opts->type == REBASE_AM) {
+		status = run_am(opts);
+		goto finished_rebase;
+	}
+
 	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
 	add_var(&script_snippet, "state_dir", opts->state_dir);

Overall, this was quite a pleasant read and a well constructed
series.  Other than two minor points (i.e. interaction with the
'post-checkout hook' topic, and discard_cache() before calling
move_to_original_branch) I did not quite understand, looks good to
me.

When merged to 'pu', I seem to be getting failure from t3425.5, .8
and .11, by the way.  I haven't dug into the actual breakages any
further than that.

Sorry for the trouble, and for my silence (I was heads-down into the Azure
Pipelines support).

I did not see any breakage in `pu` lately, hopefully things resolved
themselves?

Ciao,
Dscho

Hi Junio,

On Fri, 4 Jan 2019, Junio C Hamano wrote:

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

From: Johannes Schindelin <redacted>

This is what the legacy (scripted) rebase does in
`move_to_original_branch`, and we will need this functionality in the
next commit.

The move-to-original-branch helper does:

    - point $head_name to the commit pointed at by HEAD
    - point HEAD symref to $head_name

without touching the index or the working tree files.  It's not
exactly "reset --soft" but more like "switch-branch --soft" ;-)

Right, but it does reset the HEAD softly.

quoted

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 768bea0da8..303175fdf1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 
 #define RESET_HEAD_DETACH (1<<0)
 #define RESET_HEAD_HARD (1<<1)
+#define RESET_HEAD_REFS_ONLY (1<<2)

In the future codebase in 'pu', we have 1<<2 already taken by
another topic, so I'll tell my rerere database that the bit
assignment needs to be adjusted.

Okay.

quoted

 static int reset_head(struct object_id *oid, const char *action,
 		      const char *switch_to_branch, unsigned flags,

@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,
 {
 	unsigned detach_head = flags & RESET_HEAD_DETACH;
 	unsigned reset_hard = flags & RESET_HEAD_HARD;
+	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
 	struct object_id head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;

@@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,
 	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
 		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
 
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
 		ret = -1;
 		goto leave_reset_head;
 	}

Not touching the index, so no need to lock the index.  Makes sense.

quoted

@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,
 	if (!oid)
 		oid = &head_oid;
 
+	if (flags & RESET_HEAD_REFS_ONLY)
+		goto reset_head_refs;
+

Why not "refs_only" that we already prepared above???  Are we
munging that secondary variable before control comes here?

No, just an oversight. Will fix.

In any case, not touching the index nor the working tree, so no need
to call into the unpack_trees machinery.  Makes sense.

quoted

 	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;

@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,
 		goto leave_reset_head;
 	}
 
+reset_head_refs:
 	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);

And the control continues from the point we update the reflog.
Makes sense.

quoted

 	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
 	prefix_len = msg.len;

This helper is touched by two other topics in flight, and that was
one of the reason why it took a bit longer than usual for me to
merge this topic.  Please sanity-check the result of the conflict
resolution at the tip of 'pu' branch.

Sorry, I *just now* had time to take care of this patch series. I will
gladly check once you integrate the new patch series iteration.

Thanks,
Dscho

Hi Junio,

On Fri, 4 Jan 2019, Junio C Hamano wrote:

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

From: Johannes Schindelin <redacted>

When switching a branch *and* updating said branch to a different
revision, let's avoid a double entry by first updating the branch and
then adjusting the symbolic ref HEAD.

Ah, in the original sequence, HEAD is updated twice, leaving two
reflog entries for HEAD (and one for the underlying "switch_to"
branch by virtue of REF_UPDATE_VIA_HEAD).  In the new sequence,
update_ref() updates the underlying "switch_to" and then HEAD, so
we'd get one reflog entry for each of them.

Makes sense.  s/let's avoid a double entry/& in HEAD's reflog/ would
have avoided wasting reader's time who needlessly wondered where
that redundancy came from, though.

Will fix the commit message.

Thanks!
Dscho

quoted

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/rebase.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e1dfa74ca8..768bea0da8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action,
 				 detach_head ? REF_NO_DEREF : 0,
 				 UPDATE_REFS_MSG_ON_ERR);
 	else {
-		ret = create_symref("HEAD", switch_to_branch, msg.buf);
+		ret = update_ref(reflog_orig_head, switch_to_branch, oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
 		if (!ret)
-			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
-					 UPDATE_REFS_MSG_ON_ERR);
+			ret = create_symref("HEAD", switch_to_branch,
+					    reflog_head);
 	}
 
 leave_reset_head:

Especially on Windows, where Unix shell scripting is a foreign endeavor, and
an expensive one at that, we really want to avoid running through the Bash.

This not only makes everything faster, but also more robust, as the Bash we
use on Windows relies on a derivative of the Cygwin runtime, which in turn
has to jump through a couple of hoops that are sometimes a little too tricky
to make things work. Read: the less we rely on Unix shell scripting, the
more likely Windows users will be able to enjoy our software.

Changes since v1:

 * Rebased on top of master to avoid merge conflicts.
 * Adjusted the commit message talking about double entries, to clarify that
   it talks about HEAD's reflog.
 * Replaced a misleading action parameter "checkout" for the reset_head() 
   function by the empty string: we do not check out here, we just update
   the refs, and certainly do not want any checkout functionality (such as
   hooks) to be involved.
 * Reused a just-prepared refs_only variable instead of repeating the value
   assigned to it.
 * Fixed a stale comment about the shell variable "$upstream" (which should
   have been negated to begin with).
 * Fixed error messages when files could not be opened.

Johannes Schindelin (4):
  rebase: move `reset_head()` into a better spot
  rebase: avoid double reflog entry when switching branches
  rebase: teach `reset_head()` to optionally skip the worktree
  built-in rebase: call `git am` directly

 builtin/rebase.c | 424 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 305 insertions(+), 119 deletions(-)


base-commit: 77556354bb7ac50450e3b28999e3576969869068
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-24%2Fdscho%2Fbuiltin-rebase--am-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-24/dscho/builtin-rebase--am-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/24

Range-diff vs v1:

 1:  175dc7d29a ! 1:  e886ae3de5 rebase: move `reset_head()` into a better spot
     @@ -91,7 +91,7 @@
      +	}
      +
      +	tree = parse_tree_indirect(oid);
     -+	prime_cache_tree(the_repository->index, tree);
     ++	prime_cache_tree(the_repository, the_repository->index, tree);
      +
      +	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
      +		ret = error(_("could not write index"));
     @@ -217,7 +217,7 @@
      -	}
      -
      -	tree = parse_tree_indirect(oid);
     --	prime_cache_tree(the_repository->index, tree);
     +-	prime_cache_tree(the_repository, the_repository->index, tree);
      -
      -	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
      -		ret = error(_("could not write index"));
 2:  4c5f87b9dc ! 2:  3a68f1c509 rebase: avoid double reflog entry when switching branches
     @@ -3,8 +3,8 @@
          rebase: avoid double reflog entry when switching branches
      
          When switching a branch *and* updating said branch to a different
     -    revision, let's avoid a double entry by first updating the branch and
     -    then adjusting the symbolic ref HEAD.
     +    revision, let's avoid a double entry in HEAD's reflog by first updating
     +    the branch and then adjusting the symbolic ref HEAD.
      
          Signed-off-by: Johannes Schindelin [off-list ref]
      
 3:  21939140e0 ! 3:  387071dcd7 rebase: teach `reset_head()` to optionally skip the worktree
     @@ -40,7 +40,7 @@
       	if (!oid)
       		oid = &head_oid;
       
     -+	if (flags & RESET_HEAD_REFS_ONLY)
     ++	if (refs_only)
      +		goto reset_head_refs;
      +
       	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 4:  2b5ece8263 ! 4:  3c40318682 built-in rebase: call `git am` directly
     @@ -16,6 +16,10 @@
          itself a derivative of the Cygwin runtime): when no shell script is
          called, the POSIX emulation layer is avoided altogether.
      
     +    Note: we pass an empty action to `reset_head()` here when moving back to
     +    the original branch, as no other action is applicable, really. This
     +    parameter is used to initialize `unpack_trees()`' messages.
     +
          Signed-off-by: Johannes Schindelin [off-list ref]
      
       diff --git a/builtin/rebase.c b/builtin/rebase.c
     @@ -78,8 +82,7 @@
      +		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
      +	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
      +		    opts->head_name);
     -+	ret = reset_head(NULL, "checkout", opts->head_name,
     -+			 RESET_HEAD_REFS_ONLY,
     ++	ret = reset_head(NULL, "", opts->head_name, RESET_HEAD_REFS_ONLY,
      +			 orig_head_reflog.buf, head_reflog.buf);
      +
      +	strbuf_release(&orig_head_reflog);
     @@ -114,7 +117,6 @@
      +		if (status)
      +			return status;
      +
     -+		discard_cache();
      +		return move_to_original_branch(opts);
      +	}
      +	if (opts->action && !strcmp("skip", opts->action)) {
     @@ -124,7 +126,6 @@
      +		if (status)
      +			return status;
      +
     -+		discard_cache();
      +		return move_to_original_branch(opts);
      +	}
      +	if (opts->action && !strcmp("show-current-patch", opts->action)) {
     @@ -134,7 +135,7 @@
      +
      +	strbuf_addf(&revisions, "%s...%s",
      +		    oid_to_hex(opts->root ?
     -+			       /* this is now equivalent to ! -z "$upstream" */
     ++			       /* this is now equivalent to !opts->upstream */
      +			       &opts->onto->object.oid :
      +			       &opts->upstream->object.oid),
      +		    oid_to_hex(&opts->orig_head));
     @@ -143,7 +144,7 @@
      +	format_patch.out = open(rebased_patches,
      +				O_WRONLY | O_CREAT | O_TRUNC, 0666);
      +	if (format_patch.out < 0) {
     -+		status = error_errno(_("could not write '%s'"),
     ++		status = error_errno(_("could not open '%s' for writing"),
      +				     rebased_patches);
      +		free(rebased_patches);
      +		argv_array_clear(&am.args);
     @@ -185,7 +186,7 @@
      +
      +	am.in = open(rebased_patches, O_RDONLY);
      +	if (am.in < 0) {
     -+		status = error_errno(_("could not read '%s'"),
     ++		status = error_errno(_("could not open '%s' for reading"),
      +				     rebased_patches);
      +		free(rebased_patches);
      +		argv_array_clear(&am.args);
     @@ -207,7 +208,6 @@
      +	free(rebased_patches);
      +
      +	if (!status) {
     -+		discard_cache();
      +		return move_to_original_branch(opts);
      +	}
      +

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Over the next commits, we want to make use of it in `run_am()` (i.e.
running the `--am` backend directly, without detouring to Unix shell
script code) which in turn will be called from `run_specific_rebase()`.

So let's move it before that latter function.

This commit is best viewed using --color-moved.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/rebase.c | 238 +++++++++++++++++++++++------------------------
 1 file changed, 119 insertions(+), 119 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 00de70365e..efa90ca894 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -333,6 +333,125 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 	}
 }
 
+#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+
+#define RESET_HEAD_DETACH (1<<0)
+#define RESET_HEAD_HARD (1<<1)
+
+static int reset_head(struct object_id *oid, const char *action,
+		      const char *switch_to_branch, unsigned flags,
+		      const char *reflog_orig_head, const char *reflog_head)
+{
+	unsigned detach_head = flags & RESET_HEAD_DETACH;
+	unsigned reset_hard = flags & RESET_HEAD_HARD;
+	struct object_id head_oid;
+	struct tree_desc desc[2] = { { NULL }, { NULL } };
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	const char *reflog_action;
+	struct strbuf msg = STRBUF_INIT;
+	size_t prefix_len;
+	struct object_id *orig = NULL, oid_orig,
+		*old_orig = NULL, oid_old_orig;
+	int ret = 0, nr = 0;
+
+	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
+		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
+		ret = -1;
+		goto leave_reset_head;
+	}
+
+	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
+		ret = error(_("could not determine HEAD revision"));
+		goto leave_reset_head;
+	}
+
+	if (!oid)
+		oid = &head_oid;
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = the_repository->index;
+	unpack_tree_opts.dst_index = the_repository->index;
+	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
+	unpack_tree_opts.update = 1;
+	unpack_tree_opts.merge = 1;
+	if (!detach_head)
+		unpack_tree_opts.reset = 1;
+
+	if (read_index_unmerged(the_repository->index) < 0) {
+		ret = error(_("could not read index"));
+		goto leave_reset_head;
+	}
+
+	if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {
+		ret = error(_("failed to find tree of %s"),
+			    oid_to_hex(&head_oid));
+		goto leave_reset_head;
+	}
+
+	if (!fill_tree_descriptor(&desc[nr++], oid)) {
+		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+		goto leave_reset_head;
+	}
+
+	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
+		ret = -1;
+		goto leave_reset_head;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(the_repository, the_repository->index, tree);
+
+	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
+		ret = error(_("could not write index"));
+		goto leave_reset_head;
+	}
+
+	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
+	prefix_len = msg.len;
+
+	if (!get_oid("ORIG_HEAD", &oid_old_orig))
+		old_orig = &oid_old_orig;
+	if (!get_oid("HEAD", &oid_orig)) {
+		orig = &oid_orig;
+		if (!reflog_orig_head) {
+			strbuf_addstr(&msg, "updating ORIG_HEAD");
+			reflog_orig_head = msg.buf;
+		}
+		update_ref(reflog_orig_head, "ORIG_HEAD", orig, old_orig, 0,
+			   UPDATE_REFS_MSG_ON_ERR);
+	} else if (old_orig)
+		delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
+	if (!reflog_head) {
+		strbuf_setlen(&msg, prefix_len);
+		strbuf_addstr(&msg, "updating HEAD");
+		reflog_head = msg.buf;
+	}
+	if (!switch_to_branch)
+		ret = update_ref(reflog_head, "HEAD", oid, orig,
+				 detach_head ? REF_NO_DEREF : 0,
+				 UPDATE_REFS_MSG_ON_ERR);
+	else {
+		ret = create_symref("HEAD", switch_to_branch, msg.buf);
+		if (!ret)
+			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
+					 UPDATE_REFS_MSG_ON_ERR);
+	}
+
+leave_reset_head:
+	strbuf_release(&msg);
+	rollback_lock_file(&lock);
+	while (nr)
+		free((void *)desc[--nr].buffer);
+	return ret;
+}
+
 static const char *resolvemsg =
 N_("Resolve all conflicts manually, mark them as resolved with\n"
 "\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"

@@ -526,125 +645,6 @@ static int run_specific_rebase(struct rebase_options *opts)
 	return status ? -1 : 0;
 }
 
-#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
-
-#define RESET_HEAD_DETACH (1<<0)
-#define RESET_HEAD_HARD (1<<1)
-
-static int reset_head(struct object_id *oid, const char *action,
-		      const char *switch_to_branch, unsigned flags,
-		      const char *reflog_orig_head, const char *reflog_head)
-{
-	unsigned detach_head = flags & RESET_HEAD_DETACH;
-	unsigned reset_hard = flags & RESET_HEAD_HARD;
-	struct object_id head_oid;
-	struct tree_desc desc[2] = { { NULL }, { NULL } };
-	struct lock_file lock = LOCK_INIT;
-	struct unpack_trees_options unpack_tree_opts;
-	struct tree *tree;
-	const char *reflog_action;
-	struct strbuf msg = STRBUF_INIT;
-	size_t prefix_len;
-	struct object_id *orig = NULL, oid_orig,
-		*old_orig = NULL, oid_old_orig;
-	int ret = 0, nr = 0;
-
-	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
-		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
-
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
-		ret = -1;
-		goto leave_reset_head;
-	}
-
-	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
-		ret = error(_("could not determine HEAD revision"));
-		goto leave_reset_head;
-	}
-
-	if (!oid)
-		oid = &head_oid;
-
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
-	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
-	unpack_tree_opts.head_idx = 1;
-	unpack_tree_opts.src_index = the_repository->index;
-	unpack_tree_opts.dst_index = the_repository->index;
-	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
-	unpack_tree_opts.update = 1;
-	unpack_tree_opts.merge = 1;
-	if (!detach_head)
-		unpack_tree_opts.reset = 1;
-
-	if (read_index_unmerged(the_repository->index) < 0) {
-		ret = error(_("could not read index"));
-		goto leave_reset_head;
-	}
-
-	if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) {
-		ret = error(_("failed to find tree of %s"),
-			    oid_to_hex(&head_oid));
-		goto leave_reset_head;
-	}
-
-	if (!fill_tree_descriptor(&desc[nr++], oid)) {
-		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
-		goto leave_reset_head;
-	}
-
-	if (unpack_trees(nr, desc, &unpack_tree_opts)) {
-		ret = -1;
-		goto leave_reset_head;
-	}
-
-	tree = parse_tree_indirect(oid);
-	prime_cache_tree(the_repository, the_repository->index, tree);
-
-	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
-		ret = error(_("could not write index"));
-		goto leave_reset_head;
-	}
-
-	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
-	prefix_len = msg.len;
-
-	if (!get_oid("ORIG_HEAD", &oid_old_orig))
-		old_orig = &oid_old_orig;
-	if (!get_oid("HEAD", &oid_orig)) {
-		orig = &oid_orig;
-		if (!reflog_orig_head) {
-			strbuf_addstr(&msg, "updating ORIG_HEAD");
-			reflog_orig_head = msg.buf;
-		}
-		update_ref(reflog_orig_head, "ORIG_HEAD", orig, old_orig, 0,
-			   UPDATE_REFS_MSG_ON_ERR);
-	} else if (old_orig)
-		delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
-	if (!reflog_head) {
-		strbuf_setlen(&msg, prefix_len);
-		strbuf_addstr(&msg, "updating HEAD");
-		reflog_head = msg.buf;
-	}
-	if (!switch_to_branch)
-		ret = update_ref(reflog_head, "HEAD", oid, orig,
-				 detach_head ? REF_NO_DEREF : 0,
-				 UPDATE_REFS_MSG_ON_ERR);
-	else {
-		ret = create_symref("HEAD", switch_to_branch, msg.buf);
-		if (!ret)
-			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
-					 UPDATE_REFS_MSG_ON_ERR);
-	}
-
-leave_reset_head:
-	strbuf_release(&msg);
-	rollback_lock_file(&lock);
-	while (nr)
-		free((void *)desc[--nr].buffer);
-	return ret;
-}
-
 static int rebase_config(const char *var, const char *value, void *data)
 {
 	struct rebase_options *opts = data;