git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Varun Naik <vcnaik94@gmail.com>
Cc: git@vger.kernel.org, pclouds@gmail.com
Subject: Re: [PATCH v2] reset: unstage empty deleted ita files
Date: Fri, 26 Jul 2019 11:19:53 -0700	[thread overview]
Message-ID: <xmqqsgqsodyu.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190726044806.2216-1-vcnaik94@gmail.com> (Varun Naik's message of "Thu, 25 Jul 2019 21:48:06 -0700")

Varun Naik <vcnaik94@gmail.com> writes:

> It is possible to delete a committed file from the index and then add it
> as intent-to-add. After `git reset HEAD`, 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.

The first and the second sentence describes what you want to achieve
concisely and sensibly.  There is a vast leap between them and the
last sentence.  What's missing is:

 - What goes wrong without this one-liner fix and how does the
   command make a wrong decision to leave the path 'empty' (in the
   new test) different from that of the tree of the HEAD?

 - How does the change help the machinery to make a right decision
   instead?

I had to briefly wonder if this change interacts with "reset -N" in
any way.  In that mode, we want to make sure that diff sees the
entries that are missing from the index that exist in the tree of
the HEAD, so that update_index_from_diff() can add them back to the
index as I-T-A entries.

Making I-T-A entries invisible in the index for the purpose of
running that diff would mean that they appear as removed, but it is
OK because we'll add them back as I-T-A entries anyway, so it all
evens out, I think.

> diff --git a/builtin/reset.c b/builtin/reset.c
> index 26ef9a7bd0..47a088f4b7 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -163,6 +163,7 @@ static int read_from_tree(const struct pathspec *pathspec,
>  	opt.format_callback_data = &intent_to_add;
>  	opt.flags.override_submodule_config = 1;
>  	opt.repo = the_repository;
> +	opt.ita_invisible_in_index = 1;
>  
>  	if (do_diff_cache(tree_oid, &opt))
>  		return 1;
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 97be0d968d..9f3854e8f0 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -566,4 +566,15 @@ test_expect_success 'reset --mixed sets up work tree' '
>  	test_must_be_empty actual
>  '
>  
> +test_expect_success 'reset --mixed 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 reset HEAD nonempty empty &&
> +	git diff --staged --exit-code

We are not testing if the "diff --staged" synonym (that is not even
in "git diff --help") behaves identically to "diff --cached" here
(if we wanted to do such a test, we should do so somewhere in t4xxx
series, not here), so let's spell it in the canonical form, like so:

	git diff --cached --exit-code HEAD

At this point, we have three paths (empty, nonempty and secondfile)
in the tree of the HEAD, and we just resetted the entries for the
first two paths in the index to match.  The 'secondfile' added, in
previous tests, is still there unchanged, and is not shown in the
diff output.  The 'new-file', added as I-T-A in previous tests, is
also still there unchanged, and is not shown in the diff output.

How does the internal do_diff_cache() behave differently before and
after this patch on 'empty' and 'nonempty'?  How does the difference
affect the final outcome of "git reset" operation?

 - without ita-is-invisible bit set, we end up comparing the mode
   and blob object name; for 'nonempty', HEAD records a blob object
   name for the non-empty content, but the index has an empty blob
   object name (with I-T-A bit set, but that does not participate in
   the diff operation at the level of diff-lib.c::do_oneway_diff())
   and are deemed "modified".  Even though we should say "deleted",
   the end result turns out to be the same---we restore from the
   tree of the HEAD.

 - however, for 'empty', we mistakenly end up saying "both are empty
   blobs, so no difference; nothing to be done", leaving the i-t-a
   entry in the index.

 - with ita-is-invisible bit set, both 'nonempty' and 'empty' are
   immediately marked as "deleted" in do_oneway_diff().  This causes
   both paths to be resurrected from the tree of the HEAD the same
   way.

With the above kind of analysis, a reader can fill in the leap in
the explanation between the second and the third sentence in the
proposed log message.  But we shouldn't force readers to make that
effort to understand how the patch was meant to improve things.

Thanks.

> +'
> +
>  test_done

  reply	other threads:[~2019-07-26 18:20 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-12 15:02 [PATCH] " Varun Naik
2019-07-26  4:48 ` [PATCH v2] " Varun Naik
2019-07-26 18:19   ` Junio C Hamano [this message]
2019-07-29  6:52     ` Varun Naik
2019-07-29 16:07       ` Junio C Hamano
2019-08-01 16:15 ` [PATCH v3] diff-lib.c: handle " Varun Naik
2019-08-15 16:26   ` Varun Naik
2019-08-15 19:06   ` Junio C Hamano
2019-08-19 15:42     ` Varun Naik
2019-08-19 20:15       ` Junio C Hamano
2020-02-14 17:12         ` 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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqsgqsodyu.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=vcnaik94@gmail.com \
    --subject='Re: [PATCH v2] reset: unstage empty deleted ita files' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git