Re: [PATCH v2 2/3] setup: add discover_git_directory_reason()

From: Derrick Stolee <derrickstolee@github.com>
Date: Aug 22, 2023, 19:39 UTC
Message-ID: <1550d741-009f-4c27-90a4-f1b91701cf6c@github.com>
In-Reply-To: <xmqqttsqlvsd.fsf@gitster.g>

On 8/22/2023 3:30 PM, Junio C Hamano wrote:

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>

>> It is worth noting that GIT_DIR_NONE is not returned, so we remove this
>> option from the enum. We must be careful to keep the successful reasons
>> as positive values, so they are given explicit positive values.
>> Further, a use in scalar.c was previously impossible, so it is removed.

(Relevant bit from the message.)

Show 25 quoted lines

>> diff --git a/scalar.c b/scalar.c
>> index 938bb73f3ce..02a38e845e1 100644
>> --- a/scalar.c
>> +++ b/scalar.c
>> @@ -686,9 +686,6 @@ static int cmd_reconfigure(int argc, const char **argv)
>>  				warning(_("removing stale scalar.repo '%s'"),
>>  					dir);
>>  			strbuf_release(&buf);
>> -		} else if (discover_git_directory(&commondir, &gitdir) < 0) {
>> -			warning_errno(_("git repository gone in '%s'"), dir);
>> -			res = -1;
>>  		} else {
>>  			git_config_clear();
>>  
> 
> In the original before this series, and also after applying [PATCH
> 3/3], the reconfiguration is a three-step process:
> 
>  - what if we cannot go to that directory?
>  - what if the directory is not a usable repository?
>  - now we are in a usable repository, let's reconfigure.
> 
> and this seems to lose the second step tentatively?  It is
> resurrected in a more enhanced form in [PATCH 3/3] so it may not be
> a huge deal, but it looks like it is not intended lossage.

I know what happened here. At some point during my edits, this line was changed to "else if (!discover_git_directory(...))" which became an unreachable case.

So, based on the intermediate patch that did not survive, it made sense, but you are right that this hunk of this patch is behaving badly. Good eye!

Thanks, -Stolee