Patchwork [4,of,7] context.status: call _dirstatestatus() from within _buildstatus()

login
register
mail settings
Submitter Martin von Zweigbergk
Date Nov. 2, 2014, 10:13 p.m.
Message ID <180d0d685dc5b944b4b2.1414966428@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6543/
State Accepted
Commit 9fbb50444d551e2ad05206e22c9c917418fcf548
Headers show

Comments

Martin von Zweigbergk - Nov. 2, 2014, 10:13 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@gmail.com>
# Date 1413095408 25200
#      Sat Oct 11 23:30:08 2014 -0700
# Branch stable
# Node ID 180d0d685dc5b944b4b28b7e450e21f47397313b
# Parent  ada8ab4086be7814622374c04aa5ab6c94deacb2
context.status: call _dirstatestatus() from within _buildstatus()

By making the call to _dirstatestatus() within _buildstatus(), it
becomes clearer that it's called only for the workingctx.
Sean Farley - Nov. 3, 2014, 6:41 p.m.
Martin von Zweigbergk writes:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@gmail.com>
> # Date 1413095408 25200
> #      Sat Oct 11 23:30:08 2014 -0700
> # Branch stable
> # Node ID 180d0d685dc5b944b4b28b7e450e21f47397313b
> # Parent  ada8ab4086be7814622374c04aa5ab6c94deacb2
> context.status: call _dirstatestatus() from within _buildstatus()
>
> By making the call to _dirstatestatus() within _buildstatus(), it
> becomes clearer that it's called only for the workingctx.

... but makes it more difficult for child objects to override.
Martin von Zweigbergk - Nov. 3, 2014, 6:46 p.m.
On Mon Nov 03 2014 at 10:41:46 AM Sean Farley <sean.michael.farley@gmail.com>
wrote:

>
> Martin von Zweigbergk writes:
>
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@gmail.com>
> > # Date 1413095408 25200
> > #      Sat Oct 11 23:30:08 2014 -0700
> > # Branch stable
> > # Node ID 180d0d685dc5b944b4b28b7e450e21f47397313b
> > # Parent  ada8ab4086be7814622374c04aa5ab6c94deacb2
> > context.status: call _dirstatestatus() from within _buildstatus()
> >
> > By making the call to _dirstatestatus() within _buildstatus(), it
> > becomes clearer that it's called only for the workingctx.
>
> ... but makes it more difficult for child objects to override.
>

Do you mean that it makes it harder to override workingctx._buildstatus()?
Sean Farley - Nov. 3, 2014, 7:32 p.m.
Martin von Zweigbergk writes:

> On Mon Nov 03 2014 at 10:41:46 AM Sean Farley <sean.michael.farley@gmail.com>
> wrote:
>
>>
>> Martin von Zweigbergk writes:
>>
>> > # HG changeset patch
>> > # User Martin von Zweigbergk <martinvonz@gmail.com>
>> > # Date 1413095408 25200
>> > #      Sat Oct 11 23:30:08 2014 -0700
>> > # Branch stable
>> > # Node ID 180d0d685dc5b944b4b28b7e450e21f47397313b
>> > # Parent  ada8ab4086be7814622374c04aa5ab6c94deacb2
>> > context.status: call _dirstatestatus() from within _buildstatus()
>> >
>> > By making the call to _dirstatestatus() within _buildstatus(), it
>> > becomes clearer that it's called only for the workingctx.
>>
>> ... but makes it more difficult for child objects to override.
>>
>
> Do you mean that it makes it harder to override workingctx._buildstatus()?

I just mean in the sense of object-oriented-ness. If _dirstatestatus
didn't exist, then most of the _buildstatus logic would be the same for
all contexts. Therefore, I felt it fit better in the pre-status phase.
Martin von Zweigbergk - Nov. 3, 2014, 7:39 p.m.
On Mon Nov 03 2014 at 11:32:26 AM Sean Farley <sean.michael.farley@gmail.com>
wrote:

>
> Martin von Zweigbergk writes:
>
> > On Mon Nov 03 2014 at 10:41:46 AM Sean Farley <
> sean.michael.farley@gmail.com>
> > wrote:
> >
> >>
> >> Martin von Zweigbergk writes:
> >>
> >> > # HG changeset patch
> >> > # User Martin von Zweigbergk <martinvonz@gmail.com>
> >> > # Date 1413095408 25200
> >> > #      Sat Oct 11 23:30:08 2014 -0700
> >> > # Branch stable
> >> > # Node ID 180d0d685dc5b944b4b28b7e450e21f47397313b
> >> > # Parent  ada8ab4086be7814622374c04aa5ab6c94deacb2
> >> > context.status: call _dirstatestatus() from within _buildstatus()
> >> >
> >> > By making the call to _dirstatestatus() within _buildstatus(), it
> >> > becomes clearer that it's called only for the workingctx.
> >>
> >> ... but makes it more difficult for child objects to override.
> >>
> >
> > Do you mean that it makes it harder to override
> workingctx._buildstatus()?
>
> I just mean in the sense of object-oriented-ness. If _dirstatestatus
> didn't exist, then most of the _buildstatus logic would be the same for
> all contexts. Therefore, I felt it fit better in the pre-status phase.
>

And after my patch, most of _prestatus() (well, all of it) becomes the same
in all contexts instead :-) It seem like it's just a matter of which method
is more or less similar. I found it hard to read before when one had to
keep in mind the type of ctx1/ctx2 when reading status() in order to
understand which method would get called.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -93,13 +93,13 @@ 
         """
         return match or matchmod.always(self._repo.root, self._repo.getcwd())
 
-    def _prestatus(self, other, s, match, listignored, listclean, listunknown):
+    def _prestatus(self, other):
         """provide a hook to allow child objects to preprocess status results
 
         For example, this allows other contexts, such as workingctx, to query
         the dirstate before comparing the manifests.
         """
-        return s
+        pass
 
     def _poststatus(self, other, s, match, listignored, listclean, listunknown):
         """provide a hook to allow child objects to postprocess status results
@@ -311,8 +311,8 @@ 
             ctx1, ctx2 = ctx2, ctx1
 
         match = ctx2._matchstatus(ctx1, match)
+        ctx2._prestatus(ctx1)
         r = [[], [], [], [], [], [], []]
-        r = ctx2._prestatus(ctx1, r, match, listignored, listclean, listunknown)
         r = ctx2._buildstatus(ctx1, r, match, listignored, listclean,
                               listunknown)
         r = ctx2._poststatus(ctx1, r, match, listignored, listclean,
@@ -1410,14 +1410,14 @@ 
                 del mf[f]
         return mf
 
-    def _prestatus(self, other, s, match, listignored, listclean, listunknown):
+    def _prestatus(self, other):
         """override the parent hook with a dirstate query
 
         We use this prestatus hook to populate the status with information from
         the dirstate.
         """
         # doesn't need to call super
-        return self._dirstatestatus(match, listignored, listclean, listunknown)
+        pass
 
     def _poststatus(self, other, s, match, listignored, listclean, listunknown):
         """override the parent hook with a filter for suspect symlinks
@@ -1462,6 +1462,7 @@ 
         building a new manifest if self (working directory) is not comparing
         against its parent (repo['.']).
         """
+        s = self._dirstatestatus(match, listignored, listclean, listunknown)
         if other != self._repo['.']:
             s = super(workingctx, self)._buildstatus(other, s, match,
                                                      listignored, listclean,