[infinispan-dev] Branching proposal

Mon Mar 27 07:51:19 EDT 2017

On Mon, Mar 27, 2017 at 1:05 PM Sanne Grinovero <sanne at infinispan.org>
wrote:

> You need to make sure you optimise for engineers coding stuff which
> works on master, not maintenance.
>

Well it depends on your strategy. Note that with my proposal you can always
do that (optimize features for development not for maintenance).... just
put your PR on master branch. That's all...

The proposal is all about making maintenance easier. Development stays the
same.

> Isn't it a bit naive to expect people to work on the "last maintained
> branch" ?
>

I don't think so. In that past I worked on a project with 3 maintenance
branches plus a development branch. It worked. That's why I'm proposing
this here.

> Your proposal expect that each and every patch author:
>   a) knows for sure it's not going to be backported further to even
> older branches
>

As I explained this to Radim, occasional cherry-picking is totally fine.
During the merge git will look at the commit, say... oh this commit is
already there and move on.

>   b) you're not investigating issues on master (as investigation often
> implies making code changes already)
>

Again it depends. If you do not care about maintenance branches, you can
still code at master branch.

I also disagree with you here. We should always fix errors where they occur
and not only in maintenance branch. If the failure is on 9.1.x branch, then
the fix should go there. Not to master.

> Both of these are not very practical as I generally don't expect to
> backport each and every fix, and it's going to be someone else to
> recommend/approve some backports *after* the problem has been
> understood in depth.
>

In that kind of workflow my proposal wouldn't work well. I'm also a bit
curious, how often do we need such an approval?

> You even mention "forgetting to backport refactoring" ? Hell yes you
> should never backport refactorings..

It depends. How about our checkstyle changes? After some time I guess I
should have backported them to make cherry-picking easier.

> What I'm reading here is that you're all doing way too many backports.
> Be more conservative about them?
>

That's a very well stated question. My proposal makes the maintenance
easier but developers need to be a bit more responsible (they need to know
in which branch to implement a given fix and remember about doing merges).
On the other hand if we prefer to encourage our users to move to more
recent version quicker, than it probably doesn't make much sense.

> In my experience with recent contributions to Infinispan - either
> because the testsuite is slow, or because it takes us too long to
> figure out there has been a regression, I sent a PR to master and two
> maintenance branches but I had to repeat the exercise two more times
> to add follow up fixes on the same subject: if I had waited on
> backports it would have taken me a third of the time.
>

Note that my proposal partially addresses this concern. Individual PRs
wouldn't need to go through CI loop but they would be merged in batches. On
the other hand, even an easy merge might break the build.

>
> Thanks,
> Sanne
>
>
>
>
> On 27 March 2017 at 11:45, Sebastian Laskawiec <slaskawi at redhat.com>
> wrote:
> > From my past experience, if a commit caused a conflict when merging, we
> > always asked the author to fix it and do the merge.
> >
> > After a while it became a habit that each dev who submitted a code that
> > could result in conflicts, did all the merges.
> >
> > On Mon, Mar 27, 2017 at 12:37 PM Radim Vansa <rvansa at redhat.com> wrote:
> >>
> >> If you can't merge a commit (based on 9.0.x) to master clearly, do you
> >> need to file another PR anyway? Then the lag to get some code to master
> >> increases a lot. I am not sure how useful is git tag --contains <sha1>
> >> if you cannot be sure that you'll find all occurrences due to this kind
> >> of issues.
> >>
> >> R.
> >>
> >> On 03/27/2017 11:33 AM, Sebastian Laskawiec wrote:
> >> > Hey!
> >> >
> >> > We are about to start working on 9.1.x and 9.2.y branches so I would
> >> > like to propose alternative merging strategy.
> >> >
> >> > Our current workflow looks like this:
> >> >
> >> > X - new commit
> >> > X` - cherry pick to maintenance branch
> >> > --+-------------------+-------X----- master
> >> >   |                    \------X`---- 9.2.x
> >> >   \---------------------------X``--- 9.1.x
> >> >
> >> > Each commit needs to be reviewed in master branch and backported to
> >> > the maintenance branches. From maintenance perspective this is a bit
> >> > painful, since in above example we need to get 3 times through PR
> >> > queue. Also it's worth to mention that X is not X` nor X``.
> >> > Cherry-picking creates a copy of a commit. This makes some useful
> >> > tricks (like git tag --contains <sha1>) a bit harder to use. Finally,
> >> > this approach allows the codebase to diverge from maintenance branches
> >> > very fast (someone might just forget to backport some of the
> >> > refactoring stuff).
> >> >
> >> > The proposal:
> >> >
> >> > X, Y - new commits
> >> > / - merge commits
> >> > --+---------+------/----/--- master
> >> >   |          \----/---Y/---- 9.2.x
> >> >   \-------------X/---------- 9.1.x
> >> >
> >> > With the proposal, a developer should always implement a given feature
> >> > in the lowest possible maintenance branch. Then we will run a set of
> >> > merges from 9.1.x into 9.2.x and finally into master. The biggest
> >> > advantage of this approach is that given functionality (identified by
> >> > a commit) will have the same SHA1 for all branches. This will allow
> >> > all tools like (mentioned before) `git tag --contains <sha1>` to work.
> >> > There are also some further implications of this approach:
> >> >
> >> >   * Merging commits should be performed very often (even automatically
> >> >     in the night (if merged without any problems)).
> >> >   * After releasing each maintenance release, someone will need to do
> >> >     a merge with strategy `ours` (`git merge -s ours upstream/9.2.x`).
> >> >     This way we will not have to solve version conflicts in poms.
> >> >   * Since there is no nice way to rebase a merge commit, they should
> >> >     be pushed directly into the master branch (without review, without
> >> >     CI). After the merge, HEAD will change and CI will
> >> >     automatically pick the build. Remember, merges should be done very
> >> >     often. So I assume there won't be any problems most of the times.
> >> >   * Finally, with this approach the code diverges slight slower (at
> >> >     least from my experience). Mainly because we don't need to
> >> >     remember to cherry-pick individual commits. They are automatically
> >> >     "taken" by a merge.
> >> >
> >> > From my past experience, this strategy works pretty nice and can be
> >> > almost fully automated. It significantly lowers the maintenance pain
> >> > around cherry-picks. However there is nothing for free, and we would
> >> > need to get used to pushing merged directly into master (which is fine
> >> > to me but some of you might not like it).
> >> >
> >> > Thanks,
> >> > Sebastian
> >> >
> >> >
> >> > _______________________________________________
> >> > infinispan-dev mailing list
> >> > infinispan-dev at lists.jboss.org
> >> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >>
> >>
> >> --
> >> Radim Vansa <rvansa at redhat.com>
> >> JBoss Performance Team
> >>
> >> _______________________________________________
> >> infinispan-dev mailing list
> >> infinispan-dev at lists.jboss.org
> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20170327/ec1531a8/attachment-0001.html 