openstack team mailing list archive

[Monty Taylor <mordred@xxxxxxxxxxxx>]

Hey! I agree with most of this in general. A few comments about a
section I just happened to read:

On 06/27/2012 06:52 AM, Daniel P. Berrange wrote:

snip

> Including external references
> -----------------------------
> 
> The commit message is primarily targetted towards human interpretation,
> but there is always some metadata provided for machine use. In the case
> of OpenStack this includes at least the 'Change-id', but also optional
> "bug" ID references and "blueprint" name references. Although GIT records
> the author & committer of a patch, it is common practice across many
> open source projects to include a "Signed-off-by" tag. Though OpenStack
> does not mandate its use, the latter is still useful to include if a patch
> is a combination of work by many different developers, since GIT only
> records a single author. All machine targetted metadata, however, is
> of secondary consequence to humans and thus it should all be grouped
> together at the end of the commit message. For example:
> 
> 
>     Switch libvirt get_cpu_info method over to use config APIs
> 
>     The get_cpu_info method in the libvirt driver currently uses
>     XPath queries to extract information from the capabilities
>     XML document. Switch this over to use the new config class
>     LibvirtConfigCaps. Also provide a test case to validate
>     the data being returned
> 
>     Fixes: bug #1003373
>     Implements: blueprint libvirt-xml-cpu-model

These two are fine in general - although we actually parse and do bug
and blueprint linking a bit more permissively. I believe our current
regexes should still apply to those though.

>     Change-Id: I4946a16d27f712ae2adf8441ce78e6c0bb0bb657
>     Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> 
> As well as the 'Signed-off-by' tag, there are various other ad-hoc
> tags that can be used to credit other people involved in a patch
> who aren't the author.
> 
>  - 'Reviewed-by: ...some name.. <...email...>'
> 
>    Although Gerrit tracks formal review by project members, some
>    patches have been reviewed by people outside the community
>    prior to submission

I'm not in support of this one. Our review system has no restrictions on
who can review things - and I value the actual content of their reviews,
which itself is stored in git notes along with the change. So, in
general, you can tell me that someone else reviewed it somewhere else,
but I don't really care. They should review it in the same place
everyone else does.

>  - 'Suggested-by: ...some name.. <...email...>'
> 
>    If a person other than the patch author suggested the code
>    enhancement / influnced the design

Great idea.

>  - 'Reported-by:  ...some name.. <...email...>'
> 
>    If a person reported the bug / problem being fixed but did
>    not otherwise file a launchpad bug report.

I think this should be strongly discouraged. If there is a bug or
problem, it should be filed as a bug report. I don't really know of a
valid use case where someone can't do this, but where referencing them
here is valid. We have systems, and just as you're suggesting overall
that we be a bit stricter in how we do things, I think that being more
lax in how we record some of this metadata is counter productive.


However - strongly in favor of better commit message practice!

Monty