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

[Junio C Hamano <gitster@pobox.com>]

Ben Peart <peartben@gmail.com> 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?

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.