[PATCH] Improved logging for filemerge, making it easier to debug problems with merge configuration ... and refac

Matt Mackall mpm at selenic.com
Thu Nov 6 17:06:58 CST 2008


On Thu, 2008-11-06 at 23:15 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <mads at kiilerich.com>
> # Date 1226009733 -3600
> # Node ID 6d68d6778484a077f4b9f163f3be015e533cb644
> # Parent  eae1767cc6a8002a1d75b3af40e11782efc288ba
> Improved logging for filemerge, making it easier to debug problems with merge configuration ... and refac
> 
> Previously it was very hard to debug situations where the expected merge tool
> wasn't used - especially on windows. This patch tried to improve that.
> 
> The patch started out very simple but ended up refactoring a lot of things. It
> is mostly moving code around and cleaning up. The intention was to not change
> the semantics.
> 
> The patch as it is probably does too much and can't be reviewed as patch. And
> it probably contains some unacceptable changes. Perhaps it would be better if
> someone with the power to review and apply took a look at the code and tried to
> do something similar instead ...
> 
> Benefits:
> * Better logging
> * Less tense code
> * Quoting with util function

The above is my first hint that this should probably be three patches.
And indeed, scrolling down I discover that I can't really tell what's
what in here.

> Notes:
> * Should behaviour be preserved in all corner cases? Or is some cleanup ok?

Depends on the corner-case. Backwards-compatibility with both 1.0
configs and pre-1.0 configs is a requirement.

> * Should pattern-matched tools be checked for symlink but not binary capabilities?

If you say "I know how to merge *.doc files", you probably know how to
merge (binary) doc files, but not symlinks to doc files.

> * How should configured but bogus registry settings be handled? Just ignoring seems wrong ...

You should expect that there are tool defaults installed for tools that
are NOT installed because we want to ship sensible defaults for all
known tools. In particular, the average user should have
contrib/mergetools.hgrc in their global config when they install a
binary package.

We should only warn about tools the user explicitly asked for, not ones
we simply picked from the list of known tools.

> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -5,7 +5,7 @@
>  # This software may be used and distributed according to the terms
>  # of the GNU General Public License, incorporated herein by reference.
>  
> -from node import nullrev, short
> +from node import short

Unrelated change.

>  from i18n import _
>  import util, os, tempfile, simplemerge, re, filecmp
>  
> @@ -15,67 +15,90 @@
>  def _toolbool(ui, tool, part, default=False):
>      return ui.configbool("merge-tools", tool + "." + part, default)
>  
> -def _findtool(ui, tool):
> -    if tool in ("internal:fail", "internal:local", "internal:other"):
> +def _checktool(ui, tool, tooldesc, symlink, binary):
> +    if symlink and not _toolbool(ui, tool, "symlink"):
> +        ui.warn(_("%s can't handle symlinks\n") % tooldesc)
> +    elif binary and not _toolbool(ui, tool, "binary"):
> +        ui.warn(_("%s can't handle binary\n") % tooldesc)
> +    elif not util.gui() and _toolbool(ui, tool, "gui"):
> +        ui.warn(_("%s requires a GUI\n") % tooldesc)
> +    else:
> +        return True
> +    return False
> +
> +def _findtool(ui, tool, tooldesc):
> +    if tool in ("internal:fail", "internal:local", "internal:other", "internal:merge"):
>          return tool
> +    exename = exe = None
>      k = _toolstr(ui, tool, "regkey")
>      if k:
> -        p = util.lookup_reg(k, _toolstr(ui, tool, "regname"))
> +        n = _toolstr(ui, tool, "regname")
> +        p = util.lookup_reg(k, n)
>          if p:
> -            p = util.find_exe(p + _toolstr(ui, tool, "regappend"))
> -            if p:
> -                return p
> -    return util.find_exe(_toolstr(ui, tool, "executable", tool))
> +            a = _toolstr(ui, tool, "regappend")
> +            exename = p + a
> +            exe = util.find_exe(exename)
> +            if not exe:
> +                ui.warn(_("couldn't find merge tool '%s' (from regkey '%s' regname %r regappend %r) for %s\n") % (exename, k, n, a, tooldesc))

Definitely not a warning. Debug or possibly note.

