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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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
>