[RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[C.J. Jameson]

From fbb0e1fef08a2abd7f1620134925788f7a0e011f Mon Sep 17 00:00:00 2001
From: "C.J. Jameson" <cjcjameson@gmail.com>
Date: Tue, 19 Mar 2019 17:57:31 -0700
Subject: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

When cherry-picking from a standard merge commit, the parent should
always be 1, as it has the meaningful new changes which were added to
the mainline. If the source commit is unique in some way and a user
wants to specify a value other than 1, they still can, but it's up to
them to be aware of which parent is which.

This builds on commit 37897bfc27 ("cherry-pick: do not error on
non-merge commits when '-m 1' is specified", 2018-12-14) which tolerated
`--mainline` for non-merge commits

Signed-off-by: C.J. Jameson <cjcjameson@gmail.com>
---
 Documentation/git-cherry-pick.txt |  6 +++---
 Documentation/git-revert.txt      |  6 +++---
 builtin/revert.c                  |  4 ++--
 t/t3502-cherry-pick-merge.sh      | 19 ++++++++++++++++++-
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt
b/Documentation/git-cherry-pick.txt
index b8cfeec67e..ec526d4cb8 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -76,13 +76,13 @@ OPTIONS
  described above, and `-r` was to disable it.  Now the
  default is not to do `-x` so this option is a no-op.

--m parent-number::
---mainline parent-number::
+-m [parent-number]::
+--mainline [parent-number]::
  Usually you cannot cherry-pick a merge because you do not know which
  side of the merge should be considered the mainline.  This
  option specifies the parent number (starting from 1) of
  the mainline and allows cherry-pick to replay the change
- relative to the specified parent.
+ relative to the specified parent. The default parent-number is 1.

 -n::
 --no-commit::
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 837707a8fd..57192a7c34 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -46,13 +46,13 @@ OPTIONS
  message prior to committing the revert. This is the default if
  you run the command from a terminal.

--m parent-number::
---mainline parent-number::
+-m [parent-number]::
+--mainline [parent-number]::
  Usually you cannot revert a merge because you do not know which
  side of the merge should be considered the mainline.  This
  option specifies the parent number (starting from 1) of
  the mainline and allows revert to reverse the change
- relative to the specified parent.
+ relative to the specified parent. The default parent-number is 1.
 +
 Reverting a merge commit declares that you will never want the tree changes
 brought in by the merge.  As a result, later merges will only bring in tree
diff --git a/builtin/revert.c b/builtin/revert.c
index a47b53ceaf..280d1f00a9 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -105,8 +105,8 @@ static int run_sequencer(int argc, const char
**argv, struct replay_opts *opts)
  OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
  OPT_NOOP_NOARG('r', NULL),
  OPT_BOOL('s', "signoff", &opts->signoff, N_("add Signed-off-by:")),
- OPT_CALLBACK('m', "mainline", opts, N_("parent-number"),
-      N_("select mainline parent"), option_parse_m),
+ { OPTION_INTEGER, 'm', "mainline", &opts->mainline, N_("parent-number"),
+ N_("select mainline parent"), PARSE_OPT_OPTARG, NULL, 1 },
  OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
  OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge
strategy")),
  OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 8b635a196d..4f248be582 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -34,7 +34,6 @@ test_expect_success setup '
 test_expect_success 'cherry-pick -m complains of bogus numbers' '
  # expect 129 here to distinguish between cases where
  # there was nothing to cherry-pick
- test_expect_code 129 git cherry-pick -m &&
  test_expect_code 129 git cherry-pick -m foo b &&
  test_expect_code 129 git cherry-pick -m -1 b &&
  test_expect_code 129 git cherry-pick -m 0 b
@@ -67,6 +66,15 @@ test_expect_success 'cherry pick a merge (1)' '

 '

+test_expect_success 'cherry pick a merge with default parent (1)' '
+
+ git reset --hard &&
+ git checkout a^0 &&
+ git cherry-pick -m c &&
+ git diff --exit-code c
+
+'
+
 test_expect_success 'cherry pick a merge (2)' '

  git reset --hard &&
@@ -111,6 +119,15 @@ test_expect_success 'revert a merge (1)' '

 '

+test_expect_success 'revert a merge with default parent (1)' '
+
+ git reset --hard &&
+ git checkout c^0 &&
+ git revert -m 1 c &&
+ git diff --exit-code a --
+
+'
+
 test_expect_success 'revert a merge (2)' '

  git reset --hard &&
-- 
2.21.0

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Junio C Hamano]

"C.J. Jameson" <cjcjameson@gmail.com> writes:

> When cherry-picking from a standard merge commit, the parent should
> always be 1, as it has the meaningful new changes which were added to
> the mainline. If the source commit is unique in some way and a user
> wants to specify a value other than 1, they still can, but it's up to
> them to be aware of which parent is which.

I do not have a firm opinion for or against the "if the commit being
cherry-picked happens to be a merge, cherry-pick it relative to the
first parent by default, without requiring the '-m 1' option".  It
may not be such a bad idea after all.

But I do have a very strong opinion against adding yet another
option that takes an optional argument.  If we want to allow
cherry-picking a merge commit just as easy as cherrry-picking a
single-parent commit, "git cherry-pick -m merge" (assuming 'merge'
is the tip of a branch that is a merge commit) that still requires
the user to say "-m" is not a good improvement.  We should just
accept "git cherry-pick merge" without any "-m" if we want to move
in this direction, I would think.

> --m parent-number::
> ---mainline parent-number::
> +-m [parent-number]::
> +--mainline [parent-number]::
>   Usually you cannot cherry-pick a merge because you do not know which
>   side of the merge should be considered the mainline.  This
>   option specifies the parent number (starting from 1) of
>   the mainline and allows cherry-pick to replay the change
> - relative to the specified parent.
> + relative to the specified parent. The default parent-number is 1.

I somehow sense a total whitespace breakage.  Usually these lines
are HT indented, but I see only a single whitespace indent around
here.

So, the first part of the paragraph needs to get revamped.  It won't
be "because", but "unless", and with your change, you no longer know
which side is mainline---unless you tell Git otherwise, the first
parent is assumed to be the mainline in the new world order.

	-m <parent-number>::
	--mainline <parent-number>::
                When cherry-picking a merge, by default, the first
                parent is taken as the mainline, and the command
                replays the change relative to the first parent of
                the given commit.  If the merge commit you are
                cherry-picking records the mainline as second or
                later parent, you can use this option to replay
                the change relative to the specified parent (parents
                count from 1, which is the first parent).
	+
	The default parent-number is 1.  Giving a parent number
	larger than the number of parent the cherry-picked commit has
	is an error.

or something like that.

> diff --git a/builtin/revert.c b/builtin/revert.c
> index a47b53ceaf..280d1f00a9 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -105,8 +105,8 @@ static int run_sequencer(int argc, const char
> **argv, struct replay_opts *opts)

Line-wrapped patch cannot be applied.

>   OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
>   OPT_NOOP_NOARG('r', NULL),
>   OPT_BOOL('s', "signoff", &opts->signoff, N_("add Signed-off-by:")),
> - OPT_CALLBACK('m', "mainline", opts, N_("parent-number"),
> -      N_("select mainline parent"), option_parse_m),
> + { OPTION_INTEGER, 'm', "mainline", &opts->mainline, N_("parent-number"),
> + N_("select mainline parent"), PARSE_OPT_OPTARG, NULL, 1 },

Let's not do optarg.  Use of OPT_INTEGER is fine, if you really want
to, but then you are losing the last caller of option_parse_m() and
the callback function itself must also be removed. 

You'd also need to do a few more things:

#1 make it NONEG, so that we reject "--no-mainline"; in the new
   world order, cherry-picking a non-merge commit is replaying
   against the first parent; there is no situation where use of
   --no-mainline makes sense (perhaps other than cherry-picking the
   initial commit, for which there is no mainline parent).

#2 initialize opts->mainline to 1, so that with no --mainline option
   from the command line, we will still get opt->mainline == 1
   (alternatively, you could initialize opt->mainline == -1 and
   treat opt->mainline <= 0 as if it were set to 1 when the code
   must choose against which parent to replay the changes in a much
   deeper place in the codepath).

#3 think about how you'd loosen option compatibility check around
   --mainline (if you do #2, as opt->mainline would never be 0 now)
   and adjust the verify_opt_compatible() call(s).

#4 as OPT_INTEGER won't ensure that you get a positive integer,
   you'd need to somehow reject --mainline=0 (or --mainline=-1 for
   that matter) given from the command line.

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Duy Nguyen]

