Patchwork [5,of,5,perfarce] output and progress cleanups

login
register
mail settings
Submitter Dan Villiom Podlaski Christiansen
Date Nov. 29, 2014, 2:52 p.m.
Message ID <382131969f6483bd5f2a.1417272732@dookie.local>
Download mbox | patch
Permalink /patch/6886/
State Not Applicable
Headers show

Comments

Dan Villiom Podlaski Christiansen - Nov. 29, 2014, 2:52 p.m.
# HG changeset patch
# User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
# Date 1417271360 -3600
#      Sat Nov 29 15:29:20 2014 +0100
# Node ID 382131969f6483bd5f2aeb84391ceb147f37f637
# Parent  1f548aff09cd75a949bce95d88e4a6f80e449304
output and progress cleanups

- drop indeterminate progress for fstat - it's too spammy
- show progress output for pull
- write changes pulled to the console (loosely inspired by
  hgsubversion)
- suppress progress for sync for small changelists, and then display
  it for every change if need be
Pierre-Yves David - Dec. 2, 2014, 9:08 p.m.
On 11/29/2014 06:52 AM, Dan Villiom Podlaski Christiansen wrote:
> # HG changeset patch
> # User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
> # Date 1417271360 -3600
> #      Sat Nov 29 15:29:20 2014 +0100
> # Node ID 382131969f6483bd5f2aeb84391ceb147f37f637
> # Parent  1f548aff09cd75a949bce95d88e4a6f80e449304
> output and progress cleanups

This series looks good (a bit unsure about the last changesets that do 
multiple things and his therefor hard to read).

But I've not idea about what I'm supposed to do with such patches.
Dan Villiom Podlaski Christiansen - Dec. 2, 2014, 9:37 p.m.
On 02/12/2014, at 22.08, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> On 11/29/2014 06:52 AM, Dan Villiom Podlaski Christiansen wrote:
>> # HG changeset patch
>> # User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
>> # Date 1417271360 -3600
>> #      Sat Nov 29 15:29:20 2014 +0100
>> # Node ID 382131969f6483bd5f2aeb84391ceb147f37f637
>> # Parent  1f548aff09cd75a949bce95d88e4a6f80e449304
>> output and progress cleanups
> 
> This series looks good (a bit unsure about the last changesets that do multiple things and his therefor hard to read).
> 
> But I've not idea about what I'm supposed to do with such patches.

My understanding was that the Mercurial list was an appropriate location for discussing extensions that have no other list :) Frank replied to the introductory email — but that was suppressed by automatic filters, and so didn't get to this list.

(From a bit of searching, I see Matt decided that introductory messages are so obnoxious that they should be entirely suppressed. I found no mention of this on the wiki, and the rejection was extremely terse and unhelpful. I'd suggest either replacing the notice with something less rude, or — even better — dropping it all together and go back to politely reminding people that they shouldn't send such messages.)

--

