diff mbox series

[v2,09/14] reset_head(): make default_reflog_action optional

Message ID 9d00a218daf68a86e5dfba0c0cd66d6aaa6706b3.1638975482.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series rebase: reset_head() related fixes and improvements | expand

Commit Message

Phillip Wood Dec. 8, 2021, 2:57 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 10 ++++------
 reset.c          | 16 ++++++++++++----
 sequencer.c      |  2 +-
 3 files changed, 17 insertions(+), 11 deletions(-)

Comments

Junio C Hamano Dec. 9, 2021, 7:23 p.m. UTC | #1 
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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
> 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.

This answers the question I had on 07/14.  I think the part with
create_autostash() in this step should be dropped, and 07/14 should
be done after this step.

Unlike "", NULL makes it very clear that it will be a bug if the
implementation used it as the message.

> 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.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 10 ++++------
>  reset.c          | 16 ++++++++++++----
>  sequencer.c      |  2 +-
>  3 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 832e6954827..3d78b5c8bef 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -585,8 +585,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);
> @@ -822,7 +821,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);
>  
> @@ -1273,7 +1272,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))
> @@ -1778,8 +1777,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;
> diff --git a/reset.c b/reset.c
> index 56d6e2a06d9..4a92e4bc30b 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -22,8 +22,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 = NULL, head_oid;
>  	struct tree_desc desc[2] = { { NULL }, { NULL } };
>  	struct lock_file lock = LOCK_INIT;
> @@ -144,8 +150,10 @@ int reset_head(struct repository *r, struct object_id *oid,
>  		goto leave_reset_head;
>  	}
>  
> -	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, head, reflog_head,
> +				  reflog_orig_head, default_reflog_action,
> +				  flags);
>  
>  leave_reset_head:
>  	rollback_lock_file(&lock);
> diff --git a/sequencer.c b/sequencer.c
> index 5c65f5f1ac5..78d203dec47 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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,
> -			       "") < 0)
> +			       NULL) < 0)
>  			die(_("could not reset --hard"));
>  
>  		if (discard_index(r->index) < 0 ||
diff mbox series

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 832e6954827..3d78b5c8bef 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -585,8 +585,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);
@@ -822,7 +821,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);
 
@@ -1273,7 +1272,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))
@@ -1778,8 +1777,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;
diff --git a/reset.c b/reset.c
index 56d6e2a06d9..4a92e4bc30b 100644
--- a/reset.c
+++ b/reset.c
@@ -22,8 +22,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 = NULL, head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;
@@ -144,8 +150,10 @@  int reset_head(struct repository *r, struct object_id *oid,
 		goto leave_reset_head;
 	}
 
-	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, head, reflog_head,
+				  reflog_orig_head, default_reflog_action,
+				  flags);
 
 leave_reset_head:
 	rollback_lock_file(&lock);
diff --git a/sequencer.c b/sequencer.c
index 5c65f5f1ac5..78d203dec47 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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,
-			       "") < 0)
+			       NULL) < 0)
 			die(_("could not reset --hard"));
 
 		if (discard_index(r->index) < 0 ||