Patchwork [1,of,2] merge: drop underscore prefix from _checkunknown()

login
register
mail settings
Submitter Martin von Zweigbergk
Date Nov. 15, 2014, 12:21 a.m.
Message ID <c2491406f4623e148bee.1416010883@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6740/
State Accepted
Commit 3177d710630d05f5b4c062bb0ee58555b186fe00
Headers show

Comments

Martin von Zweigbergk - Nov. 15, 2014, 12:21 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1415949135 28800
#      Thu Nov 13 23:12:15 2014 -0800
# Node ID c2491406f4623e148bee137b1862ff0b609e4235
# Parent  991098579940552536d0a99fa3602dd1661aa388
merge: drop underscore prefix from _checkunknown()

The method has been called from commands.py since 3eab42088be4
(update: just merge unknown file collisions, 2012-02-09), so drop the
underscore prefix that suggests that it's private.
Matt Mackall - Nov. 17, 2014, 10:34 p.m.
On Fri, 2014-11-14 at 16:21 -0800, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1415949135 28800
> #      Thu Nov 13 23:12:15 2014 -0800
> # Node ID c2491406f4623e148bee137b1862ff0b609e4235
> # Parent  991098579940552536d0a99fa3602dd1661aa388
> merge: drop underscore prefix from _checkunknown()

> The method has been called from commands.py since 3eab42088be4
> (update: just merge unknown file collisions, 2012-02-09), so drop the
> underscore prefix that suggests that it's private.

I look at the underscore in Python as advisory: you shouldn't be using
this unless you have to. And when you see it used like this, you should
think "ah, this is a hack of some sort".

And in this case, it certainly is: we definitely don't want the commands
layer to know about this obscure merge detail. So I think the underscore
is correct as it stands and leaves an appropriate code smell here.
Removing it is only going to reduce the probability that a future Martin
will be bothered enough to properly refactor away the hack.
Martin von Zweigbergk - Nov. 18, 2014, 5:36 a.m.
On Mon Nov 17 2014 at 2:34:29 PM Matt Mackall <mpm@selenic.com> wrote:

> On Fri, 2014-11-14 at 16:21 -0800, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1415949135 28800
> > #      Thu Nov 13 23:12:15 2014 -0800
> > # Node ID c2491406f4623e148bee137b1862ff0b609e4235
> > # Parent  991098579940552536d0a99fa3602dd1661aa388
> > merge: drop underscore prefix from _checkunknown()
>
> > The method has been called from commands.py since 3eab42088be4
> > (update: just merge unknown file collisions, 2012-02-09), so drop the
> > underscore prefix that suggests that it's private.
>
> I look at the underscore in Python as advisory: you shouldn't be using
> this unless you have to. And when you see it used like this, you should
> think "ah, this is a hack of some sort".
>
> And in this case, it certainly is: we definitely don't want the commands
> layer to know about this obscure merge detail. So I think the underscore
> is correct as it stands and leaves an appropriate code smell here.
> Removing it is only going to reduce the probability that a future Martin
> will be bothered enough to properly refactor away the hack.
>

Oh, it didn't seems like an obscure merge detail to me at first, but that's
good to hear. It turns out the series of which this is the second patch
turned out deleting _checkunknown() altogether. I'm glad to hear it wasn't
a completely delusional idea.

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -372,7 +372,7 @@ 
         wlock.release()
 
 # Before starting the manifest merge, merge.updates will call
-# _checkunknown to check if there are any files in the merged-in
+# checkunknown to check if there are any files in the merged-in
 # changeset that collide with unknown files in the working copy.
 #
 # The largefiles are seen as unknown, so this prevents us from merging
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -6184,7 +6184,7 @@ 
             raise util.Abort(_("uncommitted changes"))
         if rev is None:
             rev = repo[repo[None].branch()].rev()
-        mergemod._checkunknown(repo, repo[None], repo[rev])
+        mergemod.checkunknown(repo, repo[None], repo[rev])
 
     repo.ui.setconfig('ui', 'forcemerge', tool, 'update')
 
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -304,7 +304,7 @@ 
         and repo.dirstate.normalize(f) not in repo.dirstate
         and mctx[f].cmp(wctx[f]))
 
-def _checkunknown(repo, wctx, mctx):
+def checkunknown(repo, wctx, mctx):
     "check for collisions between unknown files and files in mctx"
 
     error = False