[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [platform-ui-dev] Commit conventions
|
Hi Andrey,
> 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)
This is true and I think this is a good thing because it allows users to find things they are looking for or they are bothered by. The difference between bug and commit is that bug summary is editable while the commit message is not. If you feel that bug summary is not correct or can be improved then this is the thing that should be done before pushing a commit, IMHO.
> could be *completely* unrelated to the actual commit.
Well, if it fixed the bug, then it is related.
> 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!
If the problems happens only in case of the History View then it is related to the History View so it is fine if the commit message mentions it. But if it turned out that the problem is not History View-specific but only manifests itself in case of the History View and it requires some change in Platform UI repo, then the commit message should probably be updated accordingly to describe the more general problem that could happen in case of any view but was observed only for the History View only. Example: "Bug 471782 - [ViewMgmt] A 3.x view can be blank when opened in 4.x based RCP" (I wasn't following that bug so not sure if this is really technically appropriate, just wanted to give an example of a more general bug summary for this particular bug).
> 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 remember that in Eclipse our "product" is not only the IDE but also the source code that can be then reused by end users who are developers but outside eclipse.org.
> 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.
Yes, I fully agree and I also hate those repeating commit messages but I don't think it is a problem with our existing commit message rules but rather with the lack of attention paid by the committer who pushed such commits. Nobody said that all those commits have to be titled in the same way, that was just a preference (read: laziness) of the one who pushed it and I agree it was a wrong decision to do it.
> 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.
Please note that we are using bugzilla not fixzilla so in most cases we see bug description rather than fix suggestions.
> 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):
I fully agree regarding rethinking, but the "rethink" part should be done at the bug to reflect on what the bug is actually about before pushing the commit rather leaving the bug with meaningless title and equally meaningless commit message or a different commit message. If it is not done that way, then it is the fault of the committer who tries to blindly follow the existing rules rather than to use them in a meaningful way. Example:
Bug 481608 - Remove unused methods in Activators
Bug 481608 - Remove unused methods in Activators
Bug 481608 - Remove unused methods in Activators
could be replaced with
Bug 481608 - Remove unused methods in org.eclipse.my.plugin.Activator
Bug 481609 - Remove unused methods in org.eclipse.your.plugin.Activator
Bug 481610 - Remove unused methods in org.eclipse.his.plugin.Activator
So to summarise, you haven't convinced me so my vote is still 0 with preference to keep using meaningful bug summaries as the first line of the commit message.
Szymon
"Andrey Loskutov" ---2015-11-18 11:17:47---Szymon,
From: "Andrey Loskutov" <loskutov@xxxxxx>
To: platform-ui-dev@xxxxxxxxxxx
Date: 2015-11-18 11:17
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
