Why doesn't `git log -m` imply `-p`?

Why doesn't `git log -m` imply `-p`?

[Alex Henrie]

I read the following in `man git-log` today:

--diff-merges=separate, --diff-merges=m, -m
    This makes merge commits show the full diff with respect to each of
    the parents. Separate log entry and diff is generated for each
    parent.  -m doesn't produce any output without -p.

--diff-merges=combined, --diff-merges=c, -c
    With this option, diff output for a merge commit shows the
    differences from each of the parents to the merge result
    simultaneously instead of showing pairwise diff between a parent and
    the result one at a time. Furthermore, it lists only files which
    were modified from all parents.  -c implies -p.

--diff-merges=dense-combined, --diff-merges=cc, --cc
    With this option the output produced by --diff-merges=combined is
    further compressed by omitting uninteresting hunks whose contents
    in the parents have only two variants and the merge result picks one
    of them without modification.  --cc implies -p.

Why do -c and -cc imply -p, but -m does not? I tried to use both `git
log -c` and `git log -m` today and was confused when the latter didn't
produce any output. Could we change this behavior in a future version
of Git?

-Alex

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> Why do -c and -cc imply -p, but -m does not? I tried to use both `git
> log -c` and `git log -m` today and was confused when the latter didn't
> produce any output. Could we change this behavior in a future version
> of Git?

"[alias] lm = log -m" can be used when you only want the logs

    $ git lm maint..master

or when you want to also view patches your preference is to see all
sides of diffs of merges

    $ git lm -p maint..mater

but depending on who you are that may be of dubious utility.

It is best to move on, writing it off as historical accident, and
embrace the new --diff-merges=m option, instead of wasting time on
pondering "why", because accidents do not have to have a deep reason
behind them ;-)

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

> Alex Henrie <alexhenrie24@gmail.com> writes:
>
>> Why do -c and -cc imply -p, but -m does not? I tried to use both `git
>> log -c` and `git log -m` today and was confused when the latter didn't
>> produce any output. Could we change this behavior in a future version
>> of Git?
>
> "[alias] lm = log -m" can be used when you only want the logs
>
>     $ git lm maint..master
>
> or when you want to also view patches your preference is to see all
> sides of diffs of merges
>
>     $ git lm -p maint..mater
>
> but depending on who you are that may be of dubious utility.
>
> It is best to move on, writing it off as historical accident, and
> embrace the new --diff-merges=m option, instead of wasting time on
> pondering "why", because accidents do not have to have a deep reason
> behind them ;-)

Yep, but --diff-merges=m doesn't imply -p either, though it does produce
diff output without -p, for merge commits only.

As the final purpose of all this is to have -m as user-friendly short
option, I'd incline to finally let it imply -p, as --diff-merges=m now
covers another side of the coin.

What do you think?

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Alex Henrie]

On Wed, Apr 28, 2021 at 9:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > Why do -c and -cc imply -p, but -m does not? I tried to use both `git
> > log -c` and `git log -m` today and was confused when the latter didn't
> > produce any output. Could we change this behavior in a future version
> > of Git?
>
> "[alias] lm = log -m" can be used when you only want the logs
>
>     $ git lm maint..master
>
> or when you want to also view patches your preference is to see all
> sides of diffs of merges
>
>     $ git lm -p maint..mater
>
> but depending on who you are that may be of dubious utility.
>
> It is best to move on, writing it off as historical accident, and
> embrace the new --diff-merges=m option, instead of wasting time on
> pondering "why", because accidents do not have to have a deep reason
> behind them ;-)

If the behavior is an idiosyncratic accident of dubious utility, let's
replace it with something that makes sense and is useful :-) If we
make -m imply -p then no alias is necessary, `git log` would display
the log without diffs and `git log -m` would display the log with all
the diffs.

On Thu, Apr 29, 2021 at 6:38 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> As the final purpose of all this is to have -m as user-friendly short
> option, I'd incline to finally let it imply -p, as --diff-merges=m now
> covers another side of the coin.
>
> What do you think?

I am 100% in favor of that proposal, and I can work on the code this weekend.

-Alex

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

Alex Henrie <alexhenrie24@gmail.com> writes:

> On Wed, Apr 28, 2021 at 9:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> > Why do -c and -cc imply -p, but -m does not? I tried to use both `git
>> > log -c` and `git log -m` today and was confused when the latter didn't
>> > produce any output. Could we change this behavior in a future version
>> > of Git?
>>
>> "[alias] lm = log -m" can be used when you only want the logs
>>
>>     $ git lm maint..master
>>
>> or when you want to also view patches your preference is to see all
>> sides of diffs of merges
>>
>>     $ git lm -p maint..mater
>>
>> but depending on who you are that may be of dubious utility.
>>
>> It is best to move on, writing it off as historical accident, and
>> embrace the new --diff-merges=m option, instead of wasting time on
>> pondering "why", because accidents do not have to have a deep reason
>> behind them ;-)
>
> If the behavior is an idiosyncratic accident of dubious utility, let's
> replace it with something that makes sense and is useful :-) If we
> make -m imply -p then no alias is necessary, `git log` would display
> the log without diffs and `git log -m` would display the log with all
> the diffs.
>
> On Thu, Apr 29, 2021 at 6:38 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> As the final purpose of all this is to have -m as user-friendly short
>> option, I'd incline to finally let it imply -p, as --diff-merges=m now
>> covers another side of the coin.
>>
>> What do you think?
>
> I am 100% in favor of that proposal, and I can work on the code this
> weekend.

No need to bother. If we agree, I'll send a patch atop of my recent
changes that make -m format configurable.

Alternatively, we can add a configuration option, or let -m imply -p
only when -m format is explicitly configured by the user.

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Alex Henrie]

On Thu, Apr 29, 2021 at 10:35 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > On Thu, Apr 29, 2021 at 6:38 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> As the final purpose of all this is to have -m as user-friendly short
> >> option, I'd incline to finally let it imply -p, as --diff-merges=m now
> >> covers another side of the coin.
> >>
> >> What do you think?
> >
> > I am 100% in favor of that proposal, and I can work on the code this
> > weekend.
>
> No need to bother. If we agree, I'll send a patch atop of my recent
> changes that make -m format configurable.

Great, thank you!

> Alternatively, we can add a configuration option, or let -m imply -p
> only when -m format is explicitly configured by the user.

Since the goal here is simple, easily understandable, and
user-friendly behavior, I think -m should imply -p all the time, or at
least imply -p by default. The less I have to explain to new Git
users, the better.

-Alex

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

Alex Henrie <alexhenrie24@gmail.com> writes:

> On Thu, Apr 29, 2021 at 10:35 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> > On Thu, Apr 29, 2021 at 6:38 AM Sergey Organov <sorganov@gmail.com> wrote:
>> >>
>> >> As the final purpose of all this is to have -m as user-friendly short
>> >> option, I'd incline to finally let it imply -p, as --diff-merges=m now
>> >> covers another side of the coin.
>> >>
>> >> What do you think?
>> >
>> > I am 100% in favor of that proposal, and I can work on the code this
>> > weekend.
>>
>> No need to bother. If we agree, I'll send a patch atop of my recent
>> changes that make -m format configurable.
>
> Great, thank you!
>
>> Alternatively, we can add a configuration option, or let -m imply -p
>> only when -m format is explicitly configured by the user.
>
> Since the goal here is simple, easily understandable, and
> user-friendly behavior, I think -m should imply -p all the time, or at
> least imply -p by default. The less I have to explain to new Git
> users, the better.

Yep, but OTOH -m never implied -p before, and it'll take time for the
change get to release and then to reach distributions... So the actual
question here is if anybody cares enough about backward compatibility in
this particular case to complicate transition?

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Felipe Contreras]

Alex Henrie wrote:
> On Wed, Apr 28, 2021 at 9:22 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Alex Henrie <alexhenrie24@gmail.com> writes:

> > It is best to move on, writing it off as historical accident, and
> > embrace the new --diff-merges=m option, instead of wasting time on
> > pondering "why", because accidents do not have to have a deep reason
> > behind them ;-)
> 
> If the behavior is an idiosyncratic accident of dubious utility, let's
> replace it with something that makes sense and is useful :-) If we
> make -m imply -p then no alias is necessary, `git log` would display
> the log without diffs and `git log -m` would display the log with all
> the diffs.

Indeed. Mistakes in the design of the UI should not be carried on
forever.

Either all --diff-merges={m,c,cc} imply -p, or none should.

-- 
Felipe Contreras

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Alex Henrie wrote:
>> On Wed, Apr 28, 2021 at 9:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>> > Alex Henrie <alexhenrie24@gmail.com> writes:
>
>> > It is best to move on, writing it off as historical accident, and
>> > embrace the new --diff-merges=m option, instead of wasting time on
>> > pondering "why", because accidents do not have to have a deep reason
>> > behind them ;-)
>> 
>> If the behavior is an idiosyncratic accident of dubious utility, let's
>> replace it with something that makes sense and is useful :-) If we
>> make -m imply -p then no alias is necessary, `git log` would display
>> the log without diffs and `git log -m` would display the log with all
>> the diffs.
>
> Indeed. Mistakes in the design of the UI should not be carried on
> forever.
>
> Either all --diff-merges={m,c,cc} imply -p, or none should.

None of --diff-merges imply -p, and none should, -- that's one of the
goals of introducing --diff-merges.

OTOH, -c/--cc do imply -p, and then -m doesn't. That's the historical
inconsistency, not anything about recently implemented --diff-merges,
and the natural way to fix it is to let -m imply -p.

Thanks,

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Yep, but --diff-merges=m doesn't imply -p either, though it does produce
> diff output without -p, for merge commits only.

I misspoke without thinking it through.  It is absolutely wrong for
the "-m" option (or "--diff-merges=m" for that matter) to imply
"-p".

$ git log --stat --summary

would show "diff", but the kind of "diff" requested is not a textual
patch but just diffstat and the summary of new/removed files, and
the "diff" is shown only for single-parent commits, and it omits
"diff" for merge commits.  Adding "-m" to this command line is *not*
a request to show the textual patch.  It is to ask "diff" to be
shown pairwise with each of the parent.

$ git log -m --stat --summary

It is probably OK to special case "-m" given alone without any other
option [*1*] that specifies what kind of "diff" is requested and
make it imply "-p".  But unconditionally flipping "-p" on only
because you saw "-m" (or "--diff-merges=m" for that matter) is just
wrong.


[Footnote]

*1* They are not limited to "-p", "--stat" and "--summary", but
you'd need to also pay attention to "--raw", "--name-only", etc.)

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Yep, but --diff-merges=m doesn't imply -p either, though it does produce
>> diff output without -p, for merge commits only.
>
> I misspoke without thinking it through.  It is absolutely wrong for
> the "-m" option (or "--diff-merges=m" for that matter) to imply
> "-p".
>
> $ git log --stat --summary
>
> would show "diff", but the kind of "diff" requested is not a textual
> patch but just diffstat and the summary of new/removed files, and
> the "diff" is shown only for single-parent commits, and it omits
> "diff" for merge commits.  Adding "-m" to this command line is *not*
> a request to show the textual patch.  It is to ask "diff" to be
> shown pairwise with each of the parent.
>
> $ git log -m --stat --summary
>
> It is probably OK to special case "-m" given alone without any other
> option [*1*] that specifies what kind of "diff" is requested and
> make it imply "-p".  But unconditionally flipping "-p" on only
> because you saw "-m" (or "--diff-merges=m" for that matter) is just
> wrong.

Luckily,

    $ git log [--stat] --diff-merges=first-parent master..seen

seems to do almost the right thing, with respect to the "It is
probably OK to special case" I gave above.

It only "enables diff" for merge commits, which does not quite feel
right and we may want to do the same "enable diff" for single parent
commits, but the good part is that it does not blindly imply "-p".

It seems to do the "enable diff" the right way by honoring other
command line options that specify the format of the diff, so with
"--stat" included in the sample command line above, we get the
diffstat for single parent commits (because we ask for "--stat" from
the command line to show it throughout the history) and also for
merge commits (because --diff-merges=first-parent does *not* blindly
turn the textual patch '-p' on).

> [Footnote]
>
> *1* They are not limited to "-p", "--stat" and "--summary", but
> you'd need to also pay attention to "--raw", "--name-only", etc.)

I've merged the so/log-diff-merge topic to 'master', with this
(possibly) known breakage that it does not do anything for single
parent commits.  We may want to fix this last mile before the
release that is scheduled to happen around early June.

Note that I didn't check if you are doing the right thing for all
formats, or if I was lucky and --stat was only one of them you paid
attention to, when you needed to notice all others that you don't.
But if you used the same logic that allows "git show" to by default
give "-p/--cc" output while "git show --stat" to squelch the patch
output, you should be OK.

Thanks.

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Yep, but --diff-merges=m doesn't imply -p either, though it does produce
>>> diff output without -p, for merge commits only.
>>
>> I misspoke without thinking it through.  It is absolutely wrong for
>> the "-m" option (or "--diff-merges=m" for that matter) to imply
>> "-p".
>>
>> $ git log --stat --summary
>>
>> would show "diff", but the kind of "diff" requested is not a textual
>> patch but just diffstat and the summary of new/removed files, and
>> the "diff" is shown only for single-parent commits, and it omits
>> "diff" for merge commits.  Adding "-m" to this command line is *not*
>> a request to show the textual patch.  It is to ask "diff" to be
>> shown pairwise with each of the parent.
>>
>> $ git log -m --stat --summary
>>
>> It is probably OK to special case "-m" given alone without any other
>> option [*1*] that specifies what kind of "diff" is requested and
>> make it imply "-p".  But unconditionally flipping "-p" on only
>> because you saw "-m" (or "--diff-merges=m" for that matter) is just
>> wrong.
>
> Luckily,
>
>     $ git log [--stat] --diff-merges=first-parent master..seen
>
> seems to do almost the right thing, with respect to the "It is
> probably OK to special case" I gave above.

I believe any special-casing is to be a last resort, and definitely is
not the right thing to do in this particular case.

>
> It only "enables diff" for merge commits, which does not quite feel
> right and we may want to do the same "enable diff" for single parent
> commits, but the good part is that it does not blindly imply "-p".
>
> It seems to do the "enable diff" the right way by honoring other
> command line options that specify the format of the diff, so with
> "--stat" included in the sample command line above, we get the
> diffstat for single parent commits (because we ask for "--stat" from
> the command line to show it throughout the history) and also for
> merge commits (because --diff-merges=first-parent does *not* blindly
> turn the textual patch '-p' on).

Good to know! I must admit I did nothing special in this regard, just
paid attention to avoid breaking any existing logic, at least knowingly.

>
>> [Footnote]
>>
>> *1* They are not limited to "-p", "--stat" and "--summary", but
>> you'd need to also pay attention to "--raw", "--name-only", etc.)
>
> I've merged the so/log-diff-merge topic to 'master', with this
> (possibly) known breakage that it does not do anything for single
> parent commits.  We may want to fix this last mile before the
> release that is scheduled to happen around early June.

I have no idea what the breakage is or could be. Do we have any relevant
tests in place already, or can somebody suggest some to check for
possible breakage?

> Note that I didn't check if you are doing the right thing for all
> formats, or if I was lucky and --stat was only one of them you paid
> attention to, when you needed to notice all others that you don't.
> But if you used the same logic that allows "git show" to by default
> give "-p/--cc" output while "git show --stat" to squelch the patch
> output, you should be OK.

In fact I didn't do anything specific to --stat, nor to other options
you mention (--raw, --name-only, --summary), so I'd expect all of them
still work the same way they were before my --diff-merges series. Need
to be checked anyway, sure thing, and that gets us back to the questions
of tests. I personally don't know what expectations are, so it's hard
for me to implement needed tests.

Thanks,
-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> Luckily,
>>
>>     $ git log [--stat] --diff-merges=first-parent master..seen
>>
>> seems to do almost the right thing, with respect to the "It is
>> probably OK to special case" I gave above.
>
> I believe any special-casing is to be a last resort, and definitely is
> not the right thing to do in this particular case.

I do not know if I get it.  "log --diff-merges=<kind>" giving the
same output as "log" (i.e. no trace of any kind of diff) would be
puzzling to users, and to help them, it is OK to say that

 * "--diff-merges=<kind>" enables some kind of diff output
   automatially (for both merges and non-merges), and

 * when there is no user preference given as to what kind of diff is
   desired, we default to "-p".

As it is natural to expect "--stat --diff-merges=<kind> would give
only the diffstat without patch, we end up "special casing"
"--diff-merges=<kind>" that is given alone, without specifying what
kind of diff is desired, and behave as if "-p" was given.  So I
would have expected you to call this kind of "special casing" a good
thing.

>> It only "enables diff" for merge commits, which does not quite feel
>> right and we may want to do the same "enable diff" for single parent
>> commits, but the good part is that it does not blindly imply "-p".
>>
>> It seems to do the "enable diff" the right way by honoring other
>> command line options that specify the format of the diff, so with
>> "--stat" included in the sample command line above, we get the
>> diffstat for single parent commits (because we ask for "--stat" from
>> the command line to show it throughout the history) and also for
>> merge commits (because --diff-merges=first-parent does *not* blindly
>> turn the textual patch '-p' on).
>
> Good to know! I must admit I did nothing special in this regard, just
> paid attention to avoid breaking any existing logic, at least knowingly.
>
>>
>>> [Footnote]
>>>
>>> *1* They are not limited to "-p", "--stat" and "--summary", but
>>> you'd need to also pay attention to "--raw", "--name-only", etc.)
>>
>> I've merged the so/log-diff-merge topic to 'master', with this
>> (possibly) known breakage that it does not do anything for single
>> parent commits.  We may want to fix this last mile before the
>> release that is scheduled to happen around early June.
>
> I have no idea what the breakage is or could be.

Because I view

 * "--diff-merges" is a way to specify how merge commits are passed
   to the diff machinery (e.g. pass nothing to the diff machinery,
   compare only with the first parent, etc.), and

 * "--patch", "--stat", "--cc" etc are to specify if we use the diff
   machinery and what kind of output is desired.

but we are conflating the "enable diff" feature into the former to
match end-user expectation, if "--diff-merges" without any of the
"--patch", "--stat", etc. enables the "--patch" output for merge
commits, it would be confusing if we do not give the same "--patch"
output for single-parent commits, too.

But the current code gives "--patch" output only for merge commits,
doesn't it?  E.g.

    $ git log --diff-merges=first-parent master..next

would give patches only for merge commits, but

    $ git log --stat --diff-merges=first-parent master..next

would give us diffstat for all commits, including merges (against
their first parents).

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> Luckily,
>>>
>>>     $ git log [--stat] --diff-merges=first-parent master..seen
>>>
>>> seems to do almost the right thing, with respect to the "It is
>>> probably OK to special case" I gave above.
>>
>> I believe any special-casing is to be a last resort, and definitely is
>> not the right thing to do in this particular case.
>
> I do not know if I get it.  "log --diff-merges=<kind>" giving the
> same output as "log" (i.e. no trace of any kind of diff) would be
> puzzling to users, and to help them, it is OK to say that

I thought (apparently wrong) that the idea was to special-case "-m", and
only "-m". I.e., if -m is alone, let it imply -p, otherwise not. That
was the thing I was in opposition to.

>
>  * "--diff-merges=<kind>" enables some kind of diff output
>    automatially (for both merges and non-merges), and

No, I don't think this is OK, sorry. I fail to see why --diff-merges
should affect non-merge commits. I believe it shouldn't.

>
>  * when there is no user preference given as to what kind of diff is
>    desired, we default to "-p".

What kind of diff "-p" gives for merge commits, exactly? As far as I can
tell, it's "none".

>
> As it is natural to expect "--stat --diff-merges=<kind> would give
> only the diffstat without patch, we end up "special casing"
> "--diff-merges=<kind>" that is given alone, without specifying what
> kind of diff is desired, and behave as if "-p" was given.

In general, I hate dependencies between options. The only thing that is
actually needed for convenience is an ability for an option to imply
other options. What's currently there is already too complex for my
personal taste, and I'd hate to add even more complexity on top.

Right now --diff-merges is pretty simple and straightforward: it
specifies diff output for merge commits, and only for merge commits. If
any other option disables diff machinery altogether, it will disable
diff for merges as well.

OTOH, we have --patch that deals with non-merge commits.

Personally, I don't like the resulting interface very much, but it's
historical, and can't be easily changed.

> So I would have expected you to call this kind of "special casing" a
> good thing.

Well, even though I was originally commenting about -m only, I must
admit I'm in general against any "special casing", unless there is
extremely strong reason to consider it.

>
>>> It only "enables diff" for merge commits, which does not quite feel
>>> right and we may want to do the same "enable diff" for single parent
>>> commits,

I fail to see why --diff-merges should ever affect non-merge commits.
It'd be at least counter-intuitive, not to say directly opposite to the
original design goal.

>>> but the good part is that it does not blindly imply "-p".

Yep.

>>>
>>> It seems to do the "enable diff" the right way by honoring other
>>> command line options that specify the format of the diff, so with
>>> "--stat" included in the sample command line above, we get the
>>> diffstat for single parent commits (because we ask for "--stat" from
>>> the command line to show it throughout the history) and also for
>>> merge commits (because --diff-merges=first-parent does *not* blindly
>>> turn the textual patch '-p' on).
>>
>> Good to know! I must admit I did nothing special in this regard, just
>> paid attention to avoid breaking any existing logic, at least knowingly.
>>
>>>
>>>> [Footnote]
>>>>
>>>> *1* They are not limited to "-p", "--stat" and "--summary", but
>>>> you'd need to also pay attention to "--raw", "--name-only", etc.)
>>>
>>> I've merged the so/log-diff-merge topic to 'master', with this
>>> (possibly) known breakage that it does not do anything for single
>>> parent commits.  We may want to fix this last mile before the
>>> release that is scheduled to happen around early June.
>>
>> I have no idea what the breakage is or could be.
>
> Because I view
>
>  * "--diff-merges" is a way to specify how merge commits are passed
>    to the diff machinery (e.g. pass nothing to the diff machinery,
>    compare only with the first parent, etc.), and

As I see it, it only defines the way they are to be represented by the
diff machinery once passed to it, though it obviously depends on where
we put the margin of "diff machinery".

>
>  * "--patch", "--stat", "--cc" etc are to specify if we use the diff
>    machinery and what kind of output is desired.

So, in your view, --cc output is not a product of "diff machinery"?

> but we are conflating the "enable diff" feature into the former to
> match end-user expectation, if "--diff-merges" without any of the
> "--patch", "--stat", etc. enables the "--patch" output for merge
> commits, it would be confusing if we do not give the same "--patch"
> output for single-parent commits, too.
>
> But the current code gives "--patch" output only for merge commits,
> doesn't it?  E.g.

No, as far as I understand it, "--patch" output is for non-merge commits
only. One can't sensibly use patch utility to pick merge commits anyway,
so "--patch" makes no sense for merge commits and doesn't affect them,
at least for now.

>
>     $ git log --diff-merges=first-parent master..next
>
> would give patches only for merge commits, but

It will give the output similar to what "--patch" would give for
non-merge commits, yes, but in fact it's not "--patch" output, I think,
so I doubt it should be called "give patches". It's just happens to be
the same diff format.

>
>     $ git log --stat --diff-merges=first-parent master..next
>
> would give us diffstat for all commits, including merges (against
> their first parents).

Yep, but I think it just matches the old behavior that has been always
there, see below.

I'd start from the behavior even before my patches. Let's see:

  git log -n1 -p <merge_commit>
  git log -n1 --stat <merge_commit>
  git log -n1 --stat -p <merge_commit>

all give no diff no stat. No surprise for diff, though not that sure about
stat, but it could be argued either way.

  git log -n1 -c <merge_commit>

does give diff output in particular format, nice!

  git log -n1 --stat -c <merge_commit>

gives stat output, but no diff! That's not what I expected at all.
Effectively, this looks like --stat *disables* -c/-cc output?

Finally, the way to get both diff and stat for merge commits is... who'd
guess, adding -p to the command, and that provided -p is already
supposedly implied by -c (!):

  git log -n1 --stat -c -p <merge_commit>

In particular, this means that contrary to documentation, -c does not
imply -p in the common sense of the word "imply", and interdependencies
between all these options are already too complex to easily grok for a
human being.

As for newer --diff-merges, they behave similar to -c here that seems
reasonable. Overall, I still don't see any breakage introduced by
--diff-merges, and it seems to behave according to its documentation, so
shouldn't break any expectations either.

Getting back to the original question of letting -m imply -p, it
shouldn't behave differently than -c/-cc, that do imply -p, so I don't
see any significant problem that'd be added to the current status.

Right now the following two give exactly the same output:

  git log -n1 --stat -c <merge_commit>
  git log -n1 --stat -m <merge_commit>

the stat to the first parent, and it shouldn't change if we let -m "imply"
-p the same way -c "implies" -p, whatever it actually means.

Best Regards,

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>>  * "--patch", "--stat", "--cc" etc are to specify if we use the diff
>>    machinery and what kind of output is desired.
>
> So, in your view, --cc output is not a product of "diff machinery"?

I view --cc and -c as an enhanced form of --patch that is also
capable of handling multi-way diffs, in other words, choosing --cc
should be to say "give me textual patch for all commits; when there
are multiple parents, condense multi-way patches".

So, yes, strictly speaking, --diff-merges=cc was probably a mistake,
and in the ideal world, --diff-merges should have taken only one of
"compare with nothing" (optional), "compare with first-parent", and
"compare with all parents".  The last choice could output diffs in
various forms, like traditional -m (i.e. patch output separately for
each parent), --cc, -c, etc.  "compare with nothing" is optional
because we could also control on the "output format" side to say
"produce no output" (ala "git show -s").

But such an idealized orthogonal design without special casing will
often lead to usability problems and complaints that -m alone does
not produce anything, so I am OK to have cc and friends as the value
for --diff-merges for that reason.

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>>  * "--patch", "--stat", "--cc" etc are to specify if we use the diff
>>>    machinery and what kind of output is desired.
>>
>> So, in your view, --cc output is not a product of "diff machinery"?
>
> I view --cc and -c as an enhanced form of --patch that is also
> capable of handling multi-way diffs, in other words, choosing --cc
> should be to say "give me textual patch for all commits; when there
> are multiple parents, condense multi-way patches".
>
> So, yes, strictly speaking, --diff-merges=cc was probably a mistake,
> and in the ideal world, --diff-merges should have taken only one of
> "compare with nothing" (optional), "compare with first-parent", and
> "compare with all parents".  The last choice could output diffs in
> various forms, like traditional -m (i.e. patch output separately for
> each parent), --cc, -c, etc.  "compare with nothing" is optional
> because we could also control on the "output format" side to say
> "produce no output" (ala "git show -s").
>
> But such an idealized orthogonal design without special casing will
> often lead to usability problems and complaints that -m alone does
> not produce anything, so I am OK to have cc and friends as the value
> for --diff-merges for that reason.

