[PATCH 1 of 2] filemerge: add $revbase and $revother substitutions in merge tool arguments

[Adrian Buehlmann]

On 2012-07-12 22:48, Vitaliy Filippov wrote:
> # HG changeset patch
> # User Vitaliy Filippov <vitalif at yourcmc.ru>
> # Date 1342116873 -14400
> # Node ID 2422f58a70bcfbf20ec42edf9e1353fdc3f2bfc2
> # Parent  3ac9592b7ab432e1470ec97d19c9b7c58c3fda86
> filemerge: add $revbase and $revother substitutions in merge tool arguments
> 
> Usage example:
> 
> [merge-tools]
> filemerge.executable = /usr/bin/merge
> filemerge.args = -L local -L "base (rev $revbase)" -L "other (rev $revother)" -A $local $base $other
> 
> diff -r 3ac9592b7ab4 -r 2422f58a70bc mercurial/filemerge.py
> --- a/mercurial/filemerge.py	Thu Jul 12 14:20:34 2012 -0500
> +++ b/mercurial/filemerge.py	Thu Jul 12 22:14:33 2012 +0400
> @@ -253,7 +253,7 @@
>          args = _toolstr(ui, tool, "args", '$local $base $other')
>          if "$output" in args:
>              out, a = a, back # read input from backup, write to original
> -        replace = dict(local=a, base=b, other=c, output=out)
> +        replace = dict(local=a, base=b, other=c, output=out, revbase=fca.rev(), revother=fco.rev())
>          args = util.interpolate(r'\$', replace, args,
>                                  lambda s: '"%s"' % util.localpath(s))
>          r = util.system(toolpath + ' ' + args, cwd=repo.root, environ=env,

This produces a traceback on Windows:

  ** unknown exception encountered, please report by visiting
  ** http://mercurial.selenic.com/wiki/BugTracker
  ** Python 2.7.3 (default, Apr 10 2012, 23:24:47) [MSC v.1500 64 bit (AMD64)]
  ** Mercurial Distributed SCM (version 2.2.3+27-e12eae2701c5)
  ** Extensions loaded:
  Traceback (most recent call last):
    File "C:/Users/adi/hgrepos/hg-main/hg", line 38, in <module>
      mercurial.dispatch.run()
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 28, in run
      sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 65, in dispatch
      return _runcatch(req)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 88, in _runcatch
      return _dispatch(req)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 739, in _dispatch
      cmdpats, cmdoptions)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 513, in runcommand
      ret = _runcommand(ui, options, cmd, d)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 829, in _runcommand
      return checkargs()
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 800, in checkargs
      return cmdfunc()
    File "C:\Users\adi\hgrepos\hg-main\mercurial\dispatch.py", line 736, in <lambda>
      d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\util.py", line 475, in check
      return func(*args, **kwargs)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\commands.py", line 4258, in merge
      return hg.merge(repo, node, force=opts.get('force'))
    File "C:\Users\adi\hgrepos\hg-main\mercurial\hg.py", line 435, in merge
      stats = mergemod.update(repo, node, True, force, False)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\merge.py", line 610, in update
      stats = applyupdates(repo, action, wc, p2, pa, overwrite)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\merge.py", line 373, in applyupdates
      r = ms.resolve(fd, wctx, mctx)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\merge.py", line 76, in resolve
      r = filemerge.filemerge(self._repo, self._local, lfile, fcd, fco, fca)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\filemerge.py", line 322, in filemerge
      (a, b, c, back))
    File "C:\Users\adi\hgrepos\hg-main\mercurial\filemerge.py", line 258, in _xmerge
      lambda s: '"%s"' % util.localpath(s))
    File "C:\Users\adi\hgrepos\hg-main\mercurial\util.py", line 1447, in interpolate
      return r.sub(lambda x: fn(mapping[x.group()[1:]]), s)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\util.py", line 1447, in <lambda>
      return r.sub(lambda x: fn(mapping[x.group()[1:]]), s)
    File "C:\Users\adi\hgrepos\hg-main\mercurial\filemerge.py", line 258, in <lambda>
      lambda s: '"%s"' % util.localpath(s))
    File "C:\Users\adi\hgrepos\hg-main\mercurial\windows.py", line 128, in localpath
      return path.replace('/', '\\')
  AttributeError: 'int' object has no attribute 'replace'


fca.rev() and fco.rev() return int objects, but the line

          args = util.interpolate(r'\$', replace, args,
                                  lambda s: '"%s"' % util.localpath(s))

then calls util.localpath(s) on those int's, which expects an str object.

On posix platforms (e.g. Linux), util.localpath does nothing (see posix.py, line 152),
but on Windows (see windows.py, line 127), it is defined as:

def localpath(path):
    return path.replace('/', '\\')

So the current code expects those arguments to be paths.


I think the idea of your patch is good and I assume it worked on your platfrom.


What we probably need to do, is to think about whether we really should still
assume those arguments to be paths.

Since Windows can deal well with slashes as separator in paths, I think that
localpath() call in filemerge.py can be removed.

This should be done with a separate, initial patch.


Next problem is, that I think we should still make sure *strings* are put into
the dict. But with

        replace = dict(local=a, base=b, other=c, output=out, revbase=fca.rev(), revother=fco.rev())

you put int's into the dict.


Another question is, if it is really a good idea to put revision numbers in there.

Perhaps this should use

  short(fca.node())

instead of

  fca.rev()

(same for fco)


While we're at it, that line violates the style rules for python sources of
Mercurial, which is detected by the check-code test of the testsuite:

  $ PATH="/mingw/bin:/bin:/c/python:/c/python/scripts" python run-tests.py --local test-check*

  --- C:\Users\adi\hgrepos\hg-main\tests\test-check-code-hg.t
  +++ C:\Users\adi\hgrepos\hg-main\tests\test-check-code-hg.t.err
  @@ -6,6 +6,10 @@
     >     exit 80
     > fi
     $ hg manifest | xargs "$check_code" || echo 'FAILURE IS NOT AN OPTION!!!'
  +  mercurial/filemerge.py:256:
  +   >         replace = dict(local=a, base=b, other=c, output=out, revbase=fca.rev(), revother=fco.rev())
  +   line too long
  +  FAILURE IS NOT AN OPTION!!!

     $ hg manifest | xargs "$check_code" --warnings --nolineno --per-file=0 || true
     hgext/convert/cvsps.py:0:
  @@ -159,6 +163,9 @@
     mercurial/commands.py:0:
      >     ui.write('symlink: %s\n' % (util.checklink(path) and 'yes' or 'no'))
      warning: unwrapped ui message
  +  mercurial/filemerge.py:0:
  +   >         replace = dict(local=a, base=b, other=c, output=out, revbase=fca.rev(), revother=fco.rev())
  +   line too long
     tests/autodiff.py:0:
      >         ui.write('data lost for: %s\n' % fn)
      warning: unwrapped ui message

  ERROR: C:\Users\adi\hgrepos\hg-main\tests\test-check-code-hg.t output changed
  !..
  Failed test-check-code-hg.t: output changed
  # Ran 3 tests, 0 skipped, 1 failed.


So this proves that you didn't run the testsuite for this patch.

So you missed at least the item "test suite has been updated and runs cleanly"
as noted on

  http://mercurial.selenic.com/wiki/ContributingChanges#Submission_checklist

Running the testsuite on Linux should be as simple as:

  $ cd tests
  $ python run-tests.py