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