Code Review 0.4.2..0.4.3-rc3

[jbenet]

I went through and code reviewed all the PRs, commits, and merges from 0.4.2..0.4.3-rc3.

First, a huge thanks to everyone. this is a huge release, with tremendous stuff. Huge shout out to everyone who worked on it, sp @whyrusleeping @Kubuxu @kevina @lgierth @RichardLitt @chriscool and all those listed here https://github.com/ipfs/go-ipfs/blob/master/CHANGELOG.md#043-rc3---2016-08-09 congrats, this is a great release with a ton of bug fixes, stability improvements, and new features. 🎉 🎆 🎊 🎈 🎉

On this Code Review

My code review was for the purpose of finding any remaining problems before the 0.4.3 release. I read all of the code. I didn't look at everything super carefully; i focused on critical sections, UX, API changes, and security issues. I've left a bunch of comments all over the place (Merges, PRs, commits).

I took specific notes of things to address. The "MUST be addressed by 0.4.3" ones should have a commit or PR link to a merged thing (edit this issue's list to add it next to the line item when you check it) before release. It's not that much, but it's also substantial. When done, please add the link like this at the end of the line:

- [ ] <line item> -- resolved in <link-to-merged-PR>

By design, this whole list is negative, as the goal was identifying problems. Meaning, i only highlighted all the stuff that needs to change. I looked at every commit, so a lacking comment here means that those changes implicitly LGTM, and I'm fine with releasing them. So thank you for all that! 👍 😄 🎉 🎉

It's up to @whyrusleeping whether to do these changes on top of master or on top of 0.4.3-rc3. If we want to roll the latest changes (0.4.3-rc3..master) into 0.4.3 (instead of 0.4.4), then i will need to code review that section too. I will get started on that anyway, and should have it done by monday eve.

MUST happen before 0.4.3 release

• failed to ipfs-update install v0.4.3-rc3 -- fs-repo-migrations Is this in dist.ipfs.io? fs-repo-migrations#45
• change for ipfs refs local in the api. needs better changelog entry and option? Fix refs local marshalling #2812 (comment)
• needs a specific test -- Add CORS headers to Read Only Gateway Default config #2778 --- resolved in test: do explicit check of CORS headers #3142
• license question -- https://github.com/ipfs/go-ipfs/pull/2822/files#r76513373
• tests from the gateway fail need to be back @lgierth
  • actually, i thought that PR was entirely reverted... how are we sure there aren't other issues? -- A: fixing the refactored (broken) code is a net win over reverting it all, resolved in gateway: clean up its surface, and remove BlockList #2874 (comment)
  • no longer forbidden bug? -- A: nope, resolved in gateway: clean up its surface, and remove BlockList #2874 (comment)
  • bash: bring --unrestricted-api completion back (sorry!) #3159 brings back --unrestricted-api bash auto completion
• best effort file api pin needs testing https://github.com/ipfs/go-ipfs/pull/2872/files#r76513709 --- needs review Enhance tests for files API root best-effort pin. #3151
  • best effort file api pin is an important product + protocol decision i never knew about. not great, clear these with me
  • best effort pins implemented poorly Add Files API root as best-effort pin #2872 (comment)
• need to revert symlink resolve -- Resolve symlink if it is directly referenced in cli #2897 --- resolved in Directly Fix #2839 #3135
  • revert this too -- Resolve symlinks of testfiles #2926 --- already reverted in rework stdin handling #2952
• auto raise fd limit needs better testing https://github.com/ipfs/go-ipfs/pull/2891/files#r76513912 --- needs review fdlimit: improve fdlimit autoraising tests #3149
• CLI stdin breaking changes? -- Change logic of stdin input to be last and only if required #2822 (comment)
  • cli stdin redux, not reverted? -- https://github.com/ipfs/go-ipfs/pull/2902/files#r76514113 (test: revert few stdin tests that were removed #3144 (comment))
• proper repo verify testing -- https://github.com/ipfs/go-ipfs/pull/2924/files#r76514755
• PrivKey rm security harden -- https://github.com/ipfs/go-ipfs/pull/2957/files#r76539226
  • insecure measure -- https://github.com/ipfs/go-ipfs/pull/2957/files#r76539324
  • lower case test case? -- https://github.com/ipfs/go-ipfs/pull/2957/files#r76539414
  • Merged cmd: harden the security of privkey field in config show #3141
• auto-run migration doesnt have latest migrations, right? -- https://github.com/ipfs/go-ipfs/pull/2939/files
  • better errors in auto-install fail @lgierth -- https://github.com/ipfs/go-ipfs/pull/2939/files#r76539747
    • RFCR fsrepo: improve migrations copy #3158
  • more errors @lgierth -- https://github.com/ipfs/go-ipfs/pull/2939/files#r76539800
    • RFCR fsrepo: improve migrations copy #3158

Important but postponed to 0.4.4

• blockstore.{RuntimeHashing -> HashOnRead} -- https://github.com/ipfs/go-ipfs/pull/2904/files#r76514037

  Other

• ipfs swarm peers -v may output a 0 latency -- ipfs swarm peers -v may output a 0 latency #3125
• 2482dc8 merged two commits, one which undid the other. Probably best to ask author to rebase and remove the "undone" code.
• ipfs files cp/write erros and whatnot -- ipfs files cp/write from local fs? confusing errors? #3126
• ipfs files stat buggy default output -- ipfs files stat buggy default format? #3127
• please rebase PRs before merging pls. it makes long term CR easier. (particularly @Kubuxu)
• undo Add Defaults to ipfs add #2657 language and progress bar -- Add Defaults to ipfs add #2657 (comment)
• offline mode PR feedback -- https://github.com/ipfs/go-ipfs/pull/2696/files#r76381690
• [WIP] Filestore Implementation #2634 Adder error prone? -- https://github.com/ipfs/go-ipfs/pull/2792/files#r76384826
• needs squashing commits. non passing were merged ipfs repo version command #2598
• regex fix https://github.com/ipfs/go-ipfs/pull/2598/files#r76388121
• some improvements to dht messageSender code in https://github.com/ipfs/go-ipfs/pull/2817/files#r76512847
• thank you for adding tests. PRs with only tests are great.
• dup tests into graphs? Make bitswap better #2798
• not super clear why -- Improve ping API a bit by returning failure in case of only failure #2813
• clarify RoutingTable protects -- https://github.com/ipfs/go-ipfs/pull/2841/files#r76513202
• yay for all the gx-ification!
• makefile change buried in another PR https://github.com/ipfs/go-ipfs/pull/2855/files#r76513294
• SO_REUSEPORT totally disabled in go tests, scary -- Fix DHT tests failing because of repeated addresses #2855 (comment)
• codify API changes into commits -- Add way to check if node is online or offline #2867
• come up with guidelines for timeouts -- implement some simple dht request read timeouts #2863
• fd limits question https://github.com/ipfs/go-ipfs/pull/2884/files#r76513661
• copyright notice needs fixing https://github.com/ipfs/go-ipfs/pull/2883/files#r76513693
• autogenerated synopsis, short usage? -- Add synopsis autogenerator #2785 (comment)
• abstraction leak, ls resolve flag -- https://github.com/ipfs/go-ipfs/pull/2824/files#r76514362
• very nice write up + thinking through first -- In datastote.NewKey calling path.Clean is dangerous #2601 (comment)
• benchmark graphs for blockstore arc cache -- Add ARC caching and bloom filter for blockstorage #2885 (comment)
• some CR notes on https://github.com/ipfs/go-ipfs/pull/2953/files
• some CR notes on https://github.com/ipfs/go-ipfs/pull/2960/files#r76539147
• some comments on - document the config options #2974
• test migration bin names extract binaries with .exe name on windows #3021
• why is this lock here? https://github.com/ipfs/go-ipfs/pull/3026/files#r76540554
• test these changes? cli: do ToSlash after EvalSymlinks to remove platform specific slashes #3023 (comment)
• comment these things -- https://github.com/ipfs/go-ipfs/pull/3036/files#r76540668

[Kubuxu]

About rebasing PRs before merging, I can't do much in this manner, it is Jeromy who merges them, he can ping me to rebase them (he does in case of conflict) or rebase them himself (my signature won't match then).

