[PATCH] filemerge: wrap quotes around tool path

[Steve Borho]

On Mon, 2008-02-04 at 14:28 -0600, Matt Mackall wrote:
> On Mon, 2008-02-04 at 18:15 -0200, Alexis S. L. Carvalho wrote:
> > Thus spake Matt Mackall:
> > > 
> > > On Mon, 2008-02-04 at 12:33 -0600, Steve Borho wrote:
> > > > # HG changeset patch
> > > > # User Steve Borho <steve at borho.org>
> > > > # Date 1202147114 21600
> > > > # Node ID 1da25064cd8eb0278fd3414b78a16a0f8729cf96
> > > > # Parent  288ec2f6faa2a362dafa8da81bd03d96e36be5ee
> > > > filemerge: wrap quotes around tool path
> > > > 
> > > > diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> > > > --- a/mercurial/filemerge.py
> > > > +++ b/mercurial/filemerge.py
> > > > @@ -184,7 +184,7 @@
> > > >          replace = dict(local=a, base=b, other=c, output=out)
> > > >          args = re.sub("\$(local|base|other|output)",
> > > >                        lambda x: '"%s"' % replace[x.group()[1:]], args)
> > > > -        r = util.system(toolpath + ' ' + args, cwd=repo.root, environ=env)
> > > > +        r = util.system('"'+toolpath+'" '+args, cwd=repo.root, environ=env)
> > > >  
> > > >      if not r and _toolbool(ui, tool, "checkconflicts"):
> > > >          if re.match("^(<<<<<<< .*|=======|>>>>>>> .*)$", fcm.data()):
> > > 
> > > Hmm, that breaks things like:
> > > 
> > > HGMERGE=script -with -some -other -args
> > > 
> > > We were using precisely that trick in run-tests.py until yesterday, so I
> > > suspect other people are using it too.
> > 
> > We're still using it in some tests:
> > 
> > $ grep HGMERGE=\" *
> > test-filebranch:HGMERGE="python ../merge"; export HGMERGE
> > test-merge-symlinks:HGMERGE="python ../echo.py" hg merge
> > test-merge-symlinks:HGMERGE="python ../echo.py" hg up 3
> > test-merge1:HGMERGE="python ../merge"; export HGMERGE
> > test-merge6:HGMERGE="python ../merge"; export HGMERGE
> > test-rename-merge2:HGMERGE="python ../merge"; export HGMERGE
> > 
> > And AFAICS they only work because the script happens to be in another
> > directory - we call util.find_exe on the value of $HGMERGE to see if the
> > tool is around; that function sees an os.sep in the string and assumes
> > it's a full path.
> 
> Hmm, indeed. We should instead always call HGMERGE exactly as set. Doing
> this right probably means having _picktool return (tool, toolpath). That
> has the advantage of only calling _findtool once, which is nice.

Yep. I agree.

> > [ui]
> > merge = command -arg
> 
> But this case is a little different. We can treat this a couple ways:
> 
> a) make ui.merge always take precedence over tools from merge-tools,
> regardless of symlink/binary/gui/available tests

For users of binary packages that install merge tool descriptions in the
system wide hgrc, being able to pick their tool of choice via ui.merge
is handy.  ui.merge definitions with arguments in them will not match
anything in merge-tools anyway.

> b) try to be clever about spaces (tricky!)
> c) disallow this trick
> d) leave it all to os.system to figure out

I think this is a safe option, when tool auto-detection isn't being
used.

-- 
Steve Borho (steve at borho.org)
http://www.borho.org/~steve/steve.asc
Key fingerprint = 2D08 E7CF B624 624C DE1F  E2E4 B0C2 5292 F2C6 2C8C