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

Greg Ward greg at gerg.ca
Fri Sep 7 22:02:44 CDT 2012


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

       Greg


More information about the Mercurial-devel mailing list