Patchwork [01,of,35] largefiles: declare commands using decorator

login
register
mail settings
Submitter Gregory Szorc
Date May 5, 2014, 5:51 a.m.
Message ID <a40ee6509498e00f2c40.1399269066@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/4592/
State Accepted
Commit 4c94229c51fbcd08a996307de5a1c21c08bbb57b
Headers show

Comments

Gregory Szorc - May 5, 2014, 5:51 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1399262946 25200
#      Sun May 04 21:09:06 2014 -0700
# Branch stable
# Node ID a40ee6509498e00f2c4003cc5b1712738ee784fa
# Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
largefiles: declare commands using decorator
Pierre-Yves David - May 5, 2014, 7:38 a.m.
> [PATCH 01 of 35] largefiles: declare commands using decorator

While I appreciate your contributions, the areas you clean up, the 
quality of your patches, this is still terribly wrong.

You cannot blindly past bomb such gigantic series. Please start talking 
with reviewer to have a more efficient way to do this. You are currently 
creating massive disruption to the review process, impacting all other 
contributors
Gregory Szorc - May 8, 2014, 5:59 a.m.
On 5/5/2014 12:38 AM, Pierre-Yves David wrote:
>> [PATCH 01 of 35] largefiles: declare commands using decorator
> 
> While I appreciate your contributions, the areas you clean up, the
> quality of your patches, this is still terribly wrong.
> 
> You cannot blindly past bomb such gigantic series. Please start talking
> with reviewer to have a more efficient way to do this. You are currently
> creating massive disruption to the review process, impacting all other
> contributors

OK, so I grok that the run-tests patch series at >100 patches is
excessive. But I'm kinda scratching my head as to what's wrong here.

While this series is 35 patches, it is mostly cosmetic and IMO should
involve lots of rubber stamping. I was very liberal about using multiple
patches because fold is easier than split. I didn't want to risk someone
rejecting the patches because they didn't like changing multiple files
in a single patch. I believe the patch guidelines are on my side here.
Furthermore, I'm pretty sure I've seen series with fewer parts but more
actual code to review. I wouldn't get too held up on patch count without
looking at actual diff size or the nature of the diff (e.g. new code vs
copy) (although this is hard to identify in vanilla diffs - tools like
Phabricator make it easier, but you know that).

It's also not clear to me what "talking with [a] reviewer" entails.
Please clarify.

It is fair for you to take issue with me not following procedure. I just
ask that the procedure be better documented so future contributors don't
fall into unmarked pits. AFAICT
http://mercurial.selenic.com/wiki/ContributingChanges says nothing on
the subject of series length and disrupting review processes.

I apologize. I'm just trying to contribute.

Patch

diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
--- a/hgext/largefiles/lfcommands.py
+++ b/hgext/largefiles/lfcommands.py
@@ -16,16 +16,28 @@  from mercurial import util, match as mat
 from mercurial.i18n import _
 from mercurial.lock import release
 
 import lfutil
 import basestore
 
 # -- Commands ----------------------------------------------------------
 
+cmdtable = {}
+command = cmdutil.command(cmdtable)
+
+commands.inferrepo += " lfconvert"
+
+@command('lfconvert',
+    [('s', 'size', '',
+      _('minimum size (MB) for files to be converted as largefiles'), 'SIZE'),
+    ('', 'to-normal', False,
+     _('convert from a largefiles repo to a normal repo')),
+    ],
+    _('hg lfconvert SOURCE DEST [FILE ...]'))
 def lfconvert(ui, src, dest, *pats, **opts):
     '''convert a normal repository to a largefiles repository
 
     Convert repository SOURCE to a new repository DEST, identical to
     SOURCE except that certain files will be converted as largefiles:
     specifically, any file that matches any PATTERN *or* whose size is
     above the minimum size threshold is converted as a largefile. The
     size used to determine whether or not to track a file as a
@@ -514,16 +526,20 @@  def updatelfiles(ui, repo, filelist=None
 
         lfdirstate.write()
         if printmessage and lfiles:
             ui.status(_('%d largefiles updated, %d removed\n') % (updated,
                 removed))
     finally:
         wlock.release()
 
+@command('lfpull',
+    [('r', 'rev', [], _('pull largefiles for these revisions'))
+    ] + commands.remoteopts,
+    _('-r REV... [-e CMD] [--remotecmd CMD] [SOURCE]'))
 def lfpull(ui, repo, source="default", **opts):
     """pull largefiles for the specified revisions from the specified source
 
     Pull largefiles that are referenced from local changesets but missing
     locally, pulling from a remote repository to the local cache.
 
     If SOURCE is omitted, the 'default' path will be used.
     See :hg:`help urls` for more information.
@@ -548,29 +564,8 @@  def lfpull(ui, repo, source="default", *
     revs = scmutil.revrange(repo, revs)
 
     numcached = 0
     for rev in revs:
         ui.note(_('pulling largefiles for revision %s\n') % rev)
         (cached, missing) = cachelfiles(ui, repo, rev)
         numcached += len(cached)
     ui.status(_("%d largefiles cached\n") % numcached)
-
-# -- hg commands declarations ------------------------------------------------
-
-cmdtable = {
-    'lfconvert': (lfconvert,
-                  [('s', 'size', '',
-                    _('minimum size (MB) for files to be converted '
-                      'as largefiles'),
-                    'SIZE'),
-                  ('', 'to-normal', False,
-                   _('convert from a largefiles repo to a normal repo')),
-                  ],
-                  _('hg lfconvert SOURCE DEST [FILE ...]')),
-    'lfpull': (lfpull,
-               [('r', 'rev', [], _('pull largefiles for these revisions'))
-                ] +  commands.remoteopts,
-               _('-r REV... [-e CMD] [--remotecmd CMD] [SOURCE]')
-               ),
-    }
-
-commands.inferrepo += " lfconvert"