I basically agree with what you say here, and it's in fact very close to
the first variant of the design that originally came to my mind, and it
was reality that shifted things to the way they are implemented now.

That said, what do we decide about -m to finally join -c/--cc party and
start to imply -p? Last time we've discussed it, we decided that -m has
been simply overlooked when -c/--cc started to imply -p. Should we
finally fix this?


Thanks,

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> That said, what do we decide about -m to finally join -c/--cc party and
> start to imply -p? Last time we've discussed it, we decided that -m has
> been simply overlooked when -c/--cc started to imply -p. Should we
> finally fix this?

I thought I already said this, but in case I didn't, I think
"--diff-merges=separate" should imply "some kind of diff", and not
necessarily "-p".

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> That said, what do we decide about -m to finally join -c/--cc party and
>> start to imply -p? Last time we've discussed it, we decided that -m has
>> been simply overlooked when -c/--cc started to imply -p. Should we
>> finally fix this?
>
> I thought I already said this, but in case I didn't, I think
> "--diff-merges=separate" should imply "some kind of diff", and not
> necessarily "-p".

Is this a more polite way to say "no"? If not, how is it relevant for
-m, now being a synonym for --diff-merges=on?

As for particular idea, I'll repeat myself as well and say that I'm
still against implying anything by any off --diff-merges, and even more
against implying something that affects non-merge commits. --diff-merges
are not convenience options that need to be short yet give specific
functionality, so there is no place for additional implications.

That said, I think that something like your idea could be fine if we
introduce another convenience option, say, -d, that will imply both
--diff-merges=separate and "some kind of diff" (whatever the latter
actually means, I'm not sure yet.) But then again, why don't just reuse
-m that, as we've decided before, is not that useful in its current
state anyway?

I must admit that I don't entirely understand your idea above yet. Maybe
you could provide a draft of manual entry for proposed behavior of
--diff-merges=separate, for better understanding? For convenience, right
now it reads:

   --diff-merges=separate
       This makes merge commits show the full diff with respect to
       each of the parents. Separate log entry and diff is generated
       for each parent.

Thanks,

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> I thought I already said this, but in case I didn't, I think
>> "--diff-merges=separate" should imply "some kind of diff", and not
>> necessarily "-p".
>
> Is this a more polite way to say "no"? If not, how is it relevant for
> -m, now being a synonym for --diff-merges=on?

Sorry, I didn't mean to say "no" to anything.  

I wrote 'separate' not because I wanted to special case that (and
treat others like 'on' differently), but simply because I didn't
want to write "--diff-merges=<anything>" as "off/no" should not
imply "show some kind of diff".

> As for particular idea, I'll repeat myself as well and say that I'm
> still against implying anything by any off --diff-merges, and even more
> against implying something that affects non-merge commits. --diff-merges
> are not convenience options that need to be short yet give specific
> functionality, so there is no place for additional implications.

So -m (a shorthand for --diff-merges=on) should not imply any patch
generation, you mean?  It matches what we seem to have agreed on to
be the purist view in a few messages ago.  --diff-merges controls
which parent(s) comparison is made against in a merge,
-p/--cc/--raw/--stat etc. control how the result of that comparison
is expressed.

But I also remember that we agreed that the purist view design was
cumbersome to use, so --diff-merges=<anything but no> implying "show
some kind of diff" is OK, plus if nobody says "what kind" via the
command line with -p/--cc/--raw/--stat etc., it is OK to default to
'-p'.

One thing I think our unnecessary "disagreement" comes from is that
among "-m", "--cc", "-c", you say "-m" is the only thing that does
not imply "-p", but I do not view "--cc" and "-c" as sitting next to
"-m" at all in the first place.

"-m" is on the "which parent(s) to compare with" side, while "--cc"
and "-c" are "now you decided which parent(s) to compare with, how
does the result of comparison presented?" side.  And because
"--cc"/"-c" explicitly wants to work on merge commits (because it
naturally degenerates to simple "--patch" for non merges), THEY are
made to imply "-m" (i.e. compare with all parents).

So from my point of view, "--cc/-c" implying "-m" has no relevance
to whether "-m" should or should not imply "some kind of comparison
should be shown".

But because we agreed that we want to bend the purist view for
usability and included cc/c among the choices diff-merges=<choice>
can take, I think -m (but not log.diffMerges=no case) should imply
"we should show some kind of patch".

Which would mean that unless when log.diffMerges or --diff-merges
say off/no, and unless there is any option to specify how the result
of comparison should bepresented on the command line:

 - when log.diffMerges or --diff-merges say cc or c, default to --cc
   or -c.

 - otherwise,default to --patch.

is what I think should happen.  But the reason I think so is not
because "--cc" and "-c" gives output without "-m" (i.e. "-p" does
not imply "-m" and it should not).

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> I thought I already said this, but in case I didn't, I think
>>> "--diff-merges=separate" should imply "some kind of diff", and not
>>> necessarily "-p".
>>
>> Is this a more polite way to say "no"? If not, how is it relevant for
>> -m, now being a synonym for --diff-merges=on?
>
> Sorry, I didn't mean to say "no" to anything.

To me "no" is as good answer as any, I just want to reach better
understanding.  

>
> I wrote 'separate' not because I wanted to special case that (and
> treat others like 'on' differently), but simply because I didn't
> want to write "--diff-merges=<anything>" as "off/no" should not
> imply "show some kind of diff".
>
>> As for particular idea, I'll repeat myself as well and say that I'm
>> still against implying anything by any off --diff-merges, and even more
>> against implying something that affects non-merge commits. --diff-merges
>> are not convenience options that need to be short yet give specific
>> functionality, so there is no place for additional implications.
>
> So -m (a shorthand for --diff-merges=on) should not imply any patch
> generation, you mean?

No, I don't mean it. The idea is to let -m be alias for
"--diff-merges=on -p", exactly the same way --cc is currently
essentially an alias for "--diff-merges=dense-combined -p".

What I meant is that --diff-merges itself should not imply any patch
generation for non-merge commits, so --diff-merges=on should not imply
-p.

> It matches what we seem to have agreed on to be the purist view in a
> few messages ago. --diff-merges controls which parent(s) comparison is
> made against in a merge, -p/--cc/--raw/--stat etc. control how the
> result of that comparison is expressed.

I see this as your vision, but I don't recall we agree on it. At least
that's not how it currently works, as far as I can tell, see command
examples I gathered in one of my previous answers.

>
> But I also remember that we agreed that the purist view design was
> cumbersome to use, so --diff-merges=<anything but no> implying "show
> some kind of diff" is OK, plus if nobody says "what kind" via the
> command line with -p/--cc/--raw/--stat etc., it is OK to default to
> '-p'.

The latter part of this sentence is something rather new to me, that
only appeared in this particular thread of discussion recently, and it
does not match my own vision. Neither my vision of the current
implementation nor of what we should aim for.

>
> One thing I think our unnecessary "disagreement" comes from is that
> among "-m", "--cc", "-c", you say "-m" is the only thing that does
> not imply "-p", but I do not view "--cc" and "-c" as sitting next to
> "-m" at all in the first place.

I was sure you rather did when we've discussed it the last time before
this thread. Now your opinion seems to have changed, and I don't see
why. In fact I'm very confused. As far as understood, that time you said
that -m has been simply overlooked when --cc/-c started to imply -p, and
that you actually don't care about -m that much anyway.

I also recall you said that -c (and later --cc) has been invented as
alternative to not that useful -m, so -m, -c, and -cc have always been
exactly sitting next to each other in my view.

> "-m" is on the "which parent(s) to compare with" side,

This has never been the case, has it? See:

  -m
      This flag makes the merge commits show the full diff like regular
      commits; for each merge parent, a separate log entry 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.

  -c
      With this option, diff output for a merge commit shows the
      differences from each of the parents to the merge result
      simultaneously instead of showing pairwise diff between a parent
      and the result one at a time. Furthermore, it lists only files
      which were modified from all parents.

First, -m doesn't select the parents at all and shows "full diff", so it
rather defines the format. And second, -c is described exactly as being
alternative format to -m, as far as I can tell, making -c sit right next
to -m again, contrary to what you say above.

BTW, I recall I once suggested something like what you said, let -m
match what in means in cherry-pick, to what parent(s) to compare, but
it'd need -m to take (optional) argument(s), that has been considered
unacceptable, so the idea has been rejected (and for the better.)

> while "--cc" and "-c" are "now you decided which parent(s) to compare
> with, how does the result of comparison presented?" side. And because
> "--cc"/"-c" explicitly wants to work on merge commits (because it
> naturally degenerates to simple "--patch" for non merges), THEY are
> made to imply "-m" (i.e. compare with all parents).

That's a reasonable interpretation. The problem is that currently this
does not match nor design, nor implementation, nor documentation at all,
as far as I can tell.

> So from my point of view, "--cc/-c" implying "-m" has no relevance
> to whether "-m" should or should not imply "some kind of comparison
> should be shown".

What you describe is a different design that may well be a good one, but
do we actually want to change what's already there? What for?

>
> But because we agreed that we want to bend the purist view for
> usability and included cc/c among the choices diff-merges=<choice>
> can take, I think -m (but not log.diffMerges=no case) should imply
> "we should show some kind of patch".

Once again, this doesn't fit into the current design, as far as I can
tell, or I misunderstand the design, that could well be the case as
well.

>
> Which would mean that unless when log.diffMerges or --diff-merges
> say off/no, and unless there is any option to specify how the result
> of comparison should bepresented on the command line:
>
>  - when log.diffMerges or --diff-merges say cc or c, default to --cc
>    or -c.
>
>  - otherwise,default to --patch.
>
> is what I think should happen.  But the reason I think so is not
> because "--cc" and "-c" gives output without "-m" (i.e. "-p" does
> not imply "-m" and it should not).

I don't like this so far. Considering -m to be just one of different
formats to represent merge commits (among -c and --cc), as it has always
been, looks more straightforward and useful to me.

Besides, all the recent design I authored assumed -m to be just that,
one of multiple ways to specify how to represent merge commits, the
other 2 being -c and --cc. If we decide to change this view, it'd likely
need significant re-design, and I'm yet to see any actual advantages.

If, on the other hand, it's just me who fundamentally misunderstands the
design, then I need to be corrected fast, before I make significant
damage.

Thanks,

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> No, I don't mean it. The idea is to let -m be alias for
> "--diff-merges=on -p",...

Ahhhh, that makes a whole lot of difference.  Thanks.

> If, on the other hand, it's just me who fundamentally misunderstands the
> design, then I need to be corrected fast, before I make significant
> damage.

No, it was I who was confused, as I somehow incorrectly thoguht that
your plan was to make "-m" identical to "--diff-merges=on".

But if your plan is to make

    "git log -m"    (no other option)
    "git log -m -p"

behave identically to "git log --diff-merges=on -p", and similarly
make

    "git log -m --stat"
    "git log -m --raw"

behave identically to "git log --diff-merges=on --stat/--raw", I
think that such a design makes quite a lot of sense.

It will still keep the purity of "--diff-merges=<choice>" (that is,
it only is about if/how a merge is expressed in some form of diff),
while solving the longstanding usability issue of "-m" that led to
Alex's "when a user says -m, diff output is expected", that came
quite early in this thread.

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> No, I don't mean it. The idea is to let -m be alias for
>> "--diff-merges=on -p",...
>
> Ahhhh, that makes a whole lot of difference.  Thanks.
>
>> If, on the other hand, it's just me who fundamentally misunderstands the
>> design, then I need to be corrected fast, before I make significant
>> damage.
>
> No, it was I who was confused, as I somehow incorrectly thoguht that
> your plan was to make "-m" identical to "--diff-merges=on".
>
> But if your plan is to make
>
>     "git log -m"    (no other option)
>     "git log -m -p"
>
> behave identically to "git log --diff-merges=on -p",

Yep, exactly.

> and similarly make
>
>     "git log -m --stat"
>     "git log -m --raw"
>
> behave identically to "git log --diff-merges=on --stat/--raw", I
> think that such a design makes quite a lot of sense.

These I honestly didn't even think about before, but now, after you've
brought them, I'll pay attention, thanks!

>
> It will still keep the purity of "--diff-merges=<choice>" (that is,
> it only is about if/how a merge is expressed in some form of diff),
> while solving the longstanding usability issue of "-m" that led to
> Alex's "when a user says -m, diff output is expected", that came
> quite early in this thread.

It's nice we've reached mutual understanding!

The only remaining issue then is if we just go and do the change of -m
semantics, or do we need to take some backward compatibility measures?
Looks like we are rather safe to just go, as it's unlikely there will be
any real breakage. What do you think?

Thanks,
-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> It's nice we've reached mutual understanding!

Yes, and thanks for correcting me.

> The only remaining issue then is if we just go and do the change of -m
> semantics, or do we need to take some backward compatibility measures?
> Looks like we are rather safe to just go, as it's unlikely there will be
> any real breakage. What do you think?

I still wish I could come up with the usual backward compatibility
transition dance for this case, but I do not think there is one.

However.

If "-m" were doing a more useful thing than "compare with each
parent separately", people may have aliased "log -m" to something so
that their "git aliased-log" and "git aliased-log -p" would work
better for them than "git log" and "git log -p", but quite honestly,
I do not think "git log -m -p" output is readable by humans (after
all, that is why we invented -c and --cc), so the population that
get hit by this incompatible change may be very tiny minority in
relative terms.

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

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

F> Sergey Organov <sorganov@gmail.com> writes:
>
>> It's nice we've reached mutual understanding!
>
> Yes, and thanks for correcting me.
>
>> The only remaining issue then is if we just go and do the change of -m
>> semantics, or do we need to take some backward compatibility measures?
>> Looks like we are rather safe to just go, as it's unlikely there will be
>> any real breakage. What do you think?
>
> I still wish I could come up with the usual backward compatibility
> transition dance for this case, but I do not think there is one.

Fine, thanks, so I'll prepare and submit a patch.

>
> However.
>
> If "-m" were doing a more useful thing than "compare with each
> parent separately", people may have aliased "log -m" to something so
> that their "git aliased-log" and "git aliased-log -p" would work
> better for them than "git log" and "git log -p", but quite honestly,
> I do not think "git log -m -p" output is readable by humans (after
> all, that is why we invented -c and --cc), so the population that
> get hit by this incompatible change may be very tiny minority in
> relative terms.

Well, honestly, I can't even come up with an alias that would break by
this change, but it's likely I'm just not creative enough :)

Thanks,
-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Alex Henrie]

On Thu, May 6, 2021 at 2:48 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> F> Sergey Organov <sorganov@gmail.com> writes:
> >
> >> It's nice we've reached mutual understanding!
> >
> > Yes, and thanks for correcting me.
> >
> >> The only remaining issue then is if we just go and do the change of -m
> >> semantics, or do we need to take some backward compatibility measures?
> >> Looks like we are rather safe to just go, as it's unlikely there will be
> >> any real breakage. What do you think?
> >
> > I still wish I could come up with the usual backward compatibility
> > transition dance for this case, but I do not think there is one.
>
> Fine, thanks, so I'll prepare and submit a patch.

Thanks guys! I am so glad you were able to reach a consensus!

-Alex

Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov]

Alex Henrie <alexhenrie24@gmail.com> writes:

> I read the following in `man git-log` today:
>
> --diff-merges=separate, --diff-merges=m, -m
>     This makes merge commits show the full diff with respect to each of
>     the parents. Separate log entry and diff is generated for each
>     parent.  -m doesn't produce any output without -p.
>
> --diff-merges=combined, --diff-merges=c, -c
>     With this option, diff output for a merge commit shows the
>     differences from each of the parents to the merge result
>     simultaneously instead of showing pairwise diff between a parent and
>     the result one at a time. Furthermore, it lists only files which
>     were modified from all parents.  -c implies -p.
>
> --diff-merges=dense-combined, --diff-merges=cc, --cc
>     With this option the output produced by --diff-merges=combined is
>     further compressed by omitting uninteresting hunks whose contents
>     in the parents have only two variants and the merge result picks one
>     of them without modification.  --cc implies -p.
>
> Why do -c and -cc imply -p, but -m does not? I tried to use both `git
> log -c` and `git log -m` today and was confused when the latter didn't
> produce any output. Could we change this behavior in a future version
> of Git?

Patches to fix this are almost ready, but I'd like to make a warning
that you'd likely be even more confused by the current output of "-m",
unless you set "log.diffMerges" configuration option to "first-parent".
These diffs with respect to /second/ parent at the end of the output
made me really mad once upon a time.

-- Sergey Organov

Re: Why doesn't `git log -m` imply `-p`?

[Alex Henrie]

On Mon, May 10, 2021 at 6:11 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > I read the following in `man git-log` today:
> >
> > --diff-merges=separate, --diff-merges=m, -m
> >     This makes merge commits show the full diff with respect to each of
> >     the parents. Separate log entry and diff is generated for each
> >     parent.  -m doesn't produce any output without -p.
> >
> > --diff-merges=combined, --diff-merges=c, -c
> >     With this option, diff output for a merge commit shows the
> >     differences from each of the parents to the merge result
> >     simultaneously instead of showing pairwise diff between a parent and
> >     the result one at a time. Furthermore, it lists only files which
> >     were modified from all parents.  -c implies -p.
> >
> > --diff-merges=dense-combined, --diff-merges=cc, --cc
> >     With this option the output produced by --diff-merges=combined is
> >     further compressed by omitting uninteresting hunks whose contents
> >     in the parents have only two variants and the merge result picks one
> >     of them without modification.  --cc implies -p.
> >
> > Why do -c and -cc imply -p, but -m does not? I tried to use both `git
> > log -c` and `git log -m` today and was confused when the latter didn't
> > produce any output. Could we change this behavior in a future version
> > of Git?
>
> Patches to fix this are almost ready, but I'd like to make a warning
> that you'd likely be even more confused by the current output of "-m",
> unless you set "log.diffMerges" configuration option to "first-parent".
> These diffs with respect to /second/ parent at the end of the output
> made me really mad once upon a time.

Good observation. I'm not sure what would be the ideal default here,
but certainly having -m turn on /some/ kind of diff is better than
nothing. Thanks!

-Alex

[PATCH 0/6] diff-merges: let -m imply -p

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p, on
one side, and -m that did not imply -p, on the other side.

After these patches

  git log -m

will start to produce diffs without need to provide -p as well, that
improves both consistency and usability. It gets even more useful if
one sets "log.diffMerges" configuration variable to "first-parent" to
force -m produce usual diff with respect to first parent only.

Previous semantics of -m could still be accessed using
--diff-merges=separate option.

First 5 patches in the series are refactoring that is not expected to
change existing behavior, and the last one is the actual functional
change.

Sergey Organov (6):
  t4013: add test for "git diff-index -m"
  diff-merges: move specific diff-index "-m" handling to diff-index
  git-svn: stop passing "-m" to "git rev-list"
  stash list: stop passing "-m" to "git list"
  diff-merges: rename "combined_imply_patch" to "merges_imply_patch"
  diff-merges: let -m imply -p

 Documentation/diff-options.txt |  8 +++---
 builtin/diff-index.c           | 45 ++++++++++++++++++++++++++--------
 builtin/stash.c                |  2 +-
 diff-merges.c                  | 24 ++++++------------
 perl/Git/SVN.pm                |  2 +-
 revision.h                     |  2 +-
 t/t3903-stash.sh               |  2 +-
 t/t4013-diff-various.sh        | 13 ++++++++++
 8 files changed, 63 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH 1/6] t4013: add test for "git diff-index -m"

[Sergey Organov]

-m in "git diff-index" means "match missing", that differs
from its meaning in "git diff". Let's check it in diff-index.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 87def81699bf..bd16d4885cfd 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -483,6 +483,19 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '
 	test_cmp expected actual
 '
 
+# -m in "git diff-index" means "match missing", that differs
+# from its meaning in "git diff". Let's check it in diff-index.
+# The line in the output for removed file should disappear when
+# we provide -m in diff-index.
+test_expect_success 'git diff-index -m' '
+	rm -f file1 &&
+	git diff-index HEAD >without-m &&
+	lines_count=$(wc -l <without-m) &&
+	git diff-index -m HEAD >with-m &&
+	git restore file1 &&
+	test_line_count = $((lines_count - 1)) with-m
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

[PATCH 2/6] diff-merges: move specific diff-index "-m" handling to diff-index

[Sergey Organov]

Move specific handling of "-m" for diff-index to diff-index.c, so
diff-merges is left to handle only diff for merges options.

Being a better design by itself, this is especially essential in
preparation for letting -m imply -p, as "diff-index -m" obviously
should not imply -p, as it's entirely unrelated.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 builtin/diff-index.c | 45 ++++++++++++++++++++++++++++++++++----------
 diff-merges.c        | 13 +------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 176fe7ff2b4e..28bc51d0d8f4 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -12,11 +12,37 @@ static const char diff_cache_usage[] =
 "[<common-diff-options>] <tree-ish> [<path>...]"
 COMMON_DIFF_OPTIONS_HELP;
 
+static int parse_distinct_options(int argc, const char **argv,
+				  struct rev_info *revs, unsigned int *options)
+{
+	int i, left;
+
+	for (i = left = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		int leave = 0;
+
+		if (!strcmp(arg, "--cached"))
+			*options |= DIFF_INDEX_CACHED;
+		else if (!strcmp(arg, "--merge-base"))
+			*options |= DIFF_INDEX_MERGE_BASE;
+		else if (!strcmp(arg, "-m"))
+			revs->match_missing = 1;
+		else
+			leave = 1;
+
+		if (leave)
+			argv[left++] = arg;
+	}
+
+	argv[left] = NULL;
+
+	return left;
+}
+
 int cmd_diff_index(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
 	unsigned int option = 0;
-	int i;
 	int result;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -27,17 +53,16 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	rev.abbrev = 0;
 	prefix = precompose_argv_prefix(argc, argv, prefix);
 
+	/*
+	 * It's essential to parse our distinct options before calling
+	 * setup_revisions(), for the latter not to see "-m".
+	 */
+	argc = parse_distinct_options(argc, argv, &rev, &option);
 	argc = setup_revisions(argc, argv, &rev, NULL);
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
 
-		if (!strcmp(arg, "--cached"))
-			option |= DIFF_INDEX_CACHED;
-		else if (!strcmp(arg, "--merge-base"))
-			option |= DIFF_INDEX_MERGE_BASE;
-		else
-			usage(diff_cache_usage);
-	}
+	if (argc > 1)
+		usage(diff_cache_usage);
+
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 
diff --git a/diff-merges.c b/diff-merges.c
index f3a9daed7e05..4016800c422c 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -30,17 +30,6 @@ static void set_first_parent(struct rev_info *revs)
 	revs->first_parent_merges = 1;
 }
 
