[PATCH] svn subrepo: abort only on nonzero process exit code, no longer assuming warnings are errors (issue2833)

[Regis Desgroppes]

On 05/26/2011 06:51 PM, ext Augie Fackler wrote:
> On May 26, 2011, at 8:21 AM, Desgroppes Regis (Nokia-MS/Berlin) wrote:
>
>> svn subrepo: abort only on nonzero process exit code, no longer assuming
>> warnings are errors (issue2833)
>>
>> In subrepo.py, when svn writes something to its error output stream, the
>> command is considered as failed.
>> This is a wrong assumption, as svn may write simple warnings as well (as
>> seen when accessing KWallet, a keyring for KDE).
>> The process exit code should be considered instead, as proposed in the
>> below patch.
>> Could you please consider applying it? Feel free to rewrite as well.
>> Thanks,
>> Régis
>>
>> # HG changeset patch
>> # User desgropp<regis.desgroppes at nokia.com>
>> # Date 1306401763 -7200
>> # Node ID 6d174e1bce47614b09f7e3fe3b9ccdd2d0bc9044
>> # Parent  c864f5e743efefeb667c6f8ffa659a8572047059
>> svn subrepo: abort only on nonzero process exit code, no longer assuming
>> warnings are errors (issue2833)
>>
>> diff -r c864f5e743ef -r 6d174e1bce47 mercurial/subrepo.py
>> --- a/mercurial/subrepo.py    Tue May 24 17:30:00 2011 -0500
>> +++ b/mercurial/subrepo.py    Thu May 26 11:22:43 2011 +0200
>> @@ -548,8 +548,10 @@
>>                               universal_newlines=True, env=env)
>>          stdout, stderr = p.communicate()
>>          stderr = stderr.strip()
>> -        if stderr:
>> -            raise util.Abort(stderr)
>> +        if p.returncode:
>> +            raise util.Abort(stderr or 'exited with code %d' % p.returncode)
>> +        if stderr:
>> +            self._ui.warn(stderr + '\n')
> I think you should print stderr as warnings before aborting.
(note: removed mail composition time typo + used single quotes in the 
first place)
Well, it would lead to a "log and throw" 
[http://today.java.net/article/2006/04/04/exception-handling-antipatterns#logAndThrow], 
with the effect of getting the error message printed twice to the console.
So, here the proposed implementation consists in:
1. if the command failed, raising an exception with the error output as 
message (as before) if available, defaulting to a frequently observed one.
2. otherwise, printing the error output as a warning and continue.
> Other than that, this looks and sounds reasonable and correct to me.
>
>>          return stdout
>>
>>      @propertycache
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel