[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