[Svnmerge] r22788: some discussion

[Luke Call]

Giovanni Bajo wrote:
> On ven, 2007-07-20 at 22:05 -0600, Luke Call wrote:
> 
>> The second one adds some error & status checking which make bugs more
>> clear and easier to find.  Someone better at python may see much better
>> ways: 
>> http://subversion.tigris.org/nonav/issues/showattachment.cgi/671/addErrCheckingToPulldomUsage.patch
> 
> I'm not sure what you are aiming at. You are basically trying to detect
> exceptions generated by missing attributes/nodes and shut them down. But
> why? 

Because if there is a typo in a name (or a missing attribute/node), the 
exception thrown is very unhelpful in actually finding the problem--it 
points you to somewhere unrelated because a later failure in acting on 
the empty string result.  I wanted to make it explicit and clear; but 
probably a test would be good enough to catch that instead, for any new 
code that might run into that, so I return None now as you suggested.

> OK but comments shouldn't be in docstring format. In python you use "#"
> for commands, and strings for docstrings. The two have different
> meanings and usages, even though they have the same net effect on the
> code (they are ignored).

That makes word wrap a nuisance, but I've changed it.

>> Also, action_init was taking a global value (opts["revision"]),
>> assigning to it, then checking conditions against it.  Instead, I
>> created a local variable to clarify the distinction between global and
>> local.  This way it is made clearer that opts["revision"] is unchanged
>> since it was set in main(), and not something the program may or may not
>> have changed later during execution.  The old way works, but it is
>> unclear and takes some study to determine why it is safe.
> 
> I agree with this change.
> 
>> I changed testUninitForce to check for regular expressions instead of
>> specific revision numbers, because the specific numbers changed but were
>> not the point of the test.
> 
> I agree with this change. It can be committed in a separate
> (pre-approved) patch if you want.

It should probably be in the same patch, otherwise the test fails, 
because of the patch.  To keep tests passing due to a change, is part of 
the same change.

> Luke, many thanks for your "passion" into this bug. I think we're almost
> there. I think next steps are:

My goal is to simplify usage for our developers, and I think it will 
help others also.  There are now two common (for us) scenarios (where 
either branch is a copy of the other) where the default behavior 
(without a revision list parameter) works as expected instead of 
requiring them to review logs and calculate rev numbers.  Also to make 
it easier for me to integrate future changes into our diverged 
svnmerge.py, if at least part of the divergence will have been committed 
upstream.  If others benefit, that is a most happy thought as well.

> 1) Patch #1 can be committed (Dustin, can you can take care of that?)
Thanks!!!  (And to Dustin for checking it in.)

> 2) Patch #2 should be revised as I suggested, by just returning None on
> optional attributes when they're not available.

Done; uploaded here: 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/675/addNullreturnsToPulldomUsage.patch

> 3) A separate patch for renaming branch->target (and doing only that) is
> pre-approved and can be committed as soon as it is submitted.

done; available here: 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/676/renameParametersToActionInit.patch

> 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


If this is approved I'll submit another small logic improvement that can 
save time for users.  And possibly redo the patch so tests pass on 
windows, but later.  (Dustin, if you have time for that last one that's 
super--I know you'd kindly offered in the issue notes to do a couple of 
the above items that are hopefully done now.)

Many thanks to both of you, regardless, though I admit it has been 
frustrating.  I want to work now on things now that are broken, rather 
than perfecting patches that do work, but which will entirely go away 
fairly soon.  :)

Thanks,

-Luke