| 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
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.
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