Patchwork [11,of,11,sparse,V3] sparse: move post commit actions into core

login
register
mail settings
Submitter Gregory Szorc
Date July 7, 2017, 1:18 a.m.
Message ID <fe47ebda0b384aa1d300.1499390312@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/22083/
State Accepted
Headers show

Comments

Gregory Szorc - July 7, 2017, 1:18 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1499379254 25200
#      Thu Jul 06 15:14:14 2017 -0700
# Node ID fe47ebda0b384aa1d3002c6db3c4bace60472337
# Parent  1c1a25d842854cbb2a7516136f7bcaab0d9f4539
sparse: move post commit actions into core

Instead of wrapping committablectx.markcommitted(), we inline
the call into localrepository.commit(), which is the only place
we call markcommitted().

The APIs and distribution of duties between
localrepository.commit() and committablecontext.markcommitted()
are unclear to me. As is the proper order for certain operations
to be performed. There is room to improve this code.
via Mercurial-devel - July 7, 2017, 5:16 a.m.
On Thu, Jul 6, 2017 at 6:18 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1499379254 25200
> #      Thu Jul 06 15:14:14 2017 -0700
> # Node ID fe47ebda0b384aa1d3002c6db3c4bace60472337
> # Parent  1c1a25d842854cbb2a7516136f7bcaab0d9f4539
> sparse: move post commit actions into core
>
> Instead of wrapping committablectx.markcommitted(), we inline
> the call into localrepository.commit(), which is the only place
> we call markcommitted().
>
> The APIs and distribution of duties between
> localrepository.commit() and committablecontext.markcommitted()
> are unclear to me. As is the proper order for certain operations
> to be performed. There is room to improve this code.

4fb92f14a97a (commit: factor out post-commit cleanup into workingctx,
2013-02-08) says that markcommitted() was created to prepare for
in-memory merge. Given that, it seems more appropriate to leave the
call to sparse.aftercommit() at the end of
committablectx.markcommitted(), no? (I say "leave" because that's
where it was essentially called from in the wrapper.)

I'll queue 1-9 and let you think about this one a bit. Thanks!
Gregory Szorc - July 7, 2017, 6:33 p.m.
On Thu, Jul 6, 2017 at 10:16 PM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Thu, Jul 6, 2017 at 6:18 PM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1499379254 25200
> > #      Thu Jul 06 15:14:14 2017 -0700
> > # Node ID fe47ebda0b384aa1d3002c6db3c4bace60472337
> > # Parent  1c1a25d842854cbb2a7516136f7bcaab0d9f4539
> > sparse: move post commit actions into core
> >
> > Instead of wrapping committablectx.markcommitted(), we inline
> > the call into localrepository.commit(), which is the only place
> > we call markcommitted().
> >
> > The APIs and distribution of duties between
> > localrepository.commit() and committablecontext.markcommitted()
> > are unclear to me. As is the proper order for certain operations
> > to be performed. There is room to improve this code.
>
> 4fb92f14a97a (commit: factor out post-commit cleanup into workingctx,
> 2013-02-08) says that markcommitted() was created to prepare for
> in-memory merge. Given that, it seems more appropriate to leave the
> call to sparse.aftercommit() at the end of
> committablectx.markcommitted(), no? (I say "leave" because that's
> where it was essentially called from in the wrapper.)
>
> I'll queue 1-9 and let you think about this one a bit. Thanks!
>

Looking at this more, I'm not sure committablectx is the proper place for
it. This sparse code interacts with working directory state. If
committablectx (which is the base for memctx) is used outside of a working
directory, it doesn't make a lot of sense to be touching the working
directory from an in-memory commit. localrepository.commit() does, however,
only operate on working directories.

Perhaps this code belongs in workingctx.markcommitted()? But then again,
committablectx.markcommitted() touches dirstate, which is specific to the
working directory. So now I'm just confused as to what these various
contexts are for and what each should and should not do.

