[MERGE][bug #297831] don't call chdir('')

Martin Pool mbp at canonical.com
Thu Nov 20 02:36:55 GMT 2008


On Thu, Nov 20, 2008 at 4:51 AM, Aaron Bentley <aaron at aaronbentley.com> wrote:
>> So I actually disagree with this, as "if path" can be translated into a
>> very cheap "PyObject_True(path)" call, rather than having to create a
>> temporary string object for "", and then call PyObject_Compare(path,
>> tmp_obj).
>
> If you can demonstrate that this has a significant impact on
> performance, I'll let it in as-is.

I think whether it's faster or not is beside the point.  (Anyhow, it's
highly unlikely either comparison is measurable next to the system
calls that happen here.)

This review, though I'm sure it's conducted with the best intentions
from everyone, is getting a bit into the antipattern of picking nits
when there is more important stuff to do.  I think the best reviews
are those where it ends up with the original author saying "oh I
hadn't thought of that", rather than carrots and sticks to get the fix
in.  I probably would have written it as Aaron suggests but let's not
have 12 mails about it.

Personally, looking at this patch, I would ask: why are we even doing
readdir(""), that seems a bit strange by unix standards, and I would
have been inclined to define this method as not accepting it, then fix
the callers.  On the other hand, it's a pretty easy fix in this
method, and possibly easier or cleaner than specially transforming
dirname('a') => '.' .

But the main thing is, it's a reasonable correct improvement, let's
just merge it.

-- 
Martin <http://launchpad.net/~mbp/>



More information about the bazaar mailing list