[Svnmerge] r22788: some discussion

[Giovanni Bajo]

On ven, 2007-07-20 at 22:05 -0600, Luke Call wrote:

> The first one makes use of the xml routine you provided, and is
> available here: 
> http://subversion.tigris.org/nonav/issues/showattachment.cgi/670/usePulldomNotXml.patch

This is OK to commit. Many thanks for completing my work!

> 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? If you're looking for an attribute/node that does not exist, there
are two options:

1) you know that the attribute/node MUST exist. In your current
knowledge, there is no reason why it shouldn't exist. Thus, no check is
needed; if your assumptions are incorrect, let it die with an exception.

2) you know that the attribute/node is optional. In that case, either
add a function to check whether it exists or not, or (better) make it
return None when the attribute/node is not present. 

[[ The correct Python way to achieve what you were trying to do, though,
is to define your own exception (say, "DomMissingAttributeException")
and raise that one when you meet the error codition. You just need to
define a class that derives from Exception. Later, you can catch that
specific type and act accordingly. ]]


> The third one tries to finish the work
> started by r22788 on making action_init behavior more helpful.  It is
> available here: 
> http://subversion.tigris.org/nonav/issues/showattachment.cgi/672/furtherInitLogicFixesAsBegunByr22788.patch

Wow, this patch looks MUCH better now than it used to be :) I am happy
we can get away without regexp tricks now!

> I added some comments to clarify this part of action_init, because it 
> takes a while to figure at first.

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

> I went ahead and changed branch_dir and branch_props to target_dir and
> target_props.  I realize variable name changes are generally better done
> in a separate patch.  In this case a primary goal was clarifying this
> code, which has a tricky job to do.  Since I had added comments and new
> code where it was important to call things "target*" for clarity, this
> other change seemed necessary to avoid confusion, in order to refer
> consistently to source and target, rather than to source and branch.  I
> will take out that change if preferred.

It's not because we're anal, really. It's just that *any* extraneous
strange makes patch reading much harder. So what happens is that one
reviewer looks at the patch, see many unrelated changes and think: "hey
this is complex to read; I'll review it later when I have more time".
And unsurprisingly "later" never arrives :)

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

> I removed testCheckInitializeEverything, because it tested for the
> earlier behavior; the new test, testInitScenarios, covers the same
> ground plus more.  I.e., testCheckInitializeEverything previously
> verified that test-branch would be initialized up to the latest
> repository revision, or 1-13.  But really, since test-branch was copied
> from trunk, it should be initialized to 1-6, because test-branch was
> created by copying from trunk revision 6, and I think we would want
> svnmerge.py to get all possible changes on trunk since that point, in
> the general case.  That is what it does now.

I agree with this change.

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

1) Patch #1 can be committed (Dustin, can you can take care of that?)
2) Patch #2 should be revised as I suggested, by just returning None on
optional attributes when they're not available.
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.
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.
-- 
Giovanni Bajo
Develer S.r.l.
http://www.develer.com