I suppose I'll just preserve the code as part of
commitablectx.markcommitted() in the next patchbomb. We can always clean it
up later if it is wrong.
Sean Farley - July 7, 2017, 6:39 p.m.
Gregory Szorc <gregory.szorc@gmail.com> writes:

> On Thu, Jul 6, 2017 at 10:16 PM, Martin von Zweigbergk <
> martinvonz@google.com> wrote:
>
>> On Thu, Jul 6, 2017 at 6:18 PM, Gregory Szorc <gregory.szorc@gmail.com>
>> wrote:
>> > # HG changeset patch
>> > # User Gregory Szorc <gregory.szorc@gmail.com>
>> > # Date 1499379254 25200
>> > #      Thu Jul 06 15:14:14 2017 -0700
>> > # Node ID fe47ebda0b384aa1d3002c6db3c4bace60472337
>> > # Parent  1c1a25d842854cbb2a7516136f7bcaab0d9f4539
>> > sparse: move post commit actions into core
>> >
>> > Instead of wrapping committablectx.markcommitted(), we inline
>> > the call into localrepository.commit(), which is the only place
>> > we call markcommitted().
>> >
>> > The APIs and distribution of duties between
>> > localrepository.commit() and committablecontext.markcommitted()
>> > are unclear to me. As is the proper order for certain operations
>> > to be performed. There is room to improve this code.
>>
>> 4fb92f14a97a (commit: factor out post-commit cleanup into workingctx,
>> 2013-02-08) says that markcommitted() was created to prepare for
>> in-memory merge. Given that, it seems more appropriate to leave the
>> call to sparse.aftercommit() at the end of
>> committablectx.markcommitted(), no? (I say "leave" because that's
>> where it was essentially called from in the wrapper.)
>>
>> I'll queue 1-9 and let you think about this one a bit. Thanks!
>>
>
> Looking at this more, I'm not sure committablectx is the proper place for
> it. This sparse code interacts with working directory state. If
> committablectx (which is the base for memctx) is used outside of a working
> directory, it doesn't make a lot of sense to be touching the working
> directory from an in-memory commit. localrepository.commit() does, however,
> only operate on working directories.
>
> Perhaps this code belongs in workingctx.markcommitted()? But then again,
> committablectx.markcommitted() touches dirstate, which is specific to the
> working directory. So now I'm just confused as to what these various
> contexts are for and what each should and should not do.

Yes, anything touching the dirstate should be in workingctx. It was a
misunderstanding on my part that committablectx ever got dirstate stuff.

> I suppose I'll just preserve the code as part of
> commitablectx.markcommitted() in the next patchbomb. We can always clean it
> up later if it is wrong.

I have a series (I thought I sent it but it seems not) that cleans up
all that mess. So, for clarity,

