Re: [PATCH] checkout.c: unstage empty deleted ita files

[Jeff King <peff@peff.net>]

On Thu, Jul 25, 2019 at 09:56:45PM -0700, Varun Naik wrote:

> It is possible to delete a committed file from the index and then add it
> as intent-to-add. After `git checkout HEAD` or `git restore --staged`,
> the file should be identical in the index and HEAD. This patch provides
> the desired behavior even when the file is empty in the index.

OK, so the issue is that ITA entries have an empty-file sha1, so they
confuse the logic to decide if we can use the old entry. Your fix makes
sense.

> ---
> CC Jeff because you wrote the code that I am changing now.
> 
> checkout.c:update_some() discards the newly created cache entry when its
> mode and oid match those of the old entry. Since an ita file has the
> same oid as an empty file, an empty deleted ita file passes both of
> these checks, and the new entry is discarded. In this case, the file
> should be added to the cache instead.
> 
> This change should not affect newly added ita files. For those, inside
> tree.c:read_tree_1(), tree_entry_interesting() returns
> entry_not_interesting, so fn (which points to update_some()) is never
> called.

These two paragraphs would be a nice addition to the actual commit
message.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 91f8509f85..27daa09c3c 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,
>  	if (pos >= 0) {
>  		struct cache_entry *old = active_cache[pos];
>  		if (ce->ce_mode == old->ce_mode &&
> +		    !ce_intent_to_add(old) &&
>  		    oideq(&ce->oid, &old->oid)) {
>  			old->ce_flags |= CE_UPDATE;
>  			discard_cache_entry(ce);

My first thought here was that we could skip ITA entries here only when
the HEAD hash is also the empty blob, which would let us retain index
results in more cases. But it doesn't help. If the HEAD entry isn't the
empty blob, then we'll have !oideq() and we'll skip anyway, because an
ITA entry must be the empty blob (if we `git add` some other content,
then it ceases to be ITA).

So it makes sense to just always skip this "retain the old index entry"
block for any ITA entry.

> +test_expect_success 'checkout HEAD adds deleted intent-to-add file back to index' '
> +	echo "nonempty" >nonempty &&
> +	>empty &&
> +	git add nonempty empty &&
> +	git commit -m "create files to be deleted" &&
> +	git rm --cached nonempty empty &&
> +	git add -N nonempty empty &&
> +	git checkout HEAD nonempty empty &&
> +	git diff --staged --exit-code
> +'

This clearly demonstrates the problem. Nice.

> +test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '
> +	echo "nonempty" >nonempty &&
> +	>empty &&
> +	git add nonempty empty &&
> +	git commit -m "create files to be deleted" &&
> +	git rm --cached nonempty empty &&
> +	git add -N nonempty empty &&
> +	git restore --staged nonempty empty &&
> +	git diff --staged --exit-code
> +'

Hmm. This git-restore test means we don't apply to maint. But wouldn't
we want the fix for "checkout" there?

I.e., I'd expect a patch to fix and test git-checkout, and then an
additional patch to be added on the merge of that plus master to test
git-restore.

Other than that, the patch looks good to me.

-Peff