Add quotes around command line in subproccess error message

Add quotes around command line in subproccess error message

Allows the command line itself to be distinguished
from the surrounding error message.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295481

[Sam Clegg, 2015-06-01 21:34:23]

sbc@chromium.org changed reviewers:
+ maruel@chromium.org

[chromium-reviews, 2015-06-01 21:38:38]

If you use %r python will quote it for you.

On Mon, Jun 1, 2015, 14:34  <sbc@chromium.org> wrote:

> Reviewers: M-A Ruel,
>
> Description:
> Add quotes around command line in subproccess error message
>
> Allows the command line itself to be distinguished
> from the surrounding error message.
>
> Please review this at https://codereview.chromium.org/1152443004/
>
> Base URL: svn://svn.chromium.org/chrome/trunk/tools/depot_tools
>
> Affected files (+1, -1 lines):
>    M subprocess2.py
>
>
> Index: subprocess2.py
> diff --git a/subprocess2.py b/subprocess2.py
> index
>
>
6e138a503fbbd22340ab74c65f2e4580536ac0ee..55e5eb417557704043a59d3c539fc9a8e843534d
> 100644
> --- a/subprocess2.py
> +++ b/subprocess2.py
> @@ -40,7 +40,7 @@ class CalledProcessError(subprocess.CalledProcessError):
>       self.cwd = cwd
>
>     def __str__(self):
> -    out = 'Command %s returned non-zero exit status %s' % (
> +    out = "Command '%s' returned non-zero exit status %s" % (
>           ' '.join(self.cmd), self.returncode)
>       if self.cwd:
>         out += ' in ' + self.cwd
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sam Clegg, 2015-06-01 21:47:15]

On 2015/06/01 21:38:38, chromium-reviews wrote:
> If you use %r python will quote it for you.

Hmm.  OK.  I kinda prefer the explicit quotes but the result is the same

Done.

> 
> On Mon, Jun 1, 2015, 14:34  <mailto:sbc@chromium.org> wrote:
> 
> > Reviewers: M-A Ruel,
> >
> > Description:
> > Add quotes around command line in subproccess error message
> >
> > Allows the command line itself to be distinguished
> > from the surrounding error message.
> >
> > Please review this at https://codereview.chromium.org/1152443004/
> >
> > Base URL: svn://svn.chromium.org/chrome/trunk/tools/depot_tools
> >
> > Affected files (+1, -1 lines):
> >    M subprocess2.py
> >
> >
> > Index: subprocess2.py
> > diff --git a/subprocess2.py b/subprocess2.py
> > index
> >
> >
>
6e138a503fbbd22340ab74c65f2e4580536ac0ee..55e5eb417557704043a59d3c539fc9a8e843534d
> > 100644
> > --- a/subprocess2.py
> > +++ b/subprocess2.py
> > @@ -40,7 +40,7 @@ class CalledProcessError(subprocess.CalledProcessError):
> >       self.cwd = cwd
> >
> >     def __str__(self):
> > -    out = 'Command %s returned non-zero exit status %s' % (
> > +    out = "Command '%s' returned non-zero exit status %s" % (
> >           ' '.join(self.cmd), self.returncode)
> >       if self.cwd:
> >         out += ' in ' + self.cwd
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Sam Clegg, 2015-06-01 21:47:41]

The CQ bit was checked by sbc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-01 21:48:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1152443004/40001

[commit-bot: I haz the power, 2015-06-01 21:50:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-01 21:50:49]

Dry run: Presubmit check for 1152443004-40001 failed and returned exit status 1.

Running presubmit commit checks ...
Looking up path
/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/google_appengine
Didn't find an SDK
New version is 1.9.21
Fetching
https://storage.googleapis.com/appengine-sdks/featured/google_appengine_1.9.2...
Extracting into
/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/google_appengine
Extracted 14455 files
Initialized empty Git repository in
/b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/_rietveld/.git/
Updating origin
From https://chromium.googlesource.com/infra/infra
 * [new branch]      deployed   -> origin/deployed
 * [new branch]      infra/config -> origin/infra/config
 * [new branch]      master     -> origin/master
From https://chromium.googlesource.com/infra/infra
 * branch            master     -> FETCH_HEAD
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/subprocess2.py
    depot_tools/tests/gclient_smoketest.py

push-basic.sh failed
Command
/b/infra_internal/commit_queue/workdir/tools/depot_tools/tests/push-basic.sh
returned non-zero exit status 1 in
/b/infra_internal/commit_queue/workdir/tools/depot_tools/tests
Setting up test upstream git repo...
Setting up test git repo...
TESTING: git-cl upload wants a server
TESTING: git-cl status has no issue
TESTING: upload succeeds (needs a server running on localhost)
TESTING: git-cl status now knows the issue
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--    
0
100    77    0     0  100    77      0    375 --:--:-- --:--:-- --:--:--  
375
100    77    0     0  100    77      0    253 --:--:-- --:--:-- --:--:--  
253
TESTING: Base URL contains branch name
FAILURE: Base URL contains branch name


Presubmit checks took 155.5s to calculate.

[iannucci, 2015-06-01 21:51:57]

iannucci@chromium.org changed reviewers:
+ iannucci@chromium.org

[iannucci, 2015-06-01 21:51:58]

In general I prefer %r because it means that you still get a comprehensible
thing even if:
  * the thing you're printing turns out to be not-a-string
  * the thing you're printing contains quotes
  * the thing you're printing contains non-printable characters

https://codereview.chromium.org/1152443004/diff/40001/subprocess2.py
File subprocess2.py (right):

https://codereview.chromium.org/1152443004/diff/40001/subprocess2.py#newcode43
subprocess2.py:43: out = "Command %r returned non-zero exit status %s" % (
we prefer single quotes in this file, I think.

https://codereview.chromium.org/1152443004/diff/40001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/1152443004/diff/40001/tests/gclient_smoketest...
tests/gclient_smoketest.py:521: "sys.exit(1)' returned non-zero exit status 1 in
%s\n"
here too.

[Sam Clegg, 2015-06-01 21:58:45]

https://codereview.chromium.org/1152443004/diff/40001/subprocess2.py
File subprocess2.py (right):

https://codereview.chromium.org/1152443004/diff/40001/subprocess2.py#newcode43
subprocess2.py:43: out = "Command %r returned non-zero exit status %s" % (
On 2015/06/01 21:51:58, iannucci wrote:
> we prefer single quotes in this file, I think.

Done.

https://codereview.chromium.org/1152443004/diff/40001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/1152443004/diff/40001/tests/gclient_smoketest...
tests/gclient_smoketest.py:521: "sys.exit(1)' returned non-zero exit status 1 in
%s\n"
On 2015/06/01 21:51:58, iannucci wrote:
> here too.

Even when embedding single quotes?

[iannucci, 2015-06-01 22:05:45]

Ah, yeah it's fine for single quotes. I usually use backticks for this purpose
though:

'some string: `a command` and stuff'

[iannucci, 2015-06-01 22:05:59]

lgtm

[Sam Clegg, 2015-06-01 22:06:24]

The CQ bit was checked by sbc@chromium.org

[commit-bot: I haz the power, 2015-06-01 22:07:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1152443004/60001

[commit-bot: I haz the power, 2015-06-01 22:10:16]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295481