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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2019, 19:14 UTC
Message-ID: <xmqqsgxjb2zq.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <2a3ac803-133c-98fb-45e9-43f6e4a018d1@gmail.com>

Ben Peart <peartben@gmail.com> writes:

Show 17 quoted lines

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

Ahh, chicken-and-egg. Please do add an in-code comment on why this is not "is_cache_unborn()" but must be "file_exists()" immediately before that if() statement.

Show 5 quoted lines

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

I see, and it does make sense. This explanation only matters to those who read and compare v1 and v2 and much less to those who read and consume only v2, so it probably would have made a difference if it were described in the cover letter, but a passing mention in the commit log message may be enough, if we wre to leave a record of the decision somewhere, perhaps like "As the new test involves an filesystem access, do it later in the sequence to give chance to other cheaper tests to leave early" or something at the end.

Thanks.