Let the builtin rebase call the git am command directly

PATCH Superseded Johannes Schindelin via GitGitGadget 4 participants 27 messages Dec 21, 2018 – Jan 29, 2019

Share & tools

Johannes Schindelin via GitGitGadget

author

2018-12-21 13:17 UTC #0 lore raw

cover Let the builtin rebase call the git am command directly

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

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

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

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

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

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2018-12-21 13:17 UTC #1 lore raw

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

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

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

builtin/rebase.c +119 ~~-119~~

index b5c99ec10c..e1dfa74ca8 100644
@@ -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;

Signature · 1 line

gitgitgadget

Junio C Hamano

2019-01-04 18:39 UTC #2 lore raw

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

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Johannes Schindelin <[email protected]>
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.

Johannes Schindelin via GitGitGadget

author

2018-12-21 13:17 UTC #3 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

builtin/rebase.c +4 -3

index e1dfa74ca8..768bea0da8 100644
@@ -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:

Signature · 1 line

gitgitgadget

Junio C Hamano

2019-01-04 18:38 UTC #4 lore raw

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

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>
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.

Quoted 25 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/rebase.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

builtin/rebase.c +4 -3

index e1dfa74ca8..768bea0da8 100644
@@ -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:

Johannes Schindelin

2019-01-18 14:19 UTC #5 lore raw

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

Hi Junio,

Quoted 15 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
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.

Quoted 3 lines

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

Will fix the commit message.

Thanks!
Dscho

Quoted 27 lines

Quoted 26 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/rebase.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

builtin/rebase.c +4 -3

index e1dfa74ca8..768bea0da8 100644
@@ -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:

Johannes Schindelin via GitGitGadget

author

2018-12-21 13:17 UTC #6 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

builtin/rebase.c +7 -1

index 768bea0da8..303175fdf1 100644
@@ -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;

Signature · 1 line

gitgitgadget

Junio C Hamano

2019-01-04 18:38 UTC #7 lore raw

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

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>
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" ;-)

Quoted 10 lines

Signed-off-by Johannes Schindelin <[email protected]>
---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
builtin/rebase.c +0 -0
index 768bea0da8..303175fdf1 100644

builtin/rebase.c static void add_var(struct strbuf *buf, const char *name, const char *value) · line 340

Quoted 4 lines

builtin/rebase.c +1 -0

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

Quoted 11 lines

builtin/rebase.c +1 -0

 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;

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 367

Quoted 8 lines

builtin/rebase.c +1 -1

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

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 379

Quoted 7 lines

builtin/rebase.c +3 -0

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

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 421

Quoted 9 lines

builtin/rebase.c +1 -0

 	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.

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 423

Quoted 2 lines

builtin/rebase.c +0 -0

 	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.

Junio C Hamano

2019-01-04 18:40 UTC #8 lore raw

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

Quoted 85 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
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" ;-)
Quoted 10 lines
Signed-off-by Johannes Schindelin <[email protected]>
---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
builtin/rebase.c +0 -0
index 768bea0da8..303175fdf1 100644
builtin/rebase.c static void add_var(struct strbuf *buf, const char *name, const char *value) · line 340
Quoted 4 lines
builtin/rebase.c +1 -0
@@ -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.
Quoted 11 lines
builtin/rebase.c +1 -0
 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;
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 367
Quoted 8 lines
builtin/rebase.c +1 -1
@@ -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.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 379
Quoted 7 lines
builtin/rebase.c +3 -0
@@ -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.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 421
Quoted 9 lines
builtin/rebase.c +1 -0
 	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;
@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,
 		goto leave_reset_head;
 	}
 
+reset_head_refs:
 	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
And the control continues from the point we update the reflog. Makes sense.

Quoted 7 lines

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 423
Quoted 2 lines
builtin/rebase.c +0 -0
 	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.

Johannes Schindelin

2019-01-18 14:18 UTC #9 lore raw

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

In reply to Junio C Hamano

Hi Junio,

Quoted 9 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
This is what the legacy (scripted) rebase does in move_to_original_branch, and we will need this functionality in the next commit.

Quoted 7 lines

