Patchwork [1,of,4] largefiles: don't pop largefile specific arguments to the add command

login
register
mail settings
Submitter Matt Harbison
Date Jan. 16, 2015, 3:39 a.m.
Message ID <6c893ee9c85def62e2f1.1421379593@Envy>
Download mbox | patch
Permalink /patch/7477/
State Accepted
Headers show

Comments

Matt Harbison - Jan. 16, 2015, 3:39 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1420068272 18000
#      Wed Dec 31 18:24:32 2014 -0500
# Node ID 6c893ee9c85def62e2f19de9f4a2b5288a12a5fd
# Parent  049a9e3a078d7c988cb12ed456aad6ec2779ea69
largefiles: don't pop largefile specific arguments to the add command

The arguments will need to stay present when making add work with subrepos.
Martin von Zweigbergk - Jan. 16, 2015, 4:36 a.m.
On Thu Jan 15 2015 at 7:40:19 PM Matt Harbison <mharbison72@gmail.com>
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1420068272 18000
> #      Wed Dec 31 18:24:32 2014 -0500
> # Node ID 6c893ee9c85def62e2f19de9f4a2b5288a12a5fd
> # Parent  049a9e3a078d7c988cb12ed456aad6ec2779ea69
> largefiles: don't pop largefile specific arguments to the add command
>
> The arguments will need to stay present when making add work with subrepos.
>
> diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
> --- a/hgext/largefiles/overrides.py
> +++ b/hgext/largefiles/overrides.py
> @@ -90,9 +90,9 @@
>              scmutil.matchandpats)
>
>  def addlargefiles(ui, repo, isaddremove, matcher, **opts):
> -    large = opts.pop('large', None)
> +    large = opts.get('large')
>      lfsize = lfutil.getminsize(
> -        ui, lfutil.islfilesrepo(repo), opts.pop('lfsize', None))
> +        ui, lfutil.islfilesrepo(repo), opts.get('lfsize'))
>
>      lfmatcher = None
>      if lfutil.islfilesrepo(repo):
> @@ -247,7 +247,7 @@
>  # matcher which matches only the normal files and runs the original
>  # version of add.
>  def overrideadd(orig, ui, repo, *pats, **opts):
> -    normal = opts.pop('normal')
> +    normal = opts.get('normal')
>

Nit: unlike pop(), get() doesn't raise KeyError. Use opts['normal'] unless
that's a wanted change.


>      if normal:
>          if opts.get('large'):
>              raise util.Abort(_('--normal cannot be used with --large'))
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Matt Harbison - Jan. 16, 2015, 5:10 a.m.
On Thu, 15 Jan 2015 23:36:56 -0500, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Thu Jan 15 2015 at 7:40:19 PM Matt Harbison <mharbison72@gmail.com>
> wrote:
>
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1420068272 18000
>> #      Wed Dec 31 18:24:32 2014 -0500
>> # Node ID 6c893ee9c85def62e2f19de9f4a2b5288a12a5fd
>> # Parent  049a9e3a078d7c988cb12ed456aad6ec2779ea69
>> largefiles: don't pop largefile specific arguments to the add command
>>
>> The arguments will need to stay present when making add work with  
>> subrepos.
>>
>> diff --git a/hgext/largefiles/overrides.py  
>> b/hgext/largefiles/overrides.py
>> --- a/hgext/largefiles/overrides.py
>> +++ b/hgext/largefiles/overrides.py
>> @@ -90,9 +90,9 @@
>>              scmutil.matchandpats)
>>
>>  def addlargefiles(ui, repo, isaddremove, matcher, **opts):
>> -    large = opts.pop('large', None)
>> +    large = opts.get('large')
>>      lfsize = lfutil.getminsize(
>> -        ui, lfutil.islfilesrepo(repo), opts.pop('lfsize', None))
>> +        ui, lfutil.islfilesrepo(repo), opts.get('lfsize'))
>>
>>      lfmatcher = None
>>      if lfutil.islfilesrepo(repo):
>> @@ -247,7 +247,7 @@
>>  # matcher which matches only the normal files and runs the original
>>  # version of add.
>>  def overrideadd(orig, ui, repo, *pats, **opts):
>> -    normal = opts.pop('normal')
>> +    normal = opts.get('normal')
>>
>
> Nit: unlike pop(), get() doesn't raise KeyError. Use opts['normal']  
> unless
> that's a wanted change.

Hmm, you're right.  I have no idea how this was working then, because  
--normal doesn't have to be specified on the command line.  This is the  
first line of the commands.add() override, so nothing command specific is  
adding it.  The lack of a KeyError implies it is in there, but how?   
Conversely, if --large is specified on the command line, it doesn't  
complain that both options can't be specified, so it doesn't actually seem  
like it is there.  debugpathcomplete also accesses its args with [], but  
that's the only other case I see.  Any ideas?

