| Submitter | Yuya Nishihara |
|---|---|
| Date | Oct. 1, 2017, noon |
| Message ID | <86c51cf6c4ff8a9a01e3.1506859218@mimosa> |
| Download | mbox | patch |
| Permalink | /patch/24337/ |
| State | Accepted |
| Headers | show |
Comments
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
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.
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
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.)
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:" ?)
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: