git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Sergey Organov <sorganov@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified
Date: Wed, 27 Mar 2019 16:54:05 +0300	[thread overview]
Message-ID: <878sx0xw1u.fsf@javad.com> (raw)
In-Reply-To: <20190326163204.GC29627@sigill.intra.peff.net> (Jeff King's message of "Tue, 26 Mar 2019 12:32:04 -0400")

Jeff King <peff@peff.net> writes:

> On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:
>
>> How about changing "git show -p M" to output "diff -p M^ M" rather than
>> "diff-tree --cc M" for merge commits? It's really surprising specifying
>> -p has no visible effect.
>
> That's because "-p" is already the default, and the format selection is
> orthogonal to the handling of merge commits.

Seems to be more convoluted than that. If "-p" were simply default, then
"git show --raw" and "git show -p --raw" would output the same thing.
They don't.

That said, does "-p" select a format, or not? Should it? For me,
"--patch" sounds specific enough to expect it to select the format that
is most close to what "patch" utility is able to apply, that would still
be "diff -p M^ M" format, universally, be it merge commit or not.

> Providing "-m" would actually override the "--cc" default (though
> "--first-parent -m" is likely to be less noisy, per this discussion).

Right, but "--first-parent" (while accepted) even is not documented for
"git show", and it could be argued if history traversal option has any
sense for a command that shows separate commits.

Further, even in "git log", having the "--first-parent" change the way
diffs are output is, strictly speaking, violation of orthogonality (that
nobody seems to care about).

> As far as defaults go, I dunno.

I'm not after changing the default behavior. I'm rather after making the
whole system of options somewhat more logical, self consistent, and thus
less confusing.

> The idea is that "--cc" would give you a nice summary of what the
> merge _itself_ had to touch. I think that's valuable, too.

I'm not against "--cc" either, be it a default or not, even though
personally for me it's not very useful, as it shows how merge (the
operation) supposedly went, when I'm usually interested in how merge
(the resulting commit) affects the mainline, no matter how this result
has been achieved.

Another thought about "--cc" is that it's in fact a case of "merges are
symmetric" approach to the UI that is supposedly shifting to "mainline
is special" one.

> If we were starting from scratch, I think there could be
> a discussion about whether one default is better than the other. But at
> this point I have a hard time finding one so much obviously better than
> the other to merit changing the behavior.

I'm yet to figure what exactly the best set of options would be, even
personally for me, even in the "start from scratch" case, so, for now,
I'm basically just gathering relevant information and opinions.

>> Also, is current output of "git log -m", being extremely confusing,
>> suitable for anything? Maybe consider to change it to output diff with
>> respect to the first parent only? Though it's then a pity "-m" lacks
>> argument here, similar to what it has in cherry-pick.
>
> I've used "-m" without "--first-parent" sometimes in order to track down
> mis-merges manually.

Have you been interested specifically in diffs with respect to side
branches in these cases, I wonder, or did you actually look only at "-m
1" part of the whole "-m" output?

When I tried "-m", I found it rather difficult to even visually find the
margin between diffs to different parents, that confused me and forced
to resort to other methods. Besides, it didn't appear to me at that time
that there is "--first-parent" that might "help", so as I recall I ended
up using "diff -p M~1 M" for the merge in question.

> It's not usually a big deal to say "--first-parent" if that's what you
> want. But one thing I don't think is currently possible is to ask only
> for the first-parent diff, but _not_ restrict the actual traversal.

That's due to "--first-parent" breaking orthogonality. It should rather
only affect graph traversal, I'd expect.

Admittedly, it may imply some other option(s) for convenience (say, such
option could have been "-m 1", if it existed), but then there /must/
exist the option(s) it implies in the first place. Currently,
"--first-parent" behaves as if it implies some nonexistent option, so
the user has no way indeed to provide the latter without the former.

> If that's what you mean by giving an argument to "-m", then yeah, I
> think that would be a useful addition.

I thought that maybe the part of "-m" that outputs diffs to side
branch(es) is in fact useless feature when result is to be directly
consumed by human beings. Then, if we decide to change it to output diff
to single parent for porcelain command(s), it may be useful to be able
to explicitly ask for other parents than the default, the first one.

It also strikes me as inconsistent that "-m" in log/show on one hand,
and "-m" in cherry-pick on the other, being essentially the same thing,
are so different in appearance and description.

Unfortunately, adding an argument to "-m" bumps either into generic
evilness of optional arguments for options, or into backward
incompatibility (if the argument to "-m" becomes mandatory), so it
doesn't seem to be such a good thing to do after all.

-- Sergey

  parent reply	other threads:[~2019-03-27 13:54 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-25 12:42 Sergey Organov
2018-06-21 15:54 ` Junio C Hamano
2018-06-22  9:16   ` Sergey Organov
2018-12-12  5:35   ` Sergey Organov
2018-12-13  4:20     ` Junio C Hamano
2018-12-13  6:35       ` Sergey Organov
2018-12-13 15:35         ` Sergey Organov
2018-12-14  2:36           ` Junio C Hamano
2018-12-14  4:39             ` [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits Sergey Organov
2018-12-14  4:53               ` [PATCH v2 4/4] t3506: validate '-m 1 -ff' is now accepted " Sergey Organov
2018-12-14  4:53               ` [PATCH v2 3/4] t3502: validate '-m 1' argument " Sergey Organov
2019-01-03 17:22                 ` SZEDER Gábor
2019-01-06 14:41                   ` Sergey Organov
2018-12-14  4:53               ` [PATCH v2 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks Sergey Organov
2018-12-14  4:53               ` [PATCH v2 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified Sergey Organov
2018-12-25 12:39               ` [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits Sergey Organov, Sergey Organov
2018-12-26 22:52                 ` Junio C Hamano
2018-12-29  9:10                   ` Sergey Organov
2018-12-14  4:39             ` [PATCH v3 " Sergey Organov
2018-12-14  4:53               ` [PATCH v3 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified Sergey Organov
2018-12-14  4:53               ` [PATCH v3 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits Sergey Organov
2018-12-14  4:53               ` [PATCH v3 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks Sergey Organov
2018-12-14  4:53               ` [PATCH v3 4/4] t3506: validate '-m 1 -ff' is now accepted for non-merge commits Sergey Organov
2019-03-19 11:29   ` [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified Sergey Organov
2019-03-20  0:38     ` Junio C Hamano
2019-03-20  5:09       ` Jeff King
2019-03-25  6:43       ` Sergey Organov
2019-03-26 16:32         ` Jeff King
2019-03-26 22:07           ` Elijah Newren
2019-03-26 22:20             ` Jeff King
2019-03-27  0:33               ` Elijah Newren
2019-03-27 13:54           ` Sergey Organov [this message]
  -- strict thread matches above, loose matches on Subject: below --
2018-05-25 12:42 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=878sx0xw1u.fsf@javad.com \
    --to=sorganov@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH] cherry-pick: do not error on non-merge commits when '\''-m 1'\'' is specified' \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

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

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git