Re: [PATCH 2/3] diff-lib.c: enable --shift-ita in index_differs_from()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 28, 2016, 18:49 UTC
Message-ID: <xmqq8tubkgg5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160928114348.1470-3-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 22 quoted lines

> This function is basically "git diff --cached HEAD", It has three
> callers:
>
>  - One in builtin/commit.c, which uses it to determine if the index is
>    different from HEAD and go ahead making a new commit.
>
>  - Two in sequencer.c, which use it to see if the index is dirty.
>
> In the first case, if ita entries are present, index_differs_from() may
> report "dirty". However at tree creation phase, ita entries are dropped
> and the result tree may look exactly the same as HEAD (assuming that
> nothing else is changed in index). This is what we need index_differs_from()
> for, to catch new empty commits. Enabling shift_ita in index_differs_from()
> fixes this.
>
> In the second case, the presence of ita entries are enough to say the
> index is dirty and not continue on. Make an explicit check for that
> before comparing index against HEAD (whether --shift-ita is present is
> irrelevant)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

There are three callers to index_differs_from(), which asks "is the index different from the HEAD". Because you want to change the behaviour of the function for one of these callers while not exposing its undesirable behaviour for the other two callers, you guard the call to it with another call to a new helper function, which needs to scan the entire index one more time.

It somehow sounds like backwards to me.

IOW, I wonder if it makes more sense to add a new interface to tell the index_differs_from() function "I want you to use shift-ita semantics" bit, and pass that when calling it from builtin/commit.c while not toggling that bit on when the other two callers call it, without introducing the has_ita_entries() helper function.

By the way, I think "shift" is a bit unclear name for the diffopt field. The log message of [1/3] is totally unclear (it claims "smaller and safer" without explaining what it exactly does and why that is safer); the documentation update in it is slightly better in that it lets intelligent readers to guess that the option is to declare that ita entries do not yet exist in the index (hence, "git diff" would say "that's a new file", while "git diff --cached" says nothing about it). From that observation, I think a descriptive phrase that is suitable for its name than "shift" needs to be found in a short explanation of what it does: "treat ita as missing in the index", e.g. "rev.diffopt.ita_is_missing = 1", perhaps?

Other than these small implementation details, I think I like the direction these two patches are taking us (I haven't checked 3/3 yet).

Thanks.