[04/11] reset_head(): remove action parameter

Commit Message

Phillip Wood Oct. 1, 2021, 10:04 a.m. UTC

From: Phillip Wood <[e-mail]>

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.

Signed-off-by: Phillip Wood <[e-mail]>
---
 builtin/rebase.c | 14 +++++++-------
 reset.c          |  5 +++--
 reset.h          |  2 +-
 sequencer.c      |  3 +--
 4 files changed, 12 insertions(+), 12 deletions(-)

Comments

Junio C Hamano Oct. 1, 2021, 8:58 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> 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.

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.

Phillip Wood Oct. 4, 2021, 10 a.m. UTC | #2

Hi Junio

On 01/10/2021 21:58, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> 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.
> 
> 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".

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.

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

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

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

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index f4c312dd8b5..26f53c55cc4 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/reset.c b/reset.c
index 5abb1a5683e..9ab007c0c34 100644
--- a/reset.c
+++ b/reset.c
@@ -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;
diff --git a/reset.h b/reset.h
index 12f83c78e28..2daec804259 100644
--- a/reset.h
+++ b/reset.h
@@ -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);
diff --git a/sequencer.c b/sequencer.c
index 68316636921..07d2a582d39 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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"));