[PATCH] reset: unstage empty deleted ita files

[PATCH] reset: unstage empty deleted ita files

[Varun Naik]

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.

Signed-off-by: Varun Naik <vcnaik94@gmail.com>
---
My first patch!

According to an old thread on the mailing list[0], this issue is known,
but it looks like people overlooked the fix that I'm making. Basically,
this one line causes diff-lib.c:do_oneway_diff() to call show_modified()
instead of diff_index_show_file() for ita files.

An alternative to the last line of the test code is running `git status
--porcelain=v2` and then inspecting the output. I can adjust that if
necessary.

Unfortunately, "restore" has a similar problem, but it follows a
different code path into read-cache.c rather than diff-lib.c, and there
does not seem to be a similar one-line fix.

[0]: https://public-inbox.org/git/CACsJy8Bov1asw+_J_fbhKqigM==xNPi8itDGkhibkYVch4pvmQ@mail.gmail.com/

 builtin/reset.c  |  1 +
 t/t7102-reset.sh | 11 +++++++++++
 2 files changed, 12 insertions(+)

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..250fc1c890 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 removes deleted intent-to-add file from 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
+'
+
 test_done
-- 
2.22.0

[PATCH v2] reset: unstage empty deleted ita files

[Varun Naik]

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.

Signed-off-by: Varun Naik <vcnaik94@gmail.com>
---
CC Duy because you seem to be the one who is most involved in changes to
ita behavior.

The test description was incorrect, so I changed that now. I also
figured out how to fix the related problem in "restore"; separate patch
coming soon.

 builtin/reset.c  |  1 +
 t/t7102-reset.sh | 11 +++++++++++
 2 files changed, 12 insertions(+)

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
+'
+
 test_done
-- 
2.22.0

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

[Junio C Hamano]

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

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

[Varun Naik]

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

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

[Junio C Hamano]

Varun Naik <vcnaik94@gmail.com> writes:

> 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

Hmph, that is probably a good idea, as it matches the kind of
"reset" we just did, which is what we are testing.

[PATCH v3] diff-lib.c: handle empty deleted ita files

[Varun Naik]

It is possible to delete a committed file from the index and then add it
as intent-to-add. Certain forms of `git diff` should show the file.
After `git reset HEAD`, the file should be identical in the index and
HEAD. The commands already work correctly if the file has contents in
HEAD. This patch provides the desired behavior even when the file is
empty in HEAD.

The affected "diff" commands and the "reset" command call
diff-lib.c:do_oneway_diff() with a cache entry in the index and a cache
entry in HEAD. An ita file is represented in the index by a cache entry
with the same hash as an empty file. For a nonempty deleted ita file,
do_oneway_diff() calls show_modified(), which detects a diff between the
cache entry in the index and the cache entry in HEAD and therefore deems
the file "modified". However, for an empty deleted ita file,
do_oneway_diff() previously detected no such diff between the two cache
entries and therefore deemed the file "not modified". After this fix,
for any deleted ita file, do_oneway_diff() calls diff_index_show_file()
and deems the file "deleted".

`git diff-index --cached HEAD` prints a row of output for both a
"modified" and a "deleted" file, although the output differs slightly.
`git reset HEAD` treats a "modified" and a "deleted" file similarly,
resurrecting the file in the index from HEAD.

This change should not affect newly added ita files. For those, the
"tree" cache entry is NULL, so the changed code is not executed.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Varun Naik <vcnaik94@gmail.com>
---
I tried to limit the "blast radius" of affected commands as much as
possible. To find commands that were affected, I ran the following:
    git diff
    git diff HEAD
    git diff --cached HEAD
    git diff-index HEAD
    git diff-index --cached HEAD
    git diff-files

I also ran each command with the option "--ita-visible-in-index" and the
option "--ita-invisible-in-index". Of these 18 commands, the following
three showed a diff for nonempty deleted ita files, but no diff for
empty deleted ita files. All three commands now work correctly, but I
chose the first one for the test case because the option
"--ita-visible-in-index" is still marked as experimental.
    git diff-index --cached HEAD
    git diff-index --cached --ita-visible-in-index HEAD
    git diff --cached --ita-visible-in-index HEAD

The `git add` at the end of the "diff-index" test case is necessary
because `git reset --hard HEAD` at the beginning of the next test case
_also_ breaks for empty deleted ita files. That command goes into
unpack-trees.c:oneway_merge() rather than diff-lib.c:do_oneway_diff().
I plan to create a separate patch to fix that, after I figure out which
commands are part of its blast radius.

 diff-lib.c            |  5 ++++-
 t/t2203-add-intent.sh | 13 +++++++++++++
 t/t7102-reset.sh      | 11 +++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index 61812f48c2..29dba467d5 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -433,8 +433,11 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 
 	/*
 	 * Something removed from the tree?
+	 * Consider a file deleted from the index and added as ita to be "deleted",
+	 * even though it should arguably be "modified", because we want empty
+	 * deleted ita files to appear in the diff.
 	 */
