Patchwork [01,of,16,V3] dirstate: separate 'lookup' status field from others

login
register
mail settings
Submitter Martin von Zweigbergk
Date Oct. 10, 2014, 10:20 p.m.
Message ID <6b0dd84f33b7ce6a932e.1412979633@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6193/
State Superseded
Headers show

Comments

Martin von Zweigbergk - Oct. 10, 2014, 10:20 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@gmail.com>
# Date 1412397850 25200
#      Fri Oct 03 21:44:10 2014 -0700
# Node ID 6b0dd84f33b7ce6a932e9a46f6751eeda381ecfc
# Parent  1533e642262de32c5a2445789710f49237019fd6
dirstate: separate 'lookup' status field from others

The status tuple returned from dirstate.status() has an additional
field compared to the other status tuples: lookup/unsure. This field
is just an optimization and not something most callers care about
(they want the resolved value of 'modified' or 'clean'). To prepare
for a single future status type, let's separate out the 'lookup' field
from the rest by having dirstate.status() return a pair: (lookup,
status).
Mads Kiilerich - Oct. 12, 2014, 1 a.m.
1+2 LGTM, but

On 10/11/2014 12:20 AM, Martin von Zweigbergk wrote:
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -26,6 +26,51 @@
>       def join(self, obj, fname):
>           return obj._join(fname)
>   
> +class status(tuple):
> +    '''Named tuple with a list of files per status. The 'deleted', 'unknown'
> +       and 'ignored' properties are only relevant to the working copy.
> +    '''
> +
> +    __slots__ = ()
> +
> +    def __new__(cls, modified=None, added=None, removed=None, deleted=None,
> +                unknown=None, ignored=None, clean=None):
> +        return tuple.__new__(cls, (modified or [], added or [], removed or [],
> +                                   deleted or [], unknown or [], ignored or [],
> +                                   clean or []))

I would expect:
l = []
s = status(l)
l.append(7)
assert s.modified == [7]

The fallback to a new empty list should only be applied if the value is 
None - not for other false values.

For now, please just make the parameters mandatory. A later follow-up 
patch can always make them optional ... if we can be bothered ;-)

/Mads
Martin von Zweigbergk - Oct. 12, 2014, 5:10 a.m.
On Sat, Oct 11, 2014 at 6:00 PM, Mads Kiilerich <mads@kiilerich.com> wrote:
> 1+2 LGTM, but
>
> On 10/11/2014 12:20 AM, Martin von Zweigbergk wrote:
>>
>> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
>> --- a/mercurial/dirstate.py
>> +++ b/mercurial/dirstate.py
>> @@ -26,6 +26,51 @@
>>       def join(self, obj, fname):
>>           return obj._join(fname)
>>   +class status(tuple):
>> +    '''Named tuple with a list of files per status. The 'deleted',
>> 'unknown'
>> +       and 'ignored' properties are only relevant to the working copy.
>> +    '''
>> +
>> +    __slots__ = ()
>> +
>> +    def __new__(cls, modified=None, added=None, removed=None,
>> deleted=None,
>> +                unknown=None, ignored=None, clean=None):
>> +        return tuple.__new__(cls, (modified or [], added or [], removed
>> or [],
>> +                                   deleted or [], unknown or [], ignored
>> or [],
>> +                                   clean or []))
>
>
> I would expect:
> l = []
> s = status(l)
> l.append(7)
> assert s.modified == [7]
>
> The fallback to a new empty list should only be applied if the value is None
> - not for other false values.

Ah, of course. Sorry about that.

> For now, please just make the parameters mandatory. A later follow-up patch
> can always make them optional ... if we can be bothered ;-)

Will do. V4 coming up

Patch

diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -136,8 +136,8 @@ 
 
 def lfdirstatestatus(lfdirstate, repo, rev):
     match = match_.always(repo.root, repo.getcwd())
-    s = lfdirstate.status(match, [], False, False, False)
-    unsure, modified, added, removed, missing, unknown, ignored, clean = s
+    unsure, s = lfdirstate.status(match, [], False, False, False)
+    modified, added, removed, missing, unknown, ignored, clean = s
     for lfile in unsure:
         try:
             fctx = repo[rev][standin(lfile)]
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -351,9 +351,9 @@ 
     wlock = repo.wlock()
     try:
         lfdirstate = lfutil.openlfdirstate(ui, repo)
