Re: [PATCH 7 of 7] extdata: abort if external command exits with non-zero status
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 > > > > # 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7] extdata: abort if external command exits with non-zero status
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 > > > # 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:" ?) -- -- [FUJIWARA Katsunori] fo...@lares.dti.ne.jp ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7] extdata: abort if external command exits with non-zero status
On Sun, Oct 01, 2017 at 01:00:18PM +0100, Yuya Nishihara wrote: > # HG changeset patch > # User Yuya Nishihara > # 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.) ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7] extdata: abort if external command exits with non-zero status
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 > > > # 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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7] extdata: abort if external command exits with non-zero status
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 > > # 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 7 of 7] extdata: abort if external command exits with non-zero status
At Sun, 01 Oct 2017 13:00:18 +0100, Yuya Nishihara wrote: > > # HG changeset patch > # User Yuya Nishihara > # 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 -- -- [FUJIWARA Katsunori] fo...@lares.dti.ne.jp ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
[PATCH 7 of 7] extdata: abort if external command exits with non-zero status
# HG changeset patch
# User Yuya Nishihara
# 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/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:
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
