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

> > + keep_this = False
> >
> > An alternative to this flag and the check at the end would be to `return
> None`
>
> return result you mean, but I see your point.
>
> ... crap !... it really returns None. That's bad :-( Yet another point to
> address ...

Should is also be a string of some sort? Is None intended to indicate unhandled?

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

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

> > The remainder of the logic added here looks sensible to me, but it's hard to
> > comprehend the exact implications.
>
> Yeah, I know :-/ The conflicts being handled in various parts under various
> circumstances is part of the problem. I don't have a good answer for that
> (yet) as it's strongly related to tree transform features.

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

> > Also, saying something about the what effect this fix has directly would be
> > good, as this is a user-facing bug. So, mention the exception that will no
> > longer be raised in which circumstances or similar.
>
> Done.

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