Patchwork D3000: addremove: remove opts from scmutil.addremove

login
register
mail settings
Submitter phabricator
Date March 31, 2018, 6:29 p.m.
Message ID <differential-rev-PHID-DREV-wnaulrhkjfksmwjsytbr-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30074/
State Superseded
Headers show

Comments

phabricator - March 31, 2018, 6:29 p.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  contrib/perf.py
  hgext/largefiles/overrides.py
  mercurial/cmdutil.py
  mercurial/commands.py
  mercurial/scmutil.py
  mercurial/subrepo.py

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - March 31, 2018, 6:37 p.m.
khanchi97 added a subscriber: yuja.
khanchi97 added a comment.


  I ran all the tests those are related to addremove and they are passing.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - April 1, 2018, 1:57 a.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  Sorry, I didn't notice that the opts dict carries extra options other
  than subrepos, dry_run, and similarity. Because of that, my idea of
  dropping `opts` turns out to be worse than the current state.
  
  Maybe we can get rid of `dry_run` and `similarity` instead, but we'll
  have to be careful since the default `similarity` value varies on command.

INLINE COMMENTS

> commands.py:252
> +    subrepos, after = opts.get('subrepos'), opts.get('after')
> +    large, lfsize = opts.get('large'), opts.get('lfsize')
>      try:

Oops, largefiles options in core.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
sushil khanchi - April 1, 2018, 1:53 p.m.
okay, so I have sent another revision which removes `dry_run` and
`similarity` from scmutil.addremove

On Sun, Apr 1, 2018 at 7:27 AM, yuja (Yuya Nishihara) <
phabricator@mercurial-scm.org> wrote:

> yuja requested changes to this revision.
> yuja added a comment.
> This revision now requires changes to proceed.
>
>
>   Sorry, I didn't notice that the opts dict carries extra options other
>   than subrepos, dry_run, and similarity. Because of that, my idea of
>   dropping `opts` turns out to be worse than the current state.
>
>   Maybe we can get rid of `dry_run` and `similarity` instead, but we'll
>   have to be careful since the default `similarity` value varies on
> command.
>
> INLINE COMMENTS
>
> > commands.py:252
> > +    subrepos, after = opts.get('subrepos'), opts.get('after')
> > +    large, lfsize = opts.get('large'), opts.get('lfsize')
> >      try:
>
> Oops, largefiles options in core.
>
> REPOSITORY
>   rHG Mercurial
>
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D3000
>
> To: khanchi97, #hg-reviewers, yuja
> Cc: yuja, mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
phabricator - April 2, 2018, 2:53 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  Other than that, the patch looks good to me. Thanks!

INLINE COMMENTS

> perf.py:426
>          matcher = scmutil.match(repo[None])
> -        timer(lambda: scmutil.addremove(repo, matcher, "", dry_run=True))
> +        timer(lambda: scmutil.addremove(repo, matcher, "", opts))
>      finally:

Here opts['dry_run'] must be set.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, yuja
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -287,7 +287,8 @@ 
     def add(self, ui, match, prefix, explicitonly, **opts):
         return []
 
-    def addremove(self, matcher, prefix, opts, dry_run, similarity):
+    def addremove(self, matcher, prefix, subrepos, dry_run=None,
+                  after=None, large=None, lfsize=None, similarity=None):
         self.ui.warn("%s: %s" % (prefix, _("addremove is not supported")))
         return 1
 
@@ -510,15 +511,16 @@ 
                            explicitonly, **opts)
 
     @annotatesubrepoerror
-    def addremove(self, m, prefix, opts, dry_run, similarity):
+    def addremove(self, m, prefix, subrepos, dry_run=None, after=None,
+                  large=None, lfsize=None, similarity=None):
         # In the same way as sub directories are processed, once in a subrepo,
         # always entry any of its subrepos.  Don't corrupt the options that will
         # be used to process sibling subrepos however.
-        opts = copy.copy(opts)
-        opts['subrepos'] = True
+        subrepos = True
         return scmutil.addremove(self._repo, m,
-                                 self.wvfs.reljoin(prefix, self._path), opts,
-                                 dry_run, similarity)
+                                 self.wvfs.reljoin(prefix, self._path),
+                                 subrepos, dry_run, after, large, lfsize,
+                                 similarity)
 
     @annotatesubrepoerror
     def cat(self, match, fm, fntemplate, prefix, **opts):
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -731,25 +731,20 @@ 
             if tostrip:
                 repair.delayedstrip(repo.ui, repo, tostrip, operation)
 
