git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Sergey Organov <sorganov@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Alex Henrie <alexhenrie24@gmail.com>,
	Git mailing list <git@vger.kernel.org>
Subject: Re: Why doesn't `git log -m` imply `-p`?
Date: Wed, 05 May 2021 16:43:53 +0300	[thread overview]
Message-ID: <87tunh9tye.fsf@osv.gnss.ru> (raw)
In-Reply-To: <xmqqy2cu58vo.fsf@gitster.g> (Junio C. Hamano's message of "Wed, 05 May 2021 09:20:27 +0900")

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

  reply	other threads:[~2021-05-05 13:44 UTC|newest]

Thread overview: 130+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-29  1:44 Alex Henrie
2021-04-29  3:22 ` Junio C Hamano
2021-04-29 12:38   ` Sergey Organov
2021-04-29 15:25     ` Alex Henrie
2021-04-29 16:35       ` Sergey Organov
2021-04-29 17:24         ` Alex Henrie
2021-04-29 19:07           ` Sergey Organov
2021-05-04 20:09       ` Felipe Contreras
2021-05-04 20:34         ` Sergey Organov
2021-04-29 23:27     ` Junio C Hamano
2021-04-30  4:50       ` Junio C Hamano
2021-04-30 14:00         ` Sergey Organov
2021-05-01  0:41           ` Junio C Hamano
2021-05-03 17:42             ` Sergey Organov
2021-05-04  1:15               ` Junio C Hamano
2021-05-04  9:10                 ` Sergey Organov
2021-05-04 12:38                   ` Junio C Hamano
2021-05-04 14:18                     ` Sergey Organov
2021-05-05  0:20                       ` Junio C Hamano
2021-05-05 13:43                         ` Sergey Organov [this message]
2021-05-06  0:27                           ` Junio C Hamano
2021-05-06 12:59                             ` Sergey Organov
2021-05-06 20:29                               ` Junio C Hamano
2021-05-06 20:48                                 ` Sergey Organov
2021-05-07  1:31                                   ` Alex Henrie
2021-05-10 12:11 ` Sergey Organov
2021-05-10 16:56   ` Alex Henrie
2021-05-10 15:34 ` [PATCH 0/6] diff-merges: let -m imply -p Sergey Organov
2021-05-10 15:34   ` [PATCH 1/6] t4013: add test for "git diff-index -m" Sergey Organov
2021-05-10 15:34   ` [PATCH 2/6] diff-merges: move specific diff-index "-m" handling to diff-index Sergey Organov
2021-05-11  4:09     ` Junio C Hamano
2021-05-11  5:23       ` Junio C Hamano
2021-05-11  5:41         ` Junio C Hamano
2021-05-11 13:43       ` Sergey Organov
2021-05-10 15:34   ` [PATCH 3/6] git-svn: stop passing "-m" to "git rev-list" Sergey Organov
2021-05-10 15:34   ` [PATCH 4/6] stash list: stop passing "-m" to "git list" Sergey Organov
2021-05-10 15:34   ` [PATCH 5/6] diff-merges: rename "combined_imply_patch" to "merges_imply_patch" Sergey Organov
2021-05-10 15:34   ` [PATCH 6/6] diff-merges: let -m imply -p Sergey Organov
2021-05-11  4:14     ` Junio C Hamano
2021-05-11  4:56       ` Junio C Hamano
2021-05-11 14:03         ` Sergey Organov
2021-05-11 17:13           ` Alex Henrie
2021-05-11 18:46             ` Sergey Organov
2021-05-11 19:53               ` Alex Henrie
2021-05-11 20:27                 ` Sergey Organov
2021-05-12  1:16                   ` Felipe Contreras
2021-05-11 18:31           ` Elijah Newren
2021-05-11 19:00             ` Sergey Organov
2021-05-11 19:56               ` Elijah Newren
2021-05-11 20:32                 ` Sergey Organov
2021-05-11 20:43             ` Junio C Hamano
2021-05-11 21:38               ` Sergey Organov
2021-05-11 23:40                 ` Junio C Hamano
2021-05-19 21:44             ` Jonathan Nieder
2021-05-20 20:39               ` Sergey Organov
2021-05-21 18:14                 ` Felipe Contreras
2021-05-11 16:29         ` Sergey Organov
2021-05-17 12:57         ` Sergey Organov
2021-05-11 16:30       ` Sergey Organov
2021-05-19 21:48     ` Jonathan Nieder
2021-05-19 22:03       ` Sergey Organov
2021-05-19 23:32       ` Junio C Hamano
2021-05-20 13:14         ` Sergey Organov
2021-05-20 18:50           ` Jonathan Nieder
2021-05-20 19:38             ` Sergey Organov
2021-05-17 15:58 ` [PATCH v1 0/9] " Sergey Organov
2021-05-17 15:58   ` [PATCH v1 1/9] t4013: test that "-m" alone has no effect in "git log" Sergey Organov
2021-05-17 15:58   ` [PATCH v1 2/9] t4013: test "git -m --raw" Sergey Organov
2021-05-18  3:27     ` Bagas Sanjaya
2021-05-18 12:13       ` Sergey Organov
2021-05-17 15:58   ` [PATCH v1 3/9] t4013: test "git -m --stat" Sergey Organov
2021-05-17 15:58   ` [PATCH v1 4/9] t4013: test "git diff-index -m" Sergey Organov
2021-05-17 15:58   ` [PATCH v1 5/9] diff-merges: move specific diff-index "-m" handling to diff-index Sergey Organov
2021-05-17 20:10     ` Junio C Hamano
2021-05-17 20:24       ` Sergey Organov
2021-05-17 20:29     ` Junio C Hamano
2021-05-17 21:00       ` Sergey Organov
2021-05-17 15:58   ` [PATCH v1 6/9] git-svn: stop passing "-m" to "git rev-list" Sergey Organov
2021-05-17 15:58   ` [PATCH v1 7/9] stash list: stop passing "-m" to "git list" Sergey Organov
2021-05-17 15:58   ` [PATCH v1 8/9] diff-merges: rename "combined_imply_patch" to "merges_imply_patch" Sergey Organov
2021-05-17 15:58   ` [PATCH v1 9/9] diff-merges: let "-m" imply "-p" Sergey Organov
2021-05-17 19:51   ` [PATCH v1 0/9] diff-merges: let -m imply -p Junio C Hamano
2021-05-17 20:11     ` Sergey Organov
2021-05-18  3:18   ` Bagas Sanjaya
2021-05-18 12:03     ` Sergey Organov
2021-05-18 12:17     ` Sergey Organov
2021-05-18 14:17   ` Junio C Hamano
2021-05-18 15:52     ` Sergey Organov
2021-05-19 11:45 ` [PATCH v2 " Sergey Organov
2021-05-19 11:45   ` [PATCH v2 1/9] t4013: test that "-m" alone has no effect in "git log" Sergey Organov
2021-05-19 11:45   ` [PATCH v2 2/9] t4013: test "git log -m --raw" Sergey Organov
2021-05-19 11:45   ` [PATCH v2 3/9] t4013: test "git log -m --stat" Sergey Organov
2021-05-19 11:45   ` [PATCH v2 4/9] t4013: test "git diff-index -m" Sergey Organov
2021-05-19 11:45   ` [PATCH v2 5/9] diff-merges: move specific diff-index "-m" handling to diff-index Sergey Organov
2021-05-19 11:45   ` [PATCH v2 6/9] git-svn: stop passing "-m" to "git rev-list" Sergey Organov
2021-05-19 11:45   ` [PATCH v2 7/9] stash list: stop passing "-m" to "git log" Sergey Organov
2021-05-19 11:45   ` [PATCH v2 8/9] diff-merges: rename "combined_imply_patch" to "merges_imply_patch" Sergey Organov
2021-05-19 11:45   ` [PATCH v2 9/9] diff-merges: let "-m" imply "-p" Sergey Organov
2021-05-20 21:46 ` [PATCH v3 00/10] diff-merges: let -m imply -p Sergey Organov
2021-05-20 21:46   ` [PATCH v3 01/10] t4013: test that "-m" alone has no effect in "git log" Sergey Organov
2021-05-20 21:46   ` [PATCH v3 02/10] t4013: test "git log -m --raw" Sergey Organov
2021-05-20 21:46   ` [PATCH v3 03/10] t4013: test "git log -m --stat" Sergey Organov
2021-05-20 21:46   ` [PATCH v3 04/10] t4013: test "git diff-tree -m" Sergey Organov
2021-05-20 21:46   ` [PATCH v3 05/10] t4013: test "git diff-index -m" Sergey Organov
2021-05-20 21:46   ` [PATCH v3 06/10] diff-merges: move specific diff-index "-m" handling to diff-index Sergey Organov
2021-05-20 21:47   ` [PATCH v3 07/10] git-svn: stop passing "-m" to "git rev-list" Sergey Organov
2021-05-20 21:47   ` [PATCH v3 08/10] stash list: stop passing "-m" to "git log" Sergey Organov
2021-05-20 21:47   ` [PATCH v3 09/10] diff-merges: rename "combined_imply_patch" to "merges_imply_patch" Sergey Organov
2021-05-20 21:47   ` [PATCH v3 10/10] diff-merges: let "-m" imply "-p" Sergey Organov
2021-08-05  3:16     ` Jonathan Nieder
2021-08-06  1:45       ` [PATCH] Revert 'diff-merges: let "-m" imply "-p"' Jonathan Nieder
2021-08-06 17:21         ` Junio C Hamano
2021-08-06 17:55           ` Junio C Hamano
2021-08-06 19:57             ` Jonathan Nieder
2021-08-08 17:55               ` Junio C Hamano
2021-08-17  9:13                 ` Sergey Organov
2021-08-17 22:10                   ` Junio C Hamano
2021-08-18  8:56                     ` Sergey Organov
2021-08-19 18:50                       ` Junio C Hamano
2021-08-19 18:51                         ` Junio C Hamano
2021-08-20 10:24                         ` Sergey Organov
2021-08-07  1:55           ` Jonathan Nieder
2021-08-07  6:49             ` Johannes Sixt
2021-08-07 13:51               ` Jonathan Nieder
2021-08-07 17:00                 ` Junio C Hamano
2021-08-07 18:08                   ` Jonathan Nieder
2021-08-08  0:42                     ` Junio C Hamano
2021-08-17  9:17                       ` Sergey Organov
2021-08-16  9:09       ` [PATCH v3 10/10] diff-merges: let "-m" imply "-p" Sergey Organov
     [not found] <CANvKV6anT0MV7GDeY_Cd2r+BwvMjsTdmjK+s_DKth7ZqL0XRUQ@mail.gmail.com>
2021-05-19 17:03 ` Why doesn't `git log -m` imply `-p`? Sergey Organov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87tunh9tye.fsf@osv.gnss.ru \
    --to=sorganov@gmail.com \
    --cc=alexhenrie24@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --subject='Re: Why doesn'\''t `git log -m` imply `-p`?' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).