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, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH v3] diff-lib.c: handle empty deleted ita files
Date: Mon, 19 Aug 2019 08:42:08 -0700	[thread overview]
Message-ID: <CAK_rgsH0Yb=CAsCgD20xx==RaDzchtEua=q6K=7R-cfURFGHuQ@mail.gmail.com> (raw)
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:
>
>         /*
>          * 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

  reply	other threads:[~2019-08-19 15:42 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-12 15:02 [PATCH] reset: unstage " 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
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 [this message]
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_rgsH0Yb=CAsCgD20xx==RaDzchtEua=q6K=7R-cfURFGHuQ@mail.gmail.com' \
    --to=vcnaik94@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --subject='Re: [PATCH v3] diff-lib.c: handle 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