Bazaar

Merge lp://staging/~doxxx/bzr/bug939605 into lp://staging/bzr

bug939605
Merge into bzr.dev

Proposed by Gordon Tyler on 2012-02-24

Status:

Rejected

Rejected by:

Martin Packman on 2012-02-29

Proposed branch:

lp://staging/~doxxx/bzr/bug939605

Merge into:

lp://staging/bzr

Diff against target:

155 lines (+53/-21)

5 files modified

bzrlib/mergetools.py (+16/-7)
bzrlib/osutils.py (+12/-9)
bzrlib/tests/test_mergetools.py (+14/-5)
bzrlib/tests/test_osutils.py (+8/-0)
doc/en/release-notes/bzr-2.6.txt (+3/-0)

To merge this branch:

bzr merge lp://staging/~doxxx/bzr/bug939605

Medium

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Martin Pool		Approve on 2012-02-27
Martin Packman (community)	2012-02-25	Needs Information on 2012-02-27
Alexander Belchenko	2012-02-24	Approve on 2012-02-24
Review via email: mp+94488@code.staging.launchpad.net

Description of the change

Fixes bug 939605 by updating osutils.find_executable_on_path to additionally search the Windows App Path registry.

The check_availability and invoke functions in mergetools use a small utility function, _get_executable_path, to get the full path to the executable in the mergetool commandline, which skips the PATH search if the path is already absolute.

Revision history for this message

Alexander Belchenko (bialix) wrote on 2012-02-24:

Looks OK for me, though I can't comment on your change to existing test. Maybe second voice from core dev is needed. I can say that I've tested it and it works. Thank you for fixing it so fast.

Also, I'd like to have this patch backported to bzr 2.5.1, because it clearly fixed bug and don't change anything public.

And you need NEWS message, I guess ;-)

review: Approve

Revision history for this message

Gordon Tyler (doxxx) wrote on 2012-02-24:

Darn NEWS...

Revision history for this message

Gordon Tyler (doxxx) wrote on 2012-02-24:

Release notes updated.

Revision history for this message

Martin Packman (gz) wrote on 2012-02-27:

Wouldn't it be better to fold the logic you've added in _get_executable_path into osutils.find_executable_on_path? If we need a look-before-you-leap executable finder, we may as well have a single one that does the right thing rather than needing extra special cases on top. I wouldn't worry about that making the '_on_path' naming a little misleading, it's hardly the biggest problem is osutils.

For the test you could override the PATH envvar for the duration of the test. Just %SystemRoot% should do.

review: Needs Information

Revision history for this message

Gordon Tyler (doxxx) wrote on 2012-02-27:

Since Win32's ShellExecute function searches the App Path, it makes sense to include it in osutils.find_executable_on_path.

That's a good point about the SystemRoot env var. Since '%SystemRoot\system32' is guaranteed to be in the PATH, I can use something like cmd.exe to test that and use the env var to construct the path to test against.

Thanks.

Revision history for this message

Gordon Tyler (doxxx) wrote on 2012-02-27:

Updated according to Martin's suggestions.

Revision history for this message

Martin Pool (mbp) wrote on 2012-02-27:

looks reasonable to me too, and +1 more to putting this in 2.5

review: Approve

Revision history for this message

Martin Packman (gz) wrote on 2012-02-29:

Superseded by proposal targeted at 2.5:

<https://code.launchpad.net/~doxxx/bzr/bug939605-2.5/+merge/94909>

Unmerged revisions

6481. By Gordon Tyler on 2012-02-27: Re-removed unnecessary import.
6480. By Gordon Tyler on 2012-02-27: Improved readablity of check_availability.
6479. By Gordon Tyler on 2012-02-27: Removed unnecessary import.
6478. By Gordon Tyler on 2012-02-27: Updated mergetools._get_executable_path and improved test_invoke_expands_exe_path.
6477. By Gordon Tyler on 2012-02-27: Changed osutils.find_executable_on_path to search using win32utils.get_app_path if the executable is not found using the PATH env var.
6476. By Gordon Tyler on 2012-02-24: Updated release notes.
6475. By Gordon Tyler on 2012-02-24: Added _get_executable_path function to mergetools module, which searches the PATH and App Path registry on win32.
Changed check_availability and invoke to use _get_executable_path.
Updated tests.