Patchwork [05,of,10,V2] context: move _manifest from committablectx to workingctx

login
register
mail settings
Submitter Durham Goode
Date March 8, 2017, 3:22 a.m.
Message ID <36bcc5d848c6bdf33d60.1488943357@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/18982/
State Accepted
Delegated to: Martin von Zweigbergk
Headers show

Comments

Durham Goode - March 8, 2017, 3:22 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1488938190 28800
#      Tue Mar 07 17:56:30 2017 -0800
# Node ID 36bcc5d848c6bdf33d604999631a0708d1b7f067
# Parent  68644896cbdcfc956079662668e010038bfe8048
context: move _manifest from committablectx to workingctx

committablectx had a _manifest implementation that was only used by the derived
workingctx class. The other derived versions, like memctx and metadataonlyctx,
define their own _manifest functions.

Let's move the function down to workingctx, and let's break it into two parts,
the _manifest part that reads from self._status, and the part that actually
builds the new manifest. This separation will let us reuse the builder code in a
future patch to answer _buildstatus with varying status inputs, since workingctx
has special behavior for _buildstatus that the other ctx's don't have.
via Mercurial-devel - March 8, 2017, 5:46 p.m.
On Tue, Mar 7, 2017 at 7:22 PM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1488938190 28800
> #      Tue Mar 07 17:56:30 2017 -0800
> # Node ID 36bcc5d848c6bdf33d604999631a0708d1b7f067
> # Parent  68644896cbdcfc956079662668e010038bfe8048
> context: move _manifest from committablectx to workingctx
>
> committablectx had a _manifest implementation that was only used by the derived
> workingctx class. The other derived versions, like memctx and metadataonlyctx,
> define their own _manifest functions.
>
> Let's move the function down to workingctx, and let's break it into two parts,
> the _manifest part that reads from self._status, and the part that actually
> builds the new manifest. This separation will let us reuse the builder code in a
> future patch to answer _buildstatus with varying status inputs, since workingctx
> has special behavior for _buildstatus that the other ctx's don't have.
>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -1266,35 +1266,6 @@ class committablectx(basectx):
>          return self._repo.dirstate.flagfunc(self._buildflagfunc)
>
>      @propertycache
> -    def _manifest(self):
> -        """generate a manifest corresponding to the values in self._status
> -
> -        This reuse the file nodeid from parent, but we append an extra letter
> -        when modified. Modified files get an extra 'm' while added files get
> -        an extra 'a'. This is used by manifests merge to see that files
> -        are different and by update logic to avoid deleting newly added files.
> -        """
> -        parents = self.parents()
> -
> -        man = parents[0].manifest().copy()
> -
> -        ff = self._flagfunc
> -        for i, l in ((addednodeid, self._status.added),
> -                     (modifiednodeid, self._status.modified)):
> -            for f in l:
> -                man[f] = i
> -                try:
> -                    man.setflag(f, ff(f))
> -                except OSError:
> -                    pass
> -
> -        for f in self._status.deleted + self._status.removed:
> -            if f in man:
> -                del man[f]
> -
> -        return man
> -
> -    @propertycache
>      def _status(self):
>          return self._repo.status()
>
> @@ -1655,6 +1626,41 @@ class workingctx(committablectx):
>
>          return s
>
> +    @propertycache
> +    def _manifest(self):
> +        """generate a manifest corresponding to the values in self._status
> +
> +        This reuse the file nodeid from parent, but we use special node
> +        identifiers for added and modified files. This is used by manifests
> +        merge to see that files are different and by update logic to avoid
> +        deleting newly added files.
> +        """
> +        return self._buildstatusmanifest(self._status)
> +
> +    def _buildstatusmanifest(self, status):
> +        """Builds a manifest that includes the given status results, if this is
> +        a working copy context. For non-working copy contexts, it just returns
> +        the normal manifest."""


