| Submitter | Sean Farley |
|---|---|
| Date | May 16, 2014, 1:09 a.m. |
| Message ID | <b96074a56e0d75b39476.1400202562@laptop.local> |
| Download | mbox | patch |
| Permalink | /patch/4780/ |
| State | Accepted |
| Headers | show |
Comments
On 05/15/2014 06:09 PM, Sean Farley wrote: > # HG changeset patch > # User Sean Farley <sean.michael.farley@gmail.com> > # Date 1397684088 18000 > # Wed Apr 16 16:34:48 2014 -0500 > # Node ID b96074a56e0d75b39476e37d91395505b1e01d2f > # Parent 8844e50911049ffb10c3693b6cfea7e2ea4440c7 > localrepo: reverse contexts in status > > This is a slight tweak to how localrepo.status calculates what files have > changed. By forcing a changectx to be first operator and anything not a > changectx to be the second operator, we can later exploit this to allow > refactoring the status operation as a method of a context object. > > Furthermore, this change will speed up 'hg diff --reverse' when used with the > working directory because the code will now hit a fast path without needing to > calculate an unneeded second manifest. > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py > --- a/mercurial/localrepo.py > +++ b/mercurial/localrepo.py > @@ -1521,10 +1521,28 @@ class localrepository(object): > return mf > > ctx1 = self[node1] > ctx2 = self[node2] > > + # This next code block is, admittedly, fragile logic that tests for > + # reversing the contexts and wouldn't need to exist if it weren't for > + # the fast (and common) code path of comparing the working directory > + # with its first parent. > + # > + # What we're aiming for here is the ability to call: > + # > + # workingctx.status(parentctx) > + # > + # If we always built the manifest for each context and compared those, > + # then we'd be done. But the special case of the above call means we > + # just copy the manifest of the parent. > + reversed = False > + if (not isinstance(ctx1, context.changectx) > + and isinstance(ctx2, context.changectx)): Not a super fan of isinstance. Do we have any other alternative? (no is a valid answer) > + reversed = True > + ctx1, ctx2 = ctx2, ctx1 > + > working = ctx2.rev() is None > parentworking = working and ctx1 == self['.'] > match = match or matchmod.always(self.root, self.getcwd()) > listignored, listclean, listunknown = ignored, clean, unknown > > @@ -1579,10 +1597,13 @@ class localrepository(object): > removed = mf1.keys() > > if working: > modified = ctx2._filtersuspectsymlink(modified) > > + if reversed: > + added, removed = removed, added > + I see that the deleted/unknown thing disapeared, I assume it did not made much sense in the first place?
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes: > On 05/15/2014 06:09 PM, Sean Farley wrote: >> # HG changeset patch >> # User Sean Farley <sean.michael.farley@gmail.com> >> # Date 1397684088 18000 >> # Wed Apr 16 16:34:48 2014 -0500 >> # Node ID b96074a56e0d75b39476e37d91395505b1e01d2f >> # Parent 8844e50911049ffb10c3693b6cfea7e2ea4440c7 >> localrepo: reverse contexts in status >> >> This is a slight tweak to how localrepo.status calculates what files have >> changed. By forcing a changectx to be first operator and anything not a >> changectx to be the second operator, we can later exploit this to allow >> refactoring the status operation as a method of a context object. >> >> Furthermore, this change will speed up 'hg diff --reverse' when used with the >> working directory because the code will now hit a fast path without needing to >> calculate an unneeded second manifest. >> >> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py >> --- a/mercurial/localrepo.py >> +++ b/mercurial/localrepo.py >> @@ -1521,10 +1521,28 @@ class localrepository(object): >> return mf >> >> ctx1 = self[node1] >> ctx2 = self[node2] >> >> + # This next code block is, admittedly, fragile logic that tests for >> + # reversing the contexts and wouldn't need to exist if it weren't for >> + # the fast (and common) code path of comparing the working directory >> + # with its first parent. >> + # >> + # What we're aiming for here is the ability to call: >> + # >> + # workingctx.status(parentctx) >> + # >> + # If we always built the manifest for each context and compared those, >> + # then we'd be done. But the special case of the above call means we >> + # just copy the manifest of the parent. >> + reversed = False >> + if (not isinstance(ctx1, context.changectx) >> + and isinstance(ctx2, context.changectx)): > > Not a super fan of isinstance. Do we have any other alternative? > (no is a valid answer) This is just a stop gap until we can figure out something better. Especially when memctx is thrown into the mix. 'isinstance' will keep things working even when memctx will soon inherit from committablectx. After that we could discuss how to approach it (just like we could discuss the pre/post status methods at that time). >> + reversed = True >> + ctx1, ctx2 = ctx2, ctx1 >> + >> working = ctx2.rev() is None >> parentworking = working and ctx1 == self['.'] >> match = match or matchmod.always(self.root, self.getcwd()) >> listignored, listclean, listunknown = ignored, clean, unknown >> >> @@ -1579,10 +1597,13 @@ class localrepository(object): >> removed = mf1.keys() >> >> if working: >> modified = ctx2._filtersuspectsymlink(modified) >> >> + if reversed: >> + added, removed = removed, added >> + > > I see that the deleted/unknown thing disapeared, I assume it did not > made much sense in the first place? I found a counterexample for switching deleted/unknown so I removed that. If we could define this all in terms of operators and rings, that'd be great :-)
On 05/16/2014 11:16 AM, Sean Farley wrote: > > Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes: > >> On 05/15/2014 06:09 PM, Sean Farley wrote: >>> # HG changeset patch >>> # User Sean Farley <sean.michael.farley@gmail.com> >>> # Date 1397684088 18000 >>> # Wed Apr 16 16:34:48 2014 -0500 >>> # Node ID b96074a56e0d75b39476e37d91395505b1e01d2f >>> # Parent 8844e50911049ffb10c3693b6cfea7e2ea4440c7 >>> localrepo: reverse contexts in status Explanation looks good. I pushed that to the clowncopter (Hey, patchbot, pushed == queued)
Patch
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -1521,10 +1521,28 @@ class localrepository(object): return mf ctx1 = self[node1] ctx2 = self[node2] + # This next code block is, admittedly, fragile logic that tests for + # reversing the contexts and wouldn't need to exist if it weren't for + # the fast (and common) code path of comparing the working directory + # with its first parent. + # + # What we're aiming for here is the ability to call: + # + # workingctx.status(parentctx) + # + # If we always built the manifest for each context and compared those, + # then we'd be done. But the special case of the above call means we + # just copy the manifest of the parent. + reversed = False + if (not isinstance(ctx1, context.changectx) + and isinstance(ctx2, context.changectx)): + reversed = True + ctx1, ctx2 = ctx2, ctx1 + working = ctx2.rev() is None parentworking = working and ctx1 == self['.'] match = match or matchmod.always(self.root, self.getcwd()) listignored, listclean, listunknown = ignored, clean, unknown @@ -1579,10 +1597,13 @@ class localrepository(object): removed = mf1.keys() if working: modified = ctx2._filtersuspectsymlink(modified) + if reversed: + added, removed = removed, added + r = modified, added, removed, deleted, unknown, ignored, clean if listsubrepos: for subpath, sub in scmutil.itersubrepos(ctx1, ctx2): if working: