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

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

[Johannes Sixt]

Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This
breaks gitk: it invokes

   git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD

to show the staged changes (when the line "Local changes checked in to
index but not committed" is selected).

The man page of git diff-index does not mention --cc as an option. I
haven't fully grokked the meaning of --cc, so I cannot tell whether this
absence has any significance (is deliberate or an omission).

Is gitk wrong to add --cc unconditionally? Should it do so only when
there are conflicts? Or not at all?

-- Hannes

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

[Sergey Organov]

Johannes Sixt <j6t@kdbg.org> writes:

> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
> to diff-index, 2021-05-21) git diff-index no longer accepts --cc.

Yep, this is expected, and I even put corresponding comment in the
source:

	/*
	 * We need no diff for merges options, and we need to avoid conflict
	 * with our own meaning of "-m".
	 */


> This breaks gitk: it invokes
>
>    git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD
>
> to show the staged changes (when the line "Local changes checked in to
> index but not committed" is selected).
>
> The man page of git diff-index does not mention --cc as an option. I
> haven't fully grokked the meaning of --cc, so I cannot tell whether this
> absence has any significance (is deliberate or an omission).
>
> Is gitk wrong to add --cc unconditionally? Should it do so only when
> there are conflicts? Or not at all?

As far as I can tell, --cc had no effect on diff-index, it was just
silently consumed. If I'm right, this line in gitk never needed --cc.
Then either gitk is to be fixed, or we can "fix" diff-index to silently
consume --cc/-c again, for backward compatibility.

If --cc did affect diff-index, then my commit in question is wrong and
should be fixed.

Thanks,
-- Sergey Organov

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

[Jeff King]

On Mon, Aug 30, 2021 at 04:05:50PM +0300, Sergey Organov wrote:

> > Is gitk wrong to add --cc unconditionally? Should it do so only when
> > there are conflicts? Or not at all?
> 
> As far as I can tell, --cc had no effect on diff-index, it was just
> silently consumed. If I'm right, this line in gitk never needed --cc.
> Then either gitk is to be fixed, or we can "fix" diff-index to silently
> consume --cc/-c again, for backward compatibility.

I think it does have an effect. Try this to generate a simple merge
conflict:

  git init repo
  cd repo

  echo base >file
  git add file
  git commit -m base

  echo main >file
  git commit -am main

  git checkout -b side HEAD^
  echo side >file
  git commit -am side

  git merge main ;# maybe master here depending on your config

And then with pre-v2.33 Git, --cc does a combined diff:

  $ git.v2.32.0 diff-index -p  HEAD
  diff --git a/file b/file
  index 2299c37..81768df 100644
  --- a/file
  +++ b/file
  @@ -1 +1,5 @@
  +<<<<<<< HEAD
   side
  +=======
  +main
  +>>>>>>> main

  $ git.v2.32.0 diff-index --cc HEAD
  diff --cc file
  index df967b9,2299c37..0000000
  --- a/file
  +++ b/file
  @@@ -1,1 -1,1 +1,5 @@@
  - base
  ++<<<<<<< HEAD
  + side
  ++=======
  ++main
  ++>>>>>>> main

Likewise, --raw will show a combined diff, though it's a bit less useful
because the unmerged entry has a null sha1:

  $ git.v2.32.0 diff-index HEAD
  :100644 100644 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 M	file

  $ git.v2.32.0 diff-index --cc --raw HEAD
  ::100644 100644 100644 df967b96a579e45a18b8251732d16804b2e56a55 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 MM	file

-Peff

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

[Sergey Organov]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 30, 2021 at 04:05:50PM +0300, Sergey Organov wrote:
>
>> > Is gitk wrong to add --cc unconditionally? Should it do so only when
>> > there are conflicts? Or not at all?
>> 
>> As far as I can tell, --cc had no effect on diff-index, it was just
>> silently consumed. If I'm right, this line in gitk never needed --cc.
>> Then either gitk is to be fixed, or we can "fix" diff-index to silently
>> consume --cc/-c again, for backward compatibility.
>
> I think it does have an effect. Try this to generate a simple merge
> conflict:
>
>   git init repo
>   cd repo
>
>   echo base >file
>   git add file
>   git commit -m base
>
>   echo main >file
>   git commit -am main
>
>   git checkout -b side HEAD^
>   echo side >file
>   git commit -am side
>
>   git merge main ;# maybe master here depending on your config
>
> And then with pre-v2.33 Git, --cc does a combined diff:
>
>   $ git.v2.32.0 diff-index -p  HEAD
>   diff --git a/file b/file
>   index 2299c37..81768df 100644
>   --- a/file
>   +++ b/file
>   @@ -1 +1,5 @@
>   +<<<<<<< HEAD
>    side
>   +=======
>   +main
>   +>>>>>>> main
>
>   $ git.v2.32.0 diff-index --cc HEAD
>   diff --cc file
>   index df967b9,2299c37..0000000
>   --- a/file
>   +++ b/file
>   @@@ -1,1 -1,1 +1,5 @@@
>   - base
>   ++<<<<<<< HEAD
>   + side
>   ++=======
>   ++main
>   ++>>>>>>> main
>
> Likewise, --raw will show a combined diff, though it's a bit less useful
> because the unmerged entry has a null sha1:
>
>   $ git.v2.32.0 diff-index HEAD
>   :100644 100644 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 M	file
>
>   $ git.v2.32.0 diff-index --cc --raw HEAD
>   ::100644 100644 100644 df967b96a579e45a18b8251732d16804b2e56a55 2299c37978265a95cbe835a4b0f0bbf15aad5549 0000000000000000000000000000000000000000 MM	file
>
> -Peff

Well, thanks! As it does make a difference, it's a pity it's neither
documented nor being tested.

I need to review the issue more closely to give a fix then.

Thanks,
-- Sergey Organov

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

[Johannes Sixt]

Am 30.08.21 um 15:05 schrieb Sergey Organov:
> As far as I can tell, --cc had no effect on diff-index, it was just
> silently consumed. If I'm right, this line in gitk never needed --cc.
> Then either gitk is to be fixed, or we can "fix" diff-index to silently
> consume --cc/-c again, for backward compatibility.

That latter would be much preferable. Gitk uses the combination -p --cc
for *all* diff commands when it requests patch output regardless of the
number of parents. It would be tedious to special-case diff-index to not
pass --cc.

-- Hannes

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

[Sergey Organov]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 30.08.21 um 15:05 schrieb Sergey Organov:
>> As far as I can tell, --cc had no effect on diff-index, it was just
>> silently consumed. If I'm right, this line in gitk never needed --cc.
>> Then either gitk is to be fixed, or we can "fix" diff-index to silently
>> consume --cc/-c again, for backward compatibility.
>
> That latter would be much preferable. Gitk uses the combination -p --cc
> for *all* diff commands when it requests patch output regardless of the
> number of parents. It would be tedious to special-case diff-index to not
> pass --cc.

It's already noticed by Jeff that --cc was not in fact a no-op, so
diff-index is to be fixed. Sorry for the breakage.

Thanks,
-- Sergey Organov

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This
> breaks gitk: it invokes
>
>    git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD
>
> to show the staged changes (when the line "Local changes checked in to
> index but not committed" is selected).
>
> The man page of git diff-index does not mention --cc as an option. I
> haven't fully grokked the meaning of --cc, so I cannot tell whether this
> absence has any significance (is deliberate or an omission).
>
> Is gitk wrong to add --cc unconditionally? Should it do so only when
> there are conflicts? Or not at all?

I think --cc is designed to naturally fall back to -p when there is
only one parent.  Use of both -p and --cc has also long been an
acceptable combination, and even if we say the later --cc overrides
-p, there is no reason not to show single parent patch here with
--cc.

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

[Sergey Organov]

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

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
>> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This
>> breaks gitk: it invokes
>>
>>    git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD
>>
>> to show the staged changes (when the line "Local changes checked in to
>> index but not committed" is selected).
>>
>> The man page of git diff-index does not mention --cc as an option. I
>> haven't fully grokked the meaning of --cc, so I cannot tell whether this
>> absence has any significance (is deliberate or an omission).
>>
>> Is gitk wrong to add --cc unconditionally? Should it do so only when
>> there are conflicts? Or not at all?
>
> I think --cc is designed to naturally fall back to -p when there is
> only one parent.  Use of both -p and --cc has also long been an
> acceptable combination, and even if we say the later --cc overrides
> -p, there is no reason not to show single parent patch here with
> --cc.

I'm pretty sure I've checked diff-index doesn't use the flag that --cc
sets when I wrote the patch, so the only incompatibility this patch
introduced is denying the command when --cc is given, i.e., it now
behaves as if diff-index doesn't support --cc *option*, that makes sense
to me and matches diff-index documentation.

Irrespective to chosen solution, it still looks to me like gitk
shouldn't had --cc in that command in the first place. I that correct,
or do I miss something essential?

That said, if you think that diff-index should silently accept --cc (and
-c ?), for whatever reason, it's fine with me, provided it's properly
documented and there are proper test-cases in place.

Thanks,
-- Sergey Organov

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

[Junio C Hamano]

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

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
>> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This
>> breaks gitk: it invokes
>>
>>    git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD
>>
>> to show the staged changes (when the line "Local changes checked in to
>> index but not committed" is selected).
>>
>> The man page of git diff-index does not mention --cc as an option. I
>> haven't fully grokked the meaning of --cc, so I cannot tell whether this
>> absence has any significance (is deliberate or an omission).
>>
>> Is gitk wrong to add --cc unconditionally? Should it do so only when
>> there are conflicts? Or not at all?
>
> I think --cc is designed to naturally fall back to -p when there is
> only one parent.  Use of both -p and --cc has also long been an
> acceptable combination, and even if we say the later --cc overrides
> -p, there is no reason not to show single parent patch here with
> --cc.

Another tangent.

I think the use of --cc with diff-index can make sense in another
way.

    $ echo "# both" >>COPYING
    $ git add COPYING
    $ echo "# work" >>COPYING

Now we have one extra line at the end in both the index and the
working tree file, with yet another at the end of the latter.

    $ git diff-index --cc HEAD

is a way to show combined diff to go to the working tree version
starting from HEAD and starting from the index (I needed to use an
old version because the 'maint' and upwards are broken as reported).

    $ rungit v1.5.3 diff-index --cc HEAD
    diff --cc COPYING
    index 8b9c100,536e555..0000000
    --- a/COPYING
    +++ b/COPYING
    @@@ -358,4 -358,3 +358,5 @@@ proprietary programs.  If your program 
      consider it more useful to permit linking proprietary applications with the
      library.  If this is what you want to do, use the GNU Lesser General
      Public License instead of this License.
     +# both
    ++# work

Now the way "gitk" used is with "--cached", so there is no multi-way
comparisons to be combined, and it is natural to fall back to "-p",
so it is a different issue, but since we invented "--cc" to
originally emulate, and to later improve, the output from gitk,
I am reasonably sure that its use of "--cc" should be supported.

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

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
>>> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This
>>> breaks gitk: it invokes
>>>
>>>    git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD
>>>
>>> to show the staged changes (when the line "Local changes checked in to
>>> index but not committed" is selected).
>>>
>>> The man page of git diff-index does not mention --cc as an option. I
>>> haven't fully grokked the meaning of --cc, so I cannot tell whether this
>>> absence has any significance (is deliberate or an omission).
>>>
>>> Is gitk wrong to add --cc unconditionally? Should it do so only when
>>> there are conflicts? Or not at all?
>>
>> I think --cc is designed to naturally fall back to -p when there is
>> only one parent.  Use of both -p and --cc has also long been an
>> acceptable combination, and even if we say the later --cc overrides
>> -p, there is no reason not to show single parent patch here with
>> --cc.
>
> Another tangent.
>
> I think the use of --cc with diff-index can make sense in another
> way.
>
>     $ echo "# both" >>COPYING
>     $ git add COPYING
>     $ echo "# work" >>COPYING
>
> Now we have one extra line at the end in both the index and the
> working tree file, with yet another at the end of the latter.
>
>     $ git diff-index --cc HEAD
>
> is a way to show combined diff to go to the working tree version
> starting from HEAD and starting from the index (I needed to use an
> old version because the 'maint' and upwards are broken as reported).
>
>     $ rungit v1.5.3 diff-index --cc HEAD
>     diff --cc COPYING
>     index 8b9c100,536e555..0000000
>     --- a/COPYING
>     +++ b/COPYING
>     @@@ -358,4 -358,3 +358,5 @@@ proprietary programs.  If your program 
>       consider it more useful to permit linking proprietary applications with the
>       library.  If this is what you want to do, use the GNU Lesser General
>       Public License instead of this License.
>      +# both
>     ++# work
>
> Now the way "gitk" used is with "--cached", so there is no multi-way
> comparisons to be combined, and it is natural to fall back to "-p",
> so it is a different issue, but since we invented "--cc" to
> originally emulate, and to later improve, the output from gitk,
> I am reasonably sure that its use of "--cc" should be supported.

If the patch breaks essential (even if undocumented and untested)
behavior, as Jeff pointed, it should obviously be fixed. I'll look at it
more closely to suggest a fix.

Thanks,
-- Sergey Organov

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

[Sergey Organov]

Johannes Sixt <j6t@kdbg.org> writes:

> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This
> breaks gitk: it invokes
>
>    git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD
>
> to show the staged changes (when the line "Local changes checked in to
> index but not committed" is selected).
>
> The man page of git diff-index does not mention --cc as an option. I
> haven't fully grokked the meaning of --cc, so I cannot tell whether this
> absence has any significance (is deliberate or an omission).
>
> Is gitk wrong to add --cc unconditionally? Should it do so only when
> there are conflicts? Or not at all?
>

Here is a patch that fixes diff-index to accept --cc again:

Subject: [PATCH] diff-index: restore -c/--cc options handling

This fixes git:19b2517f95a0a908a8ada7417cf0717299e7e1aa
"diff-merges: move specific diff-index "-m" handling to diff-index"

That commit disabled handling of all diff for merges options in
diff-index on an assumption that they are unused. However, it later
appeared that -c and --cc, even though undocumented and not being
covered by tests, happen to have had particular effect on diff-index
output.

Restore original -c/--cc options handling by diff-index.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 builtin/diff-index.c |  6 +++---
 diff-merges.c        | 14 ++++----------
 diff-merges.h        |  2 +-
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index cf09559e422d..5fd23ab5b6c5 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -29,10 +29,10 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	prefix = precompose_argv_prefix(argc, argv, prefix);
 
 	/*
-	 * We need no diff for merges options, and we need to avoid conflict
-	 * with our own meaning of "-m".
+	 * We need (some of) diff for merges options (e.g., --cc), and we need
+	 * to avoid conflict with our own meaning of "-m".
 	 */
-	diff_merges_suppress_options_parsing();
+	diff_merges_suppress_m_parsing();
 
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	for (i = 1; i < argc; i++) {
diff --git a/diff-merges.c b/diff-merges.c
index d897fd8a2933..5060ccd890bd 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -6,7 +6,7 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *);
 static void set_separate(struct rev_info *revs);
 
 static diff_merges_setup_func_t set_to_default = set_separate;
-static int suppress_parsing;
+static int suppress_m_parsing;
 
 static void suppress(struct rev_info *revs)
 {
@@ -91,9 +91,9 @@ int diff_merges_config(const char *value)
 	return 0;
 }
 
-void diff_merges_suppress_options_parsing(void)
+void diff_merges_suppress_m_parsing(void)
 {
-	suppress_parsing = 1;
+	suppress_m_parsing = 1;
 }
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
@@ -102,10 +102,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	const char *optarg;
 	const char *arg = argv[0];
 
-	if (suppress_parsing)
-		return 0;
-
-	if (!strcmp(arg, "-m")) {
+	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
@@ -153,9 +150,6 @@ void diff_merges_set_dense_combined_if_unset(struct rev_info *revs)
 
 void diff_merges_setup_revs(struct rev_info *revs)
 {
-	if (suppress_parsing)
-		return;
-
 	if (revs->combine_merges == 0)
 		revs->dense_combined_merges = 0;
 	if (revs->separate_merges == 0)
diff --git a/diff-merges.h b/diff-merges.h
index b5d57f6563e3..19639689bb05 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -11,7 +11,7 @@ struct rev_info;
 
 int diff_merges_config(const char *value);
 
-void diff_merges_suppress_options_parsing(void);
+void diff_merges_suppress_m_parsing(void);
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
-- 
2.33.0.114.g9123bcff51bf

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

[Junio C Hamano]

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.  

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.

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.

It can and should come as a separate step, of course.  Unbreaking
gitk for an already known breakage would be more urgent than hunting
for other breakages, even though the latter might result in a more
thorough fix in the end.

Thanks.

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

[Sergey Organov]

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

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

Sorry from my side as well. I was already about to re-submit the patch
properly, but you happened to be faster with this )

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

Yep, it's too easy to break undocumented and untested behavior.

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

I'm all for more tests, but I'm afraid I'm not in a good position to
write them, especially for an undocumented behavior. I think somebody
should first document what --cc/-c does in diff-index, and only then
it makes sense to write some tests.

> It can and should come as a separate step, of course.  Unbreaking
> gitk for an already known breakage would be more urgent than hunting
> for other breakages, even though the latter might result in a more
> thorough fix in the end.

Makes sense.

Thanks,
-- Sergey Organov

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

[Sergey Organov]

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

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

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

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

[Johannes Sixt]

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

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

[Sergey Organov]

Johannes Sixt <j6t@kdbg.org> writes:

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

I see, but the problem here is that while diff-files and diff-tree both
accept --cc according to their documentation, diff-index does not. This
means that, strictly speaking, gitk makes a mistake treating all 3
commands universally with respect to command-line arguments when it uses
--cc.

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

The problem is not in the kind of commit, the problem is in the command
being invoked. diff-index doesn't support --cc according to its
documentation, and thus gitk relies on undocumented behavior of
diff-index. It might well be the case that it just happened to be
"working", thus nobody cared.

> Therefore, silent fall-back from --cc to -p in case of non-merge
> commits or non-conflicted index is absolutely necessary.

I didn't change semantics of --cc, so this thing was not broken at all.
I just disabled the --cc option in diff-index command, to match the
documentation.

As a side note, in fact Git does no "silent fall-back from --cc to -p in
case of non-merge commits", even though the behavior could be indeed
seen like this. Instead, --cc implies -p, and, as --cc does not
otherwise affect treating of non-merge commits, only -p is left active
for them. Once again, this has not been recently changed, so does not
need to be fixed.

Thanks,
-- Sergey Organov

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> 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.

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

[Sergey Organov]

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

> Johannes Sixt <j6t@kdbg.org> 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.

Except both Git documentation and implementation are in some
contradiction with the explanations above, as far as I can see, so this
looks to me like a kind of wishful thinking, sorry.

[One can read, say, year old Git documentation, prior to recent patches
(to get rid of possible bias), to see that it describes different
picture and explicitly contradicts some of the statements above, and
then I definitely did no steps to move in the direction described above
either, at least intentionally.]

Anyway, here is the current reality the way I see it (this description
assumes "-m imply -p" patch by me has been reverted due to introduction
of slight backward incompatibility):

-p enables diff for non-merge commits only
--diff-merges=XXXX enables diff for merge commits only

--cc => -p --diff-merges=cc
-c   => -p --diff-merges=c
-m   =>    --diff-merges=m

where "=>" means "is shortcut for".

Simple and clear, except -m is an outlier, as it does not imply -p.
Further, -m produces inconvenient output unless --first-parent is also
in use, and then --first-parent already implies -m, rendering explicit
-m virtually useless.

Due to this outlier, all the recent changes in this area were targeted
to make -m useful, similar to --cc/-c, not to change --cc/-c, or -p
semantics at all, so that finally we get perfect:

--cc => -p --diff-merges=cc
-c   => -p --diff-merges=c
-m   => -p --diff-merges=m

Please also notice that with this system there is no need for every
(new) way of representing of merge commits to support "fall back to -p
in case of non-merge commits" at options level, as it does not have to
deal with non-merge commits at all.

Thanks,
-- Sergey Organov

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

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.

I'm afraid this issue is left up in the air after application of the
fix-up patch, as usage of --cc in the diff-index is still undocumented.
I.e., the fix-up just restores the historical status quo that has a
problem by itself.

As current documentation of --cc elsewhere does not seem to be even
suitable for diff-index, I don't feel like providing corresponding patch
for the documentation, even less so as I still have no idea if current
behavior is intended or accidental, and if it's the latter, do we need
to keep or somehow fix it? Anybody?

Thanks,
-- Sergey Organov

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

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> I'm afraid this issue is left up in the air after application of the
> fix-up patch, as usage of --cc in the diff-index is still undocumented.

Yeah, I do think documentation update is needed, but being buried by
other topics I haven't had a chance to revisit the way how --cc is
described in the wider context in order to make an intellgent
suggestion on how to present it in the context of "diff-index".

> I.e., the fix-up just restores the historical status quo that has a
> problem by itself.

I do agree "show -p" on merge is an oddball that trips new people,
because it does not imply the "do present the changes for merges"
bit unlike "show -c/--cc" do, and from that point of view, the
generalization --diff-merges tried to bring us was a good thing.

But "-c/--cc" are explicit enough in what they want to do.  It does
want to present the changes to compare a single end state with
possibly more than one starting state (e.g. a merge) and not
requiring an explicit "-m" is quite natural.  Even more, when it
compares the end state with only one starting state (e.g. showing a
single parent commit), there is only one pairwise result to
"combine", so it is also natural that it ends up showing the same
output as "-p".  So I do not quite see the behaviour of "diff*/show
--cc" as a problem, though.  IOW, the use pattern in gitk is more
than just "historical status quo", but is quite sensible, I would
have to say.

Thanks.

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

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I'm afraid this issue is left up in the air after application of the
>> fix-up patch, as usage of --cc in the diff-index is still undocumented.
>
> Yeah, I do think documentation update is needed, but being buried by
> other topics I haven't had a chance to revisit the way how --cc is
> described in the wider context in order to make an intellgent
> suggestion on how to present it in the context of "diff-index".

I tend to believe yet another description of --cc is needed for
diff-index, separate from the current one. Just saying.

>
>> I.e., the fix-up just restores the historical status quo that has a
>> problem by itself.
>
> I do agree "show -p" on merge is an oddball that trips new people,
> because it does not imply the "do present the changes for merges"
> bit unlike "show -c/--cc" do, and from that point of view, the
> generalization --diff-merges tried to bring us was a good thing.

I'm not sure I follow. What "show -p" has to do with "diff-index --cc"?

My only point here is that usage of *--cc* in *diff-index* is entirely
undocumneted, and that needs to be somehow resolved.

>
> But "-c/--cc" are explicit enough in what they want to do.  It does
> want to present the changes to compare a single end state with
> possibly more than one starting state (e.g. a merge) and not
> requiring an explicit "-m" is quite natural.

Doesn't seem to be relevant for "diff-index --cc" lacking documentation,
but -m and -c and --cc are rather *mutually exclusive*. I.e., they all
set different formats for output of diffs for merges, so "--cc -m" ==
"-m", and "-m --cc" == "--cc", i.e., the latest overrides the format to
be used. Therefore "requiring explicit -m for -cc" simply doesn't make
sense.

> Even more, when it compares the end state with only one starting state
> (e.g. showing a single parent commit), there is only one pairwise
> result to "combine", so it is also natural that it ends up showing the
> same output as "-p". So I do not quite see the behaviour of
> "diff*/show --cc" as a problem, though.

I don't see it as a problem as well, so whom you are arguing with?

The only problem in this particular case I see is that "diff-index --cc"
is undocumented (and untested), and this has nothing to do with
log/diff/show, unless I miss your point.

> IOW, the use pattern in gitk is more than just "historical status
> quo", but is quite sensible, I would have to say.

"diff-index --cc" in gitk is a bug, as according to Git documentation
"diff-index" does not accept "--cc", period.

gitk trying to make sense of what is neither documented, nor tested, nor
guaranteed is the problem, but I was talking even not about that
problem, but rather about the cause of this: some undocumented
processing of "git diff-index --cc".

Thanks,
-- Sergey Organov

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

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"?
>
> My only point here is that usage of *--cc* in *diff-index* is entirely
> undocumneted, and that needs to be somehow resolved.

It was a response to your "historical status quo that is a problem."
I do not think there is any problem with "diff-index --cc" (except
for it wants a better documentation---but that we already agree) but
I wanted to give you some credit for having worked on "--diff-merges",
an effort to generalize things in a related area.

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

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"?
>>
>> My only point here is that usage of *--cc* in *diff-index* is entirely
>> undocumneted, and that needs to be somehow resolved.
>
> It was a response to your "historical status quo that is a problem."
> I do not think there is any problem with "diff-index --cc" (except
> for it wants a better documentation---but that we already agree) but

Ah, now I see, but it's exactly lack of documentation (and tests) that I
was referring to as the "problem of the historical status quo" on the
Git side, so I was somewhat confused by your original response.

Also, it's still unclear, even if not very essential, what exactly that
"status quo" is when seen from the point of view of gitk. Does gitk
actually utilize *particular output* of "diff-index --cc" for better, or
gitk would be just as happy if it were synonym for "diff-index -p", or
even if it'd be just as happy if --cc were silently consumed by
diff-index?

> I wanted to give you some credit for having worked on "--diff-merges",
> an effort to generalize things in a related area.

Thanks for that! More to follow )

Thanks,
-- Sergey Organov

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

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Ah, now I see, but it's exactly lack of documentation (and tests) that I
> was referring to as the "problem of the historical status quo" on the
> Git side, so I was somewhat confused by your original response.

Well, you said the fixup "restores" the status quo, but in fact,
with or without the fixup, before or after it, the lack of
documentation was there.  So I thought you were talking about
something else.

>> I wanted to give you some credit for having worked on "--diff-merges",
>> an effort to generalize things in a related area.
>
> Thanks for that! More to follow )

I somehow expect there was need for no further work in this area,
but there are also many other areas in Git where your talent is
applicable and appreciated, I am sure ;-)

Thanks.

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

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Ah, now I see, but it's exactly lack of documentation (and tests) that I
>> was referring to as the "problem of the historical status quo" on the
>> Git side, so I was somewhat confused by your original response.
>
> Well, you said the fixup "restores" the status quo, but in fact,
> with or without the fixup, before or after it, the lack of
> documentation was there.

No, what problematic patch did, it changed behavior of diff-index
exactly in accordance with its *current* documentation that doesn't
mention --cc as accepted command-line option for diff-index. So, with
that patch applied, there were no this problem with documentation
anymore. Implementation now actually matched the docs.

Unfortunately, that brought worse problem: it unexpectedly broke gitk,
that, as it appeared, depends on undocumented diff-index behavior.

So, I re-enabled --cc in diff-index, lesser of two evils, that brought
back the problem of lack of documentation and test cases for "diff-index
--cc". This way, the status quo has been restored indeed.

> So I thought you were talking about something else.
>
>>> I wanted to give you some credit for having worked on "--diff-merges",
>>> an effort to generalize things in a related area.
>>
>> Thanks for that! More to follow )
>
> I somehow expect there was need for no further work in this area,
> but there are also many other areas in Git where your talent is
> applicable and appreciated, I am sure ;-)

I'm afraid we still didn't reach one of the ultimate goals of all this:
letting -m be useful again, specifically, as suitable *user* option.

Also, current --diff-merges options are incapable of providing current
-m behavior, as has been noticed by Jonathan Nieder in another thread on
reverting "-m implies -p" commit:

  "When I try it locally, -m shows no diff by default,
   whereas --diff-merges=separate shows a diff for merges."

and I'm going to fix this by adding yet another feature for
--diff-merges. This is to be pure addition, thus causing no backward
compatibility problems.

Thanks,
-- Sergey Organov

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

[Philip Oakley]

On 17/09/2021 08:08, Sergey Organov wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"?
>>>
>>> My only point here is that usage of *--cc* in *diff-index* is entirely
>>> undocumneted, and that needs to be somehow resolved.
>> It was a response to your "historical status quo that is a problem."
>> I do not think there is any problem with "diff-index --cc" (except
>> for it wants a better documentation---but that we already agree) but
> Ah, now I see, but it's exactly lack of documentation (and tests) that I
> was referring to as the "problem of the historical status quo" on the
> Git side, so I was somewhat confused by your original response.
>
> Also, it's still unclear, even if not very essential, what exactly that
> "status quo" is when seen from the point of view of gitk. Does gitk
> actually utilize *particular output* of "diff-index --cc" for better, or
> gitk would be just as happy if it were synonym for "diff-index -p", or
> even if it'd be just as happy if --cc were silently consumed by
> diff-index?

Did Johannes Sixt's earlier answer
https://lore.kernel.org/git/cbd0d173-ef17-576b-ab7a-465d42c82265@kdbg.org/
help clarify the choices?
>> I wanted to give you some credit for having worked on "--diff-merges",
>> an effort to generalize things in a related area.
> Thanks for that! More to follow )
>
> Thanks,
> -- Sergey Organov

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

[Sergey Organov]

Philip Oakley <philipoakley@iee.email> writes:

> On 17/09/2021 08:08, Sergey Organov wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergey Organov <sorganov@gmail.com> writes:
>>>
>>>> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"?
>>>>
>>>> My only point here is that usage of *--cc* in *diff-index* is entirely
>>>> undocumneted, and that needs to be somehow resolved.
>>> It was a response to your "historical status quo that is a problem."
>>> I do not think there is any problem with "diff-index --cc" (except
>>> for it wants a better documentation---but that we already agree) but
>> Ah, now I see, but it's exactly lack of documentation (and tests) that I
>> was referring to as the "problem of the historical status quo" on the
>> Git side, so I was somewhat confused by your original response.
>>
>> Also, it's still unclear, even if not very essential, what exactly that
>> "status quo" is when seen from the point of view of gitk. Does gitk
>> actually utilize *particular output* of "diff-index --cc" for better, or
>> gitk would be just as happy if it were synonym for "diff-index -p", or
>> even if it'd be just as happy if --cc were silently consumed by
>> diff-index?
>
> Did Johannes Sixt's earlier answer
> https://lore.kernel.org/git/cbd0d173-ef17-576b-ab7a-465d42c82265@kdbg.org/
> help clarify the choices?

Sorry, no. I did read that carefully when it has been posted. Further
explanations by Johannes also only tell that gitk expects --cc to be
accepted by diff-index as it likes to treat multiple commands
universally, but don't specify what output git expects from --cc when it
passes it exactly to diff-index. Maybe it just shows the output and have
no other expectations, dunno.

"silent fall-back from --cc to -p in case of non-merge commits or
non-conflicted index is absolutely necessary" that is stated there is a
non-issue as it was always the case, and there are no questions about
it, as --cc in fact implies -p, and the latter is applied to non-merge
commits. So, how Git treats "non-conflicted index" and non-merge commits
does not depend on --cc. It's how it treats "conflicting index" that is
still unspecified.

Thanks,
-- Sergey Organov

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

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

> Philip Oakley <philipoakley@iee.email> writes:
>
>> On 17/09/2021 08:08, Sergey Organov wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Sergey Organov <sorganov@gmail.com> writes:
>>>>
>>>>> I'm not sure I follow. What "show -p" has to do with "diff-index --cc"?
>>>>>
>>>>> My only point here is that usage of *--cc* in *diff-index* is entirely
>>>>> undocumneted, and that needs to be somehow resolved.
>>>> It was a response to your "historical status quo that is a problem."
>>>> I do not think there is any problem with "diff-index --cc" (except
>>>> for it wants a better documentation---but that we already agree) but
>>> Ah, now I see, but it's exactly lack of documentation (and tests) that I
>>> was referring to as the "problem of the historical status quo" on the
>>> Git side, so I was somewhat confused by your original response.
>>>
>>> Also, it's still unclear, even if not very essential, what exactly that
>>> "status quo" is when seen from the point of view of gitk. Does gitk
>>> actually utilize *particular output* of "diff-index --cc" for better, or
>>> gitk would be just as happy if it were synonym for "diff-index -p", or
>>> even if it'd be just as happy if --cc were silently consumed by
>>> diff-index?
>>
>> Did Johannes Sixt's earlier answer
>> https://lore.kernel.org/git/cbd0d173-ef17-576b-ab7a-465d42c82265@kdbg.org/
>> help clarify the choices?
>
> Sorry, no. I did read that carefully when it has been posted. Further
> explanations by Johannes also only tell that gitk expects --cc to be
> accepted by diff-index as it likes to treat multiple commands
> universally, but don't specify what output git expects from --cc when it
> passes it exactly to diff-index. Maybe it just shows the output and have
> no other expectations, dunno.

And, even more importantly, if gitk uses "git diff-index --cc" for its
specific output, is there another, documented way to achieve the same
goal?

Thanks,
-- Sergey Organov

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

[Johannes Sixt]

Am 01.09.21 um 18:52 schrieb Sergey Organov:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Since 19b2517f95a0 (diff-merges: move specific diff-index "-m" handling
>> to diff-index, 2021-05-21) git diff-index no longer accepts --cc. This
>> breaks gitk: it invokes
>>
>>    git diff-index --cached -p -C --cc --no-commit-id -U3 HEAD
>>
>> to show the staged changes (when the line "Local changes checked in to
>> index but not committed" is selected).
>>
>> The man page of git diff-index does not mention --cc as an option. I
>> haven't fully grokked the meaning of --cc, so I cannot tell whether this
>> absence has any significance (is deliberate or an omission).
>>
>> Is gitk wrong to add --cc unconditionally? Should it do so only when
>> there are conflicts? Or not at all?
>>
> 
> Here is a patch that fixes diff-index to accept --cc again:
> 
> Subject: [PATCH] diff-index: restore -c/--cc options handling
> 
> This fixes git:19b2517f95a0a908a8ada7417cf0717299e7e1aa
> "diff-merges: move specific diff-index "-m" handling to diff-index"
> 
> That commit disabled handling of all diff for merges options in
> diff-index on an assumption that they are unused. However, it later
> appeared that -c and --cc, even though undocumented and not being
> covered by tests, happen to have had particular effect on diff-index
> output.
> 
> Restore original -c/--cc options handling by diff-index.

Thank you! I can confirm that gitk works again as expected with this patch.

-- Hannes

> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  builtin/diff-index.c |  6 +++---
>  diff-merges.c        | 14 ++++----------
>  diff-merges.h        |  2 +-
>  3 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/builtin/diff-index.c b/builtin/diff-index.c
> index cf09559e422d..5fd23ab5b6c5 100644
> --- a/builtin/diff-index.c
> +++ b/builtin/diff-index.c
> @@ -29,10 +29,10 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>  	prefix = precompose_argv_prefix(argc, argv, prefix);
>  
>  	/*
> -	 * We need no diff for merges options, and we need to avoid conflict
> -	 * with our own meaning of "-m".
> +	 * We need (some of) diff for merges options (e.g., --cc), and we need
> +	 * to avoid conflict with our own meaning of "-m".
>  	 */
> -	diff_merges_suppress_options_parsing();
> +	diff_merges_suppress_m_parsing();
>  
>  	argc = setup_revisions(argc, argv, &rev, NULL);
>  	for (i = 1; i < argc; i++) {
> diff --git a/diff-merges.c b/diff-merges.c
> index d897fd8a2933..5060ccd890bd 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -6,7 +6,7 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *);
>  static void set_separate(struct rev_info *revs);
>  
>  static diff_merges_setup_func_t set_to_default = set_separate;
> -static int suppress_parsing;
> +static int suppress_m_parsing;
>  
>  static void suppress(struct rev_info *revs)
>  {
> @@ -91,9 +91,9 @@ int diff_merges_config(const char *value)
>  	return 0;
>  }
>  
> -void diff_merges_suppress_options_parsing(void)
> +void diff_merges_suppress_m_parsing(void)
>  {
> -	suppress_parsing = 1;
> +	suppress_m_parsing = 1;
>  }
>  
>  int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
> @@ -102,10 +102,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>  	const char *optarg;
>  	const char *arg = argv[0];
>  
> -	if (suppress_parsing)
> -		return 0;
> -
> -	if (!strcmp(arg, "-m")) {
> +	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
>  		set_to_default(revs);
>  	} else if (!strcmp(arg, "-c")) {
>  		set_combined(revs);
> @@ -153,9 +150,6 @@ void diff_merges_set_dense_combined_if_unset(struct rev_info *revs)
>  
>  void diff_merges_setup_revs(struct rev_info *revs)
>  {
> -	if (suppress_parsing)
> -		return;
> -
>  	if (revs->combine_merges == 0)
>  		revs->dense_combined_merges = 0;
>  	if (revs->separate_merges == 0)
> diff --git a/diff-merges.h b/diff-merges.h
> index b5d57f6563e3..19639689bb05 100644
> --- a/diff-merges.h
> +++ b/diff-merges.h
> @@ -11,7 +11,7 @@ struct rev_info;
>  
>  int diff_merges_config(const char *value);
>  
> -void diff_merges_suppress_options_parsing(void);
> +void diff_merges_suppress_m_parsing(void);
>  
>  int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
>  
>