-def addremove(repo, matcher, prefix, opts=None, dry_run=None, similarity=None):
-    if opts is None:
-        opts = {}
+def addremove(repo, matcher, prefix, subrepos, dry_run=None,
+              after=None, large=None, lfsize=None, similarity=None):
     m = matcher
-    if dry_run is None:
-        dry_run = opts.get('dry_run')
-    if similarity is None:
-        similarity = float(opts.get('similarity') or 0)
-
     ret = 0
     join = lambda f: os.path.join(prefix, f)
 
     wctx = repo[None]
     for subpath in sorted(wctx.substate):
         submatch = matchmod.subdirmatcher(subpath, m)
-        if opts.get('subrepos') or m.exact(subpath) or any(submatch.files()):
+        if subrepos or m.exact(subpath) or any(submatch.files()):
             sub = wctx.sub(subpath)
             try:
-                if sub.addremove(submatch, prefix, opts, dry_run, similarity):
+                if sub.addremove(submatch, prefix, subrepos, dry_run, after,
+                                 large, lfsize, similarity):
                     ret = 1
             except error.LookupError:
                 repo.ui.status(_("skipping missing subrepository: %s\n")
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -247,14 +247,18 @@ 
     Returns 0 if all files are successfully added.
     """
     opts = pycompat.byteskwargs(opts)
+    dry_run = opts.get('dry_run')
+    subrepos, after = opts.get('subrepos'), opts.get('after')
+    large, lfsize = opts.get('large'), opts.get('lfsize')
     try:
         sim = float(opts.get('similarity') or 100)
     except ValueError:
         raise error.Abort(_('similarity must be a number'))
     if sim < 0 or sim > 100:
         raise error.Abort(_('similarity must be between 0 and 100'))
     matcher = scmutil.match(repo[None], pats, opts)
-    return scmutil.addremove(repo, matcher, "", opts, similarity=sim / 100.0)
+    return scmutil.addremove(repo, matcher, "", subrepos, dry_run,
+                             after, large, lfsize, similarity=sim / 100.0)
 
 @command('^annotate|blame',
     [('r', 'rev', '', _('annotate the specified revision'), _('REV')),
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2274,6 +2274,7 @@ 
     if date:
         opts['date'] = dateutil.parsedate(date)
     message = logmessage(ui, opts)
+    subrepos = opts.get('subrepos')
     matcher = scmutil.match(repo[None], pats, opts)
 
     dsguard = None
@@ -2283,7 +2284,7 @@ 
         dsguard = dirstateguard.dirstateguard(repo, 'commit')
     with dsguard or util.nullcontextmanager():
         if dsguard:
-            if scmutil.addremove(repo, matcher, "", opts) != 0:
+            if scmutil.addremove(repo, matcher, "", subrepos) != 0:
                 raise error.Abort(
                     _("failed to mark all new/missing files as added/removed"))
 
@@ -2350,8 +2351,9 @@ 
         # add/remove the files to the working copy if the "addremove" option
         # was specified.
         matcher = scmutil.match(wctx, pats, opts)
+        subrepos = opts.get('subrepos')
         if (opts.get('addremove')
-            and scmutil.addremove(repo, matcher, "", opts)):
+            and scmutil.addremove(repo, matcher, "", subrepos)):
             raise error.Abort(
                 _("failed to mark all new/missing files as added/removed"))
 
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -106,10 +106,9 @@ 
     scmutil.matchandpats = getattr(scmutil.matchandpats, 'oldmatchandpats',
             scmutil.matchandpats)
 
-def addlargefiles(ui, repo, isaddremove, matcher, **opts):
-    large = opts.get(r'large')
+def addlargefiles(ui, repo, isaddremove, matcher, dry_run, large, lfsize):
     lfsize = lfutil.getminsize(
-        ui, lfutil.islfilesrepo(repo), opts.get(r'lfsize'))
+        ui, lfutil.islfilesrepo(repo), lfsize)
 
     lfmatcher = None
     if lfutil.islfilesrepo(repo):
@@ -158,7 +157,7 @@ 
     # Need to lock, otherwise there could be a race condition between
     # when standins are created and added to the repo.
     with repo.wlock():
-        if not opts.get(r'dry_run'):
+        if not dry_run:
             standins = []
             lfdirstate = lfutil.openlfdirstate(ui, repo)
             for f in lfnames:
@@ -178,8 +177,8 @@ 
         added = [f for f in lfnames if f not in bad]
     return added, bad
 
-def removelargefiles(ui, repo, isaddremove, matcher, dryrun, **opts):
-    after = opts.get(r'after')
+def removelargefiles(ui, repo, isaddremove, matcher, dryrun=None,
+                     after=None, force=None):
     m = composelargefilematcher(matcher, repo[None].manifest())
     try:
         repo.lfstatus = True
@@ -263,7 +262,10 @@ 
     if opts.get(r'normal'):
         return orig(ui, repo, matcher, prefix, explicitonly, **opts)
 
-    ladded, lbad = addlargefiles(ui, repo, False, matcher, **opts)
+    large, lfsize = opts.get(r'large'), opts.get(r'lfsize')
+    dry_run = opts.get(r'dry_run')
+    ladded, lbad = addlargefiles(ui, repo, False, matcher,
+                                 dry_run, large, lfsize)
     normalmatcher = composenormalfilematcher(matcher, repo[None].manifest(),
                                              ladded)
     bad = orig(ui, repo, normalmatcher, prefix, explicitonly, **opts)
@@ -1214,12 +1216,11 @@ 
     finally:
         repo.lfstatus = False
 
-def scmutiladdremove(orig, repo, matcher, prefix, opts=None, dry_run=None,
-                     similarity=None):
-    if opts is None:
-        opts = {}
+def scmutiladdremove(orig, repo, matcher, prefix, subrepos, dry_run=None,
+                     after=None, large=None, lfsize=None, similarity=None):
     if not lfutil.islfilesrepo(repo):
-        return orig(repo, matcher, prefix, opts, dry_run, similarity)
+        return orig(repo, matcher, prefix, subrepos, dry_run,
+                    similarity=similarity)
     # Get the list of missing largefiles so we can remove them
     lfdirstate = lfutil.openlfdirstate(repo.ui, repo)
     unsure, s = lfdirstate.status(matchmod.always(repo.root, repo.getcwd()),
@@ -1240,17 +1241,17 @@ 
         matchfn = m.matchfn
         m.matchfn = lambda f: f in s.deleted and matchfn(f)
 
-        removelargefiles(repo.ui, repo, True, m, opts.get('dry_run'),
-                         **pycompat.strkwargs(opts))
+        removelargefiles(repo.ui, repo, True, m, dry_run, after=after)
     # Call into the normal add code, and any files that *should* be added as
     # largefiles will be
-    added, bad = addlargefiles(repo.ui, repo, True, matcher,
-                               **pycompat.strkwargs(opts))
+    added, bad = addlargefiles(repo.ui, repo, True, matcher, large=large,
+                               lfsize=lfsize, dry_run=dry_run)
     # Now that we've handled largefiles, hand off to the original addremove
     # function to take care of the rest.  Make sure it doesn't do anything with
     # largefiles by passing a matcher that will ignore them.
     matcher = composenormalfilematcher(matcher, repo[None].manifest(), added)
-    return orig(repo, matcher, prefix, opts, dry_run, similarity)
+    return orig(repo, matcher, prefix, subrepos, dry_run,
+                similarity=similarity)
 
 # Calling purge with --all will cause the largefiles to be deleted.
 # Override repo.status to prevent this from happening.
diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -419,11 +419,13 @@ 
 @command('perfaddremove', formatteropts)
 def perfaddremove(ui, repo, **opts):
     timer, fm = gettimer(ui, opts)
+    subrepos = opts.get('subrepos')
     try:
         oldquiet = repo.ui.quiet
         repo.ui.quiet = True
         matcher = scmutil.match(repo[None])
-        timer(lambda: scmutil.addremove(repo, matcher, "", dry_run=True))
+        timer(lambda: scmutil.addremove(repo, matcher, "",
+                                        subrepos, dry_run=True))
     finally:
         repo.ui.quiet = oldquiet
         fm.end()