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

login
register
mail settings
Submitter Boris Feld
Date July 16, 2017, 9:21 a.m.
Message ID <8df908eb63b41ebef19e.1500196902@FB>
Download mbox | patch
Permalink /patch/22434/
State Accepted
Headers show

Comments

Boris Feld - July 16, 2017, 9:21 a.m.
# HG changeset patch
# User Boris Feld <boris.feld@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.

The previous manual call during unbundling of the bundle2 "obsmarkers" part is
no longer necessary and has been dropped.
Augie Fackler - July 17, 2017, 2:35 p.m.
On Sun, Jul 16, 2017 at 11:21:42AM +0200, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@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

Queued the series, thanks.

>
> 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.

Indeed, let's see what we can figure out to improve this.
via Mercurial-devel - July 17, 2017, 5:35 p.m.
On Sun, Jul 16, 2017 at 2:21 AM, Boris Feld <boris.feld@octobus.net> wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@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.

>
> 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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - July 17, 2017, 5:42 p.m.
On Mon, Jul 17, 2017 at 10:35 AM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Sun, Jul 16, 2017 at 2:21 AM, Boris Feld <boris.feld@octobus.net> wrote:
>> # HG changeset patch
>> # User Boris Feld <boris.feld@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.

>
>>
>> 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@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Boris Feld - July 18, 2017, 9:38 a.m.
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@google.com> wrote:
> > On Sun, Jul 16, 2017 at 2:21 AM, Boris Feld <boris.feld@octobus.net
> > > wrote:
> > > # HG changeset patch
> > > # User Boris Feld <boris.feld@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@mercurial-scm.org
> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

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