Re: [PATCH v2] reset: unstage empty deleted ita files

[Varun Naik <vcnaik94@gmail.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