- workingctx: anything touching the disk
- memctx: anything in memory
- committablectx: anything that is shared
via Mercurial-devel - July 7, 2017, 6:42 p.m.
On Fri, Jul 7, 2017 at 11:39 AM, Sean Farley <sean@farley.io> wrote:
>
> Gregory Szorc <gregory.szorc@gmail.com> writes:
>
>> On Thu, Jul 6, 2017 at 10:16 PM, Martin von Zweigbergk <
>> martinvonz@google.com> wrote:
>>
>>> On Thu, Jul 6, 2017 at 6:18 PM, Gregory Szorc <gregory.szorc@gmail.com>
>>> wrote:
>>> > # HG changeset patch
>>> > # User Gregory Szorc <gregory.szorc@gmail.com>
>>> > # Date 1499379254 25200
>>> > #      Thu Jul 06 15:14:14 2017 -0700
>>> > # Node ID fe47ebda0b384aa1d3002c6db3c4bace60472337
>>> > # Parent  1c1a25d842854cbb2a7516136f7bcaab0d9f4539
>>> > sparse: move post commit actions into core
>>> >
>>> > Instead of wrapping committablectx.markcommitted(), we inline
>>> > the call into localrepository.commit(), which is the only place
>>> > we call markcommitted().
>>> >
>>> > The APIs and distribution of duties between
>>> > localrepository.commit() and committablecontext.markcommitted()
>>> > are unclear to me. As is the proper order for certain operations
>>> > to be performed. There is room to improve this code.
>>>
>>> 4fb92f14a97a (commit: factor out post-commit cleanup into workingctx,
>>> 2013-02-08) says that markcommitted() was created to prepare for
>>> in-memory merge. Given that, it seems more appropriate to leave the
>>> call to sparse.aftercommit() at the end of
>>> committablectx.markcommitted(), no? (I say "leave" because that's
>>> where it was essentially called from in the wrapper.)
>>>
>>> I'll queue 1-9 and let you think about this one a bit. Thanks!
>>>
>>
>> Looking at this more, I'm not sure committablectx is the proper place for
>> it. This sparse code interacts with working directory state. If
>> committablectx (which is the base for memctx) is used outside of a working
>> directory, it doesn't make a lot of sense to be touching the working
>> directory from an in-memory commit. localrepository.commit() does, however,
>> only operate on working directories.
>>
>> Perhaps this code belongs in workingctx.markcommitted()? But then again,
>> committablectx.markcommitted() touches dirstate, which is specific to the
>> working directory. So now I'm just confused as to what these various
>> contexts are for and what each should and should not do.
>
> Yes, anything touching the dirstate should be in workingctx. It was a
> misunderstanding on my part that committablectx ever got dirstate stuff.
>
>> I suppose I'll just preserve the code as part of
>> commitablectx.markcommitted() in the next patchbomb. We can always clean it
>> up later if it is wrong.
>
> I have a series (I thought I sent it but it seems not) that cleans up
> all that mess. So, for clarity,
>
> - workingctx: anything touching the disk
> - memctx: anything in memory
> - committablectx: anything that is shared

Makes sense. Greg, I'm still fine with leaving on committablectx for
now and moving to workingctx later.
Gregory Szorc - July 7, 2017, 6:43 p.m.
On Fri, Jul 7, 2017 at 11:42 AM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Fri, Jul 7, 2017 at 11:39 AM, Sean Farley <sean@farley.io> wrote:
> >
> > Gregory Szorc <gregory.szorc@gmail.com> writes:
> >
> >> On Thu, Jul 6, 2017 at 10:16 PM, Martin von Zweigbergk <
> >> martinvonz@google.com> wrote:
> >>
> >>> On Thu, Jul 6, 2017 at 6:18 PM, Gregory Szorc <gregory.szorc@gmail.com
> >
> >>> wrote:
> >>> > # HG changeset patch
> >>> > # User Gregory Szorc <gregory.szorc@gmail.com>
> >>> > # Date 1499379254 25200
> >>> > #      Thu Jul 06 15:14:14 2017 -0700
> >>> > # Node ID fe47ebda0b384aa1d3002c6db3c4bace60472337
> >>> > # Parent  1c1a25d842854cbb2a7516136f7bcaab0d9f4539
> >>> > sparse: move post commit actions into core
> >>> >
> >>> > Instead of wrapping committablectx.markcommitted(), we inline
> >>> > the call into localrepository.commit(), which is the only place
> >>> > we call markcommitted().
> >>> >
> >>> > The APIs and distribution of duties between
> >>> > localrepository.commit() and committablecontext.markcommitted()
> >>> > are unclear to me. As is the proper order for certain operations
> >>> > to be performed. There is room to improve this code.
> >>>
> >>> 4fb92f14a97a (commit: factor out post-commit cleanup into workingctx,
> >>> 2013-02-08) says that markcommitted() was created to prepare for
> >>> in-memory merge. Given that, it seems more appropriate to leave the
> >>> call to sparse.aftercommit() at the end of
> >>> committablectx.markcommitted(), no? (I say "leave" because that's
> >>> where it was essentially called from in the wrapper.)
> >>>
> >>> I'll queue 1-9 and let you think about this one a bit. Thanks!
> >>>
> >>
> >> Looking at this more, I'm not sure committablectx is the proper place
> for
> >> it. This sparse code interacts with working directory state. If
> >> committablectx (which is the base for memctx) is used outside of a
> working
> >> directory, it doesn't make a lot of sense to be touching the working
> >> directory from an in-memory commit. localrepository.commit() does,
> however,
> >> only operate on working directories.
> >>
> >> Perhaps this code belongs in workingctx.markcommitted()? But then again,
> >> committablectx.markcommitted() touches dirstate, which is specific to
> the
> >> working directory. So now I'm just confused as to what these various
> >> contexts are for and what each should and should not do.
> >
> > Yes, anything touching the dirstate should be in workingctx. It was a
> > misunderstanding on my part that committablectx ever got dirstate stuff.
> >
> >> I suppose I'll just preserve the code as part of
> >> commitablectx.markcommitted() in the next patchbomb. We can always
> clean it
> >> up later if it is wrong.
> >
> > I have a series (I thought I sent it but it seems not) that cleans up
> > all that mess. So, for clarity,
> >
> > - workingctx: anything touching the disk
> > - memctx: anything in memory
> > - committablectx: anything that is shared
>
> Makes sense. Greg, I'm still fine with leaving on committablectx for
> now and moving to workingctx later.
>

Nah - I'll just move it straight to workingctx so we do this right.

Thanks for the info, Sean! Please do send your series to clean this all up.
Sean Farley - July 7, 2017, 6:49 p.m.
Gregory Szorc <gregory.szorc@gmail.com> writes:

> On Fri, Jul 7, 2017 at 11:42 AM, Martin von Zweigbergk <
> martinvonz@google.com> wrote:
>
>> On Fri, Jul 7, 2017 at 11:39 AM, Sean Farley <sean@farley.io> wrote:
>> >
>> > Gregory Szorc <gregory.szorc@gmail.com> writes:
>> >
>> >> On Thu, Jul 6, 2017 at 10:16 PM, Martin von Zweigbergk <
>> >> martinvonz@google.com> wrote:
>> >>
>> >>> On Thu, Jul 6, 2017 at 6:18 PM, Gregory Szorc <gregory.szorc@gmail.com
>> >
>> >>> wrote:
>> >>> > # HG changeset patch
>> >>> > # User Gregory Szorc <gregory.szorc@gmail.com>
>> >>> > # Date 1499379254 25200
>> >>> > #      Thu Jul 06 15:14:14 2017 -0700
>> >>> > # Node ID fe47ebda0b384aa1d3002c6db3c4bace60472337
>> >>> > # Parent  1c1a25d842854cbb2a7516136f7bcaab0d9f4539
>> >>> > sparse: move post commit actions into core
>> >>> >
>> >>> > Instead of wrapping committablectx.markcommitted(), we inline
>> >>> > the call into localrepository.commit(), which is the only place
>> >>> > we call markcommitted().
>> >>> >
>> >>> > The APIs and distribution of duties between
>> >>> > localrepository.commit() and committablecontext.markcommitted()
>> >>> > are unclear to me. As is the proper order for certain operations
>> >>> > to be performed. There is room to improve this code.
>> >>>
>> >>> 4fb92f14a97a (commit: factor out post-commit cleanup into workingctx,
>> >>> 2013-02-08) says that markcommitted() was created to prepare for
>> >>> in-memory merge. Given that, it seems more appropriate to leave the
>> >>> call to sparse.aftercommit() at the end of
>> >>> committablectx.markcommitted(), no? (I say "leave" because that's
>> >>> where it was essentially called from in the wrapper.)
>> >>>
>> >>> I'll queue 1-9 and let you think about this one a bit. Thanks!
>> >>>
>> >>
>> >> Looking at this more, I'm not sure committablectx is the proper place
>> for
>> >> it. This sparse code interacts with working directory state. If
>> >> committablectx (which is the base for memctx) is used outside of a
>> working
>> >> directory, it doesn't make a lot of sense to be touching the working
>> >> directory from an in-memory commit. localrepository.commit() does,
>> however,
>> >> only operate on working directories.
>> >>
>> >> Perhaps this code belongs in workingctx.markcommitted()? But then again,
>> >> committablectx.markcommitted() touches dirstate, which is specific to
>> the
>> >> working directory. So now I'm just confused as to what these various
>> >> contexts are for and what each should and should not do.
>> >
>> > Yes, anything touching the dirstate should be in workingctx. It was a
>> > misunderstanding on my part that committablectx ever got dirstate stuff.
>> >
>> >> I suppose I'll just preserve the code as part of
>> >> commitablectx.markcommitted() in the next patchbomb. We can always
>> clean it
>> >> up later if it is wrong.
>> >
>> > I have a series (I thought I sent it but it seems not) that cleans up
>> > all that mess. So, for clarity,
>> >
>> > - workingctx: anything touching the disk
>> > - memctx: anything in memory
>> > - committablectx: anything that is shared
>>
>> Makes sense. Greg, I'm still fine with leaving on committablectx for
>> now and moving to workingctx later.
>>
>
> Nah - I'll just move it straight to workingctx so we do this right.
>
> Thanks for the info, Sean! Please do send your series to clean this all up.

