patch, 3 partsmaking --first-parent imply -m

> I pulled the option name from the rev_info field name. It might be too
> broad (we are not ignoring merges during the traversal, only for the
> diff). It could be "--no-diff-merges" or something, but that would
> involve flipping the sense of the boolean (but that would just be in the
> code, not user-visible, so not that big a deal).

> On Tue, Jul 28, 2020 at 9:40 AM Jeff King <peff@peff.net> wrote:
> > I pulled the option name from the rev_info field name. It might be too
> > broad (we are not ignoring merges during the traversal, only for the
> > diff). It could be "--no-diff-merges" or something, but that would
> > involve flipping the sense of the boolean (but that would just be in the
> > code, not user-visible, so not that big a deal).
> 
> Perhaps a bit bikesheddy, but I would suggest some clarifying
> notes in the documentation.  The fact that `git log` *follows*
> merges by default is pretty obvious, but the fact that it doesn't
> *diff them at all* by default appears to be surprising to most Git
> newcomers.  (It was to me, so many years ago, and I see this all
> the time on stackoverflow.)

>   Note
>   also that by default, "git log -p" does not generate
>   diffs for merges except when using --first-parent.
>   See also the -c and --cc options and note that the
>   default for "git show" is "--cc".

> The "-m" option sets revs->ignore_merges to "0", but there's no way to
> undo it. This probably isn't something anybody overly cares about, since
> "1" is already the default, but it will serve as an escape hatch when we
> flip the default for ignore_merges to "0" in more situations.
>
> We'll also add a few extra niceties:
>
>   - initialize the value to "-1" to indicate "not set", and then resolve
>     it to the normal 0/1 bool in setup_revisions(). This lets any tweak
>     functions, as well as setup_revisions() itself, avoid clobbering the
>     user's preference (which until now they couldn't actually express).
>
>   - since we now have --ignore-merges, let's add the matching
>     --no-ignore-merges, which is just a synonym for "-m". In fact, it's
>     simpler to just document --no-ignore-merges alongside "-m", and
>     leave it implied that its opposite countermands it.
>
> The new test shows that this behaves just the same as the current
> behavior without "-m".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I pulled the option name from the rev_info field name. It might be too
> broad (we are not ignoring merges during the traversal, only for the
> diff). It could be "--no-diff-merges" or something, but that would
> involve flipping the sense of the boolean (but that would just be in the
> code, not user-visible, so not that big a deal).
>
>  Documentation/rev-list-options.txt            |  1 +
>  builtin/log.c                                 |  4 +-
>  revision.c                                    | 10 ++-
>  revision.h                                    |  2 +-
>  t/t4013-diff-various.sh                       |  1 +
>  ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++
>  6 files changed, 90 insertions(+), 6 deletions(-)
>  create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index b01b2b6773..fbd8fa0381 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -1148,6 +1148,7 @@ options may be given. See linkgit:git-diff-files[1] for more options.
>  	rename or copy detection have been requested).
>  
>  -m::
> +--no-ignore-merges::

> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 43267d6024..8f9181316f 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -297,6 +297,7 @@ log --root --patch-with-stat --summary master
>  log --root -c --patch-with-stat --summary master
>  # improved by Timo's patch
>  log --root --cc --patch-with-stat --summary master
> +log --ignore-merges -p --first-parent master

>  log -p --first-parent master
>  log -m -p --first-parent master
>  log -m -p master
> diff --git a/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
> new file mode 100644
> index 0000000000..fa0cdc8a23
> --- /dev/null
> +++ b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
> @@ -0,0 +1,78 @@
> +$ git log --ignore-merges -p --first-parent master
> +commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
> +Merge: 9a6d494 c7a2ab9
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:04:00 2006 +0000
> +
> +    Merge branch 'side' into master

> >  -m::
> > +--no-ignore-merges::
> 
> This invites a natural "does --ignore-merges exist, and if so what
> does it do?"  Why not to have "--[no-]ignore-merges" as a separate
> entry immediately after the existing "-m" instead?

> After thinking on it more, I flipped it to:
> 
>   -m::
>   --diff-merges::
>      [existing text...]
> 
> and then I don't think we need to have another block for
> --no-diff-merges.
> 
> I'll likewise add a statement that "-m" is implied by "--first-parent"
> and can be counteracted with the "--no" form, which I think should spell
> out all the implications of the series.

> On Wed, Jul 29, 2020 at 02:22:07PM -0400, Jeff King wrote:
>
>> After thinking on it more, I flipped it to:
>> 
>>   -m::
>>   --diff-merges::
>>      [existing text...]
>> 
>> and then I don't think we need to have another block for
>> --no-diff-merges.
>> 
>> I'll likewise add a statement that "-m" is implied by "--first-parent"
>> and can be counteracted with the "--no" form, which I think should spell
>> out all the implications of the series.
>
> Hmm, I take that back. I tried adding this:
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 0785a0cfe9..41c859e63f 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -1154,7 +1154,9 @@ options may be given. See linkgit:git-diff-files[1] for more options.
>  	and diff is generated. An exception is that only diff against
>  	the first parent is shown when `--first-parent` option is given;
>  	in that case, the output represents the changes the merge
> -	brought _into_ the then-current branch.
> +	brought _into_ the then-current branch. Note that
> +	`--first-parent` implies `-m` if no combined-diff option is
> +	enabled; you can use `--no-diff-merges` to override that.
>  
>  -r::
>  	Show recursive diffs.
>
> but then I'm left wondering: why would you ever want to override it? I
> added the option as an escape hatch in case anybody really needed the
> old behavior.

> For some projects, it's useful to walk the first parent history, looking
> at each merge commit as a normal commit introducing all of the changes
> no its side branch. E.g.:
> 
>   git log --first-parent -m -Sfoo -p

> For some projects, it's useful to walk the first parent history, looking
> at each merge commit as a normal commit introducing all of the changes
> no its side branch. E.g.:
>
>   git log --first-parent -m -Sfoo -p
>
> might show you the topic or pull request that introduced code "foo". But
> I quite often forget to add "-m", and get confused that it doesn't
> return any results.

