[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [platform-ui-dev] Commit conventions
|
On Wednesday 18 November 2015 20:40 Daniel Megert wrote:
> I agree with this.
>
> Andrey, can you update the wiki according my last reply to your comments?
Done, please check: https://wiki.eclipse.org/Platform_UI/How_to_Contribute#Commit_Message_Conventions
> I suggest we then try this out in Platform UI and if it works for us, we
> will apply it to the whole Eclipse top-level project.
Sounds like a plan :-)
Thanks everyone for comments!
> Dani
>
>
>
> From: sxenos@xxxxxxxxx
> To: Andrey Loskutov <platform-ui-dev@xxxxxxxxxxx>,
> platform-ui-dev@xxxxxxxxxxx
> Date: 18.11.2015 17:38
> Subject: Re: [platform-ui-dev] Commit conventions
> Sent by: platform-ui-dev-bounces@xxxxxxxxxxx
>
>
>
> 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
