Re: [hornetq-dev] Suggestions for enhancing HornetQ development work flow

I'm fine with that... lets start not closing PRs then!.... let me add some background for why we were doing it. The reason we started closing the PRs wasn't to keep the list low... was to avoid someone without the context of the PR to merge it. Say you guys worked during your day in UK, when I start working in US (say if I started working later that day) and you guys are already sleeping.. if I see a PR open I could merge it even though it wasn't valid yet. (It had happened before we started closing PRs)

if github didn't have a bug it would all be good.....

Regarding commit messages... +1... lets start having some better way to communicate changes. Currently our format is BZ|JIRA - short message For non bug-fixes (small config changes.. etc).. we just have a short message... I like the way Netty does their commit messages Title Motivation: Why you're making the change Modifications: what was changed Result: The output... Even though that probably duplicates JIRA... it gives some context when looking at the github logs. github logs gets a lot more useful (mainly for me when I'm talking to support guys.. etc).

I'm open for discussion on the commit messages.. if you guys think it's too verbose.. we can use that

On Sep 23, 2014, at 10:48 AM, Martyn Taylor <mtaylor(a)redhat.com&gt; wrote: > Hi All, > > So when I first started a few of you asked me to speak up if I noticed > areas where I think we may be able to do things better. So here goes :P > > Just a quick email regarding our work flow for committing changes to > HornetQ and hopefully two areas that will take very little effort to > change but might make our development cycle a little smoother. > > The two things I'm thinking of are the way we are using close PR for > reviews and how we structure our commit messages. > > So... Closing PRs after review: > > We are currently using the Fork and Pull model using GitHub pull > requests. https://help.github.com/articles/closing-a-pull-request. > > One slight difference is that we tend to review pull requests then close > them if any changes need to be made. This has it's advantages in that > it can reduce the amount of open pull requests (which makes it easier > for reviewers to identify what needs reviewing) leaving it to down to > the PR owner to address changes (fix tests, code changes, styling > etc...). Once the changes have been made the PR owner can then reopen > the PR and it will appear again for reviewers to review. > > However there are a couple of downsides to this approach. In the > typical the typical fork/pull model the PR owner (once recieving > feedback) will likely go away and make changes to his/her fork. The > history of the developer fork is not of importance and it is typical for > developers to rebase commits and force push their own fork. However, > once a PR has been closed and the developer has forced pushed to the > fork they opened the PR from, it is no longer able to be reopened. > This kind of makes sense, as closing a PR in the context of the GitHub > fork/pull model means that this PR is no longer relevant and the close > is meant to be permanent (Though GitHub offers a get out clause by > offering a re-open option): > > The only way (at least from what I can gather) to get around this > problem is to open a new PR based on the developers fork/branch (that > has been forced pushed to). However this does result in having numerous > PRs based on the same fork/branch being present in the closed PR list. > This can sometimes cause confusion of which closed PR is the one the > developer needs to comment on or to update. I've done it myself and > went to comment on a PR that was closed, not realising I was actually > commenting on an older version of the PR. > > Also opening a new PR does mean that we lose the discussion history. > Which is often useful, particularly to add context. For larger changes > where there is a fair amount of back and forth (review, change, review, > change) etc... this is really helpful. > > Lastly one other scenario where closing the PR is a cumbersome is when > the branch that PR is based on was not rebased onto master. Again I've > done this myself... pushed up a change to my fork without running `git > rebase hornetq/master`. This can sometimes result in extra commits > showing in the PR. There is a quick fix for this: run rebase and force > push to the fork/branch and the PR will reflect the changes > immediately. However if the PR was closed before fork/branch is > rebased, it is no longer possible to reopen it again (as the force push > was made). > > Due to this I'd like to suggest that we move away from closing PRs when > changes need to be made. Instead we can simply leave the PR open whilst > it is being reworked, GitHub notifications take care of letting us know > when a new PR has been opened, when a PR has been commented on and also > when PR has been updated. So I don't think that leaving PRs open is > going to cause confusion. It is actually the normal way that the GitHub > pull and fork mechanism is meant to work: > https://help.github.com/articles/using-pull-requests. > > What do others think about this? > > Structuring commit messages... > > The other work flow related suggestion I'd like to make is how we > structure our git commit messages. There is a best practice for writing > git commit messages using (50/72) styling, see: > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html. In > short, the idea really is to add a very short summary of the message in > the title, followed by a new line then description. This makes it > easier to identify commits when looking at the commit log in various git > tools. > > An example could be something like this: > > """ > HQ-1343 Added new logger for JMS Bridge > > This commit adds a new logger dedicated to the JMS Bridge. This allows > users to identify log messages that are coming from the bridge aiding > identification of bridge failures. > """ > > Using a git tool like gitk for example to view the commits, I get all > the relevant information on 1 line: > > """ > HQ-1343 Added new logger for JMS Bridge mtaylor(a)redhat.com > 22/09/2014 > """ > > Also, setting up and editor like vim is really easy and will add syntax > highlighting for the 50/72 git commit style. > > Commits in HQ with a large 1 liner title or with a URL as the first > thing are difficult to read. The equivilent view in gitk for messaging > like this shows something like: > > """ > https://bugzilla.redhat.com/show_bug.cgi?id... > mtaylor(a)redhat.com 22/09/2014 > """ > > Which doesn't really offer me any information other than the date and > committer. > > Regardless of whether we decide to use 50/72 style. I'd like to get > your thoughts on using a format something along the lines of this: > > """ > <BZ or JIRA Ref> Short summary > > <URL> > > <Description> > """ > > All the relevant information is on the first line. JIra reference and > summary. Then more detailed description and references are added in a > detailed description below. Useful if we need to find out more > information about the commit. > > > Please let me know what your thoughts are. > > Regards > Martyn > _______________________________________________ > hornetq-dev mailing list > hornetq-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hornetq-dev