-	if (!idx) {
+	if (!idx || (cached && ce_intent_to_add(idx))) {
 		diff_index_show_file(revs, "-", tree, &tree->oid, 1,
 				     tree->ce_mode, 0);
 		return;
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 68e54d5c44..4e4a972921 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -261,6 +261,19 @@ test_expect_success '"diff HEAD" includes ita as new files' '
 	test_cmp expected actual
 '
 
+test_expect_success '"diff-index --cached HEAD" detects diff for deleted intent-to-add file' '
+	git reset --hard &&
+	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 &&
+	test_expect_code 1 git diff-index --cached --exit-code HEAD nonempty &&
+	test_expect_code 1 git diff-index --cached --exit-code HEAD empty &&
+	git add nonempty empty
+'
+
 test_expect_success 'apply --intent-to-add' '
 	git reset --hard &&
 	echo new >new-ita &&
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 97be0d968d..7b79502f7d 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 removes deleted intent-to-add file from 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 --cached --exit-code nonempty empty
+'
+
 test_done
-- 
2.22.0

Re: [PATCH v3] diff-lib.c: handle empty deleted ita files

[Varun Naik]

On Thu, Aug 1, 2019 at 9:16 AM Varun Naik <vcnaik94@gmail.com> wrote:
>
> It is possible to delete a committed file from the index and then add it
> as intent-to-add. Certain forms of `git diff` should show the file.
> After `git reset HEAD`, the file should be identical in the index and
> HEAD. The commands already work correctly if the file has contents in
> HEAD. This patch provides the desired behavior even when the file is
> empty in HEAD.
>
> The affected "diff" commands and the "reset" command call
> diff-lib.c:do_oneway_diff() with a cache entry in the index and a cache
> entry in HEAD. An ita file is represented in the index by a cache entry
> with the same hash as an empty file. For a nonempty deleted ita file,
> do_oneway_diff() calls show_modified(), which detects a diff between the
> cache entry in the index and the cache entry in HEAD and therefore deems
> the file "modified". However, for an empty deleted ita file,
> do_oneway_diff() previously detected no such diff between the two cache
> entries and therefore deemed the file "not modified". After this fix,
> for any deleted ita file, do_oneway_diff() calls diff_index_show_file()
> and deems the file "deleted".
>
> `git diff-index --cached HEAD` prints a row of output for both a
> "modified" and a "deleted" file, although the output differs slightly.
> `git reset HEAD` treats a "modified" and a "deleted" file similarly,
> resurrecting the file in the index from HEAD.
>
> This change should not affect newly added ita files. For those, the
> "tree" cache entry is NULL, so the changed code is not executed.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Varun Naik <vcnaik94@gmail.com>
> ---

Bumping this email, since I noticed v2 is still in pu.

Varun

Re: [PATCH v3] diff-lib.c: handle empty deleted ita files

[Junio C Hamano]

Varun Naik <vcnaik94@gmail.com> writes:

> diff --git a/diff-lib.c b/diff-lib.c
> index 61812f48c2..29dba467d5 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -433,8 +433,11 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  
>  	/*
>  	 * Something removed from the tree?
> +	 * Consider a file deleted from the index and added as ita to be "deleted",
> +	 * even though it should arguably be "modified", because we want empty
> +	 * deleted ita files to appear in the diff.
>  	 */
> -	if (!idx) {
> +	if (!idx || (cached && ce_intent_to_add(idx))) {
>  		diff_index_show_file(revs, "-", tree, &tree->oid, 1,
>  				     tree->ce_mode, 0);
>  		return;

There is already half of the same logic near the beginning of this
function, no?

	/*
	 * i-t-a entries do not actually exist in the index (if we're
	 * looking at its content)
	 */
	if (o->index_only &&
	    revs->diffopt.ita_invisible_in_index &&
	    idx && ce_intent_to_add(idx)) {
		idx = NULL;
		if (!tree)
			return;	/* nothing to diff.. */
	}

IOW, when ita-invisible-in-index flag is set, idx is made NULL and
all the rest of the function behaves as if there is no such entry in
the index (e.g. relative to HEAD it looks as if the entry is removed
in the index).

So for example, when ita-invisible-in-index is not set, this piece,
just above the part you touched, kicks in:

	/*
	 * Something added to the tree?
	 */
	if (!tree) {
		show_new_file(revs, idx, cached, match_missing);
		return;
	}

and says "no such entry in the tree, but you have an I-T-A entry
there in the index".

It is unclear why we can unconditionally declare "I-T-A entry does
not exist, the entry was in the tree but not in the index" in the
code you touched, without consulting ita-invisible-in-index flag.
It feels awfully inconsistent to me.

Of course, consistency could go the other way around, and the right
fix to achieve consistency might turn out to be to drop the check
for ita-invisible-in-index flag (and perhaps the flag itself) from
the early part of this function.  I dunno.

Thanks.

Re: [PATCH v3] diff-lib.c: handle empty deleted ita files

[Varun Naik]

On Thu, Aug 15, 2019 at 12:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
>         /*
>          * i-t-a entries do not actually exist in the index (if we're
>          * looking at its content)
>          */
>         if (o->index_only &&
>             revs->diffopt.ita_invisible_in_index &&
>             idx && ce_intent_to_add(idx)) {
>                 idx = NULL;
>                 if (!tree)
>                         return; /* nothing to diff.. */
>         }
>
> IOW, when ita-invisible-in-index flag is set, idx is made NULL and
> all the rest of the function behaves as if there is no such entry in
> the index (e.g. relative to HEAD it looks as if the entry is removed
> in the index).
>

That's right. I was hoping to avoid simply setting the ita-invisible-in-
index flag because then an ita file (i.e. a file marked as ita in the
index with no contents in HEAD) would be considered nonexistent in both
the tree and the index, and so the function would hit the return
statement above because it believes there is "nothing to diff".

> So for example, when ita-invisible-in-index is not set, this piece,
> just above the part you touched, kicks in:
>
>         /*
>          * Something added to the tree?
>          */
>         if (!tree) {
>                 show_new_file(revs, idx, cached, match_missing);
>                 return;
>         }
>
> and says "no such entry in the tree, but you have an I-T-A entry
> there in the index".
>

That sounds right, an ita file is considered to be a "new file".

> It is unclear why we can unconditionally declare "I-T-A entry does
> not exist, the entry was in the tree but not in the index" in the
> code you touched, without consulting ita-invisible-in-index flag.
> It feels awfully inconsistent to me.
>
> Of course, consistency could go the other way around, and the right
> fix to achieve consistency might turn out to be to drop the check
> for ita-invisible-in-index flag (and perhaps the flag itself) from
> the early part of this function.  I dunno.

Actually, I agree that it seems strange to consider a deleted ita file
as a "deleted file", when an ita file that did not previously exist in
the tree is considered a "new file". It felt like a dirty hack when I
was originally writing it. And the longer I look at the logic that I
added towards the end of the function, the more I worry that it
sacrifices readability and maintainability to achieve minimal gain.

Dropping the check at the beginning is probably not right either - this
would break a lot of other places that call into do_oneway_diff().

This function is probably not the place where we want to make changes.
It would be better to change diff-lib.c:show_modified() and
diff.c:diff_change() to consider the intent-to-add bit when performing a
diff. A small change to show_modified() prevents the function from
returning prematurely when "new_entry" has the intent-to-add bit set.
But now, the call to diff_change() hits the error "feeding unmodified %s
to diffcore". We can avoid this by adding a boolean "int ita" field to
"struct diff_filespec" and adjusting the following code:

@@ -5847,7 +5847,7 @@ static void diff_resolve_rename_copy(void)
                        else
                                p->status = DIFF_STATUS_RENAMED;
                }
-               else if (!oideq(&p->one->oid, &p->two->oid) ||
+               else if (!oideq(&p->one->oid, &p->two->oid) || p->two->ita ||
                         p->one->mode != p->two->mode ||
                         p->one->dirty_submodule ||
                         p->two->dirty_submodule ||

Next, we need to pass something like "int new_ita" to diff_change() and
set "two->ita" accordingly. This is where I'm stuck right now. It's easy
enough to adjust the call to diff_change() inside show_modified(), but
the adjustments to other calls to diff_change(), especially the one
inside diff-lib.c:run_diff_files(), might need some other accompanying
changes.

Does changing the diffcore code seem like a reasonable approach? If not,
then I can't think of a better one, and it might be best to leave this
patch unmerged.

Varun

Re: [PATCH v3] diff-lib.c: handle empty deleted ita files

[Junio C Hamano]

Varun Naik <vcnaik94@gmail.com> writes:

> This function is probably not the place where we want to make changes.
> It would be better to change diff-lib.c:show_modified() and
> diff.c:diff_change() to consider the intent-to-add bit when performing a
> diff.

I do agree that diff-lib.c::show_modified(), which is about "git
diff-cache" (and hence "git diff <tree-ish>", i.e. comparison
between a tree-ish and the index), is the right layer for this
change.

I am not offhand sure about the diff.c::diff_change(), though, and
cannot say much without first thinking about it a bit more.

Thanks.

Re: [PATCH v3] diff-lib.c: handle empty deleted ita files

[Varun Naik]

On Mon, Aug 19, 2019 at 1:15 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> I do agree that diff-lib.c::show_modified(), which is about "git
> diff-cache" (and hence "git diff <tree-ish>", i.e. comparison
> between a tree-ish and the index), is the right layer for this
> change.
>
> I am not offhand sure about the diff.c::diff_change(), though, and
> cannot say much without first thinking about it a bit more.

I'm back!

I made the changes to "struct diff_filespec" and diff_resolve_rename_copy()
that I mentioned in my previous email on this thread, and it seems to work.
But the patch seems very clunky. Even though it has been in the back of my
mind for six months, I can't think of a more elegant approach. Furthermore,
the few test cases that I could create are extremely unlikely in practice. For
all of these reasons, I would rather avoid this patch altogether.

v2 is still sitting in pu. Could we discard it? I'll send an update to the
related patch [0] in a few minutes.

[0]: https://public-inbox.org/git/20190821032100.73917-1-vcnaik94@gmail.com/

Best,
Varun