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

Junio C Hamano [off-list ref] writes:

Sergey Organov [off-list ref] writes:

quoted

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.

Will queue.

quoted

 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

Am 08.09.21 um 15:43 schrieb Sergey Organov:

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

No, it is not a left-over. The thing is,

- there is one point in the code where gitk adds options -p -C --cc (and
more) to the command line (around line 8034),

- and there is a totally different point in the code where it is decided
whether diff-index, diff-tree, or diff-files is invoked (proc diffcmd
around line 7871).

IOW, Gitk expects that these option combinations can always be passed to
all three commands.

Gitk does not want to look at a commit and then decide which incarnation
of the command it wants to use (--cc vs. -p) depending on whether it is
a merge commit or not. This decision is delegated to command that is
invoked. Therefore, silent fall-back from --cc to -p in case of
non-merge commits or non-conflicted index is absolutely necessary.

-- Hannes

Johannes Sixt [off-list ref] writes:

Gitk does not want to look at a commit and then decide which incarnation
of the command it wants to use (--cc vs. -p) depending on whether it is
a merge commit or not. This decision is delegated to command that is
invoked. Therefore, silent fall-back from --cc to -p in case of
non-merge commits or non-conflicted index is absolutely necessary.

Well explained.

"-p" in general is an instruction to show some form of textual
patch, and "--cc" and "-c" are the variants (i.e. compare with each
parent and combine the comparison results) of it that naturally
degenerates to the normal patch output when there is only one
parent.

"--cc" also flips the "m" bit, which controls if there is any tree
comparison should be made for merge commits, which matters for "log"
family of commands, so in that sense "--cc" was made to imply "-m",
but "--cc" inherently means "-p" for non-merge commits without any
need to say X implies Y.

Thanks.