rebase: reset_head() related fixes and improvements

Fix some issues with the implementation and use of reset_head(). The last patch was previously posted as [1], I have updated the commit message and rebased it onto the fixes in this series. There are a couple of small conflicts merging this into seen, I think they should be easy to resolve (in rebase.c take both sides in reset.c take the changed lines from each side). These patches are based on pw/rebase-of-a-tag-fix

[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/

Phillip Wood (11):
rebase: factor out checkout for up to date branch reset_head(): fix checkout
reset_head(): don't run checkout hook if there is an error reset_head(): remove action parameter reset_head(): factor out ref updates reset_head(): make default_reflog_action optional
rebase: cleanup reset_head() calls
reset_head(): take struct rebase_head_opts rebase --apply: fix reflog
rebase --apply: set ORIG_HEAD correctly
rebase -m: don't fork git checkout

 builtin/merge.c            |   6 +-
 builtin/rebase.c           |  97 +++++++++++++++----------
 reset.c                    | 143 ++++++++++++++++++++++---------------
 reset.h                    |  33 +++++++--
 sequencer.c                |  48 ++++---------
 sequencer.h                |   3 +-
 t/t3406-rebase-message.sh  |  23 ++++++
 t/t3418-rebase-continue.sh |  26 +++++++
 8 files changed, 240 insertions(+), 139 deletions(-)

base-commit: 7740ac691d8e7f1bed67bcbdb1ee5c5c618f7373
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1049

gitgitgadget

From: Phillip Wood <[email protected]>

This code is heavily indented and it will be convenient later in the series to have it in its own function.

---
 builtin/rebase.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

index f82bfaed118..f4c312dd8b5 100644
@@ -1099,6 +1099,23 @@ static int rebase_config(const char *var, const char *value, void *data)
 	return git_default_config(var, value, data);
 }
 
+static int checkout_up_to_date(struct rebase_options *options)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int ret = 0;
+
+	strbuf_addf(&buf, "%s: checkout %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+		    options->switch_to);
+	if (reset_head(the_repository, &options->orig_head, "checkout",
+		       options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
+		       NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0)
+		ret = error(_("could not switch to %s"), options->switch_to);
+	strbuf_release(&buf);
+
+	return ret;
+}
+
 /*
  * Determines whether the commits in from..to are linear, i.e. contain
  * no merge commits. This function *expects* `from` to be an ancestor of

@@ -1978,21 +1995,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (!(options.flags & REBASE_FORCE)) {
 			/* Lazily switch to the target branch if needed... */
 			if (options.switch_to) {
-				strbuf_reset(&buf);
-				strbuf_addf(&buf, "%s: checkout %s",
-					    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
-					    options.switch_to);
-				if (reset_head(the_repository,
-					       &options.orig_head, "checkout",
-					       options.head_name,
-					       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
-					       NULL, buf.buf,
-					       DEFAULT_REFLOG_ACTION) < 0) {
-					ret = error(_("could not switch to "
-							"%s"),
-						      options.switch_to);
+				ret = checkout_up_to_date(&options);
+				if (ret)
 					goto cleanup;
-				}
 			}
 
 			if (!(options.flags & REBASE_NO_QUIET))

gitgitgadget

From: Phillip Wood <[email protected]>

The hook should only be run if the worktree and refs were successfully updated.

---
 reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fc4dae3fd2d..5abb1a5683e 100644
@@ -125,7 +125,7 @@ reset_head_refs:
 			ret = create_symref("HEAD", switch_to_branch,
 					    reflog_head);
 	}
-	if (run_hook)
+	if (!ret && run_hook)
 		run_hook_le(NULL, "post-checkout",
 			    oid_to_hex(orig ? orig : null_oid()),
 			    oid_to_hex(oid), "1", NULL);

gitgitgadget

From: Phillip Wood <[email protected]>

The action parameter is passed as the command name to setup_unpack_trees_porcelain(). All but two cases pass either "checkout" or "reset". The case that passes "reset --hard" should be passing "reset" instead. The case that passes "Fast-forwarded" is only updating HEAD and so does not call unpack_trees(). The value can be determined by checking whether flags contains RESET_HEAD_HARD so it does not need to be specified by the caller.

---
 builtin/rebase.c | 14 +++++++-------
 reset.c          |  5 +++--
 reset.h          |  2 +-
 sequencer.c      |  3 +--
 4 files changed, 12 insertions(+), 12 deletions(-)

