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

[Derrick Stolee <derrickstolee@github.com>]

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

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