On 1/22/2019 1:54 PM, Junio C Hamano wrote:
Ben Peart <peartben@xxxxxxxxx> 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.
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.
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.
