Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

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

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

Show 11 quoted lines

> The original commit d95d728aba06a34394d15466045cbdabdada58a2 was
> reverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we
> were (and still are) not ready for a new world order. A lot more
> investigation must be done to see what is impacted. See the 78cc1a5 for
> details.
>
> This patch takes a smaller and safer step. The new behavior is
> controlled by shift_ita flag. We can gradually move more diff users to
> the new behavior after we are sure it's safe to do so. This flag is
> exposed to outside temporarily as "--shift-ita" for people who prefer
> "git diff [--cached] --stat" to "git status"

Let's stop advertising this as a resurrection of something else. The original that was unconditional was simply broken.

It is very good to refer to it (and its reversion), when justifying why this version takes the particular approach to introduce a new optional behaviour that can be toggled on selectively, by explaining why doing this unconditionally was a broken idea that needed to be reverted later.

But you would need to explain what problem this patch attempts to solve and how before even going into that. The above two paragraphs are backwards.

As I already said, --shift-ita is not quite descriptive and I think it should be renamed to something else, but I kept that in the following attempt to rewrite:

    Subject: diff-lib: allow ita entries treated as "not yet exist in index"

    When comparing the index and the working tree to show which
    paths are new, and comparing the tree recorded in the HEAD and
    the index to see if committing the contents recorded in the
    index would result in an empty commit, we would want the former
    comparison to say "these are new paths" and the latter to say
    "there is no change" for paths that are marked as intent-to-add.

    We made a similar attempt at d95d728a ("diff-lib.c: adjust
    position of i-t-a entries in diff", 2015-03-16), which redefined
    the semantics of these two comparison modes globally, which was
    a disastor and had to be reverted at 78cc1a54 ("Revert
    "diff-lib.c: adjust position of i-t-a entries in diff"",
    2015-06-23).  To make sure we do not repeat the same mistake,
    introduce a new internal diffopt option so that this different
    semantics can be asked for only by callers that ask it, while
    making sure other unaudited callers will get the same comparison
    result.  This internal option is also exposed temporarily as
    "--shift-ita" to help experiment.

After reading the three patches through, however, I do not think we use the command line option anywhere. I'm inclined to say that we shouldn't add it at all. Or at least do so in a separate follow-up patch "now we have an internal mechanism, let's expose it anyway" at the end. Which means that the last sentence in my attempted rewrite should go.

The patch to diff-lib.c machinery looks good.

Thanks.