Hey,
I have observed a difference in opinion between two groups of people on gerrit, which unfortunately is causing bad blood on both sides. I'm therefore interested in hearing your opinion about the following scenario:
Someone makes a sound commit. The commit has a clear commit message, though there is a single typo in it. Is it helpful to -1 the commit because of the typo?
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On 15/01/13 12:44, Jeroen De Dauw wrote:
I have observed a difference in opinion between two groups of people on gerrit, which unfortunately is causing bad blood on both sides. I'm therefore interested in hearing your opinion about the following scenario:
Someone makes a sound commit. The commit has a clear commit message, though there is a single typo in it. Is it helpful to -1 the commit because of the typo?
This intricate, complex and important question deserves a long and thoughtful answer.
In my opinion, if the typo is trivial (f.e. someone typed "fo" instead of "of"), there is no need to -1 the commit, however if the typo pertains to a crucial element of the commit (f.e. someone typed "fixed wkidata bug") perhaps it should, since otherwise people who search through commit messages won't be able to find commits that contain word "wikidata".
On 15.01.2013 12:58, Nikola Smolenski wrote:
In my opinion, if the typo is trivial (f.e. someone typed "fo" instead of "of"), there is no need to -1 the commit, however if the typo pertains to a crucial element of the commit (f.e. someone typed "fixed wkidata bug") perhaps it should, since otherwise people who search through commit messages won't be able to find commits that contain word "wikidata".
Ok, full text search might be an argument in some cases (does that even work on gerrit?).
But in that regard, wouldn't it be much more important to enforce (bug 12345) links to bugzilla by giving a -1 to commits that don't have them (though they clearly have, or should have, a bug report?)
I'm still in favor of requiring every tag line to contain either (bug nnnnn) or (minor), so people are reminded that bugs should be filed and linked for anything that is not trivial. That's not what I want to discuss here - it just strikes me as much more relevant than typos, yet people don't seem to be too keen to enforce that.
-- daniel
Daniel Kinzler daniel@brightbyte.de wrote:
In my opinion, if the typo is trivial (f.e. someone typed "fo" instead of "of"), there is no need to -1 the commit, however if the typo pertains to a crucial element of the commit (f.e. someone typed "fixed wkidata bug") perhaps it should, since otherwise people who search through commit messages won't be able to find commits that contain word "wikidata".
Ok, full text search might be an argument in some cases (does that even work on gerrit?).
It works in Git, for example with "git log --grep". I do think that fixing typos should be preferred to preserving typos for eternity, and -1 is better than nothing, but up- loading a new changeset is how it should be done.
But in that regard, wouldn't it be much more important to enforce (bug 12345) links to bugzilla by giving a -1 to commits that don't have them (though they clearly have, or should have, a bug report?)
I'm still in favor of requiring every tag line to contain either (bug nnnnn) or (minor), so people are reminded that bugs should be filed and linked for anything that is not trivial. That's not what I want to discuss here - it just strikes me as much more relevant than typos, yet people don't seem to be too keen to enforce that.
I cannot follow that line of thought at all. The "tag line" is rather short and should give the reader a summary of what the commit is about when browsing through a list of commits. Reserving part of that for a bug number would only be useful if the reader could associate the underlying issue by the number, but apart from bug #1 and a few (other) tracking bugs noone will be able to do so, and those bugs will (un- fortunately) never be closed.
Is there another software project that uses the summary line in a similar way to MediaWiki?
Tim
On Tue, Jan 15, 2013 at 11:44 AM, Tim Landscheidt tim@tim-landscheidt.de wrote:
Daniel Kinzler daniel@brightbyte.de wrote:
In my opinion, if the typo is trivial (f.e. someone typed "fo" instead of "of"), there is no need to -1 the commit, however if the typo pertains to a crucial element of the commit (f.e. someone typed "fixed wkidata bug") perhaps it should, since otherwise people who search through commit messages won't be able to find commits that contain word "wikidata".
Ok, full text search might be an argument in some cases (does that even work on gerrit?).
It works in Git, for example with "git log --grep". I do think that fixing typos should be preferred to preserving typos for eternity, and -1 is better than nothing, but up- loading a new changeset is how it should be done.
But in that regard, wouldn't it be much more important to enforce (bug 12345) links to bugzilla by giving a -1 to commits that don't have them (though they clearly have, or should have, a bug report?)
I'm still in favor of requiring every tag line to contain either (bug nnnnn) or (minor), so people are reminded that bugs should be filed and linked for anything that is not trivial. That's not what I want to discuss here - it just strikes me as much more relevant than typos, yet people don't seem to be too keen to enforce that.
I cannot follow that line of thought at all. The "tag line" is rather short and should give the reader a summary of what the commit is about when browsing through a list of commits. Reserving part of that for a bug number would only be useful if the reader could associate the underlying issue by the number, but apart from bug #1 and a few (other) tracking bugs noone will be able to do so, and those bugs will (un- fortunately) never be closed.
Is there another software project that uses the summary line in a similar way to MediaWiki?
Really, we should be using them in the footer so gerrit can track them.
Eg:
=== Some awesome new feature
Blah blah blah I did stuff Fixed this too.
Bug: 1234 Change-Id: I.... ===
-Chad
(anonymous) wrote:
[...] Really, we should be using them in the footer so gerrit can track them.
Eg:
=== Some awesome new feature
Blah blah blah I did stuff Fixed this too.
Bug: 1234 Change-Id: I.... ===
This should be "Fixes-Bug:" or something similar (I'll leave it to the native speakers whether "Bug-Fix:", "Fixed-Bug:", or something else is best at corresponding to "Change-Id:").
Tim
I would advise against doing anything with the word "Fix" in it, because a commit does not necessarily fix a bug. It's possible that a fix for a bug spans multiple commits, depending on the scope of the bug. When you do just "Bug", all it implies is that you can see that bug report for related information and discussion on the commit.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Jan 23, 2013 at 3:57 AM, Tim Landscheidt tim@tim-landscheidt.dewrote:
(anonymous) wrote:
[...] Really, we should be using them in the footer so gerrit can track them.
Eg:
=== Some awesome new feature
Blah blah blah I did stuff Fixed this too.
Bug: 1234 Change-Id: I.... ===
This should be "Fixes-Bug:" or something similar (I'll leave it to the native speakers whether "Bug-Fix:", "Fixed-Bug:", or something else is best at corresponding to "Change-Id:").
Tim
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Tyler Romeo tylerromeo@gmail.com wrote:
I would advise against doing anything with the word "Fix" in it, because a commit does not necessarily fix a bug. It's possible that a fix for a bug spans multiple commits, depending on the scope of the bug. When you do just "Bug", all it implies is that you can see that bug report for related information and discussion on the commit.
[...]
Then why would you use "Fixes: 123" if the commit doesn't actually fix bug #123?
Tim
That's my point. You wouldn't use "Fixes: 123" if it doesn't actually fix bug 123. However, what if there's a case like I said, where a bug is fixed across multiple commits. If you're using the "Fixes" tag, then technically you should only be tagging the last commit that finally fixes the bug, in which case all the other commits are left unmarked and are lost in the repository. With just a "Bug" tag, it indicates that the commit is related to the bug.
My reasoning has to do with the motivation behind why we tag commits. Maybe I'm wrong, but the reason we tag commits with bug numbers is so that, in the future, if one wants to find the commit(s) that fixed a certain bug, they can do a quick grep search on the commit log and find the relevant commits.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Jan 23, 2013 at 1:04 PM, Tim Landscheidt tim@tim-landscheidt.dewrote:
Tyler Romeo tylerromeo@gmail.com wrote:
I would advise against doing anything with the word "Fix" in it, because
a
commit does not necessarily fix a bug. It's possible that a fix for a bug spans multiple commits, depending on the scope of the bug. When you do
just
"Bug", all it implies is that you can see that bug report for related information and discussion on the commit.
[...]
Then why would you use "Fixes: 123" if the commit doesn't actually fix bug #123?
Tim
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, Jan 23, 2013 at 1:17 PM, Tyler Romeo tylerromeo@gmail.com wrote:
My reasoning has to do with the motivation behind why we tag commits. Maybe I'm wrong, but the reason we tag commits with bug numbers is so that, in the future, if one wants to find the commit(s) that fixed a certain bug, they can do a quick grep search on the commit log and find the relevant commits.
It's even easier than that:
https://gerrit.wikimedia.org/r/#/q/bug:43523,n,z
Gerrit indexes these values, so you can search for bug:123 or rt:456.
-Chad
Tyler Romeo tylerromeo@gmail.com wrote:
That's my point. You wouldn't use "Fixes: 123" if it doesn't actually fix bug 123. However, what if there's a case like I said, where a bug is fixed across multiple commits. If you're using the "Fixes" tag, then technically you should only be tagging the last commit that finally fixes the bug, in which case all the other commits are left unmarked and are lost in the repository. With just a "Bug" tag, it indicates that the commit is related to the bug.
Then use "Relates-To:" for the relationships, and "Fixes:" for the fixes.
My reasoning has to do with the motivation behind why we tag commits. Maybe I'm wrong, but the reason we tag commits with bug numbers is so that, in the future, if one wants to find the commit(s) that fixed a certain bug, they can do a quick grep search on the commit log and find the relevant commits.
[...]
I don't agree with that. There are lots of bugs that get fixed unknowingly because most of the 5034 bugs are not on the developers' radar. So any (consistent) information on which commit has fixed a bug must be kept in Bugzilla as we can't change Git's history, and it's hard to imagine a case where you want to know which commit(s) fixed bug x without looking at Bugzilla's page on bug x.
IMHO the primary motivation for using "Fixes: 123" (or "Relates-To: 123") is to absolve the committer from te- diously going back to the Bugzilla page and adding a Gerrit link and (in the former case) the merger from marking the bug as resolved as computers are so *much* better at that (and cheaper).
Tim
IMHO the primary motivation for using "Fixes: 123" (or "Relates-To: 123") is to absolve the committer from te- diously going back to the Bugzilla page and adding a Gerrit link and (in the former case) the merger from marking the bug as resolved as computers are so *much* better at that (and cheaper).
Can't we just use Gerrit's builtin topics for that?
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Jan 23, 2013 at 2:28 PM, Tim Landscheidt tim@tim-landscheidt.dewrote:
Tyler Romeo tylerromeo@gmail.com wrote:
That's my point. You wouldn't use "Fixes: 123" if it doesn't actually fix bug 123. However, what if there's a case like I said, where a bug is
fixed
across multiple commits. If you're using the "Fixes" tag, then
technically
you should only be tagging the last commit that finally fixes the bug, in which case all the other commits are left unmarked and are lost in the repository. With just a "Bug" tag, it indicates that the commit is
related
to the bug.
Then use "Relates-To:" for the relationships, and "Fixes:" for the fixes.
My reasoning has to do with the motivation behind why we tag commits.
Maybe
I'm wrong, but the reason we tag commits with bug numbers is so that, in the future, if one wants to find the commit(s) that fixed a certain bug, they can do a quick grep search on the commit log and find the relevant commits.
[...]
I don't agree with that. There are lots of bugs that get fixed unknowingly because most of the 5034 bugs are not on the developers' radar. So any (consistent) information on which commit has fixed a bug must be kept in Bugzilla as we can't change Git's history, and it's hard to imagine a case where you want to know which commit(s) fixed bug x without looking at Bugzilla's page on bug x.
IMHO the primary motivation for using "Fixes: 123" (or "Relates-To: 123") is to absolve the committer from te- diously going back to the Bugzilla page and adding a Gerrit link and (in the former case) the merger from marking the bug as resolved as computers are so *much* better at that (and cheaper).
Tim
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Tyler Romeo tylerromeo@gmail.com wrote:
IMHO the primary motivation for using "Fixes: 123" (or "Relates-To: 123") is to absolve the committer from te- diously going back to the Bugzilla page and adding a Gerrit link and (in the former case) the merger from marking the bug as resolved as computers are so *much* better at that (and cheaper).
Can't we just use Gerrit's builtin topics for that?
That would actually be a very cool solution. The downside would be that the bug wouldn't be mentioned in the commit message without explicit action by the author as the topic branch is lost on merge (CMIIW), and an author would have to remember when he decides to fix a bug not in one go, but with intermediate steps, to rename the branch before submit- ting. Also, we would be limited to this one application. Other projects seem to expand the footer syntax further, cf. http://wiki.openstack.org/GerritWorkflow#Committing_Changes:
| Adds keystone support
| ...Long multiline description of the change...
| Implements: blueprint authentication | Fixes: bug #123456 | Change-Id: I4946a16d27f712ae2adf8441ce78e6c0bb0bb657
Tim
On 01/15/2013 06:58 AM, Nikola Smolenski wrote:
In my opinion, if the typo is trivial (f.e. someone typed "fo" instead of "of"), there is no need to -1 the commit, however if the typo pertains to a crucial element of the commit (f.e. someone typed "fixed wkidata bug") perhaps it should, since otherwise people who search through commit messages won't be able to find commits that contain word "wikidata".
I agree. Well said. A minor mistake like that that doesn't affect grepping isn't worth a -1, since it doesn't affect the readability of the code itself.
Matt Flaschen
Japanese RPG games does something interesting. When you get a quest like "Go to the house of MrTom and pick the toothnail" the words MrTom and toothnail are bolded. Something in the system (maybe is done manually wen the quest text is written) acknowledge entities in the system ( NPC characters, locations, items) and bold it, so is easier for the player to tell the important parts of the quest without really reading it full trough it.
Computer geeks use to do this using the * character. "fixed wikidata bug by reversing the *polarity*". This type of ascii syntax is what started the wikisyntax.
It could be interesting (but I have no idea if is feasible), if git recognize automatically elements in a commit text, and colorize it on the terminal screen (or maybe bold it if the screen renders using truetype fonts). This way, if you have written wikidata many times, you will quickly spot a problem if the commit renders to you with "fixed wkidata bug by reversing the polarity" and wikidata is not bolded/colored different. A alternate would be for this script/program, to extract keywords and present to you, so if you notice the commit lack the label "wikidata", theres something wrong.
Many times people don't read what are writing, only when are given back what have write notice that theres something wrong in it. Many Internet forums recognize this by allowing people to edit / fix post in the first 5 minutes. Anyway half the battle is making so people care about this,so put a bit of effort into writing without many spell errors. Perhaps is harder if you have people from different cultures, and you have developers that are not english natives.
If people not care, will always make this type of mistakes, and if the mistakes change keywords like wikidata or function names, it may make commits harder to search for keywords. It is true this thing is so minor its not even worth a thread on the mail list. I am posting this here, because is a fun problem.
--- The Swedish Chef
----- Original Message -----
From: "." oscar.vives@gmail.com
It could be interesting (but I have no idea if is feasible), if git recognize automatically elements in a commit text, and colorize it on the terminal screen (or maybe bold it if the screen renders using truetype fonts). This way, if you have written wikidata many times, you will quickly spot a problem if the commit renders to you with "fixed wkidata bug by reversing the polarity" and wikidata is not bolded/colored different. A alternate would be for this script/program, to extract keywords and present to you, so if you notice the commit lack the label "wikidata", theres something wrong.
Talk to the people over on the derivative wikitext list, spun off to contain discussions relevant to creating a replacement MWText parser (there are, I think 5 or 6 projects in varying degrees of activity; though the list itself is pretty quiet).
Cheers, -- jra
On 15.01.2013 12:44, Jeroen De Dauw wrote:
Hey,
I have observed a difference in opinion between two groups of people on gerrit, which unfortunately is causing bad blood on both sides. I'm therefore interested in hearing your opinion about the following scenario:
Someone makes a sound commit. The commit has a clear commit message, though there is a single typo in it. Is it helpful to -1 the commit because of the typo?
Yes, I have noticed the same.
My very personal opinion:
No, a -1 is not justified because of a typo in a commit message. Doing that just causes a lot of overhead for extremely little benefit. If someone is really bothered by it, they can fix it themselves.
It's like reverting a Wikipedia edit because of a type. You don't do that. You fix it or leave it.
The only semi-valid argument I have heard in support is that commit messages (may) go into the release notes. But release notes are edited, formatted and spell-checked anyway, and they don't include all commit messages. Not even all tag lines.
Personally, if I do a quick fix of a bug I find somewhere, and the fix gets a -1 for a typo in the commit message, I'm tempted to just walk away and let it rot. I'm immature like that I guess... and I'm pretty sure I'm not the only one.
-- daniel
PS: note that this is about typos. A commit with an incomprehensible or plain wrong commit message should indeed get a -1.
On Tue, Jan 15, 2013 at 6:44 AM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
I have observed a difference in opinion between two groups of people on gerrit, which unfortunately is causing bad blood on both sides. I'm therefore interested in hearing your opinion about the following scenario:
Someone makes a sound commit. The commit has a clear commit message, though there is a single typo in it. Is it helpful to -1 the commit because of the typo?
This is a non issue in the very near future. Once we upgrade (testing now, planning for *Very Soon* after eqiad migration), we'll have the ability to edit commit messages and topics directly from the UI. I think this will save people a lot of time downloading/amending changes just to fix typos.
Personally, I think all typos should be fixed, since this becomes an immutable part of the history once submitted. But hopefully this will be much easier for people soon :)
-Chad
On 15.01.2013 13:39, Chad wrote:
This is a non issue in the very near future. Once we upgrade (testing now, planning for *Very Soon* after eqiad migration), we'll have the ability to edit commit messages and topics directly from the UI. I think this will save people a lot of time downloading/amending changes just to fix typos.
Oh yes, please!
-- daniel
Its not that difficult to read through the text before you commit, right? At least try to remove the most obvious spelling errors.
Perhaps I just find to much BS I should have given a -1 or even a -2 in some cases.
John
Le 15/01/13 12:44, Jeroen De Dauw wrote:
I have observed a difference in opinion between two groups of people on gerrit, which unfortunately is causing bad blood on both sides. I'm therefore interested in hearing your opinion about the following scenario:
Someone makes a sound commit. The commit has a clear commit message, though there is a single typo in it. Is it helpful to -1 the commit because of the typo?
Yup -1 anything that is wrong, even if it is the must trivial error ever encountered. We have a pre commit review workflow exactly for that.
If you feel brave enough, you could edit it yourself: - download the change - amend it - sent patchset back - leave a message (fixed typo) - removed yourself from the reviewer list Done :-]
I wanted to have a spell checker to lint the commit message, but eventually gave up because of the number of false positives.
I agree with Antoine. Commit messages are part of the permanent history of this project. From now until MediaWiki doesn't exist anymore, anybody can come and look at the change history and the commit messages that go with them. Now you might ask what the possibility is of somebody ever coming across a single commit message that has a typo in it, but when you're using git-blame, git-bisect, or other similar tools, it's very possible.
Also, a -1 is not a -2. It won't block the change from being merged; it's just a notification that something needs to be fixed, however minor.
I'm still in favor of requiring every tag line to contain either (bug
nnnnn) or (minor), so people are reminded that bugs should be filed and linked for anything that is not trivial. That's not what I want to discuss here - it just strikes me as much more relevant than typos, yet people don't seem to be too keen to enforce that.
I'm not so sure about *every* commit, but I definitely agree that this needs to be enforced more. If you're fixing something or adding a new feature, there should be a bug to go with it.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Tue, Jan 15, 2013 at 8:37 AM, Antoine Musso hashar+wmf@free.fr wrote:
Le 15/01/13 12:44, Jeroen De Dauw wrote:
I have observed a difference in opinion between two groups of people on gerrit, which unfortunately is causing bad blood on both sides. I'm therefore interested in hearing your opinion about the following
scenario:
Someone makes a sound commit. The commit has a clear commit message,
though
there is a single typo in it. Is it helpful to -1 the commit because of
the
typo?
Yup -1 anything that is wrong, even if it is the must trivial error ever encountered. We have a pre commit review workflow exactly for that.
If you feel brave enough, you could edit it yourself:
- download the change
- amend it
- sent patchset back
- leave a message (fixed typo)
- removed yourself from the reviewer list
Done :-]
I wanted to have a spell checker to lint the commit message, but eventually gave up because of the number of false positives.
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 15.01.2013 15:06, Tyler Romeo wrote:
I agree with Antoine. Commit messages are part of the permanent history of this project. From now until MediaWiki doesn't exist anymore, anybody can come and look at the change history and the commit messages that go with them. Now you might ask what the possibility is of somebody ever coming across a single commit message that has a typo in it, but when you're using git-blame, git-bisect, or other similar tools, it's very possible.
And then they see a typo. So what? If you look through a mailing list archive or Wikipedia edit comments, you will also see typos.
I'm much more concerned about scaring away new contributors with such nitpicking.
I'm not so sure about *every* commit, but I definitely agree that this needs to be enforced more. If you're fixing something or adding a new feature, there should be a bug to go with it.
Every commit that is not trivial. This would be so much nicer if we had good integration between bug tracker and review system :/
-- daniel
-- - Brian Caution: The mass of this product contains the energy equivalent of 85 million tons of TNT per net ounce of weight.
On Tue, Jan 15, 2013 at 10:57 AM, Daniel Kinzler daniel@brightbyte.de wrote:
On 15.01.2013 15:06, Tyler Romeo wrote:
I agree with Antoine. Commit messages are part of the permanent history of this project. From now until MediaWiki doesn't exist anymore, anybody can come and look at the change history and the commit messages that go with them. Now you might ask what the possibility is of somebody ever coming across a single commit message that has a typo in it, but when you're using git-blame, git-bisect, or other similar tools, it's very possible.
And then they see a typo. So what? If you look through a mailing list archive or Wikipedia edit comments, you will also see typos.
I'm much more concerned about scaring away new contributors with such nitpicking.
On the other hand, new users may be attracted to the fact that we have high standards.
I agree that spelling is a valid reason for a -1. After all, -1 is not the same as a revert in the svn days, it simply means that the commit is not yet "perfect". (Even in svn a revert was supposed to be no big deal).
-bawolff
On Tue, Jan 15, 2013 at 10:05 AM, bawolff bawolff+wn@gmail.com wrote:
On the other hand, new users may be attracted to the fact that we have high standards.
I agree that spelling is a valid reason for a -1. After all, -1 is not the same as a revert in the svn days, it simply means that the commit is not yet "perfect". (Even in svn a revert was supposed to be no big deal).
I would disagree, I think most developers will look for community first. While yes, some wont care that it's full of nitpicky pedants, I think that would turn off more potential developers then would help. Extending this logic, nobody would commit to the Linux kernel tree since it has had fuck and shit committed in revisions. What type of standards is that?
I would agree with Daniel above that if it's in a non-critical part of the commit message, if it bothers you that much, then follow it up yourself.
Well, I would prefer to get a notice that I made a typo than having that embarrassing typo in the commit log forever. That's the point of using a gating system, right? :)
So yes, I do think they should be corrected. (And I have committed typos in both commit messages and inside files, just as anyone else)
On 16/01/13 01:57, Daniel Kinzler wrote:
On 15.01.2013 15:06, Tyler Romeo wrote:
I agree with Antoine. Commit messages are part of the permanent history of this project. From now until MediaWiki doesn't exist anymore, anybody can come and look at the change history and the commit messages that go with them. Now you might ask what the possibility is of somebody ever coming across a single commit message that has a typo in it, but when you're using git-blame, git-bisect, or other similar tools, it's very possible.
And then they see a typo. So what? If you look through a mailing list archive or Wikipedia edit comments, you will also see typos.
I'm much more concerned about scaring away new contributors with such nitpicking.
I am also concerned about demotivating people.
Giving a change -1 means that you are asking the developer to take orders from you, under threat of having their work ignored forever. A -1 status can cause a change to be ignored by other reviewers, regardless of its merit.
If the developer can't lower their sense of pride sufficiently to allow them to engage with nitpickers, then the change might be ignored by all concerned for many months.
However, if you give minor negative feedback with +0, the change stays bold in your "review requests" list, as if you haven't reviewed it at all. I've tried giving -1 with a comment to the effect of "please merge this immediately regardless of my nitpicking above", but IIRC the comment was ignored.
I think people who give -1 should be aware of the potential roadblock they are creating. And I would like to see a feature in Gerrit to unbold +0 reviews.
Under Subversion, my policy as a reviewer was to never ask the committer to fix a typo in a comment, since committing the fix myself was easier than telling them what to fix, and doing so avoided offence. Under Gerrit, it's more difficult to submit amendments, and I hate multi-author patchsets anyway, so negative feedback seems more attractive.
Maybe the answer is better scripting for amendments and dependent commits.
-- Tim Starling
On Wed, Jan 16, 2013 at 7:09 AM, Tim Starling tstarling@wikimedia.org wrote:
I am also concerned about demotivating people.
The motivation factor works with the two positions.
I felt a little demotivated after having read all these "we don't care about typos" positions at the start of the thread and felt really relieved to read this is not the consensus opinion.
Especially, as, in September and October, when I weren't at ease with my -1 typo reviews, I mailed four people I reviewed their change such a way to ask them what they prefer: a -1 review or a new patchset fixing the typo directly (I weren't at ease with the idea to resubmit a patchset without prior author consent either). Three of them didn't bother to answer me at all (for the anecdote, the 4th preferred a -1 review). So we quit the realm of "the workflow and the UI don't allow easy correction" to enter into the realm of "we don't care".
On a side matter, typos could be a symptom of another issue: how important a commit message is? Should it be a formality to expedite in 30 seconds or an informative valuable text describing the change, crafted with care and proofread before submission or merge? What's the goal of a commit message as a changelog, communication tool and change documentation value?
At the end, the direct commit message edit in the UI will offer an acceptable solution: corrections will be more trivial than found again my branch, amend the commit, resubmit as a new patchset. Meanwhile, we can suffer the last weeks of extra work for review spelling (as 0 or -1) purpose pending the Gerrit migration.
And if you don't want to fix yourselves your commit, please create a list stating so on mediawiki.org, that will be a clear message for the code reviewers: "If you see a typo, would you be so kind as to fix it yourself and submit a new patchset?".
On Wed, Jan 16, 2013 at 8:06 AM, Sébastien Santoro dereckson@espace-win.org wrote:
At the end, the direct commit message edit in the UI will offer an acceptable solution: corrections will be more trivial than found again my branch, amend the commit, resubmit as a new patchset. Meanwhile, we can suffer the last weeks of extra work for review spelling (as 0 or -1) purpose pending the Gerrit migration.
And really, this is just a few weeks out. I've been testing the upgrade, and I'm confident we won't see any real problems. I'm planning to do it as soon as the eqiad migration is complete next week.
And if you don't want to fix yourselves your commit, please create a list stating so on mediawiki.org, that will be a clear message for the code reviewers: "If you see a typo, would you be so kind as to fix it yourself and submit a new patchset?".
Please no. We really don't want to encourage people to be lazy because they think people will clean up after them.
Really, I think the whole thread is moot with the pending upgrade. Typos should always be fixed before merging (I think we all agree?), and the new abilities to fix these from the UI means we won't need to mark people as -1 to do so.
-Chad
On 17/01/13 00:14, Chad wrote:
Really, I think the whole thread is moot with the pending upgrade. Typos should always be fixed before merging (I think we all agree?), and the new abilities to fix these from the UI means we won't need to mark people as -1 to do so.
I didn't mention commit summaries in my post. My interest is in nitpicking in general. Jeroen calls arguments over commit summaries the /ultimate/ bikeshed, which they may or may not be; there are plenty of other examples which may compete for that title.
Nitpicking is the minor end of the negative feedback spectrum. By definition, it has the smallest concrete payoff when advice is followed, in exchange for complex, context-dependent social costs. You should think carefully before you do it.
-- Tim Starling
On Wed, Jan 16, 2013 at 6:07 PM, Tim Starling tstarling@wikimedia.org wrote:
On 17/01/13 00:14, Chad wrote:
Really, I think the whole thread is moot with the pending upgrade. Typos should always be fixed before merging (I think we all agree?), and the new abilities to fix these from the UI means we won't need to mark people as -1 to do so.
I didn't mention commit summaries in my post. My interest is in nitpicking in general. Jeroen calls arguments over commit summaries the /ultimate/ bikeshed, which they may or may not be; there are plenty of other examples which may compete for that title.
Indeed, I had missed that.
Nitpicking is the minor end of the negative feedback spectrum. By definition, it has the smallest concrete payoff when advice is followed, in exchange for complex, context-dependent social costs. You should think carefully before you do it.
*nod* I agree. And really, nitpicks in code can always be cleaned up later (heck, we did it for years with SVN).
It's only nitpicks in commit messages that should always be fixed, since they're immutable after submission. And it's *that* that I think won't be a big deal anymore (since any drive-by contributor could fix a typo on the spot).
-Chad
On 2013-01-16 7:20 PM, "Chad" innocentkiller@gmail.com wrote:
On Wed, Jan 16, 2013 at 6:07 PM, Tim Starling tstarling@wikimedia.org
wrote:
On 17/01/13 00:14, Chad wrote:
Really, I think the whole thread is moot with the pending upgrade. Typos should always be fixed before merging (I think we all agree?), and the new abilities to fix these from the UI means we won't need to mark people as -1 to do so.
I didn't mention commit summaries in my post. My interest is in nitpicking in general. Jeroen calls arguments over commit summaries the /ultimate/ bikeshed, which they may or may not be; there are plenty of other examples which may compete for that title.
Indeed, I had missed that.
Nitpicking is the minor end of the negative feedback spectrum. By definition, it has the smallest concrete payoff when advice is followed, in exchange for complex, context-dependent social costs. You should think carefully before you do it.
*nod* I agree. And really, nitpicks in code can always be cleaned up later (heck, we did it for years with SVN).
It's only nitpicks in commit messages that should always be fixed, since they're immutable after submission. And it's *that* that I think won't be a big deal anymore (since any drive-by contributor could fix a typo on the spot).
If we're talking nitpicks in general. Ive seen -1 for things like someFunc($a, $b) instead of someFunc( $a, $b ) which I agree does more harm than good.
I imagine how much someone considers spelling issues to be a minor "nitpick" varries quite a lot between people. -bawolff
If we're talking nitpicks in general. Ive seen -1 for things like someFunc($a, $b) instead of someFunc( $a, $b ) which I agree does more harm than good.
I disagree. The entire purpose of code review is to make sure the code is organized and styled properly. If code isn't written in accordance with MediaWiki style requirements, that's even more of a reason to -1 than a typo in the commit message.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Jan 16, 2013 at 9:11 PM, Bawolff Bawolff bawolff@gmail.com wrote:
On 2013-01-16 7:20 PM, "Chad" innocentkiller@gmail.com wrote:
On Wed, Jan 16, 2013 at 6:07 PM, Tim Starling tstarling@wikimedia.org
wrote:
On 17/01/13 00:14, Chad wrote:
Really, I think the whole thread is moot with the pending upgrade. Typos should always be fixed before merging (I think we all agree?), and the new abilities to fix these from the UI means we won't need to mark people as -1 to do so.
I didn't mention commit summaries in my post. My interest is in nitpicking in general. Jeroen calls arguments over commit summaries the /ultimate/ bikeshed, which they may or may not be; there are plenty of other examples which may compete for that title.
Indeed, I had missed that.
Nitpicking is the minor end of the negative feedback spectrum. By definition, it has the smallest concrete payoff when advice is followed, in exchange for complex, context-dependent social costs. You should think carefully before you do it.
*nod* I agree. And really, nitpicks in code can always be cleaned up later (heck, we did it for years with SVN).
It's only nitpicks in commit messages that should always be fixed, since they're immutable after submission. And it's *that* that I think won't be a big deal anymore (since any drive-by contributor could fix a typo on the spot).
If we're talking nitpicks in general. Ive seen -1 for things like someFunc($a, $b) instead of someFunc( $a, $b ) which I agree does more harm than good.
I imagine how much someone considers spelling issues to be a minor "nitpick" varries quite a lot between people. -bawolff _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Le 16/01/13 14:06, Sébastien Santoro a écrit :
Should it be a formality to expedite in 30 seconds or an informative valuable text describing the change, crafted with care and proofread before submission or merge?
To me the commit summary introduce the patch to the reviewers and should also serve as a base for later debugging. It is always good to explain there the problem you attempt to solve or the feature being implemented, then explain your design choice and finally explain what you have tested / what people should be careful about.
Of course I do not apply that principle to myself :-] Some of the repositories on which I am the sole contributor and self reviewing, have very short commit message. But I do try to add extensive comments in the code.
A random example:
We had a trivial configuration issue in our php.ini file. The apc.shm_size parameter was missing a suffix to the amount of memory that need to be added.
The patch is:
files/php/apc.ini -apc.shm_size=240 +apc.shm_size=240M
I could have written:
'added M to apc mem size'
Instead I wrote:
------------------------ php.ini apc.shm_size requires
The apc.shm_size is an integer determining the size of each memory segment in MB. When missing the M suffix, PHP send us a warning in syslog:
php: PHP Warning: PHP Startup: apc.shm_size now uses M/G suffixes, please update your ini files in Unknown on line 0
Upstream documentation: http://php.net/manual/en/apc.configuration.php#ini.apc.shm-size
Having a change like 'fix preg call' is a -1 for me. I don't even bother reading the code in such a case. ------------------------
Yeah there are probably typo above, but at least that: - explain the change (I add M to apc.shm_size) - expose the issue (there is an annoying PHP warning) - reference doc for easy lookup for the reviewer
Done.
The PHP code base has usually very short summaries cause they simply reference the bug report. Example:
Bug #62593 Added test for change
The git project usually has very nice commit message. Have a look at http://repo.or.cz/w/git.git/commit/b72a1904aefb9e27014a11790f3af4dc90b38e8d
That optimize some code path, the summary show up command line output before and after the patch :-]
So yeah, we need extensive commit message.
If someone don't bother fixing a typo (which is like 1 minute of work), I will not bother doing it for them nor approving their change. My time is better invested in reviewing another patch.
Thanks Tim for pitching in.
On 16.01.2013 07:09, Tim Starling wrote:
Giving a change -1 means that you are asking the developer to take orders from you, under threat of having their work ignored forever. A -1 status can cause a change to be ignored by other reviewers, regardless of its merit.
If the developer can't lower their sense of pride sufficiently to allow them to engage with nitpickers, then the change might be ignored by all concerned for many months.
That's exactly the problem.
However, if you give minor negative feedback with +0, the change stays bold in your "review requests" list, as if you haven't reviewed it at all. I've tried giving -1 with a comment to the effect of "please merge this immediately regardless of my nitpicking above", but IIRC the comment was ignored.
Yes, mentioning a type in a +0 comment would be perfectly fine with me. I generally use +0 for nitpicks, i.e. anything that doesn't really hurt. Nitpicks with a -1 are really annoying.
Anyway: editing in the UI makes the whole argument mute.
-- daniel
This is a ridiculous conversation and I can't believe it now spans +20 messages.
___Don't___ -1 a patchset for a typo. The result of that is far more catastrophic. We put off volunteers and people end up wasting valuable time doing rebases due to the time taken to find another code review. Code review is hard enough as it is.
Make a ___suggestion___ about fixing the typo so the person who ends up merging it can do it themselves if necessary.
+1 or -1 depending on the __code quality__ alone.
Can we now return our attention to engaging in more interesting and useful conversations to the future of MediaWiki such as "[Wikitech-l] Disambiguation features: Do they belong in core or in an extension?"
On Tue, Jan 15, 2013 at 10:09 PM, Tim Starling tstarling@wikimedia.org wrote:
On 16/01/13 01:57, Daniel Kinzler wrote:
On 15.01.2013 15:06, Tyler Romeo wrote:
I agree with Antoine. Commit messages are part of the permanent history of this project. From now until MediaWiki doesn't exist anymore, anybody can come and look at the change history and the commit messages that go with them. Now you might ask what the possibility is of somebody ever coming across a single commit message that has a typo in it, but when you're using git-blame, git-bisect, or other similar tools, it's very possible.
And then they see a typo. So what? If you look through a mailing list archive or Wikipedia edit comments, you will also see typos.
I'm much more concerned about scaring away new contributors with such nitpicking.
I am also concerned about demotivating people.
Giving a change -1 means that you are asking the developer to take orders from you, under threat of having their work ignored forever. A -1 status can cause a change to be ignored by other reviewers, regardless of its merit.
If the developer can't lower their sense of pride sufficiently to allow them to engage with nitpickers, then the change might be ignored by all concerned for many months.
However, if you give minor negative feedback with +0, the change stays bold in your "review requests" list, as if you haven't reviewed it at all. I've tried giving -1 with a comment to the effect of "please merge this immediately regardless of my nitpicking above", but IIRC the comment was ignored.
I think people who give -1 should be aware of the potential roadblock they are creating. And I would like to see a feature in Gerrit to unbold +0 reviews.
Under Subversion, my policy as a reviewer was to never ask the committer to fix a typo in a comment, since committing the fix myself was easier than telling them what to fix, and doing so avoided offence. Under Gerrit, it's more difficult to submit amendments, and I hate multi-author patchsets anyway, so negative feedback seems more attractive.
Maybe the answer is better scripting for amendments and dependent commits.
-- Tim Starling
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, Jan 16, 2013 at 7:26 PM, Jon Robson jdlrobson@gmail.com wrote:
This is a ridiculous conversation and I can't believe it now spans +20 messages.
Apparently you don't care, but other people do care. Please do not disregard other people's opinions because you believe yours is correct. To keep it in the style of this thread; attitudes like these do demotivate _me_.
Also, I fully agree with Maarten's last comment.
Bryan
I don't mind getting dinged for typos. If I'm being sloppy it's fair to point it out.
However, I think the social contract should be that after I fix the typos you requested then you owe me a real code review where you look at the merits of the code. Code review is an awesomely useful but time consuming thing to provide. Patrolling for typos is not.
Put it this way, if I concede and agree that the bikeshed can be green, the people who spent three hours arguing for green should feel obligated to turn up to the working bee to help with the painting.
Luke Welling
On Wed, Jan 16, 2013 at 10:40 AM, Bryan Tong Minh bryan.tongminh@gmail.comwrote:
On Wed, Jan 16, 2013 at 7:26 PM, Jon Robson jdlrobson@gmail.com wrote:
This is a ridiculous conversation and I can't believe it now spans +20 messages.
Apparently you don't care, but other people do care. Please do not disregard other people's opinions because you believe yours is correct. To keep it in the style of this thread; attitudes like these do demotivate _me_.
Also, I fully agree with Maarten's last comment.
Bryan _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 2013-01-16 3:07 PM, "Luke Welling WMF" lwelling@wikimedia.org wrote:
I don't mind getting dinged for typos. If I'm being sloppy it's fair to point it out.
However, I think the social contract should be that after I fix the typos you requested then you owe me a real code review where you look at the merits of the code. Code review is an awesomely useful but time consuming thing to provide. Patrolling for typos is not.
Put it this way, if I concede and agree that the bikeshed can be green,
the
people who spent three hours arguing for green should feel obligated to turn up to the working bee to help with the painting.
Luke Welling
Well put. That sounds entirely fair to me.
-bawolff
Hey,
Thanks to all for voicing your opinions.
My observations:
* The community is split on this topic * Both camps have people that strongly defend their respective views * The above two points are unlikely to change * People are getting upset due to -1 for things that are by some considered nitpicking * People are getting upset because others are not willing to fix their -1 remarks since they consider them to be nitpicks * Commits that are otherwise fine are getting stuck because of this
Loose from how you choose to interpret and deal with those, let's try to be tolerant of each other, keep and eye on the bigger picture, and work towards what is really important as a community.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
Hey,
Thanks to all for voicing your opinions.
My observations:
- The community is split on this topic
- Both camps have people that strongly defend their respective views
- The above two points are unlikely to change
- People are getting upset due to -1 for things that are by some
considered
nitpicking
- People are getting upset because others are not willing to fix their -1
remarks since they consider them to be nitpicks
- Commits that are otherwise fine are getting stuck because of this
Loose from how you choose to interpret and deal with those, let's try to
be
tolerant of each other, keep and eye on the bigger picture, and work towards what is really important as a community.
Cheers
+2
On 01/19/2013 08:49 AM, Jeroen De Dauw wrote:
Hey,
Thanks to all for voicing your opinions.
My observations:
- The community is split on this topic
- Both camps have people that strongly defend their respective views
- The above two points are unlikely to change
- People are getting upset due to -1 for things that are by some considered
nitpicking
- People are getting upset because others are not willing to fix their -1
remarks since they consider them to be nitpicks
- Commits that are otherwise fine are getting stuck because of this
Loose from how you choose to interpret and deal with those, let's try to be tolerant of each other, keep and eye on the bigger picture, and work towards what is really important as a community.
Cheers
-- Jeroen De Dauw
Jeroen, I'm 100% in agreement with this. I thank you for starting this conversation and surfacing this issue.
I saw some advice recently that if you're trying to meet someone halfway, you should do a little more than 50%, like 60 or 65%, to account for heat loss. :-)
wikitech-l@lists.wikimedia.org