Code review comment for lp:~doxxx/bzr/mergetools-commands

Revision history for this message
Gordon Tyler (doxxx) wrote :

On 12/6/2010 7:22 PM, Gordon Tyler wrote:
> Since I'm using string.format to do filename substitution, you're right
> that this can be done with a single string commandline form.

Although, the way you're doing it in your branch is fine too. So I don't
think I'll change that.

>> In is_available, you check only for the existence of the file and
>> not whether the x bit is is set. There is a grey area there that
>> deserves at least a FIXME comment or better, a fix with proper
>> multi-platform tests (I won't block on that as long as it's
>> documented (i.e. a FIXME will do)).
>
> Right. I should use the same os.access call that find_executable_on_path
> uses.

This actually turned out to be more complicated than I thought since
os.access(path, os.X_OK) returns True for all extant files on win32. But
I believe I have it solved now.

« Back to merge proposal