On Wed, 2017-02-01 at 12:56 -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > Matt McCutchen <matt@xxxxxxxxxxxxxxxxx> writes:
> > ...
> > > Please check that my refactoring is indeed correct! All the
> > > existing tests pass
> > > for me, but the existing test coverage of these conflict messages
> > > looks poor.
> >
> > This unfortunately is doing a bit too many things at once from that
> > point of view. I need to reserve a solid quiet 20-minutes without
> > distraction to check it, which I am hoping to do tonight.
>
> Let me make sure if I understood your changes correctly:
>
> * conflict_rename_delete() knew which one is renamed and which one
> is deleted (even though the deleted one was called "other"), but
> because in the original code handle_change_delete() wants to
> always see tree A first and then tree B in its parameter list,
> the original code swapped a/b before calling it. In the original
> code, handle_change_delete() needed to figure out which one is
> the deleted one by looking at a_oid or b_oid.
>
> * In the updated code, the knowledge of which branch survives and
> which branch is deleted is passed from the caller to
> handle_change_delete(), which no longer needs to figure out by
> looking at a_oid/b_oid. The updated API only passes the oid for
> surviving branch (as deleted one would have been 0{40} anyway).
>
> * In the updated code, handle_change_delete() is told the names of
> both branches (the one that survives and the other that was
> deleted). It no longer has to switch between o->branch[12]
> depending on the NULLness of a_oid/b_oid; it knows both names and
> which one is which.
>
> * handle_modify_delete() also calls handle_change_delete(). Unlike
> conflict_rename_delete(), it is not told by its caller which
> branch keeps the path and which branch deletes the path, and
> instead relies on handle_change_delete() to figure it out.
> Because of the above change to the API, now it needs to sort it
> out before calling handle_change_delete().
>
> It all makes sense to me.
>
> The single call to update_file() that appears near the end of
> handle_change_delete() in the updated code corresponds to calls to
> the same function in 3 among 4 codepaths in the function in the
> original code. It is a bit tricky to reason about, though.
>
> In the original code, update_file() was omitted when we didn't have
> to come up with a unique alternate filename and the one that is left
> is a_oid (i.e. our side). The way to tell if we did not come up
> with a unique alternate filename used to be to see the "renamed"
> variable but now it is the NULL-ness of "alt_path".
"alt_path" is the same variable that used to be "renamed". I just
renamed it to be less confusing.
> And the way to
> tell if the side that is left is ours, we check to see o->branch1
> is the change_branch, not delete_branch.
>
> So the condition to guard the call to update_file() also looks
> correct to me.
All of the above matches my understanding. Would it have saved you
time if I had included some of this explanation in the patch "cover
letter"?
Matt