Junio C Hamano <gitster@xxxxxxxxx> writes:
> Sergey Organov <sorganov@xxxxxxxxx> 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.
>
> 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
