[Svnmerge] Re: svn commit: r18649 - trunk/contrib/client-side

[Giovanni Bajo]

David James <djames at collab.net> wrote:

>>> --- trunk/contrib/client-side/svnmerge.py (original) +++
>>> trunk/contrib/client-side/svnmerge.py Tue Feb 28 13:25:28 2006 @@
>>>      -631,7 +631,7 @@ # the --verbose flag, the --quiet flag
>>>      prevents the commit log # message from being printed.
>>>      log_opts = '--quiet -r%s:%s "%s"' % (begin, end, url)
>>> -    if opts["bidirectional"]:
>>> +    if opts.has_key("bidirectional") and opts["bidirectional"]:
>>>          log_opts = "--verbose " + log_opts
>>>      lines = launchsvn("log %s" % log_opts)
>>
>>> @@ -679,7 +679,7 @@
>>>      phantom_revs = RevisionSet("%s-%s" % (begin, end)) - revs
>>>      reflected_revs = []
>>>
>>> -    if opts["bidirectional"]:
>>> +    if opts.has_key("bidirectional") and opts["bidirectional"]:
>>>          report("checking for reflected changes in %d revision(s)"
>>>                 % len(prop_changed_revs))
>>
>> This is also pretty weird, as opts should automatically acquires
>> defaults for every option through the Option machinery. Did you
>> investigate why it's not needed for any other option?
>
> The bidirectional option is not valid for "svnmerge block", so it does
> not have a default in this case. For this reason, we're forced to deal
> with the possibility that the "bidirectional" key may not exist at
> all.

Well, the point is why "svnmerge block" uses code which depends on the
bidirectional flag, yet the flag is invalid for it. In fact, "svnmerge block"
*does* change its behaviour depending on the "bidi" flag: in fact, if
opts["bidirectional"] is True, reflected revs are automatically filtered away
from the block list. If opts["bidirectional"] is False, reflected revs might go
into the block list. It doesn't really change much, but still. I guess we can
live with the default for the flag, whichever it happens to be.

I guess the correct solution is to put defaults for all the common options
within the state dictionary, no matter which ones are actually valid for the
current command. The code that sets the defaults is currently in _fancy_getopt.
Can you move it into parse() (near the beginning) and run it for all the global
options (self.gopts) + all the common options (self.copts)? That should make
it. Or get back to me and I'll try to fix it myself.

Thanks!

Giovanni Bajo