[whyrusleeping]

@jbenet thanks for all the review! The strategy will be to branch off of the 0.4.3-rc3 tag and merge everything to there. Then merge that into master once the release happens.

[Kubuxu]

I already created version/0.4.3-rc4 which is based off the tag and there are two PRs created to it: #3134 #3135

[ghost]

Another lesson from the 0.4.3 cycle is that we should only start releasing RC builds once we're absolutely positive that this is what we want to release. We're close to a 1.5 months RC cycle.

[Kubuxu]

@jbenet Is blockstore.{RuntimeHashing -> HashOnRead} neccesary for 0.4.3, it would be best to keep the changeset minimal in the 0.4.3 branch to no go into merge conflict hell.

As it is small refactor (and master has already some changes with it), it would be better to keep it on master. Sounds good?

@lgierth I don't know why having long RC cycles is bad thing, it only show that our QA game needs improvement but including community in throughout testing is a good thing.

[jbenet]

@jbenet Is blockstore.{RuntimeHashing -> HashOnRead} neccesary for 0.4.3, it would be best to keep the changeset minimal in the 0.4.3 branch to go into merge conflict hell.

@Kubuxu it's an interface. our releases are not only binaries, they're also the code. It's pretty minimal, but okay.

[ghost]

@lgierth I don't know why having long RC cycles is bad thing, it only show that our QA game needs improvement but including community in throughout testing is a good thing.

