Re: [PATCH v3] diff-lib.c: handle empty deleted ita files
- From
Varun Naik <vcnaik94@gmail.com>
- Date
- Aug 19, 2019, 15:42 UTC
- Message-ID
- <CAK_rgsH0Yb=CAsCgD20xx==RaDzchtEua=q6K=7R-cfURFGHuQ@mail.gmail.com>
- In-Reply-To
- <xmqqv9uy2qpj.fsf@gitster-ct.c.googlers.com>
On Thu, Aug 15, 2019 at 12:06 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
>
> /*
> * 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".
Show 14 quoted lines
> 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".
Show 9 quoted lines
> 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