9:48 a.m.
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