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

[Varun Naik <vcnaik94@gmail.com>]

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