On Wed, Mar 20, 2019 at 10:57 AM C.J. Jameson <cjcjameson@gmail.com> wrote:
> --m parent-number::
> ---mainline parent-number::
> +-m [parent-number]::

Careful with this. Optional argument with a short option means the
"stuck" form which is known to cause confusion [1].

Try "git revert -mX HEAD" and "git revert -m X HEAD". The first one
reports invalid number (good). The second assumes "X" not to be an
argument of "-m" and leave it for the caller (revert in this case) to
handle, which could do unexpected things.

Personally I'm not excited to have more stuck forms like this.

[1] https://public-inbox.org/git/20190208024800.GA11392@sigill.intra.peff.net/


> +--mainline [parent-number]::
>   Usually you cannot cherry-pick a merge because you do not know which
>   side of the merge should be considered the mainline.  This
>   option specifies the parent number (starting from 1) of
>   the mainline and allows cherry-pick to replay the change
> - relative to the specified parent.
> + relative to the specified parent. The default parent-number is 1.
--
Duy

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Sergey Organov]

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

[...]

> But I do have a very strong opinion against adding yet another
> option that takes an optional argument.  If we want to allow
> cherry-picking a merge commit just as easy as cherrry-picking a
> single-parent commit, "git cherry-pick -m merge" (assuming 'merge'
> is the tip of a branch that is a merge commit) that still requires
> the user to say "-m" is not a good improvement.  We should just
> accept "git cherry-pick merge" without any "-m" if we want to move
> in this direction, I would think.

Let's just make '-m 1' the default option indeed. No need for further
complexities.

Exactly according to what Junio has already said before. Here:

https://public-inbox.org/git/xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com

Junio wrote:

> Now, it appears, at least to me, that the world pretty much accepted
> that the first-parent worldview is often very convenient and worth
> supporting by the tool, so the next logical step might be to set
> opts->mainline to 1 by default (and allow an explicit "-m $n" from
> the command line to override it).  But that should happen after this
> patch lands---it is logically a separate step, I would think.

... and as that patch already landed...

-- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Elijah Newren]

On Wed, Mar 20, 2019 at 8:09 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
> > But I do have a very strong opinion against adding yet another
> > option that takes an optional argument.  If we want to allow
> > cherry-picking a merge commit just as easy as cherrry-picking a
> > single-parent commit, "git cherry-pick -m merge" (assuming 'merge'
> > is the tip of a branch that is a merge commit) that still requires
> > the user to say "-m" is not a good improvement.  We should just
> > accept "git cherry-pick merge" without any "-m" if we want to move
> > in this direction, I would think.
>
> Let's just make '-m 1' the default option indeed. No need for further
> complexities.
>
> Exactly according to what Junio has already said before. Here:
>
> https://public-inbox.org/git/xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com
>
> Junio wrote:
>
> > Now, it appears, at least to me, that the world pretty much accepted
> > that the first-parent worldview is often very convenient and worth
> > supporting by the tool, so the next logical step might be to set
> > opts->mainline to 1 by default (and allow an explicit "-m $n" from
> > the command line to override it).  But that should happen after this
> > patch lands---it is logically a separate step, I would think.
>
> ... and as that patch already landed...

This worries me that it'll lead to bad surprises.  Perhaps some folks
cherry-pick merges around intentionally, but I would want that to be a
rare occurrence at most.  There are lots of folks at $DAYJOB that
cherry-pick things, not all of them are expert git-users, and I am
certain several have erroneously attempted to cherry-pick merges
before.  I would much rather they continued to get an error message
and then asked other folks for help so that someone can explain to
them what they should instead be doing rather than silently changing
the current error into an unwanted operation.  Granted, the users will
at least get a confusing "Merge branch <foo>" commit message for
something that isn't a merge, but I don't think the users will notice
that either.  It just means we've got both confusing and ugly history
without the necessary individual commits or with too much having been
cherry-picked.  If -m 1 is too much to ask people to specify, could we
provide some other shorthand?  Or at least make a default-off config
option people would have to set if they want a cherry-pick of a merge
to succeed without specifying -m?

Elijah

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Wed, Mar 20, 2019 at 8:09 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> [...]
>>
>> > But I do have a very strong opinion against adding yet another
>> > option that takes an optional argument.  If we want to allow
>> > cherry-picking a merge commit just as easy as cherrry-picking a
>> > single-parent commit, "git cherry-pick -m merge" (assuming 'merge'
>> > is the tip of a branch that is a merge commit) that still requires
>> > the user to say "-m" is not a good improvement.  We should just
>> > accept "git cherry-pick merge" without any "-m" if we want to move
>> > in this direction, I would think.
>>
>> Let's just make '-m 1' the default option indeed. No need for further
>> complexities.
>>
>> Exactly according to what Junio has already said before. Here:
>>
>> https://public-inbox.org/git/xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com
>>
>> Junio wrote:
>>
>> > Now, it appears, at least to me, that the world pretty much accepted
>> > that the first-parent worldview is often very convenient and worth
>> > supporting by the tool, so the next logical step might be to set
>> > opts->mainline to 1 by default (and allow an explicit "-m $n" from
>> > the command line to override it).  But that should happen after this
>> > patch lands---it is logically a separate step, I would think.
>>
>> ... and as that patch already landed...
>
> This worries me that it'll lead to bad surprises.  Perhaps some folks
> cherry-pick merges around intentionally, but I would want that to be a
> rare occurrence at most.  There are lots of folks at $DAYJOB that
> cherry-pick things, not all of them are expert git-users, and I am
> certain several have erroneously attempted to cherry-pick merges
> before.

Wow, random Joes cherry-picking here and there... Sounds like a bigger
problem is lurking here.

> I would much rather they continued to get an error message
> and then asked other folks for help so that someone can explain to
> them what they should instead be doing rather than silently changing
> the current error into an unwanted operation.  Granted, the users will
> at least get a confusing "Merge branch <foo>" commit message for
> something that isn't a merge, but I don't think the users will notice
> that either.  It just means we've got both confusing and ugly history
> without the necessary individual commits or with too much having been
> cherry-picked.

To me it seems that cherry-picking wrong commit is cherry-picking wrong
commit, no matter if it's a merge or not. I don't think that trying to
save a user from such a mistake worth the trouble, given that
cherry-pick is reversible operation, but I still see your point.

> If -m 1 is too much to ask people to specify, could we provide some
> other shorthand? Or at least make a default-off config option people
> would have to set if they want a cherry-pick of a merge to succeed
> without specifying -m?

If we decide we still need this safety precaution, I'd opt to continue
to require '-m 1' to cherry-pick a merge, rather than adding some
special support. Not such a big deal.

BTW, doesn't git have generic configuration support to add default
option to a command, I wonder (I'm aware of aliases, but they don't
seem to fit)? The C.J. then would simply add '-m 1' to 'cherry-pick' in
configuration. No luck?

-- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[C.J. Jameson]

On Wed, Mar 20, 2019 at 8:59 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Wed, Mar 20, 2019 at 8:09 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> [...]
> >>
> >> > But I do have a very strong opinion against adding yet another
> >> > option that takes an optional argument.  If we want to allow
> >> > cherry-picking a merge commit just as easy as cherrry-picking a
> >> > single-parent commit, "git cherry-pick -m merge" (assuming 'merge'
> >> > is the tip of a branch that is a merge commit) that still requires
> >> > the user to say "-m" is not a good improvement.  We should just
> >> > accept "git cherry-pick merge" without any "-m" if we want to move
> >> > in this direction, I would think.
> >>
> >> Let's just make '-m 1' the default option indeed. No need for further
> >> complexities.
> >>
> >> Exactly according to what Junio has already said before. Here:
> >>
> >> https://public-inbox.org/git/xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com
> >>
> >> Junio wrote:
> >>
> >> > Now, it appears, at least to me, that the world pretty much accepted
> >> > that the first-parent worldview is often very convenient and worth
> >> > supporting by the tool, so the next logical step might be to set
> >> > opts->mainline to 1 by default (and allow an explicit "-m $n" from
> >> > the command line to override it).  But that should happen after this
> >> > patch lands---it is logically a separate step, I would think.
> >>
> >> ... and as that patch already landed...
> >
> > This worries me that it'll lead to bad surprises.  Perhaps some folks
> > cherry-pick merges around intentionally, but I would want that to be a
> > rare occurrence at most.  There are lots of folks at $DAYJOB that
> > cherry-pick things, not all of them are expert git-users, and I am
> > certain several have erroneously attempted to cherry-pick merges
> > before.
>
> Wow, random Joes cherry-picking here and there... Sounds like a bigger
> problem is lurking here.

Indeed, I was the de facto release manager at $DAYJOB and I didn't know
about needing `--mainline` until we happened to need it. At this point,
it's just more fuel for me arguing that the merge commits are extraneous
noise in our repo's history, but projects have inertia-of-git-workflow.
>
> > I would much rather they continued to get an error message
> > and then asked other folks for help so that someone can explain to
> > them what they should instead be doing rather than silently changing
> > the current error into an unwanted operation.  Granted, the users will
> > at least get a confusing "Merge branch <foo>" commit message for
> > something that isn't a merge, but I don't think the users will notice
> > that either.  It just means we've got both confusing and ugly history
> > without the necessary individual commits or with too much having been
> > cherry-picked.
>
> To me it seems that cherry-picking wrong commit is cherry-picking wrong
> commit, no matter if it's a merge or not. I don't think that trying to
> save a user from such a mistake worth the trouble, given that
> cherry-pick is reversible operation, but I still see your point.
>
I'm with Sergey: that people would know what code is being brought in.

We could specifically code it up so that 3-way merges still need
an explicit `-m` to be set...

> > If -m 1 is too much to ask people to specify, could we provide some
> > other shorthand? Or at least make a default-off config option people
> > would have to set if they want a cherry-pick of a merge to succeed
> > without specifying -m?
>
> If we decide we still need this safety precaution, I'd opt to continue
> to require '-m 1' to cherry-pick a merge, rather than adding some
> special support. Not such a big deal.
>
> BTW, doesn't git have generic configuration support to add default
> option to a command, I wonder (I'm aware of aliases, but they don't
> seem to fit)? The C.J. then would simply add '-m 1' to 'cherry-pick' in
> configuration. No luck?
>
I'm not quite sure how I'd do that; pointers?

> -- Sergey
>

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> This worries me that it'll lead to bad surprises.  Perhaps some folks
> cherry-pick merges around intentionally, but I would want that to be a
> rare occurrence at most.  There are lots of folks at $DAYJOB that
> cherry-pick things, not all of them are expert git-users, and I am
> certain several have erroneously attempted to cherry-pick merges
> before.

It was a lot simpler back when "git cherry-pick" did not accept
ranges.  You are either knowingly cherry-picking a merge, or doing
so by mistake, and because the command rejected cherry-picking a
merge without being told with "-m $n" which parent the mainline is
by default, we are assured that the user knew that s/he was picking
a merge commit when we saw "-m $n".

It's not so simple in the world after we started allowing picks of a
range.  "cherry-pick -m1 A..B" did not work when the range A..B is a
mixture of merges and non-merges (which is the case 100% of the
time), as the command used to error out when given the -m option for
a single-parent commit.  Earlier we said that "as long as the $n
does not exceed the number of actual parents, let's allow '-m $n'
even for non-merge commits." to fix it.

