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

["C.J. Jameson" <cjcjameson@gmail.com>]

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
> >