> Jeff King <peff@peff.net> writes:
> 
> > For some projects, it's useful to walk the first parent history, looking
> > at each merge commit as a normal commit introducing all of the changes
> > no its side branch. E.g.:
> >
> >   git log --first-parent -m -Sfoo -p
> >
> > might show you the topic or pull request that introduced code "foo". But
> > I quite often forget to add "-m", and get confused that it doesn't
> > return any results.
> 
> Yes.  I agree that --first-parent should imply -m when combined with
> diff options like -p, --raw, etc.  I am not sure if -m should kick
> in without any diff options, though.  Doesn't it have side effects?

> For some projects, it's useful to walk the first parent history, looking
> at each merge commit as a normal commit introducing all of the changes
> no its side branch. E.g.:
>
>   git log --first-parent -m -Sfoo -p
>
> might show you the topic or pull request that introduced code "foo". But
> I quite often forget to add "-m", and get confused that it doesn't
> return any results.
>
> This series just makes --first-parent imply -m. That doesn't change any
> output by itself, but does mean that diff options like "-p", "-S", etc,
> behave sensibly.
>
>   [1/3]: log: drop "--cc implies -m" logic
>   [2/3]: revision: add "--ignore-merges" option to counteract "-m"
>   [3/3]: log: enable "-m" automatically with "--first-parent"
>
>  Documentation/rev-list-options.txt            |  1 +
>  builtin/log.c                                 |  7 +-
>  revision.c                                    | 10 ++-
>  revision.h                                    |  2 +-
>  t/t4013-diff-various.sh                       |  1 +
>  ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++
>  t/t4013/diff.log_-p_--first-parent_master     | 22 ++++++
>  7 files changed, 113 insertions(+), 8 deletions(-)
>  create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
>
> -Peff

> diff --git a/builtin/log.c b/builtin/log.c
> index 39b3d773a9..83b147c23a 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -731,6 +731,9 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
>  	/* Turn --cc/-c into -p --cc/-c when -p was not given */
>  	if (!rev->diffopt.output_format && rev->combine_merges)
>  		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
> +
> +	if (rev->first_parent_only && rev->ignore_merges < 0)
> +		rev->ignore_merges = 0;

> Our rev-list-options.txt include has a "Diff Formatting" section, but it
> is ifndef'd out for all manpages except git-log. And a few bits of the
> text are rather out of date.
>
> We say "some of these options are specific to git-rev-list". That's
> obviously silly since we (even before this patch) show the content only
> for git-log. But moreover, it's not true; each of the listed options is
> meaningful for other diff commands.
>
> We also say "...however other diff options may be given. See git-diff-files
> for more options." But there's no need to do so; git-log already has a
> "Common Diff Options" section which includes diff-options.txt.
>
> So let's move these options over to git-log and put them with the other
> diff options, giving a single "diff" section for the git-log
> documentation. We'll call it "Diff Formatting" but use the all-caps
> top-level header to match its sibling sections. And we'll rewrite the
> section intro to remove the useless bits and give a more generic
> overview of the section which can be later extended.

> > So let's move these options over to git-log and put them with the other
> > diff options, giving a single "diff" section for the git-log
> > documentation. We'll call it "Diff Formatting" but use the all-caps
> > top-level header to match its sibling sections. And we'll rewrite the
> > section intro to remove the useless bits and give a more generic
> > overview of the section which can be later extended.
> 
> Makes sense.  I first was afraid of regressing "git show"
> documentation because the conditional inclusion was
> 
>     > -
>     > -ifndef::git-shortlog[]
>     > -ifndef::git-rev-list[]
>     > -Diff Formatting
>     > -~~~~~~~~~~~~~~~
>     > -
> 
> but it seems that Documentation/git-show.txt does not even include
> this section being moved in the first place.
> 
> We might move these to a new file and include it from both git-log.txt
> and git-show.txt but that can be left outside the topioc.

> Jeff King <peff@peff.net> writes:
> 
> > The "-t" option is infrequently used; it doesn't deserve a spot near the
> > top of the options list. Let's push it down into the diff-options
> > include, near the definition of --raw.
> 
> True.  It does not have any effect unless --raw is in use (i.e.  it
> does have an effect to "git show --patch-with-raw" or anything that
> (also) shows "--raw" output), and it makes sense to have "-t" near
> "--raw".

> Here's a re-roll taking into account the discussion so far:
>
>   - the escape hatch option name is flipped to "--no-diff-merges" (with
>     "--diff-merges" matching "-m")
>
>   - the documentation now discusses the change as well as the existing
>     handling of merges; this involved a few extra cleanups. Try
>     "doc-diff" for a better view of the actual rendered changes.
>
>     I do think longer term we'd be better off to stop having this maze
>     of ifdef'd inclusions, and just have gitdiff(7) which covers all of
>     the possibilities in human-readable text (so yes, you might see a
>     mention of diff-files while you're looking for info on git-log, but
>     that would also broaden your mind about how the different commands
>     work). But that's clearly outside the scope of this series. I think
>     what's here is a strict improvement.

> On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote:
>
>> This series just makes --first-parent imply -m. That doesn't change any
>> output by itself, but does mean that diff options like "-p", "-S", etc,
>> behave sensibly.

>
> Here's a re-roll taking into account the discussion so far:
>
>   - the escape hatch option name is flipped to "--no-diff-merges" (with
>     "--diff-merges" matching "-m")

> > Here's a re-roll taking into account the discussion so far:
> >
> >   - the escape hatch option name is flipped to "--no-diff-merges" (with
> >     "--diff-merges" matching "-m")
> 
> Rather than being just a synonym for -m, is there a chance for
> "--diff-merges" implementation to be turned to output diff to the first
> parent only, no matter if --first-parent is active or not?
>
> Alternatively, may it have a parameter such as "-m parent-number" of
> "git cherry-pick" being set to "1" by default?

> On Thu, Jul 30, 2020 at 12:41:39AM +0300, Sergey Organov wrote:
>
>> > Here's a re-roll taking into account the discussion so far:
>> >
>> >   - the escape hatch option name is flipped to "--no-diff-merges" (with
>> >     "--diff-merges" matching "-m")
>> 
>> Rather than being just a synonym for -m, is there a chance for
>> "--diff-merges" implementation to be turned to output diff to the first
>> parent only, no matter if --first-parent is active or not?
>>
>> Alternatively, may it have a parameter such as "-m parent-number" of
>> "git cherry-pick" being set to "1" by default?
>
> Yes, I agree that would be a useful feature, but I don't think it needs
> to be part of this series. It could be implemented as --diff-merges=1 to
> show only the one against the first parent, or as its own option. But we
> can add that on top.
>
>> This -m output of diffs to all the parents is in fact primary source of
>> confusion for me, even over all these mind-blowing inter-dependencies
>> between --first-parent, --cc, -c, -m, -p and what not. Who ever needs
>> these (potentially huge) diffs against other parents, anyway?
>
> I've used "-m" second-parent diffs occasionally for hunting down
> mismerges, etc, but I agree that most of the time you just want to see
> the diff against the first parent.
>
>> Introduction of this new option is a great opportunity for improvement
>> that would be a pity to miss.
>
> Adding an optional value to the flag is something we can do later. We
> would miss the opportunity for "--diff-merges" to default to
> "--diff-merges=1", but I'm not sure I'd want to do that anyway. Having
> it be consistent with "-m" seems less confusing to me, and it is already
> too late to change that.
>
> If we want an option that defaults to "1", we can give it a new name.
> The only thing that is lost now is that --diff-merges would already be
> taken. :) But I think I'd probably call such an option "--diff-parents"
> or something like that anyway.

> What I in fact have in mind is something like:
>
>   --diff-merges[=<parent-number>|c|cc|all]
>
> to have a single point of definition of the needed format of merges
> representation.

> Sergey Organov <sorganov@gmail.com> writes:
>
>> What I in fact have in mind is something like:
>>
>>   --diff-merges[=<parent-number>|c|cc|all]
>>
>> to have a single point of definition of the needed format of merges
>> representation.
>
> A command line option that takes _optional_ argument is evil; it
> hurts teachability (e.g. "why does this option always require
> "--opt=val" and refuses '--opt val'?").

>
>> Then comes the question of the default. "all" is what'd make it behave
>> as "-m" behaves now. If it's too late for --diff-merges to have
>> different default, could the default possibly be a configuration
>> option rather than yet another command-line option?
>
> Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is
> different from --diff-merges, and -m and --[no-]diff-merges can be
> defined in terms of that, at which point we can gradually deprecate
> them if we wanted to, no?

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Sounds great, I only hoped we can do it right now, with this new
>> --diff-merges option, maybe as a pre-requisite to the patches in
>> question, but Jeff said it's too late, dunno why.
>
> A follow-up patch or two to remove the "--diff-merges" option and
> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of
> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a
> possibility.
>
> But is it worth the delay?  I dunno.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Sounds great, I only hoped we can do it right now, with this new
>>> --diff-merges option, maybe as a pre-requisite to the patches in
>>> question, but Jeff said it's too late, dunno why.
>>
>> A follow-up patch or two to remove the "--diff-merges" option and
>> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of
>> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a
>> possibility.
>>
>> But is it worth the delay?  I dunno.
>
> I don't think it's worth the delay, provided yet another new
> --diff-parents is to be implemented rather that using --diff-merges for
> that.

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergey Organov <sorganov@gmail.com> writes:
>>>
>>>> Sounds great, I only hoped we can do it right now, with this new
>>>> --diff-merges option, maybe as a pre-requisite to the patches in
>>>> question, but Jeff said it's too late, dunno why.
>>>
>>> A follow-up patch or two to remove the "--diff-merges" option and
>>> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of
>>> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a
>>> possibility.
>>>
>>> But is it worth the delay?  I dunno.
>>
>> I don't think it's worth the delay, provided yet another new
>> --diff-parents is to be implemented rather that using --diff-merges for
>> that.
>
> I was responding to your "it's too late, dunno why", as you seemed
> not to want to waste an option "--diff-merges" that will become
> unused when "--diff-parents" come and also wanted it to happen right
> now.  If you no longer want to see it happen right now, that's OK by
> me.

> As a side-note, my secret hope is for pure "git log -p" to give me diff
> against first parent for all the commits, no matter how many parents
> they happen to have. This desire still sounds like a job for
> configuration option though, rather than, or in addition to,
> --diff-merges or --diff-parents? We well can later introduce a
> config to assume --diff-merges=<config> when no explicit
> --diff-merges=<value> is specified, right?

> On Mon, Aug 03, 2020 at 11:25:05PM +0300, Sergey Organov wrote:
>
>> I mean, why introduce --[no-]diff-merges in the first place, if we do
>> agree --xxx=(none|...) is where we'd like to end up? I thought the
>> answer was "it's too late", but in fact it was an answer to changing
>> semantics of -m that I don't think I suggest.
>
> Spelling out the between the lines of my answer a bit more, it was
> really: I am happy enough with the topic as-is and do not want to rework
> it again. But if _you_ want to do so, I have no problem with it. :)

>
>> As a side-note, my secret hope is for pure "git log -p" to give me diff
>> against first parent for all the commits, no matter how many parents
>> they happen to have. This desire still sounds like a job for
>> configuration option though, rather than, or in addition to,
>> --diff-merges or --diff-parents? We well can later introduce a
>> config to assume --diff-merges=<config> when no explicit
>> --diff-merges=<value> is specified, right?
>
> Yes, I think a config there would be reasonable as long as:
>
>   - we have the command-line options to counteract it when necessary
>     (i.e., --diff-parents or your advanced --diff-merges exists, too)
>
>   - it only impacts porcelain "git log", and not plumbing like
>     diff-tree.

> > A command line option that takes _optional_ argument is evil; it
> > hurts teachability (e.g. "why does this option always require
> > "--opt=val" and refuses '--opt val'?").
> 
> Yeah, I sympathize.

> > Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is
> > different from --diff-merges, and -m and --[no-]diff-merges can be
> > defined in terms of that, at which point we can gradually deprecate
> > them if we wanted to, no?
> 
> Sounds great, I only hoped we can do it right now, with this new
> --diff-merges option, maybe as a pre-requisite to the patches in
> question, but Jeff said it's too late, dunno why.

> On Mon, Aug 03, 2020 at 06:47:20PM +0300, Sergey Organov wrote:
>
>> > A command line option that takes _optional_ argument is evil; it
>> > hurts teachability (e.g. "why does this option always require
>> > "--opt=val" and refuses '--opt val'?").
>> 
>> Yeah, I sympathize.
>
> Sorry, the optional argument was my suggestion. I don't think they're
> that evil, but I agree they require extra knowledge for the user. I
> don't mind avoiding them when possible (and it's definitely possible
> here).
>
>> > Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is
>> > different from --diff-merges, and -m and --[no-]diff-merges can be
>> > defined in terms of that, at which point we can gradually deprecate
>> > them if we wanted to, no?
>> 
>> Sounds great, I only hoped we can do it right now, with this new
>> --diff-merges option, maybe as a pre-requisite to the patches in
>> question, but Jeff said it's too late, dunno why.
>
> It's too late for "-m" to change semantics (we could do a long
> deprecation, but I don't see much point in doing so).

> > It's too late for "-m" to change semantics (we could do a long
> > deprecation, but I don't see much point in doing so).
> 
> I thought not of changing semantics of -m. Suppose we introduce
> 
>   --diff-merges=(none|<parent-number>|c|cc|all)
> 
> before your patch(es). Then your patch would read: "making --first-parent
> imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no?

> > But --diff-merges is definitely still changeable until we release
> > v2.29. My resistance was mostly that I didn't want to complicate my
> > series by adding new elements. But we could do something on top.
> 
> Can't we do yours on top instead? I'd expect it'd then be even simpler.

> On Mon, Aug 03, 2020 at 11:00:11PM +0300, Sergey Organov wrote:
>
>> > It's too late for "-m" to change semantics (we could do a long
>> > deprecation, but I don't see much point in doing so).
>> 
>> I thought not of changing semantics of -m. Suppose we introduce
>> 
>>   --diff-merges=(none|<parent-number>|c|cc|all)
>> 
>> before your patch(es). Then your patch would read: "making --first-parent
>> imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no?
>
> Sure, that would be OK with me. You'd have --diff-merges=none to get
> the current behavior, and probably make --no-diff-merges an alias for
> that.

>
>> > But --diff-merges is definitely still changeable until we release
>> > v2.29. My resistance was mostly that I didn't want to complicate my
>> > series by adding new elements. But we could do something on top.
>> 
>> Can't we do yours on top instead? I'd expect it'd then be even simpler.
>
> Mine is in 'next', so there is no rebuilding it on top of anything else
> without a revert. But I don't see any particular reason to do that
> versus just changing the behavior on top. What's in 'next' is generally
> not rewound, but the behaviors are not cemented with respect to
> backwards compatibility.

> It's too late for "-m" to change semantics (we could do a long
> deprecation, but I don't see much point in doing so). But --diff-merges
> is definitely still changeable until we release v2.29. My resistance was
> mostly that I didn't want to complicate my series by adding new
> elements. But we could do something on top.

> Jeff King <peff@peff.net> writes:
>
>> It's too late for "-m" to change semantics (we could do a long
>> deprecation, but I don't see much point in doing so). But --diff-merges
>> is definitely still changeable until we release v2.29. My resistance was
>> mostly that I didn't want to complicate my series by adding new
>> elements. But we could do something on top.
>
> Attached is rather minimal incompatible change to --diff-merges that'd
> allow extensions in the future, to get out of urge for the discussed
> changes. I'm going to follow-up with actual improvements and I'm aware
> it lacks documentation changes.

> As a minimum patch, I think it is OK to have just 'all' and 'none'
> (not even c or cc, let alone the one with ultra-long name whose
> effect is mystery) before we let the result graduate to 'master'.
> Others can be added on top, as the primary focus of Peff's series is
> to make sure "-m" can be countermanded, for which being able to say
> "no" is sufficient, and the primary reason why we are further
> futzing with the series with this addition is to leave the door open
> for later additions of different "modes" in which how
> "--diff-merges" option can operate (iow, Peff's was merely on/off,
> but you are making sure others such as <num> can be added over
> time).

> On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:
>
>> As a minimum patch, I think it is OK to have just 'all' and 'none'
>> (not even c or cc, let alone the one with ultra-long name whose
>> effect is mystery) before we let the result graduate to 'master'.
>> Others can be added on top, as the primary focus of Peff's series is
>> to make sure "-m" can be countermanded, for which being able to say
>> "no" is sufficient, and the primary reason why we are further
>> futzing with the series with this addition is to leave the door open
>> for later additions of different "modes" in which how
>> "--diff-merges" option can operate (iow, Peff's was merely on/off,
>> but you are making sure others such as <num> can be added over
>> time).
>
> I like that suggestion very much. It solves the "optional arguments are
> evil" problem without having to worry about the other bits.

> On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:
>
>> I only don't like --diff-merges=none (even though it sounds great for
>> --diff-parents=none) and used --diff-merges=off instead. It's not a
>> strong feeling though, and I'm fine with whatever we decide.
>
> I think that is fine. I took "none" to be "diff against none of the
> parents", which is the opposite of "all". But "off" conveys that, too.

> Jeff King <peff@peff.net> writes:
>
>> On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:
>>
>>> I only don't like --diff-merges=none (even though it sounds great for
>>> --diff-parents=none) and used --diff-merges=off instead. It's not a
>>> strong feeling though, and I'm fine with whatever we decide.
>>
>> I think that is fine. I took "none" to be "diff against none of the
>> parents", which is the opposite of "all". But "off" conveys that, too.
>
> For now, "off" is OK, but then we'll regret when "all" comes,
> because "off" would not exactly sit opposite to "all".

