launchpad-reviewers team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Gary wrote:

> Using get to update an existing checkout is interesting, and I'm not sure it
> is the right thing.  That's the intended domain of update, which also is
> supposed to shelve changes before the pull and unshelve them after the pull.
> I suggest that we rip that behavior out from get.  (Note that IMO we can also
> rip the branch command too, if that makes anything easier; but that can wait
> till another branch too.  Note further that we can rip out the install
> command, but that's trickier because lxcinstall defers to it ATM, so I
> definitely think that ripping install out needs to be its own branch.)

I agree with Gary: get should not update existing branches; it should call
the update command at the end of the process. Note that the update command is
not yet implemented: the current update command (intended only as a replacement
for rocketfuel-update) must be deleted or heavily modified.
 
> I was curious about the "Repository {0} is corrupted" stuff when I saw it
> too...did you check with frankban before you removed it?  If not, please do.
> I think either removing it or commenting on why we think it is necessary are
> both good possible directions.

I've taken that check from rocketfuel-setup. We have no control on how bzr handles
things, and a sanity check after a 40 min process seemed not too bad IMHO.

> ISTR someone was going to check on why we have --2a as an argument.  That
> should be the default for all versions of bzr that we encounter.  If it is
> not, I think a comment is appropriate.  Otherwise, let's remove it.

Same as above: rocketfuel-setup uses that option, and maybe at the time it was written, 
for some reason, it made sense. It seems it doesn't now so +1 on removing --2a.

> I skimmed the rest, and it looked good.  I'm not going to approve pending
> changes because I think there may need to be a discussion about one or the
> both of inferring the "lightweight" value and ripping out get's "update"
> behavior.  If someone else wants to take over the rest of the review, that
> would be cool.

Anyway, the branch is good, I am waiting for this to be merged and I am inclined to
make it happen as soon as possible. We could create another card for fixing remaining things:

- checkout option (default='sandbox')
- update stuff
- remove unneeded args (ssh args, parallel tetsting args, email, full_name etc...)




-- 
https://code.launchpad.net/~benji/lpsetup/bug-1016645-add-command-line-options/+merge/112588
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/lpsetup/bug-1016645-add-command-line-options into lp:lpsetup.