Patchwork [7,of,7] extdata: abort if external command exits with non-zero status

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 1, 2017, noon
Message ID <86c51cf6c4ff8a9a01e3.1506859218@mimosa>
Download mbox | patch
Permalink /patch/24337/
State Accepted
Headers show

Comments

Yuya Nishihara - Oct. 1, 2017, noon
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1506856910 -3600
#      Sun Oct 01 12:21:50 2017 +0100
# Node ID 86c51cf6c4ff8a9a01e34e365c8fbc50415d072e
# Parent  b3a36705720f5ed1e7cc5129b27450ca59c7952b
extdata: abort if external command exits with non-zero status

It could be a warning, but a warning in template output would be unreadable.
Katsunori FUJIWARA - Oct. 1, 2017, 4:15 p.m.
At Sun, 01 Oct 2017 13:00:18 +0100,
Yuya Nishihara wrote:
> 
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1506856910 -3600
> #      Sun Oct 01 12:21:50 2017 +0100
> # Node ID 86c51cf6c4ff8a9a01e34e365c8fbc50415d072e
> # Parent  b3a36705720f5ed1e7cc5129b27450ca59c7952b
> extdata: abort if external command exits with non-zero status
> 
> It could be a warning, but a warning in template output would be unreadable.

> diff --git a/tests/test-extdata.t b/tests/test-extdata.t
> --- a/tests/test-extdata.t
> +++ b/tests/test-extdata.t
> @@ -12,6 +12,7 @@ test revset support
>    > filedata = file:extdata.txt
>    > notes = notes.txt
>    > shelldata = shell:cat extdata.txt | grep 2

Just nit:

I sometime forget that filtering by grep might cause non-zero exit
code. So, hint like "use `| grep PATTERN | cat` to allow empty
external data" or so will prevent me from falling into a pitfall in
the future :-)


> +  > fail = shell:false
>    > EOF
>    $ cat <<'EOF' > extdata.txt
>    > 2 another comment on 2
> @@ -50,6 +51,9 @@ test bad extdata() revset source
>    $ hg log -qr "extdata(unknown)"
>    abort: unknown extdata source 'unknown'
>    [255]
> +  $ hg log -qr "extdata(fail)"
> +  abort: extdata command 'false' failed: exited with status 1
> +  [255]
>  
>  test template support:
>  
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Oct. 3, 2017, 1:22 p.m.
On Sun, 01 Oct 2017 17:15:06 +0100, FUJIWARA Katsunori wrote:
> At Sun, 01 Oct 2017 13:00:18 +0100,
> Yuya Nishihara wrote:
> > 
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1506856910 -3600
> > #      Sun Oct 01 12:21:50 2017 +0100
> > # Node ID 86c51cf6c4ff8a9a01e34e365c8fbc50415d072e
> > # Parent  b3a36705720f5ed1e7cc5129b27450ca59c7952b
> > extdata: abort if external command exits with non-zero status
> > 
> > It could be a warning, but a warning in template output would be unreadable.
> 
> > diff --git a/tests/test-extdata.t b/tests/test-extdata.t
> > --- a/tests/test-extdata.t
> > +++ b/tests/test-extdata.t
> > @@ -12,6 +12,7 @@ test revset support
> >    > filedata = file:extdata.txt
> >    > notes = notes.txt
> >    > shelldata = shell:cat extdata.txt | grep 2
> 
> Just nit:
> 
> I sometime forget that filtering by grep might cause non-zero exit
> code. So, hint like "use `| grep PATTERN | cat` to allow empty
> external data" or so will prevent me from falling into a pitfall in
> the future :-)

