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: "Jonathan Nieder" <jrnieder@gmail.com>,
	"Jeff King" <peff@peff.net>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Elijah Newren" <newren@gmail.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"huang guanlong" <gl041188@gmail.com>,
	git@vger.kernel.org, "Hudson Ayers" <hudsonayers@google.com>,
	"Taylor Yu" <tlyu@mit.edu>, "Joshua Nelson" <jyn514@gmail.com>
Subject: Re: [PATCH] Revert 'diff-merges: let "-m" imply "-p"'
Date: Fri, 20 Aug 2021 13:24:42 +0300	[thread overview]
Message-ID: <87y28wct1x.fsf@osv.gnss.ru> (raw)
In-Reply-To: <xmqqim011d6m.fsf@gitster.g> (Junio C. Hamano's message of "Thu, 19 Aug 2021 11:50:41 -0700")

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

  parent reply	other threads:[~2021-08-20 10:24 UTC|newest]

Thread overview: 129+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-29  1:44 Why doesn't `git log -m` imply `-p`? 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
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 [this message]
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

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=87y28wct1x.fsf@osv.gnss.ru \
    --to=sorganov@gmail.com \
    --cc=alexhenrie24@gmail.com \
    --cc=avarab@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=gl041188@gmail.com \
    --cc=hudsonayers@google.com \
    --cc=jrnieder@gmail.com \
    --cc=jyn514@gmail.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=philipoakley@iee.email \
    --cc=tlyu@mit.edu \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).