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
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.
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
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)?
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