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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/284038-push-strict into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This complements my first attempt to fix the bug while also fixing bugs #322808 and #65286,
> by making --strict defaults to true.
>
> Changing the default value revealed a bug in the previous implementation as push can be used without
> a working tree.
>
> Also the --strict option is meaningless if the --revision option is used.
>
> Previous reviews mentioned introducing:
> - a way to query config files for a bool option,
> - a new method on working tree for checking changes.
>
> Since both are also needed for bug #206577, I'd like to postpone these modifications until both fixes
> have landed.
>

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

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

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

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

...

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

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

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

if we want that to be the default.

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

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

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

I suppose we could land it and see what we think...

 vote needs_fixing

Mostly I want to make sure that we did actually agree that we should
default to strict.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpIwN8ACgkQJdeBCYSNAAOuRQCdEXhhM7Z9KVXOjQZYAMKJPpYV
S9EAoJ90HFDdgbzP7/JRf4JM4YxO4B0N
=uiFO
-----END PGP SIGNATURE-----