From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 25 Jan 2022 11:06:19 GMT
Subject: Re: [PATCH v2 07/14] create_autostash(): remove unneeded parameter
Message-ID: <36966d92-1460-699a-007d-da2a7da6c088@gmail.com>
In-Reply-To: <xmqqpmq5r3j9.fsf@gitster.g>

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