git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
C.J. Jameson <cjcjameson@gmail.com>
Date
Mar 25, 2019, 15:41 UTC
Message-ID
<CALm+SVL6ZyVVPqLEiwEzXru8TOjP7JVfpH=THdT9nLvH84O_Mg@mail.gmail.com>
In-Reply-To
<nycvar.QRO.7.76.6.1903251544260.41@tvgsbejvaqbjf.bet>

On Mon, Mar 25, 2019 at 7:56 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 49 quoted lines
>
> 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.

Show 11 quoted lines
> 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

Show 80 quoted lines
> (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
> >
Previous: Johannes SchindelinNext: Sergey Organov
Message 19 of 23 in “cherry-pick: set default `--mainline` parent to 1”
  1. cherry-pick: set default `--mainline` parent to 1C.J. Jameson, Mar 20, 2019
  2. Junio C HamanoMar 20, 2019
  3. Sergey OrganovMar 20, 2019
  4. Elijah NewrenMar 20, 2019
  5. Sergey OrganovMar 20, 2019
  6. C.J. JamesonMar 21, 2019
  7. Junio C HamanoMar 21, 2019
  8. Junio C HamanoMar 21, 2019
  9. Sergey OrganovMar 21, 2019
  10. Junio C HamanoMar 21, 2019
  11. Sergey OrganovMar 21, 2019
  12. Junio C HamanoMar 21, 2019
  13. Junio C HamanoMar 21, 2019
  14. Sergey OrganovMar 21, 2019
  15. Junio C HamanoMar 22, 2019
  16. Sergey OrganovMar 22, 2019
  17. C.J. JamesonMar 22, 2019
  18. Johannes SchindelinMar 25, 2019
  19. C.J. JamesonMar 25, 2019
  20. Sergey OrganovMar 21, 2019
  21. Johannes SchindelinMar 22, 2019
  22. Johannes SchindelinMar 22, 2019
  23. Duy NguyenMar 20, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.