-static void set_m(struct rev_info *revs)
-{
-	/*
-	 * To "diff-index", "-m" means "match missing", and to the "log"
-	 * family of commands, it means "show default diff for merges". Set
-	 * both fields appropriately.
-	 */
-	set_to_default(revs);
-	revs->match_missing = 1;
-}
-
 static void set_combined(struct rev_info *revs)
 {
 	suppress(revs);
@@ -108,7 +97,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	const char *arg = argv[0];
 
 	if (!strcmp(arg, "-m")) {
-		set_m(revs);
+		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->combined_imply_patch = 1;
-- 
2.25.1

Re: [PATCH 2/6] diff-merges: move specific diff-index "-m" handling to diff-index

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>  int cmd_diff_index(int argc, const char **argv, const char *prefix)
>  {
>  	struct rev_info rev;
>  	unsigned int option = 0;
> -	int i;
>  	int result;
>  
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
> @@ -27,17 +53,16 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>  	rev.abbrev = 0;
>  	prefix = precompose_argv_prefix(argc, argv, prefix);
>  
> +	/*
> +	 * It's essential to parse our distinct options before calling
> +	 * setup_revisions(), for the latter not to see "-m".
> +	 */
> +	argc = parse_distinct_options(argc, argv, &rev, &option);
>  	argc = setup_revisions(argc, argv, &rev, NULL);

This change is risky, as the loop below (which this patch moves to
parse_distinct_options()) has no knowledge of other options that
setup_revisions() helper is prepared to handle and that takes an
argument.  When parsing "git cmd --opt --cached A", setup_revisions()
may know that --opt takes an argument and eat both (i.e. the
"--cached" is not an option but an arg given to "--opt"), but the
new parse_distinct_options() helper does not; it will happily skip
"--opt" and leave it in, mistake "--cached" as an option and remove,
and instead make "A" the arg given to "--opt".

Picking up the remnant _after_ setup_revisions() ate what it
understands would not have such a downside, as long as none of our
"distinct options" take any argument.

Can't we make "-m means something special for diff-index" without
butchering the command line processing in this step?  diff-index
does not care about --diff-merges, so letting setup_revisions()
remember only the fact that "-m" was given while parsing, and then
postprocess what "-m" means depending on the command (i.e. everybody
else would treat it as a short-hand for "--diff-merges=m" plus "we
need some form of diff output, while allowing "diff-index" to treat
it differently) should not be rocket science.

Thanks.

Re: [PATCH 2/6] diff-merges: move specific diff-index "-m" handling to diff-index

[Junio C Hamano]

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

> This change is risky, as the loop below (which this patch moves to
> parse_distinct_options()) has no knowledge of other options that
> setup_revisions() helper is prepared to handle and that takes an
> argument.  When parsing "git cmd --opt --cached A", setup_revisions()
> may know that --opt takes an argument and eat both (i.e. the
> "--cached" is not an option but an arg given to "--opt"), but the
> new parse_distinct_options() helper does not; it will happily skip
> "--opt" and leave it in, mistake "--cached" as an option and remove,
> and instead make "A" the arg given to "--opt".

The above is not theoretical.  The series breaks

    $ git checkout master
    $ git diff-index -S --cached maint

IOW, what I predicted in the previous message with s/--opt/-S/
happens with this step.

> Picking up the remnant _after_ setup_revisions() ate what it
> understands would not have such a downside, as long as none of our
> "distinct options" take any argument.
>
> Can't we make "-m means something special for diff-index" without
> butchering the command line processing in this step?  diff-index
> does not care about --diff-merges, so letting setup_revisions()
> remember only the fact that "-m" was given while parsing, and then
> postprocess what "-m" means depending on the command (i.e. everybody
> else would treat it as a short-hand for "--diff-merges=m" plus "we
> need some form of diff output, while allowing "diff-index" to treat
> it differently) should not be rocket science.
>
> Thanks.

Re: [PATCH 2/6] diff-merges: move specific diff-index "-m" handling to diff-index

[Junio C Hamano]

I'll insert the following immediately after this step as a reminder
for ourselves while queuing.

---- >8 -------- >8 -------- >8 -------- >8 -------- >8 ----
Subject: [PATCH] t4062: diff-index -S can take its string as a separate arg

Butchering the command line parser in a wrong way can break this
test, confusing the parser to mistake "--cached" which is a mere
argument to the "-S" option as "compare the tree with the index"
option.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4062-diff-pickaxe.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh
index 1130c8019b..1c93812378 100755
--- a/t/t4062-diff-pickaxe.sh
+++ b/t/t4062-diff-pickaxe.sh
@@ -26,4 +26,9 @@ test_expect_success '-S --pickaxe-regex' '
 	verbose test 4096-zeroes.txt = "$(cat out)"
 '
 
+test_expect_failure '-S with separate option should not error out' '
+	# "--cached" here is not an option---it is an arg to "-S"
+	git diff-index -S --cached HEAD^
+'
+
 test_done
-- 
2.31.1-734-g054d640baf

Re: [PATCH 2/6] diff-merges: move specific diff-index "-m" handling to diff-index

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>  int cmd_diff_index(int argc, const char **argv, const char *prefix)
>>  {
>>  	struct rev_info rev;
>>  	unsigned int option = 0;
>> -	int i;
>>  	int result;
>>  
>>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>> @@ -27,17 +53,16 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>>  	rev.abbrev = 0;
>>  	prefix = precompose_argv_prefix(argc, argv, prefix);
>>  
>> +	/*
>> +	 * It's essential to parse our distinct options before calling
>> +	 * setup_revisions(), for the latter not to see "-m".
>> +	 */
>> +	argc = parse_distinct_options(argc, argv, &rev, &option);
>>  	argc = setup_revisions(argc, argv, &rev, NULL);
>
> This change is risky, as the loop below (which this patch moves to
> parse_distinct_options()) has no knowledge of other options that
> setup_revisions() helper is prepared to handle and that takes an
> argument.  When parsing "git cmd --opt --cached A", setup_revisions()
> may know that --opt takes an argument and eat both (i.e. the
> "--cached" is not an option but an arg given to "--opt"), but the
> new parse_distinct_options() helper does not; it will happily skip
> "--opt" and leave it in, mistake "--cached" as an option and remove,
> and instead make "A" the arg given to "--opt".
>
> Picking up the remnant _after_ setup_revisions() ate what it
> understands would not have such a downside, as long as none of our
> "distinct options" take any argument.
>
> Can't we make "-m means something special for diff-index" without
> butchering the command line processing in this step?  diff-index
> does not care about --diff-merges, so letting setup_revisions()
> remember only the fact that "-m" was given while parsing, and then
> postprocess what "-m" means depending on the command (i.e. everybody
> else would treat it as a short-hand for "--diff-merges=m" plus "we
> need some form of diff output, while allowing "diff-index" to treat
> it differently) should not be rocket science.

I have already considered a few ways of doing it, and what I came up
with looked least destructive to me at the moment, especially as it
broke no tests whatsoever.

I'll now re-consider my approach because of your observations, thanks!

-- Sergey Organov

[PATCH 3/6] git-svn: stop passing "-m" to "git rev-list"

[Sergey Organov]

rev-list doesn't utilize -m. It happens to eat it silently, so this
bug went unnoticed.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 perl/Git/SVN.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index f6f1dc03c608..35ff5a68963d 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1636,7 +1636,7 @@ sub has_no_changes {
 	my $commit = shift;
 
 	my @revs = split / /, command_oneline(
-		qw(rev-list --parents -1 -m), $commit);
+		qw(rev-list --parents -1), $commit);
 
 	# Commits with no parents, e.g. the start of a partial branch,
 	# have changes by definition.
-- 
2.25.1

[PATCH 4/6] stash list: stop passing "-m" to "git list"

[Sergey Organov]

Passing "-m" in "git list --first-parent -m" is not needed as
--first-parent implies --diff-merges=first-parent anyway. OTOH, it
will stop being harmless once we let "-m" imply "-p".

While we are at it, fix corresponding test description in t3903-stash
to match what it actually tests.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 builtin/stash.c  | 2 +-
 t/t3903-stash.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index d68ed784d2af..fe8e97428808 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -761,7 +761,7 @@ static int list_stash(int argc, const char **argv, const char *prefix)
 
 	cp.git_cmd = 1;
 	strvec_pushl(&cp.args, "log", "--format=%gd: %gs", "-g",
-		     "--first-parent", "-m", NULL);
+		     "--first-parent", NULL);
 	strvec_pushv(&cp.args, argv);
 	strvec_push(&cp.args, ref_stash);
 	strvec_push(&cp.args, "--");
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5f282ecf6175..873aa56e359d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -859,7 +859,7 @@ test_expect_success 'setup stash with index and worktree changes' '
 	git stash
 '
 
-test_expect_success 'stash list implies --first-parent -m' '
+test_expect_success 'stash list -p shows simple diff' '
 	cat >expect <<-EOF &&
 	stash@{0}
 
-- 
2.25.1

[PATCH 5/6] diff-merges: rename "combined_imply_patch" to "merges_imply_patch"

[Sergey Organov]

This is refactoring change in preparation for the next commit that
will let -m imply -p.

The old name doesn't match the intention to let not only -c/-cc imply
-p, but also -m, that is not a "combined" format, so we rename the
flag accordingly.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 10 +++++-----
 revision.h    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 4016800c422c..211c99482cac 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -14,7 +14,7 @@ static void suppress(struct rev_info *revs)
 	revs->combine_merges = 0;
 	revs->dense_combined_merges = 0;
 	revs->combined_all_paths = 0;
-	revs->combined_imply_patch = 0;
+	revs->merges_imply_patch = 0;
 	revs->merges_need_diff = 0;
 }
 
@@ -100,10 +100,10 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--cc")) {
 		set_dense_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
 		suppress(revs);
 	} else if (!strcmp(arg, "--combined-all-paths")) {
@@ -150,9 +150,9 @@ void diff_merges_setup_revs(struct rev_info *revs)
 		revs->first_parent_merges = 0;
 	if (revs->combined_all_paths && !revs->combine_merges)
 		die("--combined-all-paths makes no sense without -c or --cc");
-	if (revs->combined_imply_patch)
+	if (revs->merges_imply_patch)
 		revs->diff = 1;
-	if (revs->combined_imply_patch || revs->merges_need_diff) {
+	if (revs->merges_imply_patch || revs->merges_need_diff) {
 		if (!revs->diffopt.output_format)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 	}
diff --git a/revision.h b/revision.h
index 93aa012f518e..17698cb51aca 100644
--- a/revision.h
+++ b/revision.h
@@ -193,10 +193,10 @@ struct rev_info {
 			/* Diff-merge flags */
 			explicit_diff_merges: 1,
 			merges_need_diff: 1,
+			merges_imply_patch:1,
 			separate_merges: 1,
 			combine_merges:1,
 			combined_all_paths:1,
-			combined_imply_patch:1,
 			dense_combined_merges:1,
 			first_parent_merges:1;
 
-- 
2.25.1

[PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p, on
one side, and -m that did not imply -p, on the other side.

After this patch

  git log -m

will start to produce diffs without need to provide -p as well, that
improves both consistency and usability. It gets even more useful if
one sets "log.diffMerges" configuration variable to "first-parent" to
force -m produce usual diff with respect to first parent only.

Previous semantics of -m could still be accessed using
--diff-merges=separate option.

Change documentation accordingly.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt | 8 ++++----
 diff-merges.c                  | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 530d1159141f..32e6dee5ac3b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -49,10 +49,9 @@ ifdef::git-log[]
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
-	the default format. `-m` will produce the output only if `-p`
-	is given as well. The default format could be changed using
+	the default format. The default format could be changed using
 	`log.diffMerges` configuration parameter, which default value
-	is `separate`.
+	is `separate`. `-m` implies `-p`.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -62,7 +61,8 @@ ifdef::git-log[]
 --diff-merges=separate:::
 	This makes merge commits show the full diff with respect to
 	each of the parents. Separate log entry and diff is generated
-	for each parent.
+	for each parent. This is the format that `-m` produced
+	historically.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
diff --git a/diff-merges.c b/diff-merges.c
index 211c99482cac..a827482a97ff 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -98,6 +98,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!strcmp(arg, "-m")) {
 		set_to_default(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
-- 
2.25.1

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Fix long standing inconsistency between -c/--cc that do imply -p, on
> one side, and -m that did not imply -p, on the other side.
>
> After this patch
>
>   git log -m
>
> will start to produce diffs without need to provide -p as well, that
> improves both consistency and usability. It gets even more useful if
> one sets "log.diffMerges" configuration variable to "first-parent" to
> force -m produce usual diff with respect to first parent only.

Please make sure that you clearly state that you do not blindly
force --patch output in the proposed log message.  Explicitly
mentioning that "git log --stat -m" would not give a patch but just
diffstat would be assuring.

Also this needs a test to ensure that is what happens.  Having a
test for "log -m" and another for "log -m --stat" would be
sufficient.

And in the context of this step, the rename of the member in the
previous step makes quite a lot of sense.

Thanks for working on this topic.

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Junio C Hamano]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>> one side, and -m that did not imply -p, on the other side.
>>
>> After this patch
>>
>>   git log -m
>>
>> will start to produce diffs without need to provide -p as well, that
>> improves both consistency and usability. It gets even more useful if
>> one sets "log.diffMerges" configuration variable to "first-parent" to
>> force -m produce usual diff with respect to first parent only.
>
> Please make sure that you clearly state that you do not blindly
> force --patch output in the proposed log message.  Explicitly
> mentioning that "git log --stat -m" would not give a patch but just
> diffstat would be assuring.

Also, avoid "-p" in the title.  "let -m imply diff generation" might
be a good compromise.

What --cc/-c implies is to show some kind of diff for merges
(dense_combined_merges, combine_merges and !ignore_merges are the
members of the revs field that controls how merge commits) and they
ask for specific kind of diff is shown.  So "-c/--cc imply -p" is
quite wrong (you never get straight --patch output for merges when
you give -c/--cc---you get combined diff).  In a sense, you could
say -c/--cc implies -m (i.e. do show some kind of diff for merges).

Taken together, perhaps:

    Subject: diff: let -m imply diff generation

    The "-c/--cc" options to "git log" asks for merges to be shown
    with patch-like output, implicitly enabling the "-m" option
    (which is used to tell "do not ignore merge commits when showing
    patches).  However, the opposite is not true; giving "-m" alone
    does not tell "git log" that the user wants some form of patches.
    
    Make "-m" imply "we want some form of diff output", so that "git
    log -m" would behave identically to "git log -m -p".  When the
    user explicitly asks for what kind of diff output is desired,
    e.g. "git log -m --stat", there is no need to imply anything,
    specifically, do NOT blindly turn on the "-p: option to turn it
    into "git log -m --stat -p:.

or something like that.

If we enable "some kind of diff" for "-m", I actually think that by
default "git log -m" should be turned into "log --cc".  As you told
Alex in your response, "log -m -p" is a quite unpleasant format to
read---it is there only because it was the only thing we had before
we invented "-c/--cc".

But that might be outside the scope of this series.  I dunno, but if
there is no other constraints (like backward compatibility issues),
I have a moderately strong preference to use "--cc" over "-m -p"
from the get go for unconfigured people, rather than forcing
everybody to configure

> Also this needs a test to ensure that is what happens.  Having a
> test for "log -m" and another for "log -m --stat" would be
> sufficient.
>
> And in the context of this step, the rename of the member in the
> previous step makes quite a lot of sense.
>
> Thanks for working on this topic.

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

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

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

[...]

> If we enable "some kind of diff" for "-m", I actually think that by
> default "git log -m" should be turned into "log --cc".  As you told
> Alex in your response, "log -m -p" is a quite unpleasant format to
> read---it is there only because it was the only thing we had before
> we invented "-c/--cc".

Please, no! --cc has unfortunate feature of outputting exactly nothing
for a lot of merge commits, causing even more confusion than historical
"-m -p" format.

The best default for -m output is --diff-merges=first-parent. Everybody
is familiar with it, and it's useful.

> But that might be outside the scope of this series.  I dunno, but if
> there is no other constraints (like backward compatibility issues),
> I have a moderately strong preference to use "--cc" over "-m -p"
> from the get go for unconfigured people, rather than forcing
> everybody to configure

I rather have strong preference for --diff-merges=first-parent. --cc is
only suitable for Git experts, and they know how to get what they want
anyway. Yep, by using --cc. Why spare yet another short option for that?

Overall, let's rather make -m give diff to the first parent by default.
Simple. Useful. Not confusing.

Thanks,
-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Alex Henrie]

On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
> > If we enable "some kind of diff" for "-m", I actually think that by
> > default "git log -m" should be turned into "log --cc".  As you told
> > Alex in your response, "log -m -p" is a quite unpleasant format to
> > read---it is there only because it was the only thing we had before
> > we invented "-c/--cc".
>
> Please, no! --cc has unfortunate feature of outputting exactly nothing
> for a lot of merge commits, causing even more confusion than historical
> "-m -p" format.
>
> The best default for -m output is --diff-merges=first-parent. Everybody
> is familiar with it, and it's useful.
>
> > But that might be outside the scope of this series.  I dunno, but if
> > there is no other constraints (like backward compatibility issues),
> > I have a moderately strong preference to use "--cc" over "-m -p"
> > from the get go for unconfigured people, rather than forcing
> > everybody to configure
>
> I rather have strong preference for --diff-merges=first-parent. --cc is
> only suitable for Git experts, and they know how to get what they want
> anyway. Yep, by using --cc. Why spare yet another short option for that?
>
> Overall, let's rather make -m give diff to the first parent by default.
> Simple. Useful. Not confusing.

Honestly --diff-merges=separate is fine. Two weeks ago, when I started
this discussion, I was trying to use `git log -m` and `git show -m` to
find which merge commit introduced a particular change. Extremely
verbose diff output would have been great for that, the confusing part
was just that `git show -m` produced diff output and `git log -m` did
not.

Maybe what we really want is a new short option like `git log -m1`
which would both enable diff output and set --diff-merges=1.

But again, I don't have a strong opinion on which particular diff
output is "the best", so I'm happy to leave that decision to the
experts.

-Alex

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Alex Henrie <alexhenrie24@gmail.com> writes:

> On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Junio C Hamano <gitster@pobox.com> writes:
>>
>> [...]
>>
>> > If we enable "some kind of diff" for "-m", I actually think that by
>> > default "git log -m" should be turned into "log --cc".  As you told
>> > Alex in your response, "log -m -p" is a quite unpleasant format to
>> > read---it is there only because it was the only thing we had before
>> > we invented "-c/--cc".
>>
>> Please, no! --cc has unfortunate feature of outputting exactly nothing
>> for a lot of merge commits, causing even more confusion than historical
>> "-m -p" format.
>>
>> The best default for -m output is --diff-merges=first-parent. Everybody
>> is familiar with it, and it's useful.
>>
>> > But that might be outside the scope of this series.  I dunno, but if
>> > there is no other constraints (like backward compatibility issues),
>> > I have a moderately strong preference to use "--cc" over "-m -p"
>> > from the get go for unconfigured people, rather than forcing
>> > everybody to configure
>>
>> I rather have strong preference for --diff-merges=first-parent. --cc is
>> only suitable for Git experts, and they know how to get what they want
>> anyway. Yep, by using --cc. Why spare yet another short option for that?
>>
>> Overall, let's rather make -m give diff to the first parent by default.
>> Simple. Useful. Not confusing.
>
> Honestly --diff-merges=separate is fine. Two weeks ago, when I started
> this discussion, I was trying to use `git log -m` and `git show -m` to
> find which merge commit introduced a particular change. Extremely
> verbose diff output would have been great for that, the confusing part
> was just that `git show -m` produced diff output and `git log -m` did
> not.

This is not a case in favor of "separate" over "first-parent" as the
default for "-m", right?

"Which merge commit introduced particular change" is exactly what
--diff-merges=1 achieves, so "--diff-merges=separate" was not in fact
needed, as I see it. Moreover, it could have produced wrong positives.
Looks like --diff-merges=1 is a better fit.

> Maybe what we really want is a new short option like `git log -m1`
> which would both enable diff output and set --diff-merges=1.

Hopefully this will be simply "-m" soon. "-m1" is no-go as optional
arguments to short options is a bad idea. It could have been "--m1", but
I believe that's not needed.

>
> But again, I don't have a strong opinion on which particular diff
> output is "the best", so I'm happy to leave that decision to the
> experts.

There is no "the best", and at least "first-parent" and "dense-combined"
are to survive, and "dense-combined" already has its "--cc" rather short
variant, so it's logical to give -m the other one, especially as it
already has this meaning when --first-parent is provided as well.

Also, I'm not sure if -c is being in use, and if it isn't, then it could
be changed to produce dense-combined format, especially as one still
have --diff-merges=condensed nowadays anyway, so that -m and -c will
finally give 2 most useful formats.

Overall, I still find a lot of sense in giving "-m" exactly first-parent
default meaning.

Thanks,
-- Sergey Organov

P.S. If generic options machinery were in use, it could have been
possible to say:

  git log -pm

reducing the issue to consistency only. I wonder if anybody have plans
to convert setup_revisions() to parse_options() utility?

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Alex Henrie]

On Tue, May 11, 2021 at 12:46 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> [...]
> >>
> >> > If we enable "some kind of diff" for "-m", I actually think that by
> >> > default "git log -m" should be turned into "log --cc".  As you told
> >> > Alex in your response, "log -m -p" is a quite unpleasant format to
> >> > read---it is there only because it was the only thing we had before
> >> > we invented "-c/--cc".
> >>
> >> Please, no! --cc has unfortunate feature of outputting exactly nothing
> >> for a lot of merge commits, causing even more confusion than historical
> >> "-m -p" format.
> >>
> >> The best default for -m output is --diff-merges=first-parent. Everybody
> >> is familiar with it, and it's useful.
> >>
> >> > But that might be outside the scope of this series.  I dunno, but if
> >> > there is no other constraints (like backward compatibility issues),
> >> > I have a moderately strong preference to use "--cc" over "-m -p"
> >> > from the get go for unconfigured people, rather than forcing
> >> > everybody to configure
> >>
> >> I rather have strong preference for --diff-merges=first-parent. --cc is
> >> only suitable for Git experts, and they know how to get what they want
> >> anyway. Yep, by using --cc. Why spare yet another short option for that?
> >>
> >> Overall, let's rather make -m give diff to the first parent by default.
> >> Simple. Useful. Not confusing.
> >
> > Honestly --diff-merges=separate is fine. Two weeks ago, when I started
> > this discussion, I was trying to use `git log -m` and `git show -m` to
> > find which merge commit introduced a particular change. Extremely
> > verbose diff output would have been great for that, the confusing part
> > was just that `git show -m` produced diff output and `git log -m` did
> > not.
>
> This is not a case in favor of "separate" over "first-parent" as the
> default for "-m", right?
>
> "Which merge commit introduced particular change" is exactly what
> --diff-merges=1 achieves, so "--diff-merges=separate" was not in fact
> needed, as I see it. Moreover, it could have produced wrong positives.
> Looks like --diff-merges=1 is a better fit.

I didn't know which branch the change came from. If the change came
from the first branch, it would not have appeared under the merge
commit with --diff-merges=first-parent. But the change would
definitely appear with --diff-merges=separate, which enabled me to
identify the merge commit that included it. So yes, this is a case in
favor of "separate" over "first-parent", but it's probably not a
common enough scenario to demand keeping "separate" for -m.

On Tue, May 11, 2021 at 12:31 PM Elijah Newren <newren@gmail.com> wrote:
>
> Interesting.  I have a strong preference for --diff-merges=remerge
> (yeah, I know it's not upstream, but it's been ready to submit for
> months, but just backed up behind the other ort changes.  Sorry, I
> can't push those through any faster).  I've had others using it for
> about 9 months now.

--diff-merges=remerge is the default I would expect when no options
have been configured or passed on the command line (although it would
not have helped in the scenario I described above). I look forward to
using it!

-Alex

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Alex Henrie <alexhenrie24@gmail.com> writes:

> On Tue, May 11, 2021 at 12:46 PM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> > On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>> >>
>> >> Junio C Hamano <gitster@pobox.com> writes:
>> >>
>> >> > Junio C Hamano <gitster@pobox.com> writes:
>> >>
>> >> [...]
>> >>
>> >> > If we enable "some kind of diff" for "-m", I actually think that by
>> >> > default "git log -m" should be turned into "log --cc".  As you told
>> >> > Alex in your response, "log -m -p" is a quite unpleasant format to
>> >> > read---it is there only because it was the only thing we had before
>> >> > we invented "-c/--cc".
>> >>
>> >> Please, no! --cc has unfortunate feature of outputting exactly nothing
>> >> for a lot of merge commits, causing even more confusion than historical
>> >> "-m -p" format.
>> >>
>> >> The best default for -m output is --diff-merges=first-parent. Everybody
>> >> is familiar with it, and it's useful.
>> >>
>> >> > But that might be outside the scope of this series.  I dunno, but if
>> >> > there is no other constraints (like backward compatibility issues),
>> >> > I have a moderately strong preference to use "--cc" over "-m -p"
>> >> > from the get go for unconfigured people, rather than forcing
>> >> > everybody to configure
>> >>
>> >> I rather have strong preference for --diff-merges=first-parent. --cc is
>> >> only suitable for Git experts, and they know how to get what they want
>> >> anyway. Yep, by using --cc. Why spare yet another short option for that?
>> >>
>> >> Overall, let's rather make -m give diff to the first parent by default.
>> >> Simple. Useful. Not confusing.
>> >
>> > Honestly --diff-merges=separate is fine. Two weeks ago, when I started
>> > this discussion, I was trying to use `git log -m` and `git show -m` to
>> > find which merge commit introduced a particular change. Extremely
>> > verbose diff output would have been great for that, the confusing part
>> > was just that `git show -m` produced diff output and `git log -m` did
>> > not.
>>
>> This is not a case in favor of "separate" over "first-parent" as the
>> default for "-m", right?
>>
>> "Which merge commit introduced particular change" is exactly what
>> --diff-merges=1 achieves, so "--diff-merges=separate" was not in fact
>> needed, as I see it. Moreover, it could have produced wrong positives.
>> Looks like --diff-merges=1 is a better fit.
>
> I didn't know which branch the change came from. If the change came
> from the first branch, it would not have appeared under the merge
> commit with --diff-merges=first-parent.

The change in question either came from this merge, or it didn't. If it
came from this merge, it will be there in --diff-merges=first-parent
even if you have octopus merge and multiple parents, in which case it
will be cumulative change from all the side parents.

> But the change would definitely appear with --diff-merges=separate,
> which enabled me to identify the merge commit that included it.

The second part of --diff-merges=separate, that is absent in
--diff-merges=first-parent, shows the diff that this merge would have
made to the side branch, if you had considered the result of merge the
tip of that branch, and you didn't. This second part just makes no sense
at all when you care about changes to your files in any typical git
workflow, as far as I can tell.

> So yes, this is a case in favor of "separate" over "first-parent", but
> it's probably not a common enough scenario to demand keeping
> "separate" for -m.

To me it looks like it rather only shows how deeply confusing
--diff-merges=separate actually is. I'd just kill it.


Thanks,

-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Felipe Contreras]

Sergey Organov wrote:
> Alex Henrie <alexhenrie24@gmail.com> writes:
> 
> > I didn't know which branch the change came from. If the change came
> > from the first branch, it would not have appeared under the merge
> > commit with --diff-merges=first-parent.
> 
> The change in question either came from this merge, or it didn't. If it
> came from this merge, it will be there in --diff-merges=first-parent
> even if you have octopus merge and multiple parents, in which case it
> will be cumulative change from all the side parents.

Precisely.

The primary thing I care about is what is the status of the branch
*after* the merge. In other words; if I rewind history and I hadn't done
the merge, what would be diffent?

That in my mind is the actual content of the merge.

-- 
Felipe Contreras

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Elijah Newren]

On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
> > If we enable "some kind of diff" for "-m", I actually think that by
> > default "git log -m" should be turned into "log --cc".  As you told
> > Alex in your response, "log -m -p" is a quite unpleasant format to
> > read---it is there only because it was the only thing we had before
> > we invented "-c/--cc".
>
> Please, no! --cc has unfortunate feature of outputting exactly nothing
> for a lot of merge commits, causing even more confusion than historical
> "-m -p" format.
>
> The best default for -m output is --diff-merges=first-parent. Everybody
> is familiar with it, and it's useful.
>
> > But that might be outside the scope of this series.  I dunno, but if
> > there is no other constraints (like backward compatibility issues),
> > I have a moderately strong preference to use "--cc" over "-m -p"
> > from the get go for unconfigured people, rather than forcing
> > everybody to configure
>
> I rather have strong preference for --diff-merges=first-parent. --cc is
> only suitable for Git experts, and they know how to get what they want
> anyway. Yep, by using --cc. Why spare yet another short option for that?

Interesting.  I have a strong preference for --diff-merges=remerge
(yeah, I know it's not upstream, but it's been ready to submit for
months, but just backed up behind the other ort changes.  Sorry, I
can't push those through any faster).  I've had others using it for
about 9 months now.

I think --cc is a lot better than -m for helping you find what users
changed when they did the merge, but I agree the format is somewhat
difficult for many users to understand.  (--diff-merges=remerge, or
--remerge-diff, fixes these problems, IMO.)  I think
--diff-merges=first-parent, while fine when explicitly requested on
the command line, would be wildly misleading as a default because it
would attribute changes to a merge commit that were made elsewhere.

> Overall, let's rather make -m give diff to the first parent by default.
> Simple. Useful. Not confusing.

I think it's confusing.

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Junio C Hamano <gitster@pobox.com> writes:
>>
>> [...]
>>
>> > If we enable "some kind of diff" for "-m", I actually think that by
>> > default "git log -m" should be turned into "log --cc".  As you told
>> > Alex in your response, "log -m -p" is a quite unpleasant format to
>> > read---it is there only because it was the only thing we had before
>> > we invented "-c/--cc".
>>
>> Please, no! --cc has unfortunate feature of outputting exactly nothing
>> for a lot of merge commits, causing even more confusion than historical
>> "-m -p" format.
>>
>> The best default for -m output is --diff-merges=first-parent. Everybody
>> is familiar with it, and it's useful.
>>
>> > But that might be outside the scope of this series.  I dunno, but if
>> > there is no other constraints (like backward compatibility issues),
>> > I have a moderately strong preference to use "--cc" over "-m -p"
>> > from the get go for unconfigured people, rather than forcing
>> > everybody to configure
>>
>> I rather have strong preference for --diff-merges=first-parent. --cc is
>> only suitable for Git experts, and they know how to get what they want
>> anyway. Yep, by using --cc. Why spare yet another short option for that?
>
> Interesting.  I have a strong preference for --diff-merges=remerge
> (yeah, I know it's not upstream, but it's been ready to submit for
> months, but just backed up behind the other ort changes.  Sorry, I
> can't push those through any faster).  I've had others using it for
> about 9 months now.

Once somebody uses it for 9 months and starts to understand what it is
and really loves it, she can still set log.diffMerges=remerge (new
feature) and have fun.

>
> I think --cc is a lot better than -m for helping you find what users
> changed when they did the merge,

Yes, but it doesn't mean it should be the default.

In my workflows, the first thing that matters is what commit did what
changes on the current branch. I don't typically care what the user
changed during the merge operation, only about the result. If I do care,
then only after I find the merge commit is responsible, and I can then
use --cc if I want to.

> but I agree the format is somewhat difficult for many users to
> understand. (--diff-merges=remerge, or --remerge-diff, fixes these
> problems, IMO.) I think --diff-merges=first-parent, while fine when
> explicitly requested on the command line, would be wildly misleading
> as a default because it would attribute changes to a merge commit that
> were made elsewhere.

No, it's exactly this merge commit that made these changes to the
current branch. The changes you refer to have been made on another
branch, and not by this particular merge commit, and we fortunately have
the reference to those commits through the second parent of this one.

>
>> Overall, let's rather make -m give diff to the first parent by default.
>> Simple. Useful. Not confusing.
>
> I think it's confusing.

I think it isn't, once you accept that merge commit does introduce
changes to the branch, by itself.

Thanks,
-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Elijah Newren]

On Tue, May 11, 2021 at 12:00 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> [...]
> >>
> >> > If we enable "some kind of diff" for "-m", I actually think that by
> >> > default "git log -m" should be turned into "log --cc".  As you told
> >> > Alex in your response, "log -m -p" is a quite unpleasant format to
> >> > read---it is there only because it was the only thing we had before
> >> > we invented "-c/--cc".
> >>
> >> Please, no! --cc has unfortunate feature of outputting exactly nothing
> >> for a lot of merge commits, causing even more confusion than historical
> >> "-m -p" format.
> >>
> >> The best default for -m output is --diff-merges=first-parent. Everybody
> >> is familiar with it, and it's useful.
> >>
> >> > But that might be outside the scope of this series.  I dunno, but if
> >> > there is no other constraints (like backward compatibility issues),
> >> > I have a moderately strong preference to use "--cc" over "-m -p"
> >> > from the get go for unconfigured people, rather than forcing
> >> > everybody to configure
> >>
> >> I rather have strong preference for --diff-merges=first-parent. --cc is
> >> only suitable for Git experts, and they know how to get what they want
> >> anyway. Yep, by using --cc. Why spare yet another short option for that?
> >
> > Interesting.  I have a strong preference for --diff-merges=remerge
> > (yeah, I know it's not upstream, but it's been ready to submit for
> > months, but just backed up behind the other ort changes.  Sorry, I
> > can't push those through any faster).  I've had others using it for
> > about 9 months now.
>
> Once somebody uses it for 9 months and starts to understand what it is
> and really loves it, she can still set log.diffMerges=remerge (new
> feature) and have fun.
>
> >
> > I think --cc is a lot better than -m for helping you find what users
> > changed when they did the merge,
>
> Yes, but it doesn't mean it should be the default.

I didn't say it should be.

> In my workflows, the first thing that matters is what commit did what
> changes on the current branch. I don't typically care what the user
> changed during the merge operation, only about the result. If I do care,
> then only after I find the merge commit is responsible, and I can then
> use --cc if I want to.
>
> > but I agree the format is somewhat difficult for many users to
> > understand. (--diff-merges=remerge, or --remerge-diff, fixes these
> > problems, IMO.) I think --diff-merges=first-parent, while fine when
> > explicitly requested on the command line, would be wildly misleading
> > as a default because it would attribute changes to a merge commit that
> > were made elsewhere.
>
> No, it's exactly this merge commit that made these changes to the
> current branch. The changes you refer to have been made on another
> branch, and not by this particular merge commit, and we fortunately have
> the reference to those commits through the second parent of this one.

If you only care about "what introduced these changes to the current
branch", then it's not only the diff against second parent that is
irrelevant: ALL commits that are part of the history only via the
second or later parents are also irrelevant and thus you should be
using --first-parent when asking this question.  That changes both
history traversal and the diff output.  It's a reasonable question,
sure, but I didn't see you suggest that -m should change both.
Perhaps you did mean that, in which case I think that's a proposal
that would make -m become useful.  It'd be a drastically different
option than what -m is today, which would normally make me wonder if
it'd be considered a backward compatibility issue, but given how
useless -m has been and since Junio apparently even suggested changing
-m, this might be okay.

> >> Overall, let's rather make -m give diff to the first parent by default.
> >> Simple. Useful. Not confusing.
> >
> > I think it's confusing.
>
> I think it isn't, once you accept that merge commit does introduce
> changes to the branch, by itself.

Asking what commits introduced changes to the branch is a useful
question, but one which requires changing history traversal.  Unless
making -m imply --first-parent is part of the proposal, I still find
it somewhat confusing.

Granted, "what changes were introduced to the branch?" is certainly
not the only question you might be looking for answers to.  Others
include "what changes were people making in each commit" or "who added
calls to this function and why" (e.g. with git log S$function -p
$maybe_some_merge_diff_flag) or "who caused this function to end up
the way it is" (e.g. with git log -L :<funcname>:<file> -p
$some_diff_merge_flag).  For any of those, a first parent diff is
pretty terrible and highly misleading.  A separate diff is no better.
--cc would often be useful, with some caveats -- the biggest of which
is just how esoteric combined-diffs are.  --remerge-diff is way more
natural for each of these questions I'm coming up with.  Are there
other questions that -m helps us answer where first-parent diffs make
sense and are generally correct without also needing --first-parent?
I'm not thinking of any right now.

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Tue, May 11, 2021 at 12:00 PM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>> > On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>> >>
>> >> Junio C Hamano <gitster@pobox.com> writes:
>> >>
>> >> > Junio C Hamano <gitster@pobox.com> writes:
>> >>
>> >> [...]
>> >>
>> >> > If we enable "some kind of diff" for "-m", I actually think that by
>> >> > default "git log -m" should be turned into "log --cc".  As you told
>> >> > Alex in your response, "log -m -p" is a quite unpleasant format to
>> >> > read---it is there only because it was the only thing we had before
>> >> > we invented "-c/--cc".
>> >>
>> >> Please, no! --cc has unfortunate feature of outputting exactly nothing
>> >> for a lot of merge commits, causing even more confusion than historical
>> >> "-m -p" format.
>> >>
>> >> The best default for -m output is --diff-merges=first-parent. Everybody
>> >> is familiar with it, and it's useful.
>> >>
>> >> > But that might be outside the scope of this series.  I dunno, but if
>> >> > there is no other constraints (like backward compatibility issues),
>> >> > I have a moderately strong preference to use "--cc" over "-m -p"
>> >> > from the get go for unconfigured people, rather than forcing
>> >> > everybody to configure
>> >>
>> >> I rather have strong preference for --diff-merges=first-parent. --cc is
>> >> only suitable for Git experts, and they know how to get what they want
>> >> anyway. Yep, by using --cc. Why spare yet another short option for that?
>> >
>> > Interesting.  I have a strong preference for --diff-merges=remerge
>> > (yeah, I know it's not upstream, but it's been ready to submit for
>> > months, but just backed up behind the other ort changes.  Sorry, I
>> > can't push those through any faster).  I've had others using it for
>> > about 9 months now.
>>
>> Once somebody uses it for 9 months and starts to understand what it is
>> and really loves it, she can still set log.diffMerges=remerge (new
>> feature) and have fun.
>>
>> >
>> > I think --cc is a lot better than -m for helping you find what users
>> > changed when they did the merge,
>>
>> Yes, but it doesn't mean it should be the default.
>
> I didn't say it should be.
>
>> In my workflows, the first thing that matters is what commit did what
>> changes on the current branch. I don't typically care what the user
>> changed during the merge operation, only about the result. If I do care,
>> then only after I find the merge commit is responsible, and I can then
>> use --cc if I want to.
>>
>> > but I agree the format is somewhat difficult for many users to
>> > understand. (--diff-merges=remerge, or --remerge-diff, fixes these
>> > problems, IMO.) I think --diff-merges=first-parent, while fine when
>> > explicitly requested on the command line, would be wildly misleading
>> > as a default because it would attribute changes to a merge commit that
>> > were made elsewhere.
>>
>> No, it's exactly this merge commit that made these changes to the
>> current branch. The changes you refer to have been made on another
>> branch, and not by this particular merge commit, and we fortunately have
>> the reference to those commits through the second parent of this one.
>
> If you only care about "what introduced these changes to the current
> branch", then it's not only the diff against second parent that is
> irrelevant: ALL commits that are part of the history only via the
> second or later parents are also irrelevant and thus you should be
> using --first-parent when asking this question.  That changes both
> history traversal and the diff output.

No, it's exactly why I don't always want --first-parent. I want to
traverse *all* the history, yet to see what's changed by every commit on
*this* branch in the process of traversal, be it a merge on not a merge
commit.

Thanks,

-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>> ...
> I think --cc is a lot better than -m for helping you find what users
> changed when they did the merge, but I agree the format is somewhat
> difficult for many users to understand.  (--diff-merges=remerge, or
> --remerge-diff, fixes these problems, IMO.)  I think
> --diff-merges=first-parent, while fine when explicitly requested on
> the command line, would be wildly misleading as a default because it
> would attribute changes to a merge commit that were made elsewhere.
>
>> Overall, let's rather make -m give diff to the first parent by default.
>> Simple. Useful. Not confusing.
>
> I think it's confusing.

I do not think it is particularly confusing---after all, it shows
"here is a comparison between two trees" the users are familiar with
in a single strand of pearls.

But I do think that it is an utterly misleading option to show
merges in general.  When "log" is used with the "--first-parent"
traversal, "compare with the first parent to show everything the
side branch did" is an acceptable alternative, but even there, it is
far less suitable than the "remerge" or "cc", I would think, as the
default format.

Thanks.

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

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

> Elijah Newren <newren@gmail.com> writes:
>
>> On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote:
>>> ...
>> I think --cc is a lot better than -m for helping you find what users
>> changed when they did the merge, but I agree the format is somewhat
>> difficult for many users to understand.  (--diff-merges=remerge, or
>> --remerge-diff, fixes these problems, IMO.)  I think
>> --diff-merges=first-parent, while fine when explicitly requested on
>> the command line, would be wildly misleading as a default because it
>> would attribute changes to a merge commit that were made elsewhere.
>>
>>> Overall, let's rather make -m give diff to the first parent by default.
>>> Simple. Useful. Not confusing.
>>
>> I think it's confusing.
>
> I do not think it is particularly confusing---after all, it shows
> "here is a comparison between two trees" the users are familiar with
> in a single strand of pearls.

Moreover, it's comparison between two most natural trees, the same trees
as for non-merge commits.

>
> But I do think that it is an utterly misleading option to show
> merges in general.  When "log" is used with the "--first-parent"
> traversal, "compare with the first parent to show everything the
> side branch did" is an acceptable alternative, but even there, it is
> far less suitable than the "remerge" or "cc", I would think, as the
> default format.

What I still fail to see is why somebody wants to mix traversal rules
with representation of merge commits, sorry.

If I want to see merges in --cc, I want them in --cc, no matter which
way I get to particular commit through the DAG. If I want to see them in
--diff-merges=1, I want them this way, no matter how I happened to get
to particular commit.

Is it more likely that I want to see merges in --diff-merges=1 when I
use --first-parent? Yes, of course! That's why --first-parent implies
--diff-merges=1. Vice versa? No, why?

Thanks,

-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> But I do think that it is an utterly misleading option to show
>> merges in general.  When "log" is used with the "--first-parent"
>> traversal, "compare with the first parent to show everything the
>> side branch did" is an acceptable alternative, but even there, it is
>> far less suitable than the "remerge" or "cc", I would think, as the
>> default format.
>
> What I still fail to see is why somebody wants to mix traversal rules
> with representation of merge commits, sorry.

It is not about "mixing".

What I meant was that diff between the pre-merge state and the merge
result as two-tree comparison may be an acceptable view of "what did
this _merge_ contribute to the history" and that is in line is what
the first-parent traversal is trying to show the reader of the
history, which is "this merge contributed to the history, and then
this other merge contributed to the history, ...".  It also may make
sense when one is inspecting a merge in isolation, i.e. "git show M".

When one is viewing individual commits without omitting commits on
the side branch, however, showing "what did the side branch as a
whole contributed to the history at this merge" makes little sense
and is misleading.  Such a patch given for a merge mostly overlaps
with what the reader sees immediately after seeing the merge
(i.e. the commits on the side branch, with their own patches, each
of which has already been shown in the first-parent comparison of
the merge).  In other words, "git log --no-first-parent" is about
contribution of each single commit (whether it is a merge or a
non-merge) makes, and the --diff-merges=1 presentation is not in
line with that worldview, in which the contribution to the history
the merge alone makes is what --cc or remerge tries to show,
i.e. "what did the merge do, on top of what the commits on the side
branch contributed to the history?"

So, it is not like two unrelated things are mixed together.  It is
more like showing the result of a merge as a diff relative to its
first-parent is tied to and consistent with the viewpoint of history
as single strand of pearls that is a series of merges into the
first-parent chain, but not with non-first-parent presentation of
the history.

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Jonathan Nieder]

Hi,

Elijah Newren wrote:

> Interesting.  I have a strong preference for --diff-merges=remerge
> (yeah, I know it's not upstream, but it's been ready to submit for
> months, but just backed up behind the other ort changes.  Sorry, I
> can't push those through any faster).  I've had others using it for
> about 9 months now.

I agree.  The --diff-merges=remerge behavior has also been the default
in Gerrit since time immemorial, and when I first started using Gerrit
it was one of my favorite things about it.

That is because it shows the human content of the merge: it shows
exactly what changes were made after the automated part was done.

I don't have a strong opinion about what the default for -m should be.

> I think --cc is a lot better than -m for helping you find what users
> changed when they did the merge, but I agree the format is somewhat
> difficult for many users to understand.

My experience in training new users has been that --cc is quite
counterintuitive for them.  Like you're hinting, I suspect it's
fundamentally just meant to be a fast approximation to
--diff-merges=remerge.

Thanks,
Jonathan

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Hi,

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi,
>
> Elijah Newren wrote:
>
>> Interesting.  I have a strong preference for --diff-merges=remerge
>> (yeah, I know it's not upstream, but it's been ready to submit for
>> months, but just backed up behind the other ort changes.  Sorry, I
>> can't push those through any faster).  I've had others using it for
>> about 9 months now.
>
> I agree.  The --diff-merges=remerge behavior has also been the default
> in Gerrit since time immemorial, and when I first started using Gerrit
> it was one of my favorite things about it.

Due to previous set of diff-merges patches you now can set
log.diffMerges=remerge and have fun having it as the default for "-m" in
pure Git. I've introduced log.diffMerges exactly because I do realize
that different people and different workflows might have different
preferences here.

> That is because it shows the human content of the merge: it shows
> exactly what changes were made after the automated part was done.

No, in fact it shows what additional changes were to be made if the
merge had been achieved using particular automated merge algorithm you
apply right now. Provided you utilize no external knowledge, you have no
idea how merge has been in fact achieved, as core Git (fortunately)
neither cares nor stores such information.

Contrary to your view, and I believe more inline with Git core
philosophy, I (usually) don't care *how* particular changes were
achieved, I rather care *what* they are, so pure diff to the first
parent is the best basic format for me. Only if I need to see *why* the
changes are this particular way, I'd use advanced tools and formats to
get to the best *guess*, and that's where these advanced formats get
handy.

In other words, for me merge commit is mostly *commit*, and only then
it's *merge*: "here are the changes, and, by the way, here is where
these changes came from."

That said, please don't get me wrong: I'm all for advanced features such
as --diff-merges=remerge, as I do understand how and when they are
useful, I just personally won't make them active by default.

>
> I don't have a strong opinion about what the default for -m should be.
>
>> I think --cc is a lot better than -m for helping you find what users
>> changed when they did the merge, but I agree the format is somewhat
>> difficult for many users to understand.

I do have strong opinion. --diff-merges=1 is the only sensible
factory-default. Not only it has no implicit assumptions about how given
commit has been achieved, it's also the only format even entire Git
newbie might already be familiar with.

Then, --cc and I suspect "remerge" too, give exactly empty output most
of times, that'd cause even more surprises when one of them is the
default: you enable diff for merges, but still get nothing. OTOH,
--diff-merges=1 will give empty output if and only if given commit
actually brings no changes to the branch, exactly as any commit out
there. No surprises at all.

Principle of least surprise is still a good thing to follow.

Thanks,
-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Felipe Contreras]

Sergey Organov wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

> > I don't have a strong opinion about what the default for -m should be.
> >
> >> I think --cc is a lot better than -m for helping you find what users
> >> changed when they did the merge, but I agree the format is somewhat
> >> difficult for many users to understand.
> 
> I do have strong opinion. --diff-merges=1 is the only sensible
> factory-default. Not only it has no implicit assumptions about how given
> commit has been achieved, it's also the only format even entire Git
> newbie might already be familiar with.

I agree.

> Principle of least surprise is still a good thing to follow.

I could not agree more.

-- 
Felipe Contreras

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>>> one side, and -m that did not imply -p, on the other side.
>>>
>>> After this patch
>>>
>>>   git log -m
>>>
>>> will start to produce diffs without need to provide -p as well, that
>>> improves both consistency and usability. It gets even more useful if
>>> one sets "log.diffMerges" configuration variable to "first-parent" to
>>> force -m produce usual diff with respect to first parent only.
>>
>> Please make sure that you clearly state that you do not blindly
>> force --patch output in the proposed log message.  Explicitly
>> mentioning that "git log --stat -m" would not give a patch but just
>> diffstat would be assuring.
>
> Also, avoid "-p" in the title.  "let -m imply diff generation" might
> be a good compromise.
>
> What --cc/-c implies is to show some kind of diff for merges
> (dense_combined_merges, combine_merges and !ignore_merges are the
> members of the revs field that controls how merge commits) and they
> ask for specific kind of diff is shown.  So "-c/--cc imply -p" is
> quite wrong (you never get straight --patch output for merges when
> you give -c/--cc---you get combined diff).  In a sense, you could
> say -c/--cc implies -m (i.e. do show some kind of diff for merges).
>
> Taken together, perhaps:
>
>     Subject: diff: let -m imply diff generation
>
>     The "-c/--cc" options to "git log" asks for merges to be shown
>     with patch-like output, implicitly enabling the "-m" option
>     (which is used to tell "do not ignore merge commits when showing
>     patches).  However, the opposite is not true; giving "-m" alone
>     does not tell "git log" that the user wants some form of patches.
>     
>     Make "-m" imply "we want some form of diff output", so that "git
>     log -m" would behave identically to "git log -m -p".  When the
>     user explicitly asks for what kind of diff output is desired,
>     e.g. "git log -m --stat", there is no need to imply anything,
>     specifically, do NOT blindly turn on the "-p: option to turn it
>     into "git log -m --stat -p:.
>
> or something like that.


Fine with me, will do, thanks!

-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>>> one side, and -m that did not imply -p, on the other side.
>>>
>>> After this patch
>>>
>>>   git log -m
>>>
>>> will start to produce diffs without need to provide -p as well, that
>>> improves both consistency and usability. It gets even more useful if
>>> one sets "log.diffMerges" configuration variable to "first-parent" to
>>> force -m produce usual diff with respect to first parent only.
>>
>> Please make sure that you clearly state that you do not blindly
>> force --patch output in the proposed log message.  Explicitly
>> mentioning that "git log --stat -m" would not give a patch but just
>> diffstat would be assuring.
>
> Also, avoid "-p" in the title.  "let -m imply diff generation" might
> be a good compromise.
>
> What --cc/-c implies is to show some kind of diff for merges
> (dense_combined_merges, combine_merges and !ignore_merges are the
> members of the revs field that controls how merge commits) and they
> ask for specific kind of diff is shown.  So "-c/--cc imply -p" is
> quite wrong (you never get straight --patch output for merges when
> you give -c/--cc---you get combined diff).  In a sense, you could
> say -c/--cc implies -m (i.e. do show some kind of diff for merges).
>
> Taken together, perhaps:
>
>     Subject: diff: let -m imply diff generation
>
>     The "-c/--cc" options to "git log" asks for merges to be shown
>     with patch-like output, implicitly enabling the "-m" option
>     (which is used to tell "do not ignore merge commits when showing
>     patches).  However, the opposite is not true; giving "-m" alone
>     does not tell "git log" that the user wants some form of patches.
>     
>     Make "-m" imply "we want some form of diff output", so that "git
>     log -m" would behave identically to "git log -m -p".  When the
>     user explicitly asks for what kind of diff output is desired,
>     e.g. "git log -m --stat", there is no need to imply anything,
>     specifically, do NOT blindly turn on the "-p: option to turn it
>     into "git log -m --stat -p:.
>
> or something like that.

While working on this, I've added more tests and explanations to the
next re-roll as I've promised, but I didn't change the subject nor did I
try to explain that much, as my original subject is consistent with
current Git documentation, concise, and straight to the point.

"<opt> implies -p" -- that's how current Git manual describes the
behavior, and that's how I described it in these patches. Whatever
"imply" actually means for -c/--cc, it now means for -m as well, and
that's the essence of the patches, so I stand for my original subject.

I think that if we realize that "<opt> implies -p" is not precise enough
description of actual behavior, it should be a separate topic of
documentation improvements, and doesn't belong to these series anyway.

Thanks,
-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>> one side, and -m that did not imply -p, on the other side.
>>
>> After this patch
>>
>>   git log -m
>>
>> will start to produce diffs without need to provide -p as well, that
>> improves both consistency and usability. It gets even more useful if
>> one sets "log.diffMerges" configuration variable to "first-parent" to
>> force -m produce usual diff with respect to first parent only.
>
> Please make sure that you clearly state that you do not blindly
> force --patch output in the proposed log message.  Explicitly
> mentioning that "git log --stat -m" would not give a patch but just
> diffstat would be assuring.
>
> Also this needs a test to ensure that is what happens.  Having a
> test for "log -m" and another for "log -m --stat" would be
> sufficient.

OK, will do, thanks!

-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Jonathan Nieder]

Sergey Organov wrote:

> Fix long standing inconsistency between -c/--cc that do imply -p, on
> one side, and -m that did not imply -p, on the other side.
>
> After this patch
>
>   git log -m
>
> will start to produce diffs without need to provide -p as well,

Personally I don't ever use -m without -p and --first-parent, so in
that sense this feels like a change in the right direction.

Does this also affect the plumbing command "git diff-tree"?  I'm
guessing "no" because diff-tree already generates a diff by default,
but it seems worth spelling out in the commit message to prevent
worries about the effect on scripts that expect stable plumbing
behavior.

Thanks,
Jonathan

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sergey Organov wrote:
>
>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>> one side, and -m that did not imply -p, on the other side.
>>
>> After this patch
>>
>>   git log -m
>>
>> will start to produce diffs without need to provide -p as well,
>
> Personally I don't ever use -m without -p and --first-parent, so in
> that sense this feels like a change in the right direction.
>
> Does this also affect the plumbing command "git diff-tree"?  I'm
> guessing "no" because diff-tree already generates a diff by default,
> but it seems worth spelling out in the commit message to prevent
> worries about the effect on scripts that expect stable plumbing
> behavior.

Well, here are existing relevant tests:

diff-tree master
diff-tree -p master
diff-tree -p -m master

that all still pass after the patches, and I believe

diff-tree -m master

does change the behavior, but provided it's useless before these
patches, it should be OK to have this change.

Thanks,
-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sergey Organov wrote:
>
>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>> one side, and -m that did not imply -p, on the other side.
>>
>> After this patch
>>
>>   git log -m
>>
>> will start to produce diffs without need to provide -p as well,
>
> Personally I don't ever use -m without -p and --first-parent, so in
> that sense this feels like a change in the right direction.
>
> Does this also affect the plumbing command "git diff-tree"?  I'm
> guessing "no" because diff-tree already generates a diff by default,
> but it seems worth spelling out in the commit message to prevent
> worries about the effect on scripts that expect stable plumbing
> behavior.

This is about "log" from the "rev-list" family, not "diff" to
compare two endpoints, so "git diff" won't be affected, and "git
diff-tree" is not affected, either.

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Sergey Organov wrote:
>>
>>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>>> one side, and -m that did not imply -p, on the other side.
>>>
>>> After this patch
>>>
>>>   git log -m
>>>
>>> will start to produce diffs without need to provide -p as well,
>>
>> Personally I don't ever use -m without -p and --first-parent, so in
>> that sense this feels like a change in the right direction.
>>
>> Does this also affect the plumbing command "git diff-tree"?  I'm
>> guessing "no" because diff-tree already generates a diff by default,
>> but it seems worth spelling out in the commit message to prevent
>> worries about the effect on scripts that expect stable plumbing
>> behavior.
>
> This is about "log" from the "rev-list" family, not "diff" to
> compare two endpoints, so "git diff" won't be affected, and "git
> diff-tree" is not affected, either.

You are right. I've added a test for "git diff-tree -m", and the changes
don't break it.

Can easily re-roll if you think the test worth it.

Thanks,
-- Sergey Organov

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Jonathan Nieder]

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

>> This is about "log" from the "rev-list" family, not "diff" to
>> compare two endpoints, so "git diff" won't be affected, and "git
>> diff-tree" is not affected, either.
>
> You are right. I've added a test for "git diff-tree -m", and the changes
> don't break it.

Good.

> Can easily re-roll if you think the test worth it.

I care more about the commit message than the test --- i.e. mentioning
that this doesn't affect commands like

	git rev-list HEAD~5..HEAD | git diff-tree --stdin -m

Thanks,
Jonathan

Re: [PATCH 6/6] diff-merges: let -m imply -p

[Sergey Organov]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sergey Organov wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>
>>> This is about "log" from the "rev-list" family, not "diff" to
>>> compare two endpoints, so "git diff" won't be affected, and "git
>>> diff-tree" is not affected, either.
>>
>> You are right. I've added a test for "git diff-tree -m", and the changes
>> don't break it.
>
> Good.
>
>> Can easily re-roll if you think the test worth it.
>
> I care more about the commit message than the test --- i.e. mentioning
> that this doesn't affect commands like
>
> 	git rev-list HEAD~5..HEAD | git diff-tree --stdin -m

OK, re-roll to follow...

Thanks,
-- Sergey Organov

[PATCH v1 0/9] diff-merges: let -m imply -p

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p, on
one side, and -m that did not imply -p, on the other side.

Only the last patch is the actual functional change. The reset of
patches are additional tests and refactorings that are not expected to
alter existing behaviors.

After these patches

  git log -m

will start to produce diffs without need to provide -p as well, that
improves both consistency and usability. It gets even more useful if
one sets "log.diffMerges" configuration variable to "first-parent" to
force -m produce usual diff with respect to first parent only.

These patches don't change behavior when specific diff format is
explicitly provided on the command-line, so that commands like

  git log -m --raw
  git log -m --stat

are not affected.

Exact historical semantics of -m is still provided by
--diff-merges=separate.

Updates in v1:

  * Stop parsing distinct diff-index options beforehand, as it could
    cause unexpected behaviors. Implement different strategy to avoid
    clash of diff-index "-m" and diff-merges "-m".

  * Added tests for "git log -m --raw" and "git log -m".

Sergey Organov (9):
  t4013: test that "-m" alone has no effect in "git log"
  t4013: test "git -m --raw"
  t4013: test "git -m --stat"
  t4013: test "git diff-index -m"
  diff-merges: move specific diff-index "-m" handling to diff-index
  git-svn: stop passing "-m" to "git rev-list"
  stash list: stop passing "-m" to "git list"
  diff-merges: rename "combined_imply_patch" to "merges_imply_patch"
  diff-merges: let "-m" imply "-p"

 Documentation/diff-options.txt    |  8 ++--
 builtin/diff-index.c              |  9 +++++
 builtin/stash.c                   |  2 +-
 diff-merges.c                     | 36 +++++++++--------
 diff-merges.h                     |  2 +
 perl/Git/SVN.pm                   |  2 +-
 revision.h                        |  2 +-
 t/t3903-stash.sh                  |  2 +-
 t/t4013-diff-various.sh           | 23 +++++++++++
 t/t4013/diff.log_-m_--raw_master  | 61 ++++++++++++++++++++++++++++
 t/t4013/diff.log_-m_--stat_master | 66 +++++++++++++++++++++++++++++++
 11 files changed, 188 insertions(+), 25 deletions(-)
 create mode 100644 t/t4013/diff.log_-m_--raw_master
 create mode 100644 t/t4013/diff.log_-m_--stat_master

Interdiff against v0:
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 28bc51d0d8f4..cf09559e422d 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "config.h"
 #include "diff.h"
+#include "diff-merges.h"
 #include "commit.h"
 #include "revision.h"
 #include "builtin.h"
@@ -12,37 +13,11 @@ static const char diff_cache_usage[] =
 "[<common-diff-options>] <tree-ish> [<path>...]"
 COMMON_DIFF_OPTIONS_HELP;
 
-static int parse_distinct_options(int argc, const char **argv,
-				  struct rev_info *revs, unsigned int *options)
-{
-	int i, left;
-
-	for (i = left = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		int leave = 0;
-
-		if (!strcmp(arg, "--cached"))
-			*options |= DIFF_INDEX_CACHED;
-		else if (!strcmp(arg, "--merge-base"))
-			*options |= DIFF_INDEX_MERGE_BASE;
-		else if (!strcmp(arg, "-m"))
-			revs->match_missing = 1;
-		else
-			leave = 1;
-
-		if (leave)
-			argv[left++] = arg;
-	}
-
-	argv[left] = NULL;
-
-	return left;
-}
-
 int cmd_diff_index(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
 	unsigned int option = 0;
+	int i;
 	int result;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -54,15 +29,24 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	prefix = precompose_argv_prefix(argc, argv, prefix);
 
 	/*
-	 * It's essential to parse our distinct options before calling
-	 * setup_revisions(), for the latter not to see "-m".
+	 * We need no diff for merges options, and we need to avoid conflict
+	 * with our own meaning of "-m".
 	 */
-	argc = parse_distinct_options(argc, argv, &rev, &option);
+	diff_merges_suppress_options_parsing();
+
 	argc = setup_revisions(argc, argv, &rev, NULL);
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
 
-	if (argc > 1)
-		usage(diff_cache_usage);
-
+		if (!strcmp(arg, "--cached"))
+			option |= DIFF_INDEX_CACHED;
+		else if (!strcmp(arg, "--merge-base"))
+			option |= DIFF_INDEX_MERGE_BASE;
+		else if (!strcmp(arg, "-m"))
+			rev.match_missing = 1;
+		else
+			usage(diff_cache_usage);
+	}
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 
diff --git a/diff-merges.c b/diff-merges.c
index a827482a97ff..0dfcaa1b11b0 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -6,6 +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 void suppress(struct rev_info *revs)
 {
@@ -90,12 +91,20 @@ int diff_merges_config(const char *value)
 	return 0;
 }
 
+void diff_merges_suppress_options_parsing(void)
+{
+	suppress_parsing = 1;
+}
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 {
 	int argcount = 1;
 	const char *optarg;
 	const char *arg = argv[0];
 
+	if (suppress_parsing)
+		return 0;
+
 	if (!strcmp(arg, "-m")) {
 		set_to_default(revs);
 		revs->merges_imply_patch = 1;
@@ -145,6 +154,9 @@ 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 09d9a6c9a4fb..b5d57f6563e3 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -11,6 +11,8 @@ struct rev_info;
 
 int diff_merges_config(const char *value);
 
+void diff_merges_suppress_options_parsing(void);
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
 void diff_merges_suppress(struct rev_info *revs);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index bd16d4885cfd..15ca3c75bff7 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -337,6 +337,8 @@ log -m -p --first-parent master
 log -m -p master
 log --cc -m -p master
 log -c -m -p master
+log -m --raw master
+log -m --stat master
 log -SF master
 log -S F master
 log -SF -p master
@@ -452,6 +454,14 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+test_expect_success 'log -m matches log -m -p' '
+	git log -m -p master >result &&
+	process_diffs result >expected &&
+	git log -m >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	git log -p --diff-merges=separate master >result &&
 	process_diffs result >expected &&
diff --git a/t/t4013/diff.log_-m_--raw_master b/t/t4013/diff.log_-m_--raw_master
new file mode 100644
index 000000000000..cd2ecc462832
--- /dev/null
+++ b/t/t4013/diff.log_-m_--raw_master
@@ -0,0 +1,61 @@
+$ git log -m --raw master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 cead32e... 992913c... M	dir/sub
+:100644 100644 b414108... 10a8a9f... M	file0
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 7289e35... 992913c... M	dir/sub
+:100644 100644 f4615da... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+:100644 000000 7289e35... 0000000... D	file3
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+:100644 100644 35d242b... 7289e35... M	dir/sub
+:100644 100644 01e79c3... f4615da... M	file0
+:000000 100644 0000000... 7289e35... A	file3
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+:100644 100644 8422d40... cead32e... M	dir/sub
+:000000 100644 0000000... b1e6722... A	file1
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+:100644 100644 35d242b... 8422d40... M	dir/sub
+:100644 100644 01e79c3... b414108... M	file0
+:100644 000000 01e79c3... 0000000... D	file2
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
diff --git a/t/t4013/diff.log_-m_--stat_master b/t/t4013/diff.log_-m_--stat_master
new file mode 100644
index 000000000000..c7db084fd905
--- /dev/null
+++ b/t/t4013/diff.log_-m_--stat_master
@@ -0,0 +1,66 @@
+$ git log -m --stat master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 4 ++++
+ file0   | 3 +++
+ file1   | 3 +++
+ file2   | 3 ---
+ file3   | 4 ----
+ 5 files changed, 10 insertions(+), 7 deletions(-)
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file3   | 4 ++++
+ 3 files changed, 9 insertions(+)
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+ dir/sub | 2 ++
+ file1   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file2   | 3 ---
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.25.1

[PATCH v1 1/9] t4013: test that "-m" alone has no effect in "git log"

[Sergey Organov]

This is to notice current behavior that we are going to change when
when we start to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 87def81699bf..e9f67cd24351 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+test_expect_success 'log -m matches pure log' '
+	git log master >result &&
+	process_diffs result >expected &&
+	git log -m >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	git log -p --diff-merges=separate master >result &&
 	process_diffs result >expected &&
-- 
2.25.1

[PATCH v1 2/9] t4013: test "git -m --raw"

[Sergey Organov]

This is to ensure we won't break different diff formats when we start
to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh          |  1 +
 t/t4013/diff.log_-m_--raw_master | 61 ++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 t/t4013/diff.log_-m_--raw_master

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e9f67cd24351..1809355f9bb1 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -337,6 +337,7 @@ log -m -p --first-parent master
 log -m -p master
 log --cc -m -p master
 log -c -m -p master
+log -m --raw master
 log -SF master
 log -S F master
 log -SF -p master
diff --git a/t/t4013/diff.log_-m_--raw_master b/t/t4013/diff.log_-m_--raw_master
new file mode 100644
index 000000000000..cd2ecc462832
--- /dev/null
+++ b/t/t4013/diff.log_-m_--raw_master
@@ -0,0 +1,61 @@
+$ git log -m --raw master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 cead32e... 992913c... M	dir/sub
+:100644 100644 b414108... 10a8a9f... M	file0
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 7289e35... 992913c... M	dir/sub
+:100644 100644 f4615da... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+:100644 000000 7289e35... 0000000... D	file3
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+:100644 100644 35d242b... 7289e35... M	dir/sub
+:100644 100644 01e79c3... f4615da... M	file0
+:000000 100644 0000000... 7289e35... A	file3
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+:100644 100644 8422d40... cead32e... M	dir/sub
+:000000 100644 0000000... b1e6722... A	file1
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+:100644 100644 35d242b... 8422d40... M	dir/sub
+:100644 100644 01e79c3... b414108... M	file0
+:100644 000000 01e79c3... 0000000... D	file2
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.25.1

Re: [PATCH v1 2/9] t4013: test "git -m --raw"

[Bagas Sanjaya]

On 17/05/21 22.58, Sergey Organov wrote:
> This is to ensure we won't break different diff formats when we start
> to imply "-p" by "-m".
> 

This patch, and patch 3, have the same commit message (and thus the logic),
so these can be squashed.

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>   t/t4013-diff-various.sh          |  1 +
>   t/t4013/diff.log_-m_--raw_master | 61 ++++++++++++++++++++++++++++++++
>   2 files changed, 62 insertions(+)
>   create mode 100644 t/t4013/diff.log_-m_--raw_master
> 
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index e9f67cd24351..1809355f9bb1 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -337,6 +337,7 @@ log -m -p --first-parent master
>   log -m -p master
>   log --cc -m -p master
>   log -c -m -p master
> +log -m --raw master
>   log -SF master
>   log -S F master
>   log -SF -p master

In near future, we will switch default branch created by `git init` to
main. Had the test be prepared?

> +:100644 100644 7289e35... 992913c... M	dir/sub
> +:100644 100644 f4615da... 10a8a9f... M	file0
> +:000000 100644 0000000... b1e6722... A	file1
> +:100644 000000 01e79c3... 0000000... D	file2
> +:100644 000000 7289e35... 0000000... D	file3
> +
> +commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:03:00 2006 +0000
> +
> +    Side
> +
> +:100644 100644 35d242b... 7289e35... M	dir/sub
> +:100644 100644 01e79c3... f4615da... M	file0
> +:000000 100644 0000000... 7289e35... A	file3
> +
> +commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:02:00 2006 +0000
> +
> +    Third
> +
> +:100644 100644 8422d40... cead32e... M	dir/sub
> +:000000 100644 0000000... b1e6722... A	file1
> +
> +commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:01:00 2006 +0000
> +
> +    Second
> +
> +    This is the second commit.
> +

The rest of commits only have commit title, why do this second commit
have also commit message body?

> +:100644 100644 35d242b... 8422d40... M	dir/sub
> +:100644 100644 01e79c3... b414108... M	file0
> +:100644 000000 01e79c3... 0000000... D	file2
> +
> +commit 444ac553ac7612cc88969031b02b3767fb8a353a
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:00:00 2006 +0000
> +
> +    Initial
> +$
> 

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v1 2/9] t4013: test "git -m --raw"

[Sergey Organov]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 17/05/21 22.58, Sergey Organov wrote:
>> This is to ensure we won't break different diff formats when we start
>> to imply "-p" by "-m".
>> 
>
> This patch, and patch 3, have the same commit message (and thus the logic),
> so these can be squashed.

Yep, but they have different subjects. Usually I need a serious reason
to squash otherwise independent changes, but if you guys prefer them
squashed, we'd need to come up with suitable squashed subject, and I'll
squash them no problem.

>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>   t/t4013-diff-various.sh          |  1 +
>>   t/t4013/diff.log_-m_--raw_master | 61 ++++++++++++++++++++++++++++++++
>>   2 files changed, 62 insertions(+)
>>   create mode 100644 t/t4013/diff.log_-m_--raw_master
>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
>> index e9f67cd24351..1809355f9bb1 100755
>> --- a/t/t4013-diff-various.sh
>> +++ b/t/t4013-diff-various.sh
>> @@ -337,6 +337,7 @@ log -m -p --first-parent master
>>   log -m -p master
>>   log --cc -m -p master
>>   log -c -m -p master
>> +log -m --raw master
>>   log -SF master
>>   log -S F master
>>   log -SF -p master
>
> In near future, we will switch default branch created by `git init` to
> main. Had the test be prepared?

No idea, sorry. I'd personally rather change preparation stage of this
test file to explicitly create "master".

>
>> +:100644 100644 7289e35... 992913c... M	dir/sub
>> +:100644 100644 f4615da... 10a8a9f... M	file0
>> +:000000 100644 0000000... b1e6722... A	file1
>> +:100644 000000 01e79c3... 0000000... D	file2
>> +:100644 000000 7289e35... 0000000... D	file3
>> +
>> +commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
>> +Author: A U Thor <author@example.com>
>> +Date:   Mon Jun 26 00:03:00 2006 +0000
>> +
>> +    Side
>> +
>> +:100644 100644 35d242b... 7289e35... M	dir/sub
>> +:100644 100644 01e79c3... f4615da... M	file0
>> +:000000 100644 0000000... 7289e35... A	file3
>> +
>> +commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
>> +Author: A U Thor <author@example.com>
>> +Date:   Mon Jun 26 00:02:00 2006 +0000
>> +
>> +    Third
>> +
>> +:100644 100644 8422d40... cead32e... M	dir/sub
>> +:000000 100644 0000000... b1e6722... A	file1
>> +
>> +commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
>> +Author: A U Thor <author@example.com>
>> +Date:   Mon Jun 26 00:01:00 2006 +0000
>> +
>> +    Second
>> +
>> +    This is the second commit.
>> +
>
> The rest of commits only have commit title, why do this second commit
> have also commit message body?

Probably for testing of messages bodies? Anyway, that's how these tests
are arranged, doesn't come with these particular patches.

BTW, in the original there are trailing whitespaces on otherwise empty
line between "Second" and "This is the seconds commit".

>
>> +:100644 100644 35d242b... 8422d40... M	dir/sub
>> +:100644 100644 01e79c3... b414108... M	file0
>> +:100644 000000 01e79c3... 0000000... D	file2
>> +
>> +commit 444ac553ac7612cc88969031b02b3767fb8a353a
>> +Author: A U Thor <author@example.com>
>> +Date:   Mon Jun 26 00:00:00 2006 +0000
>> +
>> +    Initial
>> +$
>> 
>
> Thanks.

Thanks,

-- Sergey Organov

[PATCH v1 3/9] t4013: test "git -m --stat"

[Sergey Organov]

This is to ensure we won't break different diff formats when we start
to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh           |  1 +
 t/t4013/diff.log_-m_--stat_master | 66 +++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)
 create mode 100644 t/t4013/diff.log_-m_--stat_master

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 1809355f9bb1..e53ca7aa503f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -338,6 +338,7 @@ log -m -p master
 log --cc -m -p master
 log -c -m -p master
 log -m --raw master
+log -m --stat master
 log -SF master
 log -S F master
 log -SF -p master
diff --git a/t/t4013/diff.log_-m_--stat_master b/t/t4013/diff.log_-m_--stat_master
new file mode 100644
index 000000000000..c7db084fd905
--- /dev/null
+++ b/t/t4013/diff.log_-m_--stat_master
@@ -0,0 +1,66 @@
+$ git log -m --stat master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 4 ++++
+ file0   | 3 +++
+ file1   | 3 +++
+ file2   | 3 ---
+ file3   | 4 ----
+ 5 files changed, 10 insertions(+), 7 deletions(-)
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file3   | 4 ++++
+ 3 files changed, 9 insertions(+)
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+ dir/sub | 2 ++
+ file1   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file2   | 3 ---
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.25.1

[PATCH v1 4/9] t4013: test "git diff-index -m"

[Sergey Organov]

-m in "git diff-index" means "match missing", that differs
from its meaning in "git diff". Let's check it in diff-index.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e53ca7aa503f..c35cc73d08bd 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -493,6 +493,19 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '
 	test_cmp expected actual
 '
 
+# -m in "git diff-index" means "match missing", that differs
+# from its meaning in "git diff". Let's check it in diff-index.
+# The line in the output for removed file should disappear when
+# we provide -m in diff-index.
+test_expect_success 'git diff-index -m' '
+	rm -f file1 &&
+	git diff-index HEAD >without-m &&
+	lines_count=$(wc -l <without-m) &&
+	git diff-index -m HEAD >with-m &&
+	git restore file1 &&
+	test_line_count = $((lines_count - 1)) with-m
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

[PATCH v1 5/9] diff-merges: move specific diff-index "-m" handling to diff-index

[Sergey Organov]

Move specific handling of "-m" for diff-index to diff-index.c, so
diff-merges is left to handle only diff for merges options.

Being a better design by itself, this is especially essential in
preparation for letting -m imply -p, as "diff-index -m" obviously
should not imply -p, as it's entirely unrelated.

To handle this, in addition to moving specific diff-index "-m" code
out of diff-merges, we introduce new

  diff_merges_suppress_options_parsing()

and call it before generic options processing in cmd_diff_index().

This new diff_merges_suppress_options_parsing() could then be reused
and called before invocations of setup_revisions() for other commands
that don't need --diff-merges options, but that's outside of the scope
of these patch series.

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

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 176fe7ff2b4e..cf09559e422d 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "config.h"
 #include "diff.h"
+#include "diff-merges.h"
 #include "commit.h"
 #include "revision.h"
 #include "builtin.h"
@@ -27,6 +28,12 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	rev.abbrev = 0;
 	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".
+	 */
+	diff_merges_suppress_options_parsing();
+
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -35,6 +42,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 			option |= DIFF_INDEX_CACHED;
 		else if (!strcmp(arg, "--merge-base"))
 			option |= DIFF_INDEX_MERGE_BASE;
+		else if (!strcmp(arg, "-m"))
+			rev.match_missing = 1;
 		else
 			usage(diff_cache_usage);
 	}
