Re: [PATCH v2 0/2] Fix regression in checkout -b

From: Ben Peart <peartben@gmail.com>
Date: Jan 22, 2019, 19:31 UTC
Message-ID: <2a3ac803-133c-98fb-45e9-43f6e4a018d1@gmail.com>
In-Reply-To: <xmqq4la0h6am.fsf@gitster-ct.c.googlers.com>

On 1/22/2019 1:54 PM, Junio C Hamano wrote:

Show 41 quoted lines

> Ben Peart <peartben@gmail.com> writes:
> 
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index af6b5c8336..9c6e94319e 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -517,12 +517,6 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>>   	if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
>>   		return 0;
>>   
>> -	/*
>> -	 * We must do the merge if this is the initial checkout
>> -	 */
>> -	if (is_cache_unborn())
>> -		return 0;
>> -
>>   	/*
>>   	 * We must do the merge if we are actually moving to a new commit.
>>   	 */
>> @@ -598,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>>   	 * Remaining variables are not checkout options but used to track state
>>   	 */
>>   
>> +	 /*
>> +	  * Do the merge if this is the initial checkout
>> +	  *
>> +	  */
>> +	if (!file_exists(get_index_file()))
>> +		return 0;
>> +
>>   	return 1;
>>   }
> 
> This is curious.  The location the new special case is added is
> different, and the way the new special case is detected is also
> different, between v1 and v2.  Are both of them significant?  IOW,
> if we moved the check down but kept using is_cache_unborn(), would
> it break?  Or if we did not move the check but switched to check the
> index file on the filesystem instead of calling is_cache_unborn(),
> would it break?
>

I had to change the check to not use is_cache_unborn() because at this point, the index has not been loaded so cache_nr and timestamp.sec are always zero (thus defeating the entire optimization). Since part of the optimization was to avoid loading the index when it isn't necessary, the only replacement I could think of was to check for the existence of the index file as if it is missing entirely, it is clearly unborn. This solved the behavior change for the --no-checkout sequence reported.

The only reason I moved it lower in the function was a micro perf optimization. Since file_exists() does file I/O, I thought I'd do all the in memory/flag checks first in case they drop out early and we can avoid the unnecessary file I/O. As long as it is tested before the 'return 1;' call, it is logically correct.

Show 7 quoted lines

> There are three existing callers of is_{cache,index}_unborn(), all
> of which want to use it to decide if we are in this funny "unborn"
> state.  If this fixes the issue we saw in v1 of these two patches,
> does that mean these three existing callers also are buggy in the
> same way and we are better off rewriting is_index_unborn() to see if
> the index file is on the disk?
>

It is just the fact that I needed to check for an unborn index _before_ it was loaded that makes me unable to use is_{cache,index}_unborn() here. The other callers should still be fine. I could add a comment in the code to clarify this if you think it will cause confusion later.

Show 9 quoted lines

> I am *not* suggesting to make such a drastic change to the existing
> system.  I am wondering why they are working fine but only this new
> code has to avoid the existing is_index_unborn() logic and go
> directly to the filesystem.  Especially as this new exception added
> to "skip-merge-working-tree" is to allow the special case code in
> merge-working-tree that depends on is_cache_unborn() to trigger.
> 
> Thanks for working on this.
>