On Tue, Oct 24, 2017 at 1:20 PM, Enrico Olivelli <eolive...@gmail.com> wrote:
> Il mar 24 ott 2017, 18:44 Sijie Guo <guosi...@gmail.com> ha scritto: > > > Enrico, > > > > I had a comment in the way how you split the changes. If you split the > > changes by features, do you really need create branches on main repo for > > sending reviews? I don't see any updates to my comments though. > > > > I apologize Sijie, I will split the work as you are suggesting here > https://github.com/apache/bookkeeper/pull/643 > The goal is to make reviews simpler and be able to address comments easily > > > > > I would suggest us thinking carefully before using share branches, or at > > least until we have scripts to automate this, doing this manually is > going > > to be a big pain for committers and hard to maintain as a whole > community. > > > > This is exactly the reason why I have not yet pushed anything to custom > branche and I was waiting for opinions and/or approval. > > I think it is better to stick to natural pull request from private forks > I know Ivan is playing around a script for it. It would be good that we create an issue to work on integrating the idea into the merge script. - Sijie > > > > Sijie > > > > On Oct 24, 2017 8:23 AM, "Enrico Olivelli" <eolive...@gmail.com> wrote: > > > > Sijie, Ivan and/or other PMC > > > > I would like to create branches on shared Apache repo in order to create > > the chain of Pull Requests. > > I think we should agree on a naming convention > > > > What about > > issue-471-xxxxx > > issue-471-yyyyy > > issue-471-zzzzz > > > > where xxxxxx is a local name which describes better the intent of the > > branch and 471 is the id of the master issue for the work, in this case > > issue 471 (BP-14) > > > > I am maintaining the stack of pull requests on my private fork > > https://github.com/eolivelli/bookkeeper/pulls > > > > I would like to move to the final solution as soon as possible, there are > > already many comments from Sijie and Ivan and I am not very happy to lose > > them by closing the Pull Requests > > > > I am happy that this case is helping us to draft the way of working for > > future "big issues" on BookKeeper > > > > -- Enrico > > > > > > > > > > 2017-10-21 9:39 GMT+02:00 Enrico Olivelli <eolive...@gmail.com>: > > > > > > > > > > > Il sab 21 ott 2017, 01:00 Ivan Kelly <iv...@apache.org> ha scritto: > > > > > >> Hey Enrico, > > >> > > >> I think we can do better than that stacked PR blog says. GH now allows > > >> you to change the target of a PR which wasn't the case when that was > > >> written. > > > > > > > > > Good news! > > > > > > I think we could even start using branches in the main repo, > > >> and delete them after we merge, and set the base of the pull request > > >> to be the previous patch in the series. > > >> > > >> I've put up an example, and commands I used are here: > > >> https://gist.github.com/ivankelly/bfe16c81156dc3be7123921f37a6274d > > >> > > >> Each PR can be reviewed individually. Any changes for a PR can be > > >> pushed for that branch. If conflicts occur, you can rebase or merge to > > >> fix it. In the later patches. > > >> > > >> When a patch is merged, any pull requests that depend on it can have > > >> it's base changed to master, and then the branch can be deleted. > > >> > > >> It should be straightforward to add a script to do this to avoid the > > >> typing. > > >> > > >> Something like. > > >> dev/bk-stacked-pr.py -h foobar -n ivan/my-feature > > >> > > >> Where -h is the sha1 head of list of commits to create a pull request > > >> for, and -n is the basename of the branches that will be created. > > >> > > >> Unfortunately this would only be possible for committers, but > > >> something similar could be done with private forks, though the reviews > > >> wouldn't be as visible. > > >> > > >> The base changing, branch cleanup can be done in bk-merge-pr.py. > > >> > > > > > > Thank you I will try to use your examples. > > > > > > In order to use shared branches on apache repo I think that it would be > > > very useful but only for this kind of big patches. > > > Do we need some PMC formal consensus to start using such feature > > branches? > > > > > > We must define naming conventions > > > > > > On such branches we should allow force push, maybe we are protecting > only > > > master and release branches, I will check this > > > > > > Next week I will continue adding new patches and maybe move to apache > > repo > > > if we have the approval. > > > > > > Enrico > > > > > > > > >> -Ivan > > >> > > >> On Fri, Oct 20, 2017 at 8:19 AM, Enrico Olivelli <eolive...@gmail.com > > > > >> wrote: > > >> > I have created a chain of stacked Pull Requests using my private > fork > > >> > (eolivelli/bookkeeper) > > >> > > > >> > https://github.com/eolivelli/bookkeeper/pulls > > >> > > > >> > https://github.com/eolivelli/bookkeeper/pull/2 part 1 > > >> > https://github.com/eolivelli/bookkeeper/pull/3 part 2 > > >> > https://github.com/eolivelli/bookkeeper/pull/4 part 3 > > >> > > > >> > Each Pull Request is against the previous one: > > >> > > > >> > Pros: > > >> > - this makes it very simple to handle diffs visually and so to > comment > > >> on > > >> > specific changes > > >> > - when I change a line in PR1, simply I use "merge" (git checkout > > >> > PR2-branch, git merge PR1-branch, git checkout PR3-branch, git merge > > >> > PR3-branch) > > >> > > > >> > Cons: > > >> > - the conversation will be hosted entirely on my GitHub fork, non on > > the > > >> > Apache one > > >> > > > >> > > > >> > The alternative approach is to have separate Pull Requests on Apache > > >> repo, > > >> > like these > > >> > > > >> > https://github.com/apache/bookkeeper/pull/642 > > >> > https://github.com/apache/bookkeeper/pull/643 > > >> > > > >> > Pros: > > >> > - the conversation is on Apache stuff > > >> > > > >> > Cons: > > >> > - the developer (me in this case) has to fix all the commits and > > perform > > >> > clean up in order to port changes in downstream pull requests to > > >> upstream > > >> > once > > >> > > > >> > > > >> > > > >> > I hope we can find a good way of working on this kind on "huge > > changes" > > >> and > > >> > let "reviewers" and "contributors" do their work without wasting > > >> precious > > >> > time > > >> > > > >> > As a contributor in this case I prefer approach 1), because life is > > >> really > > >> > easy, maybe I am not a git guru and there is a better way to handle > > >> > approach 2) > > >> > > > >> > There is a 1 and 1/2 approach, that is to do 1) but with branches on > > >> Apache > > >> > repo, but Ivan suggested to use private forks (and I agree with > Ivan) > > >> > > > >> > Thank you all (expecially Sijie and Ivan) for you help and support, > I > > do > > >> > not want to waste reviewers' time, It takes time and patience to > > >> "really" > > >> > review code > > >> > > > >> > Thoughts ? > > >> > > > >> > Enrico > > >> > > > >> > > > >> > > > >> > > > >> > 2017-10-18 17:38 GMT+02:00 Ivan Kelly <iv...@apache.org>: > > >> > > > >> >> For branching, I would create the on your own personal fork. > > >> >> > > >> >> -Ivan > > >> >> > > >> >> On Wed, Oct 18, 2017 at 8:37 AM, Ivan Kelly <iv...@apache.org> > > wrote: > > >> >> > I had a quick look, I'll get to reviewing today (in bay area this > > >> >> > week, so day is just starting), but looks great. > > >> >> > > > >> >> > -Ivan > > >> >> > > > >> >> > On Wed, Oct 18, 2017 at 8:31 AM, Enrico Olivelli < > > >> eolive...@gmail.com> > > >> >> wrote: > > >> >> >> Hi, > > >> >> >> I am trying to split the patch for BP-14 in multiple pieces > > >> >> >> > > >> >> >> This is the umbrella issue, I will link here all the Pull > Requests > > >> >> >> https://github.com/apache/bookkeeper/issues/471 > > >> >> >> > > >> >> >> I am actually in work-in-progress, I have covered metadata and > > >> client > > >> >> side > > >> >> >> changes with the first 2 PRs. > > >> >> >> > > >> >> >> It will take some more days to create all of the PRs. > > >> >> >> > > >> >> >> In order to make it simpler to create stacked PRs it would be > > >> better to > > >> >> >> create branches and PRs not against the master but every PR is > to > > be > > >> >> sent > > >> >> >> against the previous one in the list, see [1] as Ivan suggested. > > >> >> >> > > >> >> >> I wounder if it would be better to use my own github repo or to > > >> create > > >> >> >> branches on Apache repo. > > >> >> >> I see many projects are using branches directly on apache > > >> repository, > > >> >> for > > >> >> >> instance Apache Maven. > > >> >> >> I am not a fan of this because it would lead to some kind of > > >> pollution > > >> >> in > > >> >> >> the primary repo. > > >> >> >> > > >> >> >> I will try to set up the whole thing in my repo and come back > > maybe > > >> >> with a > > >> >> >> better approach. > > >> >> >> > > >> >> >> For me it is a real experiment I hope I am not creating too much > > >> noise > > >> >> >> > > >> >> >> Cheers > > >> >> >> Enrico > > >> >> >> > > >> >> >> [1] > > >> >> >> https://graysonkoonce.com/stacked-pull-requests-keeping- > > >> >> github-diffs-small/ > > >> >> > > >> > > > -- > > > > > > > > > -- Enrico Olivelli > > > > > > -- > > > -- Enrico Olivelli >
