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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2019, 18:54 UTC
Message-ID: <xmqq4la0h6am.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190121195008.8700-1-peartben@gmail.com>

Ben Peart <peartben@gmail.com> writes:

Show 30 quoted lines

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

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?

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.