We can just reject this RFC patch and we'd be in a slightly safer
place.  You still need to tell us with "-m 1" on the command line
that you are picking a range with merges in it.  But then I am sure
that clueless people blindly would alias "pick = cherry-pick -m1" and
use "git pick" and blindly pick ranges here and there, so I am not
sure such a slightly-more safety buys us very much.

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Junio C Hamano]

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

> Elijah Newren <newren@gmail.com> writes:
>
>> This worries me that it'll lead to bad surprises.  Perhaps some folks
>> cherry-pick merges around intentionally, but I would want that to be a
>> rare occurrence at most.
>
> We can just reject this RFC patch and we'd be in a slightly safer
> place.  You still need to tell us with "-m 1" on the command line
> that you are picking a range with merges in it.  But then I am sure
> that clueless people blindly would alias "pick = cherry-pick -m1" and
> use "git pick" and blindly pick ranges here and there, so I am not
> sure such a slightly-more safety buys us very much.

To put it a bit differently, I share with you that picking merges
should be deliberate and it is safer to make sure allowing it only
when the told us that s/he knows the commit being picked is a merge,
but when we started allowing "-m 1" for non-merge commits in the
current world where cherry-pick can work on a range, the ship has
already sailed.

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> This worries me that it'll lead to bad surprises.  Perhaps some folks
>>> cherry-pick merges around intentionally, but I would want that to be a
>>> rare occurrence at most.
>>
>> We can just reject this RFC patch and we'd be in a slightly safer
>> place.  You still need to tell us with "-m 1" on the command line
>> that you are picking a range with merges in it.  But then I am sure
>> that clueless people blindly would alias "pick = cherry-pick -m1" and
>> use "git pick" and blindly pick ranges here and there, so I am not
>> sure such a slightly-more safety buys us very much.
>
> To put it a bit differently, I share with you that picking merges
> should be deliberate and it is safer to make sure allowing it only
> when the told us that s/he knows the commit being picked is a merge,

Something like "--[no-]ban-merges" then [*], having "--ban-merges" as
default?

> but when we started allowing "-m 1" for non-merge commits in the
> current world where cherry-pick can work on a range, the ship has
> already sailed.

Except that it could be a different ship, provided we've got
"--ban-merges". Having "-m 1" as default stops to be an issue, and
explicit "-m 1" could then imply --no-ban-merges, that could be in turn
overwritten by explicit "--ban-merges", if necessary.

[*] "--[no]-merges" won't do, as one would expect merges to be silently
dropped from a range of cherry-picks, similar to what "git log" or "git
rev-list" does. BTW, is it a good idea for cherry-pick to start to use
"git rev-list" and support all the options available there?

-- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> To put it a bit differently, I share with you that picking merges
>> should be deliberate and it is safer to make sure allowing it only
>> when the told us that s/he knows the commit being picked is a merge,
>
> Something like "--[no-]ban-merges" then [*], having "--ban-merges" as
> default?
>
>> but when we started allowing "-m 1" for non-merge commits in the
>> current world where cherry-pick can work on a range, the ship has
>> already sailed.
>
> Except that it could be a different ship, provided we've got
> "--ban-merges". Having "-m 1" as default stops to be an issue, and
> explicit "-m 1" could then imply --no-ban-merges, that could be in turn
> overwritten by explicit "--ban-merges", if necessary.

The same effect can be had by just reverting "let's allow -m1 for
single-parent commit", can't it?  That is a far simpler solution, I
would say.

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> To put it a bit differently, I share with you that picking merges
>>> should be deliberate and it is safer to make sure allowing it only
>>> when the told us that s/he knows the commit being picked is a merge,
>>
>> Something like "--[no-]ban-merges" then [*], having "--ban-merges" as
>> default?
>>
>>> but when we started allowing "-m 1" for non-merge commits in the
>>> current world where cherry-pick can work on a range, the ship has
>>> already sailed.
>>
>> Except that it could be a different ship, provided we've got
>> "--ban-merges". Having "-m 1" as default stops to be an issue, and
>> explicit "-m 1" could then imply --no-ban-merges, that could be in turn
>> overwritten by explicit "--ban-merges", if necessary.
>
> The same effect can be had by just reverting "let's allow -m1 for
> single-parent commit", can't it?  That is a far simpler solution, I
> would say.

Those one didn't introduce the issue currently at hand, as we still
don't allow merges by default, so why do we need to rewind it?

-- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> To put it a bit differently, I share with you that picking merges
>>> should be deliberate and it is safer to make sure allowing it only
>>> when the told us that s/he knows the commit being picked is a merge,
>>
>> Something like "--[no-]ban-merges" then [*], having "--ban-merges" as
>> default?
>>
>>> but when we started allowing "-m 1" for non-merge commits in the
>>> current world where cherry-pick can work on a range, the ship has
>>> already sailed.
>>
>> Except that it could be a different ship, provided we've got
>> "--ban-merges". Having "-m 1" as default stops to be an issue, and
>> explicit "-m 1" could then imply --no-ban-merges, that could be in turn
>> overwritten by explicit "--ban-merges", if necessary.
>
> The same effect can be had by just reverting "let's allow -m1 for
> single-parent commit", can't it?  That is a far simpler solution, I
> would say.

To clarify further my short answer:

S>  Those one didn't introduce the issue currently at hand, as we still
S>  don't allow merges by default, so why do we need to rewind it?

The "let's allow -m1 for single-parent commit" even doesn't affect merge
commits at all, so I fail to see how reverting it may help to resolve
the issue that C.J. wants to solve with his proposed patch.

If we want to disallow picking merges by default, it's already so right
now, no reverts or patches required.

Then, as I see it, it's the current way of allowing merges, that
cryptically reads as "-m 1", that makes the OP unhappy. This problem was
already there before the aforementioned patch, so reverting the patch
won't solve it. OTOH, it's exactly this problem that "--[no]-ban-merges"
would solve.

Finally, "--[no-]ban-merges" would allow to easily make "-m 1" the
default later, as a separate change, should we decide it's the right
thing to do.

-- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> The same effect can be had by just reverting "let's allow -m1 for
>> single-parent commit", can't it?  That is a far simpler solution, I
>> would say.
>
> Those one didn't introduce the issue currently at hand, as we still
> don't allow merges by default, so why do we need to rewind it?

With it reverted, "[alias] cp = cherry-pick -m1" can be used to
train the user to blindly pick a range that has a merge without
thinking, which is what I meant by "ship has already sailed".