The move-to-original-branch helper does:
- point $head_name to the commit pointed at by HEAD
- point HEAD symref to $head_name
without touching the index or the working tree files. It's not exactly "reset --soft" but more like "switch-branch --soft" ;-)

Right, but it does reset the HEAD softly.

Quoted 11 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

builtin/rebase.c +0 -0

index 768bea0da8..303175fdf1 100644
@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)

Quoted 7 lines

builtin/rebase.c static void add_var(struct strbuf *buf, const char *name, const char *value) · line 340
Quoted 3 lines
builtin/rebase.c +1 -0
 #define RESET_HEAD_DETACH (1<<0)
 #define RESET_HEAD_HARD (1<<1)
+#define RESET_HEAD_REFS_ONLY (1<<2)
In the future codebase in 'pu', we have 1<<2 already taken by another topic, so I'll tell my rerere database that the bit assignment needs to be adjusted.

Okay.

Quoted 26 lines

Quoted 11 lines

builtin/rebase.c +1 -0

 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;

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 367

Quoted 8 lines

builtin/rebase.c +1 -1

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

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

Quoted 4 lines

builtin/rebase.c +0 -0

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

Quoted 6 lines

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 379
Quoted 3 lines
builtin/rebase.c +3 -0
+	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.

Quoted 16 lines

In any case, not touching the index nor the working tree, so no need to call into the unpack_trees machinery. Makes sense.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 421
Quoted 9 lines
builtin/rebase.c +1 -0
 	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;
@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,
 		goto leave_reset_head;
 	}
 
+reset_head_refs:
 	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
And the control continues from the point we update the reflog. Makes sense.

Quoted 7 lines

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 423
Quoted 2 lines
builtin/rebase.c +0 -0
 	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

Johannes Schindelin via GitGitGadget

author

2018-12-21 13:17 UTC #10 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

builtin/rebase.c +183 -0

index 303175fdf1..e327c30b6b 100644
@@ -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);

Signature · 1 line

gitgitgadget

Junio C Hamano

2019-01-04 18:38 UTC #11 lore raw

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

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 12 lines · writes:

builtin/rebase.c +12 -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", "");

builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 270

Quoted 10 lines

builtin/rebase.c +10 -0

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

builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 278

Quoted 8 lines

builtin/rebase.c +8 -0

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

Quoted 4 lines

builtin/rebase.c +0 -0

@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,
 	return ret;
 }

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 502

Quoted 10 lines

builtin/rebase.c +10 -0

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

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 510

Quoted 7 lines

builtin/rebase.c +7 -0

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

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 515

Quoted 4 lines

builtin/rebase.c +4 -0

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

Quoted 16 lines

builtin/rebase.c +12 -0

@@ -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)) {

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 545

Quoted 10 lines

builtin/rebase.c +10 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 555

Quoted 10 lines

builtin/rebase.c +10 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 560

Quoted 5 lines

builtin/rebase.c +5 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 564

Quoted 3 lines

builtin/rebase.c +3 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 571

Quoted 7 lines

builtin/rebase.c +6 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 574

Quoted 3 lines

builtin/rebase.c +3 -0

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

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

Quoted 7 lines

builtin/rebase.c +7 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 591

Quoted 10 lines

builtin/rebase.c +10 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 600

Quoted 8 lines

builtin/rebase.c +8 -0

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

Quoted 6 lines

builtin/rebase.c +6 -0

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

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 616

Quoted 10 lines

builtin/rebase.c +10 -0

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

Quoted 34 lines

builtin/rebase.c +30 -0

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

builtin/rebase.c static int run_specific_rebase(struct rebase_options *opts) · line 733

Quoted 10 lines

builtin/rebase.c +5 -0

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

Johannes Schindelin

2019-01-18 14:16 UTC #12 lore raw

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

Hi Junio,

Quoted 30 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 12 lines · writes:

builtin/rebase.c +12 -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", "");

builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 270

Quoted 10 lines

builtin/rebase.c +10 -0

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

Quoted 6 lines

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

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

Quoted 38 lines

Quoted 4 lines
builtin/rebase.c +0 -0
@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,
 	return ret;
 }
 
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 502
Quoted 10 lines
builtin/rebase.c +10 -0
+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.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 510
Quoted 7 lines
builtin/rebase.c +7 -0
+	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?

