Patchwork [1,of,7] context.status: remove overriding in workingctx

login
register
mail settings
Submitter Martin von Zweigbergk
Date Nov. 2, 2014, 10:13 p.m.
Message ID <d68ec9fdce58cb01ec34.1414966425@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6541/
State Accepted
Commit 7ea2ef4479f218632e899411a771bd05aa033432
Headers show

Comments

Martin von Zweigbergk - Nov. 2, 2014, 10:13 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1414097000 25200
#      Thu Oct 23 13:43:20 2014 -0700
# Branch stable
# Node ID d68ec9fdce58cb01ec349e2731cf09247b10f436
# Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
context.status: remove overriding in workingctx

The workingctx method simply calls the super method. The only effect
it has is that it uses a different default argument for the 'other'
argument. The only in-tree caller is patch.diff, which always passes
an argument to the method, so it should be safe to remove the
overriding. Having the default argument depend on the type seems
rather dangerous anyway.
Sean Farley - Nov. 3, 2014, 6:34 p.m.
Martin von Zweigbergk writes:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1414097000 25200
> #      Thu Oct 23 13:43:20 2014 -0700
> # Branch stable
> # Node ID d68ec9fdce58cb01ec349e2731cf09247b10f436
> # Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
> context.status: remove overriding in workingctx

Overall, I strongly think this series is moving in the wrong
direction. Some back story:

The 'status' method was originally located in localrepo and was a giant
function that had a lot of hard-to-follow code paths making it difficult
to add memctx support. For example, see here:

http://selenic.com/hg/file/3b4c/mercurial/localrepo.py#l1504

The path I took was to put the 'status' operation into the context
object itself. Ideally, this would be one method that each context could
overload. Unfortunately, there is some caching done due to the expense
of querying the disk, so that is complicated the design some. That
caching led to having a pre/post status operation so that the context
could extend or filter out files to its own needs.

> The workingctx method simply calls the super method. The only effect
> it has is that it uses a different default argument for the 'other'
> argument. The only in-tree caller is patch.diff, which always passes
> an argument to the method, so it should be safe to remove the
> overriding. Having the default argument depend on the type seems
> rather dangerous anyway.

The only in-tree caller might be patch.diff but there were some
extensions that used it also. The design constraint was that
repo[None].status() had to maintain its fast path for checking the
dirstate and disk.
Martin von Zweigbergk - Nov. 3, 2014, 6:55 p.m.
On Mon Nov 03 2014 at 10:34:50 AM Sean Farley <sean.michael.farley@gmail.com>
wrote:

>
> Martin von Zweigbergk writes:
>
> > The workingctx method simply calls the super method. The only effect
> > it has is that it uses a different default argument for the 'other'
> > argument. The only in-tree caller is patch.diff, which always passes
> > an argument to the method, so it should be safe to remove the
> > overriding. Having the default argument depend on the type seems
> > rather dangerous anyway.
>
> The only in-tree caller might be patch.diff but there were some
> extensions that used it also.


What I don't like is that ctx.status() might be comparing to either
repo['.'] or repo[None] depending on what type ctx has. I don't know if we
have code where the 'ctx' could be either type of context, but it just
feels a little weird. I don't care that much, so if we want to keep it for
BC, that's fine.


> The design constraint was that
> repo[None].status() had to maintain its fast path for checking the
> dirstate and disk.
>

I don't believe I'm changing that. Am I?
Sean Farley - Nov. 3, 2014, 8:15 p.m.
Martin von Zweigbergk writes:

> On Mon Nov 03 2014 at 10:34:50 AM Sean Farley <sean.michael.farley@gmail.com>
> wrote:
>
>>
>> Martin von Zweigbergk writes:
>>
>> > The workingctx method simply calls the super method. The only effect
>> > it has is that it uses a different default argument for the 'other'
>> > argument. The only in-tree caller is patch.diff, which always passes
>> > an argument to the method, so it should be safe to remove the
>> > overriding. Having the default argument depend on the type seems
>> > rather dangerous anyway.
>>
>> The only in-tree caller might be patch.diff but there were some
>> extensions that used it also.
>
>
> What I don't like is that ctx.status() might be comparing to either
> repo['.'] or repo[None] depending on what type ctx has. I don't know if we
> have code where the 'ctx' could be either type of context, but it just
> feels a little weird. I don't care that much, so if we want to keep it for
> BC, that's fine.

I completely agree. It feels very out-of-place that repo[None].status()
caches the status of the working directory against its parent but that
repo[None].status(REV) would calculate another status result all
together.

>
>> The design constraint was that
>> repo[None].status() had to maintain its fast path for checking the
>> dirstate and disk.
>>
>
> I don't believe I'm changing that. Am I?

No, I just put it there as reference for some of my choices. There's an
overarching goal I was trying to go towards: in-memory merge ... which
required removing the working ctx code from 'status'. This status
refactoring into context was a bit out of the way but still needed to be
done. Therefore, I wanted it to refactor but leave it extensible for
later.

If I were to tackle this today, I think the design (which I propose here
for others to review) I would go with would be something like:

- put the fast path of calculating the status into dirstate (or maybe
  its own object)

