[PATCH 2 of 5 STABLE] largefiles: exit from remove with 1 on warnings

Matt Harbison matt_harbison at yahoo.com
Sat Sep 8 20:36:14 CDT 2012


Greg Ward wrote:
> On 06 September 2012, Matt Harbison said:
>>>>       def warn(files, reason):
>>>> +        ret = 0
>>>>           for f in files:
>>>>               ui.warn(_('not removing %s: %s (use forget to undo)\n')
>>>>                       % (m.rel(f), reason))
>>>> +            ret = 1
>>>> +        return ret
>
> [to which I replied]
>>> Hmmm. What about:
>>>
>>> --- a/hgext/largefiles/overrides.py
>>> +++ b/hgext/largefiles/overrides.py
>>> @@ -141,6 +141,7 @@
>>>           for f in files:
>>>               ui.warn(_('not removing %s: %s (use forget to undo)\n')
>>>                       % (m.rel(f), reason))
>>> +        return int(len(f)>   0)
>>>
>>>       if after:
>>>           remove, forget = deleted, []
>>>
>>> instead?
>
> [spurring this response]
>
>> Seems good to me.  I'm not a python programmer by trade
>
> I am, and it looks fine to me. Well, except I screwed up the
> whitespace -- *sigh*.
>
>> Maybe it would be better though to take this
>> out of the closure entirely?  I wasn't thrilled with 'result =
>> warn(..) or result', but with this, it can probably be a one liner
>> after all of the warnings are emitted.
>
> No, I think you've got the right pattern. First, warn() is called in
> several places; second, it might be called with an empty list. You
> only want the non-zero return value when the list is non-empty, and
> warn() is the obvious place to make that call.
>
> As for "result = warn(...) or result": it's slightly odd, but a
> similar pattern emerges when writing Mercurial hooks too.  It's a
> slight conflation of booleans and integers, but what the hell, if it's
> good enough for Kernighan and Ritchie, it's good enough for me. ;-)

Heh, I abuse int/bool all the time, so I didn't even think about that. 
What I meant was some people prefer to avoid conditionals or logical 
operands with side effects- in this case printing the warning.  (I was 
originally going to put 'result = result or warn(..)' so the 'or result' 
part doesn't fade into the parameter list of warn(), but then the 
warning wouldn't always be printed).

FWIW, I was saying something like this:

     if after:
         remove, forget = deleted, []
         warn(modified + added + clean, _('file still exists'))
+       result = int(len(clean)>0)
     else:
         remove, forget = deleted + clean, []
         warn(modified, _('file is modified'))
         warn(added, _('file has been marked for add'))

+   result = result or int(len(modified)>0 or len(added)>0)

Which I guess isn't as readable as I thought it would be.  So as long as 
there are no rules against logical operands with side effects, I'll make 
the adjustment to warn().

--Matt


More information about the Mercurial-devel mailing list