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

[Martyn Taylor]

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