index f4c312dd8b5..26f53c55cc4 100644
@@ -789,7 +789,7 @@ static int move_to_original_branch(struct rebase_options *opts)
 		    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(the_repository, NULL, "", opts->head_name,
+	ret = reset_head(the_repository, NULL, opts->head_name,
 			 RESET_HEAD_REFS_ONLY,
 			 orig_head_reflog.buf, head_reflog.buf,
 			 DEFAULT_REFLOG_ACTION);

@@ -880,7 +880,7 @@ static int run_am(struct rebase_options *opts)
 		free(rebased_patches);
 		strvec_clear(&am.args);
 
-		reset_head(the_repository, &opts->orig_head, "checkout",
+		reset_head(the_repository, &opts->orig_head,
 			   opts->head_name, 0,
 			   "HEAD", NULL, DEFAULT_REFLOG_ACTION);
 		error(_("\ngit encountered an error while preparing the "

@@ -1107,7 +1107,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);
-	if (reset_head(the_repository, &options->orig_head, "checkout",
+	if (reset_head(the_repository, &options->orig_head,
 		       options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 		       NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0)
 		ret = error(_("could not switch to %s"), options->switch_to);

@@ -1557,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
-		if (reset_head(the_repository, NULL, "reset", NULL, RESET_HEAD_HARD,
+		if (reset_head(the_repository, NULL, NULL, RESET_HEAD_HARD,
 			       NULL, NULL, DEFAULT_REFLOG_ACTION) < 0)
 			die(_("could not discard worktree changes"));
 		remove_branch_state(the_repository, 0);

@@ -1575,7 +1575,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (read_basic_state(&options))
 			exit(1);
-		if (reset_head(the_repository, &options.orig_head, "reset",
+		if (reset_head(the_repository, &options.orig_head,
 			       options.head_name, RESET_HEAD_HARD,
 			       NULL, NULL, DEFAULT_REFLOG_ACTION) < 0)
 			die(_("could not move back to %s"),

@@ -2064,7 +2064,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
-	if (reset_head(the_repository, &options.onto->object.oid, "checkout", NULL,
+	if (reset_head(the_repository, &options.onto->object.oid, NULL,
 		       RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 		       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 		       NULL, msg.buf, DEFAULT_REFLOG_ACTION))

@@ -2082,7 +2082,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
 			options.head_name ? options.head_name : "detached HEAD",
 			oid_to_hex(&options.onto->object.oid));
-		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
+		reset_head(the_repository, NULL, options.head_name,
 			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
 			   DEFAULT_REFLOG_ACTION);
 		strbuf_release(&msg);

index 5abb1a5683e..9ab007c0c34 100644
@@ -8,7 +8,7 @@
 #include "tree.h"
 #include "unpack-trees.h"
 
-int reset_head(struct repository *r, struct object_id *oid, const char *action,
+int reset_head(struct repository *r, struct object_id *oid,
 	       const char *switch_to_branch, unsigned flags,
 	       const char *reflog_orig_head, const char *reflog_head,
 	       const char *default_reflog_action)

@@ -23,7 +23,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	struct lock_file lock = LOCK_INIT;
 	struct unpack_trees_options unpack_tree_opts = { 0 };
 	struct tree *tree;
-	const char *reflog_action;
+	const char *action, *reflog_action;
 	struct strbuf msg = STRBUF_INIT;
 	size_t prefix_len;
 	struct object_id *orig = NULL, oid_orig,

@@ -49,6 +49,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	if (refs_only)
 		goto reset_head_refs;
 
+	action = reset_hard ? "reset" : "checkout";
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;
 	unpack_tree_opts.src_index = r->index;

index 12f83c78e28..2daec804259 100644
@@ -12,7 +12,7 @@
 #define RESET_HEAD_REFS_ONLY (1<<3)
 #define RESET_ORIG_HEAD (1<<4)
 
-int reset_head(struct repository *r, struct object_id *oid, const char *action,
+int reset_head(struct repository *r, struct object_id *oid,
 	       const char *switch_to_branch, unsigned flags,
 	       const char *reflog_orig_head, const char *reflog_head,
 	       const char *default_reflog_action);

index 68316636921..07d2a582d39 100644
@@ -4130,8 +4130,7 @@ void create_autostash(struct repository *r, const char *path,
 			    path);
 		write_file(path, "%s", oid_to_hex(&oid));
 		printf(_("Created autostash: %s\n"), buf.buf);
-		if (reset_head(r, NULL, "reset --hard",
-			       NULL, RESET_HEAD_HARD, NULL, NULL,
+		if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
 			       default_reflog_action) < 0)
 			die(_("could not reset --hard"));
 

gitgitgadget

From: Phillip Wood <[email protected]>

In the next commit we will stop trying to update HEAD when we are just clearing changes from the working tree. Move the code that updates the refs to its own function in preparation for that.

---
 reset.c | 112 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 62 insertions(+), 50 deletions(-)

index 9ab007c0c34..668c7639127 100644
@@ -8,26 +8,75 @@
 #include "tree.h"
 #include "unpack-trees.h"
 
+static int update_refs(const struct object_id *oid, const char *switch_to_branch,
+		       const char *reflog_head, const char *reflog_orig_head,
+		       const char *default_reflog_action, unsigned flags)
+{
+	unsigned detach_head = flags & RESET_HEAD_DETACH;
+	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
+	struct object_id *orig = NULL, oid_orig, *old_orig = NULL, oid_old_orig;
+	struct strbuf msg = STRBUF_INIT;
+	const char *reflog_action;
+	size_t prefix_len;
+	int ret;
+
+	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : default_reflog_action);
+	prefix_len = msg.len;
+
+	if (update_orig_head) {
+		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 = update_ref(reflog_head, switch_to_branch, oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+		if (!ret)
+			ret = create_symref("HEAD", switch_to_branch,
+					    reflog_head);
+	}
+	if (!ret && run_hook)
+		run_hook_le(NULL, "post-checkout",
+			    oid_to_hex(orig ? orig : null_oid()),
+			    oid_to_hex(oid), "1", NULL);
+	strbuf_release(&msg);
+	return ret;
+}
+
 int reset_head(struct repository *r, struct object_id *oid,
 	       const char *switch_to_branch, unsigned flags,
 	       const char *reflog_orig_head, const char *reflog_head,
 	       const char *default_reflog_action)
 {
-	unsigned detach_head = flags & RESET_HEAD_DETACH;
 	unsigned reset_hard = flags & RESET_HEAD_HARD;
-	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
-	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
 	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 = { 0 };
 	struct tree *tree;
-	const char *action, *reflog_action;
-	struct strbuf msg = STRBUF_INIT;
-	size_t prefix_len;
-	struct object_id *orig = NULL, oid_orig,
-		*old_orig = NULL, oid_old_orig;
+	const char *action;
 	int ret = 0, nr = 0;
 
 	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))

@@ -47,7 +96,9 @@ int reset_head(struct repository *r, struct object_id *oid,
 		oid = &head_oid;
 
 	if (refs_only)
-		goto reset_head_refs;
+		return update_refs(oid, switch_to_branch, reflog_head,
+				   reflog_orig_head, default_reflog_action,
+				   flags);
 
 	action = reset_hard ? "reset" : "checkout";
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);

@@ -90,49 +141,10 @@ int reset_head(struct repository *r, struct object_id *oid,
 		goto leave_reset_head;
 	}
 
-reset_head_refs:
-	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : default_reflog_action);
-	prefix_len = msg.len;
-
-	if (update_orig_head) {
-		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 = update_ref(reflog_head, switch_to_branch, oid,
-				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-		if (!ret)
-			ret = create_symref("HEAD", switch_to_branch,
-					    reflog_head);
-	}
-	if (!ret && run_hook)
-		run_hook_le(NULL, "post-checkout",
-			    oid_to_hex(orig ? orig : null_oid()),
-			    oid_to_hex(oid), "1", NULL);
+	ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head,
+			  default_reflog_action, flags);
 
 leave_reset_head:
-	strbuf_release(&msg);
 	rollback_lock_file(&lock);
 	clear_unpack_trees_porcelain(&unpack_tree_opts);
 	while (nr)

gitgitgadget

From: Phillip Wood <[email protected]>

This parameter is only needed when a ref is going to be updated and the caller does not pass an explicit reflog message. Callers that are just discarding changes in the working tree like create_autostash() do not update any refs so should not have to worry about passing this parameter.

---
 builtin/merge.c  |  6 ++----
 builtin/rebase.c | 14 ++++++--------
 reset.c          | 16 ++++++++++++----
 sequencer.c      |  6 +++---
 sequencer.h      |  3 +--
 5 files changed, 24 insertions(+), 21 deletions(-)

index d2c52b6e971..35833a963fc 100644
@@ -1561,8 +1561,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		if (autostash)
 			create_autostash(the_repository,
-					 git_path_merge_autostash(the_repository),
-					 "merge");
+					 git_path_merge_autostash(the_repository));
 		if (checkout_fast_forward(the_repository,
 					  &head_commit->object.oid,
 					  &commit->object.oid,

@@ -1632,8 +1631,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	if (autostash)
 		create_autostash(the_repository,
-				 git_path_merge_autostash(the_repository),
-				 "merge");
+				 git_path_merge_autostash(the_repository));
 
 	/* We are going to make a new commit. */
 	git_committer_info(IDENT_STRICT);

index 26f53c55cc4..1a6af508f49 100644
@@ -791,8 +791,7 @@ static int move_to_original_branch(struct rebase_options *opts)
 		    opts->head_name);
 	ret = reset_head(the_repository, NULL, opts->head_name,
 			 RESET_HEAD_REFS_ONLY,
-			 orig_head_reflog.buf, head_reflog.buf,
-			 DEFAULT_REFLOG_ACTION);
+			 orig_head_reflog.buf, head_reflog.buf, NULL);
 
 	strbuf_release(&orig_head_reflog);
 	strbuf_release(&head_reflog);

@@ -1109,7 +1108,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 		    options->switch_to);
 	if (reset_head(the_repository, &options->orig_head,
 		       options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
-		       NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0)
+		       NULL, buf.buf, NULL) < 0)
 		ret = error(_("could not switch to %s"), options->switch_to);
 	strbuf_release(&buf);
 

@@ -1558,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		string_list_clear(&merge_rr, 1);
 
 		if (reset_head(the_repository, NULL, NULL, RESET_HEAD_HARD,
-			       NULL, NULL, DEFAULT_REFLOG_ACTION) < 0)
+			       NULL, NULL, NULL) < 0)
 			die(_("could not discard worktree changes"));
 		remove_branch_state(the_repository, 0);
 		if (read_basic_state(&options))

@@ -1964,8 +1963,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		die(_("could not read index"));
 
 	if (options.autostash) {
-		create_autostash(the_repository, state_dir_path("autostash", &options),
-				 DEFAULT_REFLOG_ACTION);
+		create_autostash(the_repository,
+				 state_dir_path("autostash", &options));
 	}
 
 	if (require_clean_work_tree(the_repository, "rebase",

@@ -2083,8 +2082,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			options.head_name ? options.head_name : "detached HEAD",
 			oid_to_hex(&options.onto->object.oid));
 		reset_head(the_repository, NULL, options.head_name,
-			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
-			   DEFAULT_REFLOG_ACTION);
+			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf, NULL);
 		strbuf_release(&msg);
 		ret = finish_rebase(&options);
 		goto cleanup;

index 668c7639127..be4e18eed98 100644
@@ -21,8 +21,13 @@ static int update_refs(const struct object_id *oid, const char *switch_to_branch
 	size_t prefix_len;
 	int ret;
 
-	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : default_reflog_action);
+	if ((update_orig_head && !reflog_orig_head) || !reflog_head) {
+		if (!default_reflog_action)
+			BUG("default_reflog_action must be given when reflog messages are omitted");
+		reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+		strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action :
+							  default_reflog_action);
+	}
 	prefix_len = msg.len;
 
 	if (update_orig_head) {

@@ -71,6 +76,7 @@ int reset_head(struct repository *r, struct object_id *oid,
 {
 	unsigned reset_hard = flags & RESET_HEAD_HARD;
 	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
+	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
 	struct object_id head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;

@@ -141,8 +147,10 @@ int reset_head(struct repository *r, struct object_id *oid,
 		goto leave_reset_head;
 	}
 
-	ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head,
-			  default_reflog_action, flags);
+	if (oid != &head_oid || update_orig_head || switch_to_branch)
+		ret = update_refs(oid, switch_to_branch, reflog_head,
+				  reflog_orig_head, default_reflog_action,
+				  flags);
 
 leave_reset_head:
 	rollback_lock_file(&lock);

index 07d2a582d39..56a204f18c5 100644
@@ -4093,8 +4093,8 @@ static enum todo_command peek_command(struct todo_list *todo_list, int offset)
 	return -1;
 }
 
-void create_autostash(struct repository *r, const char *path,
-		      const char *default_reflog_action)
+void create_autostash(struct repository *r, const char *path)
+
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct lock_file lock_file = LOCK_INIT;

@@ -4131,7 +4131,7 @@ void create_autostash(struct repository *r, const char *path,
 		write_file(path, "%s", oid_to_hex(&oid));
 		printf(_("Created autostash: %s\n"), buf.buf);
 		if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
-			       default_reflog_action) < 0)
+			       NULL) < 0)
 			die(_("could not reset --hard"));
 
 		if (discard_index(r->index) < 0 ||

index 2088344cb37..92357536ff4 100644
@@ -198,8 +198,7 @@ void commit_post_rewrite(struct repository *r,
 			 const struct commit *current_head,
 			 const struct object_id *new_head);
 
-void create_autostash(struct repository *r, const char *path,
-		      const char *default_reflog_action);
+void create_autostash(struct repository *r, const char *path);
 int save_autostash(const char *path);
 int apply_autostash(const char *path);
 int apply_autostash_oid(const char *stash_oid);

gitgitgadget

From: Phillip Wood <[email protected]>

move_to_original_branch() passes the message intended for the branch reflog as orig_head_msg. Fix this by adding a branch_msg member to struct reset_head_opts and add a regression test. Note that these reflog messages do not respect GIT_REFLOG_ACTION. They are not alone in that and will be fixed in a future series.

The "merge" backend already has tests that check both the branch and HEAD reflogs.

---
 builtin/rebase.c          |  8 ++++----
 reset.c                   | 12 ++++++++++--
 reset.h                   |  2 ++
 t/t3406-rebase-message.sh | 23 +++++++++++++++++++++++
 4 files changed, 39 insertions(+), 6 deletions(-)

index ae5de271e9a..c40a8b843f4 100644
@@ -776,7 +776,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 
 static int move_to_original_branch(struct rebase_options *opts)
 {
-	struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
+	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
 	struct reset_head_opts ropts = { 0 };
 	int ret;
 

@@ -786,17 +786,17 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&orig_head_reflog, "rebase finished: %s onto %s",
+	strbuf_addf(&branch_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);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
-	ropts.orig_head_msg = orig_head_reflog.buf;
+	ropts.branch_msg = branch_reflog.buf;
 	ropts.head_msg = head_reflog.buf;
 	ret = reset_head(the_repository, &ropts);
 
-	strbuf_release(&orig_head_reflog);
+	strbuf_release(&branch_reflog);
 	strbuf_release(&head_reflog);
 	return ret;
 }

index e77a8ac423d..2c32600234d 100644
@@ -15,6 +15,7 @@ static int update_refs(const struct reset_head_opts *opts,
 	unsigned run_hook = opts->flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
 	const char *switch_to_branch = opts->branch;
+	const char *reflog_branch = opts->branch_msg;
 	const char *reflog_head = opts->head_msg;
 	const char *reflog_orig_head = opts->orig_head_msg;
 	const char *default_reflog_action = opts->default_reflog_action;

@@ -58,8 +59,9 @@ static int update_refs(const struct reset_head_opts *opts,
 				 detach_head ? REF_NO_DEREF : 0,
 				 UPDATE_REFS_MSG_ON_ERR);
 	else {
-		ret = update_ref(reflog_head, switch_to_branch, oid,
-				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+		ret = update_ref(reflog_branch ? reflog_branch : reflog_head,
+				 switch_to_branch, oid, NULL, 0,
+				 UPDATE_REFS_MSG_ON_ERR);
 		if (!ret)
 			ret = create_symref("HEAD", switch_to_branch,
 					    reflog_head);

@@ -90,6 +92,12 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
 	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
 		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
 
+	if (opts->orig_head_msg && !update_orig_head)
+		BUG("ORIG_HEAD reflog message given without updating ORIG_HEAD");
+
+	if (opts->branch_msg && !opts->branch)
+		BUG("branch reflog message given without a branch");
+
 	if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
 		ret = -1;
 		goto leave_reset_head;

index 996d7f569b6..c0f4e99a3be 100644
@@ -19,6 +19,8 @@ struct reset_head_opts {
 	const char *branch;
 	/* Flags defined above */
 	unsigned flags;
+	 /* Optional reflog message for branch, defaults to head_msg. */
+	const char *branch_msg;
 	 /*
 	  * Optional reflog message for HEAD, if this is not set then
 	  * default_reflog_action must be.

index 77a313f62eb..d17b450e811 100755
@@ -105,6 +105,29 @@ test_expect_success 'GIT_REFLOG_ACTION' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase --apply reflog' '
+	git checkout -b reflog-apply start &&
+	old_head_reflog="$(git log -g --format=%gs -1 HEAD)" &&
+
+	git rebase --apply Y &&
+
+	git log -g --format=%gs -4 HEAD >actual &&
+	cat >expect <<-EOF &&
+	rebase finished: returning to refs/heads/reflog-apply
+	rebase: Z
+	rebase: checkout Y
+	$old_head_reflog
+	EOF
+	test_cmp expect actual &&
+
+	git log -g --format=%gs -2 reflog-apply >actual &&
+	cat >expect <<-EOF &&
+	rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y)
+	branch: Created from start
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase -i onto unrelated history' '
 	git init unrelated &&
 	test_commit -C unrelated 1 &&

gitgitgadget

From: Phillip Wood <[email protected]>

This function already takes a confusingly large number of parameters some of which are optional or not always required. The following commits will add a couple more parameters so change it to take a struct of options first.

---
 builtin/rebase.c | 57 ++++++++++++++++++++++++++++++------------------
 reset.c          | 36 +++++++++++++++---------------
 reset.h          | 29 ++++++++++++++++++++----
 sequencer.c      |  5 ++---
 4 files changed, 80 insertions(+), 47 deletions(-)

index 89b348ffa75..ae5de271e9a 100644
@@ -777,6 +777,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 static int move_to_original_branch(struct rebase_options *opts)
 {
 	struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
+	struct reset_head_opts ropts = { 0 };
 	int ret;
 
 	if (!opts->head_name)

@@ -789,9 +790,11 @@ static int move_to_original_branch(struct rebase_options *opts)
 		    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(the_repository, NULL, opts->head_name,
-			 RESET_HEAD_REFS_ONLY,
-			 orig_head_reflog.buf, head_reflog.buf, NULL);
+	ropts.branch = opts->head_name;
+	ropts.flags = RESET_HEAD_REFS_ONLY;
+	ropts.orig_head_msg = orig_head_reflog.buf;
+	ropts.head_msg = head_reflog.buf;
+	ret = reset_head(the_repository, &ropts);
 
 	strbuf_release(&orig_head_reflog);
 	strbuf_release(&head_reflog);

@@ -875,13 +878,15 @@ static int run_am(struct rebase_options *opts)
 
 	status = run_command(&format_patch);
 	if (status) {
+		struct reset_head_opts ropts = { 0 };
 		unlink(rebased_patches);
 		free(rebased_patches);
 		strvec_clear(&am.args);
 
-		reset_head(the_repository, &opts->orig_head,
-			   opts->head_name, 0,
-			   NULL, NULL, DEFAULT_REFLOG_ACTION);
+		ropts.oid = &opts->orig_head;
+		ropts.branch = opts->head_name;
+		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+		reset_head(the_repository, &ropts);
 		error(_("\ngit encountered an error while preparing the "
 			"patches to replay\n"
 			"these revisions:\n"

@@ -1101,14 +1106,17 @@ static int rebase_config(const char *var, const char *value, void *data)
 static int checkout_up_to_date(struct rebase_options *options)
 {
 	struct strbuf buf = STRBUF_INIT;
+	struct reset_head_opts ropts = { 0 };
 	int ret = 0;
 
 	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);
-	if (reset_head(the_repository, &options->orig_head,
-		       options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
-		       NULL, buf.buf, NULL) < 0)
+	ropts.oid = &options->orig_head;
+	ropts.branch = options->head_name;
+	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+	ropts.head_msg = buf.buf;
+	if (reset_head(the_repository, &ropts) < 0)
 		ret = error(_("could not switch to %s"), options->switch_to);
 	strbuf_release(&buf);
 

@@ -1319,6 +1327,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	char *squash_onto_name = NULL;
 	int reschedule_failed_exec = -1;
 	int allow_preemptive_ff = 1;
+	struct reset_head_opts ropts = { 0 };
 	struct option builtin_rebase_options[] = {
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),

@@ -1555,9 +1564,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
-
-		if (reset_head(the_repository, NULL, NULL, RESET_HEAD_HARD,
-			       NULL, NULL, NULL) < 0)
+		ropts.flags = RESET_HEAD_HARD;
+		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not discard worktree changes"));
 		remove_branch_state(the_repository, 0);
 		if (read_basic_state(&options))

@@ -1574,9 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (read_basic_state(&options))
 			exit(1);
-		if (reset_head(the_repository, &options.orig_head,
-			       options.head_name, RESET_HEAD_HARD,
-			       NULL, NULL, DEFAULT_REFLOG_ACTION) < 0)
+		ropts.oid = &options.orig_head;
+		ropts.branch = options.head_name;
+		ropts.flags = RESET_HEAD_HARD;
+		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
 		remove_branch_state(the_repository, 0);

@@ -2063,10 +2073,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
-	if (reset_head(the_repository, &options.onto->object.oid, NULL,
-		       RESET_HEAD_DETACH | RESET_ORIG_HEAD |
-		       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
-		       NULL, msg.buf, DEFAULT_REFLOG_ACTION))
+	ropts.oid = &options.onto->object.oid;
+	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
+			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+	ropts.head_msg = msg.buf;
+	ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+	if (reset_head(the_repository, &ropts))
 		die(_("Could not detach HEAD"));
 	strbuf_release(&msg);
 

@@ -2081,8 +2093,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
 			options.head_name ? options.head_name : "detached HEAD",
 			oid_to_hex(&options.onto->object.oid));
-		reset_head(the_repository, NULL, options.head_name,
-			   RESET_HEAD_REFS_ONLY, NULL, msg.buf, NULL);
+		memset(&ropts, 0, sizeof(ropts));
+		ropts.branch = options.head_name;
+		ropts.flags = RESET_HEAD_REFS_ONLY;
+		ropts.head_msg = msg.buf;
+		reset_head(the_repository, &ropts);
 		strbuf_release(&msg);
 		ret = finish_rebase(&options);
 		goto cleanup;

index be4e18eed98..e77a8ac423d 100644
@@ -8,13 +8,16 @@
 #include "tree.h"
 #include "unpack-trees.h"
 
-static int update_refs(const struct object_id *oid, const char *switch_to_branch,
-		       const char *reflog_head, const char *reflog_orig_head,
-		       const char *default_reflog_action, unsigned flags)
+static int update_refs(const struct reset_head_opts *opts,
+		       const struct object_id *oid)
 {
-	unsigned detach_head = flags & RESET_HEAD_DETACH;
-	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
-	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
+	unsigned detach_head = opts->flags & RESET_HEAD_DETACH;
+	unsigned run_hook = opts->flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+	unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
+	const char *switch_to_branch = opts->branch;
+	const char *reflog_head = opts->head_msg;
+	const char *reflog_orig_head = opts->orig_head_msg;
+	const char *default_reflog_action = opts->default_reflog_action;
 	struct object_id *orig = NULL, oid_orig, *old_orig = NULL, oid_old_orig;
 	struct strbuf msg = STRBUF_INIT;
 	const char *reflog_action;

@@ -69,14 +72,13 @@ static int update_refs(const struct object_id *oid, const char *switch_to_branch
 	return ret;
 }
 
-int reset_head(struct repository *r, struct object_id *oid,
-	       const char *switch_to_branch, unsigned flags,
-	       const char *reflog_orig_head, const char *reflog_head,
-	       const char *default_reflog_action)
+int reset_head(struct repository *r, const struct reset_head_opts *opts)
 {
-	unsigned reset_hard = flags & RESET_HEAD_HARD;
-	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
-	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
+	const struct object_id *oid = opts->oid;
+	const char *switch_to_branch = opts->branch;
+	unsigned reset_hard = opts->flags & RESET_HEAD_HARD;
+	unsigned refs_only = opts->flags & RESET_HEAD_REFS_ONLY;
+	unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
 	struct object_id head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;

@@ -102,9 +104,7 @@ int reset_head(struct repository *r, struct object_id *oid,
 		oid = &head_oid;
 
 	if (refs_only)
-		return update_refs(oid, switch_to_branch, reflog_head,
-				   reflog_orig_head, default_reflog_action,
-				   flags);
+		return update_refs(opts, oid);
 
 	action = reset_hard ? "reset" : "checkout";
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);

@@ -148,9 +148,7 @@ int reset_head(struct repository *r, struct object_id *oid,
 	}
 
 	if (oid != &head_oid || update_orig_head || switch_to_branch)
-		ret = update_refs(oid, switch_to_branch, reflog_head,
-				  reflog_orig_head, default_reflog_action,
-				  flags);
+		ret = update_refs(opts, oid);
 
 leave_reset_head:
 	rollback_lock_file(&lock);

index 2daec804259..996d7f569b6 100644
@@ -12,9 +12,30 @@
 #define RESET_HEAD_REFS_ONLY (1<<3)
 #define RESET_ORIG_HEAD (1<<4)
 
-int reset_head(struct repository *r, struct object_id *oid,
-	       const char *switch_to_branch, unsigned flags,
-	       const char *reflog_orig_head, const char *reflog_head,
-	       const char *default_reflog_action);
+struct reset_head_opts {
+	/* The oid of the commit to checkout/reset to. Defaults to HEAD */
+	const struct object_id *oid;
+	 /* Optional branch to switch to */
+	const char *branch;
+	/* Flags defined above */
+	unsigned flags;
+	 /*
+	  * Optional reflog message for HEAD, if this is not set then
+	  * default_reflog_action must be.
+	  */
+	const char *head_msg;
+	/*
+	 * Optional reflog message for ORIG_HEAD, if this is not set and flags
+	 * contains RESET_ORIG_HEAD then default_reflog_action must be set.
+	 */
+	const char *orig_head_msg;
+	/*
+	 * Action to use in default reflog messages, only required if a ref is
+	 * being updated and the reflog messages above are omitted.
+	 */
+	const char *default_reflog_action;
+};
+
+int reset_head(struct repository *r, const struct reset_head_opts *opts);
 
 #endif

index 56a204f18c5..36ab6854c6b 100644
@@ -4109,6 +4109,7 @@ void create_autostash(struct repository *r, const char *path)
 	if (has_unstaged_changes(r, 1) ||
 	    has_uncommitted_changes(r, 1)) {
 		struct child_process stash = CHILD_PROCESS_INIT;
+		struct reset_head_opts ropts = { .flags = RESET_HEAD_HARD };
 		struct object_id oid;
 
 		strvec_pushl(&stash.args,

@@ -4130,10 +4131,8 @@ void create_autostash(struct repository *r, const char *path)
 			    path);
 		write_file(path, "%s", oid_to_hex(&oid));
 		printf(_("Created autostash: %s\n"), buf.buf);
-		if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
-			       NULL) < 0)
+		if (reset_head(r, &ropts) < 0)
 			die(_("could not reset --hard"));
-
 		if (discard_index(r->index) < 0 ||
 			repo_read_index(r) < 0)
 			die(_("could not read index"));

gitgitgadget

From: Phillip Wood <[email protected]>

At the start of a rebase ORIG_HEAD is updated to tip of the branch being rebased. Unfortunately reset_head() always uses the current value of HEAD for this which is incorrect if the rebase is started with 'git rebase <upstream> <branch>' as in that case ORIG_HEAD should be updated to <branch>. This only affects the "apply" backend as the "merge" backend does not yet use reset_head() for the initial checkout. Fix this by passing in orig_head when calling reset_head() and add some regression tests.

---
 builtin/rebase.c           |  1 +
 reset.c                    |  4 +++-
 reset.h                    |  2 ++
 t/t3418-rebase-continue.sh | 26 ++++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 1 deletion(-)

index c40a8b843f4..c36a8a10e9b 100644
@@ -2074,6 +2074,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
+	ropts.orig_head = &options.orig_head,
 	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	ropts.head_msg = msg.buf;

index 2c32600234d..82bcf2dcb8b 100644
@@ -14,6 +14,7 @@ static int update_refs(const struct reset_head_opts *opts,
 	unsigned detach_head = opts->flags & RESET_HEAD_DETACH;
 	unsigned run_hook = opts->flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
+	const struct object_id *orig_head = opts->orig_head;
 	const char *switch_to_branch = opts->branch;
 	const char *reflog_branch = opts->branch_msg;
 	const char *reflog_head = opts->head_msg;

@@ -43,7 +44,8 @@ static int update_refs(const struct reset_head_opts *opts,
 				strbuf_addstr(&msg, "updating ORIG_HEAD");
 				reflog_orig_head = msg.buf;
 			}
-			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
+			update_ref(reflog_orig_head, "ORIG_HEAD",
+				   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);

index c0f4e99a3be..91a868c5ba8 100644
@@ -15,6 +15,8 @@
 struct reset_head_opts {
 	/* The oid of the commit to checkout/reset to. Defaults to HEAD */
 	const struct object_id *oid;
+	/* Optional commit when setting ORIG_HEAD. Defaults to HEAD */
+	const struct object_id *orig_head;
 	 /* Optional branch to switch to */
 	const char *branch;
 	/* Flags defined above */

index 738fbae9b29..be63456c5b9 100755
@@ -323,4 +323,30 @@ test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
 	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
 '
 
+test_orig_head_helper() {
+	test_when_finished 'git rebase --abort &&
+		git checkout topic &&
+		git reset --hard commit-new-file-F2-on-topic-branch' &&
+	git update-ref -d ORIG_HEAD &&
+	test_must_fail git rebase "$@" &&
+	test_cmp_rev ORIG_HEAD commit-new-file-F2-on-topic-branch
+}
+
+test_orig_head() {
+	type=$1
+	test_expect_success "rebase $type sets ORIG_HEAD correctly" '
+		git checkout topic &&
+		git reset --hard commit-new-file-F2-on-topic-branch &&
+		test_orig_head_helper $type main
+	'
+
+	test_expect_success "rebase $type <upstream> <branch> sets ORIG_HEAD correctly" '
+		git checkout main &&
+		test_orig_head_helper $type main topic
+	'
+}
+
+test_orig_head --apply
+test_orig_head --merge
+
 test_done

gitgitgadget

From: Phillip Wood <[email protected]>

The reset bit should only be set if flags contains RESET_HEAD_HARD. The test for !deatch_head dates back to the original implementation of reset_head() in ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"", 2018-08-07) and was correct until e65123a71d ("builtin rebase: support git rebase <upstream> <switch-to>", 2018-09-04) started using reset_head() to checkout <switch-to> when fast-forwarding.

---
 reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 79310ae071b..fc4dae3fd2d 100644
@@ -57,7 +57,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unpack_tree_opts.update = 1;
 	unpack_tree_opts.merge = 1;
 	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
-	if (!detach_head)
+	if (reset_hard)
 		unpack_tree_opts.reset = 1;
 
 	if (repo_read_index_unmerged(r) < 0) {

gitgitgadget

From: Phillip Wood <[email protected]>

Now that reset_head() can handle the initial checkout of onto correctly use it in the "merge" backend instead of forking 'git checkout'. This opens the way for us to stop calling the post-checkout hook in the future. Not running 'git checkout' means that 'rebase -i/m' no longer recurse submodules when checking out 'onto' (thanks to Philippe Blain for pointing this out). As the rest of rebase does not know what to do with submodules this is probably a good thing. When using merge-ort rebase ought be able to handle submodules correctly if it parsed the submodule config, such a change is left for a future patch series.

The "apply" based rebase has avoided forking git checkout since ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"", 2018-08-07). The code that handles the checkout was moved into libgit by b309a97108 ("reset: extract reset_head() from rebase", 2020-04-07).

---
 sequencer.c | 38 +++++++++++---------------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

index 36ab6854c6b..9011dd84afd 100644
@@ -4217,42 +4217,26 @@ int apply_autostash_oid(const char *stash_oid)
 	return apply_save_autostash_oid(stash_oid, 1);
 }
 
-static int run_git_checkout(struct repository *r, struct replay_opts *opts,
-			    const char *commit, const char *action)
-{
-	struct child_process cmd = CHILD_PROCESS_INIT;
-	int ret;
-
-	cmd.git_cmd = 1;
-
-	strvec_push(&cmd.args, "checkout");
-	strvec_push(&cmd.args, commit);
-	strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
-
-	if (opts->verbose)
-		ret = run_command(&cmd);
-	else
-		ret = run_command_silent_on_success(&cmd);
-
-	if (!ret)
-		discard_index(r->index);
-
-	return ret;
-}
-
 static int checkout_onto(struct repository *r, struct replay_opts *opts,
 			 const char *onto_name, const struct object_id *onto,
 			 const struct object_id *orig_head)
 {
-	const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
-
-	if (run_git_checkout(r, opts, oid_to_hex(onto), action)) {
+	struct reset_head_opts ropts = {
+		.oid = onto,
+		.orig_head = orig_head,
+		.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
+				RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
+		.head_msg = reflog_message(opts, "start", "checkout %s",
+					   onto_name),
+		.default_reflog_action = "rebase"
+	};
+	if (reset_head(r, &ropts)) {
 		apply_autostash(rebase_path_autostash());
 		sequencer_remove_state(opts);
 		return error(_("could not detach HEAD"));
 	}
 
-	return update_ref(NULL, "ORIG_HEAD", orig_head, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+	return 0;
 }
 
 static int stopped_at_head(struct repository *r)

gitgitgadget

From: Phillip Wood <[email protected]>

If ORIG_HEAD is not set by passing RESET_ORIG_HEAD then there is no need to pass anything for reflog_orig_head. In addition to the callers fixed in this commit move_to_original_branch() also passes reflog_orig_head without setting ORIG_HEAD. That caller is mistakenly passing the message it wants to put in the branch reflog which is not currently possible so we delay fixing that caller until we can pass the message as the branch reflog.

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

index 1a6af508f49..89b348ffa75 100644
@@ -881,7 +881,7 @@ static int run_am(struct rebase_options *opts)
 
 		reset_head(the_repository, &opts->orig_head,
 			   opts->head_name, 0,
-			   "HEAD", NULL, DEFAULT_REFLOG_ACTION);
+			   NULL, NULL, DEFAULT_REFLOG_ACTION);
 		error(_("\ngit encountered an error while preparing the "
 			"patches to replay\n"
 			"these revisions:\n"

@@ -2082,7 +2082,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			options.head_name ? options.head_name : "detached HEAD",
 			oid_to_hex(&options.onto->object.oid));
 		reset_head(the_repository, NULL, options.head_name,
-			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf, NULL);
+			   RESET_HEAD_REFS_ONLY, NULL, msg.buf, NULL);
 		strbuf_release(&msg);
 		ret = finish_rebase(&options);
 		goto cleanup;

gitgitgadget

Sorry, but it is not quite clear what exactly is "fix checkout" in the context of this change, even with the above paragraph that describes the internals but not any end-user visible effect.

Can this step come with its own addition to t/ to demonstrate the breakage that is fixed?

OK. This is a behaviour change visible to end-users, and deserves a mention in the release notes.

- When "git rebase" attempted to check out a branch (or detached the HEAD) to work on, we used to always call the "post-checkout" hook, even if the checkout failed to update the ref. The hook is no longer called if the checkout fails.

or something.

Again, can the bug this step fixes be protected with a new test in t/ please?

Describe how the parameter is meant to be used (presumably "this is to record in the reflog", perhaps?); without such explanation, it is hard to either agree or disagree with the claim that "reset --hard" should be "reset".

Also state if this change is supposed to have any externally observable effect.

Perhaps this improves what is shown in an error message by affecting what setup_unpack_trees_porcelain() does? I am just guessing, because the proposed log message is not telling. Please do not make me (or other readers of "git log") guess.

Thanks.

"clearing changes" meaning...? Like "rebase --abort"?

This only talks about internal implementation details of passing a parameter that may not be used, but ...

Would all the members in the struct always be used by the function? If not, such a change will *not* solve the "confusingly ... required" problem at all.

I am not necessarily against a change to consolidate a bag of parameters into a pointer to a struct. I am against a change that is justified with a benefit that the change does not bring to us.

Thanks.

OK. This sounds like a good idea.

All correct. It is somewhat surprising that this wasn't caught and reported as an issue for this long X-<.

s/deatch_head/detach_head/

When merged with other topics in flight in 'seen', this seems to fail the t1092 test (most likely, ds/add-rm-with-sparse-index is what this interacts badly with).

Thanks.

There are Two CI runs at GitHub:

- https://github.com/git/git/actions/runs/1297117791 (d3a1c4e)
- https://github.com/git/git/actions/runs/1297232973 (a1108c2)

The difference between the former (which passes all the tests) and he latter (which fails) are only two topics:

    $ git log --first-parent --oneline d3a1c4e..a1108c2
    a1108c2b1b Merge branch 'hn/reftable' into seen
    e575f29006 Merge branch 'pw/fix-some-issues-in-reset-head' into seen

I think the following is the same failure I saw earlier

https://github.com/git/git/runs/3773780195?check_suite_focus=true#step:6:5033

that the write-tree codepath hits assertion failure by detecting a corruption in the cache-tree data structure.

e575f29006 (i.e. without the reftable topic) fails t1092. If you revert e575f29006^2 (i.e. the "do not fork 'git checkout'") from that merge, all tests pass including t1092.

The reftable topic is queued near the tip of 'seen' not necessarily because it _breaks_ CI (I do not think it does), but it needed a handful of fixup commits on top. The topic needs rerolling with the fixes squashed in.

Thanks.

Hi Junio

Oh dear, thanks for letting me know.

The test that fails (t1092-sparse-checkout-compatibility.sh:'merge, cherry-pick, and rebase') was introduced by c0b99303db ("t1092: add cherry-pick, rebase tests", 2021-09-08) and is in v2.33.0. It does not test the "apply" backend of rebase, changing it to do so makes it fail on the current master as that backend already uses reset_head() for the initial checkout so it is an existing bug that is exposed by the changes in this series. It seems to be a use-after-free (see below) I'll try and investigate but I've got no idea what is going on at the moment - the index is not my area of expertise.

Best Wishes

Phillip

==74345==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080 READ of size 4 at 0x606000001b20 thread T0

    #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863
    #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #4 0x557cbe830a2b in cache_tree_verify 
/home/phil/src/git/cache-tree.c:910
    #5 0x557cbea53741 in write_locked_index 
/home/phil/src/git/read-cache.c:3250
    #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
    #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
    #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
    #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
    #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
    #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
    #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
    #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
    #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d)

0x606000001b20 is located 0 bytes inside of 56-byte region [0x606000001b20,0x606000001b58) freed by thread T0 here:

    #0 0x7fdd4bacff19 in __interceptor_free 
/build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
    #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35
    #2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
    #3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
    #4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
    #5 0x557cbeb2557a in ensure_full_index 
/home/phil/src/git/sparse-index.c:310
    #6 0x557cbea45c4a in index_name_stage_pos 
/home/phil/src/git/read-cache.c:588
    #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850
    #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #11 0x557cbe830a2b in cache_tree_verify 
/home/phil/src/git/cache-tree.c:910
    #12 0x557cbea53741 in write_locked_index 
/home/phil/src/git/read-cache.c:3250
    #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
    #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
    #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
    #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
    #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
    #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
    #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
    #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

previously allocated by thread T0 here:

    #0 0x7fdd4bad0459 in __interceptor_calloc 
/build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140
    #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17
    #3 0x557cbe82b7d8 in prime_cache_tree_rec 
/home/phil/src/git/cache-tree.c:763
    #4 0x557cbe82b837 in prime_cache_tree_rec 
/home/phil/src/git/cache-tree.c:764
    #5 0x557cbe82b837 in prime_cache_tree_rec 
/home/phil/src/git/cache-tree.c:764
    #6 0x557cbe8304e1 in prime_cache_tree 
/home/phil/src/git/cache-tree.c:779
    #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85
    #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
    #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
    #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
    #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
    #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
    #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
    #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

SUMMARY: AddressSanitizer: heap-use-after-free /home/phil/src/git/cache-tree.c:863 in verify_one Shadow bytes around the buggy address:
0x0c0c7fff8310: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd 0x0c0c7fff8320: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa 0x0c0c7fff8330: fa fa fa fa 00 00 00 00 00 00 00 02 fa fa fa fa 0x0c0c7fff8340: fd fd fd fd fd fd fd fa fa fa fa fa 00 00 00 00 0x0c0c7fff8350: 00 00 00 02 fa fa fa fa fd fd fd fd fd fd fd fa
=>0x0c0c7fff8360: fa fa fa fa[fd]fd fd fd fd fd fd fa fa fa fa fa
0x0c0c7fff8370: 00 00 00 00 00 00 00 02 fa fa fa fa fd fd fd fd 0x0c0c7fff8380: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 02 0x0c0c7fff8390: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa 0x0c0c7fff83a0: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd 0x0c0c7fff83b0: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07

  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc

==74345==ABORTING

removing the call to prime_cache_tree() from reset_head() fixes the crash. It is called after unpack_trees() and before write_locked_index(), I'm not sure what the implications of removing it are - why it was there it is needed?

Best Wishes

Phillip

Am 02.10.21 um 14:27 schrieb Phillip Wood:

Looks like cache_tree_verify() indirectly triggered ensure_full_index(), which throws the cache_tree away, then kept going using stale pointers. It should instead stop and start over in such a case, no?

Hi Junio

"git checkout" refuses to overwrite untracked files but reset_head() does when checking out a branch.

I can add a test to check that a checkout does not remove untracked files. However such a test would pass on top of en/remaving-untracked-fixes without the fix in this patch. I cannot think of a way to specifically test that unpack_tree_opts.reset == 0 unless RESET_HEAD_HARD is given after en/removing-untracked-fixes is merged. Elijah's fixes will stop the "reset" mode of reset_head() from wiping untracked files which is good. The reset flag to unpack trees also affects unmerged entries but rebase does not try to checkout anything if the index contains unmerged entries.

Best Wishes

Phillip

Hi Junio

It only affects "git rebase --apply" as the "merge" backend forks "git checkout" which does not run the hook if it cannot update the worktree or refs.

I'll try and come up with something - it should be possible to arrange an untracked file to make unpack_trees() to fail, I'm not sure how we'd make update_ref() fail

Best Wishes

Phillip

Hi Junio

How about

The only use of the action parameter is to setup the error messages for unpack_trees(). All but two cases pass either "checkout" or "reset". The case that passes "reset --hard" would be better passing "reset" so that the error messages match the builtin reset command like all the other callers that are doing a reset. The case that passes "Fast-forwarded" is only updating HEAD and so the parameter is unused in that case as it does not call unpack_trees(). The value to pass to setup_unpack_trees_porcelain() can be determined by checking whether flags contains RESET_HEAD_HARD without the caller having to specify it.

It'll change the error message if we cannot clear the stashed changes form the working tree from saying "reset --hard" to "reset".

Best Wishes

Phillip

Hi Junio

"rebase --abort" will update HEAD, "rebase --skip" would be a better example, I'll expand the commit message.

Best Wishes

Phillip

Hi Junio

That would be a change for the good, but it is not what this patch does.
If oid == &head_oid then it means that the caller did not give us a tree to checkout so HEAD will not change unless we are switching branches. Using a pointer comparison rather than calling oidcmp() was a deliberate choice to avoid any user visible changes as there are no callers that try to run the checkout-hook without passing an oid or branch. The aim here is to avoid having to pass default_reflog when it is not needed without making any user visible changes.

I'm not sure what we could test

Best Wishes

Phillip

Hi Junio

I'll rephrase to emphasize that it solves the problem of remembering which order to pass things in and makes it clear what is being passed

I found the existing api hard to use because it is easy to lose track of which position each parameter should be in. With the struct the order does not matter and having to set opts.parameter = value makes it clear which parameters are being passed.

Best Wishes

Phillip

By the name of the function, I would have expected that would be the intended behaviour, though. Isn't it emulating "reset --hard" that was used when the calling commands were implemented as scripts?

What I am wondering is if the overwriting behaviour is annoying for some callers but is essential for other callers, and unless we tell readers that we vetted all the callers and everybody wants to keep the paths overwritten from the tree-ish in the working tree that are not in the index, I feel uneasy to add a label "fix" to such a change to a callee used from multiple code paths.

Meaning that this fix is redundant, as the other topic has already been cooking and well in the process of graduating?

Thanks.

Hi René

Thanks for that, I found it really helpful

Best Wishes

Phillip

If all else fails I think it's perfectly OK to just have that failure be triggered by a:

    git_env_bool("GIT_TEST_FAIL_UPDATE_REF", 1) or
    git_env_bool("GIT_TEST_FAIL_UPDATE_REF_FOR_POST_CHECKOUT", 1)

*tests it out a bit*

I tried this and it "worked", as in it'll fail :)

index 97a9501c06f..25e2b0ab49a 100644
@@ -2016,6 +2016,9 @@ int refs_create_symref(struct ref_store *refs,
        char *msg;
        int retval;
 
+       if (git_env_bool("GIT_TEST_FAIL_REFS_CREATE_SYMREF", 0))
+               return -1;
+
        msg = normalize_reflog_message(logmsg);
        retval = refs->be->create_symref(refs, ref_target, refs_heads_master,
                                         msg);

That seems to me to also be a worthwhile thing to do/test for other reasons, i.e. my HEAD won't update, but now my index is in some state where it points to the new branch, but presumably we'd want to revert that in case of the failure (probably a much more general issue than you're poking at here...).

From: Phillip Wood <[email protected]>

This code is heavily indented and it will be convenient later in the series to have it in its own function.

---
 builtin/rebase.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

index 34b4744e5f3..f5c37b7d4a5 100644
@@ -812,6 +812,23 @@ static int rebase_config(const char *var, const char *value, void *data)
 	return git_default_config(var, value, data);
 }
 
+static int checkout_up_to_date(struct rebase_options *options)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int ret = 0;
+
+	strbuf_addf(&buf, "%s: checkout %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+		    options->switch_to);
+	if (reset_head(the_repository, &options->orig_head, "checkout",
+		       options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
+		       NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0)
+		ret = error(_("could not switch to %s"), options->switch_to);
+	strbuf_release(&buf);
+
+	return ret;
+}
+
 /*
  * Determines whether the commits in from..to are linear, i.e. contain
  * no merge commits. This function *expects* `from` to be an ancestor of

@@ -1673,21 +1690,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (!(options.flags & REBASE_FORCE)) {
 			/* Lazily switch to the target branch if needed... */
 			if (options.switch_to) {
-				strbuf_reset(&buf);
-				strbuf_addf(&buf, "%s: checkout %s",
-					    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
-					    options.switch_to);
-				if (reset_head(the_repository,
-					       &options.orig_head, "checkout",
-					       options.head_name,
-					       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
-					       NULL, buf.buf,
-					       DEFAULT_REFLOG_ACTION) < 0) {
-					ret = error(_("could not switch to "
-							"%s"),
-						      options.switch_to);
+				ret = checkout_up_to_date(&options);
+				if (ret)
 					goto cleanup;
-				}
 			}
 
 			if (!(options.flags & REBASE_NO_QUIET))

gitgitgadget

Thanks for the comments on V1. I have tried to improve the commit messages to explain better the motivation and implications of the changes in this series and I have added some more tests. I have rebased onto v2.34.0 to avoid some merges conflicts.

Changes since V1:

* Patch 1 - unchanged.
* Patches 2, 3 - these are new and fix an bug I noticed while adding a test to patch 4.
* Patches 4, 5 - improved commit messages and added tests.
* Patch 6 - reworded commit message.
* Patch 7 - split out some changes that used to be in patch 9.
* Patch 8 - in principle the same but the range-diff is noisy due to the addition of patch 3.
* Patch 9 - reworded commit message.
* Patch 10 - unchanged.
* Patch 11 - reworded commit message and a couple of comments.
* Patch 12 - minor changes to comments.
* Patch 13 - cosmetic changes to commit message and tests.
* Patch 14 - cosmetic changes to commit message.

Cover letter for V1: Fix some issues with the implementation and use of reset_head(). The last patch was previously posted as [1], I have updated the commit message and rebased it onto the fixes in this series. There are a couple of small conflicts merging this into seen, I think they should be easy to resolve (in rebase.c take both sides in reset.c take the changed lines from each side). These patches are based on pw/rebase-of-a-tag-fix

[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/

Phillip Wood (14):
rebase: factor out checkout for up to date branch
t5403: refactor rebase post-checkout hook tests
rebase: pass correct arguments to post-checkout hook
rebase: do not remove untracked files on checkout rebase --apply: don't run post-checkout hook if there is an error reset_head(): remove action parameter create_autostash(): remove unneeded parameter reset_head(): factor out ref updates
reset_head(): make default_reflog_action optional
rebase: cleanup reset_head() calls
reset_head(): take struct rebase_head_opts
rebase --apply: fix reflog
rebase --apply: set ORIG_HEAD correctly
rebase -m: don't fork git checkout

 builtin/merge.c               |   6 +-
 builtin/rebase.c              | 101 +++++++++++++----------
 reset.c                       | 149 ++++++++++++++++++++--------------
 reset.h                       |  48 ++++++++++-
 sequencer.c                   |  47 ++++-------
 sequencer.h                   |   3 +-
 t/t3406-rebase-message.sh     |  23 ++++++
 t/t3418-rebase-continue.sh    |  26 ++++++
 t/t5403-post-checkout-hook.sh |  67 +++++++++++----
 9 files changed, 312 insertions(+), 158 deletions(-)

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1049

Range-diff vs v1:

  1:  4d3441c2b25 =  1:  0e84d00572e rebase: factor out checkout for up to date branch
  -:  ----------- >  2:  a67a5a03b94 t5403: refactor rebase post-checkout hook tests
  -:  ----------- >  3:  07867760e68 rebase: pass correct arguments to post-checkout hook
  2:  c8f64113216 !  4:  2b499704c8f reset_head(): fix checkout

@@ Metadata
Author: Phillip Wood <[email protected]>

## Commit message ##
- reset_head(): fix checkout
+ rebase: do not remove untracked files on checkout

- The reset bit should only be set if flags contains RESET_HEAD_HARD.
- The test for !deatch_head dates back to the original implementation
- of reset_head() in ac7f467fef ("builtin/rebase: support running "git
- rebase <upstream>"", 2018-08-07) and was correct until e65123a71d
+ If "git rebase [--apply|--merge] <upstream> <branch>" detects that
+ <upstream> is an ancestor of <branch> then it will fast-forward and
+ checkout <branch>. Normally a checkout or picking a commit during a
+ rebase will refuse to overwrite untracked files, however rebase does
+ overwrite untracked files when checking <branch>.
+
+ The fix is to only set reset in unpack_tree_opts if flags contains
+ RESET_HEAD_HARD. t5403 may seem like an odd home for the new test
+ but it will be extended in the next commit to check that the
+ post-checkout hook is not run when the checkout fails.
+
+ The test for !deatch_head dates back to the
+ original implementation of reset_head() in
+ ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
+ 2018-08-07) and was correct until e65123a71d
("builtin rebase: support git rebase <upstream> <switch-to>", 2018-09-04) started using reset_head() to checkout <switch-to> when fast-forwarding.

+ Note that 480d3d6bf9 ("Change unpack_trees' 'reset' flag into an
+ enum", 2021-09-27) also fixes this bug as it changes reset_head() to
+ never remove untracked files. I think this fix is still worthwhile as
+ it makes it clear that the same settings are used for detached and
+ non-detached checkouts.
+

## reset.c ##
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char *action,
- unpack_tree_opts.update = 1;
unpack_tree_opts.merge = 1;
+ unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
- if (!detach_head)
+ if (reset_hard)
- unpack_tree_opts.reset = 1;
+ unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;

if (repo_read_index_unmerged(r) < 0) {
+
+ ## t/t5403-post-checkout-hook.sh ##
+@@ t/t5403-post-checkout-hook.sh: test_rebase () {
+ test_cmp_rev three $new &&
+ test $flag = 1
+ '
++
++ test_expect_success "rebase $args checkout does not remove untracked files" '
++ test_when_finished "test_might_fail git rebase --abort" &&
++ git update-ref refs/heads/rebase-fast-forward three &&
++ git checkout two &&
++ echo untracked >three.t &&
++ test_when_finished "rm three.t" &&
++ test_must_fail git rebase $args HEAD rebase-fast-forward 2>err &&
++ grep "untracked working tree files would be overwritten by checkout" err
++'
+ }
+
+ test_rebase --apply &&
3: 28872cbca68 ! 5: 04e7340a7e7 reset_head(): don't run checkout hook if there is an error
@@ Metadata
Author: Phillip Wood <[email protected]>

## Commit message ##
- reset_head(): don't run checkout hook if there is an error
+ rebase --apply: don't run post-checkout hook if there is an error

The hook should only be run if the worktree and refs were successfully
- updated.
+ updated. This primarily affects "rebase --apply" but also "rebase
+ --merge" when it fast-forwards.

@@ reset.c: reset_head_refs:
- if (run_hook)
+ if (!ret && run_hook)
run_hook_le(NULL, "post-checkout",
- oid_to_hex(orig ? orig : null_oid()),
+ oid_to_hex(head ? head : null_oid()),
oid_to_hex(oid), "1", NULL);
+
+ ## t/t5403-post-checkout-hook.sh ##
+@@ t/t5403-post-checkout-hook.sh: test_rebase () {
+
+ test_expect_success "rebase $args checkout does not remove untracked files" '
+ test_when_finished "test_might_fail git rebase --abort" &&
++ test_when_finished "rm -f .git/post-checkout.args" &&
+ git update-ref refs/heads/rebase-fast-forward three &&
+ git checkout two &&
++ rm -f .git/post-checkout.args &&
+ echo untracked >three.t &&
+ test_when_finished "rm three.t" &&
+ test_must_fail git rebase $args HEAD rebase-fast-forward 2>err &&
+- grep "untracked working tree files would be overwritten by checkout" err
++ grep "untracked working tree files would be overwritten by checkout" err &&
++ test_path_is_missing .git/post-checkout.args
++
+ '
+ }
+
4: fbaf64d6b28 ! 6: 32ffa98c1bc reset_head(): remove action parameter
@@ Metadata
## Commit message ##
reset_head(): remove action parameter

- The action parameter is passed as the command name to
- setup_unpack_trees_porcelain(). All but two cases pass either
- "checkout" or "reset". The case that passes "reset --hard" should be
- passing "reset" instead. The case that passes "Fast-forwarded" is only
- updating HEAD and so does not call unpack_trees(). The value can be
- determined by checking whether flags contains RESET_HEAD_HARD so it
- does not need to be specified by the caller.
+ The only use of the action parameter is to setup the error messages
+ for unpack_trees(). All but two cases pass either "checkout" or
+ "reset". The case that passes "reset --hard" would be better passing
+ "reset" so that the error messages match the builtin reset command
+ like all the other callers that are doing a reset. The case that
+ passes "Fast-forwarded" is only updating HEAD and so the parameter is
+ unused in that case as it does not call unpack_trees(). The value to
+ pass to setup_unpack_trees_porcelain() can be determined by checking
+ whether flags contains RESET_HEAD_HARD without the caller having to
+ specify it.

@@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char
+ const char *action, *reflog_action;
struct strbuf msg = STRBUF_INIT;
size_t prefix_len;
- struct object_id *orig = NULL, oid_orig,
+ struct object_id *old_orig = NULL, oid_old_orig;
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char *action,
if (refs_only)
goto reset_head_refs;

  -:  ----------- >  7:  341fe183c18 create_autostash(): remove unneeded parameter
  5:  0744c3d143b !  8:  29e06e7d36d reset_head(): factor out ref updates
     @@ Metadata
       ## Commit message ##
          reset_head(): factor out ref updates
      
     -    In the next commit we will stop trying to update HEAD when we are just
     -    clearing changes from the working tree. Move the code that updates the
     -    refs to its own function in preparation for that.
     +    In the next commit we will stop trying to update HEAD when we are
     +    removing uncommitted changes from the working tree. Move the code that
     +    updates the refs to its own function in preparation for that.
      
          Signed-off-by: Phillip Wood <[email protected]>
      
     @@ reset.c
       #include "unpack-trees.h"
       
      +static int update_refs(const struct object_id *oid, const char *switch_to_branch,
     -+		       const char *reflog_head, const char *reflog_orig_head,
     ++		       const struct object_id *head, const char *reflog_head,
     ++		       const char *reflog_orig_head,
      +		       const char *default_reflog_action, unsigned flags)
      +{
      +	unsigned detach_head = flags & RESET_HEAD_DETACH;
      +	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
      +	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
     -+	struct object_id *orig = NULL, oid_orig, *old_orig = NULL, oid_old_orig;
     ++	struct object_id *old_orig = NULL, oid_old_orig;
      +	struct strbuf msg = STRBUF_INIT;
      +	const char *reflog_action;
      +	size_t prefix_len;
     @@ reset.c
      +	if (update_orig_head) {
      +		if (!get_oid("ORIG_HEAD", &oid_old_orig))
      +			old_orig = &oid_old_orig;
     -+		if (!get_oid("HEAD", &oid_orig)) {
     -+			orig = &oid_orig;
     ++		if (head) {
      +			if (!reflog_orig_head) {
      +				strbuf_addstr(&msg, "updating ORIG_HEAD");
      +				reflog_orig_head = msg.buf;
      +			}
     -+			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
     ++			update_ref(reflog_orig_head, "ORIG_HEAD", head,
      +				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
      +		} else if (old_orig)
      +			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
     @@ reset.c
      +		reflog_head = msg.buf;
      +	}
      +	if (!switch_to_branch)
     -+		ret = update_ref(reflog_head, "HEAD", oid, orig,
     ++		ret = update_ref(reflog_head, "HEAD", oid, head,
      +				 detach_head ? REF_NO_DEREF : 0,
      +				 UPDATE_REFS_MSG_ON_ERR);
      +	else {
     @@ reset.c
      +	}
      +	if (!ret && run_hook)
      +		run_hook_le(NULL, "post-checkout",
     -+			    oid_to_hex(orig ? orig : null_oid()),
     ++			    oid_to_hex(head ? head : null_oid()),
      +			    oid_to_hex(oid), "1", NULL);
      +	strbuf_release(&msg);
      +	return ret;
     @@ reset.c
      -	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
       	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
      -	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
     - 	struct object_id head_oid;
     + 	struct object_id *head = NULL, head_oid;
       	struct tree_desc desc[2] = { { NULL }, { NULL } };
       	struct lock_file lock = LOCK_INIT;
       	struct unpack_trees_options unpack_tree_opts = { 0 };
     @@ reset.c
      -	const char *action, *reflog_action;
      -	struct strbuf msg = STRBUF_INIT;
      -	size_t prefix_len;
     --	struct object_id *orig = NULL, oid_orig,
     --		*old_orig = NULL, oid_old_orig;
     +-	struct object_id *old_orig = NULL, oid_old_orig;
      +	const char *action;
       	int ret = 0, nr = 0;
       
     @@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
       
       	if (refs_only)
      -		goto reset_head_refs;
     -+		return update_refs(oid, switch_to_branch, reflog_head,
     ++		return update_refs(oid, switch_to_branch, head, reflog_head,
      +				   reflog_orig_head, default_reflog_action,
      +				   flags);
       
     @@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
      -	if (update_orig_head) {
      -		if (!get_oid("ORIG_HEAD", &oid_old_orig))
      -			old_orig = &oid_old_orig;
     --		if (!get_oid("HEAD", &oid_orig)) {
     --			orig = &oid_orig;
     +-		if (head) {
      -			if (!reflog_orig_head) {
      -				strbuf_addstr(&msg, "updating ORIG_HEAD");
      -				reflog_orig_head = msg.buf;
      -			}
     --			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
     +-			update_ref(reflog_orig_head, "ORIG_HEAD", head,
      -				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
      -		} else if (old_orig)
      -			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
     @@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
      -		reflog_head = msg.buf;
      -	}
      -	if (!switch_to_branch)
     --		ret = update_ref(reflog_head, "HEAD", oid, orig,
     +-		ret = update_ref(reflog_head, "HEAD", oid, head,
      -				 detach_head ? REF_NO_DEREF : 0,
      -				 UPDATE_REFS_MSG_ON_ERR);
      -	else {
     @@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
      -	}
      -	if (!ret && run_hook)
      -		run_hook_le(NULL, "post-checkout",
     --			    oid_to_hex(orig ? orig : null_oid()),
     +-			    oid_to_hex(head ? head : null_oid()),
      -			    oid_to_hex(oid), "1", NULL);
     -+	ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head,
     -+			  default_reflog_action, flags);
     ++	ret = update_refs(oid, switch_to_branch, head, reflog_head,
     ++			  reflog_orig_head, default_reflog_action, flags);
       
       leave_reset_head:
      -	strbuf_release(&msg);

6: 4503defe591 ! 9: 9d00a218daf reset_head(): make default_reflog_action optional
@@ Commit message

This parameter is only needed when a ref is going to be updated and the caller does not pass an explicit reflog message. Callers that are
- just discarding changes in the working tree like create_autostash() do
- not update any refs so should not have to worry about passing this
- parameter.
+ only discarding uncommitted changes in the working tree such as such
+ as "rebase --skip" or create_autostash() do not update any refs so
+ should not have to worry about passing this parameter.

- Signed-off-by: Phillip Wood <[email protected]>
+ This change is not intended to have any user visible changes. The
+ pointer comparison between oid and &head_oid checks that the
+ caller did not pass an oid to be checked out. As no callers pass
+ RESET_HEAD_RUN_POST_CHECKOUT_HOOK without passing an oid there are
+ no changes to when the post-checkout hook is run. As update_ref() only
+ updates the ref if the oid passed to it differs from the current ref
+ there are no changes to when HEAD is updated.

- ## builtin/merge.c ##
-@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix) -
- if (autostash)
- create_autostash(the_repository,
-- git_path_merge_autostash(the_repository),
-- "merge");
-+ git_path_merge_autostash(the_repository));
- if (checkout_fast_forward(the_repository,
- &head_commit->object.oid,
- &commit->object.oid,
-@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix) -
- if (autostash)
- create_autostash(the_repository,
-- git_path_merge_autostash(the_repository),
-- "merge");
-+ git_path_merge_autostash(the_repository));
-
- /* We are going to make a new commit. */
- git_committer_info(IDENT_STRICT);
+ Signed-off-by: Phillip Wood <[email protected]>

## builtin/rebase.c ##
@@ builtin/rebase.c: static int move_to_original_branch(struct rebase_options *opts)
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
if (read_basic_state(&options))
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- die(_("could not read index"));
-
- if (options.autostash) {
-- create_autostash(the_repository, state_dir_path("autostash", &options),
-- DEFAULT_REFLOG_ACTION);
-+ create_autostash(the_repository,
-+ state_dir_path("autostash", &options));
- }
-
- if (require_clean_work_tree(the_repository, "rebase",
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
options.head_name ? options.head_name : "detached HEAD", oid_to_hex(&options.onto->object.oid));
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
unsigned reset_hard = flags & RESET_HEAD_HARD;
unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
+ unsigned update_orig_head = flags & RESET_ORIG_HEAD;
- struct object_id head_oid;
+ struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
goto leave_reset_head;
}

-- ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head,
-- default_reflog_action, flags);
+- ret = update_refs(oid, switch_to_branch, head, reflog_head,
+- reflog_orig_head, default_reflog_action, flags);
+ if (oid != &head_oid || update_orig_head || switch_to_branch)
-+ ret = update_refs(oid, switch_to_branch, reflog_head,
++ ret = update_refs(oid, switch_to_branch, head, reflog_head,
+ reflog_orig_head, default_reflog_action,
+ flags);

@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
rollback_lock_file(&lock);

## sequencer.c ##
-@@ sequencer.c: static enum todo_command peek_command(struct todo_list *todo_list, int offset)
- return -1;
- }
-
--void create_autostash(struct repository *r, const char *path,
-- const char *default_reflog_action)
-+void create_autostash(struct repository *r, const char *path)
-+
- {
- struct strbuf buf = STRBUF_INIT;
- struct lock_file lock_file = LOCK_INIT;
-@@ sequencer.c: void create_autostash(struct repository *r, const char *path, +@@ sequencer.c: void create_autostash(struct repository *r, const char *path)
write_file(path, "%s", oid_to_hex(&oid));
printf(_("Created autostash: %s\n"), buf.buf);
if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
-- default_reflog_action) < 0)
+- "") < 0)
+ NULL) < 0)
die(_("could not reset --hard"));

if (discard_index(r->index) < 0 ||
-
- ## sequencer.h ##
-@@ sequencer.h: void commit_post_rewrite(struct repository *r,
- const struct commit *current_head,
- const struct object_id *new_head);
-
--void create_autostash(struct repository *r, const char *path,
-- const char *default_reflog_action);
-+void create_autostash(struct repository *r, const char *path);
- int save_autostash(const char *path);
- int apply_autostash(const char *path);
- int apply_autostash_oid(const char *stash_oid);

  7:  5ffc7e64ff1 = 10:  5ea636009e7 rebase: cleanup reset_head() calls
  8:  267e074e6db ! 11:  24b0566aba5 reset_head(): take struct rebase_head_opts

@@ Metadata
## Commit message ##
reset_head(): take struct rebase_head_opts

- This function already takes a confusingly large number of parameters
- some of which are optional or not always required. The following
- commits will add a couple more parameters so change it to take a
- struct of options first.
+ This function takes a confusingly large number of parameters which
+ makes it difficult to remember which order to pass them in. The
+ following commits will add a couple more parameters which makes the
+ problem worse. To address this change the function to take a struct of
+ options. Using a struct means that it is no longer necessary to
+ remember which order to pass the parameters in and anyone reading the
+ code can easily see which value is passed to each parameter.

## builtin/rebase.c ##
-@@ builtin/rebase.c: static void add_var(struct strbuf *buf, const char *name, const char *value) +@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)
static int move_to_original_branch(struct rebase_options *opts)
{
struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
strbuf_release(&buf);

@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- char *squash_onto_name = NULL;
int reschedule_failed_exec = -1;
int allow_preemptive_ff = 1;
+ int preserve_merges_selected = 0;
+ struct reset_head_opts ropts = { 0 };
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
@@ reset.c
#include "unpack-trees.h"

-static int update_refs(const struct object_id *oid, const char *switch_to_branch,
-- const char *reflog_head, const char *reflog_orig_head,
+- const struct object_id *head, const char *reflog_head,
+- const char *reflog_orig_head,
- const char *default_reflog_action, unsigned flags)
+static int update_refs(const struct reset_head_opts *opts,
-+ const struct object_id *oid)
++ const struct object_id *oid,
++ const struct object_id *head)
{
- unsigned detach_head = flags & RESET_HEAD_DETACH;
- unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
@@ reset.c
+ const char *reflog_head = opts->head_msg;
+ const char *reflog_orig_head = opts->orig_head_msg;
+ const char *default_reflog_action = opts->default_reflog_action;
- struct object_id *orig = NULL, oid_orig, *old_orig = NULL, oid_old_orig;
+ struct object_id *old_orig = NULL, oid_old_orig;
struct strbuf msg = STRBUF_INIT;
const char *reflog_action;
@@ reset.c: static int update_refs(const struct object_id *oid, const char *switch_to_branch
@@ reset.c: static int update_refs(const struct object_id *oid, const char *switch_
+ unsigned reset_hard = opts->flags & RESET_HEAD_HARD;
+ unsigned refs_only = opts->flags & RESET_HEAD_REFS_ONLY;
+ unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
- struct object_id head_oid;
+ struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
oid = &head_oid;

if (refs_only)
-- return update_refs(oid, switch_to_branch, reflog_head,
+- return update_refs(oid, switch_to_branch, head, reflog_head,
- reflog_orig_head, default_reflog_action,
- flags);
-+ return update_refs(opts, oid);
++ return update_refs(opts, oid, head);

action = reset_hard ? "reset" : "checkout"; setup_unpack_trees_porcelain(&unpack_tree_opts, action);
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
}

if (oid != &head_oid || update_orig_head || switch_to_branch)
-- ret = update_refs(oid, switch_to_branch, reflog_head,
+- ret = update_refs(oid, switch_to_branch, head, reflog_head,
- reflog_orig_head, default_reflog_action,
- flags);
-+ ret = update_refs(opts, oid);
++ ret = update_refs(opts, oid, head);

leave_reset_head:
rollback_lock_file(&lock);

## reset.h ##
@@
+
+ #define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION" +
++/* Request a detached checkout */
+ #define RESET_HEAD_DETACH (1<<0)
++/* Request a reset rather than a checkout */
+ #define RESET_HEAD_HARD (1<<1)
++/* Run the post-checkout hook */
+ #define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2) ++/* Only update refs, do not touch the worktree */ #define RESET_HEAD_REFS_ONLY (1<<3)
++/* Update ORIG_HEAD as well as HEAD */
#define RESET_ORIG_HEAD (1<<4)

-int reset_head(struct repository *r, struct object_id *oid,
@@ reset.h
- const char *reflog_orig_head, const char *reflog_head,
- const char *default_reflog_action);
+struct reset_head_opts {
-+ /* The oid of the commit to checkout/reset to. Defaults to HEAD */
++ /*
++ * The commit to checkout/reset to. Defaults to HEAD.
++ */
+ const struct object_id *oid;
-+ /* Optional branch to switch to */
++ /*
++ * Optional branch to switch to.
++ */
+ const char *branch;
-+ /* Flags defined above */
++ /*
++ * Flags defined above.
++ */
+ unsigned flags;
-+ /*
-+ * Optional reflog message for HEAD, if this is not set then
-+ * default_reflog_action must be.
-+ */
++ /*
++ * Optional reflog message for HEAD, if this omitted but oid or branch
++ * are given then default_reflog_action must be given.
++ */
+ const char *head_msg;
+ /*
-+ * Optional reflog message for ORIG_HEAD, if this is not set and flags
-+ * contains RESET_ORIG_HEAD then default_reflog_action must be set.
++ * Optional reflog message for ORIG_HEAD, if this omitted and flags
++ * contains RESET_ORIG_HEAD then default_reflog_action must be given.
+ */
+ const char *orig_head_msg;
+ /*
9: cdb0de221d5 ! 12: dc5d11291e7 rebase --apply: fix reflog
@@ Commit message

## builtin/rebase.c ##
-@@ builtin/rebase.c: static void add_var(struct strbuf *buf, const char *name, const char *value) +@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)

static int move_to_original_branch(struct rebase_options *opts)
{
@@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts

## reset.h ##
@@ reset.h: struct reset_head_opts {
- const char *branch;
- /* Flags defined above */
+ * Flags defined above.
+ */
unsigned flags;
-+ /* Optional reflog message for branch, defaults to head_msg. */
++ /*
++ * Optional reflog message for branch, defaults to head_msg.
++ */
+ const char *branch_msg;
- /*
- * Optional reflog message for HEAD, if this is not set then
- * default_reflog_action must be.
+ /*
+ * Optional reflog message for HEAD, if this omitted but oid or branch
+ * are given then default_reflog_action must be given.

## t/t3406-rebase-message.sh ##
@@ t/t3406-rebase-message.sh: test_expect_success 'GIT_REFLOG_ACTION' '
10: e8884efcc83 ! 13: 45a5b5e9818 rebase --apply: set ORIG_HEAD correctly

     @@ Commit message
          At the start of a rebase ORIG_HEAD is updated to tip of the branch
          being rebased. Unfortunately reset_head() always uses the current
          value of HEAD for this which is incorrect if the rebase is started
     -    with 'git rebase <upstream> <branch>' as in that case ORIG_HEAD should
     +    with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should
          be updated to <branch>. This only affects the "apply" backend as the
          "merge" backend does not yet use reset_head() for the initial
          checkout. Fix this by passing in orig_head when calling reset_head()
     @@ reset.c: static int update_refs(const struct reset_head_opts *opts,
       				strbuf_addstr(&msg, "updating ORIG_HEAD");
       				reflog_orig_head = msg.buf;
       			}
     --			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
     +-			update_ref(reflog_orig_head, "ORIG_HEAD", head,
      +			update_ref(reflog_orig_head, "ORIG_HEAD",
     -+				   orig_head ? orig_head : orig,
     ++				   orig_head ? orig_head : head,
       				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
       		} else if (old_orig)
       			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
      
       ## reset.h ##
     -@@
     - struct reset_head_opts {
     - 	/* The oid of the commit to checkout/reset to. Defaults to HEAD */
     +@@ reset.h: struct reset_head_opts {
     + 	 * The commit to checkout/reset to. Defaults to HEAD.
     + 	 */
       	const struct object_id *oid;
     -+	/* Optional commit when setting ORIG_HEAD. Defaults to HEAD */
     ++	/*
     ++	 * Optional value to set ORIG_HEAD. Defaults to HEAD.
     ++	 */
      +	const struct object_id *orig_head;
     - 	 /* Optional branch to switch to */
     - 	const char *branch;
     - 	/* Flags defined above */
     + 	/*
     + 	 * Optional branch to switch to.
     + 	 */
      
       ## t/t3418-rebase-continue.sh ##
      @@ t/t3418-rebase-continue.sh: test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
       	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
       '
       
     -+test_orig_head_helper() {
     ++test_orig_head_helper () {
      +	test_when_finished 'git rebase --abort &&
      +		git checkout topic &&
      +		git reset --hard commit-new-file-F2-on-topic-branch' &&
     @@ t/t3418-rebase-continue.sh: test_expect_success 'there is no --no-reschedule-fai
      +	test_cmp_rev ORIG_HEAD commit-new-file-F2-on-topic-branch
      +}
      +
     -+test_orig_head() {
     ++test_orig_head () {
      +	type=$1
      +	test_expect_success "rebase $type sets ORIG_HEAD correctly" '
      +		git checkout topic &&

11: 2c8c60c3f31 ! 14: 3f64b9274b5 rebase -m: don't fork git checkout
@@ Commit message
rebase -m: don't fork git checkout

Now that reset_head() can handle the initial checkout of onto
- correctly use it in the "merge" backend instead of forking 'git
- checkout'. This opens the way for us to stop calling the
- post-checkout hook in the future. Not running 'git checkout' means
- that 'rebase -i/m' no longer recurse submodules when checking out
- 'onto' (thanks to Philippe Blain for pointing this out). As the rest
+ correctly use it in the "merge" backend instead of forking "git
+ checkout". This opens the way for us to stop calling the
+ post-checkout hook in the future. Not running "git checkout" means
+ that "rebase -i/m" no longer recurse submodules when checking out
+ "onto" (thanks to Philippe Blain for pointing this out). As the rest
of rebase does not know what to do with submodules this is probably a good thing. When using merge-ort rebase ought be able to handle submodules correctly if it parsed the submodule config, such a change

gitgitgadget

From: Phillip Wood <[email protected]>

These tests only test the default backend and do not check that the arguments passed to the hook are correct. Fix this by running the tests with both backends and adding checks for the hook arguments.

---
 t/t5403-post-checkout-hook.sh | 42 ++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 16 deletions(-)

index 1ec9e23be75..272b02687ba 100755
@@ -49,23 +49,33 @@ test_expect_success 'post-checkout receives the right args when not switching br
 	test $old = $new && test $flag = 0
 '
 
-test_expect_success 'post-checkout is triggered on rebase' '
-	test_when_finished "rm -f .git/post-checkout.args" &&
-	git checkout -b rebase-test main &&
-	rm -f .git/post-checkout.args &&
-	git rebase rebase-on-me &&
-	read old new flag <.git/post-checkout.args &&
-	test $old != $new && test $flag = 1
-'
+test_rebase () {
+	args="$*" &&
+	test_expect_success "post-checkout is triggered on rebase $args" '
+		test_when_finished "rm -f .git/post-checkout.args" &&
+		git checkout -B rebase-test main &&
+		rm -f .git/post-checkout.args &&
+		git rebase $args rebase-on-me &&
+		read old new flag <.git/post-checkout.args &&
+		test_cmp_rev main $old &&
+		test_cmp_rev rebase-on-me $new &&
+		test $flag = 1
+	'
 
-test_expect_success 'post-checkout is triggered on rebase with fast-forward' '
-	test_when_finished "rm -f .git/post-checkout.args" &&
-	git checkout -b ff-rebase-test rebase-on-me^ &&
-	rm -f .git/post-checkout.args &&
-	git rebase rebase-on-me &&
-	read old new flag <.git/post-checkout.args &&
-	test $old != $new && test $flag = 1
-'
+	test_expect_success "post-checkout is triggered on rebase $args with fast-forward" '
+		test_when_finished "rm -f .git/post-checkout.args" &&
+		git checkout -B ff-rebase-test rebase-on-me^ &&
+		rm -f .git/post-checkout.args &&
+		git rebase $args rebase-on-me &&
+		read old new flag <.git/post-checkout.args &&
+		test_cmp_rev rebase-on-me^ $old &&
+		test_cmp_rev rebase-on-me $new &&
+		test $flag = 1
+	'
+}
+
+test_rebase --apply &&
+test_rebase --merge
 
 test_expect_success 'post-checkout hook is triggered by clone' '
 	mkdir -p templates/hooks &&

gitgitgadget

From: Phillip Wood <[email protected]>

If a rebase started with "rebase [--apply|--merge] <upstream> <branch>" detects that <upstream> is an ancestor of <branch> then it fast-forwards and checks out <branch>. Unfortunately in that case it passed the null oid as the first argument to the post-checkout hook rather than the oid of HEAD.

A side effect of this change is that the call to update_ref() which updates HEAD now always receives the old value of HEAD. This provides protection against another process updating HEAD during the checkout.

---
 reset.c                       | 18 +++++++++---------
 t/t5403-post-checkout-hook.sh | 13 +++++++++++++
 2 files changed, 22 insertions(+), 9 deletions(-)

index f214df3d96c..315fef91d33 100644
@@ -18,7 +18,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
 	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
-	struct object_id head_oid;
+	struct object_id *head = NULL, head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;
 	struct unpack_trees_options unpack_tree_opts = { 0 };

@@ -26,8 +26,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	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;
+	struct object_id *old_orig = NULL, oid_old_orig;
 	int ret = 0, nr = 0;
 
 	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))

@@ -38,7 +37,9 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 		goto leave_reset_head;
 	}
 
-	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
+	if (!get_oid("HEAD", &head_oid)) {
+		head = &head_oid;
+	} else if (!oid || !reset_hard) {
 		ret = error(_("could not determine HEAD revision"));
 		goto leave_reset_head;
 	}

@@ -98,13 +99,12 @@ reset_head_refs:
 	if (update_orig_head) {
 		if (!get_oid("ORIG_HEAD", &oid_old_orig))
 			old_orig = &oid_old_orig;
-		if (!get_oid("HEAD", &oid_orig)) {
-			orig = &oid_orig;
+		if (head) {
 			if (!reflog_orig_head) {
 				strbuf_addstr(&msg, "updating ORIG_HEAD");
 				reflog_orig_head = msg.buf;
 			}
-			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
+			update_ref(reflog_orig_head, "ORIG_HEAD", head,
 				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
 		} else if (old_orig)
 			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);

@@ -116,7 +116,7 @@ reset_head_refs:
 		reflog_head = msg.buf;
 	}
 	if (!switch_to_branch)
-		ret = update_ref(reflog_head, "HEAD", oid, orig,
+		ret = update_ref(reflog_head, "HEAD", oid, head,
 				 detach_head ? REF_NO_DEREF : 0,
 				 UPDATE_REFS_MSG_ON_ERR);
 	else {

@@ -128,7 +128,7 @@ reset_head_refs:
 	}
 	if (run_hook)
 		run_hook_le(NULL, "post-checkout",
-			    oid_to_hex(orig ? orig : null_oid()),
+			    oid_to_hex(head ? head : null_oid()),
 			    oid_to_hex(oid), "1", NULL);
 
 leave_reset_head:

index 272b02687ba..17ab518f268 100755
@@ -72,6 +72,19 @@ test_rebase () {
 		test_cmp_rev rebase-on-me $new &&
 		test $flag = 1
 	'
+
+	test_expect_success "rebase $args fast-forward branch checkout runs post-checkout hook" '
+		test_when_finished "test_might_fail git rebase --abort" &&
+		test_when_finished "rm -f .git/post-checkout.args" &&
+		git update-ref refs/heads/rebase-fast-forward three &&
+		git checkout two  &&
+		rm -f .git/post-checkout.args &&
+		git rebase $args HEAD rebase-fast-forward  &&
+		read old new flag <.git/post-checkout.args &&
+		test_cmp_rev two $old &&
+		test_cmp_rev three $new &&
+		test $flag = 1
+	'
 }
 
 test_rebase --apply &&

gitgitgadget

From: Phillip Wood <[email protected]>

If "git rebase [--apply|--merge] <upstream> <branch>" detects that <upstream> is an ancestor of <branch> then it will fast-forward and checkout <branch>. Normally a checkout or picking a commit during a rebase will refuse to overwrite untracked files, however rebase does overwrite untracked files when checking <branch>.

The fix is to only set reset in unpack_tree_opts if flags contains RESET_HEAD_HARD. t5403 may seem like an odd home for the new test but it will be extended in the next commit to check that the post-checkout hook is not run when the checkout fails.

The test for !deatch_head dates back to the
original implementation of reset_head() in
ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"", 2018-08-07) and was correct until e65123a71d
("builtin rebase: support git rebase <upstream> <switch-to>", 2018-09-04) started using reset_head() to checkout <switch-to> when fast-forwarding.

Note that 480d3d6bf9 ("Change unpack_trees' 'reset' flag into an enum", 2021-09-27) also fixes this bug as it changes reset_head() to never remove untracked files. I think this fix is still worthwhile as it makes it clear that the same settings are used for detached and non-detached checkouts.

---
 reset.c                       |  2 +-
 t/t5403-post-checkout-hook.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

index 315fef91d33..3e7b9e2e131 100644
@@ -59,7 +59,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	unpack_tree_opts.merge = 1;
 	unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
-	if (!detach_head)
+	if (reset_hard)
 		unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
 
 	if (repo_read_index_unmerged(r) < 0) {

index 17ab518f268..fd2817b4068 100755
@@ -85,6 +85,16 @@ test_rebase () {
 		test_cmp_rev three $new &&
 		test $flag = 1
 	'
+
+	test_expect_success "rebase $args checkout does not remove untracked files" '
+		test_when_finished "test_might_fail git rebase --abort" &&
+		git update-ref refs/heads/rebase-fast-forward three &&
+		git checkout two &&
+		echo untracked >three.t &&
+		test_when_finished "rm three.t" &&
+		test_must_fail git rebase $args HEAD rebase-fast-forward 2>err &&
+		grep "untracked working tree files would be overwritten by checkout" err
+'
 }
 
 test_rebase --apply &&

gitgitgadget

From: Phillip Wood <[email protected]>

The hook should only be run if the worktree and refs were successfully updated. This primarily affects "rebase --apply" but also "rebase --merge" when it fast-forwards.

---
 reset.c                       | 2 +-
 t/t5403-post-checkout-hook.sh | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

index 3e7b9e2e131..3537de91f65 100644
@@ -126,7 +126,7 @@ reset_head_refs:
 			ret = create_symref("HEAD", switch_to_branch,
 					    reflog_head);
 	}
-	if (run_hook)
+	if (!ret && run_hook)
 		run_hook_le(NULL, "post-checkout",
 			    oid_to_hex(head ? head : null_oid()),
 			    oid_to_hex(oid), "1", NULL);

index fd2817b4068..d1181816906 100755
@@ -88,12 +88,16 @@ test_rebase () {
 
 	test_expect_success "rebase $args checkout does not remove untracked files" '
 		test_when_finished "test_might_fail git rebase --abort" &&
+		test_when_finished "rm -f .git/post-checkout.args" &&
 		git update-ref refs/heads/rebase-fast-forward three &&
 		git checkout two &&
+		rm -f .git/post-checkout.args &&
 		echo untracked >three.t &&
 		test_when_finished "rm three.t" &&
 		test_must_fail git rebase $args HEAD rebase-fast-forward 2>err &&
-		grep "untracked working tree files would be overwritten by checkout" err
+		grep "untracked working tree files would be overwritten by checkout" err &&
+		test_path_is_missing .git/post-checkout.args
+
 '
 }
 

gitgitgadget

From: Phillip Wood <[email protected]>

The only use of the action parameter is to setup the error messages for unpack_trees(). All but two cases pass either "checkout" or "reset". The case that passes "reset --hard" would be better passing "reset" so that the error messages match the builtin reset command like all the other callers that are doing a reset. The case that passes "Fast-forwarded" is only updating HEAD and so the parameter is unused in that case as it does not call unpack_trees(). The value to pass to setup_unpack_trees_porcelain() can be determined by checking whether flags contains RESET_HEAD_HARD without the caller having to specify it.

---
 builtin/rebase.c | 14 +++++++-------
 reset.c          |  5 +++--
 reset.h          |  2 +-
 sequencer.c      |  3 +--
 4 files changed, 12 insertions(+), 12 deletions(-)

index f5c37b7d4a5..2e5a535b54e 100644
@@ -583,7 +583,7 @@ static int move_to_original_branch(struct rebase_options *opts)
 		    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(the_repository, NULL, "", opts->head_name,
+	ret = reset_head(the_repository, NULL, opts->head_name,
 			 RESET_HEAD_REFS_ONLY,
 			 orig_head_reflog.buf, head_reflog.buf,
 			 DEFAULT_REFLOG_ACTION);

@@ -674,7 +674,7 @@ static int run_am(struct rebase_options *opts)
 		free(rebased_patches);
 		strvec_clear(&am.args);
 
-		reset_head(the_repository, &opts->orig_head, "checkout",
+		reset_head(the_repository, &opts->orig_head,
 			   opts->head_name, 0,
 			   "HEAD", NULL, DEFAULT_REFLOG_ACTION);
 		error(_("\ngit encountered an error while preparing the "

@@ -820,7 +820,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);
-	if (reset_head(the_repository, &options->orig_head, "checkout",
+	if (reset_head(the_repository, &options->orig_head,
 		       options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 		       NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0)
 		ret = error(_("could not switch to %s"), options->switch_to);

@@ -1272,7 +1272,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
-		if (reset_head(the_repository, NULL, "reset", NULL, RESET_HEAD_HARD,
+		if (reset_head(the_repository, NULL, NULL, RESET_HEAD_HARD,
 			       NULL, NULL, DEFAULT_REFLOG_ACTION) < 0)
 			die(_("could not discard worktree changes"));
 		remove_branch_state(the_repository, 0);

@@ -1290,7 +1290,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (read_basic_state(&options))
 			exit(1);
-		if (reset_head(the_repository, &options.orig_head, "reset",
+		if (reset_head(the_repository, &options.orig_head,
 			       options.head_name, RESET_HEAD_HARD,
 			       NULL, NULL, DEFAULT_REFLOG_ACTION) < 0)
 			die(_("could not move back to %s"),

@@ -1759,7 +1759,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
-	if (reset_head(the_repository, &options.onto->object.oid, "checkout", NULL,
+	if (reset_head(the_repository, &options.onto->object.oid, NULL,
 		       RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 		       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 		       NULL, msg.buf, DEFAULT_REFLOG_ACTION))

@@ -1777,7 +1777,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
 			options.head_name ? options.head_name : "detached HEAD",
 			oid_to_hex(&options.onto->object.oid));
-		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
+		reset_head(the_repository, NULL, options.head_name,
 			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
 			   DEFAULT_REFLOG_ACTION);
 		strbuf_release(&msg);

index 3537de91f65..7841b2b2a02 100644
@@ -8,7 +8,7 @@
 #include "tree.h"
 #include "unpack-trees.h"
 
-int reset_head(struct repository *r, struct object_id *oid, const char *action,
+int reset_head(struct repository *r, struct object_id *oid,
 	       const char *switch_to_branch, unsigned flags,
 	       const char *reflog_orig_head, const char *reflog_head,
 	       const char *default_reflog_action)

@@ -23,7 +23,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	struct lock_file lock = LOCK_INIT;
 	struct unpack_trees_options unpack_tree_opts = { 0 };
 	struct tree *tree;
-	const char *reflog_action;
+	const char *action, *reflog_action;
 	struct strbuf msg = STRBUF_INIT;
 	size_t prefix_len;
 	struct object_id *old_orig = NULL, oid_old_orig;

@@ -50,6 +50,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	if (refs_only)
 		goto reset_head_refs;
 
+	action = reset_hard ? "reset" : "checkout";
 	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
 	unpack_tree_opts.head_idx = 1;
 	unpack_tree_opts.src_index = r->index;

index 12f83c78e28..2daec804259 100644
@@ -12,7 +12,7 @@
 #define RESET_HEAD_REFS_ONLY (1<<3)
 #define RESET_ORIG_HEAD (1<<4)
 
-int reset_head(struct repository *r, struct object_id *oid, const char *action,
+int reset_head(struct repository *r, struct object_id *oid,
 	       const char *switch_to_branch, unsigned flags,
 	       const char *reflog_orig_head, const char *reflog_head,
 	       const char *default_reflog_action);

index ea96837cde3..3d3c3fbe305 100644
@@ -4131,8 +4131,7 @@ void create_autostash(struct repository *r, const char *path,
 			    path);
 		write_file(path, "%s", oid_to_hex(&oid));
 		printf(_("Created autostash: %s\n"), buf.buf);
-		if (reset_head(r, NULL, "reset --hard",
-			       NULL, RESET_HEAD_HARD, NULL, NULL,
+		if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
 			       default_reflog_action) < 0)
 			die(_("could not reset --hard"));
 

gitgitgadget

From: Phillip Wood <[email protected]>

The default_reflog parameter of create_autostash() is passed to reset_head(). However as creating a stash does not involve updating any refs the parameter is not used by reset_head(). Removing the parameter from create_autostash() simplifies the callers.

---
 builtin/merge.c  | 6 ++----
 builtin/rebase.c | 8 ++++----
 sequencer.c      | 5 ++---
 sequencer.h      | 3 +--
 4 files changed, 9 insertions(+), 13 deletions(-)

index ea3112e0c0b..cb0e4e22258 100644
@@ -1565,8 +1565,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		if (autostash)
 			create_autostash(the_repository,
-					 git_path_merge_autostash(the_repository),
-					 "merge");
+					 git_path_merge_autostash(the_repository));
 		if (checkout_fast_forward(the_repository,
 					  &head_commit->object.oid,
 					  &commit->object.oid,

@@ -1637,8 +1636,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	if (autostash)
 		create_autostash(the_repository,
-				 git_path_merge_autostash(the_repository),
-				 "merge");
+				 git_path_merge_autostash(the_repository));
 
 	/* We are going to make a new commit. */
 	git_committer_info(IDENT_STRICT);

index 2e5a535b54e..832e6954827 100644
@@ -1658,10 +1658,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (repo_read_index(the_repository) < 0)
 		die(_("could not read index"));
 
-	if (options.autostash) {
-		create_autostash(the_repository, state_dir_path("autostash", &options),
-				 DEFAULT_REFLOG_ACTION);
-	}
+	if (options.autostash)
+		create_autostash(the_repository,
+				 state_dir_path("autostash", &options));
+
 
 	if (require_clean_work_tree(the_repository, "rebase",
 				    _("Please commit or stash them."), 1, 1)) {

index 3d3c3fbe305..5c65f5f1ac5 100644
@@ -4094,8 +4094,7 @@ static enum todo_command peek_command(struct todo_list *todo_list, int offset)
 	return -1;
 }
 
-void create_autostash(struct repository *r, const char *path,
-		      const char *default_reflog_action)
+void create_autostash(struct repository *r, const char *path)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct lock_file lock_file = LOCK_INIT;

@@ -4132,7 +4131,7 @@ void create_autostash(struct repository *r, const char *path,
 		write_file(path, "%s", oid_to_hex(&oid));
 		printf(_("Created autostash: %s\n"), buf.buf);
 		if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
-			       default_reflog_action) < 0)
+			       "") < 0)
 			die(_("could not reset --hard"));
 
 		if (discard_index(r->index) < 0 ||

index 05a7d2ba6b3..da64473636b 100644
@@ -197,8 +197,7 @@ void commit_post_rewrite(struct repository *r,
 			 const struct commit *current_head,
 			 const struct object_id *new_head);
 
-void create_autostash(struct repository *r, const char *path,
-		      const char *default_reflog_action);
+void create_autostash(struct repository *r, const char *path);
 int save_autostash(const char *path);
 int apply_autostash(const char *path);
 int apply_autostash_oid(const char *stash_oid);