Re: diff-index --cc no longer permitted, gitk is now broken (slightly)

From: Sergey Organov <sorganov@gmail.com>
Date: Sep 8, 2021, 13:43 UTC
Message-ID: <87pmtjkwsj.fsf@osv.gnss.ru>
In-Reply-To: <xmqqy288b64q.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 9 quoted lines

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Here is a patch that fixes diff-index to accept --cc again:
>
> Sorry for the delay; I did not notice there was a patch buried in a
> discussion thread.
>
> We might later need to do this suppression in more codepaths if we
> find more regressions, but let's have one fix at a time.

I'm pretty positive there should be nothing left. This commit was diff-index specific, and doesn't affect anything else. Nowhere in entire series the semantics of --cc itself has been changed, it has been only disabled as particular option in diff-index command-line parsing. Overall, this is pretty local change.

Show 10 quoted lines

>
> Will queue.
>
>>  builtin/diff-index.c |  6 +++---
>>  diff-merges.c        | 14 ++++----------
>>  diff-merges.h        |  2 +-
>
> This would deserve new tests that cover the existing use cases,
> given that both of us (and other reviewers in the original thread)
> did not notice how big a regression we are causing.

I don't see it as a "big regression", but no wonder the breakage was entirely unexpected, see below.

>
> We care about --cc naturally falling back to -p when there is only
> one other thing to compare with, and also we care about --cc that
> allows us to compare during conflict resolution, at least, I think.

The problem here is not with -c/--cc itself, it is rather with diff-index. It's neither documented nor tested nor obvious what -c/--cc should mean in diff-index, given -c/--cc description (e.g., in "git help log"):

       -c
           With this option, diff output for a merge commit shows the
           differences [...]

How an option to deal with merge commits is applicable to diff-index, that:

    git-diff-index - Compare a tree to the working tree or index

???

Besides, nobody yet told us why gitk uses --cc option in invocation of 'diff-index' in the first place. Does it actually *rely* on particular undocumented behavior of "diff-index --cc", or is it just a copy-paste *leftover*?

Overall, the original commit had a mistake, as the commit that was meant to be pure refactoring had changed observable behavior, even if undocumented. However, the essence of the commit, disabling of "diff for merge /commits/" options in diff-index that does not deal with /commits/, could still be the right thing to do long term.

Thanks, -- Sergey Organov