> +        else:
> +            ui.warn(_("couldn't find regkey '%s' regname %r for %s\n") % (k, n, tooldesc))
> +    if not exe:
> +        exename = _toolstr(ui, tool, "executable", tool)
> +        exe = util.find_exe(exename)
> +    if exe:
> +        return util.shellquote(exe)
> +    ui.warn(_("couldn't find merge tool '%s' for %s\n") % (exename, tooldesc))
>  
>  def _picktool(repo, ui, path, binary, symlink):
> -    def check(tool, pat, symlink, binary):
> -        tmsg = tool
> -        if pat:
> -            tmsg += " specified for " + pat
> -        if pat and not _findtool(ui, tool): # skip search if not matching
> -            ui.warn(_("couldn't find merge tool %s\n") % tmsg)
> -        elif symlink and not _toolbool(ui, tool, "symlink"):
> -            ui.warn(_("tool %s can't handle symlinks\n") % tmsg)
> -        elif binary and not _toolbool(ui, tool, "binary"):
> -            ui.warn(_("tool %s can't handle binary\n") % tmsg)
> -        elif not util.gui() and _toolbool(ui, tool, "gui"):
> -            ui.warn(_("tool %s requires a GUI\n") % tmsg)
> -        else:
> -            return True
> -        return False
> +    
> +    def candidates():
> +        # HGMERGE takes precedence
> +        hgmerge = os.environ.get("HGMERGE")
> +        if hgmerge:
> +            yield (True, hgmerge, None)
> +    
> +        # then patterns
> +        for pat, tool in ui.configitems("merge-patterns"):
> +            mf = util.matcher(repo.root, "", [pat], [], [])[1]
> +            if mf(path):
> +                yield (False, tool, _("%r for pattern '%s'") % (tool, pat))
> +    
> +        # read configured merge-tools and their priorities
> +        toolpriorities = {}
> +        for k,v in ui.configitems("merge-tools"):
> +            t = k.split('.')[0]
> +            if t not in toolpriorities:
> +                toolpriorities[t] = int(_toolstr(ui, t, "priority", "0"))
> +    
> +        # uimerge 
> +        uimerge = ui.config("ui", "merge")
> +        if uimerge:
> +            if toolpriorities.has_key(uimerge):
> +                yield (False, uimerge, _("%r from ui.merge") % (uimerge, ))
> +            else:
> +                yield (True, uimerge, None)
> +    
> +        for p, t in util.sort([(-p, t) for (t, p) in toolpriorities.items()]):
> +            yield (False, t, _("%r with priority %s") % (t, p))
> +    
> +        # then old default hgmerge
> +        yield (False, "hgmerge", _("%r (old default)") % 'hgmerge')
>  
> -    # HGMERGE takes precedence
> -    hgmerge = os.environ.get("HGMERGE")
> -    if hgmerge:
> -        return (hgmerge, hgmerge)
> +        # internal merge as last resort
> +        if not symlink and not binary:
> +            yield (True, "internal:merge", None)
> +        
> +    # choose first tool assumed or checked ok 
> +    for ok, tool, tooldesc in candidates():
> +        if ok:
> +            return (tool, tool)
> +        if _checktool(ui, tool, tooldesc, symlink, binary):
> +            toolpath = _findtool(ui, tool, tooldesc)
> +            if toolpath:
> +                return (tool, toolpath)
>  
> -    # then patterns
> -    for pat, tool in ui.configitems("merge-patterns"):
> -        mf = util.matcher(repo.root, "", [pat], [], [])[1]
> -        if mf(path) and check(tool, pat, symlink, False):
> -                toolpath = _findtool(ui, tool)
> -                return (tool, '"' + toolpath + '"')
> -
> -    # then merge tools
> -    tools = {}
> -    for k,v in ui.configitems("merge-tools"):
> -        t = k.split('.')[0]
> -        if t not in tools:
> -            tools[t] = int(_toolstr(ui, t, "priority", "0"))
> -    names = tools.keys()
> -    tools = util.sort([(-p,t) for t,p in tools.items()])
> -    uimerge = ui.config("ui", "merge")
> -    if uimerge:
> -        if uimerge not in names:
> -            return (uimerge, uimerge)
> -        tools.insert(0, (None, uimerge)) # highest priority
> -    tools.append((None, "hgmerge")) # the old default, if found
> -    for p,t in tools:
> -        toolpath = _findtool(ui, t)
> -        if toolpath and check(t, None, symlink, binary):
> -            return (t, '"' + toolpath + '"')
> -    # internal merge as last resort
> -    return (not (symlink or binary) and "internal:merge" or None, None)
> +    # asking user is last last resort
> +    return (None, None)
>  
>  def _eoltype(data):
>      "Guess the EOL type of a file"
> @@ -89,16 +112,16 @@
>          return '\n'
>      return None # unknown
>  
> -def _matcheol(file, origfile):
> +def _matcheol(fname, origfile):
>      "Convert EOL markers in a file to match origfile"
>      tostyle = _eoltype(open(origfile, "rb").read())
>      if tostyle:
> -        data = open(file, "rb").read()
> +        data = open(fname, "rb").read()
>          style = _eoltype(data)
>          if style:
>              newdata = data.replace(style, tostyle)
>              if newdata != data:
> -                open(file, "wb").write(newdata)
> +                open(fname, "wb").write(newdata)

Unrelated change.

>  def filemerge(repo, mynode, orig, fcd, fco, fca):
>      """perform a 3-way merge in the working directory
> @@ -169,7 +192,7 @@
>      if _toolbool(ui, tool, "premerge", not (binary or symlink)):
>          r = simplemerge.simplemerge(a, b, c, quiet=True)
>          if not r:
> -            ui.debug(_(" premerge successful\n"))
> +            ui.debug(_(" premerge successful and sufficient\n"))

Redundant. Makes the patch much bigger by breaking all the tests.

>              os.unlink(back)
>              os.unlink(b)
>              os.unlink(c)
> diff --git a/tests/test-copy-move-merge.out b/tests/test-copy-move-merge.out
> --- a/tests/test-copy-move-merge.out
> +++ b/tests/test-copy-move-merge.out
> @@ -18,11 +18,11 @@
>  picked tool 'internal:merge' for b (binary False symlink False)
>  merging a and b to b
>  my b at fb3948d97f07+ other b at 40da226db0f0 ancestor a at 583c7b748052
> - premerge successful
> + premerge successful and sufficient
>  picked tool 'internal:merge' for c (binary False symlink False)
>  merging a and c to c
>  my c at fb3948d97f07+ other c at 40da226db0f0 ancestor a at 583c7b748052
> - premerge successful
> + premerge successful and sufficient
>  0 files updated, 2 files merged, 0 files removed, 0 files unresolved
>  (branch merge, don't forget to commit)
>  -- b --
> diff --git a/tests/test-double-merge.out b/tests/test-double-merge.out
> --- a/tests/test-double-merge.out
> +++ b/tests/test-double-merge.out
> @@ -15,11 +15,11 @@
>  picked tool 'internal:merge' for bar (binary False symlink False)
>  merging foo and bar to bar
>  my bar at 2092631ce82b+ other bar at 7731dad1c2b9 ancestor foo at 310fd17130da
> - premerge successful
> + premerge successful and sufficient
>  picked tool 'internal:merge' for foo (binary False symlink False)
>  merging foo
>  my foo at 2092631ce82b+ other foo at 7731dad1c2b9 ancestor foo at 310fd17130da
> - premerge successful
> + premerge successful and sufficient
>  0 files updated, 2 files merged, 0 files removed, 0 files unresolved
>  (branch merge, don't forget to commit)
>  -- foo --
> diff --git a/tests/test-issue672.out b/tests/test-issue672.out
> --- a/tests/test-issue672.out
> +++ b/tests/test-issue672.out
> @@ -34,7 +34,7 @@
>  picked tool 'internal:merge' for 1a (binary False symlink False)
>  merging 1a and 1 to 1a
>  my 1a at ac7575e3c052+ other 1 at 746e9549ea96 ancestor 1 at 81f4b099af3d
> - premerge successful
> + premerge successful and sufficient
>  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
>  (branch merge, don't forget to commit)
>  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> @@ -53,6 +53,6 @@
>  picked tool 'internal:merge' for 1a (binary False symlink False)
>  merging 1 and 1a to 1a
>  my 1a at 746e9549ea96+ other 1a at ac7575e3c052 ancestor 1 at 81f4b099af3d
> - premerge successful
> + premerge successful and sufficient
>  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
>  (branch merge, don't forget to commit)
> diff --git a/tests/test-merge-commit.out b/tests/test-merge-commit.out
> --- a/tests/test-merge-commit.out
> +++ b/tests/test-merge-commit.out
> @@ -31,7 +31,7 @@
>  picked tool 'internal:merge' for bar (binary False symlink False)
>  merging bar
>  my bar at 2d2f9a22c82b+ other bar at 7d3b554bfdf1 ancestor bar at 0a3ab4856510
> - premerge successful
> + premerge successful and sufficient
>  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
>  (branch merge, don't forget to commit)
>  % contents of bar should be line1 line2
> @@ -81,7 +81,7 @@
>  picked tool 'internal:merge' for bar (binary False symlink False)
>  merging bar
>  my bar at 2d2f9a22c82b+ other bar at 96ab80c60897 ancestor bar at 0a3ab4856510
> - premerge successful
> + premerge successful and sufficient
>  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
>  (branch merge, don't forget to commit)
>  % contents of bar should be line1 line2
> diff --git a/tests/test-merge-internal-tools-pattern b/tests/test-merge-internal-tools-pattern
> --- a/tests/test-merge-internal-tools-pattern
> +++ b/tests/test-merge-internal-tools-pattern
> @@ -50,7 +50,7 @@
>  echo "# merge using default tool"
>  hg update -C 2
>  rm .hg/hgrc
> -hg merge
> +/usr/bin/hg merge
>  cat f
>  hg stat
>  
> diff --git a/tests/test-merge-internal-tools-pattern.out b/tests/test-merge-internal-tools-pattern.out
> --- a/tests/test-merge-internal-tools-pattern.out
> +++ b/tests/test-merge-internal-tools-pattern.out
> @@ -32,6 +32,7 @@
>  M f
>  # merge using default tool
>  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +couldn't find merge tool 'hgmerge' for 'hgmerge' (old default)

Who cares?

>  merging f
>  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
>  (branch merge, don't forget to commit)
> diff --git a/tests/test-rename-merge1.out b/tests/test-rename-merge1.out
> --- a/tests/test-rename-merge1.out
> +++ b/tests/test-rename-merge1.out
> @@ -24,7 +24,7 @@
>  picked tool 'internal:merge' for b (binary False symlink False)
>  merging a and b to b
>  my b at f26ec4fc3fa3+ other b at 8e765a822af2 ancestor a at af1939970a1c
> - premerge successful
> + premerge successful and sufficient
>  warning: detected divergent renames of a2 to:
>   c2
>   b2
> diff --git a/tests/test-rename-merge2.out b/tests/test-rename-merge2.out
> --- a/tests/test-rename-merge2.out
> +++ b/tests/test-rename-merge2.out
> @@ -18,7 +18,7 @@
>  picked tool 'python ../merge' for b (binary False symlink False)
>  merging a and b to b
>  my b at e300d1c794ec+ other b at 735846fee2d7 ancestor a at 924404dff337
> - premerge successful
> + premerge successful and sufficient
>  picked tool 'python ../merge' for rev (binary False symlink False)
>  merging rev
>  my rev at e300d1c794ec+ other rev at 735846fee2d7 ancestor rev at 924404dff337
> @@ -52,7 +52,7 @@
>  picked tool 'python ../merge' for b (binary False symlink False)
>  merging b and a to b
>  my b at ac809aeed39a+ other a at f4db7e329e71 ancestor a at 924404dff337
> - premerge successful
> + premerge successful and sufficient
>  picked tool 'python ../merge' for rev (binary False symlink False)
>  merging rev
>  my rev at ac809aeed39a+ other rev at f4db7e329e71 ancestor rev at 924404dff337
> @@ -85,7 +85,7 @@
>  picked tool 'python ../merge' for b (binary False symlink False)
>  merging a and b to b
>  my b at e300d1c794ec+ other b at e03727d2d66b ancestor a at 924404dff337
> - premerge successful
> + premerge successful and sufficient
>  picked tool 'python ../merge' for rev (binary False symlink False)
>  merging rev
>  my rev at e300d1c794ec+ other rev at e03727d2d66b ancestor rev at 924404dff337
> @@ -116,7 +116,7 @@
>  picked tool 'python ../merge' for b (binary False symlink False)
>  merging b and a to b
>  my b at ecf3cb2a4219+ other a at f4db7e329e71 ancestor a at 924404dff337
> - premerge successful
> + premerge successful and sufficient
>  picked tool 'python ../merge' for rev (binary False symlink False)
>  merging rev
>  my rev at ecf3cb2a4219+ other rev at f4db7e329e71 ancestor rev at 924404dff337
> @@ -581,7 +581,7 @@
>  picked tool 'python ../merge' for b (binary False symlink False)
>  merging b and a to b
>  my b at ecf3cb2a4219+ other a at 2b958612230f ancestor a at 924404dff337
> - premerge successful
> + premerge successful and sufficient
>  getting c
>  picked tool 'python ../merge' for rev (binary False symlink False)
>  merging rev
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
-- 
Mathematics is the supreme nostalgia of our time.



More information about the Mercurial-devel mailing list