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

[Benjamin C. Pierce]

This sounds reasonable.  If we change it, though, we'll need to make sure we test on all architectures -- different shells are picky in different ways about this sort of thing.

   - B


On Feb 21, 2012, at 3:33 AM, Alan Schmitt wrote:

> 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
> _______________________________________________
> Unison-hackers mailing list
> Unison-hackers at lists.seas.upenn.edu
> http://lists.seas.upenn.edu/mailman/listinfo/unison-hackers