Good catch. So it might not be ideal to check the exit code by default.
Augie Fackler - Oct. 4, 2017, 12:54 p.m.
On Tue, Oct 03, 2017 at 10:22:52PM +0900, Yuya Nishihara wrote:
> On Sun, 01 Oct 2017 17:15:06 +0100, FUJIWARA Katsunori wrote:
> > At Sun, 01 Oct 2017 13:00:18 +0100,
> > Yuya Nishihara wrote:
> > >
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@tcha.org>
> > > # Date 1506856910 -3600
> > > #      Sun Oct 01 12:21:50 2017 +0100
> > > # Node ID 86c51cf6c4ff8a9a01e34e365c8fbc50415d072e
> > > # Parent  b3a36705720f5ed1e7cc5129b27450ca59c7952b
> > > extdata: abort if external command exits with non-zero status
> > >
> > > It could be a warning, but a warning in template output would be unreadable.
> >
> > > diff --git a/tests/test-extdata.t b/tests/test-extdata.t
> > > --- a/tests/test-extdata.t
> > > +++ b/tests/test-extdata.t
> > > @@ -12,6 +12,7 @@ test revset support
> > >    > filedata = file:extdata.txt
> > >    > notes = notes.txt
> > >    > shelldata = shell:cat extdata.txt | grep 2
> >
> > Just nit:
> >
> > I sometime forget that filtering by grep might cause non-zero exit
> > code. So, hint like "use `| grep PATTERN | cat` to allow empty
> > external data" or so will prevent me from falling into a pitfall in
> > the future :-)
>
> Good catch. So it might not be ideal to check the exit code by default.

Maybe it should be as a debug message, so that with --debug these
things can be figured out.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Oct. 4, 2017, 12:58 p.m.
On Sun, Oct 01, 2017 at 01:00:18PM +0100, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1506856910 -3600
> #      Sun Oct 01 12:21:50 2017 +0100
> # Node ID 86c51cf6c4ff8a9a01e34e365c8fbc50415d072e
> # Parent  b3a36705720f5ed1e7cc5129b27450ca59c7952b
> extdata: abort if external command exits with non-zero status

I've queued 1-6, but will await a version of 7 that logs a warning or
similar per foozy's comment about grep (which I appreciated, as I
would have missed that.)
Katsunori FUJIWARA - Oct. 4, 2017, 5:25 p.m.
At Tue, 3 Oct 2017 22:22:52 +0900,
Yuya Nishihara wrote:
> 
> On Sun, 01 Oct 2017 17:15:06 +0100, FUJIWARA Katsunori wrote:
> > At Sun, 01 Oct 2017 13:00:18 +0100,
> > Yuya Nishihara wrote:
> > > 
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@tcha.org>
> > > # Date 1506856910 -3600
> > > #      Sun Oct 01 12:21:50 2017 +0100
> > > # Node ID 86c51cf6c4ff8a9a01e34e365c8fbc50415d072e
> > > # Parent  b3a36705720f5ed1e7cc5129b27450ca59c7952b
> > > extdata: abort if external command exits with non-zero status
> > > 
> > > It could be a warning, but a warning in template output would be unreadable.
> > 
> > > diff --git a/tests/test-extdata.t b/tests/test-extdata.t
> > > --- a/tests/test-extdata.t
> > > +++ b/tests/test-extdata.t
> > > @@ -12,6 +12,7 @@ test revset support
> > >    > filedata = file:extdata.txt
> > >    > notes = notes.txt
> > >    > shelldata = shell:cat extdata.txt | grep 2
> > 
> > Just nit:
> > 
> > I sometime forget that filtering by grep might cause non-zero exit
> > code. So, hint like "use `| grep PATTERN | cat` to allow empty
> > external data" or so will prevent me from falling into a pitfall in
> > the future :-)
> 
> Good catch. So it might not be ideal to check the exit code by default.
> 

But it is also fact that "abort at non-zero exit code" itself is
reasonable for consistency, because validity of external data isn't
ensured at "non-zero exit code".

Therefore, let's write document with an explanation like
"unintentional failure with grep" (in the future) ! :-)