> > For now, "off" is OK, but then we'll regret when "all" comes,
> > because "off" would not exactly sit opposite to "all".
> 
> IMHO, "off" does not need to be opposite for "all", as it suppresses
> diff output altogether. I read --diff-merge=off as "turn /off/ diff
> output for merge commits".
> 
> Besides, "all", that I don't like either, is among "c" and "cc", all 3
> being different versions of diffs against all the parents, no?

> That's just coming from the angle of "what is the most general and
> orthogonal set of features". I think the vast majority of what anyone
> would want to do would be covered by doing a diff against only a single
> parent, and then it would almost always be the first parent. And
> certainly you'd need to add a bunch of code to the combined diff
> machinery to make it support arbitrary sets of parents. So this probably
> isn't that interesting a direction to go, at least for now. 

> > I'm just
> > raising the issue now because we'll be locked into the semantics of this
> > option, which may not be able to express the full set of what's possible
> > (so we'd be stuck adding another option later).
> 
> Yeah, but a good thing is that we won't have to worry about this
> until much later, as long as we would just be introducing "diff
> against no parents" and nothing else (or together with "diff against
> all parents", which would make it easier to explain "-m").

> On Tue, Aug 04, 2020 at 03:49:17PM -0700, Junio C Hamano wrote:
>
>> > I'm just
>> > raising the issue now because we'll be locked into the semantics of this
>> > option, which may not be able to express the full set of what's possible
>> > (so we'd be stuck adding another option later).
>> 
>> Yeah, but a good thing is that we won't have to worry about this
>> until much later, as long as we would just be introducing "diff
>> against no parents" and nothing else (or together with "diff against
>> all parents", which would make it easier to explain "-m").
>
> Agreed. My only question is whether the possibility of later having
> those other options might influence how we name the two options we add
> now. I think it's clear to all of us in this thread how those two easy
> options should behave, but if the intent is to eventually allow these to
> be mutually exclusive:
>
>   - no diff
>   - combined
>   - dense combined
>   - individual diff against each parent
>
> but orthogonal to the selection of the parent-set (none, all, or
> selected ones) then e.g. "all" makes less sense for "individual diff
> against each parent". I don't have a good succinct name suggestion,
> though.

> Agreed. My only question is whether the possibility of later having
> those other options might influence how we name the two options we add
> now. I think it's clear to all of us in this thread how those two easy
> options should behave, but if the intent is to eventually allow these to
> be mutually exclusive:
>
>   - no diff
>   - combined
>   - dense combined
>   - individual diff against each parent
>
> but orthogonal to the selection of the parent-set (none, all, or
> selected ones) then e.g. "all" makes less sense for "individual diff
> against each parent". I don't have a good succinct name suggestion,
> though.
>
> TBH, I would be happy enough with any of the suggestions in the thread,
> so I am really just finishing the thought here, and not trying to derail
> progress. :)

> Jeff King <peff@peff.net> writes:
>
>> Agreed. My only question is whether the possibility of later having
>> those other options might influence how we name the two options we add
>> now. I think it's clear to all of us in this thread how those two easy
>> options should behave, but if the intent is to eventually allow these to
>> be mutually exclusive:
>>
>>   - no diff
>>   - combined
>>   - dense combined
>>   - individual diff against each parent
>>
>> but orthogonal to the selection of the parent-set (none, all, or
>> selected ones) then e.g. "all" makes less sense for "individual diff
>> against each parent". I don't have a good succinct name suggestion,
>> though.
>>
>> TBH, I would be happy enough with any of the suggestions in the thread,
>> so I am really just finishing the thought here, and not trying to derail
>> progress. :)
>
> I agree in principle that the above is a good framework to think
> about the issue around "what to do with diff when showing a merge
> commit", but I suspect that overly spending our effort to cover the
> possibilities become mostly useless mental exercise, mostly because
> (1) comparing with second parent is mostly useful only when the
> merge was done in the wrong direction (i.e. an attempt by a leaf
> contributor to "catch up to the trunc"), (2) octopus merges are rare
> curiosity and discouraged due to bisect efficiency anyway, and (3)
> even when looking at an octopus merge, omitting some and using only
> a few selected parents to view with --cc/-c has dubious usefulness,
> as the postimage has to show contributions from all the parents
> plus "evil" adjustment anyway (iow, the primary effect of omitting
> parents while viewing --cc/-c is to make it fuzzy which part of
> apparently "evil" adjustment is what the merge did vs what the
> hidden parents did).  These are all examples that show not all the
> combinations are useful.
>
> So...

>> I agree in principle that the above is a good framework to think
>> about the issue around "what to do with diff when showing a merge
>> commit", but I suspect that overly spending our effort to cover the
>> possibilities become mostly useless mental exercise, mostly because
>> (1) comparing with second parent is mostly useful only when the
>> merge was done in the wrong direction (i.e. an attempt by a leaf
>> contributor to "catch up to the trunc"), (2) octopus merges are rare
>> curiosity and discouraged due to bisect efficiency anyway, and (3)
>> even when looking at an octopus merge, omitting some and using only
>> a few selected parents to view with --cc/-c has dubious usefulness,
>> as the postimage has to show contributions from all the parents
>> plus "evil" adjustment anyway (iow, the primary effect of omitting
>> parents while viewing --cc/-c is to make it fuzzy which part of
>> apparently "evil" adjustment is what the merge did vs what the
>> hidden parents did).  These are all examples that show not all the
>> combinations are useful.
>>
>> So...
>
> So, does
>
> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)
>
> make sense as covering all the current features?

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> I agree in principle that the above is a good framework to think
>>> about the issue around "what to do with diff when showing a merge
>>> commit", but I suspect that overly spending our effort to cover the
>>> possibilities become mostly useless mental exercise, mostly because
>>> (1) comparing with second parent is mostly useful only when the
>>> merge was done in the wrong direction (i.e. an attempt by a leaf
>>> contributor to "catch up to the trunc"), (2) octopus merges are rare
>>> curiosity and discouraged due to bisect efficiency anyway, and (3)
>>> even when looking at an octopus merge, omitting some and using only
>>> a few selected parents to view with --cc/-c has dubious usefulness,
>>> as the postimage has to show contributions from all the parents
>>> plus "evil" adjustment anyway (iow, the primary effect of omitting
>>> parents while viewing --cc/-c is to make it fuzzy which part of
>>> apparently "evil" adjustment is what the merge did vs what the
>>> hidden parents did).  These are all examples that show not all the
>>> combinations are useful.
>>>
>>> So...
>>
>> So, does
>>
>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)
>>
>> make sense as covering all the current features?
>
> If we are primarily interested in theoretical completeness, it may.
> If we are interested more in practical usefulness, I am not sure if
> such a "full flexibility" matrix is a good way to present the
> feature to the end-users.

