From: Ben Peart Date: Tue, 22 Jan 2019 19:31:16 GMT Subject: Re: [PATCH v2 0/2] Fix regression in checkout -b Message-ID: <2a3ac803-133c-98fb-45e9-43f6e4a018d1@gmail.com> In-Reply-To: On 1/22/2019 1:54 PM, Junio C Hamano wrote: > Ben Peart 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. >