Code review comment for lp:~vila/bzr/284038-push-strict

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> To start with, I don't remember an agreement that strict
    jam> should be the default.

https://bugs.edge.launchpad.net/bzr/+bug/322808/comments/2
https://bugs.edge.launchpad.net/bzr/+bug/65286/comments/1
https://bugs.edge.launchpad.net/bzr/+bug/65286/comments/2

I interpreted that as an agreement, I also agree with the idea
that it may be confusing for new users if left off by default.

    jam> + The ``push_strict`` option can be declared in a configuration
    jam> + file. option is used.

    jam> ^- The news entry needs a bit of updating. Something like:

    jam> The configuration option ``push_strict`` can be used to set the default
    jam> for this behavior.

Right.

    jam> ...

    jam> + if (tree is not None and revision_id is None
    jam> + and (strict is None or strict)): # Default to True:
    jam> + changes = tree.changes_from(tree.basis_tree())
    jam> + if changes.has_changed() or len(tree.get_parent_ids()) > 1:
    jam> + raise errors.UncommittedChanges(tree)

    jam> Rather than doing 'strict is None', just set:

    jam> def run(..., strict=True):

    jam> if we want that to be the default.

No, because if we do that, we can't distinguish anymore between
the config file variable and the command-line option (the later
should still override the former).

    jam> So provided that there was actually agreement that strict should be the
    jam> default, the patch seems ok with some small tweaks.

    jam> I'll note that "commit" is not strict by default, and
    jam> I've often used push before commit, especially if I'm
    jam> about to switch.

Feel free to add 'push_strict=False' in your bazaar.conf file,
you're not a *new* user :-)

    jam> I could agree that strict=True is the 'safer' default
    jam> for most people, so having to edit a config to turn it
    jam> off is the "power-user" approach.

Indeed.

Since my other merge proposal (for send/bundle --strict) will
raise, at least, the exact same questions, I'll wait for your
answer here before landing.

        Vincent