Patchwork [2,of,3,stable] mergetools: drop superfluous quoting of diffargs variables

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 27, 2015, 2:15 a.m.
Message ID <bf99a88b77b67ac4c886.1422324951@ssl.google-analytics.com>
Download mbox | patch
Permalink /patch/7557/
State Deferred
Headers show

Comments

Mads Kiilerich - Jan. 27, 2015, 2:15 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1422324789 -3600
#      Tue Jan 27 03:13:09 2015 +0100
# Branch stable
# Node ID bf99a88b77b67ac4c88674ba5d1d19dd97092426
# Parent  aa7caa9b564fca7095f860178f18737cf360285d
mergetools: drop superfluous quoting of diffargs variables

The variables are quoted automatically and there is no need for manual quoting.
Until recently the manual quoting broke extdiff, for example on files with space
in the name.

It was fixed so the manual quoting no longer should do any harm but there is
also no reason to have it. Let's keep it simple and get rid of it.

The remaining entry for araxis might need further cleanup.
Matt Mackall - Jan. 27, 2015, 10:39 p.m.
On Tue, 2015-01-27 at 03:15 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1422324789 -3600
> #      Tue Jan 27 03:13:09 2015 +0100
> # Branch stable
> # Node ID bf99a88b77b67ac4c88674ba5d1d19dd97092426
> # Parent  aa7caa9b564fca7095f860178f18737cf360285d
> mergetools: drop superfluous quoting of diffargs variables

Seems like this wants to be on default?

I've queued the first patch, thanks.
Mads Kiilerich - Jan. 28, 2015, 12:31 a.m.
On 01/27/2015 11:39 PM, Matt Mackall wrote:
> On Tue, 2015-01-27 at 03:15 +0100, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1422324789 -3600
>> #      Tue Jan 27 03:13:09 2015 +0100
>> # Branch stable
>> # Node ID bf99a88b77b67ac4c88674ba5d1d19dd97092426
>> # Parent  aa7caa9b564fca7095f860178f18737cf360285d
>> mergetools: drop superfluous quoting of diffargs variables
> Seems like this wants to be on default?

extdiff with the "new" automatic quoting is incompatible with the 
diffargs we ship. I think that is a bug that should be fixed on stable. 
(I guess the "only quote when necessary" functionality do that the 
problem only is seen when running extdiff on a single file with a name 
that needs quoting.)

We could perhaps restore backwards compatibility, but I still think it 
makes sense to ship a configuration that makes sense, instead of 
shipping one with extra and apparently unsafe quoting. The superfluous 
quoting would then not be fatal but it would still be an "issue" that I 
would consider for stable.

/Mads
Matt Mackall - Jan. 28, 2015, 12:57 a.m.
On Wed, 2015-01-28 at 01:31 +0100, Mads Kiilerich wrote:
> On 01/27/2015 11:39 PM, Matt Mackall wrote:
> > On Tue, 2015-01-27 at 03:15 +0100, Mads Kiilerich wrote:
> >> # HG changeset patch
> >> # User Mads Kiilerich <madski@unity3d.com>
> >> # Date 1422324789 -3600
> >> #      Tue Jan 27 03:13:09 2015 +0100
> >> # Branch stable
> >> # Node ID bf99a88b77b67ac4c88674ba5d1d19dd97092426
> >> # Parent  aa7caa9b564fca7095f860178f18737cf360285d
> >> mergetools: drop superfluous quoting of diffargs variables

superfluous = unneeded but harmless = does not belong on stable

So, is the quoting superfluous or not?

> > Seems like this wants to be on default?
> 
> extdiff with the "new" automatic quoting is incompatible with the 
> diffargs we ship. I think that is a bug that should be fixed on stable. 

Wasn't that the point of patch one that I accepted (and then dropped
because it broke tests, see your mail)?
Mads Kiilerich - Jan. 28, 2015, 1:29 a.m.
On 01/28/2015 01:57 AM, Matt Mackall wrote:
> On Wed, 2015-01-28 at 01:31 +0100, Mads Kiilerich wrote:
>> On 01/27/2015 11:39 PM, Matt Mackall wrote:
>>> On Tue, 2015-01-27 at 03:15 +0100, Mads Kiilerich wrote:
>>>> # HG changeset patch
>>>> # User Mads Kiilerich <madski@unity3d.com>
>>>> # Date 1422324789 -3600
>>>> #      Tue Jan 27 03:13:09 2015 +0100
>>>> # Branch stable
>>>> # Node ID bf99a88b77b67ac4c88674ba5d1d19dd97092426
>>>> # Parent  aa7caa9b564fca7095f860178f18737cf360285d
>>>> mergetools: drop superfluous quoting of diffargs variables
> superfluous = unneeded but harmless = does not belong on stable
>
> So, is the quoting superfluous or not?

As stable is now, the quoting is not harmless. As it was posted as patch 
2 coming after patch 1, the quoting was harmless (but still wrong).

>>> Seems like this wants to be on default?
>> extdiff with the "new" automatic quoting is incompatible with the
>> diffargs we ship. I think that is a bug that should be fixed on stable.
> Wasn't that the point of patch one that I accepted (and then dropped
> because it broke tests, see your mail)?

Correct, that tried to be backwards compatible and handle "incorrect" 
quoting.

/Mads

Patch

