Re: [PATCH v3] rebase --merge: optionally skip upstreamed commits

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 11, 2020, 01:11 UTC
Message-ID: <20200411011152.115895-1-jonathantanmy@google.com>
In-Reply-To: <CABPp-BFEtd9X1XJwwpJ2YP6a4Zyc_yevwnMfNA+Dr45p4-7YEA@mail.gmail.com>

Show 7 quoted lines

> >   Reapply all clean cherry-picks of any upstream commit instead of
> >   dropping them. (If these commits then become empty after rebasing,
> >   because they contain a subset of already upstream changes, the
> >   behavior towards them is controlled by the `--empty` flag.)
> 
> Perhaps add "preemptively" in there, so that it reads "...instead of
> preemptively dropping them..."?

Sounds good. Yes I can do this.

Show 13 quoted lines

> > If this works, I'll send out a new version containing Elijah's patches
> > and mine in whatever branch my patch shows up in [1].
> >
> > [1] https://lore.kernel.org/git/xmqqd08fhvx5.fsf@gitster.c.googlers.com/
> 
> Yeah, I was making changes to this exact same area in my series to
> reference your flags.[2]
> 
> [2] https://lore.kernel.org/git/e15c599c874956f1a297424c68fe28e04c71807b.1586541094.git.gitgitgadget@gmail.com/
> 
> Would you mind if I took your proposed changes, put them in your
> patch, and then rebased your patch on top of my series and touched up
> the wording in the manpage to have the options reference each other?

Go ahead! Thanks.

Show 11 quoted lines

> > > Why not just list --keep-cherry-pick[s] in the list of options that
> > > require use of the merge backend (i.e. the list containing '--merge')
> > > instead of adding another sentence here?
> >
> > My reading of the list containing "--merge" is that they *trigger* the
> > merge backend, not require the merge backend. My new option requires but
> > does not trigger it (unless we want to change it to do so, which I'm
> > fine with).
> 
> Interesting; what part of the man page comes across that way?  That
> may just be poor wording.

"--merge" is documented as "Use merging strategies to rebase", which I interpret as triggering the merge backend. There are other things in the list like "--strategy" and "--interactive", which seem to be things that trigger the merge backend too, so I concluded that the list is about triggering the merge backend, not requiring it.

Show 7 quoted lines

> However, if an option requires a certain backend, is there a reason
> why we would want to require the user to manually specify that backend
> for their chosen option to work?  We know exactly which backend they
> need, so we could just trigger it.  For every other case in rebase I
> can think of, whenever a certain backend was required for an option we
> always made the option trigger that backend (or throw an error if a
> different backend had already been requested).

I guess I wanted to leave open the option to have the same feature in the "apply" (formerly "am") backend. The use cases I am thinking of won't need that in the near future (for partial clone to make use of it in the "apply" backend, the "apply" backend would have to be further improved to batch fetching of missing blobs), though, so it might be best to just require and trigger "merge" (like the other cases you mention). I'll do that in the next version.