No prob :-) Will do after this week so that we don't conflict.

Patch

diff --git a/hgext/sparse.py b/hgext/sparse.py
--- a/hgext/sparse.py
+++ b/hgext/sparse.py
@@ -79,7 +79,6 @@  from mercurial.node import nullid
 from mercurial import (
     cmdutil,
     commands,
-    context,
     dirstate,
     error,
     extensions,
@@ -100,9 +99,6 @@  testedwith = 'ships-with-hg-core'
 cmdtable = {}
 command = registrar.command(cmdtable)
 
-def uisetup(ui):
-    _setupcommit(ui)
-
 def extsetup(ui):
     sparse.enabled = True
 
@@ -134,27 +130,6 @@  def replacefilecache(cls, propname, repl
         raise AttributeError(_("type '%s' has no property '%s'") % (origcls,
                              propname))
 
-def _setupcommit(ui):
-    def _refreshoncommit(orig, self, node):
-        """Refresh the checkout when commits touch .hgsparse
-        """
-        orig(self, node)
-        repo = self._repo
-
-        ctx = repo[node]
-        profiles = sparse.patternsforrev(repo, ctx.rev())[2]
-
-        # profiles will only have data if sparse is enabled.
-        if set(profiles) & set(ctx.files()):
-            origstatus = repo.status()
-            origsparsematch = sparse.matcher(repo)
-            sparse.refreshwdir(repo, origstatus, origsparsematch, force=True)
-
-        sparse.prunetemporaryincludes(repo)
-
-    extensions.wrapfunction(context.committablectx, 'markcommitted',
-        _refreshoncommit)
-
 def _setuplog(ui):
     entry = commands.table['^log|history']
     entry[1].append(('', 'sparse', None,
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -53,6 +53,7 @@  from . import (
     revset,
     revsetlang,
     scmutil,
+    sparse,
     store,
     subrepo,
     tags as tagsmod,
@@ -1742,6 +1743,7 @@  class localrepository(object):
             # update bookmarks, dirstate and mergestate
             bookmarks.update(self, [p1, p2], ret)
             cctx.markcommitted(ret)
+            sparse.aftercommit(self, ret)
             ms.reset()
             tr.close()
 
diff --git a/mercurial/sparse.py b/mercurial/sparse.py
--- a/mercurial/sparse.py
+++ b/mercurial/sparse.py
@@ -478,3 +478,17 @@  def refreshwdir(repo, origstatus, origsp
         dirstate.normallookup(file)
 
     return added, dropped, lookup
+
+def aftercommit(repo, node):
+    """Perform actions after a working directory commit."""
+    ctx = repo[node]
+
+    profiles = patternsforrev(repo, ctx.rev())[2]
+
+    # profiles will only have data if sparse is enabled.
+    if set(profiles) & set(ctx.files()):
+        origstatus = repo.status()
+        origsparsematch = matcher(repo)
+        refreshwdir(repo, origstatus, origsparsematch, force=True)
+
+    prunetemporaryincludes(repo)