diff --git a/diff-merges.c b/diff-merges.c
index f3a9daed7e05..9ca00cdd0cc6 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -6,6 +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 void suppress(struct rev_info *revs)
 {
@@ -30,17 +31,6 @@ static void set_first_parent(struct rev_info *revs)
 	revs->first_parent_merges = 1;
 }
 
-static void set_m(struct rev_info *revs)
-{
-	/*
-	 * To "diff-index", "-m" means "match missing", and to the "log"
-	 * family of commands, it means "show default diff for merges". Set
-	 * both fields appropriately.
-	 */
-	set_to_default(revs);
-	revs->match_missing = 1;
-}
-
 static void set_combined(struct rev_info *revs)
 {
 	suppress(revs);
@@ -101,14 +91,22 @@ int diff_merges_config(const char *value)
 	return 0;
 }
 
+void diff_merges_suppress_options_parsing(void)
+{
+	suppress_parsing = 1;
+}
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 {
 	int argcount = 1;
 	const char *optarg;
 	const char *arg = argv[0];
 
+	if (suppress_parsing)
+		return 0;
+
 	if (!strcmp(arg, "-m")) {
-		set_m(revs);
+		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->combined_imply_patch = 1;
@@ -155,6 +153,9 @@ 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 09d9a6c9a4fb..b5d57f6563e3 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -11,6 +11,8 @@ struct rev_info;
 
 int diff_merges_config(const char *value);
 
+void diff_merges_suppress_options_parsing(void);
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
 void diff_merges_suppress(struct rev_info *revs);
-- 
2.25.1

Re: [PATCH v1 5/9] diff-merges: move specific diff-index "-m" handling to diff-index

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Move specific handling of "-m" for diff-index to diff-index.c, so
> diff-merges is left to handle only diff for merges options.
>
> Being a better design by itself, this is especially essential in
> preparation for letting -m imply -p, as "diff-index -m" obviously
> should not imply -p, as it's entirely unrelated.
>
> To handle this, in addition to moving specific diff-index "-m" code
> out of diff-merges, we introduce new
>
>   diff_merges_suppress_options_parsing()
>
> and call it before generic options processing in cmd_diff_index().

This change has a small but obvious fallout.

    $ git diff-index -c --cached HEAD^

now starts failing loudly.  Earlier, it silently fell back to
"combined" diff of one parent, which is "-p".

I think the end result is good (and luckily, "DIFF FORMAT FOR
MERGES" section explicitly limits "-c" and "--cc" to diff-tree,
diff-files and diff (and by implication excludes diff-index) so I am
sure there are small but non-zero number of people somewhere in the
world who has "diff-index -c" in their scripts that suddenly starts
failing with the version of Git with this change, but we can just
say their use was broken ;-)

Re: [PATCH v1 5/9] diff-merges: move specific diff-index "-m" handling to diff-index

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Move specific handling of "-m" for diff-index to diff-index.c, so
>> diff-merges is left to handle only diff for merges options.
>>
>> Being a better design by itself, this is especially essential in
>> preparation for letting -m imply -p, as "diff-index -m" obviously
>> should not imply -p, as it's entirely unrelated.
>>
>> To handle this, in addition to moving specific diff-index "-m" code
>> out of diff-merges, we introduce new
>>
>>   diff_merges_suppress_options_parsing()
>>
>> and call it before generic options processing in cmd_diff_index().
>
> This change has a small but obvious fallout.
>
>     $ git diff-index -c --cached HEAD^
>
> now starts failing loudly.  Earlier, it silently fell back to
> "combined" diff of one parent, which is "-p".
>
> I think the end result is good (and luckily, "DIFF FORMAT FOR
> MERGES" section explicitly limits "-c" and "--cc" to diff-tree,
> diff-files and diff (and by implication excludes diff-index) so I am
> sure there are small but non-zero number of people somewhere in the
> world who has "diff-index -c" in their scripts that suddenly starts
> failing with the version of Git with this change, but we can just
> say their use was broken ;-)

Well, I'm not sure. If it's a problem, I think I can add -c/--cc parsing
to diff-index that will simply imply -p. This way we will be more
backward-compatible.

Thanks,

-- Sergey Organov.

Re: [PATCH v1 5/9] diff-merges: move specific diff-index "-m" handling to diff-index

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Move specific handling of "-m" for diff-index to diff-index.c, so
> diff-merges is left to handle only diff for merges options.
>
> Being a better design by itself, this is especially essential in
> preparation for letting -m imply -p, as "diff-index -m" obviously
> should not imply -p, as it's entirely unrelated.
>
> To handle this, in addition to moving specific diff-index "-m" code
> out of diff-merges, we introduce new
>
>   diff_merges_suppress_options_parsing()
>
> and call it before generic options processing in cmd_diff_index().

This change has a small but obvious fallout.

    $ git diff-index -c --cached HEAD^

now starts failing loudly.  Earlier, it silently fell back to
"combined" diff of one parent, which is "-p".

I think the end result is good (and luckily, "DIFF FORMAT FOR
MERGES" section explicitly limits "-c" and "--cc" to diff-tree,
diff-files and diff (and by implication excludes diff-index) so I am
sure there are small but non-zero number of people somewhere in the
world who has "diff-index -c" in their scripts that suddenly starts
failing with the version of Git with this change, but we can just
say their use was broken ;-)

Having said all that, I have to wonder if it still is needed to keep
the "diff-index -m" working, or we would be better off breaking it
to avoid a change like this that makes us bend over backwards to
work around the command line parsing infrastructure.

The only reason why "diff-index -m" exists is because it was part of
the idea Linus had for the merge implementation that we ended up
deciding not taking, where merges and possibly other bulk operations
that would affect the working tree is done in a separate, temporary
directory that is sparsely populated, the user is asked to edit away
conflicts in the temporary directory and expected to monitor his or
her own progress using "diff-index -m".  Our plan was to populate
such a temporary directory with only paths that are involved in the
operation in progress, without instantiating paths that are not
touched, so "treat missing files as if they haven't been modified"
was a handy ingredient for such a mode of operation.

But we ended up going with a different design, in which the main
working tree area is used to perform merges and to resolve
conflicts, which made this "pretend missing files as unmodified"
unnecessary feature.  In the end, we made a good move, as the
current design allows users to verify their changes in the context
of a full checkout (e.g. "make" would not have been a good way to
validate the conflict resolution if it is done in a separate
temporary directory that is sparsely populated with only the paths
involved in the merge---you need all files for building, including
the ones that are not modified, and "make" does not know to treat
missing files as if they are unmodified).

Re: [PATCH v1 5/9] diff-merges: move specific diff-index "-m" handling to diff-index

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Move specific handling of "-m" for diff-index to diff-index.c, so
>> diff-merges is left to handle only diff for merges options.
>>
>> Being a better design by itself, this is especially essential in
>> preparation for letting -m imply -p, as "diff-index -m" obviously
>> should not imply -p, as it's entirely unrelated.
>>
>> To handle this, in addition to moving specific diff-index "-m" code
>> out of diff-merges, we introduce new
>>
>>   diff_merges_suppress_options_parsing()
>>
>> and call it before generic options processing in cmd_diff_index().
>
> This change has a small but obvious fallout.
>
>     $ git diff-index -c --cached HEAD^
>
> now starts failing loudly.  Earlier, it silently fell back to
> "combined" diff of one parent, which is "-p".
>
> I think the end result is good (and luckily, "DIFF FORMAT FOR
> MERGES" section explicitly limits "-c" and "--cc" to diff-tree,
> diff-files and diff (and by implication excludes diff-index) so I am
> sure there are small but non-zero number of people somewhere in the
> world who has "diff-index -c" in their scripts that suddenly starts
> failing with the version of Git with this change, but we can just
> say their use was broken ;-)
>
> Having said all that, I have to wonder if it still is needed to keep
> the "diff-index -m" working, or we would be better off breaking it
> to avoid a change like this that makes us bend over backwards to
> work around the command line parsing infrastructure.
>
> The only reason why "diff-index -m" exists is because it was part of
> the idea Linus had for the merge implementation that we ended up
> deciding not taking, where merges and possibly other bulk operations
> that would affect the working tree is done in a separate, temporary
> directory that is sparsely populated, the user is asked to edit away
> conflicts in the temporary directory and expected to monitor his or
> her own progress using "diff-index -m".  Our plan was to populate
> such a temporary directory with only paths that are involved in the
> operation in progress, without instantiating paths that are not
> touched, so "treat missing files as if they haven't been modified"
> was a handy ingredient for such a mode of operation.
>
> But we ended up going with a different design, in which the main
> working tree area is used to perform merges and to resolve
> conflicts, which made this "pretend missing files as unmodified"
> unnecessary feature.  In the end, we made a good move, as the
> current design allows users to verify their changes in the context
> of a full checkout (e.g. "make" would not have been a good way to
> validate the conflict resolution if it is done in a separate
> temporary directory that is sparsely populated with only the paths
> involved in the merge---you need all files for building, including
> the ones that are not modified, and "make" does not know to treat
> missing files as if they are unmodified).

This could be a good thing to do, but as I wrote in the description,
there are in fact other commands that don't need diff-merges options and
currently just eat them silently instead of bailing out.

It's likely that 90% of uses of setup_revisions() don't need
diff-merges, so a feature to exclude diff-merges parsing seems to be
useful by itself.

Thanks,

-- Sergey Organov

[PATCH v1 6/9] git-svn: stop passing "-m" to "git rev-list"

[Sergey Organov]

rev-list doesn't utilize -m. It happens to eat it silently, so this
bug went unnoticed.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 perl/Git/SVN.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index f6f1dc03c608..35ff5a68963d 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1636,7 +1636,7 @@ sub has_no_changes {
 	my $commit = shift;
 
 	my @revs = split / /, command_oneline(
-		qw(rev-list --parents -1 -m), $commit);
+		qw(rev-list --parents -1), $commit);
 
 	# Commits with no parents, e.g. the start of a partial branch,
 	# have changes by definition.
-- 
2.25.1

[PATCH v1 7/9] stash list: stop passing "-m" to "git list"

[Sergey Organov]

Passing "-m" in "git list --first-parent -m" is not needed as
--first-parent implies --diff-merges=first-parent anyway. OTOH, it
will stop being harmless once we let "-m" imply "-p".

While we are at it, fix corresponding test description in t3903-stash
to match what it actually tests.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 builtin/stash.c  | 2 +-
 t/t3903-stash.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index d68ed784d2af..fe8e97428808 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -761,7 +761,7 @@ static int list_stash(int argc, const char **argv, const char *prefix)
 
 	cp.git_cmd = 1;
 	strvec_pushl(&cp.args, "log", "--format=%gd: %gs", "-g",
-		     "--first-parent", "-m", NULL);
+		     "--first-parent", NULL);
 	strvec_pushv(&cp.args, argv);
 	strvec_push(&cp.args, ref_stash);
 	strvec_push(&cp.args, "--");
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5f282ecf6175..873aa56e359d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -859,7 +859,7 @@ test_expect_success 'setup stash with index and worktree changes' '
 	git stash
 '
 
-test_expect_success 'stash list implies --first-parent -m' '
+test_expect_success 'stash list -p shows simple diff' '
 	cat >expect <<-EOF &&
 	stash@{0}
 
-- 
2.25.1

[PATCH v1 8/9] diff-merges: rename "combined_imply_patch" to "merges_imply_patch"

[Sergey Organov]

This is refactoring change in preparation for the next commit that
will let -m imply -p.

The old name doesn't match the intention to let not only -c/-cc imply
-p, but also -m, that is not a "combined" format, so we rename the
flag accordingly.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 10 +++++-----
 revision.h    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 9ca00cdd0cc6..d897fd8a2933 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -15,7 +15,7 @@ static void suppress(struct rev_info *revs)
 	revs->combine_merges = 0;
 	revs->dense_combined_merges = 0;
 	revs->combined_all_paths = 0;
-	revs->combined_imply_patch = 0;
+	revs->merges_imply_patch = 0;
 	revs->merges_need_diff = 0;
 }
 
