store: add PublishBazaarBranch function

https://code.launchpad.net/~niemeyer/juju/master/+merge/93330

(do not edit description out of merge proposal)

[niemeyer, 2012-02-16 03:53:32]

Please take a look.

[rog, 2012-02-16 10:05:27]

looks good. only superficial comments.

https://codereview.appspot.com/5674066/diff/1/store/branch.go
File store/branch.go (right):

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode18
store/branch.go:18: // revision id for the branch tip. In case publishing this
specific
s/In case/If/

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode22
store/branch.go:22: // differ from the provided parameter.
i had to look twice here to see what "provided parameter" is referring to.
perhaps s/provided parameter/digest parameter/

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode23
store/branch.go:23: func PublishBazaarBranch(store *Store, urls []*charm.URL,
burl string, digest string) (err error) {
no need to name the return value here, i think.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode36
store/branch.go:36: // be the revision assigned to the charm, and it will also
fail in
i don't quite get the meaning of the second sentence here.
perhaps "This will compute the revision to be assigned to the charm," ?

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode37
store/branch.go:37: // case the operation is unnecessary because charms are
up-to-date.
s/in case/if/ :-)

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode40
store/branch.go:40: return
be consistent: return err.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode53
store/branch.go:53: if branchDir == "" {
given that PublishBazaarBranch is quite long, i wonder if this section might not
be better factored out into a separate function, which could make the unusual
control flow a little clearer.
for example:
func getBranch(burl string) (branchDir string, tipDigest string, err error)

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode98
store/branch.go:98: // since the revision was properly published before.
does this want a test?

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode144
store/branch.go:144: return fmt.Errorf("%v\n%s", err, output)
this might be awkward if there's a lot of output.

https://codereview.appspot.com/5674066/diff/1/store/branch_test.go
File store/branch_test.go (right):

https://codereview.appspot.com/5674066/diff/1/store/branch_test.go#newcode122
store/branch_test.go:122: type bzrDir string
nice

https://codereview.appspot.com/5674066/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/5674066/diff/1/store/store.go#newcode275
store/store.go:275: // data imported. This is typicall set to the VCS revision
digest.
s/typicall/typically/

[niemeyer, 2012-02-17 18:09:46]

https://codereview.appspot.com/5674066/diff/1/store/branch.go
File store/branch.go (right):

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode18
store/branch.go:18: // revision id for the branch tip. In case publishing this
specific
On 2012/02/16 10:05:27, rog wrote:
> s/In case/If/

Done.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode22
store/branch.go:22: // differ from the provided parameter.
On 2012/02/16 10:05:27, rog wrote:
> i had to look twice here to see what "provided parameter" is referring to.
> perhaps s/provided parameter/digest parameter/

Done.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode23
store/branch.go:23: func PublishBazaarBranch(store *Store, urls []*charm.URL,
burl string, digest string) (err error) {
On 2012/02/16 10:05:27, rog wrote:
> no need to name the return value here, i think.

Done.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode36
store/branch.go:36: // be the revision assigned to the charm, and it will also
fail in
On 2012/02/16 10:05:27, rog wrote:
> i don't quite get the meaning of the second sentence here.
> perhaps "This will compute the revision to be assigned to the charm," ?

Done.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode37
store/branch.go:37: // case the operation is unnecessary because charms are
up-to-date.
On 2012/02/16 10:05:27, rog wrote:
> s/in case/if/ :-)

Done.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode40
store/branch.go:40: return
On 2012/02/16 10:05:27, rog wrote:
> be consistent: return err.

Done.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode53
store/branch.go:53: if branchDir == "" {
On 2012/02/16 10:05:27, rog wrote:
> given that PublishBazaarBranch is quite long, i wonder if this section might
not
> be better factored out into a separate function, which could make the unusual
> control flow a little clearer.
>
> func getBranch(burl string) (branchDir string, tipDigest string, err error)

It'd also need the tempDir to do the RemoveAll defer outside. That means pretty
much all the data inside the function is relevant to the outside function.
Moving the few remaining lines and the comments there would make things less
comprehensible rather than improve the situation.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode98
store/branch.go:98: // since the revision was properly published before.
On 2012/02/16 10:05:27, rog wrote:
> does this want a test?

This is behavior is properly verified in the store tests where we can manipulate
the ordering more easily. It's hard to test it in that outer layer because we
have to emulate the concurrency that the function is prepared to prevent, as the
comment describes.

https://codereview.appspot.com/5674066/diff/1/store/branch.go#newcode144
store/branch.go:144: return fmt.Errorf("%v\n%s", err, output)
On 2012/02/16 10:05:27, rog wrote:
> this might be awkward if there's a lot of output.

Indeed, but there's no reason for bzr to produce enough output for that to be
anywhere close to being an issue, and we do want that output for debugging
purposes.

https://codereview.appspot.com/5674066/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/5674066/diff/1/store/store.go#newcode275
store/store.go:275: // data imported. This is typicall set to the VCS revision
digest.
On 2012/02/16 10:05:27, rog wrote:
> s/typicall/typically/

Done.

[niemeyer, 2012-02-17 18:18:48]

Please take a look.

[rog, 2012-02-17 18:22:36]

On 2012/02/17 18:18:48, niemeyer wrote:
> Please take a look.

LGTM

[niemeyer, 2012-02-19 09:09:37]

*** Submitted:

store: add PublishBazaarBranch function

R=rog
CC=
https://codereview.appspot.com/5674066

[fwereade, 2012-02-21 11:49:24]

LGTM