[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
[ecf-dev] Git commit messages

Firstly, good work on being part of Indigo, which ships later today!

I wanted to raise a general comment regarding the commit messages for ECF. There's a de-facto Git comment style, as I wrote about in my second Git Tip of the Week series, which references the discussion in git-commit:


Basically, the convention is to have a single short line of text (less than 50 chars) which summarises the change, followed by a blank line, and then more text separated by blank lines (if there's more than one paragraph). The final section is for Key: Value pairs, which can include Signed-off-by and Change-Id as well as custom values, like Bug: 12345.

Many of the changes in the current ECF log are one-liners, and often just reference a bugzilla item. This doesn't help someone who has cloned the repository knowing why the changes were made. Consider these two changes below (both for the same fix, bug 349078).

Being able to do 'git log --oneline' and see descriptive changes is a lot more useful than being able to see a list of referenced bugs, as well as giving someone who has cloned (and may not be connected to the internet) the repository.

I'm mentioning this to hopefully encourage a better style of commit messages in Eclipse generally, but in ECF specifically. Please don't take this as a 'you must do this' but rather as a starting point for further discussion.


--- 8< ---

https://github.com/eclipse/ecf/commit/e41dc1510e9e9a05e52ac9d284d129cce1d11646 (Scott's change)

Improved fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=349078

https://github.com/alblue/ecf/commit/531ef894bc3467391922fdac2432d1ea83c42986 (My change)

Export remote services last, to minimise racing

When we register as an EventHook, we may receive
duplicate services started at the same time. However,
the first thing the register call does is get a list
of services (which may be a long list) so the race
is between

* Register as EventHook
* Get list of existing services

If we list the existing services first and register
them, this may take some time (since exporting the
endpoints may cause further listeners to fire).

On the other hand, registering as an EventHook is fairly
quick, and the next call will be to get services, so
there is a smaller chance of a race condition this way

Bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=349078