The short answer though is, I don't think we want to raise a KeyError when  
the user doesn't specify an optional argument.

--Matt
Martin von Zweigbergk - Jan. 16, 2015, 5:23 a.m.
At first I had assumed that it was protected from reaching that piece of
code without 'normal' present, but that doesn't seem to be the case. I
would have assumed, like you seem to have, that opts['normal'] would only
present if --normal was given on the command line, but take a look at
uisetup.py, which seems to register extra options to the command. I haven't
checked, but I would guess the third option there is the default (None in
this case), which some early piece of code (dispatch.py?) must read and
create opts based on that and the arguments given on the command line.
Mostly guessing here, though.

On Thu Jan 15 2015 at 9:11:11 PM Matt Harbison <mharbison72@gmail.com>
wrote:

> On Thu, 15 Jan 2015 23:36:56 -0500, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > On Thu Jan 15 2015 at 7:40:19 PM Matt Harbison <mharbison72@gmail.com>
> > wrote:
> >
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1420068272 18000
> >> #      Wed Dec 31 18:24:32 2014 -0500
> >> # Node ID 6c893ee9c85def62e2f19de9f4a2b5288a12a5fd
> >> # Parent  049a9e3a078d7c988cb12ed456aad6ec2779ea69
> >> largefiles: don't pop largefile specific arguments to the add command
> >>
> >> The arguments will need to stay present when making add work with
> >> subrepos.
> >>
> >> diff --git a/hgext/largefiles/overrides.py
> >> b/hgext/largefiles/overrides.py
> >> --- a/hgext/largefiles/overrides.py
> >> +++ b/hgext/largefiles/overrides.py
> >> @@ -90,9 +90,9 @@
> >>              scmutil.matchandpats)
> >>
> >>  def addlargefiles(ui, repo, isaddremove, matcher, **opts):
> >> -    large = opts.pop('large', None)
> >> +    large = opts.get('large')
> >>      lfsize = lfutil.getminsize(
> >> -        ui, lfutil.islfilesrepo(repo), opts.pop('lfsize', None))
> >> +        ui, lfutil.islfilesrepo(repo), opts.get('lfsize'))
> >>
> >>      lfmatcher = None
> >>      if lfutil.islfilesrepo(repo):
> >> @@ -247,7 +247,7 @@
> >>  # matcher which matches only the normal files and runs the original
> >>  # version of add.
> >>  def overrideadd(orig, ui, repo, *pats, **opts):
> >> -    normal = opts.pop('normal')
> >> +    normal = opts.get('normal')
> >>
> >
> > Nit: unlike pop(), get() doesn't raise KeyError. Use opts['normal']
> > unless
> > that's a wanted change.
>
> Hmm, you're right.  I have no idea how this was working then, because
> --normal doesn't have to be specified on the command line.  This is the
> first line of the commands.add() override, so nothing command specific is
> adding it.  The lack of a KeyError implies it is in there, but how?
> Conversely, if --large is specified on the command line, it doesn't
> complain that both options can't be specified, so it doesn't actually seem
> like it is there.  debugpathcomplete also accesses its args with [], but
> that's the only other case I see.  Any ideas?
>
> The short answer though is, I don't think we want to raise a KeyError when
> the user doesn't specify an optional argument.
>
> --Matt
>
Pierre-Yves David - Jan. 16, 2015, 9:14 a.m.
On 01/15/2015 09:23 PM, Martin von Zweigbergk wrote:
> At first I had assumed that it was protected from reaching that piece of
> code without 'normal' present, but that doesn't seem to be the case. I
> would have assumed, like you seem to have, that opts['normal'] would
> only present if --normal was given on the command line, but take a look
> at uisetup.py, which seems to register extra options to the command. I
> haven't checked, but I would guess the third option there is the default
> (None in this case), which some early piece of code (dispatch.py?) must
> read and create opts based on that and the arguments given on the
> command line. Mostly guessing here, though.

The command parsing code does magic around that an always populated the 
dict.

It is usually considered polite to write code as if this was not there 
to allow extension to call the command.

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -90,9 +90,9 @@ 
             scmutil.matchandpats)
 
 def addlargefiles(ui, repo, isaddremove, matcher, **opts):
-    large = opts.pop('large', None)
+    large = opts.get('large')
     lfsize = lfutil.getminsize(
-        ui, lfutil.islfilesrepo(repo), opts.pop('lfsize', None))
+        ui, lfutil.islfilesrepo(repo), opts.get('lfsize'))
 
     lfmatcher = None
     if lfutil.islfilesrepo(repo):
@@ -247,7 +247,7 @@ 
 # matcher which matches only the normal files and runs the original
 # version of add.
 def overrideadd(orig, ui, repo, *pats, **opts):
-    normal = opts.pop('normal')
+    normal = opts.get('normal')
     if normal:
         if opts.get('large'):
             raise util.Abort(_('--normal cannot be used with --large'))