Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [platform-ui-dev] Commit conventions

Hi Andrey,

Thanks for summarising the discussion. Just one remark from me regarding the example that you gave: please note that in case of a commit related to a bug (which IMHO should be 99% of all commits) the only difference compared to what we are doing right now is that the content of the first line following the "Bug <number> - " header doesn't have to contain the bug summary field from bugzilla. I would definitely prefer to see there the bug summary field rather than information about technical details of the commit and the reason is that for human eyes textual information is a lot more "catchy" than numbers and so it makes it easier to correlate a commit to a certain bug. Consider the following git history (I just made it up it so don't bother looking at the bugs or class/method names):

Bug 456965 - Add if condition before entering the loop in EPartService
Bug 456865 - Check if part is of suitable type to be processed by EPartService
Bug 465965 - Add if condition to avoid multiple calls
Bug 456965 - Enhance the if condition with else clause
Bug 465965 - Change the condition to respect programmatic closing

and compare it with this:

Bug 456965 - NPE in EPartService.processDelayedChanges:17
Bug 456865 - ClassCastException in EPartService.processDelayedChanges:23
Bug 465965 - IllegalArgumentException in Workbench.busyClose:1234
Bug 456965 - NPE in EPartService.processDelayedChanges:17
Bug 465965 - IllegalArgumentException in Workbench.busyClose:1234

In the first example you cannot easily tell which commit is about which bug and you need to parse bug numbers digit by digit and bug by bug in order to find commits for the same bug, for example when trying to backport the complete fix to the maintenance branch. In the second example you see it almost instantly thanks to the bug summary in the first line.

For me, the bug summary is the same as the textual ID of the bug. It also makes it consistent with bugzilla - you can run a search query against git history and against bugzilla with the same phrase (bug summary) and you can expect to receive the same information. This cannot be done if you skip the bug summary in the commit message. Technical details of commits are very useful and help to understand the reason behind the change but IMHO the bug summary should have the precedence as it is more important to identify a bug - you can have a commit with no detailed explanation paragraph but you should not have a commit without the bug summary. Hence, bug summary in the first line makes more sense. From usability perspective, bug summary in the first line is also easier because you can simply copy & paste the full header directly from bugzilla and skip all the rest and still get a meaningful commit message with bug number and all the necessary information (second example above).

It does not mean I give -1 and if majority of committers prefer to avoid the bug summary in the first line, then so be it, but for me it seems that making a rule for not using the bug summary in the first line is a step backwards.

Thanks,
Szymon

Inactive hide details for Andrey Loskutov ---2015-11-18 00:04:12---Hi all, thanks for replying, see my comments to your feedbacAndrey Loskutov ---2015-11-18 00:04:12---Hi all, thanks for replying, see my comments to your feedback below!

From: Andrey Loskutov <loskutov@xxxxxx>
To: platform-ui-dev@xxxxxxxxxxx
Date: 2015-11-18 00:04
Subject: Re: [platform-ui-dev] Commit conventions
Sent by: platform-ui-dev-bounces@xxxxxxxxxxx





Hi all,

thanks for replying, see my comments to your feedback below!
So far we got some valuable feedback and since it was very positive, I would like to summarize and update the proposal.

On Monday 16 November 2015 15:40 Andrey Loskutov wrote:

> The proposal is short: change Platform UI commit guidelines to follow guidelines from EGit/JGit, see [5].
>
> This would imply:
> 1) A commit subject (first line) must not repeat bugzilla bug title but must be a concise change summary.
> 2) Trivial commits do not require to point to any bug, or other way around: commits without associated bug are possible.
> 3) Bug: <number> footer line is used instead of the "<Bug number and title from bugzilla>" subject to allow gerrit connect commits to bugzilla.

On Monday 16 November 2015 16:08 Lars Vogel wrote:

> I think this is not a Platform UI specific issue, it is the convention
> for the whole platform and / or for the whole top-level Eclipse
> project. I think, if we want to simplify that, we need to change the
> rules for the top-level project, as more and more people work
> cross-project.

I'm always trying to be as concrete as I can and get things done if I can. My primary goal of this proposal is Platform UI project.
I think we (Platform UI as a team) can decide for ourselves which commit conventions we would like to have and we do not depend on PMC decision here.

> Dani as project lead for the Eclipse platform and Eclipse top-level
> project would need to decide that. Or bring it to the Eclipse
> top-level PMC.

