[PATCH 2 of 3] transaction-summary: display the summary for all transactions

[Boris Feld]

On Mon, 2017-07-17 at 10:42 -0700, Martin von Zweigbergk wrote:
> On Mon, Jul 17, 2017 at 10:35 AM, Martin von Zweigbergk
> <martinvonz at google.com> wrote:
> > On Sun, Jul 16, 2017 at 2:21 AM, Boris Feld <boris.feld at octobus.net
> > > wrote:
> > > # HG changeset patch
> > > # User Boris Feld <boris.feld at octobus.net>
> > > # Date 1500164406 -7200
> > > #      Sun Jul 16 02:20:06 2017 +0200
> > > # Node ID 8df908eb63b41ebef19e71f4f3f0085be4e6f8b3
> > > # Parent  ed5dfde9455a023b9b26152ee55ade0085b5516a
> > > # EXP-Topic tr.report
> > > transaction-summary: display the summary for all transactions
> > > 
> > > Now that we records "all" changes happening in a transaction (in
> > > tr.changes)
> > > we will be able to provide better report on various changes
> > > (phases turned
> > > public, changeset obsoleted, branch merged or created, etc..)
> > > 
> > > This is far too late in the cycle to play with this, but having
> > > this existing
> > > method called more widely will help extensions to play around
> > > with various
> > > options during the 4.4 cycle.
> > > 
> > > Instead of calling registersummarycallback only for transactions
> > > we want, we
> > > always call it and use the transaction name to decide when to
> > > report (eg: we
> > > do not want `hg amend` to report new obsoleted changesets).
> > > Filtering on
> > > transaction name does not seems great, but seems good enough for
> > > the moment.
> > > We can change the API during the next cycle.
> > 
> > changegroup.apply() uses tr.hookargs['source'] for this. Should we
> > add
> > a "source" argument to localrepo.transaction() and maybe store it
> > on
> > the transaction object itself, so we don't have to do this weird
> > prefix matching here and reaching into hookargs in cg.apply()?
> > OTOH, I
> > think behaving differently depending on who your caller is is a
> > little
> > unusual too, so maybe we should find a better way of passing down
> > the
> > information of what the caller wants (not who the caller is). But I
> > do
> > see the simplicity of the current approach, so I'm not sure.
> 
> Concretely, I suppose the current approach means that any extensions
> that start a transaction and then run e.g. unbundle will no longer
> get
> the summary report. They can of course easily add themselves to
> _reportobsoletedsource if they do want that report, so it's not a big
> deal.
> 

Yes extensions will need to add themselves to _reportobsoletedsource,
we should document it in the release note / documentation. Do you want
me to send a series for that?

The proposed way of filtering is working for simple cases. I think that
with feedback the feedback we will gather from extensions authors
during the 4.4 cycle, will help us design a cleaner API for it. What do
you think?

> > 
> > > 
> > > The previous manual call during unbundling of the bundle2
> > > "obsmarkers" part is
> > > no longer necessary and has been dropped.
> > > 
> > > diff -r ed5dfde9455a -r 8df908eb63b4 mercurial/bundle2.py
> > > --- a/mercurial/bundle2.py      Sun Jul 16 02:38:14 2017 +0200
> > > +++ b/mercurial/bundle2.py      Sun Jul 16 02:20:06 2017 +0200
> > > @@ -161,7 +161,6 @@
> > >      phases,
> > >      pushkey,
> > >      pycompat,
> > > -    scmutil,
> > >      tags,
> > >      url,
> > >      util,
> > > @@ -1814,7 +1813,6 @@
> > >      if new:
> > >          op.repo.ui.status(_('%i new obsolescence markers\n') %
> > > new)
> > >      op.records.add('obsmarkers', {'new': new})
> > > -    scmutil.registersummarycallback(op.repo, tr)
> > >      if op.reply is not None:
> > >          rpart = op.reply.newpart('reply:obsmarkers')
> > >          rpart.addparam('in-reply-to', str(inpart.id),
> > > mandatory=False)
> > > diff -r ed5dfde9455a -r 8df908eb63b4 mercurial/localrepo.py
> > > --- a/mercurial/localrepo.py    Sun Jul 16 02:38:14 2017 +0200
> > > +++ b/mercurial/localrepo.py    Sun Jul 16 02:20:06 2017 +0200
> > > @@ -1092,6 +1092,7 @@
> > >                  raise error.ProgrammingError('transaction
> > > requires locking')
> > >          tr = self.currenttransaction()
> > >          if tr is not None:
> > > +            scmutil.registersummarycallback(self, tr, desc)
> > >              return tr.nest()
> > > 
> > >          # abort here if the journal already exists
> > > @@ -1247,6 +1248,7 @@
> > >          # to stored data if transaction has no error.
> > >          tr.addpostclose('refresh-filecachestats',
> > > self._refreshfilecachestats)
> > >          self._transref = weakref.ref(tr)
> > > +        scmutil.registersummarycallback(self, tr, desc)
> > >          return tr
> > > 
> > >      def _journalfiles(self):
> > > diff -r ed5dfde9455a -r 8df908eb63b4 mercurial/scmutil.py
> > > --- a/mercurial/scmutil.py      Sun Jul 16 02:38:14 2017 +0200
> > > +++ b/mercurial/scmutil.py      Sun Jul 16 02:20:06 2017 +0200
> > > @@ -1080,14 +1080,25 @@
> > >          with self.vfs(self.path, mode='wb', atomictemp=True) as
> > > fp:
> > >              fp.write(''.join(lines))
> > > 
> > > -def registersummarycallback(repo, otr):
> > > +_reportobsoletedsource = [
> > > +    'pull',
> > > +    'push',
> > > +    'serve',
> > > +    'unbundle',
> > > +]
> > > +
> > > +def registersummarycallback(repo, otr, txnname=''):
> > >      """register a callback to issue a summary after the
> > > transaction is closed
> > >      """
> > > -    reporef = weakref.ref(repo)
> > > -    def reportsummary(tr):
> > > -        """the actual callback reporting the summary"""
> > > -        repo = reporef()
> > > -        obsoleted = obsutil.getobsoleted(repo, tr)
> > > -        if obsoleted:
> > > -            repo.ui.status(_('obsoleted %i changesets\n') %
> > > len(obsoleted))
> > > -    otr.addpostclose('00-txnreport', reportsummary)
> > > +    for source in _reportobsoletedsource:
> > > +        if txnname.startswith(source):
> > > +            reporef = weakref.ref(repo)
> > > +            def reportsummary(tr):
> > > +                """the actual callback reporting the summary"""
> > > +                repo = reporef()
> > > +                obsoleted = obsutil.getobsoleted(repo, tr)
> > > +                if obsoleted:
> > > +                    repo.ui.status(_('obsoleted %i
> > > changesets\n')
> > > +                                   % len(obsoleted))
> > > +            otr.addpostclose('00-txnreport', reportsummary)
> > > +            break
> > > _______________________________________________
> > > Mercurial-devel mailing list
> > > Mercurial-devel at mercurial-scm.org
> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel