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

Ben Peart [off-list ref] writes:

quoted

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.

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.