mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Varun Naik <>
Subject: Re: [PATCH] checkout.c: unstage empty deleted ita files
Date: Fri, 26 Jul 2019 04:57:57 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

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

> 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

Other than that, the patch looks good to me.


  parent reply	other threads:[~2019-07-26  8:58 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-26  4:56 [PATCH] checkout.c: unstage empty deleted ita files Varun Naik
2019-07-26  5:01 ` Varun Naik
2019-07-26  8:57 ` Jeff King [this message]
2019-07-29  6:54   ` Varun Naik
2019-07-29  9:11     ` Jeff King
2019-08-01 16:07 ` [PATCH v2] " Varun Naik
2019-08-01 17:34   ` Junio C Hamano
2019-08-01 21:51   ` Jeff King
2019-08-01 16:09 ` [PATCH] restore: add test for " Varun Naik
2019-08-02 16:16   ` Junio C Hamano
2019-08-02 16:28 ` [PATCH v3] checkout.c: unstage empty " Varun Naik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).