diff --git a/mercurial/default.d/mergetools.rc b/mercurial/default.d/mergetools.rc
--- a/mercurial/default.d/mergetools.rc
+++ b/mercurial/default.d/mergetools.rc
@@ -7,7 +7,7 @@  kdiff3.regkeyalt=Software\Wow6432Node\KD
 kdiff3.regappend=\kdiff3.exe
 kdiff3.fixeol=True
 kdiff3.gui=True
-kdiff3.diffargs=--L1 '$plabel1' --L2 '$clabel' $parent $child
+kdiff3.diffargs=--L1 $plabel1 --L2 $clabel $parent $child
 
 gvimdiff.args=--nofork -d -g -O $local $other $base
 gvimdiff.regkey=Software\Vim\GVim
@@ -28,17 +28,17 @@  gpyfm.gui=True
 meld.gui=True
 meld.args=--label='local' $local --label='merged' $base --label='other' $other -o $output
 meld.check=changed
-meld.diffargs=-a --label='$plabel1' $parent --label='$clabel' $child
+meld.diffargs=-a --label=$plabel1 $parent --label=$clabel $child
 
 tkdiff.args=$local $other -a $base -o $output
 tkdiff.gui=True
 tkdiff.priority=-8
-tkdiff.diffargs=-L '$plabel1' $parent -L '$clabel' $child
+tkdiff.diffargs=-L $plabel1 $parent -L $clabel $child
 
 xxdiff.args=--show-merged-pane --exit-with-merge-status --title1 local --title2 base --title3 other --merged-filename $output --merge $local $base $other
 xxdiff.gui=True
 xxdiff.priority=-8
-xxdiff.diffargs=--title1 '$plabel1' $parent --title2 '$clabel' $child
+xxdiff.diffargs=--title1 $plabel1 $parent --title2 $clabel $child
 
 diffmerge.regkey=Software\SourceGear\SourceGear DiffMerge\
 diffmerge.regkeyalt=Software\Wow6432Node\SourceGear\SourceGear DiffMerge\
@@ -47,7 +47,7 @@  diffmerge.priority=-7
 diffmerge.args=-nosplash -merge -title1=local -title2=merged -title3=other $local $base $other -result=$output
 diffmerge.check=changed
 diffmerge.gui=True
-diffmerge.diffargs=--nosplash --title1='$plabel1' --title2='$clabel' $parent $child
+diffmerge.diffargs=--nosplash --title1=$plabel1 --title2=$clabel $parent $child
 
 p4merge.args=$base $local $other $output
 p4merge.regkey=Software\Perforce\Environment
@@ -70,13 +70,13 @@  tortoisemerge.regkeyalt=Software\Wow6432
 tortoisemerge.check=changed
 tortoisemerge.gui=True
 tortoisemerge.priority=-8
-tortoisemerge.diffargs=/base:$parent /mine:$child /basename:'$plabel1' /minename:'$clabel'
+tortoisemerge.diffargs=/base:$parent /mine:$child /basename:$plabel1 /minename:$clabel
 
 ecmerge.args=$base $local $other --mode=merge3 --title0=base --title1=local --title2=other --to=$output
 ecmerge.regkey=Software\Elli\xc3\xa9 Computing\Merge
 ecmerge.regkeyalt=Software\Wow6432Node\Elli\xc3\xa9 Computing\Merge
 ecmerge.gui=True
-ecmerge.diffargs=$parent $child --mode=diff2 --title1='$plabel1' --title2='$clabel'
+ecmerge.diffargs=$parent $child --mode=diff2 --title1=$plabel1 --title2=$clabel
 
 # editmerge is a small script shipped in contrib.
 # It needs this config otherwise it behaves the same as internal:local
@@ -94,13 +94,13 @@  beyondcompare3.regkey=Software\Scooter S
 beyondcompare3.regname=ExePath
 beyondcompare3.gui=True
 beyondcompare3.priority=-2
-beyondcompare3.diffargs=/lro /lefttitle='$plabel1' /righttitle='$clabel' /solo /expandall $parent $child
+beyondcompare3.diffargs=/lro /lefttitle=$plabel1 /righttitle=$clabel /solo /expandall $parent $child
 
 ; Linux version of Beyond Compare
 bcompare.args=$local $other $base -mergeoutput=$output -ro -lefttitle=parent1 -centertitle=base -righttitle=parent2 -outputtitle=merged -automerge -reviewconflicts -solo
 bcompare.gui=True
 bcompare.priority=-1
-bcompare.diffargs=-lro -lefttitle='$plabel1' -righttitle='$clabel' -solo -expandall $parent $child
+bcompare.diffargs=-lro -lefttitle=$plabel1 -righttitle=$clabel -solo -expandall $parent $child
 
 winmerge.args=/e /x /wl /ub /dl other /dr local $other $local $output
 winmerge.regkey=Software\Thingamahoochie\WinMerge
@@ -109,7 +109,7 @@  winmerge.regname=Executable
 winmerge.check=changed
 winmerge.gui=True
 winmerge.priority=-10
-winmerge.diffargs=/r /e /x /ub /wl /dl '$plabel1' /dr '$clabel' $parent $child
+winmerge.diffargs=/r /e /x /ub /wl /dl $plabel1 /dr $clabel $parent $child
 
 araxis.regkey=SOFTWARE\Classes\TypeLib\{46799e0a-7bd1-4330-911c-9660bb964ea2}\7.0\HELPDIR
 araxis.regappend=\ConsoleCompare.exe