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

>>>>> "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.

    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.