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

From: Sergey Organov <sorganov@gmail.com>
Date: May 5, 2021, 13:43 UTC
Message-ID: <87tunh9tye.fsf@osv.gnss.ru>
In-Reply-To: <xmqqy2cu58vo.fsf@gitster.g>

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

Show 10 quoted lines

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

Show 14 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 13 quoted lines

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