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 _______________________________________________ Svnmerge mailing list [email protected] http://www.orcaware.com/mailman/listinfo/svnmerge
