[asterisk-dev] Subsystems in commit message summaries: +1

[Shaun Ruffell]

On Tue, Oct 22, 2013 at 07:53:53AM -0500, Matthew Jordan wrote:
> On Mon, Oct 21, 2013 at 8:17 PM, Olle E. Johansson <oej at edvina.net> wrote:

> > I would kindly like to suggest that we add the module name to
> > these reports...
> 

[snip]

> The other option is we follow the convention that commit messages should,
> in their summary, list the affected modules. So the cookie summary should
> be:
> 
> http: Tolerate presence of RFC2965 Cookie2 header by ignoring it
> 
> Some folks do this all the time; some don't. It isn't part of the
> recommended commit message guidelines [2]. The downside of making it part
> of the commit message guidelines is that it eats into the 80 character
> limit for summaries and is (yet another) manual step.
>
> [2] https://wiki.asterisk.org/wiki/display/AST/Commit+Messages

My 2¢...

+1 from me for adding the subsystem at the beginning of the commit
message. Especially if the commit does affect some limited number of
subsystems and is not a general change across the entire project.

I am also of the opinion that prepending the subsystem should save
space since a well written commit summary normally indicates the
affected subsystem. For example, "http: Tolerate presence..." is
more concise than saying "Tolerate presence...in the res_http
module"

It also quickly allows a reader to determine if they even care about
the patch. For example:

  "Fix major leak of sensitive customer data in chan_defrob"
  
  vs

  "chan_defrob: Fix major leak of sensitive customer data"

Since I don't use or load chan_defrob, I don't even need to suffer
the initial panic I felt when reading the beginning of the first
commit message.

Here is another example:

  $ git log -20 --pretty="%s"
  Remove a noisy debug message from bridging code.
  Segfault in LIBEDIT_INTERNAL after tgetstr(), when libncurses5-dev isn't installed
  Fixing r401281; the model name is Channel, with a capital C ........
  Fixed malformed Access-Control-Allow-Methods header. Was causing Safari to barf on POST and DELETE. ........
  Fix IAX2 incoming call address lookups
  Return a channel snapshot when originating using ARI, and subscribe the Stasis application to it.
  res_parking: Remove setting useless flag. ........
  This is just a quick script for dumping swagger-ui into static-http, so that it can be served by the Asterisk web server.
  Resolve some memory leaks due to incorrect for loop / ao2 ref usage.
  Add channel lock protection around translation path setup.
  Tweak ast_bridge_depart() doxygen. ........
  Remove the bit about requiring ast_bridge_depart() to be called before ast_bridge_destroy(). ........
  Clarify in ast_bridge_destroy() about how departable channels must be handled. ........
  Remove Port Restriction When Checking For NAT
  Properly copy/remove the device state cache flag over a masquerade.
  Fix Setting A chan_sip Dialog's SIP_NAT_FORCE_RPORT Flag
  res_parking: Fix bug where reloading immediately wipes new parkpos extensions
  Reduce log level of a non-pubsub error message
  ARI: Fix crash when POST /playback/{id}/control does not have an operation parameter.
  Oops. Leftover /stasis reference ........

When looking above at the last 20 commit summaries from the current
trunk of Asterisk, the three commits which prepend the subsystem are
in my opinion the easiest to determine if they will affect me.  They
are closely followed by the other ones that still mention the
subsystem which I still had to read twice.

For comparison to the last 20 DAHDI commits:

  $ git log -20 --pretty="%s" master
  add a 'location' attribute to sysfs (dahdi_device):
  xpp: ring/mwi settings: add to FXS init script
  xpp: FXS: ring/mwi settings: a sysfs interface
  xpp: refactor FXS ring settings
  xpp: Serialize dahdi registration
  wcte13xp: Workaround rare nmi on modprobe on select systems
  xpp: fix waitfor_xpds race at startup
  build_tools/make_version: Fix typo in build_tools/make_version.
  sysfs: bugfix: shorten too long file names
  wcte13xp: Start the span in unconfigured, instead of red state
  dahdi: Do not set rxbufpolicy when opening dahdi net device.
  wctc4xxp: Ensure the descriptors are zeroed out on start.
  wcte13xp: New driver for digium's te13x product range
  xpp: Don't use create_proc_read_entry()
  dahdi_dynamic_ethmf: Don't use create_proc_read_entry()
  dahdi: Replace create_proc_entry() with proc_create_data()
  xpp: FXO: fix firmware pol-rev detection
  xpp: FXO: add a "squelch_polrev" parameter
  xpp: FXO: common function for POLREV reporting
  oct612x: Fix confusing compile error when kernel source is not present.

My eyes naturally scan down the beginning of each line and I can
(mostly) see if the change is against a driver that I use or am
working with.

Granted, prepending the subsystem duplicates information in the
commit itself. One could always assume that a reader will look in
the patch to find the affected files and leave the subsystem off.
However, I believe the rationale for adding it to the summary is the
same used when deciding to denormalize a database. On a project the
size of Asterisk, more total time will be spent reading the commit
summary than writing it so the process should be optimized for the
reader.

Hopefully in the not-too-far-off future the commit messages can be
part of the commit under review. This way the person committing a
patch doesn't have to draft the summary. The task of drafting the
summary can be assigned to the patch authors so the extra manual
step shouldn't be too onerous.

I also fully accept that I'm probably in the minority.

-- 
Shaun Ruffell
Digium, Inc. | Linux Kernel Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org