git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Varun Naik <vcnaik94@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, pclouds@gmail.com
Subject: Re: [PATCH v2] reset: unstage empty deleted ita files
Date: Sun, 28 Jul 2019 23:52:20 -0700	[thread overview]
Message-ID: <CAK_rgsFDpW3JhMAc=VBm=WD3drFnV0g=QN15+uZYdsnBTnX_Cg@mail.gmail.com> (raw)
In-Reply-To: <xmqqsgqsodyu.fsf@gitster-ct.c.googlers.com>

On Fri, Jul 26, 2019 at 11:20 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
>

To guard against changes to the test cases in the future, would it be
better if I write something like the following instead?
    git diff --cached --exit-code HEAD nonempty empty

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

Thank you for the detailed explanation, this really helped me
understand the internals of do_diff_cache(). While adjusting my commit
message, I realized that my change actually breaks `git reset HEAD`
for ita files (i.e. after `git add -N nonempty` and `git reset HEAD
nonempty`, the file is still marked as intent-to-add). So, instead of
setting the flag ita_invisible_in_index to 1 before calling
do_diff_cache(), we want to handle a specific edge case deep inside
the diff machinery. It looks like I fixed another bug in the process,
so I will write a test case for that and then send out v3.

> Thanks.
>
> > +'
> > +
> >  test_done

  reply	other threads:[~2019-07-29  6:52 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
2019-07-29  6:52     ` Varun Naik [this message]
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='CAK_rgsFDpW3JhMAc=VBm=WD3drFnV0g=QN15+uZYdsnBTnX_Cg@mail.gmail.com' \
    --to=vcnaik94@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@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