Skip to main content

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

I get the feeling you and Dani are saying the same thing in a different way. No need to argue. :-)

Sent from my BlackBerry 10 smartphone.
From: Andrey Loskutov
Sent: Wednesday, November 18, 2015 07:56
To: platform-ui-dev@xxxxxxxxxxx
Reply To: Eclipse Platform UI component developers list.
Subject: Re: [platform-ui-dev] Commit conventions

Hi Dani,
 
> We shouldn't make this too formal. It should make our life easier, both when writing and when reading the commit messages. I suggest we start with the following:
 
What is wrong with formal rules, if they are clear for everyone? It is easier to follow formal rules if you start to contribute as to guess and to learn those unwritten rules by receiving "-1" review comments.
 
> In most cases there should be one bug with one corresponding Gerrit change.
 
What is a "most case"? This opens a room for questions and leaves no answer to "is my case different?". I think separation between functional/non functional and trivial changes is important to mention.
 
>Here the bug number and corresponding summary should be used. If the summary is not correct, then consider to fix the summary in bugzilla.
 
I can't agree with that, bug title is usually not a right commit subject. The bug describes the problem, and the commit describes a solution. This is a fundamental difference, same as the difference between a software repository and a bug tracker. Very seldom we have a case where both problem and solution matching 1:1, usually this is true only for bugs entered as an enhancement or as a plain exception stack trace.
 
Changing the bug title to describe a *solution* does not make sense, because bugs are user-oriented problem reports. Users don't care in most cases if a part service has to be fixed if a history view does not open anymore. Users searching for "cannot open history view" will never find a bug "fixed part service" and will file a duplicate.
 
We should of course update bug titles if they aren't good or matching the actual issue, but again, this bug title change should be done to allow users better find the problem or to better explain the problem (which is usually tightly coupled).
 
The main audience for the bug title is our end user community, and most of them are not Platform UI developers.
The main audience of the git commit history is first and foremost Platform UI team *and of course* interested Eclipse developers but never ever the rest of the Eclipse end user community. We have millions of end users searching for bugs but only few thousands of developers searching changes in the git history. This two user groups are simply too different.
 
The main driver for the original proposal was for me was to stop pulluting git repository with meaningless and unrelated entries and to make our, developers life easier. The main point of the proposal is: "The change summary should be short, clear and concise description about the change". If it matches the bug title it is OK, but not other way around, and definitely not enforce that bug title == commit subject.
 
A requirement to update bug title to match commit message is definitely going in the different direction and btw (very important!) will not work for majority of contributors without extended bugzilla rights. They will never be able to meet this requirement.

> When more changes are needed to fix a bug, then it's fine to adjust the summary in the Gerrit change to match the concrete fix, but it has to start with "Bug xyz - " or "Bug xyz: ".
+1

> Don't tie unrelated changes to a bug (number) just because you are working on that bug. Simply use e.g. "Fixed typo" or "Removed trailing whitespace". Don't include such unrelated changes in the real fix/change.
+1
 
So taking your input into account,I propose 3 versions of commit conventions: simple, formal and "by example". For those with difficulties to follow the simple one we will have the formal one and so on.
 
Simple:
 
In general, each commit with functional code changes requires a bug entry, and a commit with trivial changes does not.
A commit related to an exising bug report has to start with "Bug xyz - " or "Bug xyz: ".
Don't tie unrelated changes to a bug (number) just because you are working on that bug. Simply use e.g. "Fixed typo" or "Removed trailing whitespace". Don't include such unrelated changes in the real fix/change.
The change summary should be short, clear and concise description about the change.
The body of the commit message should be more detailed explanatory text (if necessary), describing the change.
 
Formal and "by example" conventions are same as in my previos mail.
 
Kind regards,
Andrey Loskutov

http://google.com/+AndreyLoskutov
 
 
Gesendet: Mittwoch, 18. November 2015 um 12:04 Uhr
Von: "Daniel Megert" <daniel_megert@xxxxxxxxxx>
An: "Eclipse Platform UI component developers list." <platform-ui-dev@xxxxxxxxxxx>
Betreff: Re: [platform-ui-dev] Commit conventions
We shouldn't make this too formal. It should make our life easier, both when writing and when reading the commit messages. I suggest we start with the following:

- In most cases there should be one bug with one corresponding Gerrit change. Here the bug number and corresponding summary should be used. If the summary is not correct, then consider to fix the summary in bugzilla.
- When more changes are needed to fix a bug, then it's fine to adjust the summary in the Gerrit change to match the concrete fix, but it has to start with "Bug xyz - " or "Bug xyz: ".
- Don't tie unrelated changes to a bug (number) just because you are working on that bug. Simply use e.g. "Fixed typo" or "Removed trailing whitespace". Don't include such unrelated changes in the real fix/change.

Dani



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



Szymon,
 
the problem with bug titles is that they are describing the result of a bug ("Eclipse shows Error dialog" or "NPE in EPartService.processDelayedChanges:17") and this bug title (often written by non technical users or automated error bots) could be *completely* unrelated to the actual commit. Just follow the recent example - "Bug 471782 - [ViewMgmt] History View always blank" - the commit has *absolutely* nothing to do with History View at all. But from now on *every* single time someone searches for the changes in the History View will try to understand those two commits!

The commit subject should describe what the commit changed, not what some user or automated error bot selected for the bug title. We should differentiate between a software repository and a product histories. For nice bug reports related to the product defects we have Bugzilla and other reporting tools. The git history however describes the software changes and intended to be read more often by developers and not end users.

Please also notice, that not all bugs can be fixed with a single commit - just check the Platform UI git log, you will see lot of "same"??? commits fixing the same bug "again and again". It is a pain for any reader, such commit subjects are meaningless! Especially for non trivial changes spawning multiple commits having a bug title as a commit title is a nonsense. And BTW nobody disallows to use bug title in the commit subject - if the bug title describes what is changed we have a perfect fit.

>From usability point of view it makes much more sense to spend a minute and rethink what the commit actually does instead of blindly copy/paste the (mostly unrelated) bug title. The time you think to save by copy/paste will cause major problems for all developers trying to understand which commit could have affected the test failures in the last nightly build if it sees this list (this and the history view bug was the final motivation for me to write the proposal):

Bug 481608 - Remove unused methods in Activators
Bug 481608 - Remove unused methods in Activators
Bug 481608 - Remove unused methods in Activators
Bug 481608 - Remove unused methods in Activators
Bug 481608 - Remove unused methods in Activators
Bug 481608 - Remove unused methods in Activators

For searching for bug "XYZ" is the title the less important part - the bug ID is most important, because it is unique. The proposal includes the bug ID in the commit subject so all the manual/automatic search and match queries should just work (and you have less to type btw).

So if I use your examples, the possible subjects could be:

Bug 456965 - Fix for NPE in EPartService.processDelayedChanges:17
Bug 456865 - Check if part is of suitable type to be processed by EPartService
Bug 465965 - Avoid multiple calls to Workbench.busyClose

Kind regards,
Andrey Loskutov


http://google.com/+AndreyLoskutov
 
 

Gesendet: Mittwoch, 18. November 2015 um 10:16 Uhr
Von: "Szymon Ptaszkiewicz" <szymon.ptaszkiewicz@xxxxxxxxxx>
An: "Eclipse Platform UI component developers list." <platform-ui-dev@xxxxxxxxxxx>
Betreff: 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

Andrey 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



_______________________________________________ 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[https://dev.eclipse.org/mailman/listinfo/platform-ui-dev]
_______________________________________________
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

_______________________________________________ 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