- figure out if the symlink stuff can also be put into dirstate

- simplify context status code and inheritance

This would allow repo[None].status() to return the dirstate.status (or
some kind of status object) which will be cached and then have
ctx.status(REV) return the on-the-fly calculated status.

This is just a plan off the top of my head, so expect bugs.
Sean Farley - Nov. 5, 2014, 12:31 a.m.
Sean Farley writes:

> Martin von Zweigbergk writes:
>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1414097000 25200
>> #      Thu Oct 23 13:43:20 2014 -0700
>> # Branch stable
>> # Node ID d68ec9fdce58cb01ec349e2731cf09247b10f436
>> # Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
>> context.status: remove overriding in workingctx
>
> Overall, I strongly think this series is moving in the wrong
> direction.

After some IRC discussion, I took the time to test some of my memctx WIP
with these changes and it's not as bad as I initially worried. This
patch series cleans up some (arguably over-designed) parts of status
that I'm ok with dropping.
Pierre-Yves David - Nov. 5, 2014, 12:32 a.m.
On 11/05/2014 12:31 AM, Sean Farley wrote:
>
> Sean Farley writes:
>
>> Martin von Zweigbergk writes:
>>
>>> # HG changeset patch
>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>> # Date 1414097000 25200
>>> #      Thu Oct 23 13:43:20 2014 -0700
>>> # Branch stable
>>> # Node ID d68ec9fdce58cb01ec349e2731cf09247b10f436
>>> # Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
>>> context.status: remove overriding in workingctx
>>
>> Overall, I strongly think this series is moving in the wrong
>> direction.
>
> After some IRC discussion, I took the time to test some of my memctx WIP
> with these changes and it's not as bad as I initially worried. This
> patch series cleans up some (arguably over-designed) parts of status
> that I'm ok with dropping.

Does this mean the whole series looks good to you as is? Or is a V2 
still to be expected?
Sean Farley - Nov. 5, 2014, 12:47 a.m.
Pierre-Yves David writes:

> On 11/05/2014 12:31 AM, Sean Farley wrote:
>>
>> Sean Farley writes:
>>
>>> Martin von Zweigbergk writes:
>>>
>>>> # HG changeset patch
>>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>>> # Date 1414097000 25200
>>>> #      Thu Oct 23 13:43:20 2014 -0700
>>>> # Branch stable
>>>> # Node ID d68ec9fdce58cb01ec349e2731cf09247b10f436
>>>> # Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
>>>> context.status: remove overriding in workingctx
>>>
>>> Overall, I strongly think this series is moving in the wrong
>>> direction.
>>
>> After some IRC discussion, I took the time to test some of my memctx WIP
>> with these changes and it's not as bad as I initially worried. This
>> patch series cleans up some (arguably over-designed) parts of status
>> that I'm ok with dropping.
>
> Does this mean the whole series looks good to you as is? Or is a V2 
> still to be expected?

The series does look good. I was just over-worrying with the design of
pre/post status. I can live with just overriding _buildstatus.
Martin von Zweigbergk - Nov. 5, 2014, 12:51 a.m.
Great. Thanks for reviewing, Sean.

On Tue Nov 04 2014 at 4:47:39 PM Sean Farley <sean.michael.farley@gmail.com>
wrote:

>
> Pierre-Yves David writes:
>
> > On 11/05/2014 12:31 AM, Sean Farley wrote:
> >>
> >> Sean Farley writes:
> >>
> >>> Martin von Zweigbergk writes:
> >>>
> >>>> # HG changeset patch
> >>>> # User Martin von Zweigbergk <martinvonz@google.com>
> >>>> # Date 1414097000 25200
> >>>> #      Thu Oct 23 13:43:20 2014 -0700
> >>>> # Branch stable
> >>>> # Node ID d68ec9fdce58cb01ec349e2731cf09247b10f436
> >>>> # Parent  cc1cbb0bba8ed1d95c8f1b8e27d4d2893e0dcca7
> >>>> context.status: remove overriding in workingctx
> >>>
> >>> Overall, I strongly think this series is moving in the wrong
> >>> direction.
> >>
> >> After some IRC discussion, I took the time to test some of my memctx WIP
> >> with these changes and it's not as bad as I initially worried. This
> >> patch series cleans up some (arguably over-designed) parts of status
> >> that I'm ok with dropping.
> >
> > Does this mean the whole series looks good to you as is? Or is a V2
> > still to be expected?
>
> The series does look good. I was just over-worrying with the design of
> pre/post status. I can live with just overriding _buildstatus.
>

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1496,14 +1496,6 @@ 
             match.bad = bad
         return match
 
-    def status(self, other='.', match=None, listignored=False,
-               listclean=False, listunknown=False, listsubrepos=False):
-        # yet to be determined: what to do if 'other' is a 'workingctx' or a
-        # 'memctx'?
-        return super(workingctx, self).status(other, match, listignored,
-                                              listclean, listunknown,
-                                              listsubrepos)
-
 class committablefilectx(basefilectx):
     """A committablefilectx provides common functionality for a file context
     that wants the ability to commit, e.g. workingfilectx or memfilectx."""