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

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

Vincent Ladeuil wrote:
>>>>>> "jam" == John A Meinel <email address hidden> writes:
>
> <snip/>
>
> jam> Also...
>
> jam> We should *definitely* be checking:
>
> jam> tree.last_revision() == tree.branch.last_revision()
>
> jam> And possibly consider what to do in the case of bound
> jam> branches and heavyweight checkouts. I'm not *as*
> jam> concerned about the latter, but because in the former
> jam> case, IIRC we will actually be pushing
> jam> "tree.branch.last_revision()" *not*
> jam> tree.last_revision()...
>
> Hmm, so far --strict is defined as triggering if there are
> *uncommitted* changes, you're stretching the definition here...

So I can agree that it is a different bug except:

1) I think 'bzr push --strict' is about 'push should push the state I
*think* I have'. And uncommitted changes, (uncommitted merges), and a
tree last revision being out of sync with the branch's revision, are all
cases where the push isn't pushing what the user expected.

2) I think the source of '--strict' is MySQL fallout from switching from
BK where a merge had autocommit so "bk merge; bk push" did something,
while with bzr you need "bzr merge; bzr commit; bzr push".

The fact that they haven't run into issues with a workingtree being out
of sync with the branch is more luck than noticing that we have a
serious flaw.

3) The fix is fairly trivial, the bug is somewhat serious, and you're
already working on the code. Yes it is feature creep, but it is a small
amount for something worthwhile (IMO).

>
> jam> So certainly '--strict' should fail if the tree and
> jam> branch aren't at the same rev.
>
> I'm not sure I agree with you (nor that I disagree), but I think
> that's worth filing a different bug.
>
> Vincent
>

I read your links, and while there wasn't a general discussion on it,
Martin at least seems to agree it should be the default. I'm just a bit
extra sensitive to commandline behavioral changes. Since suddenly
something that used to work won't anymore.

Also, note that "UncommittedChanges" has no help text to tell the user
that they can use "--no-strict" in order to push anyway. I don't know if
that matters or not. But since we are explictly changing a behavior, it
is good to help users find out how to get their old behavior back.

Anyway, you can probably merge this as is, though I'd like it if you
added a tree.last_revision() check.

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

iEYEARECAAYFAkpI7LsACgkQJdeBCYSNAANHawCgzbfvzWlHnaFF07twKpOsj9Dd
dioAoLoZtL3GnDb1V05wi0kQyjDaeE0y
=a+Cd
-----END PGP SIGNATURE-----