With it reverted, a range pick of a straight single strand of pearls
would still work just fine.  And the user is forced to think and
chop a range with a merge into a set of subranges each of which is a
single strand of pearls, plus picking individual merges (if picking
these merges is really what the user wants, that is).  As ensuring
the users to think is the whole point of excercise, the original
system before we allowed "-m1" for single parent commit was after
all giving us the right balance, I guess, without another new
options.

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> The same effect can be had by just reverting "let's allow -m1 for
>> single-parent commit", can't it?  That is a far simpler solution, I
>> would say.
>
> Those one didn't introduce the issue currently at hand, as we still
> don't allow merges by default, so why do we need to rewind it?

With it reverted, "[alias] cp = cherry-pick -m1" can be used to
train the user to blindly pick a range that has a merge without
thinking, which is what I meant by "ship has already sailed".

With it reverted, a range pick of a straight single strand of pearls
would still work just fine.  And the user is forced to think and
chop a range with a merge into a set of subranges each of which is a
single strand of pearls, plus picking individual merges (if picking
these merges is really what the user wants, that is).  As ensuring
the users to think is the whole point of excercise, the original
system before we allowed "-m1" for single parent commit was after
all giving us the right balance, I guess, without having to add yet
another new option.

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> The same effect can be had by just reverting "let's allow -m1 for
>>> single-parent commit", can't it?  That is a far simpler solution, I
>>> would say.
>>
>> Those one didn't introduce the issue currently at hand, as we still
>> don't allow merges by default, so why do we need to rewind it?
>
> With it reverted, "[alias] cp = cherry-pick -m1" can be used to train
> the user to blindly pick a range that has a merge without thinking,
> which is what I meant by "ship has already sailed".

Did you mean "With it *not* reverted" here? Because with it reverted,
"cherry-pick -m1" will rather loudly fail on the first non-merge commit
in the range. Such alias would be useless without that patch.

Those who don't like such alias are still free not to define or use it.

> With it reverted, a range pick of a straight single strand of pearls
> would still work just fine.  And the user is forced to think and
> chop a range with a merge into a set of subranges each of which is a
> single strand of pearls, plus picking individual merges (if picking
> these merges is really what the user wants, that is).

Unfortunately, this gets us back to the exact problem with practical
use-case the patch was provoked by. There was a large number of commits
to be picked, and the set has been carefully built by "git rev-list" (as
"git cherry-pick" built-in features were not enough: yet another issue).
Some of these commits were merges, and handling all this manually (and
repeatedly) would be a real pain. It was easier to locally patch "git
cherry-pick -m1" to achieve the goal. Then I thought that it could help
others in the future, and then took time to provide the patch. It'd be
a real pity to get back to where I started.

BTW, the above also shows that the issue with "-m 1" existed even before
"git cherry-pick" started to accept ranges, as I was not using these
ranges anyway.

Overall, I mean that I still need a way to tell "git cherry-pick" that I
do know what I'm doing, so that it stops complaining on a non-issue.
Thus if the patch is reverted, a new option should be added just for
this goal, -- back to where we are now, but an option already added.

> As ensuring the users to think is the whole point of excercise,

Yeah, provided they do have a suitable way out. Without that patch,
there was none.

> the original system before we allowed "-m1" for single parent commit
> was after all giving us the right balance, I guess, without having to
> add yet another new option.

No, unfortunately it didn't, and to me the patch in question still seems
to be in the right direction, legitimate, and useful.

Moreover, I still can't see how it's harmful. Just don't use '-m 1' if
you don't want merges.

-- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> With it reverted, "[alias] cp = cherry-pick -m1" can be used to train
>> the user to blindly pick a range that has a merge without thinking,
>> which is what I meant by "ship has already sailed".
>
> Did you mean "With it *not* reverted" here?

Thanks for a correction.  Yes, if we do not revert it, then that
would allow people to follow a bad workflow we do not want to
recommend (and I think that is what Elijah does not want to do), and
that is why I said the ship has already sailed.

> Those who don't like such alias are still free not to define or use it.

That's not the point.  Those who do want to be careful can learn to
use a new option --forbid-stupid-things, but why should they?  They
should be forbidden from doing stupid things by default, which is
the point of this exchange.

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Johannes Schindelin]

Hi Elijah,

On Wed, 20 Mar 2019, Elijah Newren wrote:

> On Wed, Mar 20, 2019 at 8:09 AM Sergey Organov <sorganov@gmail.com> wrote:
> >
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > [...]
> >
> > > But I do have a very strong opinion against adding yet another
> > > option that takes an optional argument.  If we want to allow
> > > cherry-picking a merge commit just as easy as cherrry-picking a
> > > single-parent commit, "git cherry-pick -m merge" (assuming 'merge'
> > > is the tip of a branch that is a merge commit) that still requires
> > > the user to say "-m" is not a good improvement.  We should just
> > > accept "git cherry-pick merge" without any "-m" if we want to move
> > > in this direction, I would think.
> >
> > Let's just make '-m 1' the default option indeed. No need for further
> > complexities.
> >
> > Exactly according to what Junio has already said before. Here:
> >
> > https://public-inbox.org/git/xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com
> >
> > Junio wrote:
> >
> > > Now, it appears, at least to me, that the world pretty much accepted
> > > that the first-parent worldview is often very convenient and worth
> > > supporting by the tool, so the next logical step might be to set
> > > opts->mainline to 1 by default (and allow an explicit "-m $n" from
> > > the command line to override it).  But that should happen after this
> > > patch lands---it is logically a separate step, I would think.
> >
> > ... and as that patch already landed...
>
> This worries me that it'll lead to bad surprises.

Indeed. Merge commits simply do not have the same semantics as regular
commits. They not only have more than one parent, they also have the
further complication that they, unlike regular commits, do not introduce
regular code changes, but need to reconcile diverging code changes
instead.

As such, cherry-picking a merge commit typically leads to *many, many*
more merge conflicts than cherry-picking regular commits.

It would appear to be a wise idea to keep that safety line in place, where
users cherry-picking the wrong commit would have to tell Git that they
really are sure that they want to cherry-pick a merge commit.

And I know that I've been there myself, so it is not just some users you
might dismiss as less than smart (which is actually not a smart thing to
do, users are the essential audience of our software, and if we make it
hard for users, it is not the users who failed).

We cannot just "ignore away" that merge commits are different from regular
commits: Neither in the data shape nor in their purpose are they the same.

Ciao,
Dscho

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Johannes Schindelin]

Hi Junio,

On Thu, 21 Mar 2019, Junio C Hamano wrote:

> Elijah Newren <newren@gmail.com> writes:
>
> > This worries me that it'll lead to bad surprises.  Perhaps some folks
> > cherry-pick merges around intentionally, but I would want that to be a
> > rare occurrence at most.  There are lots of folks at $DAYJOB that
> > cherry-pick things, not all of them are expert git-users, and I am
> > certain several have erroneously attempted to cherry-pick merges
> > before.
>
> It was a lot simpler back when "git cherry-pick" did not accept
> ranges.  You are either knowingly cherry-picking a merge, or doing
> so by mistake, and because the command rejected cherry-picking a
> merge without being told with "-m $n" which parent the mainline is
> by default, we are assured that the user knew that s/he was picking
> a merge commit when we saw "-m $n".

Indeed.

> It's not so simple in the world after we started allowing picks of a
> range.  "cherry-pick -m1 A..B" did not work when the range A..B is a
> mixture of merges and non-merges (which is the case 100% of the
> time), as the command used to error out when given the -m option for
> a single-parent commit.  Earlier we said that "as long as the $n
> does not exceed the number of actual parents, let's allow '-m $n'
> even for non-merge commits." to fix it.
>
> We can just reject this RFC patch and we'd be in a slightly safer
> place.  You still need to tell us with "-m 1" on the command line
> that you are picking a range with merges in it.  But then I am sure
> that clueless people blindly would alias "pick = cherry-pick -m1" and
> use "git pick" and blindly pick ranges here and there, so I am not
> sure such a slightly-more safety buys us very much.

It gets even better.

Recently, I found myself wishing for the equivalent of `--rebase-merges`
in `git cherry-pick`. In other words, I wanted to transplant some commits
(including merge commits) from a different branch onto the current branch.

Currently, I work around this by this kind of dance:

	$ git checkout B^0
	$ git rebase -ir --onto HEAD@{1} A
	$ git checkout @{-1}
	$ git merge --ff-only HEAD@{1}

which does work, but it changes the worktree quite a bit (often causing
complete rebuilds due to "changed" header files, when those rebuilds were
not actually necessary).

It also has the rather real downside of not mixing well with an
already-running rebase...

And with that use case in mind, I think that making the `-m 1` option the
default is even less desirable than I thought before.

Ciao,
Dscho

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> With it reverted, "[alias] cp = cherry-pick -m1" can be used to train
>>> the user to blindly pick a range that has a merge without thinking,
>>> which is what I meant by "ship has already sailed".
>>
>> Did you mean "With it *not* reverted" here?
>
> Thanks for a correction.  Yes, if we do not revert it, then that
> would allow people to follow a bad workflow we do not want to
> recommend (and I think that is what Elijah does not want to do), and
> that is why I said the ship has already sailed.

I still don't think it makes sense to revert the patch (that fixed a
real-life issue) on the sole ground that, as a side-effect, it has
provided an opportunity that could potentially be abused, specifically
by defining a random alias, and then shooting oneself in the foot with
it. And even then no irreversible damage actually happens.

Moreover, if somebody actually wants to "follow a bad workflow", he
still needs to ask for it explicitly, either by providing '-m 1', or by
defining and using an alias, so let him do it please, maybe he even does
know what he is doing, after all.

>
>> Those who don't like such alias are still free not to define or use it.
>
> That's not the point.  Those who do want to be careful can learn to
> use a new option --forbid-stupid-things, but why should they?

Sure thing, who said they should? Fortunately, that's exactly the
current state, no need to invent and specify any --forbid-stupid-things
option, and even if we pretend the option is already there and is
active by default, still no need to revert anything.

> They should be forbidden from doing stupid things by default, which is
> the point of this exchange.

I already agreed before to assume this, and it seems that we now all
agree this safety should be preserved, as there are those who actually
care. However, as merges are already forbidden right now with all the
current defaults, I fail to see how it could justify reverting of
already applied patch.

To me, the actual question here is: what's the option that overrides
that default? The current answer is: "-m 1", that admittedly is not very
nice, but has not been introduced by any of the recent patches, so is
not solvable by reverting any of them.

To summarize, as it looks to me, it's mostly the current way of allowing
merges, that cryptically reads as "-m 1", that makes the OP unhappy.
This was already the case before the "allow '-m 1' for non-merge
commits" patch, so reverting it won't solve the problem in any suitable
way.

Due to all the above, may we please finally let alone the already
applied patch and focus on finding (or denying) actual solution to the
original issue of this thread?

If so, I'm still on the ground of providing new, say,
"--no-forbid-merges" option, if anything. I'm with Duy Nguyen that the
way suggested by RFC, making value optional for yet another short
option, is to be avoided at all costs.

-- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[C.J. Jameson]

Thank you everyone for the comments!

Confirming Sergey's summary: at this point, I think my only residual
opinion is that requiring `-m 1` or `--mainline 1` is a little cryptic
for someone who's just cherry-picking a single commit, which happens
to be a merge commit. `--no-forbid-merges` would be the thorough way
of accommodating it, but it's even more verbose and you'd still need
to discover it...

I'd be fine abandon this -- thanks again!

C.J.

On Fri, Mar 22, 2019 at 8:22 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Sergey Organov <sorganov@gmail.com> writes:
> >
> >>> With it reverted, "[alias] cp = cherry-pick -m1" can be used to train
> >>> the user to blindly pick a range that has a merge without thinking,
> >>> which is what I meant by "ship has already sailed".
> >>
> >> Did you mean "With it *not* reverted" here?
> >
> > Thanks for a correction.  Yes, if we do not revert it, then that
> > would allow people to follow a bad workflow we do not want to
> > recommend (and I think that is what Elijah does not want to do), and
> > that is why I said the ship has already sailed.
>
> I still don't think it makes sense to revert the patch (that fixed a
> real-life issue) on the sole ground that, as a side-effect, it has
> provided an opportunity that could potentially be abused, specifically
> by defining a random alias, and then shooting oneself in the foot with
> it. And even then no irreversible damage actually happens.
>
> Moreover, if somebody actually wants to "follow a bad workflow", he
> still needs to ask for it explicitly, either by providing '-m 1', or by
> defining and using an alias, so let him do it please, maybe he even does
> know what he is doing, after all.
>
> >
> >> Those who don't like such alias are still free not to define or use it.
> >
> > That's not the point.  Those who do want to be careful can learn to
> > use a new option --forbid-stupid-things, but why should they?
>
> Sure thing, who said they should? Fortunately, that's exactly the
> current state, no need to invent and specify any --forbid-stupid-things
> option, and even if we pretend the option is already there and is
> active by default, still no need to revert anything.
>
> > They should be forbidden from doing stupid things by default, which is
> > the point of this exchange.
>
> I already agreed before to assume this, and it seems that we now all
> agree this safety should be preserved, as there are those who actually
> care. However, as merges are already forbidden right now with all the
> current defaults, I fail to see how it could justify reverting of
> already applied patch.
>
> To me, the actual question here is: what's the option that overrides
> that default? The current answer is: "-m 1", that admittedly is not very
> nice, but has not been introduced by any of the recent patches, so is
> not solvable by reverting any of them.
>
> To summarize, as it looks to me, it's mostly the current way of allowing
> merges, that cryptically reads as "-m 1", that makes the OP unhappy.
> This was already the case before the "allow '-m 1' for non-merge
> commits" patch, so reverting it won't solve the problem in any suitable
> way.
>
> Due to all the above, may we please finally let alone the already
> applied patch and focus on finding (or denying) actual solution to the
> original issue of this thread?
>
> If so, I'm still on the ground of providing new, say,
> "--no-forbid-merges" option, if anything. I'm with Duy Nguyen that the
> way suggested by RFC, making value optional for yet another short
> option, is to be avoided at all costs.
>
> -- Sergey

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[Johannes Schindelin]