(or does we need another "ignore exist code of shell command" scheme
other than "shell:" ?)
Yuya Nishihara - Oct. 5, 2017, 1:32 p.m.
On Wed, 04 Oct 2017 18:25:41 +0100, FUJIWARA Katsunori wrote:
> At Tue, 3 Oct 2017 22:22:52 +0900,
> Yuya Nishihara wrote:
> > 
> > On Sun, 01 Oct 2017 17:15:06 +0100, FUJIWARA Katsunori wrote:
> > > At Sun, 01 Oct 2017 13:00:18 +0100,
> > > Yuya Nishihara wrote:
> > > > 
> > > > # HG changeset patch
> > > > # User Yuya Nishihara <yuya@tcha.org>
> > > > # Date 1506856910 -3600
> > > > #      Sun Oct 01 12:21:50 2017 +0100
> > > > # Node ID 86c51cf6c4ff8a9a01e34e365c8fbc50415d072e
> > > > # Parent  b3a36705720f5ed1e7cc5129b27450ca59c7952b
> > > > extdata: abort if external command exits with non-zero status
> > > > 
> > > > It could be a warning, but a warning in template output would be unreadable.
> > > 
> > > > diff --git a/tests/test-extdata.t b/tests/test-extdata.t
> > > > --- a/tests/test-extdata.t
> > > > +++ b/tests/test-extdata.t
> > > > @@ -12,6 +12,7 @@ test revset support
> > > >    > filedata = file:extdata.txt
> > > >    > notes = notes.txt
> > > >    > shelldata = shell:cat extdata.txt | grep 2
> > > 
> > > Just nit:
> > > 
> > > I sometime forget that filtering by grep might cause non-zero exit
> > > code. So, hint like "use `| grep PATTERN | cat` to allow empty
> > > external data" or so will prevent me from falling into a pitfall in
> > > the future :-)
> > 
> > Good catch. So it might not be ideal to check the exit code by default.
> 
> But it is also fact that "abort at non-zero exit code" itself is
> reasonable for consistency, because validity of external data isn't
> ensured at "non-zero exit code".
> 
> Therefore, let's write document with an explanation like
> "unintentional failure with grep" (in the future) ! :-)
> 
> (or does we need another "ignore exist code of shell command" scheme
> other than "shell:" ?)

encode/decode filters?

I'm not sure if which will be better. For grep, erroring out is inconvenient,
but for curl, non-zero status should be detected. However, the latter isn't
possible if an error is ignored at all. So I lean towards raising Abort in
such case.

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -1039,7 +1039,7 @@  def extdatasource(repo, source):
         raise error.Abort(_("unknown extdata source '%s'") % source)
 
     data = {}
-    src = proc = None
+    src = proc = err = None
     try:
         if spec.startswith("shell:"):
             # external commands should be run relative to the repo root
@@ -1065,8 +1065,13 @@  def extdatasource(repo, source):
     finally:
         if proc:
             proc.communicate()
+            if proc.returncode != 0:
+                err = (_("extdata command '%s' failed: %s")
+                       % (cmd, util.explainexit(proc.returncode)[0]))
         if src:
             src.close()
+    if err:
+        raise error.Abort(err)
 
     return data
 
diff --git a/tests/test-extdata.t b/tests/test-extdata.t
--- a/tests/test-extdata.t
+++ b/tests/test-extdata.t
@@ -12,6 +12,7 @@  test revset support
   > filedata = file:extdata.txt
   > notes = notes.txt
   > shelldata = shell:cat extdata.txt | grep 2
+  > fail = shell:false
   > EOF
   $ cat <<'EOF' > extdata.txt
   > 2 another comment on 2
@@ -50,6 +51,9 @@  test bad extdata() revset source
   $ hg log -qr "extdata(unknown)"
   abort: unknown extdata source 'unknown'
   [255]
+  $ hg log -qr "extdata(fail)"
+  abort: extdata command 'false' failed: exited with status 1
+  [255]
 
 test template support: