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

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

Vincent Ladeuil wrote:
>>>>>> "jam" == John A Meinel writes:
>
> jam> Vincent Ladeuil wrote:
> >>>>>>> "jam" == John A Meinel 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...
>
> jam> So I can agree that it is a different bug except:
>
> jam> 1) I think 'bzr push --strict' is about 'push should
> jam> push the state I *think* I have'.
>
> I understood that :)
>
> jam> And uncommitted changes, (uncommitted merges), and a
> jam> tree last revision being out of sync with the branch's
> jam> revision, are all cases where the push isn't pushing
> jam> what the user expected.
>
> Right. You're right. I agree with the rationale, but... I don't
> clearly understand when a user can find itself in this
> situation... I can see one case for a bound branch, except that I
> can't commit in that situation so I have nothing to push... Or
> maybe you mean a commit --local ? But in that case push should
> already fail with "diverged branches" (strict or no strict) no ?
>
> jam> 2) I think the source of '--strict' is MySQL fallout
> jam> from switching from BK where a merge had autocommit so
> jam> "bk merge; bk push" did something, while with bzr you
> jam> need "bzr merge; bzr commit; bzr push".
>
> <snip/>
>
> I agree here too, mysql workflow wasn't my main reason for
> agreeing about making --strict the default. In fact my first fix
> addressed mysql workflow *without* making it he default.
>
> I think the default should be strict because uneducated users
> will easily forget to commit before pushing, and even experienced
> ones may prefer, for some branches, to work in strict mode.
>
> jam> 3) The fix is fairly trivial, the bug is somewhat
> jam> serious, and you're already working on the code. Yes it
> jam> is feature creep, but it is a small amount for something
> jam> worthwhile (IMO).
>
> I don't have a problem with working more on that, but I just
> don't know how to write the tests (the what, not the how).
>
> jam> I read your links, and while there wasn't a general
> jam> discussion on it, Martin at least seems to agree it
> jam> should be the default.
>
> Sorry to have jumped to that conclusion without discussing it
> first then.
>
> jam> I'm just a bit extra sensitive to commandline behavioral
> jam> changes. Since suddenly something that used to work
> jam> won't anymore.
>
> Well, I thought about that and I came to the conclusion that it's
> pretty rare than one find itself in the situation to push while
> having uncommitted changes. At least to me, it sounds like
> advanced usage and I don't mind having to add an option in that
> case, even if I didn't have to do it before, especially since I
> can configure that behavior at any level.

bzr co --lightweight branch wt1
bzr co --lightweight branch wt2
cd wt2
bzr commit -m "commit from wt2"

cd ../wt1

bzr st

# Should give a "working tree is out of date" error

At this point the "fix" isn't 'bzr commit' but something more like "bzr
update", to get you back in sync.

So yes, it isn't UncommittedChanges, it is "tree not in sync" or
something along those lines.

>
> jam> Also, note that "UncommittedChanges" has no help text to
> jam> tell the user that they can use "--no-strict" in order
> jam> to push anyway. I don't know if that matters or not. But
> jam> since we are explictly changing a behavior, it is good
> jam> to help users find out how to get their old behavior
> jam> back.
>
> Hmm, good point. First they need to be directed to 'bzr status'
> and then to 'push --no-strict' though. I'll add something in that
> direction before landing then, feel free to propose a better one,
> there will be a follow-up submission obviously.
>
> jam> Anyway, you can probably merge this as is, though I'd
> jam> like it if you added a tree.last_revision() check.
>
> Then let's discuss that after I land that patch because I just
> don't know how to address your concern otherwise.

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

iEYEARECAAYFAkpKFZoACgkQJdeBCYSNAAPT0QCeI8E9lL2iQtfy72ZBC599VgJM
274An3+/kZudjbJoB7fD21xfBRT/2RW0
=jTiq
-----END PGP SIGNATURE-----