Hi C.J.

On Fri, 22 Mar 2019, C.J. Jameson wrote:

> Confirming Sergey's summary: at this point, I think my only residual
> opinion is that requiring `-m 1` or `--mainline 1` is a little cryptic
> for someone who's just cherry-picking a single commit, which happens
> to be a merge commit.

Two points:

- You do not happen to cherry-pick a merge commit. If you do that, and do
  not realize that it is a merge commit, then you have no idea what you
  actually wanted to pick (the changes relative to the first parent? or the
  to the second parent?). The fact that Git does not let you do that gives
  you the opportunity to pause, think, and then be sure what you actually
  want.

  So Git does the right thing there.

- If it is a little cryptic that `-m 1` (or `-m 2` or `-m 3` or...) is
  needed, then this needs to be made less cryptic. A fantastic idea would
  be to teach Git to mention this hint in the exact circumstance when it
  matters most: when the user just tried to cherry-pick a merge commit.

  I actually do not know whether Git does the right thing there. If it
  does not, that would be a good direction to focus your efforts on.

> `--no-forbid-merges` would be the thorough way of accommodating it, but
> it's even more verbose and you'd still need to discover it...

But it would definitely make sure that we do not pretend that merge
commits are anything like non-merge commits.

As I pointed out elsewhere in this thread: non-merge commits' purpose in
life is to introduce something new, in a relatively small, neat package.
If it does not fit into one small, neat package, you need to split it
among multiple non-merge commits.

A *merge commit*'s purpose in life is not to introduce something new, but
to reconcile diverging changes. I.e. it is more of a moderator, a mediator
between battling non-merge commits. Often, the only real changes a merge
commit introduces are changes that are necessary to make the two diverging
branches work well together.

It can be hard in practice to appreciate the difference, but it is very
real.

For example, when you `git cherry-pick -m 1 <merge>`, you will typically
end up with a *ton* more merge conflicts than when you cherry-pick a
non-merge commit.

And when you cherry-pick a merge commit, it is much more like performing a
squash merge than performing a cherry-pick, and resolving the merge
conflicts looks a lot more like bringing peace to a shouting match, much
like when you resolve merge conflicts during a *merge* (as opposed to
resolving merge conflicts during a cherry-pick, which feels a lot more
like helping a patch move into a new city it does not yet quite know).

(This is at least what my experience is, from working with 70+ branches
in Windows that I maintain on top of core Git.)

Ciao,
Dscho

> I'd be fine abandon this -- thanks again!
>
> C.J.
>
> On Fri, Mar 22, 2019 at 8:22 AM Sergey Organov <sorganov@gmail.com> wrote:
> >
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > > Sergey Organov <sorganov@gmail.com> writes:
> > >
> > >>> With it reverted, "[alias] cp = cherry-pick -m1" can be used to train
> > >>> the user to blindly pick a range that has a merge without thinking,
> > >>> which is what I meant by "ship has already sailed".
> > >>
> > >> Did you mean "With it *not* reverted" here?
> > >
> > > Thanks for a correction.  Yes, if we do not revert it, then that
> > > would allow people to follow a bad workflow we do not want to
> > > recommend (and I think that is what Elijah does not want to do), and
> > > that is why I said the ship has already sailed.
> >
> > I still don't think it makes sense to revert the patch (that fixed a
> > real-life issue) on the sole ground that, as a side-effect, it has
> > provided an opportunity that could potentially be abused, specifically
> > by defining a random alias, and then shooting oneself in the foot with
> > it. And even then no irreversible damage actually happens.
> >
> > Moreover, if somebody actually wants to "follow a bad workflow", he
> > still needs to ask for it explicitly, either by providing '-m 1', or by
> > defining and using an alias, so let him do it please, maybe he even does
> > know what he is doing, after all.
> >
> > >
> > >> Those who don't like such alias are still free not to define or use it.
> > >
> > > That's not the point.  Those who do want to be careful can learn to
> > > use a new option --forbid-stupid-things, but why should they?
> >
> > Sure thing, who said they should? Fortunately, that's exactly the
> > current state, no need to invent and specify any --forbid-stupid-things
> > option, and even if we pretend the option is already there and is
> > active by default, still no need to revert anything.
> >
> > > They should be forbidden from doing stupid things by default, which is
> > > the point of this exchange.
> >
> > I already agreed before to assume this, and it seems that we now all
> > agree this safety should be preserved, as there are those who actually
> > care. However, as merges are already forbidden right now with all the
> > current defaults, I fail to see how it could justify reverting of
> > already applied patch.
> >
> > To me, the actual question here is: what's the option that overrides
> > that default? The current answer is: "-m 1", that admittedly is not very
> > nice, but has not been introduced by any of the recent patches, so is
> > not solvable by reverting any of them.
> >
> > To summarize, as it looks to me, it's mostly the current way of allowing
> > merges, that cryptically reads as "-m 1", that makes the OP unhappy.
> > This was already the case before the "allow '-m 1' for non-merge
> > commits" patch, so reverting it won't solve the problem in any suitable
> > way.
> >
> > Due to all the above, may we please finally let alone the already
> > applied patch and focus on finding (or denying) actual solution to the
> > original issue of this thread?
> >
> > If so, I'm still on the ground of providing new, say,
> > "--no-forbid-merges" option, if anything. I'm with Duy Nguyen that the
> > way suggested by RFC, making value optional for yet another short
> > option, is to be avoided at all costs.
> >
> > -- Sergey
>

Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

[C.J. Jameson]

On Mon, Mar 25, 2019 at 7:56 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi C.J.
>
> On Fri, 22 Mar 2019, C.J. Jameson wrote:
>
> > Confirming Sergey's summary: at this point, I think my only residual
> > opinion is that requiring `-m 1` or `--mainline 1` is a little cryptic
> > for someone who's just cherry-picking a single commit, which happens
> > to be a merge commit.
>
> Two points:
>
> - You do not happen to cherry-pick a merge commit. If you do that, and do
>   not realize that it is a merge commit, then you have no idea what you
>   actually wanted to pick (the changes relative to the first parent? or the
>   to the second parent?). The fact that Git does not let you do that gives
>   you the opportunity to pause, think, and then be sure what you actually
>   want.
>
>   So Git does the right thing there.
>
> - If it is a little cryptic that `-m 1` (or `-m 2` or `-m 3` or...) is
>   needed, then this needs to be made less cryptic. A fantastic idea would
>   be to teach Git to mention this hint in the exact circumstance when it
>   matters most: when the user just tried to cherry-pick a merge commit.
>
>   I actually do not know whether Git does the right thing there. If it
>   does not, that would be a good direction to focus your efforts on.
>
> > `--no-forbid-merges` would be the thorough way of accommodating it, but
> > it's even more verbose and you'd still need to discover it...
>
> But it would definitely make sure that we do not pretend that merge
> commits are anything like non-merge commits.
>
> As I pointed out elsewhere in this thread: non-merge commits' purpose in
> life is to introduce something new, in a relatively small, neat package.
> If it does not fit into one small, neat package, you need to split it
> among multiple non-merge commits.
>
> A *merge commit*'s purpose in life is not to introduce something new, but
> to reconcile diverging changes. I.e. it is more of a moderator, a mediator
> between battling non-merge commits. Often, the only real changes a merge
> commit introduces are changes that are necessary to make the two diverging
> branches work well together.
>
> It can be hard in practice to appreciate the difference, but it is very
> real.
>
Totally. My context is that we have way too many low-value / no-value merge
commits in our repo, and we put our version tags on the merge commits rather
than the non-merge commits (the ones with the meaningful code changes).
I wish it were different...

It's cool you phrase it this way, because it highlights how our repo's settings
stray from the meaningful purpose-in-life of these git fundamentals. In Gitlab
(same is possible in Github), we have our repo configured to:

- always create a merge commit when merging to master, even if it's
  fast-forwardable
- auto-squash all Merge Requests / Pull Requests into one commit
  when merging

Thank you all for helping me get a very deep understanding of the matter,
because it enables me to keep pushing my team to select different settings.

> For example, when you `git cherry-pick -m 1 <merge>`, you will typically
> end up with a *ton* more merge conflicts than when you cherry-pick a
> non-merge commit.
>
> And when you cherry-pick a merge commit, it is much more like performing a
> squash merge than performing a cherry-pick, and resolving the merge
> conflicts looks a lot more like bringing peace to a shouting match, much
> like when you resolve merge conflicts during a *merge* (as opposed to
> resolving merge conflicts during a cherry-pick, which feels a lot more
> like helping a patch move into a new city it does not yet quite know).
>
Yea... we don't feel the pain of squash-like merging at cherry-pick time,
because pull-requestors' merge requests have already been squashed down
whether they like it or not...! :( uff da

> (This is at least what my experience is, from working with 70+ branches
> in Windows that I maintain on top of core Git.)
>
> Ciao,
> Dscho
>
> > I'd be fine abandon this -- thanks again!
> >
> > C.J.
> >
> > On Fri, Mar 22, 2019 at 8:22 AM Sergey Organov <sorganov@gmail.com> wrote:
> > >
> > > Junio C Hamano <gitster@pobox.com> writes:
> > >
> > > > Sergey Organov <sorganov@gmail.com> writes:
> > > >
> > > >>> With it reverted, "[alias] cp = cherry-pick -m1" can be used to train
> > > >>> the user to blindly pick a range that has a merge without thinking,
> > > >>> which is what I meant by "ship has already sailed".
> > > >>
> > > >> Did you mean "With it *not* reverted" here?
> > > >
> > > > Thanks for a correction.  Yes, if we do not revert it, then that
> > > > would allow people to follow a bad workflow we do not want to
> > > > recommend (and I think that is what Elijah does not want to do), and
> > > > that is why I said the ship has already sailed.
> > >
> > > I still don't think it makes sense to revert the patch (that fixed a
> > > real-life issue) on the sole ground that, as a side-effect, it has
> > > provided an opportunity that could potentially be abused, specifically
> > > by defining a random alias, and then shooting oneself in the foot with
> > > it. And even then no irreversible damage actually happens.
> > >
> > > Moreover, if somebody actually wants to "follow a bad workflow", he
> > > still needs to ask for it explicitly, either by providing '-m 1', or by
> > > defining and using an alias, so let him do it please, maybe he even does
> > > know what he is doing, after all.
> > >
> > > >
> > > >> Those who don't like such alias are still free not to define or use it.
> > > >
> > > > That's not the point.  Those who do want to be careful can learn to
> > > > use a new option --forbid-stupid-things, but why should they?
> > >
> > > Sure thing, who said they should? Fortunately, that's exactly the
> > > current state, no need to invent and specify any --forbid-stupid-things
> > > option, and even if we pretend the option is already there and is
> > > active by default, still no need to revert anything.
> > >
> > > > They should be forbidden from doing stupid things by default, which is
> > > > the point of this exchange.
> > >
> > > I already agreed before to assume this, and it seems that we now all
> > > agree this safety should be preserved, as there are those who actually
> > > care. However, as merges are already forbidden right now with all the
> > > current defaults, I fail to see how it could justify reverting of
> > > already applied patch.
> > >
> > > To me, the actual question here is: what's the option that overrides
> > > that default? The current answer is: "-m 1", that admittedly is not very
> > > nice, but has not been introduced by any of the recent patches, so is
> > > not solvable by reverting any of them.
> > >
> > > To summarize, as it looks to me, it's mostly the current way of allowing
> > > merges, that cryptically reads as "-m 1", that makes the OP unhappy.
> > > This was already the case before the "allow '-m 1' for non-merge
> > > commits" patch, so reverting it won't solve the problem in any suitable
> > > way.
> > >
> > > Due to all the above, may we please finally let alone the already
> > > applied patch and focus on finding (or denying) actual solution to the
> > > original issue of this thread?
> > >
> > > If so, I'm still on the ground of providing new, say,
> > > "--no-forbid-merges" option, if anything. I'm with Duy Nguyen that the
> > > way suggested by RFC, making value optional for yet another short
> > > option, is to be avoided at all costs.
> > >
> > > -- Sergey
> >