Patchwork D6163: copies: extract function for deciding whether to use changeset-centric algos

login
register
mail settings
Submitter phabricator
Date March 20, 2019, 7:03 p.m.
Message ID <differential-rev-PHID-DREV-ifc33fpnhepc5xzrbh4v-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39349/
State Superseded
Headers show

Comments

phabricator - March 20, 2019, 7:03 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We'll eventually have a "experimental.copies.read-from=changeset-only"
  option too and I don't want to spread the logic for determining if we
  should use changeset-centric of filelog-centric algorithms.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6163

AFFECTED FILES
  mercurial/copies.py
  mercurial/scmutil.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - March 20, 2019, 7:11 p.m.
pulkit added inline comments.

INLINE COMMENTS

> copies.py:163
>  
> +def usechangesetcentricalgo(repo):
> +    """Checks if we should use changeset-centric copy algorithms"""

maybe we need to have https://phab.mercurial-scm.org/D2010, but we can drop `algo` (and maybe `use` also) from the name I guess.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6163

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 20, 2019, 7:17 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in copies.py:163
> maybe we need to have https://phab.mercurial-scm.org/D2010, but we can drop `algo` (and maybe `use` also) from the name I guess.

I initially didn't have `use` and found it less clear. I also think "use changeset-centric" sounds weird. Maybe just `changesetcentric` as you said then, but that also seems unclear. It feels like we're working around https://phab.mercurial-scm.org/D2010. I can accept that we don't want to queue that, but I don't really like any of the shorter names either. Maybe the current name is fine given the docstring?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6163

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - April 2, 2019, 4:49 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> martinvonz wrote in copies.py:163
> I initially didn't have `use` and found it less clear. I also think "use changeset-centric" sounds weird. Maybe just `changesetcentric` as you said then, but that also seems unclear. It feels like we're working around https://phab.mercurial-scm.org/D2010. I can accept that we don't want to queue that, but I don't really like any of the shorter names either. Maybe the current name is fine given the docstring?

Or maybe there's another name that would work? Maybe `usecontextbasedalgo`? Or `shouldgetcopiesfromcontext`? Other suggestions?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6163

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - April 3, 2019, 11:49 a.m.
pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in copies.py:163
> Or maybe there's another name that would work? Maybe `usecontextbasedalgo`? Or `shouldgetcopiesfromcontext`? Other suggestions?

I don't have any concerns on name in specific. I was trying to work around https://phab.mercurial-scm.org/D2010 in the above suggestion.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6163

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - April 3, 2019, 1:04 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in copies.py:163
> I don't have any concerns on name in specific. I was trying to work around https://phab.mercurial-scm.org/D2010 in the above suggestion.

Does that mean you're okay with the current form of this patch?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6163

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - April 4, 2019, 5:34 p.m.
pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in copies.py:163
> Does that mean you're okay with the current form of this patch?

Yes.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6163

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -1205,7 +1205,7 @@ 
             wctx.copy(old, new)
 
 def getrenamedfn(repo, endrev=None):
-    if repo.ui.config('experimental', 'copies.read-from') == 'compatibility':
+    if copiesmod.usechangesetcentricalgo(repo):
         def getrenamed(fn, rev):
             ctx = repo[rev]
             p1copies = ctx.p1copies()
diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -160,13 +160,18 @@ 
     mb = b.manifest()
     return mb.filesnotin(ma, match=match)
 
+def usechangesetcentricalgo(repo):
+    """Checks if we should use changeset-centric copy algorithms"""
+    return (repo.ui.config('experimental', 'copies.read-from') ==
+            'compatibility')
+
 def _committedforwardcopies(a, b, match):
     """Like _forwardcopies(), but b.rev() cannot be None (working copy)"""
     # files might have to be traced back to the fctx parent of the last
     # one-side-only changeset, but not further back than that
     repo = a._repo
 
-    if repo.ui.config('experimental', 'copies.read-from') == 'compatibility':
+    if usechangesetcentricalgo(repo):
         return _changesetforwardcopies(a, b, match)
 
     debug = repo.ui.debugflag and repo.ui.configbool('devel', 'debug.copies')