Re: [PATCH 5/6] checkout(-index): do not checkout i-t-a entries

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2015, 19:56 UTC
Message-ID: <xmqq7fjyl536.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1451181092-26054-6-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 25 quoted lines

> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..d9fe8f4 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -48,6 +48,7 @@ static int checkout_file(const char *name, const char *prefix)
>  	int pos = cache_name_pos(name, namelen);
>  	int has_same_name = 0;
>  	int did_checkout = 0;
> +	int has_intent_to_add = 0;
>  	int errs = 0;
>  
>  	if (pos < 0)
> @@ -56,8 +57,11 @@ static int checkout_file(const char *name, const char *prefix)
>  	while (pos < active_nr) {
>  		struct cache_entry *ce = active_cache[pos];
>  		if (ce_namelen(ce) != namelen ||
> -		    memcmp(ce->name, name, namelen))
> +		    memcmp(ce->name, name, namelen)) {
> +			if (ce_intent_to_add(ce))
> +				has_intent_to_add = 1;
>  			break;
> +		}
>  		has_same_name = 1;
>  		pos++;
>  		if (ce_stage(ce) != checkout_stage

Sorry, but I cannot see what this hunk is trying to do.

Earlier, 'pos' is set to where the 'name' would be inserted (or would replace, if there is an existing entry with the same name). We iterate over the index starting from that 'pos' until we see a different name (i.e. the length is different, or the name does not match) and then break out, because that entry is not something we want to muck with, because the entry here you are checking with ce_intent_to_add() is an entry that is entirely unrelated to the path you are checking out.

So you do not check it out, just as before, which is correct, but then this 'has_intent_to_add' flag is used to say something nonsensical below ...

Show 7 quoted lines

> @@ -77,7 +81,9 @@ static int checkout_file(const char *name, const char *prefix)
>  
>  	if (!state.quiet) {
>  		fprintf(stderr, "git checkout-index: %s ", name);
> -		if (!has_same_name)
> +		if (has_intent_to_add)
> +			fprintf(stderr, "is not yet in the cache");

... the reason why you are here does not have anything to do with the entry that happened to come after the path you wanted to check out was marked with the I-T-A bit. The reason why you are here is because 'did_checkout' was unset, which in turn was because there wasn't a path that match 'name' you are checking out in the index, with or without the I-T-A bit.

    $ rm .git/index
    $ >file
    $ git add -N file
    $ git checkout-index fild

would hit this codepath, I think, and the code would give a message that says "fild is not yet in the cache", which may be technically correct but it is the same as !has_same_name's "fild is not in the cache".

Probably the I-T-A check must go after the code decides that 'ce' is an entry for the 'name' the caller asked us to check out (and of course we shouldn't check it out in that case).

Show 6 quoted lines

> @@ -99,6 +105,8 @@ static void checkout_all(const char *prefix, int prefix_length)
>  		if (ce_stage(ce) != checkout_stage
>  		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>  			continue;
> +		if (ce_intent_to_add(ce))
> +			continue;

This one is probably correct, I didn't check the context to fully see if it is valid, though.

Show 23 quoted lines

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3e141fc..ac37d92 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -328,12 +328,15 @@ static int checkout_paths(const struct checkout_opts *opts,
>  	if (opts->merge)
>  		unmerge_marked_index(&the_index);
>  
> -	/* Any unmerged paths? */
>  	for (pos = 0; pos < active_nr; pos++) {
> -		const struct cache_entry *ce = active_cache[pos];
> +		struct cache_entry *ce = active_cache[pos];
>  		if (ce->ce_flags & CE_MATCHED) {
> -			if (!ce_stage(ce))
> +			if (!ce_stage(ce)) {
> +				if (ce_intent_to_add(ce))
> +					ce->ce_flags &= ~CE_MATCHED;
>  				continue;
> +			}
> +			/* Unmerged paths */
>  			if (opts->force) {
>  				warning(_("path '%s' is unmerged"), ce->name);
>  			} else if (opts->writeout_stage) {

This loop has become dual-purpose; it is drops CE_MATCHED bit from cache entries with the I-T-A bit, and it continues to handle errors for an attempt to checkout unmerged paths.

    $ rm .git/index
    $ >file
    $ git add -N file
    $ git checkout file

would hit this codepath, and it will force-skip "file".

There is a small source of confusion for new people here, though.

    $ git checkout filo

would complain that there is no 'filo'.  But

    $ git checkout file

would silently turn itself into a no-op. Don't we want to at least give some indication that we are not checking out "file" because its contents do not yet exist in the index, or something?

Show 43 quoted lines

> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 96c8755..52e9f7f 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -111,5 +111,39 @@ test_expect_success 'apply:check_preimage() not creating empty file' '
>  	)
>  '
>  
> +test_expect_success 'checkout ignores i-t-a' '
> +	git init checkout &&
> +	(
> +		cd checkout &&
> +		echo data >file &&
> +		git add -N file &&
> +		git checkout -- file &&
> +		echo data >expected &&
> +		test_cmp expected file
> +	)
> +'
> +
> +test_expect_success 'checkout-index ignores i-t-a' '
> +	(
> +		cd checkout &&
> +		git checkout-index file &&
> +		echo data >expected &&
> +		test_cmp expected file
> +	)
> +'
> +
> +test_expect_success 'checkout-index --all ignores i-t-a' '
> +	(
> +		cd checkout &&
> +		echo data >anotherfile &&
> +		git add anotherfile &&
> +		rm anotherfile &&
> +		git checkout-index --all &&
> +		echo data >expected &&
> +		test_cmp expected file &&
> +		test_cmp expected anotherfile
> +	)
> +'
> +
>  test_done