-        s = lfdirstate.status(match_.always(repo.root, repo.getcwd()),
-            [], False, False, False)
-        (unsure, modified, added, removed, missing, unknown, ignored, clean) = s
+        unsure, s = lfdirstate.status(match_.always(repo.root, repo.getcwd()),
+                                      [], False, False, False)
+        modified = s[0]
 
         if opts['check']:
             mod = len(modified) > 0
@@ -1110,9 +1110,9 @@ 
         return orig(repo, pats, opts, dry_run, similarity)
     # Get the list of missing largefiles so we can remove them
     lfdirstate = lfutil.openlfdirstate(repo.ui, repo)
-    s = lfdirstate.status(match_.always(repo.root, repo.getcwd()), [], False,
-        False, False)
-    (unsure, modified, added, removed, missing, unknown, ignored, clean) = s
+    unsure, s = lfdirstate.status(match_.always(repo.root, repo.getcwd()), [],
+                                  False, False, False)
+    missing = s[3]
 
     # Call into the normal remove code, but the removing of the standin, we want
     # to have handled by original addremove.  Monkey patching here makes sure
@@ -1288,9 +1288,10 @@ 
             # update standins for linear-merge or force-branch-merge,
             # because largefiles in the working directory may be modified
             lfdirstate = lfutil.openlfdirstate(repo.ui, repo)
-            s = lfdirstate.status(match_.always(repo.root, repo.getcwd()),
-                                  [], False, False, False)
-            unsure, modified, added = s[:3]
+            unsure, s = lfdirstate.status(match_.always(repo.root,
+                                                        repo.getcwd()),
+                                          [], False, False, False)
+            modified, added = s[:2]
             for lfile in unsure + modified + added:
                 lfutil.updatestandin(repo, lfutil.standin(lfile))
 
diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py
+++ b/hgext/largefiles/reposetup.py
@@ -159,10 +159,10 @@ 
                                     if sfindirstate(f)]
                     # Don't waste time getting the ignored and unknown
                     # files from lfdirstate
-                    s = lfdirstate.status(match, [], False,
-                            listclean, False)
-                    (unsure, modified, added, removed, missing, _unknown,
-                            _ignored, clean) = s
+                    unsure, s = lfdirstate.status(match, [], False, listclean,
+                                                  False)
+                    (modified, added, removed, missing, _unknown, _ignored,
+                     clean) = s
                     if parentworking:
                         for lfile in unsure:
                             standin = lfutil.standin(lfile)
@@ -296,9 +296,9 @@ 
                     # large.
                     lfdirstate = lfutil.openlfdirstate(ui, self)
                     dirtymatch = match_.always(self.root, self.getcwd())
-                    s = lfdirstate.status(dirtymatch, [], False, False, False)
-                    (unsure, modified, added, removed, _missing, _unknown,
-                            _ignored, _clean) = s
+                    unsure, s = lfdirstate.status(dirtymatch, [], False, False,
+                                                  False)
+                    modified, added, removed = s[:3]
                     modifiedfiles = unsure + modified + added + removed
                     lfiles = lfutil.listlfiles(self)
                     # this only loops through largefiles that exist (not
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1418,9 +1418,9 @@ 
         subrepos = []
         if '.hgsub' in self:
             subrepos = sorted(self.substate)
-        s = self._repo.dirstate.status(match, subrepos, listignored,
-                                       listclean, listunknown)
-        cmp, modified, added, removed, deleted, unknown, ignored, clean = s
+        cmp, s = self._repo.dirstate.status(match, subrepos, listignored,
+                                            listclean, listunknown)
+        modified, added, removed, deleted, unknown, ignored, clean = s
 
         # check for any possibly clean files
         if cmp:
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -802,8 +802,8 @@ 
 
     def status(self, match, subrepos, ignored, clean, unknown):
         '''Determine the status of the working copy relative to the
-        dirstate and return a tuple of lists (unsure, modified, added,
-        removed, deleted, unknown, ignored, clean), where:
+        dirstate and return a nested tuple of lists (unsure, (modified, added,
+        removed, deleted, unknown, ignored, clean)), where:
 
           unsure:
             files that might have been modified since the dirstate was
@@ -901,8 +901,8 @@ 
             elif state == 'r':
                 radd(fn)
 
-        return (lookup, modified, added, removed, deleted, unknown, ignored,
-                clean)
+        return (lookup, (modified, added, removed, deleted, unknown, ignored,
+                         clean))
 
     def matches(self, match):
         '''