> I thought it's just a -c, -cc, and -m in better wording. No any
> matrix:
> 
> -c  = --diff-merges=combined
> -cc = --diff-merges=dense
> -m  = --diff-merges=split
> 
> Just separate mutually exclusive options assembled into one multi-value
> option, so it's explicit they are mutually exclusive. I don't see any
> matrix here.

> On Fri, Aug 07, 2020 at 10:12:29PM +0300, Sergey Organov wrote:
>
>> I thought it's just a -c, -cc, and -m in better wording. No any
>> matrix:
>> 
>> -c  = --diff-merges=combined
>> -cc = --diff-merges=dense
>> -m  = --diff-merges=split
>> 
>> Just separate mutually exclusive options assembled into one multi-value
>> option, so it's explicit they are mutually exclusive. I don't see any
>> matrix here.
>
> FWIW, those names make sense to me. Coupled with "none" or "off" for
> disabling all of them.

>>> So, does
>>>
>>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)
>>>
>>> make sense as covering all the current features?
>>
>> If we are primarily interested in theoretical completeness, it may.
>> If we are interested more in practical usefulness, I am not sure if
>> such a "full flexibility" matrix is a good way to present the
>> feature to the end-users.
>
> I thought it's just a -c, -cc, and -m in better wording. No any
> matrix:
>
> -c  = --diff-merges=combined
> -cc = --diff-merges=dense
> -m  = --diff-merges=split
>
> Just separate mutually exclusive options assembled into one multi-value
> option, so it's explicit they are mutually exclusive. I don't see any
> matrix here.

> Sergey Organov <sorganov@gmail.com> writes:
>
>>>> So, does
>>>>
>>>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)
>>>>
>>>> make sense as covering all the current features?
>>>
>>> If we are primarily interested in theoretical completeness, it may.
>>> If we are interested more in practical usefulness, I am not sure if
>>> such a "full flexibility" matrix is a good way to present the
>>> feature to the end-users.
>>
>> I thought it's just a -c, -cc, and -m in better wording. No any
>> matrix:
>>
>> -c  = --diff-merges=combined
>> -cc = --diff-merges=dense
>> -m  = --diff-merges=split
>>
>> Just separate mutually exclusive options assembled into one multi-value
>> option, so it's explicit they are mutually exclusive. I don't see any
>> matrix here.
>
> Oh, matrix comes from specifying the set of parents in a separate
> parameter.  If we are not doing that, then you cannot even express
> "when showing a merge, compare only with the first parent", no?
>
> And when you add --diff-parents=1 (i.e. diff with first-parent), you
> are opening the interface up so that it can express dubious
> combinations like --diff-merges=dense-combined --diff-parents=1,3
> (i.e. --cc but exclude the second parent as one of the preimages).

> I had no intention to introduce --diff-parents, at least for now, and
> maybe never. What I said about it was theoretical discussion rather than
> actual proposal.
>
> If we agree on the above, I intended to instead propose something like:
>
> --diff-merges=first-parent or just =first

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I had no intention to introduce --diff-parents, at least for now, and
>> maybe never. What I said about it was theoretical discussion rather than
>> actual proposal.
>>
>> If we agree on the above, I intended to instead propose something like:
>>
>> --diff-merges=first-parent or just =first
>
> OK, so the combined, combined-dense and split were meant to work
> with all the parents, and off is the only one that means comparison
> with no parents.  That makes sense.

> The only question regarding it I then have for now is what are
> preferences for names selection inside single option? Abbreviated yet
> somewhat sensible, or verbose? I mean:
>
> --diff-merges=first vs --diff-merges=first-parent
>
> --diff-merges=comb vs --diff-merges=combined
>
> etc. What's better?

> Sergey Organov <sorganov@gmail.com> writes:
>
>> The only question regarding it I then have for now is what are
>> preferences for names selection inside single option? Abbreviated yet
>> somewhat sensible, or verbose? I mean:
>>
>> --diff-merges=first vs --diff-merges=first-parent
>>
>> --diff-merges=comb vs --diff-merges=combined
>>
>> etc. What's better?
>
> If we were to shoot for easy-to-type, we could go for ultra-short
> abbreviations like 'no', 'c', 'cc', 'each' (the last one is the
> traditional "-m" when used without "--first-parent"; diff with each
> parent) and later add 'fp', but in a sense we are already lost the
> easy-to-type goal by "--diff-merges" being sufficiently long.

>
> So, if I were to vote, it would be
>
>     "--diff-merges=" ( first-parent        | 1 |
> 		         combined            | c |
>                        dense-combined      | cc |
>                        each-parent         | m )
>     "--no-diff-merges"
>
> leaving some room to add '2' and  <any posInt> later.

