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

[Gregory Szorc]

On Fri, Jul 7, 2017 at 11:42 AM, Martin von Zweigbergk <
martinvonz at google.com> wrote:

> On Fri, Jul 7, 2017 at 11:39 AM, Sean Farley <sean at farley.io> wrote:
> >
> > Gregory Szorc <gregory.szorc at gmail.com> writes:
> >
> >> On Thu, Jul 6, 2017 at 10:16 PM, Martin von Zweigbergk <
> >> martinvonz at google.com> wrote:
> >>
> >>> On Thu, Jul 6, 2017 at 6:18 PM, Gregory Szorc <gregory.szorc at gmail.com
> >
> >>> wrote:
> >>> > # HG changeset patch
> >>> > # User Gregory Szorc <gregory.szorc at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170707/71bc1cfb/attachment.html>