Looks like this description was from an earlier version of the patch
(because now it's always a working copy context, right?). What do you
want me to replace it with?


> +        parents = self.parents()
> +
> +        man = parents[0].manifest().copy()
> +
> +        ff = self._flagfunc
> +        for i, l in ((addednodeid, status.added),
> +                     (modifiednodeid, status.modified)):
> +            for f in l:
> +                man[f] = i
> +                try:
> +                    man.setflag(f, ff(f))
> +                except OSError:
> +                    pass
> +
> +        for f in status.deleted + status.removed:
> +            if f in man:
> +                del man[f]
> +
> +        return man
> +
>      def _buildstatus(self, other, s, match, listignored, listclean,
>                       listunknown):
>          """build a status with respect to another context
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Durham Goode - March 8, 2017, 5:56 p.m.
On 3/8/17 9:46 AM, Martin von Zweigbergk wrote:
> On Tue, Mar 7, 2017 at 7:22 PM, Durham Goode <durham@fb.com> wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1488938190 28800
>> #      Tue Mar 07 17:56:30 2017 -0800
>> # Node ID 36bcc5d848c6bdf33d604999631a0708d1b7f067
>> # Parent  68644896cbdcfc956079662668e010038bfe8048
>> context: move _manifest from committablectx to workingctx
>>
>> committablectx had a _manifest implementation that was only used by the derived
>> workingctx class. The other derived versions, like memctx and metadataonlyctx,
>> define their own _manifest functions.
>>
>> Let's move the function down to workingctx, and let's break it into two parts,
>> the _manifest part that reads from self._status, and the part that actually
>> builds the new manifest. This separation will let us reuse the builder code in a
>> future patch to answer _buildstatus with varying status inputs, since workingctx
>> has special behavior for _buildstatus that the other ctx's don't have.
>>
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -1266,35 +1266,6 @@ class committablectx(basectx):
>>          return self._repo.dirstate.flagfunc(self._buildflagfunc)
>>
>>      @propertycache
>> -    def _manifest(self):
>> -        """generate a manifest corresponding to the values in self._status
>> -
>> -        This reuse the file nodeid from parent, but we append an extra letter
>> -        when modified. Modified files get an extra 'm' while added files get
>> -        an extra 'a'. This is used by manifests merge to see that files
>> -        are different and by update logic to avoid deleting newly added files.
>> -        """
>> -        parents = self.parents()
>> -
>> -        man = parents[0].manifest().copy()
>> -
>> -        ff = self._flagfunc
>> -        for i, l in ((addednodeid, self._status.added),
>> -                     (modifiednodeid, self._status.modified)):
>> -            for f in l:
>> -                man[f] = i
>> -                try:
>> -                    man.setflag(f, ff(f))
>> -                except OSError:
>> -                    pass
>> -
>> -        for f in self._status.deleted + self._status.removed:
>> -            if f in man:
>> -                del man[f]
>> -
>> -        return man
>> -
>> -    @propertycache
>>      def _status(self):
>>          return self._repo.status()
>>
>> @@ -1655,6 +1626,41 @@ class workingctx(committablectx):
>>
>>          return s
>>
>> +    @propertycache
>> +    def _manifest(self):
>> +        """generate a manifest corresponding to the values in self._status
>> +
>> +        This reuse the file nodeid from parent, but we use special node
>> +        identifiers for added and modified files. This is used by manifests
>> +        merge to see that files are different and by update logic to avoid
>> +        deleting newly added files.
>> +        """
>> +        return self._buildstatusmanifest(self._status)
>> +
>> +    def _buildstatusmanifest(self, status):
>> +        """Builds a manifest that includes the given status results, if this is
>> +        a working copy context. For non-working copy contexts, it just returns
>> +        the normal manifest."""
>
>
> Looks like this description was from an earlier version of the patch
> (because now it's always a working copy context, right?). What do you
> want me to replace it with?

A later patches introduce a _buildstatusmanifest() function on the 
basectx that just returns _self.manifest. I was just too lazy to change 
the comment here then change it back in the later patch.

>> +        parents = self.parents()
>> +
>> +        man = parents[0].manifest().copy()
>> +
>> +        ff = self._flagfunc
>> +        for i, l in ((addednodeid, status.added),
>> +                     (modifiednodeid, status.modified)):
>> +            for f in l:
>> +                man[f] = i
>> +                try:
>> +                    man.setflag(f, ff(f))
>> +                except OSError:
>> +                    pass
>> +
>> +        for f in status.deleted + status.removed:
>> +            if f in man:
>> +                del man[f]
>> +
>> +        return man
>> +
>>      def _buildstatus(self, other, s, match, listignored, listclean,
>>                       listunknown):
>>          """build a status with respect to another context
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=edaqYnwqBrkdf9lyCr2VHlwEOrw4aM2BOIn1zLX3_XE&s=ZyBsFB7mt6ZvhfS4-ISncmyuFWRzj3hd1S2smSAWe1I&e=
via Mercurial-devel - March 8, 2017, 6:01 p.m.
On Wed, Mar 8, 2017 at 9:56 AM, Durham Goode <durham@fb.com> wrote:
>
>
> On 3/8/17 9:46 AM, Martin von Zweigbergk wrote:
>>
>> On Tue, Mar 7, 2017 at 7:22 PM, Durham Goode <durham@fb.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1488938190 28800
>>> #      Tue Mar 07 17:56:30 2017 -0800
>>> # Node ID 36bcc5d848c6bdf33d604999631a0708d1b7f067
>>> # Parent  68644896cbdcfc956079662668e010038bfe8048
>>> context: move _manifest from committablectx to workingctx
>>>
>>> committablectx had a _manifest implementation that was only used by the
>>> derived
>>> workingctx class. The other derived versions, like memctx and
>>> metadataonlyctx,
>>> define their own _manifest functions.
>>>
>>> Let's move the function down to workingctx, and let's break it into two
>>> parts,
>>> the _manifest part that reads from self._status, and the part that
>>> actually
>>> builds the new manifest. This separation will let us reuse the builder
>>> code in a
>>> future patch to answer _buildstatus with varying status inputs, since
>>> workingctx
>>> has special behavior for _buildstatus that the other ctx's don't have.
>>>
>>> diff --git a/mercurial/context.py b/mercurial/context.py
>>> --- a/mercurial/context.py
>>> +++ b/mercurial/context.py
>>> @@ -1266,35 +1266,6 @@ class committablectx(basectx):
>>>          return self._repo.dirstate.flagfunc(self._buildflagfunc)
>>>
>>>      @propertycache
>>> -    def _manifest(self):
>>> -        """generate a manifest corresponding to the values in
>>> self._status
>>> -
>>> -        This reuse the file nodeid from parent, but we append an extra
>>> letter
>>> -        when modified. Modified files get an extra 'm' while added files
>>> get
>>> -        an extra 'a'. This is used by manifests merge to see that files
>>> -        are different and by update logic to avoid deleting newly added
>>> files.
>>> -        """
>>> -        parents = self.parents()
>>> -
>>> -        man = parents[0].manifest().copy()
>>> -
>>> -        ff = self._flagfunc
>>> -        for i, l in ((addednodeid, self._status.added),
>>> -                     (modifiednodeid, self._status.modified)):
>>> -            for f in l:
>>> -                man[f] = i
>>> -                try:
>>> -                    man.setflag(f, ff(f))
>>> -                except OSError:
>>> -                    pass
>>> -
>>> -        for f in self._status.deleted + self._status.removed:
>>> -            if f in man:
>>> -                del man[f]
>>> -
>>> -        return man
>>> -
>>> -    @propertycache
>>>      def _status(self):
>>>          return self._repo.status()
>>>
>>> @@ -1655,6 +1626,41 @@ class workingctx(committablectx):
>>>
>>>          return s
>>>
>>> +    @propertycache
>>> +    def _manifest(self):
>>> +        """generate a manifest corresponding to the values in
>>> self._status
>>> +
>>> +        This reuse the file nodeid from parent, but we use special node
>>> +        identifiers for added and modified files. This is used by
>>> manifests
>>> +        merge to see that files are different and by update logic to
>>> avoid
>>> +        deleting newly added files.
>>> +        """
>>> +        return self._buildstatusmanifest(self._status)
>>> +
>>> +    def _buildstatusmanifest(self, status):
>>> +        """Builds a manifest that includes the given status results, if
>>> this is
>>> +        a working copy context. For non-working copy contexts, it just
>>> returns
>>> +        the normal manifest."""
>>
>>
>>
>> Looks like this description was from an earlier version of the patch
>> (because now it's always a working copy context, right?). What do you
>> want me to replace it with?
>
>
> A later patches introduce a _buildstatusmanifest() function on the basectx
> that just returns _self.manifest. I was just too lazy to change the comment
> here then change it back in the later patch.

That part of the description didn't exist before this patch. I'll just
drop the references to "working copy" in this patch then.

>
>>> +        parents = self.parents()
>>> +
>>> +        man = parents[0].manifest().copy()
>>> +
>>> +        ff = self._flagfunc
>>> +        for i, l in ((addednodeid, status.added),
>>> +                     (modifiednodeid, status.modified)):
>>> +            for f in l:
>>> +                man[f] = i
>>> +                try:
>>> +                    man.setflag(f, ff(f))
>>> +                except OSError:
>>> +                    pass
>>> +
>>> +        for f in status.deleted + status.removed:
>>> +            if f in man:
>>> +                del man[f]
>>> +
>>> +        return man
>>> +
>>>      def _buildstatus(self, other, s, match, listignored, listclean,
>>>                       listunknown):
>>>          """build a status with respect to another context
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@mercurial-scm.org
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=edaqYnwqBrkdf9lyCr2VHlwEOrw4aM2BOIn1zLX3_XE&s=ZyBsFB7mt6ZvhfS4-ISncmyuFWRzj3hd1S2smSAWe1I&e=

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1266,35 +1266,6 @@  class committablectx(basectx):
         return self._repo.dirstate.flagfunc(self._buildflagfunc)
 
     @propertycache
-    def _manifest(self):
-        """generate a manifest corresponding to the values in self._status
-
-        This reuse the file nodeid from parent, but we append an extra letter
-        when modified. Modified files get an extra 'm' while added files get
-        an extra 'a'. This is used by manifests merge to see that files
-        are different and by update logic to avoid deleting newly added files.
-        """
-        parents = self.parents()
-
-        man = parents[0].manifest().copy()
-
-        ff = self._flagfunc
-        for i, l in ((addednodeid, self._status.added),
-                     (modifiednodeid, self._status.modified)):
-            for f in l:
-                man[f] = i
-                try:
-                    man.setflag(f, ff(f))
-                except OSError:
-                    pass
-
-        for f in self._status.deleted + self._status.removed:
-            if f in man:
-                del man[f]
-
-        return man
-
-    @propertycache
     def _status(self):
         return self._repo.status()
 
@@ -1655,6 +1626,41 @@  class workingctx(committablectx):
 
         return s
 
+    @propertycache
+    def _manifest(self):
+        """generate a manifest corresponding to the values in self._status
+
+        This reuse the file nodeid from parent, but we use special node
+        identifiers for added and modified files. This is used by manifests
+        merge to see that files are different and by update logic to avoid
+        deleting newly added files.
+        """
+        return self._buildstatusmanifest(self._status)
+
+    def _buildstatusmanifest(self, status):
+        """Builds a manifest that includes the given status results, if this is
+        a working copy context. For non-working copy contexts, it just returns
+        the normal manifest."""
+        parents = self.parents()
+
+        man = parents[0].manifest().copy()
+
+        ff = self._flagfunc
+        for i, l in ((addednodeid, status.added),
+                     (modifiednodeid, status.modified)):
+            for f in l:
+                man[f] = i
+                try:
+                    man.setflag(f, ff(f))
+                except OSError:
+                    pass
+
+        for f in status.deleted + status.removed:
+            if f in man:
+                del man[f]
+
+        return man
+
     def _buildstatus(self, other, s, match, listignored, listclean,
                      listunknown):
         """build a status with respect to another context