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

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

[Johannes Schindelin via GitGitGadget]

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

[PATCH 1/4] rebase: move `reset_head()` into a better spot

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

Re: [PATCH 1/4] rebase: move `reset_head()` into a better spot

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

[PATCH 2/4] rebase: avoid double reflog entry when switching branches

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

Re: [PATCH 2/4] rebase: avoid double reflog entry when switching branches

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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:

Re: [PATCH 2/4] rebase: avoid double reflog entry when switching branches

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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

> 
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  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:
> 

[PATCH 4/4] built-in rebase: call `git am` directly

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

Re: [PATCH 4/4] built-in rebase: call `git am` directly

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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.

> @@ -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.

> +	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.

> @@ -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?

> +	}
> +	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.

> +		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.

Thanks.

Re: [PATCH 4/4] built-in rebase: call `git am` directly

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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.

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.

> > @@ -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.
> 
> > +	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.

> 
> > +	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.

> 
> > @@ -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.

> 
> > +	}
> > +	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"?

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.

> 
> > +			       &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.

Okay.

> > +	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.

> > +		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?

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.

> > +		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.

> 
> > +		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

Re: [PATCH 4/4] built-in rebase: call `git am` directly

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> 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?

The (semantic) conflict resolution can be seen in

    $ git show 'origin/pu^{/^Merge branch .js/rebase-am. into}'

which is recorded in my rerere database ;-)

Re: [PATCH 4/4] built-in rebase: call `git am` directly

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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?
> 
> The (semantic) conflict resolution can be seen in
> 
>     $ git show 'origin/pu^{/^Merge branch .js/rebase-am. into}'
> 
> which is recorded in my rerere database ;-)

That looks all good to me, with the minor nit that you did not wrap at
<= 80 columns/row:

 -                       "--no-cover-letter", "--pretty=mboxrd", NULL);
++                       "--no-cover-letter", "--pretty=mboxrd", "--topo-order", NULL);

I am not a stickler to this line length rule, though, as I long switched
away from the 80x25 screen and can easily resize my text windows.

Ciao,
Dscho

Re: [PATCH 4/4] built-in rebase: call `git am` directly

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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.

 ...

>  builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++

Now at some point as a follow-up change, we'd need to remove the
git-rebase--am.sh that is no longer used, together with the
reference to it in the Makefile, no?

Re: [PATCH 4/4] built-in rebase: call `git am` directly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> 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.
>
>  ...
>
>>  builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
>
> Now at some point as a follow-up change, we'd need to remove the
> git-rebase--am.sh that is no longer used, together with the
> reference to it in the Makefile, no?

Answering to myself.  That needs to wait until the legacy-rebase is
retired, so it's not like "immediately after this series is done"
kind of thing.

Anyway, thanks for working on this.

[PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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" ;-)

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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.

>  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.

> @@ -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?

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

>  	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.

>  	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.

Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> 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" ;-)
>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  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.
>
>>  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.
>
>> @@ -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?
>
> In any case, not touching the index nor the working tree, so no need
> to call into the unpack_trees machinery.  Makes sense.
>
>>  	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.
>
>>  	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.

The topics are os/rebase-runs-post-checkout-hook and nd/backup-log
IIRC.

Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.

> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  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.

> >  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.
> 
> > @@ -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.
> 
> >  	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.
> 
> >  	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

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

[Johannes Schindelin via GitGitGadget]

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 <johannes.schindelin@gmx.de>
      
 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 <johannes.schindelin@gmx.de>
      
       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

[PATCH v2 1/4] rebase: move `reset_head()` into a better spot

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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;
-- 
gitgitgadget

[PATCH v2 2/4] rebase: avoid double reflog entry when switching branches

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When switching a branch *and* updating said branch to a different
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 <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index efa90ca894..5c827b2f03 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

[PATCH v2 3/4] rebase: teach `reset_head()` to optionally skip the worktree

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5c827b2f03..c65493a484 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 (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

[PATCH v2 4/4] built-in rebase: call `git am` directly

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

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 <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 179 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index c65493a484..0727ddaf00 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,29 @@ 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, "", 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 +520,126 @@ 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;
+
+		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;
+
+		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 !opts->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 open '%s' for writing"),
+				     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 open '%s' for reading"),
+				     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) {
+		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 +720,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

Re: [PATCH v2 4/4] built-in rebase: call `git am` directly

[Alban Gruin]

Hi Johannes,

Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :
> -%<-
> +static int run_am(struct rebase_options *opts)
> +{
> -%<-
> +
> +	if (!status) {
> +		return move_to_original_branch(opts);
> +	}
> +
> +	if (is_directory(opts->state_dir))
> +		write_basic_state(opts);
> +
> +	return status;
> +}
> +

This means that the state directory will be created only if there is a
problem (ie. a conflict), not if the user cancels the rebase by hitting
^C.  In this case, the user will be left in a corrupted state where the
builtin rebase cannot abort the rebase properly.

-- Alban

Re: [PATCH v2 4/4] built-in rebase: call `git am` directly

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

Hi Alban,

On Sat, 26 Jan 2019, Alban Gruin wrote:

> Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :
> > -%<-
> > +static int run_am(struct rebase_options *opts)
> > +{
> > -%<-
> > +
> > +	if (!status) {
> > +		return move_to_original_branch(opts);
> > +	}
> > +
> > +	if (is_directory(opts->state_dir))
> > +		write_basic_state(opts);
> > +
> > +	return status;
> > +}
> > +
> 
> This means that the state directory will be created only if there is a
> problem (ie. a conflict), not if the user cancels the rebase by hitting
> ^C.  In this case, the user will be left in a corrupted state where the
> builtin rebase cannot abort the rebase properly.

This is an almost literal translation of
https://github.com/git/git/blob/v2.20.1/git-rebase--am.sh#L77-L83:

```sh
if test 0 != $ret
then
        test -d "$state_dir" && write_basic_state
        return $ret
fi

move_to_original_branch
```

All I did was to switch the order to handle the simple (and common) case
first.

So I do not think that this changes the behavior of the shell-scripted
`am` backend.

Looking at the code, for a moment I thought that I had introduced a bug
where the state_dir is not cleaned up after move_to_original_branch() is
called, but I call run_am() in run_specific_rebase() and skip directly to
the finished_rebase label after that, which does take care of calling
finish_rebase() (which removes the state_dir, among other things such as
applying the autostash).

Ciao,
Dscho

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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.

I do not appreciate this very much.  

We already have a good resolution prepared when merging this to
either 'next' or 'master', which also resolves conflict with the
other topic that requires this topic to add "--topo-order" in its
call.  Rebasing series means invalidating the previous work recorded
in rerere.

	side note. The rerere database entry can be recovered from
	master..pu with contrib/rerere-train.sh).

>  * Adjusted the commit message talking about double entries, to clarify that
>    it talks about HEAD's reflog.

Good.

>  * 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.

OK.

>  * Reused a just-prepared refs_only variable instead of repeating the value
>    assigned to it.

OK.

>  * 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.

Good.

Will take a look.  Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> 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.
>
> I do not appreciate this very much.  
>
> We already have a good resolution prepared when merging this to
> either 'next' or 'master', which also resolves conflict with the
> other topic that requires this topic to add "--topo-order" in its
> call.  Rebasing series means invalidating the previous work recorded
> in rerere.
>
> 	side note. The rerere database entry can be recovered from
> 	master..pu with contrib/rerere-train.sh).

I prepared a set of patches rebased back on top of the previous base
and applied them on top of the previous base (call the result X).

Applying the posted patches directly on top of master, and merging X
into master, produces the same tree.

So I'll keep the X (i.e. these patches backward-rebased to the
previous base) and replace js/rebase-am with it.  That is safer when
recreating 'pu' and merging it to 'next', which I hope we can do
soonish.

> Will take a look.  Thanks.

They were pleasant and smooth read.  Crafted nicely.

Thanks.

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

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > 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.
> 
> I do not appreciate this very much.  
> 
> We already have a good resolution prepared when merging this to
> either 'next' or 'master', which also resolves conflict with the
> other topic that requires this topic to add "--topo-order" in its
> call.  Rebasing series means invalidating the previous work recorded
> in rerere.

You misunderstand. The PR had merge conflicts with `master`. As such, the
Azure Pipeline that tests it on Windows, macOS and Linux could not give me
enough confidence. So I *had* to rebase.

Besides, I think it is a terrible idea to leave the resolution of merge
conflicts to you. You are not in the best position to judge how to resolve
them, and as a consequence you spend more time and effort on this than
necessary. Of course, it's your call if you want to do it, as a
contributor I'd rather be certain that *I* resolved the conflicts.

Ciao,
Dscho

> 	side note. The rerere database entry can be recovered from
> 	master..pu with contrib/rerere-train.sh).
> 
> >  * Adjusted the commit message talking about double entries, to clarify that
> >    it talks about HEAD's reflog.
> 
> Good.
> 
> >  * 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.
> 
> OK.
> 
> >  * Reused a just-prepared refs_only variable instead of repeating the value
> >    assigned to it.
> 
> OK.
> 
> >  * 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.
> 
> Good.
> 
> Will take a look.  Thanks.
> 
>