> On Wed, Aug 05, 2020 at 01:06:51AM +0300, Sergey Organov wrote:
>
>> > For now, "off" is OK, but then we'll regret when "all" comes,
>> > because "off" would not exactly sit opposite to "all".
>> 
>> IMHO, "off" does not need to be opposite for "all", as it suppresses
>> diff output altogether. I read --diff-merge=off as "turn /off/ diff
>> output for merge commits".
>> 
>> Besides, "all", that I don't like either, is among "c" and "cc", all 3
>> being different versions of diffs against all the parents, no?
>
> I think "all-parents" is much more descriptive than "all" (which might
> make you think "all merges", but it has nothing to do with that). It
> would make more sense if we later add the building to say "diff against
> parent 1" or "diff against parents 1 and 3".
>
> You might also consider whether "combined" is actually mutually
> exclusive with parent selection. We have focused on which parents you'd
> want to "-m" against. But in the most general case, you could ask for a
> combined-diff between parents 1 and 3 of an octopus merge.
>
> That's just coming from the angle of "what is the most general and
> orthogonal set of features". I think the vast majority of what anyone
> would want to do would be covered by doing a diff against only a single
> parent, and then it would almost always be the first parent. And
> certainly you'd need to add a bunch of code to the combined diff
> machinery to make it support arbitrary sets of parents. So this probably
> isn't that interesting a direction to go, at least for now. I'm just
> raising the issue now because we'll be locked into the semantics of this
> option, which may not be able to express the full set of what's possible
> (so we'd be stuck adding another option later).

> On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:
>
>> As a minimum patch, I think it is OK to have just 'all' and 'none'
>> (not even c or cc, let alone the one with ultra-long name whose
>> effect is mystery) before we let the result graduate to 'master'.
>> Others can be added on top, as the primary focus of Peff's series is
>> to make sure "-m" can be countermanded, for which being able to say
>> "no" is sufficient, and the primary reason why we are further
>> futzing with the series with this addition is to leave the door open
>> for later additions of different "modes" in which how
>> "--diff-merges" option can operate (iow, Peff's was merely on/off,
>> but you are making sure others such as <num> can be added over
>> time).
>
> I like that suggestion very much. It solves the "optional arguments are
> evil" problem without having to worry about the other bits.

> diff --git a/revision.c b/revision.c
> index 669bc856694f..dcdff59bc36a 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->diff = 1;
>  		revs->diffopt.flags.recursive = 1;
>  		revs->diffopt.flags.tree_in_recursive = 1;
> -	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>  		revs->ignore_merges = 0;
> +		if (!strcmp(optarg, "off")) {
> +			revs->ignore_merges = 1;
> +		} else if (!strcmp(optarg, "all")) {
> +			revs->diff = 0;

> +		} else if (!strcmp(optarg, "c")) {
> +			revs->diff = 1;
> +			revs->dense_combined_merges = 0;
> +			revs->combine_merges = 1;
> +		} else if (!strcmp(optarg, "cc")) {
> +			revs->diff = 1;
> +			revs->dense_combined_merges = 1;
> +			revs->combine_merges = 1;
> +		} else if (!strcmp(optarg, "combined-all-paths")) {
> +			revs->diff = 1;
> +			revs->combined_all_paths = 1;

> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:
>
>> Attached is rather minimal incompatible change to --diff-merges that'd
>> allow extensions in the future, to get out of urge for the discussed
>> changes. I'm going to follow-up with actual improvements and I'm aware
>> it lacks documentation changes.
>
> Thanks, I like the direction here. Definitely it would need
> documentation, but also tests (probably in t4013 alongside the ones my
> series added; in fact you'd probably need to adjust my tests for the
> non-optional argument).
>
>> diff --git a/revision.c b/revision.c
>> index 669bc856694f..dcdff59bc36a 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct
>> rev_info *revs, int argc, const char **arg
>>  		revs->diff = 1;
>>  		revs->diffopt.flags.recursive = 1;
>>  		revs->diffopt.flags.tree_in_recursive = 1;
>> -	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
>> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>>  		revs->ignore_merges = 0;
>> +		if (!strcmp(optarg, "off")) {
>> +			revs->ignore_merges = 1;
>> +		} else if (!strcmp(optarg, "all")) {
>> +			revs->diff = 0;
>
> Should this be revs->ignore_merges = 0?

> >> diff --git a/revision.c b/revision.c
> >> index 669bc856694f..dcdff59bc36a 100644
> >> --- a/revision.c
> >> +++ b/revision.c
> >> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct
> >> rev_info *revs, int argc, const char **arg
> >>  		revs->diff = 1;
> >>  		revs->diffopt.flags.recursive = 1;
> >>  		revs->diffopt.flags.tree_in_recursive = 1;
> >> -	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
> >> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
> >>  		revs->ignore_merges = 0;
> >> +		if (!strcmp(optarg, "off")) {
> >> +			revs->ignore_merges = 1;
> >> +		} else if (!strcmp(optarg, "all")) {
> >> +			revs->diff = 0;
> >
> > Should this be revs->ignore_merges = 0?
> 
> It's 4 lines above, as it's in fact common for all the cases but the
> first one.

> On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote:
>
>> >> diff --git a/revision.c b/revision.c
>> >> index 669bc856694f..dcdff59bc36a 100644
>> >> --- a/revision.c
>> >> +++ b/revision.c
>> >> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct
>> >> rev_info *revs, int argc, const char **arg
>> >>  		revs->diff = 1;
>> >>  		revs->diffopt.flags.recursive = 1;
>> >>  		revs->diffopt.flags.tree_in_recursive = 1;
>> >> -	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
>> >> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>> >>  		revs->ignore_merges = 0;
>> >> +		if (!strcmp(optarg, "off")) {
>> >> +			revs->ignore_merges = 1;
>> >> +		} else if (!strcmp(optarg, "all")) {
>> >> +			revs->diff = 0;
>> >
>> > Should this be revs->ignore_merges = 0?
>> 
>> It's 4 lines above, as it's in fact common for all the cases but the
>> first one.
>
> Ah, I missed that. That raises more questions, though. ;)
>
> For "-m" we do not need to set revs->diff; why do we need to do so
> here?

>
> For "--cc", we do not need to set revs->ignore_merges. Why do we need to
> do so here? We do need it set eventually, but I think setup_revisions()
> later handles that, and wants ignore_merges untouched to decide whether
> the user asked for it or not.

> >> It's 4 lines above, as it's in fact common for all the cases but the
> >> first one.
> >
> > Ah, I missed that. That raises more questions, though. ;)
> >
> > For "-m" we do not need to set revs->diff; why do we need to do so
> > here?
> 
> Good question!
> 
> I believe --diff-merges=all should reset revs->diff back to 0 to
> entirely undo all the effects of '-c' and '--cc', provided those
> appeared before on the command-line, that '-m' fails to do.

> On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote:
>
>> >> It's 4 lines above, as it's in fact common for all the cases but the
>> >> first one.
>> >
>> > Ah, I missed that. That raises more questions, though. ;)
>> >
>> > For "-m" we do not need to set revs->diff; why do we need to do so
>> > here?
>> 
>> Good question!
>> 
>> I believe --diff-merges=all should reset revs->diff back to 0 to
>> entirely undo all the effects of '-c' and '--cc', provided those
>> appeared before on the command-line, that '-m' fails to do.
>
> Making it counteract "--cc" makes sense, but revs->diff is used for much
> more than that. So "--cc" sets revs->diff to 1, but so do many other
> unrelated options (e.g., "--full-diff" for one).
>
> I think to do it right you'd need to have this part of the code just set
> a single enum variable, like:
>
>   ...
>   else if (!strcmp(arg, "--cc")) {
>           revs->diff_merges = DIFF_MERGES_DENSE_COMBINED;
>   } else if (!strcmp(arg, "-m")) {
>           revs->diff_merges = DIFF_MERGES_ALL_PARENTS;
>   }
>   ...etc...
>
> and then later resolve that into the set of flags we need:
>
>   switch (revs->diff_merges) {
>   case DIFF_MERGES_DENSE_COMBINED:
> 	revs->diff = 1;
> 	revs->dense_combined_merges = 1;
> 	revs->combine_merges = 1;
> 	revs->ignore_merges = 0;
> 	break;
>   case DIFF_MERGES_ALL_PARENTS:
> 	revs->ignore_merges = 0;
> 	break;
>   ...etc...
>   }
>
> it may even be that we can get rid of the separate combine_merges and
> dense_combined_merges flag and just have callers look at
> rev.diff_merges, which would simplify the code even further. But that
> cleanup could also come later on top.

> Jeff King <peff@peff.net> writes:
>
>>> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>>>  		revs->ignore_merges = 0;
>>> +		if (!strcmp(optarg, "off")) {
>>> +			revs->ignore_merges = 1;
>>> +		} else if (!strcmp(optarg, "all")) {
>>> +			revs->diff = 0;
>>
>> Should this be revs->ignore_merges = 0?
>
> It's 4 lines above, as it's in fact common for all the cases but the
> first one.

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>>> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>>>>  		revs->ignore_merges = 0;
>>>> +		if (!strcmp(optarg, "off")) {
>>>> +			revs->ignore_merges = 1;
>>>> +		} else if (!strcmp(optarg, "all")) {
>>>> +			revs->diff = 0;
>>>
>>> Should this be revs->ignore_merges = 0?
>>
>> It's 4 lines above, as it's in fact common for all the cases but the
>> first one.
>
> I may be mistaken, but I thought Peff was asking about turning
> revs->diff off.

> I somehow thought that the equivalence planned for
> the short term is:
>
>             (new)               (peff's)         (master)
> 	diff-merges=none == --no-diff-merges == ! -m
> 	diff-merges=all  == --diff-merges    == -m

>
>> +		} else {
>> +			die("--diff-merges: unknown value '%s'.", optarg);
>> +		}
>
> A few nits:

> >   - we usually don't have a period at the end of our error messages
> 
> Oops, I got the dot from
> 
>   die("--unpacked=<packfile> no longer supported.");
> 
> in the same file. Will fix.

> Get rid of the trailing dot and mark for translation.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  revision.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index 669bc856694f..d08cb5c0e9cd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2315,7 +2315,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if (!strcmp(arg, "--unpacked")) {
>  		revs->unpacked = 1;
>  	} else if (starts_with(arg, "--unpacked=")) {
> -		die("--unpacked=<packfile> no longer supported.");
> +		die(_("--unpacked=<packfile> no longer supported"));
>  	} else if (!strcmp(arg, "-r")) {
>  		revs->diff = 1;
>  		revs->diffopt.flags.recursive = 1;

> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:
>
>> Attached is rather minimal incompatible change to --diff-merges that'd
>> allow extensions in the future, to get out of urge for the discussed
>> changes. I'm going to follow-up with actual improvements and I'm aware
>> it lacks documentation changes.
>
> Thanks, I like the direction here. Definitely it would need
> documentation, but also tests (probably in t4013 alongside the ones my
> series added; in fact you'd probably need to adjust my tests for the
> non-optional argument).

> Jeff King <peff@peff.net> writes:
> ...
> In this case your original test:
>
> git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master
>
> would fail, as implied --diff-merges=1 then wins.

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>> ...
>> In this case your original test:
>>
>> git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master
>>
>> would fail, as implied --diff-merges=1 then wins.
>
> IMHO, I think this is an absolutely wrong thing to do.  At least to
> me (and I suspect it would be to many users), what "--first-parent
> implies 'when showing a diff, compare only with the first parent'"
> means is that it should do so unless told to do otherwise.
>
>     git log --no-diff-merges -p --first-parent
>
> explicitly tells the command that the user does not want to see
> patches for merge commits.  I do not see any reason why
> "--first-parent", which merely *implies* a specific diff generation
> preference for merges, countermand it.  IOW the implication is
> conditional.
>
> It is like saying
>
>     git log --first-parent
>
> should show patches because it *implies* comparing only with the
> first parent, but you can see why it is wrong.  It is because that
> implication is conditional---it kicks in only when the command is
> told to compare with any parent (i.e. "-p").  
>
> I.e. the implication is "compare only with the first parent if told
> to compare, and if not told what to compare with explicitly".

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Yes, we can invent the rule that implied options don't ...
>
> "invent"?  It is nothing new, isn't it?  IIRC, Peff's "first-parent
> implies 'm' but can be countermanded with --no-diff-merges" defines
> "implication" exactly that way.  I do not think that is a recent
> invention but it is just following the patterns set by other options
> that has conditional implications.
>
> IOW,
>
> 	$ git log --no-diff-merges --first-parent -p next
> 	$ git log --first-parent -p --no-diff-merges next
>
> should both mean the same thing.  The user said no patch is wanted
> for merge commits with --no-diff-merges and --first-parent does not
> affect it.