Code review comment for lp:~parthm/bzr/no-chown-if-bzrlog-exists

Couple of style points, one of which causes a bug.

Generally you want the body of try/except loops to be as minimal, hence my use of else in the example before. I have mixed feelings about nested functions that aren't actually being used as closures. Factoring common logic branches into one route is generally a good thing, even if it means using break-as-goto.

Rather than things along the lines of:
     permissions = 0666
     fd = os.open(filename, flags, permissions)
it's better to use:
     fd = os.open(filename, flags, mode=0666)
which is still self-documenting, and avoids the variable.

As you do that with flags too, you're defeating the race protection of the loop by making all attempts after the first exclusive, potentially causing an infinite loop instead:
    flags = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
    while True:
        ...
            fd = os.open(filename, flags)
        ...
            flags = flags | os.O_CREAT | os.O_EXCL
            permissions = 0666
            fd = os.open(filename, flags, permissions)

Also bt.test_trace.test__open_bzr_log_uses_stderr_for_failures was failing, which can be fixed by moving a later catch one step up the exception hierarchy.

Pull <lp:~gz/bzr/no-chown-if-bzrlog-exists> for my fixes for these. I've poked a couple of other minor bits while I was there, but didn't do anything major. (I'd really like windows to avoid this codepath entirely, but this doesn't need to be done right now.)