@@ -109,10 +109,10 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--cc")) {
 		set_dense_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
 		suppress(revs);
 	} else if (!strcmp(arg, "--combined-all-paths")) {
@@ -162,9 +162,9 @@ void diff_merges_setup_revs(struct rev_info *revs)
 		revs->first_parent_merges = 0;
 	if (revs->combined_all_paths && !revs->combine_merges)
 		die("--combined-all-paths makes no sense without -c or --cc");
-	if (revs->combined_imply_patch)
+	if (revs->merges_imply_patch)
 		revs->diff = 1;
-	if (revs->combined_imply_patch || revs->merges_need_diff) {
+	if (revs->merges_imply_patch || revs->merges_need_diff) {
 		if (!revs->diffopt.output_format)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 	}
diff --git a/revision.h b/revision.h
index 93aa012f518e..17698cb51aca 100644
--- a/revision.h
+++ b/revision.h
@@ -193,10 +193,10 @@ struct rev_info {
 			/* Diff-merge flags */
 			explicit_diff_merges: 1,
 			merges_need_diff: 1,
+			merges_imply_patch:1,
 			separate_merges: 1,
 			combine_merges:1,
 			combined_all_paths:1,
-			combined_imply_patch:1,
 			dense_combined_merges:1,
 			first_parent_merges:1;
 
-- 
2.25.1

[PATCH v1 9/9] diff-merges: let "-m" imply "-p"

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p, on
one side, and -m that did not imply -p, on the other side.

Change corresponding test accordingly, as "log -m" output should now
match one from "log -m -p", rather than from just "log".

Change documentation accordingly.

NOTES:

After this patch

  git log -m

will start to produce diffs without need to provide -p as well, that
improves both consistency and usability. It gets even more useful if
one sets "log.diffMerges" configuration variable to "first-parent" to
force -m produce usual diff with respect to first parent only.

This patch does not change behavior when specific diff format is
explicitly provided on the command-line, so that commands like

  git log -m --raw
  git log -m --stat

are not affected.

Exact historical semantics of -m is still provided by
--diff-merges=separate.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt | 8 ++++----
 diff-merges.c                  | 1 +
 t/t4013-diff-various.sh        | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 530d1159141f..32e6dee5ac3b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -49,10 +49,9 @@ ifdef::git-log[]
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
-	the default format. `-m` will produce the output only if `-p`
-	is given as well. The default format could be changed using
+	the default format. The default format could be changed using
 	`log.diffMerges` configuration parameter, which default value
-	is `separate`.
+	is `separate`. `-m` implies `-p`.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -62,7 +61,8 @@ ifdef::git-log[]
 --diff-merges=separate:::
 	This makes merge commits show the full diff with respect to
 	each of the parents. Separate log entry and diff is generated
-	for each parent.
+	for each parent. This is the format that `-m` produced
+	historically.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
diff --git a/diff-merges.c b/diff-merges.c
index d897fd8a2933..0dfcaa1b11b0 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -107,6 +107,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!strcmp(arg, "-m")) {
 		set_to_default(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index c35cc73d08bd..15ca3c75bff7 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -454,8 +454,8 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
-test_expect_success 'log -m matches pure log' '
-	git log master >result &&
+test_expect_success 'log -m matches log -m -p' '
+	git log -m -p master >result &&
 	process_diffs result >expected &&
 	git log -m >result &&
 	process_diffs result >actual &&
-- 
2.25.1

Re: [PATCH v1 0/9] diff-merges: let -m imply -p

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Fix long standing inconsistency between -c/--cc that do imply -p, on
> one side, and -m that did not imply -p, on the other side.
> ...
> Exact historical semantics of -m is still provided by
> --diff-merges=separate.
>
> Updates in v1:

OK, so this is [v2] not v1 ;-)

Will replace.  Thanks.

Re: [PATCH v1 0/9] diff-merges: let -m imply -p

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>> one side, and -m that did not imply -p, on the other side.
>> ...
>> Exact historical semantics of -m is still provided by
>> --diff-merges=separate.
>>
>> Updates in v1:
>
> OK, so this is [v2] not v1 ;-)

I dunno. Actually, I'm a bit confused.

Is simple [PATCH] v0 or v1? For me, as C-programmer, it's obviously v0,
and that's what

  git format-patch --reroll-count=0
 
produces anyway, and the next one is

  git format-patch --reroll-count=1

that produces [PATCH v1]. That's how it became v1.

>
> Will replace.  Thanks.

Thanks!

-- Sergey Organov

Re: [PATCH v1 0/9] diff-merges: let -m imply -p

[Bagas Sanjaya]

On 17/05/21 22.58, Sergey Organov wrote:
> Fix long standing inconsistency between -c/--cc that do imply -p, on
> one side, and -m that did not imply -p, on the other side.
> 

The commas are unnecessary.

> Only the last patch is the actual functional change. The reset of
> patches are additional tests and refactorings that are not expected to
> alter existing behaviors.
> 

Oops, s/reset/rest/.

> After these patches
> 
>    git log -m
> 
> will start to produce diffs without need to provide -p as well, that
> improves both consistency and usability. It gets even more useful if
> one sets "log.diffMerges" configuration variable to "first-parent" to
> force -m produce usual diff with respect to first parent only.
> 

Better say "After this patch series, git log -m produces diffs (implies
-p)".

> These patches don't change behavior when specific diff format is
> explicitly provided on the command-line, so that commands like
> 
>    git log -m --raw
>    git log -m --stat
> 
> are not affected.
> 

Better say "This series, however, don't change behavior...".

> Exact historical semantics of -m is still provided by
> --diff-merges=separate.
> 

Better say "Previously, the -m semantics is same as --diff-merges=separate".

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v1 0/9] diff-merges: let -m imply -p