But of course it would be helpful for other teams if the Dani or you could bring the updated proposal to the PMC as a recommendation for other projects with old or outdated guidelines.

> A weak +1 from my side for this simplification.

On Tuesday 17 November 2015 15:08 Stefan Xenos wrote:

> I really like having the bug number in the subject, since the bug has the
> most useful context for the change. I find I use the bug number more than
> anything else in the commit comment, and the most important information
> should be right up front. So -1 for removing the bug number from the
> subject.
>
> However, changing the subjects from being a copy of the bug description to
> being a description of the change makes complete sense to me.
>
> If we're going to say that bug descriptions are not mandatory, we should
> have clear guidance as to when they are or aren't required. I'd propose
> that if the patch contains no functional changes (it only changes
> whitespace and comments) no bug should be necessary. If it contains any
> functional changes a bug should be necessary. That would let us update
> copyright headers, reformat files, remove trailing whitespace, etc. without
> a bug number... but would mean a bug would always be present if there's
> ever anything to discuss about the change.

I count this as +1 and a basis for updated proposal (follows).

On Tuesday 17 November 2015 11:11 Paul Webster wrote:

> +1.  I find the whole bug description in the subject useful, but as long as
> the bug prefix was still there, "Bug 37664" that would be good enough for
> me.

I count this as +1 for updated proposal.

On Tuesday 17 November 2015 17:15 Daniel Megert wrote:

> +1, unless that additional change is big enough to deserve a separate bug
> report.

I count this as +1 for updated proposal.

So we have 4 answers with 3 clear +1 for Stefan's updated proposal and a "weak" one for original proposal.
Also count +1 from me to the updated proposal and we have +4.5 for the change.

I would like to provide here an updated formal proposal which we could put in the wiki:

1) In general, each commit with functional code changes requires a bug entry. Non functional commits changing lot of code also require a bug entry. A bug must always be present if there's ever anything to discuss about the change.
2) If the commit contains no functional changes or the change is fairly small and trivial it does not require a bug. Example: converting an old style "for" loop to modern "foreach" loop, white space changes, formatting, comments in one or few files.
3) A commit subject (first line) must start with the "Bug <bug number> - " prefix followed by a change summary. This prefix is not needed for trivial commits not requiring a bug entry.
4) The change summary should be short, clear and concise description about the change, should start with capital letter and should not end with a dot. A blank line separates it from the body of the message.
5) The body of the commit message should be more detailed explanatory text (if necessary), describing the change.
6) The footer of the commit is separated with a blank line from the message and contain "Change-Id:", "Signed-off-by:" and optional "CQ:" fields:
- A Gerrit "Change-Id" is required for all changes pushed to Gerrit (to enable pushing new patchsets for the same change), it should be added in the format shown below. Use the Gerrit commit message hook or EGit to add the Change-Id.
- A "Signed-off-by" can be added at the end of the commit message, the email must be the one used to sign CLA (see example below).
- If a Contribution Questionnaire has been issued to initiate and track the review of contributed changes by the Eclipse Foundation's IP team the IPZilla bug number should be added as "CQ:" footer in the format shown below.

Example commit header for a functional change:

#############
Bug XXXXXX - Functional change in service XYZ

More detailed explanatory text (if necessary), describing the change, the direction of the fix etc.

Change-Id: I0000000000000000000000000000000000000000
Signed-off-by: Your Name <your.email@xxxxxxxxxxxx>
#############

Example commit for a non-functional functional change:

#############
Non functional white space change in service XYZ

<No detailed explanatory text is required for a trivial change>

Change-Id: I0000000000000000000000000000000000000000
Signed-off-by: Your Name <your.email@xxxxxxxxxxxx>
#############

Example commit header for a change which also required a Contribution Questionnaire:

#############
Bug XXXXXX - Huge functional contribution to service XYZ

More detailed explanatory text (mandatory for such a big change), describing the change, the direction of the fix etc.

CQ: XXXX
Change-Id: I0000000000000000000000000000000000000000
Signed-off-by: Your Name <your.email@xxxxxxxxxxxx>
#############

Thanks for contributing to the discussion!

--
Kind regards,
google.com/+AndreyLoskutov
_______________________________________________
platform-ui-dev mailing list
platform-ui-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/platform-ui-dev




Back to the top