Code review comment for lp:~parthm/bzr/81689-win-symlink-warning

Revision history for this message
Parth Malwankar (parthm) wrote :

Thanks Jelmer.
The patch is tested on Windows and Linux and seems to work well.

One change that looks somewhat risky at first glance is that mutabletree.has_changes now has a "fast path" (the original) which merely checks if any change exists in the list and a "slow path" that actually filters our symlinks on platforms that don't support windows. This was required to get merge to work correctly. As Windows, would use "slow path" with this change, I tried a basic merge benchmark with the bazaar branch. The performance for slow and fast path seems comparable at least with the small set of changes I did as a test. Logs for the benchmark are below.

Would very much appreciate the inputs on this patch.

Regards,
Parth

D:\ext-src\bzr.dev\foo>c:/python27/python.exe d:/ext-src/bzr.dev/81689-win-symlink-warning/bzr merge ..\bar
 M BRANCH.TODO
 M INSTALL
 M MANIFEST.in
 M Makefile
 M TODO
 M profile_imports.py
 M setup.py
All changes applied successfully.
bzr: warning: some compiled extensions could not be loaded; see <https://answers.launchpad.net/bzr/+faq/703>

Version Number: Windows NT 5.1 (Build 2600)
Exit Time: 9:41 pm, Friday, February 24 2012
Elapsed Time: 0:00:01.875
Process Time: 0:00:00.031
System Calls: 35955
Context Switches: 8848
Page Faults: 31238
Bytes Read: 10693159
Bytes Written: 648409
Bytes Other: 417932

D:\ext-src\bzr.dev\foo>c:/python27/python.exe d:/ext-src/bzr.dev/trunk/bzr merge ..\bar
 M BRANCH.TODO
 M INSTALL
 M MANIFEST.in
 M Makefile
 M TODO
 M profile_imports.py
 M setup.py
All changes applied successfully.
bzr: warning: some compiled extensions could not be loaded; see <https://answers
.launchpad.net/bzr/+faq/703>

Version Number: Windows NT 5.1 (Build 2600)
Exit Time: 9:42 pm, Friday, February 24 2012
Elapsed Time: 0:00:02.109
Process Time: 0:00:00.015
System Calls: 36312
Context Switches: 10706
Page Faults: 54597
Bytes Read: 11065998
Bytes Written: 638656
Bytes Other: 428054

« Back to merge proposal