Dan Villiom Podlaski Christiansen
danchr@gmail.com
Matt Mackall - Dec. 2, 2014, 11:51 p.m.
On Tue, 2014-12-02 at 22:37 +0100, Dan Villiom Podlaski Christiansen
wrote:
> On 02/12/2014, at 22.08, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > On 11/29/2014 06:52 AM, Dan Villiom Podlaski Christiansen wrote:
> >> # HG changeset patch
> >> # User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
> >> # Date 1417271360 -3600
> >> #      Sat Nov 29 15:29:20 2014 +0100
> >> # Node ID 382131969f6483bd5f2aeb84391ceb147f37f637
> >> # Parent  1f548aff09cd75a949bce95d88e4a6f80e449304
> >> output and progress cleanups
> > 
> > This series looks good (a bit unsure about the last changesets that do multiple things and his therefor hard to read).
> > 
> > But I've not idea about what I'm supposed to do with such patches.
> 
> My understanding was that the Mercurial list was an appropriate location for discussing extensions that have no other list :) Frank replied to the introductory email — but that was suppressed by automatic filters, and so didn't get to this list.
> 
> (From a bit of searching, I see Matt decided that introductory
> messages are so obnoxious that they should be entirely suppressed. I
> found no mention of this on the wiki,

I've been discouraging them since at least early 2009. You haven't been
gone that long. But here's what it says on the wiki:

        http://mercurial.selenic.com/wiki/ContributingChanges
        
        5. all relevant info is in the commit message for posterity (not
        a "0 of N" message) 
        
        ...
        
        Please do not send a 0 of X summary message. Those will be
        deleted in the inbox of code reviewers, and never be read. It's
        totally fine to discuss the history and purpose of a patch in a
        patch description. Future archaeologists will thank you. 
        
>  and the rejection was extremely terse and unhelpful. I'd suggest
> either replacing the notice with something less rude, or — even better
> — dropping it all together and go back to politely reminding people
> that they shouldn't send such messages.)

Tried that for years. Didn't work.
Augie Fackler - Dec. 4, 2014, 4:15 p.m.
On Tue, Dec 02, 2014 at 05:51:10PM -0600, Matt Mackall wrote:
> On Tue, 2014-12-02 at 22:37 +0100, Dan Villiom Podlaski Christiansen
> wrote:
> > On 02/12/2014, at 22.08, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > > On 11/29/2014 06:52 AM, Dan Villiom Podlaski Christiansen wrote:
> > >> # HG changeset patch
> > >> # User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
> > >> # Date 1417271360 -3600
> > >> #      Sat Nov 29 15:29:20 2014 +0100
> > >> # Node ID 382131969f6483bd5f2aeb84391ceb147f37f637
> > >> # Parent  1f548aff09cd75a949bce95d88e4a6f80e449304
> > >> output and progress cleanups
> > >
> > > This series looks good (a bit unsure about the last changesets that do multiple things and his therefor hard to read).
> > >
> > > But I've not idea about what I'm supposed to do with such patches.
> >
> > My understanding was that the Mercurial list was an appropriate location for discussing extensions that have no other list :) Frank replied to the introductory email — but that was suppressed by automatic filters, and so didn't get to this list.
> >
> > (From a bit of searching, I see Matt decided that introductory
> > messages are so obnoxious that they should be entirely suppressed. I
> > found no mention of this on the wiki,
>
> I've been discouraging them since at least early 2009. You haven't been
> gone that long. But here's what it says on the wiki:
>
>         http://mercurial.selenic.com/wiki/ContributingChanges
>
>         5. all relevant info is in the commit message for posterity (not
>         a "0 of N" message)
>
>         ...
>
>         Please do not send a 0 of X summary message. Those will be
>         deleted in the inbox of code reviewers, and never be read. It's
>         totally fine to discuss the history and purpose of a patch in a
>         patch description. Future archaeologists will thank you.
>
> >  and the rejection was extremely terse and unhelpful. I'd suggest
> > either replacing the notice with something less rude, or — even better
> > — dropping it all together and go back to politely reminding people
> > that they shouldn't send such messages.)
>
> Tried that for years. Didn't work.

What does the rejection say? I'd rather it wasn't rude to newbies that
just didn't read the guidelines.

>
> --
> Mathematics is the supreme nostalgia of our time.
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Dec. 5, 2014, 1:21 a.m.
On Thu, 2014-12-04 at 11:15 -0500, Augie Fackler wrote:
> On Tue, Dec 02, 2014 at 05:51:10PM -0600, Matt Mackall wrote:
> > On Tue, 2014-12-02 at 22:37 +0100, Dan Villiom Podlaski Christiansen
> > wrote:
> > > On 02/12/2014, at 22.08, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > > > On 11/29/2014 06:52 AM, Dan Villiom Podlaski Christiansen wrote:
> > > >> # HG changeset patch
> > > >> # User Dan Villiom Podlaski Christiansen  <danchr@gmail.com>
> > > >> # Date 1417271360 -3600
> > > >> #      Sat Nov 29 15:29:20 2014 +0100
> > > >> # Node ID 382131969f6483bd5f2aeb84391ceb147f37f637
> > > >> # Parent  1f548aff09cd75a949bce95d88e4a6f80e449304
> > > >> output and progress cleanups
> > > >
> > > > This series looks good (a bit unsure about the last changesets that do multiple things and his therefor hard to read).
> > > >
> > > > But I've not idea about what I'm supposed to do with such patches.
> > >
> > > My understanding was that the Mercurial list was an appropriate location for discussing extensions that have no other list :) Frank replied to the introductory email — but that was suppressed by automatic filters, and so didn't get to this list.
> > >
> > > (From a bit of searching, I see Matt decided that introductory
> > > messages are so obnoxious that they should be entirely suppressed. I
> > > found no mention of this on the wiki,
> >
> > I've been discouraging them since at least early 2009. You haven't been
> > gone that long. But here's what it says on the wiki:
> >
> >         http://mercurial.selenic.com/wiki/ContributingChanges
> >
> >         5. all relevant info is in the commit message for posterity (not
> >         a "0 of N" message)
> >
> >         ...
> >
> >         Please do not send a 0 of X summary message. Those will be
> >         deleted in the inbox of code reviewers, and never be read. It's
> >         totally fine to discuss the history and purpose of a patch in a
> >         patch description. Future archaeologists will thank you.
> >
> > >  and the rejection was extremely terse and unhelpful. I'd suggest
> > > either replacing the notice with something less rude, or — even better
> > > — dropping it all together and go back to politely reminding people
> > > that they shouldn't send such messages.)
> >
> > Tried that for years. Didn't work.
> 
> What does the rejection say? I'd rather it wasn't rude to newbies that
> just didn't read the guidelines.

Just tested, it says:

 Message rejected by filter rule match

It's the standard, non-configurable Mailman message.

I'm halfway through writing a bot to remind people very very nicely when
they should throttle their patch sending, so it'll be easy enough to add
this reminder there as well and remove the filter.

Patch

diff --git a/perfarce.py b/perfarce.py
--- a/perfarce.py
+++ b/perfarce.py
@@ -809,14 +809,7 @@  class p4client(object):
         else:
             p4cmd = 'fstat -e %d %s' % (change, util.shellquote('%s...' % self.partial))
 
-        for d in self.run(p4cmd):
-            if len(result) % 250 == 0:
-                if hasattr(self.ui, 'progress'):
-                    self.ui.progress('p4 fstat', len(result), unit='entries')
-                else:
-                    self.ui.note('%d files\r' % len(result))
-                    self.ui.flush()
-
+        for i, d in enumerate(self.run(p4cmd)):
             if 'desc' in d or d['clientFile'].startswith('.hg'):
                 continue
             else:
@@ -829,8 +822,6 @@  class p4client(object):
                 ac = d['headAction']
                 result.append((df, int(rv), tp, self.actions[ac], lf))
 
-        if hasattr(self.ui, 'progress'):
-            self.ui.progress('p4 fstat', None)
         self.ui.note(_('%d files \n') % len(result))
 
         return result
@@ -853,9 +844,8 @@  class p4client(object):
         n = 0
         for d in self.run(cmd, files=[("%s@%d" % (os.path.join(self.partial, f), change)) for f in files], abort=False):
             n += 1
-            if n % 250 == 0:
-                if hasattr(self.ui, 'progress'):
-                    self.ui.progress('p4 sync', n, unit='files')
+            if len(files) > 10 and hasattr(self.ui, 'progress'):
+                self.ui.progress('p4 sync', n, unit='files', total=len(files))
             code = d.get('code')
             if code == 'error':
                 data = d['data'].strip()
@@ -864,7 +854,7 @@  class p4client(object):
                 else:
                     raise util.Abort('p4: %s' % data)
 
-        if hasattr(self.ui, 'progress'):
+        if len(files) > 10 and hasattr(self.ui, 'progress'):
             self.ui.progress('p4 sync', None)
 
         if files and n < len(files):
@@ -990,7 +980,7 @@  class p4client(object):
 
 
     @staticmethod
-    def pullcommon(original, ui, repo, source, **opts):
+    def pullcommon(original, actiontext, ui, repo, source, **opts):
         'Shared code for pull and incoming'
 
         if opts.get('mq',None):
@@ -1005,6 +995,10 @@  class p4client(object):
         except p4badclient,e:
             raise util.Abort(str(e))
 
+        ui.status(_('%s %s\n') % (actiontext, util.hidepassword(source)),
+                  label='pull.source')
+        ui.status(_('searching for changes\n'))
+
         # if present, --rev will be the last Perforce changeset number to get
         stoprev = opts.get('rev')
         stoprev = stoprev and max(int(r) for r in stoprev) or 0
@@ -1044,6 +1038,11 @@  class p4client(object):
                 changes.append(c)
         changes.sort()
 
+        if changes:
+            ui.status(_("%s changes found\n" % len(changes)))
+        else:
+            ui.status(_("no changes found\n"))
+
         return False, (client, p4rev, p4id, startrev, changes)
 
 
@@ -1193,7 +1192,8 @@  def incoming(original, ui, repo, source=
     Returns 0 if there are incoming changes, 1 otherwise.
     '''
 
-    done, r = p4client.pullcommon(original, ui, repo, source, **opts)
+    done, r = p4client.pullcommon(original, 'comparing with', ui, repo, source,
+                                  **opts)
     if done:
         return r
 
@@ -1234,7 +1234,8 @@  def incoming(original, ui, repo, source=
 def pull(original, ui, repo, source=None, **opts):
     '''Wrap the pull command to look for p4 paths, import changelists'''
 
-    done, r = p4client.pullcommon(original, ui, repo, source, **opts)
+    done, r = p4client.pullcommon(original, 'pulling from', ui, repo, source,
+                                  **opts)
     if done:
         return r
 
@@ -1242,21 +1243,6 @@  def pull(original, ui, repo, source=None
     entries = {}
     c = 0
 
-    def memfilectx(context, repo, path, data, islink, isexec):
-        'wrapper to handle 3.1 vs older differences'
-        try:
-            return context.memfilectx(repo=repo, path=path, data=data, islink=islink, isexec=isexec)
-        except TypeError:
-            return context.memfilectx(path=path, data=data, islink=islink, isexec=isexec, copied=None)
-
-    def getfilectx(repo, memctx, fn):
-        'callback to read file data'
-        if fn.startswith('.hg'):
-            return repo[parent].filectx(fn)
-
-        mode, contents = client.getfile(entries[fn])
-        return memfilectx(context, repo, fn, contents, 'l' in mode, 'x' in mode)
-
     # for clone we support a --startrev option to fold initial changelists
     if startrev:
         if len(changes) < 2:
@@ -1276,11 +1262,27 @@  def pull(original, ui, repo, source=None
     trim = ui.configbool('perfarce', 'pull_trim_log', False)
 
     try:
-        for c in changes:
+        for i, c in enumerate(changes, start=1):
             ui.note(_('change %s\n') % c)
+
+            if hasattr(ui, 'progress'):
+                ui.progress('pull', i, item=str(c), unit='changelists',
+                            total=len(changes))
+
             cl = client.describe(c)
             files = client.fstat(c, all=bool(startrev))
 
+            msg = '[#%s] %s: %s' % (cl.change, util.shortuser(cl.user),
+                                    cl.desc.splitlines()[0])
+            termwidth = getattr(ui, 'termwidth', None) or util.termwidth
+            w = termwidth()
+            ui.status(util.ellipsis(msg, w) + '\n')
+
+            if hasattr(ui, 'progress'):
+                ui.progress('pull', i,
+                            item=cl.change, unit='changelists',
+                            total=len(changes))
+
             if client.keep:
                 if startrev:
                     client.sync(c, all=True, force=True)
@@ -1289,6 +1291,11 @@  def pull(original, ui, repo, source=None
                     client.sync(c, force=True, files=[f[0] for f in files if f[3]=="R"]+
                                                      [f[0] for f in files if f[3]!="R"])
 
+            if hasattr(ui, 'progress'):
+                ui.progress('pull', i,
+                            item=cl.change, unit='changelists',
+                            total=len(changes))
+
             nodes, match = client.parsenodes(cl.desc)
             if nodes:
                 parent = nodes[-1]
@@ -1327,6 +1334,28 @@  def pull(original, ui, repo, source=None
             else:
                 entries.update((f[4], f) for f in files)
 
+            def memfilectx(context, repo, path, data, islink, isexec):
+                'wrapper to handle 3.1 vs older differences'
+                try:
+                    return context.memfilectx(repo=repo, path=path, data=data, islink=islink, isexec=isexec)
+                except TypeError:
+                    return context.memfilectx(path=path, data=data, islink=islink, isexec=isexec, copied=None)
+
+            seen = [0]
+            def getfilectx(repo, memctx, fn):
+                'callback to read file data'
+                if fn.startswith('.hg'):
+                    return repo[parent].filectx(fn)
+
+                seen[0] += 1
+
+                if len(files) > 1 and hasattr(ui, 'progress'):
+                    ui.progress('p4 print', seen[0], item=fn,
+                                unit='files', total=len(files))
+
+                mode, contents = client.getfile(entries[fn])
+                return memfilectx(context, repo, fn, contents, 'l' in mode, 'x' in mode)
+
             ctx = context.memctx(repo, (p4rev, parent), cl.desc,
                                  entries.keys() + hgfiles,
                                  getfilectx, cl.user, cl.date, extra)
@@ -1334,6 +1363,9 @@  def pull(original, ui, repo, source=None
             p4rev = repo.commitctx(ctx)
             ctx = repo[p4rev]
 
+            if len(files) > 1 and hasattr(ui, 'progress'):
+                ui.progress('sync', None, unit='files', total=len(files))
+
             for l in client.labels(c):
                 tags[l] = (c, ctx.hex())
 
@@ -1342,6 +1374,9 @@  def pull(original, ui, repo, source=None
             ui.note(_('added changeset %d:%s\n') % (ctx.rev(), ctx))
 
     finally:
+        if hasattr(ui, 'progress'):
+            ui.progress('pull', None)
+
         if tags:
             p4rev, p4id = client.find()
             ctx = repo[p4rev]