Nguyễn Thái Ngọc Duy  <> writes:

> 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 <>
> ---

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


Reply via email to