[Svnmerge] r22788: some discussion

[Luke Call]

Luke Call wrote:
> Giovanni Bajo wrote:
>>>> 4) Patch #3 needs only minor nits (as reviewed here). I want to
>>>> re-review it when it's finished, but I don't foresee any specific
>>>> problem. Also please submit this one in context diff format (if
>>>> possible) as the unified format isn't clear in this case.
>>> Done; now available here (context format): 
>>> http://subversion.tigris.org/nonav/issues/showattachment.cgi/677/furtherInitLogicFixesAsBegunByr22788-take3-unifiedFormat.patch 
>>>
>>>
>>> ..and here (unified format): 
>>> http://subversion.tigris.org/nonav/issues/showattachment.cgi/678/furtherInitLogicFixesAsBegunByr22788-take3-contextFormat.patch 
>>
>> OK, approved, with one minor correction: the comments in action_init() 
>> still speaks of "branch" and "trunk". You should updated them to speak 
>> about "source" and "target", otherwise they don't match anymore with the 
>> code.
> 
> While I preferred not to have the variables say "trunk" and "branch" 
> because it is not limited to that, using those words in comments helps 
> the explanation because that is already a common idiom in the svn 
> documentation. For that reason the comments already have *both* 
> nomenclatures.  It can take a little longer to visualize what's going on 
> when it only says "the copy target is the merge source, and the copy 
> source is the merge target".  The most possible reader help is important 
> to strive for since this part of the code has been so confusing to folks 
> in the past:  multiple developers and reviewers have got it wrong or had 
> to work hard to follow it, because this code is doing a non-obvious task.
> 
> Is there any specific suggested wording that would make it harder for a 
> new reader to mistake what's going on, and easier to read quickly...?

Here's an updated patch that changes those comments to remove 
trunk/branch, in case that is truly preferred:

3) 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/685/furtherInitLogicFixesAsBegunByr22788-take4-contextFormat.patch

With this, perhaps it and the others you approved can now be checked in?

1) 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/675/addNullreturnsToPulldomUsage.patch
2) 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/676/renameParametersToActionInit.patch


Thanks,

Luke