diff mbox series

[v2,07/14] create_autostash(): remove unneeded parameter

Message ID 341fe183c18ee28b459ba26f2c8c369d9367c328.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>

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge.c  | 6 ++----
 builtin/rebase.c | 8 ++++----
 sequencer.c      | 5 ++---
 sequencer.h      | 3 +--
 4 files changed, 9 insertions(+), 13 deletions(-)

Comments

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

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

It does make the callers of create_autostash() easier to reason
about, but ...

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

... makes the reader wonder what the empty string is doing here.
The fact that reset_head() does not care about the last parameter
when not given oid or switch_to_branch parameters feels like too
much implementation detail to expect the callers to know about.

Unless it is documented in reset.[ch] before the beginning of the
"int reset_head(...)" definition/declaration, that is.

>  			die(_("could not reset --hard"));
>  
>  		if (discard_index(r->index) < 0 ||
> diff --git a/sequencer.h b/sequencer.h
> index 05a7d2ba6b3..da64473636b 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -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);
Phillip Wood Jan. 25, 2022, 11:06 a.m. UTC | #2 
On 09/12/2021 19:17, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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.
> 
> It does make the callers of create_autostash() easier to reason
> about, but ...
> 
>> @@ -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)
> 
> ... makes the reader wonder what the empty string is doing here.
> The fact that reset_head() does not care about the last parameter
> when not given oid or switch_to_branch parameters feels like too
> much implementation detail to expect the callers to know about.
> 
> Unless it is documented in reset.[ch] before the beginning of the
> "int reset_head(...)" definition/declaration, that is.

I've moved this patch down the series so it looks like
-			       default_reflog_action) < 0)
+			       NULL) < 0)

which should be clearer

Best Wishes

Phillip

> 
>>   			die(_("could not reset --hard"));
>>   
>>   		if (discard_index(r->index) < 0 ||
>> diff --git a/sequencer.h b/sequencer.h
>> index 05a7d2ba6b3..da64473636b 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -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);
diff mbox series

Patch

diff --git a/builtin/merge.c b/builtin/merge.c
index ea3112e0c0b..cb0e4e22258 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -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);
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2e5a535b54e..832e6954827 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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)) {
diff --git a/sequencer.c b/sequencer.c
index 3d3c3fbe305..5c65f5f1ac5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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 ||
diff --git a/sequencer.h b/sequencer.h
index 05a7d2ba6b3..da64473636b 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -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);