[Unison-hackers] merge command invocation: bug report (misquoting of merge arguments)

[Alan Schmitt]

On 20 Feb 2012, at 18:45, Neal wrote:

> Are you asking about how to fix the bug in Unison, or about how to 
> work around it?

How to fix the bug.

> As for fixing the bug, I'm not sure.  It appears that Unison 
> implements the merge by putting single quotes around some of the 
> arguments and then passing the entire line to the shell as a whole.  
> That seems destined to have unexpected behaviors.  (Again, what if a 
> file name contains a single quote?  Maybe Unison would need to escape 
> quotes in the file names.)

The problem with escaping is that it may be system dependent, but we 
already have code to deal with that.

> Probably Unison should treat the various variables consistently.

Yes, definitely.

> In short, I'm not sure what the right behavior would be.  Maybe the 
> easiest patch would be to make sure unison single-quotes all the 
> variables consistently, and also escapes any single quotes in the 
> contents of the variable appropriately so that the shell handles them 
> correctly.

I think this is already done, according to this code (uutil.ml):

(* Using single quotes is simpler under Unix but they are not accepted
    by the Windows shell.  Double quotes without further quoting is
    sufficient with Windows as filenames are not allowed to contain
    double quotes. *)
let quotes s =
   if Util.osType = `Win32 && not Util.isCygwin then
     "\"" ^ s ^ "\""
   else
     "'" ^ Util.replacesubstring s "'" "'\\''" ^ "'"

So the patch would be a one-liner (files.ml, line 867), from:

       let cmd = formatMergeCmd
           path1
           (Fspath.quotes (Fspath.concat workingDirForMerge working1))
           (Fspath.quotes (Fspath.concat workingDirForMerge working2))
           (match arch with None -> None | Some f -> Some(Fspath.quotes 
f))
           (Fspath.quotes (Fspath.concat workingDirForMerge new1))
           (Fspath.quotes (Fspath.concat workingDirForMerge new2))
           (Fspath.quotes (Fspath.concat workingDirForMerge newarch)) in

to the same with Fspath.quotes called on path1.

I'm ccing unison-hackers. Is it reasonable to always quote 'path1'?

Alan