[PATCH stable] remove: properly set return code when warnings are issued

[Mads Kiilerich]

  Brodie Rao wrote, On 08/27/2010 06:43 PM:
> # HG changeset patch
> # User Brodie Rao<brodie at bitheap.org>
> # Date 1282927125 14400
> # Branch stable
> # Node ID d69ffbe1e52c99401d618c72352c058065b57b06
> # Parent  85439f43749f373ee584bfa6d27658752287ce06
> remove: properly set return code when warnings are issued
>
> Assigning the return value in a nested function does not change the
> value in the outer scope. Changing ret to a list and mutating it fixes
> this.
>
> diff -r 85439f43749f -r d69ffbe1e52c mercurial/commands.py
> --- a/mercurial/commands.py	Thu Aug 26 23:11:03 2010 +0200
> +++ b/mercurial/commands.py	Fri Aug 27 12:38:45 2010 -0400
> @@ -2899,7 +2899,7 @@ def remove(ui, repo, *pats, **opts):
>       Returns 0 on success, 1 if any warnings encountered.
>       """
>
> -    ret = 0
> +    ret = [0]
>       after, force = opts.get('after'), opts.get('force')
>       if not pats and not after:
>           raise util.Abort(_('no files specified'))
> @@ -2911,13 +2911,13 @@ def remove(ui, repo, *pats, **opts):
>       for f in m.files():
>           if f not in repo.dirstate and not os.path.isdir(m.rel(f)):
>               ui.warn(_('not removing %s: file is untracked\n') % m.rel(f))
> -            ret = 1
> +            ret[0] = 1
>
>       def warn(files, reason):
>           for f in files:
>               ui.warn(_('not removing %s: file %s (use -f to force removal)\n')
>                       % (m.rel(f), reason))
> -            ret = 1
> +            ret[0] = 1
>
>       if force:
>           remove, forget = modified + deleted + clean, added
> @@ -2935,7 +2935,7 @@ def remove(ui, repo, *pats, **opts):
>
>       repo[None].forget(forget)
>       repo[None].remove(remove, unlink=not after)
> -    return ret
> +    return ret[0]
>

FWIW I'm no fan of this. One reason to use local functions is to 
abstract things and avoid side effects. Working around it like this with 
mutable objects isn't pretty. Returning a value is the right way to do 
it, but it will obviously be ridiculous in this case.

Moving ret = 1 out of the function would be a smaller and IMHO better 
change.

But ... this function tries to create a localized message by patching 
other localized messages together. Translators don't like that, and 
think it would be better if we didn't do that. So ... I think the best 
solution would be to inline/expand/remove the warn function completely.

There are not tests for return codes? That's a pitty; changed return 
codes will(?) influence scripts more than changed textual output.

/Mads