Code review comment for lp:~vila/bzr/880701-resolve-duplicate

> > ... crap !... it really returns None. That's bad :-( Yet another point to
> > address ...
>
> Should is also be a string of some sort?

Probably.

> Is None intended to indicate unhandled?

I think it's a hole in the implementation. It doesn't really matter for now
but it's weird that it's not more related to whatever the hooks return.

>
> > Anyway, I don't really agree with returning early as this method is already
> > very hard to read and I'm afraid adding early returns, while locally making
> > it more understandable, will make it harder to grasp globally.
> >
> > It's 120 lines long and really need a better refactoring that I don't want
> > to do now :-/
>
> It's reasonable to avoid funny control flow in a function this large, the
> flags are at least quite obvious.

Good, that was the main intent even if I still think this method should be
split into more understandable parts.

>
> > Also, there is:
> >
> > if not self.this_tree.has_id(file_id) and result == "modified":
> > self.tt.version_file(file_id, trans_id)
> >
> > that is still needed (or may be not but the fact that I can't tell is
> > another indication we still have more work to do here).
>
> That result being None thing for those two cases skips this currently, which
> is correct as versioning file_id already happens in your new code when it's
> required.

Damn, you're right ! More evidence this method is hard to grasp.

> Ideally I think we want someone else to look at this too, but we are rather
> short of people currently.

As mentioned on IRC, I'm confident this introduces a special case for
'content conflicts' *only* and those were leading to a 'marlformed
transform'.

> You confused me for a bit there by having two entries under bug fixes now,
> but the new wording is a definite improvement.

Sorry. I should have put those two entries from the start since they address
the two issues mentioned in the bug report. Thanks for helping clarify them anyway.

I'm now waiting for 2.4.3 to open to land there and then in bzr.dev.