Code review comment for lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm

Most of these are genuinely trivial now -- if we're in agreement on them
all, LGTM, but ping me if there's any uncertainty

https://codereview.appspot.com/53210044/diff/100001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/charm.go#newcode87
state/charm.go:87: func CharmArchiveName(name string, revision int)
(string, error) {
quibble quibble whine, I think this probably belongs in apiserver/client

https://codereview.appspot.com/53210044/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode633
state/state.go:633: Assert: txn.DocExists,
hmm, should be be asserting that it hasn't had archiveurl (etc) filled
in? I think there's a very very cornery corner case in which we read
while it's still a placeholder, but write when it's already uploaded.

That'd be a good transaction-hooks test ;).

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode661
state/state.go:661: if err != nil {
this makes me twitch a little -- AFAICT the only possible value is
txn.ErrAborted, but that assumption feels very vulnerable to code
perturbations in the loop above. Maybe check for err == nil inside the
loop, and return newCharm() there, so that ErrAborted becomes the *only*
way to fall out of the loop, and just always return
ErrExcessiveContention?

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go
File utils/trivial_test.go (right):

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#newcode87
utils/trivial_test.go:87: {
}{{
     ...
},{
     ...

to compress both vertical and horizontal space?

And, hey, can we please just move these inside the test func? If they're
not used elsewhere they really don't deserve to be global IMO.

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#newcode105
utils/trivial_test.go:105: func (utilsSuite)
TestReadSHA256AndReadFileSHA256(c *gc.C) {
*utilsSuite, please, across the board, lest someone add methods that
require pointer receivers and silently deactivate all the other tests.

https://codereview.appspot.com/53210044/