Windows diff apps don't understand symlinks created by Cygwin bzr diff --using

[Matt McClure]

Assumptions:

    * Cygwin bzr
    * Windows-native diff program

Symptoms:

Using a diff command like the one below, the external diff program isn't able to find the "new" file on the "right" side of the diff.

    $ bzr diff --using='c:/Program\ Files/TortoiseSVN/bin/TortoiseMerge.exe'

TortoiseMerge opens a dialog prompting the user to specify which files to diff.

I can see that bzr invoked TortoiseMerge as:

    "c:\Program Files\TortoiseSVN\bin\TortoiseMerge.exe" old/TODO.mlm.txt new/TODO.mlm.txt

in the working directory: c:\Documents and Settings\mlm\Local Settings\Temp\bzr-diff-A80rhV\ . The "new" directory is a symlink to my working tree:

    $ cd 'c:\Documents and Settings\mlm\Local Settings\Temp\bzr-diff-A80rhV\'
    $ ls -l
    total 1
    lrwxrwxrwx 1 mlm None 41 Mar 30 12:16 new -> /c/home/mlm/sandbox/bzr/repo/bzr.1.2.mlm/
    drwxr-xr-x+ 2 mlm None 0 Mar 30 12:16 old

[Alexander Belchenko (bialix)]

Mixing Cygwin applications with native Windows ones is always very bad idea.

[Matt McClure (matthewlmcclure)]

Alexander, with all due respect, that's simply FUD. There are plenty of use cases where invoking Windows programs from a Cygwin shell works just fine.

Please consider the attached patch.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt McClure wrote:
> Alexander, with all due respect, that's simply FUD. There are plenty of
> use cases where invoking Windows programs from a Cygwin shell works just
> fine.
>
> Please consider the attached patch.

That patch addresses the issue in the wrong place. Bazaar creates a
symlink in order to provide desirable pathnames. It would otherwise be
using the real path already. By dereferencing the symlink, you're
negating the purpose of creating the symlink in the first place.

Of course, it can handle Python implementations that don't provide
symlinks. It only uses symlinks when Python supports them.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFH8PL90F+nu1YWqI0RAscpAJ9TWgWjn7IRWabMSoU42Z9JblLGPgCfWBpV
Iz9IFLezMpfrr38l/ghxF9E=
=WHkd
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

Thankyou for the patch Matt.

The intention seems to be to make sure we produce windows style names
for normpath, with a drive letter.

I am concerned that if we merge this as-is it will be a big
performance regression because of forking a new process on every
normalizepath call, something which would normally be pretty quick.

It seems like we should be able to do this in-process, either by using
a cygwin api callable from Python(?) or just doing the transformation
ourselves?

(Per <http://www.cygwin.com/cygwin-ug-net/using-utils.html>)

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

[Martin Pool (mbp)]

It's not clear to me from the original post that Bazaar actually did
create the symlink - was it created by some other cygwin operation?

If bzr created a versioned symlink with cygwin syntax it might be
reasonable for us to use Windows syntax when running a non-cygwin
program. I'm not sure how we would tell though...

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

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> It's not clear to me from the original post that Bazaar actually did
> create the symlink - was it created by some other cygwin operation?

It was created by Bazaar, when doing diff --using.

This is done in bzrlib.diff.DiffFromTool._try_symlink_root

> If bzr created a versioned symlink with cygwin syntax it might be
> reasonable for us to use Windows syntax when running a non-cygwin
> program. I'm not sure how we would tell though...

This is not about diffing symlinks using external programs. It's about
diffing regular files which, as an optimization, Bazaar symlinks to
instead of copying. --using only affects diffing files, not symlinks or
directories.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFH8Pnm0F+nu1YWqI0RAoVmAJsHU8Mi5C7J0b2ytNW2YF/CanMCYACfZGtu
G8o+/DDtDdA8RFuk0auHlNo=
=5E+P
-----END PGP SIGNATURE-----

[Alexander Belchenko (bialix)]

Matt McClure пишет:
> Alexander, with all due respect, that's simply FUD. There are plenty of
> use cases where invoking Windows programs from a Cygwin shell works just
> fine.

In your concrete example TortoiseMerge knows absolutely nothing about Cygwin's
symlinks. So I don't care if you call it FUD.

> Please consider the attached patch.

Please, send your patch to Bazaar ML with [PATCH] or [MERGE] prefix in subject line,
so more people will take a chance to look and review it.

Thanks.

[Matt McClure (matthewlmcclure)]

Thanks for all the feedback.

On Mon, Mar 31, 2008 at 10:19 AM, Aaron Bentley <email address hidden> wrote:
> That patch addresses the issue in the wrong place.

Could you suggest a more appropriate place?

> Bazaar creates a
> symlink in order to provide desirable pathnames. It would otherwise be
> using the real path already. By dereferencing the symlink, you're
> negating the purpose of creating the symlink in the first place.
>

In my case the symlink does harm: it prevents the diff program from
finding files in the symlinked path. And it fails to provide the
above value: even if TortoiseMerge could read the symlinked path, it
doesn't display the "full" relative path; it only displays the
filename portion.

I had the impression that symlinks were primarily used -- vs. copying
-- for better performance and reduced disk usage.

> Of course, it can handle Python implementations that don't provide
> symlinks. It only uses symlinks when Python supports them.

I considered keeping the nice pathnames by disabling symlinks
altogether for Cygwin, but I discovered that symlinks have another
advantage: the right side of the diff is the actual workspace file, so
I can edit it in the diff program if I want to change the diff.

On Mon, Mar 31, 2008 at 10:28 AM, Martin Pool <email address hidden> wrote:
> I am concerned that if we merge this as-is it will be a big
> performance regression because of forking a new process on every
> normalizepath call, something which would normally be pretty quick.

Regarding performance, "diff --using" doesn't seem like a case where a
few extra ticks can't be tolerated. I can't find any callers of
normalizepath other than the calls that my patch adds. Is that right?
 If so, the performance impact isn't felt anywhere other than diff
--using.

> It seems like we should be able to do this in-process, either by using
> a cygwin api callable from Python(?) or just doing the transformation
> ourselves?

I wasn't able to find Python bindings for the Cygwin DLL. Do they exist?

Doing the transformation ourselves will produce quite a bit of code, I
think. We'd have to handle the Cygwin mount table. In general, C:
could be mounted anywhere.

I suppose parsing the "symlink file" is another option. Cygwin puts
the Windows path in there when it creates the symlink.

On Mon, Mar 31, 2008 at 11:12 AM, Alexander Belchenko <email address hidden> wrote:
> Please, send your patch to Bazaar ML with [PATCH] or [MERGE] prefix in subject line,
> so more people will take a chance to look and review it.

Is the mailing list a better forum?

Matt

[Alexander Belchenko (bialix)]

Matt McClure пишет:
> Thanks for all the feedback.

> On Mon, Mar 31, 2008 at 11:12 AM, Alexander Belchenko <email address hidden> wrote:
>> Please, send your patch to Bazaar ML with [PATCH] or [MERGE] prefix in subject line,
>> so more people will take a chance to look and review it.
>
> Is the mailing list a better forum?

Yes, it is.

[Matt McClure (matthewlmcclure)]

The fix for this issue was merged: http://article.gmane.org/gmane.comp.version-control.bazaar-ng.general/42705