Sure -- I just mean that if we know that @jbenet is still to review it, and is likely going to have lots of feedback, it's IMO not yet a release candidate. A release candidate is "almost exactly this is going to be the release, unless there are bad bugs or regressions".

[jbenet]

Agree with @lgierth

[kevina]

@jbenet (also @whyrusleeping @Kubuxu) concerning "best effort" pins, let me know what I need to do. At the moment this seams to be due to a misunderstanding, see #2698 for the reasoning behind them.

[RichardLitt]

Thanks, @jbenet. Good to see a lot of these comments!

[whyrusleeping]

@lgierth You're generally more eloquent than I am, could you handle adding better 'error case help text' here? https://github.com/ipfs/go-ipfs/pull/2939/files

[ghost]

@lgierth You're generally more eloquent than I am, could you handle adding better 'error case help text' here? https://github.com/ipfs/go-ipfs/pull/2939/files

Sure, here: #3158

[jbenet]

People have been checking boxes above without editing the post and adding:

- [ ] <problem description and issue links> --- resolved in <link-to-merged-PR>

see the "On this Code Review" rection

[jbenet]

Oh i see some RFCR things. i'll check those

[ghost]

People have been checking boxes above without editing the post and adding:

sorry that was sloppy -- I clarified that section about the gateway/blocking/reverting questions. Check the linked answers, I think these are resolved. (you're right, I shouldn't check them off, just provide you with enough info.)

[kevina]

I went through and clarified referenced pull requests with ---resolved in #XXX for merged commits. I also added ---needs review #XXX for unmerged commits pending review.

Github makes it way to easy to accidentally toggle a checkbox and does not provide the history of edited comments to verify what I did. Sorry, if I did this and caused something to be wrong.

[whyrusleeping]

@jbenet alright, your 'must haves' are all addressed at this point. For the rest, we will make issues out of each of them and get them addressed for 0.4.4

[Kubuxu]

Moved to 0.4.4 milestone.