[Sergey Organov]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 17/05/21 22.58, Sergey Organov wrote:
>> Fix long standing inconsistency between -c/--cc that do imply -p, on
>> one side, and -m that did not imply -p, on the other side.
>> 
>
> The commas are unnecessary.
>
>> Only the last patch is the actual functional change. The reset of
>> patches are additional tests and refactorings that are not expected to
>> alter existing behaviors.
>> 
>
> Oops, s/reset/rest/.
>
>> After these patches
>>    git log -m
>> will start to produce diffs without need to provide -p as well, that
>> improves both consistency and usability. It gets even more useful if
>> one sets "log.diffMerges" configuration variable to "first-parent" to
>> force -m produce usual diff with respect to first parent only.
>> 
>
> Better say "After this patch series, git log -m produces diffs (implies
> -p)".
>
>> These patches don't change behavior when specific diff format is
>> explicitly provided on the command-line, so that commands like
>>    git log -m --raw
>>    git log -m --stat
>> are not affected.
>> 
>
> Better say "This series, however, don't change behavior...".
>
>> Exact historical semantics of -m is still provided by
>> --diff-merges=separate.
>> 
>
> Better say "Previously, the -m semantics is same as
> --diff-merges=separate".

Agreed, will fix, thanks!

-- Sergey Organov

Re: [PATCH v1 0/9] diff-merges: let -m imply -p

[Sergey Organov]

Bagas Sanjaya <bagasdotme@gmail.com> writes:


[...]

>> Exact historical semantics of -m is still provided by
>> --diff-merges=separate.
>> 
>
> Better say "Previously, the -m semantics is same as
> --diff-merges=separate".

Oops, unlike the rest of your suggestions, this one I don't get, sorry.

Thanks,

-- Sergey Organov

Re: [PATCH v1 0/9] diff-merges: let -m imply -p

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Sergey Organov (9):
>   t4013: test that "-m" alone has no effect in "git log"
>   t4013: test "git -m --raw"
>   t4013: test "git -m --stat"

These two are mistitled (s/git -m/git log -m/).  Not a big deal as I
could "rebase -i" locally if there is no other changes needed, but
if you are going to have a reroll for other reasons, please correct
them.

>   stash list: stop passing "-m" to "git list"

s/git list/git log/, I would think.

>   diff-merges: rename "combined_imply_patch" to "merges_imply_patch"
>   diff-merges: let "-m" imply "-p"

Re: [PATCH v1 0/9] diff-merges: let -m imply -p

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Sergey Organov (9):
>>   t4013: test that "-m" alone has no effect in "git log"
>>   t4013: test "git -m --raw"
>>   t4013: test "git -m --stat"
>
> These two are mistitled (s/git -m/git log -m/).  Not a big deal as I
> could "rebase -i" locally if there is no other changes needed, but
> if you are going to have a reroll for other reasons, please correct
> them.

I'll correct and re-roll, please don't bother rebasing.

>
>>   stash list: stop passing "-m" to "git list"
>
> s/git list/git log/, I would think.

Yep, and the description itself is as buggy.

Thanks,

-- Sergey Organov

[...]

[PATCH v2 0/9] diff-merges: let -m imply -p

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p, on
one side, and -m that did not imply -p, on the other side.

Only the last patch is the actual functional change. The rest of
patches are additional tests and refactorings that are not expected to
alter existing behaviors.

After these patches

  git log -m

produces diffs without need to provide -p as well, that improves both
consistency and usability. It gets even more useful if one sets
"log.diffMerges" configuration variable to "first-parent" to force -m
produce usual diff with respect to first parent only.

These series, however, don't change behavior when specific diff format
is explicitly provided on the command-line, so that commands like

  git log -m --raw
  git log -m --stat

are not affected.

Exact historical semantics of -m is still provided by
--diff-merges=separate.

Updates in v2:

  * Fix style and typos in descriptions

Updates in v1:

  * Stop parsing distinct diff-index options beforehand, as it could
    cause unexpected behaviors. Implement different strategy to avoid
    clash of diff-index "-m" and diff-merges "-m".

  * Added tests for "git log -m --raw" and "git log -m".

Sergey Organov (9):
  t4013: test that "-m" alone has no effect in "git log"
  t4013: test "git log -m --raw"
  t4013: test "git log -m --stat"
  t4013: test "git diff-index -m"
  diff-merges: move specific diff-index "-m" handling to diff-index
  git-svn: stop passing "-m" to "git rev-list"
  stash list: stop passing "-m" to "git log"
  diff-merges: rename "combined_imply_patch" to "merges_imply_patch"
  diff-merges: let "-m" imply "-p"

 Documentation/diff-options.txt    |  8 ++--
 builtin/diff-index.c              |  9 +++++
 builtin/stash.c                   |  2 +-
 diff-merges.c                     | 36 +++++++++--------
 diff-merges.h                     |  2 +
 perl/Git/SVN.pm                   |  2 +-
 revision.h                        |  2 +-
 t/t3903-stash.sh                  |  2 +-
 t/t4013-diff-various.sh           | 23 +++++++++++
 t/t4013/diff.log_-m_--raw_master  | 61 ++++++++++++++++++++++++++++
 t/t4013/diff.log_-m_--stat_master | 66 +++++++++++++++++++++++++++++++
 11 files changed, 188 insertions(+), 25 deletions(-)
 create mode 100644 t/t4013/diff.log_-m_--raw_master
 create mode 100644 t/t4013/diff.log_-m_--stat_master

Interdiff against v1:
-- 
2.25.1

[PATCH v2 1/9] t4013: test that "-m" alone has no effect in "git log"

[Sergey Organov]

This is to notice current behavior that we are going to change when
when we start to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 87def81699bf..e9f67cd24351 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+test_expect_success 'log -m matches pure log' '
+	git log master >result &&
+	process_diffs result >expected &&
+	git log -m >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	git log -p --diff-merges=separate master >result &&
 	process_diffs result >expected &&
-- 
2.25.1

[PATCH v2 2/9] t4013: test "git log -m --raw"

[Sergey Organov]

This is to ensure we won't break different diff formats when we start
to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh          |  1 +
 t/t4013/diff.log_-m_--raw_master | 61 ++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 t/t4013/diff.log_-m_--raw_master

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e9f67cd24351..1809355f9bb1 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -337,6 +337,7 @@ log -m -p --first-parent master
 log -m -p master
 log --cc -m -p master
 log -c -m -p master
+log -m --raw master
 log -SF master
 log -S F master
 log -SF -p master
diff --git a/t/t4013/diff.log_-m_--raw_master b/t/t4013/diff.log_-m_--raw_master
new file mode 100644
index 000000000000..cd2ecc462832
--- /dev/null
+++ b/t/t4013/diff.log_-m_--raw_master
@@ -0,0 +1,61 @@
+$ git log -m --raw master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 cead32e... 992913c... M	dir/sub
+:100644 100644 b414108... 10a8a9f... M	file0
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 7289e35... 992913c... M	dir/sub
+:100644 100644 f4615da... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+:100644 000000 7289e35... 0000000... D	file3
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+:100644 100644 35d242b... 7289e35... M	dir/sub
+:100644 100644 01e79c3... f4615da... M	file0
+:000000 100644 0000000... 7289e35... A	file3
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+:100644 100644 8422d40... cead32e... M	dir/sub
+:000000 100644 0000000... b1e6722... A	file1
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+:100644 100644 35d242b... 8422d40... M	dir/sub
+:100644 100644 01e79c3... b414108... M	file0
+:100644 000000 01e79c3... 0000000... D	file2
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.25.1

[PATCH v2 3/9] t4013: test "git log -m --stat"

[Sergey Organov]

This is to ensure we won't break different diff formats when we start
to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh           |  1 +
 t/t4013/diff.log_-m_--stat_master | 66 +++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)
 create mode 100644 t/t4013/diff.log_-m_--stat_master

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 1809355f9bb1..e53ca7aa503f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -338,6 +338,7 @@ log -m -p master
 log --cc -m -p master
 log -c -m -p master
 log -m --raw master
+log -m --stat master
 log -SF master
 log -S F master
 log -SF -p master
diff --git a/t/t4013/diff.log_-m_--stat_master b/t/t4013/diff.log_-m_--stat_master
new file mode 100644
index 000000000000..c7db084fd905
--- /dev/null
+++ b/t/t4013/diff.log_-m_--stat_master
@@ -0,0 +1,66 @@
+$ git log -m --stat master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 4 ++++
+ file0   | 3 +++
+ file1   | 3 +++
+ file2   | 3 ---
+ file3   | 4 ----
+ 5 files changed, 10 insertions(+), 7 deletions(-)
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file3   | 4 ++++
+ 3 files changed, 9 insertions(+)
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+ dir/sub | 2 ++
+ file1   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file2   | 3 ---
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.25.1

[PATCH v2 4/9] t4013: test "git diff-index -m"

[Sergey Organov]

-m in "git diff-index" means "match missing", that differs
from its meaning in "git diff". Let's check it in diff-index.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e53ca7aa503f..c35cc73d08bd 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -493,6 +493,19 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '
 	test_cmp expected actual
 '
 
+# -m in "git diff-index" means "match missing", that differs
+# from its meaning in "git diff". Let's check it in diff-index.
+# The line in the output for removed file should disappear when
+# we provide -m in diff-index.
+test_expect_success 'git diff-index -m' '
+	rm -f file1 &&
+	git diff-index HEAD >without-m &&
+	lines_count=$(wc -l <without-m) &&
+	git diff-index -m HEAD >with-m &&
+	git restore file1 &&
+	test_line_count = $((lines_count - 1)) with-m
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

[PATCH v2 5/9] diff-merges: move specific diff-index "-m" handling to diff-index

[Sergey Organov]

Move specific handling of "-m" for diff-index to diff-index.c, so
diff-merges is left to handle only diff for merges options.

Being a better design by itself, this is especially essential in
preparation for letting -m imply -p, as "diff-index -m" obviously
should not imply -p, as it's entirely unrelated.

To handle this, in addition to moving specific diff-index "-m" code
out of diff-merges, we introduce new

  diff_merges_suppress_options_parsing()

and call it before generic options processing in cmd_diff_index().

This new diff_merges_suppress_options_parsing() could then be reused
and called before invocations of setup_revisions() for other commands
that don't need --diff-merges options, but that's outside of the scope
of these patch series.

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

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 176fe7ff2b4e..cf09559e422d 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "config.h"
 #include "diff.h"
+#include "diff-merges.h"
 #include "commit.h"
 #include "revision.h"
 #include "builtin.h"
@@ -27,6 +28,12 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	rev.abbrev = 0;
 	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".
+	 */
+	diff_merges_suppress_options_parsing();
+
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -35,6 +42,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 			option |= DIFF_INDEX_CACHED;
 		else if (!strcmp(arg, "--merge-base"))
 			option |= DIFF_INDEX_MERGE_BASE;
+		else if (!strcmp(arg, "-m"))
+			rev.match_missing = 1;
 		else
 			usage(diff_cache_usage);
 	}
diff --git a/diff-merges.c b/diff-merges.c
index f3a9daed7e05..9ca00cdd0cc6 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -6,6 +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 void suppress(struct rev_info *revs)
 {
@@ -30,17 +31,6 @@ static void set_first_parent(struct rev_info *revs)
 	revs->first_parent_merges = 1;
 }
 
-static void set_m(struct rev_info *revs)
-{
-	/*
-	 * To "diff-index", "-m" means "match missing", and to the "log"
-	 * family of commands, it means "show default diff for merges". Set
-	 * both fields appropriately.
-	 */
-	set_to_default(revs);
-	revs->match_missing = 1;
-}
-
 static void set_combined(struct rev_info *revs)
 {
 	suppress(revs);
@@ -101,14 +91,22 @@ int diff_merges_config(const char *value)
 	return 0;
 }
 
+void diff_merges_suppress_options_parsing(void)
+{
+	suppress_parsing = 1;
+}
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 {
 	int argcount = 1;
 	const char *optarg;
 	const char *arg = argv[0];
 
+	if (suppress_parsing)
+		return 0;
+
 	if (!strcmp(arg, "-m")) {
-		set_m(revs);
+		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->combined_imply_patch = 1;
@@ -155,6 +153,9 @@ 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 09d9a6c9a4fb..b5d57f6563e3 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -11,6 +11,8 @@ struct rev_info;
 
 int diff_merges_config(const char *value);
 
+void diff_merges_suppress_options_parsing(void);
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
 void diff_merges_suppress(struct rev_info *revs);
-- 
2.25.1

[PATCH v2 6/9] git-svn: stop passing "-m" to "git rev-list"

[Sergey Organov]

rev-list doesn't utilize -m. It happens to eat it silently, so this
bug went unnoticed.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 perl/Git/SVN.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index f6f1dc03c608..35ff5a68963d 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1636,7 +1636,7 @@ sub has_no_changes {
 	my $commit = shift;
 
 	my @revs = split / /, command_oneline(
-		qw(rev-list --parents -1 -m), $commit);
+		qw(rev-list --parents -1), $commit);
 
 	# Commits with no parents, e.g. the start of a partial branch,
 	# have changes by definition.
-- 
2.25.1

[PATCH v2 7/9] stash list: stop passing "-m" to "git log"

[Sergey Organov]

Passing "-m" in "git log --first-parent -m" is not needed as
--first-parent implies --diff-merges=first-parent anyway. OTOH, it
will stop being harmless once we let "-m" imply "-p".

While we are at it, fix corresponding test description in t3903-stash
to match what it actually tests.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 builtin/stash.c  | 2 +-
 t/t3903-stash.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index d68ed784d2af..fe8e97428808 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -761,7 +761,7 @@ static int list_stash(int argc, const char **argv, const char *prefix)
 
 	cp.git_cmd = 1;
 	strvec_pushl(&cp.args, "log", "--format=%gd: %gs", "-g",
-		     "--first-parent", "-m", NULL);
+		     "--first-parent", NULL);
 	strvec_pushv(&cp.args, argv);
 	strvec_push(&cp.args, ref_stash);
 	strvec_push(&cp.args, "--");
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5f282ecf6175..873aa56e359d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -859,7 +859,7 @@ test_expect_success 'setup stash with index and worktree changes' '
 	git stash
 '
 
-test_expect_success 'stash list implies --first-parent -m' '
+test_expect_success 'stash list -p shows simple diff' '
 	cat >expect <<-EOF &&
 	stash@{0}
 
-- 
2.25.1

[PATCH v2 8/9] diff-merges: rename "combined_imply_patch" to "merges_imply_patch"

[Sergey Organov]

This is refactoring change in preparation for the next commit that
will let -m imply -p.

The old name doesn't match the intention to let not only -c/-cc imply
-p, but also -m, that is not a "combined" format, so we rename the
flag accordingly.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 10 +++++-----
 revision.h    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 9ca00cdd0cc6..d897fd8a2933 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -15,7 +15,7 @@ static void suppress(struct rev_info *revs)
 	revs->combine_merges = 0;
 	revs->dense_combined_merges = 0;
 	revs->combined_all_paths = 0;
-	revs->combined_imply_patch = 0;
+	revs->merges_imply_patch = 0;
 	revs->merges_need_diff = 0;
 }
 
