Issue 6279048: CLs to include in Go 1.0.2 (Closed)
DescriptionCLs to include in Go 1.0.2
This file is not for checking in. It's here to make per-line commenting easier.
Patch Set 1 #Patch Set 2 : diff -r a252a6b64258 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r a252a6b64258 https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 4 : diff -r a2cb3130bd86 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r a2cb3130bd86 https://go.googlecode.com/hg/ #
Total comments: 16
Patch Set 6 : diff -r 98a72e834d58 https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 7 : diff -r 319a9f3330d0 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r 319a9f3330d0 https://go.googlecode.com/hg/ #
Total comments: 3
Patch Set 9 : diff -r b4cdcec897fb https://go.googlecode.com/hg/ #Patch Set 10 : diff -r 7550d1465219 https://go.googlecode.com/hg/ #Patch Set 11 : diff -r 7550d1465219 https://go.googlecode.com/hg/ #Patch Set 12 : diff -r 267090160633 https://go.googlecode.com/hg/ #MessagesTotal messages: 21
http://codereview.appspot.com/6279048/diff/4002/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/4002/go102#newcode105 go102:105: 32a8b0e41031 mime/multipart: fix handling of empty parts without CRLF before next part It'd be nice to include this. It's big, but it's also now better tested, and it's a pretty high-level package. The failure case that this fixes was non-trivial to track down, too.
Sign in to reply to this message.
your criteria mystify me.
Sign in to reply to this message.
http://codereview.appspot.com/6279048/diff/4002/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/4002/go102#newcode118 go102:118: 85e2b70e9d44 runtime: handle windows exceptions, even in cgo programs It would be good to include this IMHO. Even though it is big, it seems to be an important bugfix for Windows.
Sign in to reply to this message.
http://codereview.appspot.com/6279048/diff/4002/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/4002/go102#newcode101 go102:101: 4ef88bab4b0d path/filepath: implement documented SkipDir behavior Please, include 45d1063d8520 syscall: correct Win32finddata definition It fixes memory corruption.
Sign in to reply to this message.
On Sun, Jun 3, 2012 at 12:16 AM, <r@golang.org> wrote: > your criteria mystify me. I believe the rule is the same as last time: The benefit of the change has to outweigh the risk of making it. For most code, that means two things. First, a change has to fix a bug, not be adding a new feature. Second, the fix has to be trivial, or if not the bug has to be very important to fix (like the map key bug). To evaluate that, you have to inspect the CL itself, and I looked at every CL since Go 1.0.1, using the commands at the top of the file to generate the list and the plumb rule I posted to 9fans to view the changes. Last time Andrew also inspected the changes, and then we compared our lists. I assume he will do that again. There have been 280 CLs since Go 1.0.1. My bug fix list contains 54. We have been focusing a lot of attention on bug fixes, so the percentage is just under 20%. The "NOT TAKEN" section only shows bug fixes that I thought were below the bar but that I also thought people might disagree with me about. There are still nearly 200 CLs not listed at all. There are also some categories that we just always take, because the risk is little to none and taking them now will avoid merge conflicts when there's something we want later. A+C files and documentation-only changes have no risk, and changes to godoc have minor risk - if godoc fails in some minor way it doesn't make other go programs crash. In this round I also applied that to misc/emacs, which has seen a lot of fixes recently including one fix for an editor deadlock. I originally listed these first in the file, because they were shorter, but they really are secondary, so I've moved them farther down. Russ
Sign in to reply to this message.
please consider adding this (see Russ's comment #4): 31e36672e3df 2: +30/-28 syscall: fix 32-bit uid calls
Sign in to reply to this message.
FYI http://codereview.appspot.com/6279048/diff/11001/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/11001/go102#newcode58 go102:58: 4849e2acf1e9 2: +76/-3 net/http: flush server response gracefully when ignoring request body I don't care that this one goes in. This was just a precursor to the one that's still out for review which is more important. Taking 50% of net/http patches for each 1.0.x release will get merge-painful eventually. I'm not even sure how important taking the hasToken ones are. You're doing the work, though, so up to you. http://codereview.appspot.com/6279048/diff/11001/go102#newcode108 go102:108: 32a8b0e41031 2: +292/-66 mime/multipart: fix handling of empty parts without CRLF before next part Could you look at this one again? It's relatively big for a bug fix, but we'd assumed this was going into 1.0.2 to fix the App Engine blobstore bug with empty post values. I spent a fair bit of time writing comments, tests, seeing what various clients did, and simplifying the code to the point I was more comfortable with the code than I was before.
Sign in to reply to this message.
FYI http://codereview.appspot.com/6279048/diff/11001/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/11001/go102#newcode9 go102:9: bug fixes that are trivial or important enough to take a non-trivial fix: I think it's fairly important, and very safe, to include 5e1544310d03, which avoids leaving a binary in /tmp after running the tests.
Sign in to reply to this message.
http://codereview.appspot.com/6279048/diff/11001/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/11001/go102#newcode9 go102:9: bug fixes that are trivial or important enough to take a non-trivial fix: if you're categorizing it would be worthwhile to separate 'trivial' and 'too important to miss' into separate sections http://codereview.appspot.com/6279048/diff/11001/go102#newcode15 go102:15: d063a7844d48 1: +33/-27 cgo: rename C names for Go types to avoid conflicting with package is this an API change? http://codereview.appspot.com/6279048/diff/11001/go102#newcode34 go102:34: c6213d8a9118 1: +1/-0 crypto/rsa: add SHA-224 hash prefix this seems like a feature rather than a fix http://codereview.appspot.com/6279048/diff/11001/go102#newcode115 go102:115: 1734b211731d 2: +46/-10 sync/atomic: use cas64 to implement {Load,Store,Add}{Uint,Int}64 on Linux/ARM these last two sound important
Sign in to reply to this message.
PTAL http://codereview.appspot.com/6279048/diff/4002/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/4002/go102#newcode101 go102:101: 4ef88bab4b0d path/filepath: implement documented SkipDir behavior On 2012/06/03 11:11:59, brainman wrote: > Please, include > 45d1063d8520 syscall: correct Win32finddata definition > It fixes memory corruption. Done. http://codereview.appspot.com/6279048/diff/4002/go102#newcode105 go102:105: 32a8b0e41031 mime/multipart: fix handling of empty parts without CRLF before next part On 2012/06/03 02:06:27, dsymonds wrote: > It'd be nice to include this. It's big, but it's also now better tested, and > it's a pretty high-level package. The failure case that this fixes was > non-trivial to track down, too. Done. http://codereview.appspot.com/6279048/diff/4002/go102#newcode118 go102:118: 85e2b70e9d44 runtime: handle windows exceptions, even in cgo programs It's bigger than I'm comfortable with. Alex, any opinion on how important it is? http://codereview.appspot.com/6279048/diff/11001/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/11001/go102#newcode9 go102:9: bug fixes that are trivial or important enough to take a non-trivial fix: On 2012/06/04 18:30:39, r wrote: > if you're categorizing it would be worthwhile to separate 'trivial' and 'too > important to miss' into separate sections Done. http://codereview.appspot.com/6279048/diff/11001/go102#newcode9 go102:9: bug fixes that are trivial or important enough to take a non-trivial fix: On 2012/06/04 04:52:53, iant wrote: > I think it's fairly important, and very safe, to include 5e1544310d03, which > avoids leaving a binary in /tmp after running the tests. Done. http://codereview.appspot.com/6279048/diff/11001/go102#newcode9 go102:9: bug fixes that are trivial or important enough to take a non-trivial fix: On 2012/06/04 04:52:53, iant wrote: > I think it's fairly important, and very safe, to include 5e1544310d03, which > avoids leaving a binary in /tmp after running the tests. Done. http://codereview.appspot.com/6279048/diff/11001/go102#newcode15 go102:15: d063a7844d48 1: +33/-27 cgo: rename C names for Go types to avoid conflicting with package On 2012/06/04 18:30:39, r wrote: > is this an API change? No, it's just a change to what code gcc sees. http://codereview.appspot.com/6279048/diff/11001/go102#newcode34 go102:34: c6213d8a9118 1: +1/-0 crypto/rsa: add SHA-224 hash prefix On 2012/06/04 18:30:39, r wrote: > this seems like a feature rather than a fix I am treating this as a fix for a missing entry in a table. http://codereview.appspot.com/6279048/diff/11001/go102#newcode58 go102:58: 4849e2acf1e9 2: +76/-3 net/http: flush server response gracefully when ignoring request body On 2012/06/03 19:26:10, bradfitz wrote: > I don't care that this one goes in. This was just a precursor to the one that's > still out for review which is more important. > > Taking 50% of net/http patches for each 1.0.x release will get merge-painful > eventually. I'm not even sure how important taking the hasToken ones are. > You're doing the work, though, so up to you. Done. http://codereview.appspot.com/6279048/diff/11001/go102#newcode108 go102:108: 32a8b0e41031 2: +292/-66 mime/multipart: fix handling of empty parts without CRLF before next part On 2012/06/03 19:26:10, bradfitz wrote: > Could you look at this one again? It's relatively big for a bug fix, but we'd > assumed this was going into 1.0.2 to fix the App Engine blobstore bug with empty > post values. > > I spent a fair bit of time writing comments, tests, seeing what various clients > did, and simplifying the code to the point I was more comfortable with the code > than I was before. Done. http://codereview.appspot.com/6279048/diff/11001/go102#newcode115 go102:115: 1734b211731d 2: +46/-10 sync/atomic: use cas64 to implement {Load,Store,Add}{Uint,Int}64 on Linux/ARM These make 64-bit compare-and-swap work on ARMv5. They're big and complex, and I didn't hear anyone clamoring for it so I left them out. I'm willing to reconsider if someone makes the case.
Sign in to reply to this message.
Please include: 008b163fb38a doc/install: we don't print 'The compiler is 6g' anymore 222a28e46943 doc: mention 'hg update default' in contribution guidelines ae0301294b9b go spec: clarify promotion rules for methods/fields of anonymous fields
Sign in to reply to this message.
On 2012/06/06 22:29:31, rsc wrote: > > http://codereview.appspot.com/6279048/diff/4002/go102#newcode118 > go102:118: 85e2b70e9d44 runtime: handle windows exceptions, even in cgo programs > It's bigger than I'm comfortable with. Alex, any opinion on how important it is? > Yes it could bite us. This change affects only cgo programs. It has (should have <g>) no effect on non-cgo programs. At this moment, if cgo program gets any exceptional error (memory access error, division on zero and such), it will crash in unexpected way - no stack trace, no running recover(), it might end-up invoking system debugger (depending on particular pc setup). To make matters worse, this behavior will only happens to all threads, but first. So, very unpleasant situation. On the other hand, it only affect handful of people - those who use cgo on windows. I hope there aren't many sufferers :-). And these should be proficient enough to be able to use tip for the time being, if we direct them so. Alex
Sign in to reply to this message.
I think I will leave windows/cgo to fend for itself, at least for now. Russ
Sign in to reply to this message.
http://codereview.appspot.com/6279048/diff/15001/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/15001/go102#newcode9 go102:9: bug fixes that are important: 31e36672e3df 2: +30/-28 syscall: fix 32-bit uid calls i think this fix is fairly important, because failed getuid() can cause lots of other troubles; and this fix is pretty trivial. http://codereview.appspot.com/6279048/diff/15001/go102#newcode65 go102:65: 5538444d6f32 2: +2227/-1 api: add Linux/ARM to go1 API d8e47164f8dd 2: +4064/-7 api: add FreeBSD to go1 API http://codereview.appspot.com/6279048/diff/15001/go102#newcode95 go102:95: d7030ef36339 1: +1/-0 CONTRIBUTORS: Add Ryan Barrett (Google CLA) and two more: a6e2b17a16d7 2: +2/-0 A+C: Markus Sonderegger (individua CLA) a47632179cb7 2: +2/-0 A+C: Daniel Morsing (individual CLA)
Sign in to reply to this message.
List updated again. This time it's a valid set of patches that produces a working tree. http://codereview.appspot.com/6279048/diff/15001/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/15001/go102#newcode9 go102:9: bug fixes that are important: On 2012/06/07 06:42:04, minux wrote: > 31e36672e3df 2: +30/-28 syscall: fix 32-bit uid calls > > i think this fix is fairly important, because failed getuid() can cause lots of > other troubles; and this fix is pretty trivial. Done. http://codereview.appspot.com/6279048/diff/15001/go102#newcode65 go102:65: 5538444d6f32 2: +2227/-1 api: add Linux/ARM to go1 API On 2012/06/07 06:42:04, minux wrote: > d8e47164f8dd 2: +4064/-7 api: add FreeBSD to go1 API Done. http://codereview.appspot.com/6279048/diff/15001/go102#newcode95 go102:95: d7030ef36339 1: +1/-0 CONTRIBUTORS: Add Ryan Barrett (Google CLA) On 2012/06/07 06:42:04, minux wrote: > and two more: > a6e2b17a16d7 2: +2/-0 A+C: Markus Sonderegger (individua CLA) > a47632179cb7 2: +2/-0 A+C: Daniel Morsing (individual CLA) Done.
Sign in to reply to this message.
http://codereview.appspot.com/6279048/diff/11001/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/11001/go102#newcode15 go102:15: d063a7844d48 1: +33/-27 cgo: rename C names for Go types to avoid conflicting with package Aren't these names used in exported functions? maybe the code remains compatible anyway?
Sign in to reply to this message.
On Thu, Jun 7, 2012 at 3:00 PM, <remyoudompheng@gmail.com> wrote: > Aren't these names used in exported functions? maybe the code remains > compatible anyway? I think they only appear in the cgo-generated code, as seen by gcc. I don't believe they are used by people writing cgo input. Russ
Sign in to reply to this message.
http://codereview.appspot.com/6279048/diff/25002/go102 File go102 (right): http://codereview.appspot.com/6279048/diff/25002/go102#newcode13 go102:13: 9b455eb64690 3: +3/-3 cmd/5c, cmd/5g, cmd/5l: fix array indexing warning under Clang 3.1 perhaps we should wait for issue 3708 to accompany this change. http://codereview.appspot.com/6279048/diff/25002/go102#newcode78 go102:78: ac7250065a04 1: +3/-1 syscall: simplify text returned by Errno.Error() when FormatMessage fails duplicate. http://codereview.appspot.com/6279048/diff/25002/go102#newcode147 go102:147: 66a5ac82dc70 4: +62/-16 syscall: implement SetsockoptLinger for windows strange, rev 66a5ac82dc70 is both taken and not taken.
Sign in to reply to this message.
On Thu, Jun 7, 2012 at 3:58 PM, <minux.ma@gmail.com> wrote: > strange, rev 66a5ac82dc70 is both taken and not taken. everything below the NOT TAKEN line is ignored by my program. i had to add some things that i hadn't planned to take, like this one, in order to be able to apply later patches. in this case it was one or two tiny syscall additions. the windows syscall story is still up in the air.
Sign in to reply to this message.
The current version of this file corresponds to http://code.google.com/r/rsc-go1-0-2-test-3/. I think it's pretty close to what we'll use.
Sign in to reply to this message.
|