Quoted 10 lines

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

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

Quoted 8 lines

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 515
Quoted 5 lines
builtin/rebase.c +4 -0
+	strbuf_release(&orig_head_reflog);
+	strbuf_release(&head_reflog);
+	return ret;
+}
Unlike the scripted version, this does not die() upon failure, so the caller needs to be careful about the returned status.

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

Quoted 36 lines

Quoted 17 lines
builtin/rebase.c +12 -0
@@ -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)) {
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 545
Quoted 10 lines
builtin/rebase.c +10 -0
+		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.

Quoted 5 lines

It's not like move_to_original_branch() wants to call read_cache() and get the result from the "am" that run_command() executed, either.
Puzzled. Care to explain a bit more in the in-code comment?

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

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

Quoted 23 lines

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 555
Quoted 11 lines
builtin/rebase.c +10 -0
+	}
+	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.
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 560
Quoted 5 lines
builtin/rebase.c +5 -0
+	}
+	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+		argv_array_push(&am.args, "--show-current-patch");
+		return run_command(&am);
+	}
Up to this point, it is a faithful conversion of the first case/esac statement. Good.

Quoted 8 lines

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 564
Quoted 3 lines
builtin/rebase.c +3 -0
+	strbuf_addf(&revisions, "%s...%s",
+		    oid_to_hex(opts->root ?
+			       /* this is now equivalent to ! -z "$upstream" */
Does "this" refer to the "opts->root being true" check?
Because you are flipping the polarity of the test from scripted version, shouldn't the comment be updated to "-z $upstream"?

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

Quoted 5 lines

Quoted 4 lines

builtin/rebase.c +3 -0

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

Quoted 10 lines

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 571
Quoted 3 lines
builtin/rebase.c +3 -0
+	rebased_patches = xstrdup(git_path("rebased-patches"));
+	format_patch.out = open(rebased_patches,
+				O_WRONLY | O_CREAT | O_TRUNC, 0666);
Unlike scripted version, we do not remove a (possibly) existing file. We give CREAT in case there is no existing one, and TRUNC in case there is an existing one. Makes sense. A more faithful translation would have unlink(2)ed a (possibly) existing one, and then because we can afford to, passed O_EXCL to avoid stomping on somebody else racing with us, but I do not think it is worth it.

Okay.

Quoted 5 lines

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 574
Quoted 3 lines
builtin/rebase.c +3 -0
+	if (format_patch.out < 0) {
+		status = error_errno(_("could not write '%s'"),
+				     rebased_patches);
s/write '%s'/open '%s' for writing/? I dunno.

Yep, of course! Will fix.

Quoted 23 lines

Quoted 7 lines
builtin/rebase.c +7 -0
+		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",
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 591
Quoted 10 lines
builtin/rebase.c +10 -0
+			 "--full-index", "--cherry-pick", "--right-only",
+			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
+			 "--no-cover-letter", "--pretty=mboxrd", NULL);
+	if (opts->git_format_patch_opt.len)
+		argv_array_split(&format_patch.args,
+				 opts->git_format_patch_opt.buf);
+	argv_array_push(&format_patch.args, revisions.buf);
+	if (opts->restrict_revision)
+		argv_array_pushf(&format_patch.args, "^%s",
+				 oid_to_hex(&opts->restrict_revision->object.oid));
It is kinda surprising to see that we have learned quite a lot of fringe "configurations" we need to explicitly override like this.
Looks like a quite faithful conversion, anyway.

Quoted 12 lines

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 600
Quoted 8 lines
builtin/rebase.c +8 -0
+	status = run_command(&format_patch);
+	if (status) {
+		unlink(rebased_patches);
+		free(rebased_patches);
+		argv_array_clear(&am.args);
+
+		reset_head(&opts->orig_head, "checkout", opts->head_name, 0,
+			   "HEAD", NULL);
This one may need to trigger post-checkout hook. The scripted version does two different things depending on the value of $head_name, but we can just use the same code without conditional?

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

Quoted 10 lines

builtin/rebase.c +10 -0

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

Quoted 8 lines

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 616
Quoted 6 lines
builtin/rebase.c +6 -0
+	strbuf_release(&revisions);
+
+	am.in = open(rebased_patches, O_RDONLY);
+	if (am.in < 0) {
+		status = error_errno(_("could not read '%s'"),
+				     rebased_patches);
s/write '%s'/open '%s' for reading/? I dunno.

Yep, will fix.

Quoted 53 lines

Quoted 35 lines

builtin/rebase.c +30 -0

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

builtin/rebase.c static int run_specific_rebase(struct rebase_options *opts) · line 733

Quoted 10 lines

builtin/rebase.c +5 -0

@@ -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);

Quoted 3 lines

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

Junio C Hamano

2019-01-18 17:59 UTC #13 lore raw

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

Quoted 10 lines · Johannes Schindelin

Quoted 9 lines
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.

Quoted 5 lines

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 ;-)

Johannes Schindelin

2019-01-18 18:14 UTC #14 lore raw

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

Hi Junio,

Quoted 15 lines · Junio C Hamano

Quoted 13 lines · Johannes Schindelin
Quoted 9 lines
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).

Quoted 8 lines

Quoted 2 lines
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

Junio C Hamano

2019-01-04 23:19 UTC #15 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 4 lines · 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.

...

Quoted 1 line

 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?

Junio C Hamano

2019-01-07 17:19 UTC #16 lore raw

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

Quoted 8 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · 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.

Quoted 7 lines

...
Quoted 1 line
 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.

Johannes Schindelin via GitGitGadget

author

2019-01-18 15:09 UTC #17 lore raw

v2 cover Let the builtin rebase call the git am command directly

In reply to 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.

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.

 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 <[email protected]>

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 <[email protected]>
      
       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);
      +	}
      +

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-01-18 15:09 UTC #18 lore raw

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