@@ -109,10 +109,10 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--cc")) {
 		set_dense_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
 		suppress(revs);
 	} else if (!strcmp(arg, "--combined-all-paths")) {
@@ -162,9 +162,9 @@ void diff_merges_setup_revs(struct rev_info *revs)
 		revs->first_parent_merges = 0;
 	if (revs->combined_all_paths && !revs->combine_merges)
 		die("--combined-all-paths makes no sense without -c or --cc");
-	if (revs->combined_imply_patch)
+	if (revs->merges_imply_patch)
 		revs->diff = 1;
-	if (revs->combined_imply_patch || revs->merges_need_diff) {
+	if (revs->merges_imply_patch || revs->merges_need_diff) {
 		if (!revs->diffopt.output_format)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 	}
diff --git a/revision.h b/revision.h
index 93aa012f518e..17698cb51aca 100644
--- a/revision.h
+++ b/revision.h
@@ -193,10 +193,10 @@ struct rev_info {
 			/* Diff-merge flags */
 			explicit_diff_merges: 1,
 			merges_need_diff: 1,
+			merges_imply_patch:1,
 			separate_merges: 1,
 			combine_merges:1,
 			combined_all_paths:1,
-			combined_imply_patch:1,
 			dense_combined_merges:1,
 			first_parent_merges:1;
 
-- 
2.25.1

[PATCH v2 9/9] diff-merges: let "-m" imply "-p"

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p on
one side, and -m that did not imply -p on the other side.

Change corresponding test accordingly, as "log -m" output should now
match one from "log -m -p", rather than from just "log".

Change documentation accordingly.

NOTES:

After this patch

  git log -m

produces diffs without need to provide -p as well, that improves both
consistency and usability. It gets even more useful if one sets
"log.diffMerges" configuration variable to "first-parent" to force -m
produce usual diff with respect to first parent only.

This patch, however, does not change behavior when specific diff
format is explicitly provided on the command-line, so that commands
like

  git log -m --raw
  git log -m --stat

are not affected.

Exact historical semantics of -m is still provided by
--diff-merges=separate.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt | 8 ++++----
 diff-merges.c                  | 1 +
 t/t4013-diff-various.sh        | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 530d1159141f..32e6dee5ac3b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -49,10 +49,9 @@ ifdef::git-log[]
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
-	the default format. `-m` will produce the output only if `-p`
-	is given as well. The default format could be changed using
+	the default format. The default format could be changed using
 	`log.diffMerges` configuration parameter, which default value
-	is `separate`.
+	is `separate`. `-m` implies `-p`.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -62,7 +61,8 @@ ifdef::git-log[]
 --diff-merges=separate:::
 	This makes merge commits show the full diff with respect to
 	each of the parents. Separate log entry and diff is generated
-	for each parent.
+	for each parent. This is the format that `-m` produced
+	historically.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
diff --git a/diff-merges.c b/diff-merges.c
index d897fd8a2933..0dfcaa1b11b0 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -107,6 +107,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!strcmp(arg, "-m")) {
 		set_to_default(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index c35cc73d08bd..15ca3c75bff7 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -454,8 +454,8 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
-test_expect_success 'log -m matches pure log' '
-	git log master >result &&
+test_expect_success 'log -m matches log -m -p' '
+	git log -m -p master >result &&
 	process_diffs result >expected &&
 	git log -m >result &&
 	process_diffs result >actual &&
-- 
2.25.1

[PATCH v3 00/10] diff-merges: let -m imply -p

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p, on
one side, and -m that did not imply -p, on the other side.

Only the last patch is the actual functional change. The rest of
patches are additional tests and refactorings that are not expected to
alter existing behaviors.

After these patches

  git log -m

produces diffs without need to provide -p as well, that improves both
consistency and usability. It gets even more useful if one sets
"log.diffMerges" configuration variable to "first-parent" to force -m
produce usual diff with respect to first parent only.

These series, however, don't change behavior when specific diff format
is explicitly provided on the command-line, so that commands like

  git log -m --raw
  git log -m --stat

are not affected, nor do they change commands where specific diff
format is active by default, such as:

  git diff-tree -m

It's also worth to be noticed that exact historical semantics of -m is
still provided by --diff-merges=separate.

Updates in v3:

  * Add test for "diff-tree -m" and mention it in descriptions

  * Fix one more typo in descriptions

Updates in v2:

  * Fix style and typos in descriptions

Updates in v1:

  * Stop parsing distinct diff-index options beforehand, as it could
    cause unexpected behaviors. Implement different strategy to avoid
    clash of diff-index "-m" and diff-merges "-m".

  * Added tests for "git log -m --raw" and "git log -m".

Sergey Organov (10):
  t4013: test that "-m" alone has no effect in "git log"
  t4013: test "git log -m --raw"
  t4013: test "git log -m --stat"
  t4013: test "git diff-tree -m"
  t4013: test "git diff-index -m"
  diff-merges: move specific diff-index "-m" handling to diff-index
  git-svn: stop passing "-m" to "git rev-list"
  stash list: stop passing "-m" to "git log"
  diff-merges: rename "combined_imply_patch" to "merges_imply_patch"
  diff-merges: let "-m" imply "-p"

 Documentation/diff-options.txt    |  8 ++--
 builtin/diff-index.c              |  9 +++++
 builtin/stash.c                   |  2 +-
 diff-merges.c                     | 36 +++++++++--------
 diff-merges.h                     |  2 +
 perl/Git/SVN.pm                   |  2 +-
 revision.h                        |  2 +-
 t/t3903-stash.sh                  |  2 +-
 t/t4013-diff-various.sh           | 24 +++++++++++
 t/t4013/diff.diff-tree_-m_master  | 11 ++++++
 t/t4013/diff.log_-m_--raw_master  | 61 ++++++++++++++++++++++++++++
 t/t4013/diff.log_-m_--stat_master | 66 +++++++++++++++++++++++++++++++
 12 files changed, 200 insertions(+), 25 deletions(-)
 create mode 100644 t/t4013/diff.diff-tree_-m_master
 create mode 100644 t/t4013/diff.log_-m_--raw_master
 create mode 100644 t/t4013/diff.log_-m_--stat_master

Interdiff against v2:
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 15ca3c75bff7..7fadc985cccd 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -293,6 +293,7 @@ diff-tree --stat initial mode
 diff-tree --summary initial mode
 
 diff-tree master
+diff-tree -m master
 diff-tree -p master
 diff-tree -p -m master
 diff-tree -c master
diff --git a/t/t4013/diff.diff-tree_-m_master b/t/t4013/diff.diff-tree_-m_master
new file mode 100644
index 000000000000..6d0a2207fb30
--- /dev/null
+++ b/t/t4013/diff.diff-tree_-m_master
@@ -0,0 +1,11 @@
+$ git diff-tree -m master
+59d314ad6f356dd08601a4cd5e530381da3e3c64
+:040000 040000 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 0564e026437809817a64fff393079714b6dd4628 M	dir
+:100644 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+59d314ad6f356dd08601a4cd5e530381da3e3c64
+:040000 040000 f977ed46ae6873c1c30ab878e15a4accedc3618b 0564e026437809817a64fff393079714b6dd4628 M	dir
+:100644 100644 f4615da674c09df322d6ba8d6b21ecfb1b1ba510 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
+:100644 000000 7289e35bff32727c08dda207511bec138fdb9ea5 0000000000000000000000000000000000000000 D	file3
+$
-- 
2.25.1

[PATCH v3 01/10] t4013: test that "-m" alone has no effect in "git log"

[Sergey Organov]

This is to notice current behavior that we are going to change when we
start to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 87def81699bf..e9f67cd24351 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+test_expect_success 'log -m matches pure log' '
+	git log master >result &&
+	process_diffs result >expected &&
+	git log -m >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	git log -p --diff-merges=separate master >result &&
 	process_diffs result >expected &&
-- 
2.25.1

[PATCH v3 02/10] t4013: test "git log -m --raw"

[Sergey Organov]

This is to ensure we won't break different diff formats when we start
to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh          |  1 +
 t/t4013/diff.log_-m_--raw_master | 61 ++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 t/t4013/diff.log_-m_--raw_master

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e9f67cd24351..1809355f9bb1 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -337,6 +337,7 @@ log -m -p --first-parent master
 log -m -p master
 log --cc -m -p master
 log -c -m -p master
+log -m --raw master
 log -SF master
 log -S F master
 log -SF -p master
diff --git a/t/t4013/diff.log_-m_--raw_master b/t/t4013/diff.log_-m_--raw_master
new file mode 100644
index 000000000000..cd2ecc462832
--- /dev/null
+++ b/t/t4013/diff.log_-m_--raw_master
@@ -0,0 +1,61 @@
+$ git log -m --raw master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 cead32e... 992913c... M	dir/sub
+:100644 100644 b414108... 10a8a9f... M	file0
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+:100644 100644 7289e35... 992913c... M	dir/sub
+:100644 100644 f4615da... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+:100644 000000 7289e35... 0000000... D	file3
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+:100644 100644 35d242b... 7289e35... M	dir/sub
+:100644 100644 01e79c3... f4615da... M	file0
+:000000 100644 0000000... 7289e35... A	file3
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+:100644 100644 8422d40... cead32e... M	dir/sub
+:000000 100644 0000000... b1e6722... A	file1
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+:100644 100644 35d242b... 8422d40... M	dir/sub
+:100644 100644 01e79c3... b414108... M	file0
+:100644 000000 01e79c3... 0000000... D	file2
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.25.1

[PATCH v3 03/10] t4013: test "git log -m --stat"

[Sergey Organov]

This is to ensure we won't break different diff formats when we start
to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh           |  1 +
 t/t4013/diff.log_-m_--stat_master | 66 +++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)
 create mode 100644 t/t4013/diff.log_-m_--stat_master

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 1809355f9bb1..e53ca7aa503f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -338,6 +338,7 @@ log -m -p master
 log --cc -m -p master
 log -c -m -p master
 log -m --raw master
+log -m --stat master
 log -SF master
 log -S F master
 log -SF -p master
diff --git a/t/t4013/diff.log_-m_--stat_master b/t/t4013/diff.log_-m_--stat_master
new file mode 100644
index 000000000000..c7db084fd905
--- /dev/null
+++ b/t/t4013/diff.log_-m_--stat_master
@@ -0,0 +1,66 @@
+$ git log -m --stat master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (from c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a)
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+ dir/sub | 4 ++++
+ file0   | 3 +++
+ file1   | 3 +++
+ file2   | 3 ---
+ file3   | 4 ----
+ 5 files changed, 10 insertions(+), 7 deletions(-)
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file3   | 4 ++++
+ 3 files changed, 9 insertions(+)
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+ dir/sub | 2 ++
+ file1   | 3 +++
+ 2 files changed, 5 insertions(+)
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file2   | 3 ---
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.25.1

[PATCH v3 04/10] t4013: test "git diff-tree -m"

[Sergey Organov]

We want to ensure we don't affect plumbing commands with our changes
of "-m" semantics, so add corresponding test.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh          |  1 +
 t/t4013/diff.diff-tree_-m_master | 11 +++++++++++
 2 files changed, 12 insertions(+)
 create mode 100644 t/t4013/diff.diff-tree_-m_master

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e53ca7aa503f..bdc23b1180b0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -293,6 +293,7 @@ diff-tree --stat initial mode
 diff-tree --summary initial mode
 
 diff-tree master
+diff-tree -m master
 diff-tree -p master
 diff-tree -p -m master
 diff-tree -c master
diff --git a/t/t4013/diff.diff-tree_-m_master b/t/t4013/diff.diff-tree_-m_master
new file mode 100644
index 000000000000..6d0a2207fb30
--- /dev/null
+++ b/t/t4013/diff.diff-tree_-m_master
@@ -0,0 +1,11 @@
+$ git diff-tree -m master
+59d314ad6f356dd08601a4cd5e530381da3e3c64
+:040000 040000 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 0564e026437809817a64fff393079714b6dd4628 M	dir
+:100644 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+59d314ad6f356dd08601a4cd5e530381da3e3c64
+:040000 040000 f977ed46ae6873c1c30ab878e15a4accedc3618b 0564e026437809817a64fff393079714b6dd4628 M	dir
+:100644 100644 f4615da674c09df322d6ba8d6b21ecfb1b1ba510 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
+:100644 000000 7289e35bff32727c08dda207511bec138fdb9ea5 0000000000000000000000000000000000000000 D	file3
+$
-- 
2.25.1

[PATCH v3 05/10] t4013: test "git diff-index -m"

[Sergey Organov]

-m in "git diff-index" means "match missing", that differs
from its meaning in "git diff". Let's check it in diff-index.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index bdc23b1180b0..e561a8e48521 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -494,6 +494,19 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '
 	test_cmp expected actual
 '
 
+# -m in "git diff-index" means "match missing", that differs
+# from its meaning in "git diff". Let's check it in diff-index.
+# The line in the output for removed file should disappear when
+# we provide -m in diff-index.
+test_expect_success 'git diff-index -m' '
+	rm -f file1 &&
+	git diff-index HEAD >without-m &&
+	lines_count=$(wc -l <without-m) &&
+	git diff-index -m HEAD >with-m &&
+	git restore file1 &&
+	test_line_count = $((lines_count - 1)) with-m
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

[PATCH v3 06/10] diff-merges: move specific diff-index "-m" handling to diff-index

[Sergey Organov]

Move specific handling of "-m" for diff-index to diff-index.c, so
diff-merges is left to handle only diff for merges options.

Being a better design by itself, this is especially essential in
preparation for letting -m imply -p, as "diff-index -m" obviously
should not imply -p, as it's entirely unrelated.

To handle this, in addition to moving specific diff-index "-m" code
out of diff-merges, we introduce new

  diff_merges_suppress_options_parsing()

and call it before generic options processing in cmd_diff_index().

This new diff_merges_suppress_options_parsing() could then be reused
and called before invocations of setup_revisions() for other commands
that don't need --diff-merges options, but that's outside of the scope
of these patch series.

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

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 176fe7ff2b4e..cf09559e422d 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "config.h"
 #include "diff.h"
+#include "diff-merges.h"
 #include "commit.h"
 #include "revision.h"
 #include "builtin.h"
@@ -27,6 +28,12 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	rev.abbrev = 0;
 	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".
+	 */
+	diff_merges_suppress_options_parsing();
+
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -35,6 +42,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 			option |= DIFF_INDEX_CACHED;
 		else if (!strcmp(arg, "--merge-base"))
 			option |= DIFF_INDEX_MERGE_BASE;
+		else if (!strcmp(arg, "-m"))
+			rev.match_missing = 1;
 		else
 			usage(diff_cache_usage);
 	}
diff --git a/diff-merges.c b/diff-merges.c
index f3a9daed7e05..9ca00cdd0cc6 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -6,6 +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 void suppress(struct rev_info *revs)
 {
@@ -30,17 +31,6 @@ static void set_first_parent(struct rev_info *revs)
 	revs->first_parent_merges = 1;
 }
 
-static void set_m(struct rev_info *revs)
-{
-	/*
-	 * To "diff-index", "-m" means "match missing", and to the "log"
-	 * family of commands, it means "show default diff for merges". Set
-	 * both fields appropriately.
-	 */
-	set_to_default(revs);
-	revs->match_missing = 1;
-}
-
 static void set_combined(struct rev_info *revs)
 {
 	suppress(revs);
@@ -101,14 +91,22 @@ int diff_merges_config(const char *value)
 	return 0;
 }
 
+void diff_merges_suppress_options_parsing(void)
+{
+	suppress_parsing = 1;
+}
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 {
 	int argcount = 1;
 	const char *optarg;
 	const char *arg = argv[0];
 
+	if (suppress_parsing)
+		return 0;
+
 	if (!strcmp(arg, "-m")) {
-		set_m(revs);
+		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->combined_imply_patch = 1;
@@ -155,6 +153,9 @@ 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 09d9a6c9a4fb..b5d57f6563e3 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -11,6 +11,8 @@ struct rev_info;
 
 int diff_merges_config(const char *value);
 
+void diff_merges_suppress_options_parsing(void);
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
 void diff_merges_suppress(struct rev_info *revs);
-- 
2.25.1

[PATCH v3 07/10] git-svn: stop passing "-m" to "git rev-list"

[Sergey Organov]

rev-list doesn't utilize -m. It happens to eat it silently, so this
bug went unnoticed.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 perl/Git/SVN.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index f6f1dc03c608..35ff5a68963d 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1636,7 +1636,7 @@ sub has_no_changes {
 	my $commit = shift;
 
 	my @revs = split / /, command_oneline(
-		qw(rev-list --parents -1 -m), $commit);
+		qw(rev-list --parents -1), $commit);
 
 	# Commits with no parents, e.g. the start of a partial branch,
 	# have changes by definition.
-- 
2.25.1

[PATCH v3 08/10] stash list: stop passing "-m" to "git log"

[Sergey Organov]

Passing "-m" in "git log --first-parent -m" is not needed as
--first-parent implies --diff-merges=first-parent anyway. OTOH, it
will stop being harmless once we let "-m" imply "-p".

While we are at it, fix corresponding test description in t3903-stash
to match what it actually tests.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 builtin/stash.c  | 2 +-
 t/t3903-stash.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index d68ed784d2af..fe8e97428808 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -761,7 +761,7 @@ static int list_stash(int argc, const char **argv, const char *prefix)
 
 	cp.git_cmd = 1;
 	strvec_pushl(&cp.args, "log", "--format=%gd: %gs", "-g",
-		     "--first-parent", "-m", NULL);
+		     "--first-parent", NULL);
 	strvec_pushv(&cp.args, argv);
 	strvec_push(&cp.args, ref_stash);
 	strvec_push(&cp.args, "--");
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5f282ecf6175..873aa56e359d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -859,7 +859,7 @@ test_expect_success 'setup stash with index and worktree changes' '
 	git stash
 '
 
-test_expect_success 'stash list implies --first-parent -m' '
+test_expect_success 'stash list -p shows simple diff' '
 	cat >expect <<-EOF &&
 	stash@{0}
 
-- 
2.25.1

[PATCH v3 09/10] diff-merges: rename "combined_imply_patch" to "merges_imply_patch"

[Sergey Organov]

This is refactoring change in preparation for the next commit that
will let -m imply -p.

The old name doesn't match the intention to let not only -c/-cc imply
-p, but also -m, that is not a "combined" format, so we rename the
flag accordingly.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 10 +++++-----
 revision.h    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 9ca00cdd0cc6..d897fd8a2933 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -15,7 +15,7 @@ static void suppress(struct rev_info *revs)
 	revs->combine_merges = 0;
 	revs->dense_combined_merges = 0;
 	revs->combined_all_paths = 0;
-	revs->combined_imply_patch = 0;
+	revs->merges_imply_patch = 0;
 	revs->merges_need_diff = 0;
 }
 
@@ -109,10 +109,10 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		set_to_default(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--cc")) {
 		set_dense_combined(revs);
-		revs->combined_imply_patch = 1;
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
 		suppress(revs);
 	} else if (!strcmp(arg, "--combined-all-paths")) {
@@ -162,9 +162,9 @@ void diff_merges_setup_revs(struct rev_info *revs)
 		revs->first_parent_merges = 0;
 	if (revs->combined_all_paths && !revs->combine_merges)
 		die("--combined-all-paths makes no sense without -c or --cc");
-	if (revs->combined_imply_patch)
+	if (revs->merges_imply_patch)
 		revs->diff = 1;
-	if (revs->combined_imply_patch || revs->merges_need_diff) {
+	if (revs->merges_imply_patch || revs->merges_need_diff) {
 		if (!revs->diffopt.output_format)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 	}
diff --git a/revision.h b/revision.h
index 93aa012f518e..17698cb51aca 100644
--- a/revision.h
+++ b/revision.h
@@ -193,10 +193,10 @@ struct rev_info {
 			/* Diff-merge flags */
 			explicit_diff_merges: 1,
 			merges_need_diff: 1,
+			merges_imply_patch:1,
 			separate_merges: 1,
 			combine_merges:1,
 			combined_all_paths:1,
-			combined_imply_patch:1,
 			dense_combined_merges:1,
 			first_parent_merges:1;
 
-- 
2.25.1

[PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p on
one side, and -m that did not imply -p on the other side.

Change corresponding test accordingly, as "log -m" output should now
match one from "log -m -p", rather than from just "log".

Change documentation accordingly.

NOTES:

After this patch

  git log -m

produces diffs without need to provide -p as well, that improves both
consistency and usability. It gets even more useful if one sets
"log.diffMerges" configuration variable to "first-parent" to force -m
produce usual diff with respect to first parent only.

This patch, however, does not change behavior when specific diff
format is explicitly provided on the command-line, so that commands
like

  git log -m --raw
  git log -m --stat

are not affected, nor does it change commands where specific diff
format is active by default, such as:

  git diff-tree -m

It's also worth to be noticed that exact historical semantics of -m is
still provided by --diff-merges=separate.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt | 8 ++++----
 diff-merges.c                  | 1 +
 t/t4013-diff-various.sh        | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 530d1159141f..32e6dee5ac3b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -49,10 +49,9 @@ ifdef::git-log[]
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
-	the default format. `-m` will produce the output only if `-p`
-	is given as well. The default format could be changed using
+	the default format. The default format could be changed using
 	`log.diffMerges` configuration parameter, which default value
-	is `separate`.
+	is `separate`. `-m` implies `-p`.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -62,7 +61,8 @@ ifdef::git-log[]
 --diff-merges=separate:::
 	This makes merge commits show the full diff with respect to
 	each of the parents. Separate log entry and diff is generated
-	for each parent.
+	for each parent. This is the format that `-m` produced
+	historically.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
diff --git a/diff-merges.c b/diff-merges.c
index d897fd8a2933..0dfcaa1b11b0 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -107,6 +107,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!strcmp(arg, "-m")) {
 		set_to_default(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e561a8e48521..7fadc985cccd 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -455,8 +455,8 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
-test_expect_success 'log -m matches pure log' '
-	git log master >result &&
+test_expect_success 'log -m matches log -m -p' '
+	git log -m -p master >result &&
 	process_diffs result >expected &&
 	git log -m >result &&
 	process_diffs result >actual &&
-- 
2.25.1

Re: [PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Jonathan Nieder]

Hi Sergey,

Sergey Organov wrote:

> Fix long standing inconsistency between -c/--cc that do imply -p on
> one side, and -m that did not imply -p on the other side.

As I mentioned before, I quite like this change for interactive use.

But:

[...]
> It's also worth to be noticed that exact historical semantics of -m is
> still provided by --diff-merges=separate.

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

Anyway, the reason I write is that this ended up tripping up some
scripts, namely Rust's bootstrap.py
(https://github.com/rust-lang/rust/commit/df004df3a79b70b4af2b8c267457a5be76bb0d85):

	git log --author=bors --format=%H -n1 \
		-m --first-parent \
		-- src/llvm-project

It's not clear what that *meant* the -m to do --- perhaps they intended
it as an abbreviation for --merges.  That was fixed in Rust by
https://github.com/rust-lang/rust/pull/87513; in the current code at
https://github.com/rust-lang/rust/blob/master/src/bootstrap/bootstrap.py
Rust now uses

	git log --author=bors --format=%H -n1 \
		--no-patch --first-parent \
		-- [etc]

There's also an open pull request at
https://github.com/rust-lang/rust/pull/87532 to simplify it further, to

	git rev-list --author=bors@rust-lang.org -n1 \
		--merges --first-parent HEAD \
		-- [etc]

In any event, the code using -m was pretty clearly a typo, and people
trying to build current Rust won't be affected since it's fixed
already, so this might not be too worrisome.

What happens if someone wants to build an older version of Rust?
bootstrap.py is "symlinked" from x.py at the toplevel of a Rust
distribution; the README explains

	## Installing from Source

	The Rust build system uses a Python script called `x.py` to build the compiler,
	which manages the bootstrapping process. It lives in the root of the project.

	The `x.py` command can be run directly on most systems in the following format:

	```sh
	./x.py <subcommand> [flags]
	```

so this tool is fundamental to everything.  The relevant code using 'git
log -m' is used in the

	if self.downloading_llvm() and stage0:

case to find out how recently llvm changed, in order to check that we
have downloaded that recent of a version of llvm.  It has a
not-too-complicated workaround: if you build LLVM from source using
the src/llvm-project submodule then this logic does not get triggered.

In other words, I don't think this issue will be _too_ problematic for
people working with the Rust project.  Hudson or Taylor (cc-ed) may be
able to correct me if that's wrong.

Still, it does feel a bit like we've pulled the rug from underneath
script authors.  "git log --format=%H" is generally a pretty stable
tool, and here we've changed it when passing -m from not printing
diffs to printing diffs.  What do you think we should do?

Some possibilities:

 a. Revert 'diff-merges: let "-m" imply "-p"'.  This buys us time to
    make a more targeted change, make the change more gradually in a
    future release, or just stop encouraging use of "-m" in docs.

 b. Make "-m" imply "-p", except in some more 'script-ish'
    circumstances (e.g. when using log --format with a format string)

 c. Go ahead with the change and advertise it in release notes.

Searching for other examples using
https://codesearch.debian.net/search?q=%5Cbgit%5Cb.*%5Cblog%5Cb.*-m%5Cb&literal=0,
I find that almost all uses of "git log -m" also include "-p", so (c)
is kind of tempting.  What do you think?

Thanks,
Jonathan

[PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Jonathan Nieder]

This reverts commit f5bfcc823ba242a46e20fb6f71c9fbf7ebb222fe, which
made "git log -m" imply "--patch" by default.  The logic was that
"-m", which makes diff generation for merges perform a diff against
each parent, has no use unless I am viewing the diff, so we could save
the user some typing by turning on display of the resulting diff
automatically.  That wasn't expected to adversely affect scripts
because scripts would either be using a command like "git diff-tree"
that already emits diffs by default or would be combining -m with a
diff generation option such as --name-status.  By saving typing for
interactive use without adversely affecting scripts in the wild, it
would be a pure improvement.

The problem is that although diff generation options are only relevant
for the displayed diff, a script author can imagine them affecting
path limiting.  For example, I might run

	git log -w --format=%H -- README

hoping to list commits that edited README, excluding whitespace-only
changes.  In fact, a whitespace-only change is not TREESAME so the use
of -w here has no effect (since we don't apply these diff generation
flags to the diff_options struct rev_info::pruning used for this
purpose), but the documentation suggests that it should work

	Suppose you specified foo as the <paths>. We shall call
	commits that modify foo !TREESAME, and the rest TREESAME. (In
	a diff filtered for foo, they look different and equal,
	respectively.)

and a script author who has not tested whitespace-only changes
wouldn't notice.

Similarly, a script author could include

	git log -m --first-parent --format=%H -- README

to filter the first-parent history for commits that modified README.
The -m is a no-op but it reflects the script author's intent.  For
example, until 1e20a407fe2 (stash list: stop passing "-m" to "git
log", 2021-05-21), "git stash list" did this.

As a result, we can't safely change "-m" to imply "-p" without fear of
breaking such scripts.  Restore the previous behavior.

Noticed because Rust's src/bootstrap/bootstrap.py made use of this
same construct: https://github.com/rust-lang/rust/pull/87513.  That
script has been updated to omit the unnecessary "-m" option, but we
can expect other scripts in the wild to have similar expectations.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Joshua Nelson wrote[1]:
> Jonathan Nieder wrote:

>> What happens if someone wants to build an older version of Rust?
>
> For what it's worth, almost no one builds old versions of rust from source
> except for distros, and distros wouldn't ever set download-ci-llvm = true. So
> this shouldn't affect anyone in practice now that we've removed `-m` on master.

Thanks.  With that out of the way, I started thinking more clearly
about the intent behind this use of `-m` and I'm starting to think it
wasn't a typo after all.

As a result, in terms of

>>  a. Revert 'diff-merges: let "-m" imply "-p"'.  This buys us time to
>>     make a more targeted change, make the change more gradually in a
>>     future release, or just stop encouraging use of "-m" in docs.
>>
>>  b. Make "-m" imply "-p", except in some more 'script-ish'
>>     circumstances (e.g. when using log --format with a format string)
>>
>>  c. Go ahead with the change and advertise it in release notes.

now I lean toward (a).  How about this patch?

Thanks,
Jonathan

[1] https://lore.kernel.org/git/CAJ+j++Vj1gY93QuKDhDODXOJGXTiFFEzy0Oew+LWD7a5e7iaTA@mail.gmail.com/

 Documentation/diff-options.txt | 8 ++++----
 diff-merges.c                  | 1 -
 t/t4013-diff-various.sh        | 4 ++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 0aebe832057..c89d530d3d1 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -49,9 +49,10 @@ ifdef::git-log[]
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
-	the default format. The default format could be changed using
+	the default format. `-m` will produce the output only if `-p`
+	is given as well. The default format could be changed using
 	`log.diffMerges` configuration parameter, which default value
-	is `separate`. `-m` implies `-p`.
+	is `separate`.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -61,8 +62,7 @@ ifdef::git-log[]
 --diff-merges=separate:::
 	This makes merge commits show the full diff with respect to
 	each of the parents. Separate log entry and diff is generated
-	for each parent. This is the format that `-m` produced
-	historically.
+	for each parent.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
diff --git a/diff-merges.c b/diff-merges.c
index 0dfcaa1b11b..d897fd8a293 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -107,7 +107,6 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!strcmp(arg, "-m")) {
 		set_to_default(revs);
-		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 7fadc985ccc..e561a8e4852 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -455,8 +455,8 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
-test_expect_success 'log -m matches log -m -p' '
-	git log -m -p master >result &&
+test_expect_success 'log -m matches pure log' '
+	git log master >result &&
 	process_diffs result >expected &&
 	git log -m >result &&
 	process_diffs result >actual &&
-- 
2.32.0.605.g8dce9f2422-goog

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The problem is that although diff generation options are only relevant
> for the displayed diff, a script author can imagine them affecting
> path limiting.  For example, I might run

I am somewhat puzzled.  What does "can imagine" exactly mean and
justify this change?  A script author may imagine "git cat-file" can
be expected to meow, but the command actually does not meow and end
up disappointing the author, but that wouldn't justify a rename of
"cat-file" to something else.

> 	git log -w --format=%H -- README
>
> hoping to list commits that edited README, excluding whitespace-only
> changes.  In fact, a whitespace-only change is not TREESAME so the use
> of -w here has no effect (since we don't apply these diff generation
> flags to the diff_options struct rev_info::pruning used for this
> purpose), but the documentation suggests that it should work
>
> 	Suppose you specified foo as the <paths>. We shall call
> 	commits that modify foo !TREESAME, and the rest TREESAME. (In
> 	a diff filtered for foo, they look different and equal,
> 	respectively.)
>
> and a script author who has not tested whitespace-only changes
> wouldn't notice.

It would need to be corrected by a bugfix of either TREESAME
computation, or a documentation fix, I would think.  I fail to see
the similarity you perceive to the "-m" issue at hand, though.

> Similarly, a script author could include
>
> 	git log -m --first-parent --format=%H -- README
>
> to filter the first-parent history for commits that modified README.
> The -m is a no-op but it reflects the script author's intent.

So the expectation is with "-m" we'd give single parent commits on
the fp chain, and merges from side branches that change README, in
addition to merges from side branches that was forked way before the
README was updated on the trunk (hence had ancient README but the
merge kept the version from the trunk)?

> For
> example, until 1e20a407fe2 (stash list: stop passing "-m" to "git
> log", 2021-05-21), "git stash list" did this.

This is not a example that supports your conclusion, though.  The
reason why 288c67ca (stash: default listing to working-tree diff,
2014-08-06) added "-m" on the command line to make it:

  git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --

is to prepare for the users who may pass "-p" as part of the "$@";
they wil get no patches out of these merge commits that represent
stash entries otherwise, and they'd have to pass "-m -p" instead,
without the change.

> As a result, we can't safely change "-m" to imply "-p" without fear of
> breaking such scripts.  Restore the previous behavior.

So the above is *not* an example of a script that would have been
broken with this change.

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

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

>> Similarly, a script author could include
>>
>> 	git log -m --first-parent --format=%H -- README
>>
>> to filter the first-parent history for commits that modified README.
>> The -m is a no-op but it reflects the script author's intent.
>
> So the expectation is with "-m" we'd give single parent commits on
> the fp chain, and merges from side branches that change README, in
> addition to merges from side branches that was forked way before the
> README was updated on the trunk (hence had ancient README but the
> merge kept the version from the trunk)?
>
>> For
>> example, until 1e20a407fe2 (stash list: stop passing "-m" to "git
>> log", 2021-05-21), "git stash list" did this.
>
> This is not a example that supports your conclusion, though.  The
> reason why 288c67ca (stash: default listing to working-tree diff,
> 2014-08-06) added "-m" on the command line to make it:
>
>   git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
>
> is to prepare for the users who may pass "-p" as part of the "$@";
> they wil get no patches out of these merge commits that represent
> stash entries otherwise, and they'd have to pass "-m -p" instead,
> without the change.
>
>> As a result, we can't safely change "-m" to imply "-p" without fear of
>> breaking such scripts.  Restore the previous behavior.
>
> So the above is *not* an example of a script that would have been
> broken with this change.

Sorry, I have to take 70% of the above back.  While it is *not* an
example that shows an author's intent that changes from not just
trunk but all side branches to README are to be shown, the original
left after 288c67ca (stash: default listing to working-tree diff,
2014-08-06) would have been broken by the change you are proposing
to revert.  It used to be just "if you give -p we'll show a patch"
but if we make "-m" to mean "-m -p", it does get broken.

Which is BAD.

I am inclined to take the revert, but I do not think reverting it
alone will break more things than it fixes.

For example, 1e20a407 (stash list: stop passing "-m" to "git log",
2021-05-21) that dropped "-m" must be reverted as well, no?

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Jonathan Nieder]

Junio C Hamano wrote:

> For example, 1e20a407 (stash list: stop passing "-m" to "git log",
> 2021-05-21) that dropped "-m" must be reverted as well, no?

No, that change is fine.  The "-m" doesn't have an effect one way or
another after this revert.

Thanks,
Jonathan

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> For example, 1e20a407 (stash list: stop passing "-m" to "git log",
>> 2021-05-21) that dropped "-m" must be reverted as well, no?
>
> No, that change is fine.  The "-m" doesn't have an effect one way or
> another after this revert.

Ah, we are saved by the fact that "--first-parent" was made to imply
"-m", so a "-p" coming from the command line of "git stash list"
would do "log --first-parent -p" that shows the patch we want
without the need for "-m"... nice.

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Sergey Organov]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>
>>> For example, 1e20a407 (stash list: stop passing "-m" to "git log",
>>> 2021-05-21) that dropped "-m" must be reverted as well, no?
>>
>> No, that change is fine.  The "-m" doesn't have an effect one way or
>> another after this revert.
>
> Ah, we are saved by the fact that "--first-parent" was made to imply
> "-m", so a "-p" coming from the command line of "git stash list"
> would do "log --first-parent -p" that shows the patch we want
> without the need for "-m"... nice.

So, do I get it right that there is actually no reason to use "log
--first-parent -m" anymore, since the time the much older commit made
--first-parent imply -m?

If so, I'd object against this particular patch as the pros of patch
being reverted outweighs its cons, and the original patch never meant to
be entirely backward compatible in the first place, when it was
accepted.

Thanks,
-- Sergey Organov

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> So, do I get it right that there is actually no reason to use "log
> --first-parent -m" anymore, since the time the much older commit made
> --first-parent imply -m?

It was necessary for scripts to say

    git log --first-parent -m "$@"

if they wanted to optionally show a first-parent diff for a merge
when the user of the script passes "-p" in "$@" (and not to show
patch if the user does not pass "-p").  

That changed with 9ab89a24 (log: enable "-m" automatically with
"--first-parent", 2020-07-29).

After that commit, it no longer was needed, but it still was correct
to expect that no patch will be shown with "--first-parent -m",
unless you give "-p" at the same time.  The original change that the
patch under discussion reverted broke that expectation.

We need to note that the "-m" implied by "--first-parent" is "if we
were to show some comparison, do so also for merge commits", not the
"if the user says '-m', it must mean that the user wants to see
comparison, period, so make it imply '-p'".  The latter is what was
reverted.

> If so, I'd object against this particular patch as the pros of patch
> being reverted outweighs its cons, and the original patch never meant to
> be entirely backward compatible in the first place, when it was
> accepted.

I agree that we both (and if there were other reviewers, they too)
mistakenly thought that the change in behaviour was innocuous enough
when we queued the patch, but our mistakes were caught while the
topic was still cooking in 'next', and I have Jonathan to thank for
being extra careful.

Thanks.

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> So, do I get it right that there is actually no reason to use "log
>> --first-parent -m" anymore, since the time the much older commit made
>> --first-parent imply -m?
>
> It was necessary for scripts to say
>
>     git log --first-parent -m "$@"
>
> if they wanted to optionally show a first-parent diff for a merge
> when the user of the script passes "-p" in "$@" (and not to show
> patch if the user does not pass "-p").  
>
> That changed with 9ab89a24 (log: enable "-m" automatically with
> "--first-parent", 2020-07-29).

Yes, and since then it's no more needed to say "--first-parent -m" in
this case, as "--fist-parent" will do.

>
> After that commit, it no longer was needed, but it still was correct
> to expect that no patch will be shown with "--first-parent -m",
> unless you give "-p" at the same time.  The original change that the
> patch under discussion reverted broke that expectation.

>
> We need to note that the "-m" implied by "--first-parent" is "if we
> were to show some comparison, do so also for merge commits", not the
> "if the user says '-m', it must mean that the user wants to see
> comparison, period, so make it imply '-p'".  The latter is what was
> reverted.

Yes, there is minor backward incompatibility indeed, and that was
expected. This could be seen from the patch in the same series that
fixes "git stash" by removing unneeded -m.

The fix for the scripts is as simple as removing -m from "--first-parent
-m". It's a one-time change.

>
>> If so, I'd object against this particular patch as the pros of patch
>> being reverted outweighs its cons, and the original patch never meant to
>> be entirely backward compatible in the first place, when it was
>> accepted.
>
> I agree that we both (and if there were other reviewers, they too)
> mistakenly thought that the change in behaviour was innocuous enough
> when we queued the patch, but our mistakes were caught while the
> topic was still cooking in 'next', and I have Jonathan to thank for
> being extra careful.

So, what would be the procedure to get this change back, as this minor
backward incompatibility shouldn't be the show-stopper for the change
that otherwise is an improvement?

Thanks,
-- Sergey Organov

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> We need to note that the "-m" implied by "--first-parent" is "if we
>> were to show some comparison, do so also for merge commits", not the
>> "if the user says '-m', it must mean that the user wants to see
>> comparison, period, so make it imply '-p'".  The latter is what was
>> reverted.
>
> Yes, there is minor backward incompatibility indeed, and that was
> expected. This could be seen from the patch in the same series that
> fixes "git stash" by removing unneeded -m.
>
> The fix for the scripts is as simple as removing -m from "--first-parent
> -m". It's a one-time change.
> ...
>> I agree that we both (and if there were other reviewers, they too)
>> mistakenly thought that the change in behaviour was innocuous enough
>> when we queued the patch, but our mistakes were caught while the
>> topic was still cooking in 'next', and I have Jonathan to thank for
>> being extra careful.
>
> So, what would be the procedure to get this change back, as this minor
> backward incompatibility shouldn't be the show-stopper for the change
> that otherwise is an improvement?

Your repeating "minor" does not make it minor.  Anything you force
existing users and scripts to change is "fixing the scripts", but
"working around the breakage you brought to them", which is closer
to being a show-stopper.  I understand that you like this feature a
lot, but you'd need to be a bit more considerate to your users and
other people.

I think it is a design mistake to make a plain vanilla "-m" to imply
"-p" (or any "output of result of comparison"), simply because the
implication goes in the other direction, so there will never be "get
this change back", period, but see below.

"git log" when showing a commit and asked to "output result of
comparison" like patch, combined diff, raw diff, etc. would:

 - show the comparison for non-merge commits and when
   "--first-parent" is specified (the latter is natural since it
   makes us consistently pretend that the merges were squash
   merges).

 - shows the comparison for merge commits when -m is given.

but because "--cc" and "-c" (which are used to specify how the
result of comparison is shown; they are not about specifying if
"normally we show only non-merges" is disabled) do not make sense in
the context of non-merge commits (in other words, the user is better
off giving "-p" if merges are not to be shown), they are made to
imply "-m".  And that is a sensible design choice.  On the other
hand, "--raw" (which is used to specify how the result of comparison
is shown; it not about specifying if "normally we show only
non-merges" is disabled) does make sense in the context of non-merge
commits, so unlike "--cc"/"-c", it does not imply "-m".  And that
also is a sensible design choice.  "-p" falls into the same bucket
as "--raw", so it should not imply "-m".

But some folks may not like "log -p" to be silent about comparison
for merge commits (like you are).  To accomodate them, it might make
sense to have a configuration that says "I like -m, so when -p or
--raw or any 'how to show comparison result' option is given, please
make it imply '-m'", but it should not be the default.

Thanks.

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

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

> Your repeating "minor" does not make it minor.  Anything you force
> existing users and scripts to change is "fixing the scripts", but

... is NOT "fixing the scripts", of course.

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> We need to note that the "-m" implied by "--first-parent" is "if we
>>> were to show some comparison, do so also for merge commits", not the
>>> "if the user says '-m', it must mean that the user wants to see
>>> comparison, period, so make it imply '-p'".  The latter is what was
>>> reverted.
>>
>> Yes, there is minor backward incompatibility indeed, and that was
>> expected. This could be seen from the patch in the same series that
>> fixes "git stash" by removing unneeded -m.
>>
>> The fix for the scripts is as simple as removing -m from "--first-parent
>> -m". It's a one-time change.
>> ...
>>> I agree that we both (and if there were other reviewers, they too)
>>> mistakenly thought that the change in behaviour was innocuous enough
>>> when we queued the patch, but our mistakes were caught while the
>>> topic was still cooking in 'next', and I have Jonathan to thank for
>>> being extra careful.
>>
>> So, what would be the procedure to get this change back, as this minor
>> backward incompatibility shouldn't be the show-stopper for the change
>> that otherwise is an improvement?
>
> Your repeating "minor" does not make it minor.  Anything you force
> existing users and scripts to change is "fixing the scripts", but
> "working around the breakage you brought to them", which is closer
> to being a show-stopper.

Backward compatibility is important, no questions, but later on you
start to say that this change is a *design* mistake, so discussing
backward compatibility issues gets rather useless.

That said, scripts that still have "log --first-parent -m" are remnants
of former sub-optimal design that was improved by "--first-parent imply
-m" change about a year ago, and with current Git these scripts are
confusing anyway, so fixing them by removing the work around they
historically have would be a good idea no matter if the change in
question is accepted or not.

I mean your "working around the breakage you brought to them" is simply
wrong. These changes to the scripts in question are not work-arounds
they are rather improvements. It's "log --first-parent -m" in the
scripts that is a work-around, and getting rid of -m there is getting
rid of work-around that is not needed anymore (for about a year
already.)

> I understand that you like this feature a lot, but you'd need to be a
> bit more considerate to your users and other people.

First, I believe I *am* considerate, and second, I don't either "like"
or "dislike" the feature, personally. It's a matter of consistency of
UI, and the fact that such requests appear on the list (not from me)
only supports this view. There are other people here who do think this
is an improvement.

> I think it is a design mistake to make a plain vanilla "-m" to imply
> "-p" (or any "output of result of comparison"), simply because the
> implication goes in the other direction, so there will never be "get
> this change back", period, but see below.

Well, I thought we've already discussed this to death and agreed this is
an improvement, before I even started to implement the patches, and now
what? I'm confused.

I still believe it's reasonable for "git log -m" to output diffs without
need to explicitly specify -p, and I still see no design mistake here,
especially if it were implemented this way from the beginning,
especially given that "git log --cc" and and "git log -c" already behave
exactly this way.

>
> "git log" when showing a commit and asked to "output result of
> comparison" like patch, combined diff, raw diff, etc. would:
>
>  - show the comparison for non-merge commits and when
>    "--first-parent" is specified (the latter is natural since it
>    makes us consistently pretend that the merges were squash
>    merges).
>
>  - shows the comparison for merge commits when -m is given.
>
> but because "--cc" and "-c" (which are used to specify how the
> result of comparison is shown; they are not about specifying if
> "normally we show only non-merges" is disabled) do not make sense in
> the context of non-merge commits (in other words, the user is better
> off giving "-p" if merges are not to be shown), they are made to
> imply "-m". And that is a sensible design choice.  

No, sorry, they are made to imply -p, not -m.

> On the other
> hand, "--raw" (which is used to specify how the result of comparison
> is shown; it not about specifying if "normally we show only
> non-merges" is disabled) does make sense in the context of non-merge
> commits, so unlike "--cc"/"-c", it does not imply "-m".  And that
> also is a sensible design choice.  "-p" falls into the same bucket
> as "--raw", so it should not imply "-m".

Yes, but this has nothing to do with the patch in question, as -p still
doesn't imply -m with this patch. It's another way around: the patch
makes -m imply -p, the same way -c/--cc imply -p.

>
> But some folks may not like "log -p" to be silent about comparison
> for merge commits (like you are).

No, not me, and I didn't see anybody who insisted on it yet. It's fine
with me it's silent by default.

> To accomodate them, it might make sense to have a configuration that
> says "I like -m, so when -p or --raw or any 'how to show comparison
> result' option is given, please make it imply '-m'", but it should not
> be the default.

This has nothing to do with the patch in question, and I actually don't
like the idea, sorry.

Overall, my opinion is still that there is nothing wrong with "-m
implies -p", as implemented by the patch, as if user asks to output
diffs even for merge commits, it's likely they need diffs for *all* of
them. This is again consistent with how -c/--cc work.

Now, only provided we *again* and *finally* agree that -m should better
imply -p, we can get back to discussing backward incompatibility this
change does introduce, and how to get transition smoother if it needs to
be.

Thanks,
-- Sergey Organov

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> I am somewhat puzzled.  What does "can imagine" exactly mean and
> justify this change?  A script author may imagine "git cat-file" can
> be expected to meow, but the command actually does not meow and end
> up disappointing the author, but that wouldn't justify a rename of
> "cat-file" to something else.

Sorry for the lack of clarity.  I was describing what leads a script
author to include "-m" in a place where it has no effect.

You might be inclined to wonder why it matters _why_ a script author
would do such a thing, if the script author is wrong.  To me, it
matters because it allows us to estimate how common it is for scripts
to use "-m" in this way.

The motivating example (Rust) shows that there is at least one script
that _did_ use "-m" in this way.  Rust has mitigation, but the above
logic leads me to believe that they are not the only project that will
be affected.  And more generally, when a script author has a
reasonable reason to believe something will work, they write scripts
where it _does_ work, and then an update breaks their script, I think
it's reasonable for them not to be happy.

Jonathan

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Johannes Sixt]

Am 07.08.21 um 03:55 schrieb Jonathan Nieder:
> The motivating example (Rust) shows that there is at least one script
> that _did_ use "-m" in this way.  Rust has mitigation, but the above
> logic leads me to believe that they are not the only project that will
> be affected.  And more generally, when a script author has a
> reasonable reason to believe something will work, they write scripts
> where it _does_ work, and then an update breaks their script, I think
> it's reasonable for them not to be happy.

As you know, we have "plumbing" commands with a stable interface and
"porcelain" commands for which we reserve to change the behavior without
advance notice. By your reasoning we would not need to distinguish
between the two categories and were forced to keep all behavior stable.
This undoing of a behavior change in a "porcelain" command with the
argument that one script depended on the old behavior and that others
might do so as well would set an unwanted precedent.

Perhaps we need to point script authors to "plumbing" commands more clearly?

(BTW, I have no opinion on whether -m should or should not imply -p.)

-- Hannes

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Jonathan Nieder]

Hi Hannes,

Johannes Sixt wrote:
> Am 07.08.21 um 03:55 schrieb Jonathan Nieder:

>> The motivating example (Rust) shows that there is at least one script
>> that _did_ use "-m" in this way.  Rust has mitigation, but the above
>> logic leads me to believe that they are not the only project that will
>> be affected.  And more generally, when a script author has a
>> reasonable reason to believe something will work, they write scripts
>> where it _does_ work, and then an update breaks their script, I think
>> it's reasonable for them not to be happy.
>
> As you know, we have "plumbing" commands with a stable interface and
> "porcelain" commands for which we reserve to change the behavior without
> advance notice. By your reasoning we would not need to distinguish
> between the two categories and were forced to keep all behavior stable.
> This undoing of a behavior change in a "porcelain" command with the
> argument that one script depended on the old behavior and that others
> might do so as well would set an unwanted precedent.

Hm, this is worth elucidating a bit more, since I am definitely in
favor of continuing to change porcelain commands for the better where
we can.  If we decide that "git log --format=<fmt>" is no longer part
of the stable scripting interface we provide, then that would be a
huge change for our callers (and it's probably too late), but I would
certainly be in favor of us going back in time and doing that. :)

More generally, we've been able to make changes to porcelain commands
that don't hurt our ability to act as a platform for scripts, and I
want us to continue to be able to do that.  "Do not break any script"
is certainly not the standard I think we should apply, as illustrated
by my thoughts upthread when I thought '-m' in this Rust example was a
typo.

But by now it's very clear to me that it was not a typo.

In other words:

- this isn't only about one obscure script.  The point of the "this
  was not a typo" logic is to illustrate that in addition to the
  examples that we know about it is very likely that there are
  examples that we don't know about, in teams' script collections
  beyond the reach of search engines.

- In fact, in addition to the motivating example that makes it
  possible to build Rust, we had multiple in-tree scripts that would
  also have broken by this, if they had not been adapted to work
  around that in the same series!  I should have noticed that in
  review, and I'm sorry that I didn't.

> Perhaps we need to point script authors to "plumbing" commands more clearly?

I think the existence of "plumbing" is fairly well known, but users
don't always have an easy time using it.  The "porcelain" is what ends
up getting the most attention in improvements, and so while I
encourage script authors to use 'git rev-list <revs> | git diff-tree
-s --stdin --format=<fmt>' in place of 'git log --format=<fmt>
<revs>', most do not listen, and I can't really blame them given how
much more convenient the latter is and how many more options it
supports.

I don't think that situation will change unless we

 a. Maintain a second, parallel implementation of each porcelain
    command that only uses plumbing.  This would provide an example of
    how to use plumbing and would ensure that the plumbing grows in
    capability at the same time as the corresponding porcelain.  Or

 b. Expose a library interface, so that we can expose the actual
    helpers that support the standard implementation of porcelain
    commands.

I tried a little of (a) years ago by updating contrib/examples/ to
pass tests: https://lore.kernel.org/git/20100817065147.GA18293@burratino/.
It was fun but I don't think it's really sustainable.

In the long term, I think (b) is going to be an important thing to do,
and I think it will be helpful.  Some automated callers would
appreciate the ability to pass structured input instead of having to
pretend to be shell scripts. :)  True shell scripts would also benefit
because the plumbing commands can more directly map to such a library
API.

> (BTW, I have no opinion on whether -m should or should not imply -p.)

Nevertheless, thanks for weighing in.

Jonathan

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> More generally, we've been able to make changes to porcelain commands
> that don't hurt our ability to act as a platform for scripts, and I
> want us to continue to be able to do that.  "Do not break any script"
> is certainly not the standard I think we should apply, as illustrated
> by my thoughts upthread when I thought '-m' in this Rust example was a
> typo.
>
> But by now it's very clear to me that it was not a typo.

This is a tangent that does not change the conclusion, because the
use of "-m" in "stash list" was not a typo but a deliberate attempt
to allow "-p" from the end-users to do what they wanted to do, and
it was clearly broken by this change (as you said, the need to hide
the breakage in the same series should have ringed a loud bell for
us).

But I didn't see how you think your Rust thing is not a typo, and I
still don't.  Unless you think Rust folks expected "-m" to do what
"-m" was not designed to do, that is, and I do not think that
"people thought it did something entirely differently, when it was a
no-op, so we shouldn't suddenly make it not a no-op" is a good
rationale that affects how we choose the evolution path for our
tools.

THanks.

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> But I didn't see how you think your Rust thing is not a typo, and I
> still don't.  Unless you think Rust folks expected "-m" to do what
> "-m" was not designed to do, that is, and I do not think that
> "people thought it did something entirely differently, when it was a
> no-op, so we shouldn't suddenly make it not a no-op" is a good
> rationale that affects how we choose the evolution path for our
> tools.

Please don't treat this as an attempt to be argumentative: as you've
said, there's plenty of other reason for us to know in retrospect that
making "-m" imply "-p" is problematic for scripts.  Since you asked, I
think it's still worth describing my logic about the Rust example.

I believe the Rust folks expected "-m" to do something that it is
designed to do.  They _also_ overlooked a different subtlety about the
interaction between diff generation and path limiting.  It's good that
Rust's bootstrap.py is fixed now to be more straightforward (by now
it's even using the plumbing command); but it is very easy for another
script author to have had the same confusion, which I might add was a
harmless confusion until this change.  If we changed the behavior to
match their expectation _better_ then it would be a perfectly fine
compatibility break that would be expected to improve the behavior of
more scripts than it hurts.  This change was not in that category.

1. When I add "-m --first-parent" to my "git log -p" invocation, it
   changes what diff it generates.  Until 9ab89a24390 (log: enable
   "-m" automatically with "--first-parent", 2020-07-29), the diff
   shown for a merge with --first-parent was simply "no diff".  A
   script written before mid-2020 that wants to operate on the
   --first-parent diff is highly likely to pass -m.

2. The -m only affects diff generation and does not affect path
   limiting.  So when no diff is being generated it is in fact a
   no-op.  This point is fairly subtle, though, and because it is not
   documented, script authors _in practice_ would only discover it by
   experimentation.

3. A script using -m with the intent of affecting path limiting
   doesn't get any feedback via experimentation that they've made a
   mistake because path limiting with --first-parent already does what
   the script author was hoping for.

What's relevant is not whether the script author was in the wrong or
in the right: it's whether we expect there to be a significant number
of scripts negatively affected by the change.  Because of (1) to (3)
above, I do.

Jonathan

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Please don't treat this as an attempt to be argumentative:...

I won't.  I asked you what I didn't understand in what you said.
Answering the question is not being argumentative ;-)

> What's relevant is not whether the script author was in the wrong or
> in the right.

I do not agree with this reasoning at all.  Only if vast majority of
users incorrectly used the command and the option, we may need to
consider such a move as an exception, but not as a general rule.

But "stash list" example shows that "log --first-parent -m" without
"-p" in a script has a valid reason, and a change that hurts those
who correctly used a command and an option in a way they were
intended to do _is_ problematic.

Thanks.

Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'

[Sergey Organov]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Please don't treat this as an attempt to be argumentative:...
>
> I won't.  I asked you what I didn't understand in what you said.
> Answering the question is not being argumentative ;-)
>
>> What's relevant is not whether the script author was in the wrong or
>> in the right.
>
> I do not agree with this reasoning at all.  Only if vast majority of
> users incorrectly used the command and the option, we may need to
> consider such a move as an exception, but not as a general rule.
>
> But "stash list" example shows that "log --first-parent -m" without
> "-p" in a script has a valid reason, and a change that hurts those
> who correctly used a command and an option in a way they were
> intended to do _is_ problematic.

The patch never meant to be entirely backward compatible in the first
place, and, as far as I can see, "log --first-parent -m" doesn't make
sense anymore, since --fist-parent implies -m, that has been settled
already.

Thanks,
Sergey Organov

Re: [PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Sergey Organov]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi Sergey,
>
> Sergey Organov wrote:
>
>> Fix long standing inconsistency between -c/--cc that do imply -p on
>> one side, and -m that did not imply -p on the other side.
>
> As I mentioned before, I quite like this change for interactive use.
>
> But:
>
> [...]
>> It's also worth to be noticed that exact historical semantics of -m is
>> still provided by --diff-merges=separate.
>
> Is that true?  When I try it locally, -m shows no diff by default,
> whereas --diff-merges=separate shows a diff for merges.

You are right, it's not true that --diff-merges=separate behaves exactly
like -m did before this commit.

Actually, I think this notice was meant to be referring to the

"It gets even more useful if one sets "log.diffMerges" configuration
 variable to "first-parent" to force -m produce usual diff with respect
 to first parent only."

part of the message when I wrote it, but it ends up being confusing and
thus wrong, sorry.

Thanks,
-- Sergey Organov

Re: [PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Joshua Nelson]

Jonathan Nieder wrote:

> What happens if someone wants to build an older version of Rust?

For what it's worth, almost no one builds old versions of rust from source
except for distros, and distros wouldn't ever set download-ci-llvm = true. So
this shouldn't affect anyone in practice now that we've removed `-m` on master.

Joshua