Bikeshedding commit message policy - Round 3 - Fight!

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Bikeshedding commit message policy - Round 3 - Fight!

Renato Golin Linaro
Folks,

On review http://reviews.llvm.org/D8197, we're basically down to two
bikeshedding issues:

1. Title tags

Some people use "[CSE] Change blah", others use "CSE: Change blah". I
hadn't put anything regarding tags because not everyone use it and
when they do, it's slightly different. I personally don't think it's a
reason to argue about, so I'm in favour of removing the comment
altogether and let people do what they feel is best for their own
commits.

Anyone feel strongly about this? I vote for removing the paragraph and
let people realise they can do that by looking at the past commit
messages.


2. Attribution

The dev policy (http://llvm.org/docs/DeveloperPolicy.html#attribution-of-changes)
gives an example of how to do it: “patch contributed by J. Random
Hacker!”. I personally took that as a guideline, not a rule, and never
really used the exclamation mark, nor I say "contributed", and that
seems what most people do, too. Since having them differently on both
places will bring people to bike shed, I think we need to stick to
one, or be explicitly vague about it.

People have scripts that rely on that, and since it seems to be
working, I'm pretty sure it *doesn't* rely on the word "contributed"
not it relies on the exclamation mark. I'm strongly in favour of not
requiring any of it.

Anyone feels that strongly about "contributed" or the exclamation mark?

I see three solutions here:

1. We stand by that exact phrase, since it's "kosher". I'm against it.

2. We just say that a line that contains the words "patch", "by",
"<name><punctuation>" will be parsed by attribution. I'm against it,
as this will start another bike shed on the exact regular expression
to use. Not to mention this will be an internationalisation and
abbreviation hell.

3. We say "Patch by Foo Bar." and let people be reasonably creative. I
vote for this one.

cheers,
--renato

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Hal Finkel
----- Original Message -----

> From: "Renato Golin" <[hidden email]>
> To: "LLVM Dev" <[hidden email]>, "Clang Dev" <[hidden email]>
> Sent: Saturday, March 14, 2015 2:57:20 PM
> Subject: [cfe-dev] Bikeshedding commit message policy - Round 3 - Fight!
>
> Folks,
>
> On review http://reviews.llvm.org/D8197, we're basically down to two
> bikeshedding issues:
>
> 1. Title tags
>
> Some people use "[CSE] Change blah", others use "CSE: Change blah". I
> hadn't put anything regarding tags because not everyone use it and
> when they do, it's slightly different. I personally don't think it's
> a
> reason to argue about, so I'm in favour of removing the comment
> altogether and let people do what they feel is best for their own
> commits.
>
> Anyone feel strongly about this? I vote for removing the paragraph
> and
> let people realise they can do that by looking at the past commit
> messages.

I used to use CSE:, but have now switched to using [CSE] because that seems to be the prevailing convention (and is somewhat more visually distinctive). I think it makes sense to codify that convention, but not to require them. Sometimes, there is nothing appropriate to use. Sometimes, the first or second word of the commit message is naturally the same as what the title tag would be, and so including the title tag seems redundant.

>
>
> 2. Attribution
>
> The dev policy
> (http://llvm.org/docs/DeveloperPolicy.html#attribution-of-changes)
> gives an example of how to do it: “patch contributed by J. Random
> Hacker!”. I personally took that as a guideline, not a rule, and
> never
> really used the exclamation mark, nor I say "contributed", and that
> seems what most people do, too. Since having them differently on both
> places will bring people to bike shed, I think we need to stick to
> one, or be explicitly vague about it.
>
> People have scripts that rely on that, and since it seems to be
> working, I'm pretty sure it *doesn't* rely on the word "contributed"
> not it relies on the exclamation mark. I'm strongly in favour of not
> requiring any of it.
>
> Anyone feels that strongly about "contributed" or the exclamation
> mark?
>
> I see three solutions here:
>
> 1. We stand by that exact phrase, since it's "kosher". I'm against
> it.
>
> 2. We just say that a line that contains the words "patch", "by",
> "<name><punctuation>" will be parsed by attribution. I'm against it,
> as this will start another bike shed on the exact regular expression
> to use. Not to mention this will be an internationalisation and
> abbreviation hell.
>
> 3. We say "Patch by Foo Bar." and let people be reasonably creative.
> I
> vote for this one.

I agree. I think the important part is that the name appear in an obvious context. That way, if I search my Inbox for the name, the relevant commit will appear, and it will be obvious why without reading a lot of text.

 -Hal

>
> cheers,
> --renato
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>

--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Renato Golin Linaro
On 15 March 2015 at 15:06, Hal Finkel <[hidden email]> wrote:
> I used to use CSE:, but have now switched to using [CSE] because that seems to be the prevailing convention (and is somewhat more visually distinctive). I think it makes sense to codify that convention, but not to require them. Sometimes, there is nothing appropriate to use. Sometimes, the first or second word of the commit message is naturally the same as what the title tag would be, and so including the title tag seems redundant.

I agree with you that [CSE] is the most visually striking, but I
wonder how much do we want to code when to use them. There will always
be arguments to all sides, and I think this is not a topic important
enough for us to make it official.

See how the exclamation mark became self important on the attribution
and you'll see what I mean. I think common sense will always prevail
if we don't try to push too many standards.

cheers,
--renato

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Hal Finkel
----- Original Message -----

> From: "Renato Golin" <[hidden email]>
> To: "Hal Finkel" <[hidden email]>
> Cc: "LLVM Dev" <[hidden email]>, "Clang Dev" <[hidden email]>
> Sent: Sunday, March 15, 2015 10:48:37 AM
> Subject: Re: [cfe-dev] Bikeshedding commit message policy - Round 3 - Fight!
>
> On 15 March 2015 at 15:06, Hal Finkel <[hidden email]> wrote:
> > I used to use CSE:, but have now switched to using [CSE] because
> > that seems to be the prevailing convention (and is somewhat more
> > visually distinctive). I think it makes sense to codify that
> > convention, but not to require them. Sometimes, there is nothing
> > appropriate to use. Sometimes, the first or second word of the
> > commit message is naturally the same as what the title tag would
> > be, and so including the title tag seems redundant.
>
> I agree with you that [CSE] is the most visually striking, but I
> wonder how much do we want to code when to use them.

I don't want to code when to use them. But it makes sense to say, "If you want to include a title tag, do it like this...".

> There will
> always
> be arguments to all sides, and I think this is not a topic important
> enough for us to make it official.
>
> See how the exclamation mark became self important on the attribution
> and you'll see what I mean. I think common sense will always prevail
> if we don't try to push too many standards.

I think this is different for a number of reasons.

 -Hal

>
> cheers,
> --renato
>

--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Renato Golin Linaro
On 15 March 2015 at 16:31, Hal Finkel <[hidden email]> wrote:
> I don't want to code when to use them. But it makes sense to say, "If you want to include a title tag, do it like this...".

I'm ok with that.

So, do we have consensus?

1. Don't require, but recommend using [] for tags.

2. Don't specify attribution more than just "patch by Foo." and expect
people to be sensible. Current scripts already work with a good
variation anyway.

cheers,
--renato
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Chris Lattner

> On Mar 15, 2015, at 11:49 AM, Renato Golin <[hidden email]> wrote:
>
> On 15 March 2015 at 16:31, Hal Finkel <[hidden email]> wrote:
>> I don't want to code when to use them. But it makes sense to say, "If you want to include a title tag, do it like this...".
>
> I'm ok with that.
>
> So, do we have consensus?
>
> 1. Don't require, but recommend using [] for tags.
>
> 2. Don't specify attribution more than just "patch by Foo." and expect
> people to be sensible. Current scripts already work with a good
> variation anyway.

Can you post the entire revised diff that you want to include?  Is it Diff 21913 on Phab?  If so, LGTM.

-Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Renato Golin Linaro
On 15 March 2015 at 20:22, Chris Lattner <[hidden email]> wrote:
> Can you post the entire revised diff that you want to include?  Is it Diff 21913 on Phab?  If so, LGTM.

Hi Chris,

Here's the final version:

http://reviews.llvm.org/D8197

cheers,
--renato
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Chris Lattner

> On Mar 15, 2015, at 1:33 PM, Renato Golin <[hidden email]> wrote:
>
> On 15 March 2015 at 20:22, Chris Lattner <[hidden email]> wrote:
>> Can you post the entire revised diff that you want to include?  Is it Diff 21913 on Phab?  If so, LGTM.
>
> Hi Chris,
>
> Here's the final version:
>
> http://reviews.llvm.org/D8197

Looks fine to me, here are some suggested wording changes to make it flow better.  I also added a request to include bugzilla number.

-Chris

Commit messages
---------------

Although we don't enforce the format of commit messages, we prefer that
you follow these guidelines to help review, search in logs, email formatting and so on.
These guidelines are very similar to rules used by other open source projects.

Most importantly, the contents of the message should be carefully written to
convey the rationale of the change (without delving too much in detail). It
also should avoid being vague or overly specific. For example, "bits were not
set right" will leave the reviewer wondering about which bits, and why they
weren't right, while "Correctly set overflow bits in TargetInfo" conveys almost
all there is to the change.

Below are some guidelines about the format of the message itself:

* Separate the commit message into title, body and, if you're not the original
  author, a "Patch by" attribution line (see below).

* The title should be concise. Because all commits are emailed to the list with the
  first line as the subject, long titles are frowned upon.  Short titles also look better
  in `git log`.

* When the changes are restricted to a specific part of the code (e.g. a
  back-end or optimization pass), it is customary to add a tag to the
  beginning of the line in square brackets.  For example, "[SCEV] ..."
  or "[OpenMP] ...". This helps email filters and searches for post-commit
  reviews.

* The body, if it exists, should be separated from the title by an empty line.

* The body should be concise, but explanatory, including a complete
  reasoning.  Unless it is required to understand the change, examples,
  code snippets and gory details should be left to bug comments, web
  review or the mailing list.

* If the patch fixes a bug in bugzilla, please include the PR# in the message.

* `Attribution of Changes`_ should be in a separate line, after the end of
  the body, as simple as "Patch by John Doe.". This is how we officially
  handle attribution, and there are automated processes that rely on this
  format.

* Text formatting and spelling should follow the same rules as documentation
  and in-code comments, ex. capitalization, full stop, etc.

For minor violations of these recommendations, the community normally favors
reminding the contributor of this policy over reverting. Minor corrections and
omissions can be handled by sending a reply to the commits mailing list.


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bikeshedding commit message policy - Round 3 - Fight!

Renato Golin Linaro
On 15 March 2015 at 21:07, Chris Lattner <[hidden email]> wrote:
> Looks fine to me, here are some suggested wording changes to make it flow better.  I also added a request to include bugzilla number.

Thanks Chris, committed on r232334.

I've included all your changes, thanks for the help.

cheers,
--renato
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev