Code review comment for lp:~vila/bzr/conflict-manager

>>>>> "Aaron" == Aaron Bentley <email address hidden> writes:

    Aaron> This looks a bit weird to me. Haven't done a full
    Aaron> review, but for two-way conflicts, bzr takes the
    Aaron> changes from OTHER, so keep_mine should never be a
    Aaron> no-op, and take_their should often be a no-op.

I didn't touch the code that generate the conflicts, so any
incoherency there was there. The overall code is already hard to
modify so I don't intend to change that sort of thing, yet.

But, yes, I was surprised to find that we don't always act the
same way when faced with conflicts and I'd like to provide a more
"regular" behavior (like using .THIS .OTHER instead of .new or
.diverted for example).

Also, if there is a conflict, it sounds acceptable that *both*
keep_mine and take_theirs should not be no-ops... with the
down-side that this will left the working tree in a worse state
than choosing between THIS or OTHER.

I'd welcome any feedback about why we do things the way they are
done today !

    Aaron> take_their should actually be take_theirs. ("my" is
    Aaron> to "their" as "mine" is to "theirs".)

Damn, I wrote it --take-theirs first :)

    Aaron> Also, the safest way to rename multiple files at once
    Aaron> is using a TreeTransform.

Thanks. I think there is a single place that deserves that, I'll check.

    Aaron> There are some grammar issues in the proposed new
    Aaron> text:

Thanks, I addressed them in my branch and only answer other
points below.

<snip/>

    Aaron> +Various actions are available depending on the kind of conflict, for some of
    Aaron> +these actions, Bazaar can provide some help. In the end you should at least
    Aaron> +inform Bazaar that you're done with the conflict with::
    Aaron> +
    Aaron> + ``bzr resolve FILE --action=done'
    Aaron> +
    Aaron> +Note that this is the default action when a single file is involved so you can
    Aaron> +simply use::
    Aaron> +
    Aaron> + ``bzr resolve FILE``

    Aaron> Why tell them to use --action=done and then
    Aaron> immediately contradict yourself?

Because the former is explicit why the later is implicit (and
don't contradict each other). I try to always use the explicit
form in docs.

    Aaron> And why --action=done instead of --done ?

Same.

    Aaron> +When you have resolved text conflicts, just run ``bzr
    Aaron> +resolve --auto``, and Bazaar will auto-detect which
    Aaron> +conflicts you have resolved.

    Aaron> Requiring --auto is a regression IMO.

Hehe, this text doesn't imply that --auto is *required* it just
mention it explicitly (same rule as above).

Well, I didn't change the behavior of --auto (finally), but I
intend to. The code that still implements the magic behavior of
--auto. But as said, I find that behavior pretty surprising and
I think we should get rid of it.

141 + # FIXME: There is a special case here related to the option
142 + # handling that could be clearer and easier to discover by
143 + # providing an --auto action (bug #344013 and #383396) and
144 + # make it mandatory instead of implicit and active only
145 + # when no file_list is provided -- vila 091229

If this patch is accepted then --auto can become an action as
--done but with a slightly different behavior (as it is today)
and a larger applicability (I added notes in this patch about
which other conflict types may (could ? :) be taken into
account).

      Vincent