From: Johannes Schindelin <[email protected]>

So let's move it before that latter function.

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

Signed-off-by Johannes Schindelin <[email protected]>

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

builtin/rebase.c +119 ~~-119~~

index 00de70365e..efa90ca894 100644
@@ -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;

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-01-18 15:09 UTC #19 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

builtin/rebase.c +4 -3

index efa90ca894..5c827b2f03 100644
@@ -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:

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-01-18 15:09 UTC #20 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

builtin/rebase.c +7 -1

index 5c827b2f03..c65493a484 100644
@@ -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;

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-01-18 15:09 UTC #21 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

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 <[email protected]>

---
 builtin/rebase.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 179 insertions(+)

builtin/rebase.c +179 -0

index c65493a484..0727ddaf00 100644
@@ -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);

Signature · 1 line

gitgitgadget

Alban Gruin

2019-01-26 21:13 UTC #22 lore raw

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

Hi Johannes,

Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :

Quoted 5 lines

-%<-

builtin/rebase.c +3 -1

+static int run_am(struct rebase_options *opts)
+{
-%<-
+

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 642

Quoted 10 lines

builtin/rebase.c +10 -0

+	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

Johannes Schindelin

2019-01-29 09:46 UTC #23 lore raw

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

Hi Alban,

Quoted 13 lines · Alban Gruin

Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :
Quoted 12 lines
-%<-
builtin/rebase.c +10 -1
+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);
+

Quoted 8 lines

builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 551
Quoted 3 lines
builtin/rebase.c +3 -0
+	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

Junio C Hamano

2019-01-18 18:06 UTC #24 lore raw

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 9 lines · 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.

Quoted 3 lines

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

Quoted 2 lines

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

Good.

Quoted 4 lines

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

Quoted 2 lines

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

OK.

Quoted 3 lines

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

Junio C Hamano

2019-01-18 20:13 UTC #25 lore raw

Quoted 18 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 9 lines · 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.
Quoted 3 lines
Changes since v1:
* Rebased on top of master to avoid merge conflicts.
I do not appreciate this very much.

Quoted 8 lines

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.

Quoted 1 line

Will take a look. Thanks.

They were pleasant and smooth read. Crafted nicely.

Thanks.

Johannes Schindelin

2019-01-18 20:31 UTC #26 lore raw

In reply to Junio C Hamano

Hi Junio,

Quoted 16 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 9 lines · 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.
Quoted 3 lines
Changes since v1:
* Rebased on top of master to avoid merge conflicts.

Quoted 7 lines

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

Quoted 29 lines

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