bug#6963: 23.2; emacsclient -e "(remember-other-frame)" doesn’t give an error-code on error.

bug#6963: 23.2; emacsclient -e "(remember-other-frame)" doesn’t give an error-code on error.

[Arne Babenhauserheide]

[-- Attachment #1: Type: Text/Plain, Size: 710 bytes --]

I wanted to use emacsclient with remember mode via 
   
    emacsclient -e "(remember-other-frame)" || emacsclient -ce "(remember-
other-frame)" 
   
to make sure that I get a remember field as fast as possible, whether I 
already have a frame or not. 
   
I get “*ERROR*: Unknown terminal type”, but the -ce command isn’t evoked. 
   
What I expected was to get an error code, so the second command gets called 
and gives me the remember frame. 
   
(which is darn useful, by the way!)

Best wishes and many thanks for working on emacs! It’s just great! 
Arne

--
Ich hab' nichts zu verbergen – hab ich gedacht: 

- http://draketo.de/licht/lieder/ich-hab-nichts-zu-verbergen


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 316 bytes --]

bug#6963: More usecases, patch attached

[Wolfgang Schnerring]

[-- Attachment #1.1: Type: text/plain, Size: 868 bytes --]

emacsclient is such a usefull tool. But that it does not signal error
situations via its exit status severely hinders its usage in shell
scripts or in integration with other programs.

For example, I was bit by this rather badly when I tried to use
emacsclient to start an ediff session, to use as a merge tool for the
Mercurial SCM.
I may have made a typo in configuring said ediff command or for
whatever other reason, Emacs returned an error. But Mercurial was
blissfully unaware of this, since emacsclient returns 0 no matter
what. End result: Mercurial committed a broken merge that I had to
spend at least two hours some days later to debug.

I've attached a patch that makes emacsclient exit with nonzero status
when it receives an error message from Emacs -- it already *prints*
"*ERROR*" in these cases, so I feel this makes a lot of sense.

Thanks,
Wolfgang

[-- Attachment #1.2: emacsclient-exitstatus.patch --]
[-- Type: text/plain, Size: 1116 bytes --]

=== modified file 'lib-src/emacsclient.c'
--- lib-src/emacsclient.c	2010-08-11 08:20:34 +0000
+++ lib-src/emacsclient.c	2010-09-29 05:57:38 +0000
@@ -1506,6 +1506,7 @@
   char *cwd, *str;
   char string[BUFSIZ+1];
   int null_socket_name, null_server_file, start_daemon_if_needed;
+  int exit_status = EXIT_SUCCESS;
 
   main_argv = argv;
   progname = argv[0];
@@ -1746,6 +1747,7 @@
             printf ("\n");
           fprintf (stderr, "*ERROR*: %s", str);
           needlf = str[0] == '\0' ? needlf : str[strlen (str) - 1] != '\n';
+          exit_status = EXIT_FAILURE;
         }
 #ifdef SIGSTOP
       else if (strprefix ("-suspend ", string))
@@ -1764,6 +1766,7 @@
             printf ("\n");
           printf ("*ERROR*: Unknown message: %s", string);
           needlf = string[0] == '\0' ? needlf : string[strlen (string) - 1] != '\n';
+          exit_status = EXIT_FAILURE;
         }
     }
 
@@ -1773,7 +1776,7 @@
   fsync (1);
 
   CLOSE_SOCKET (emacs_socket);
-  return EXIT_SUCCESS;
+  return exit_status;
 }
 
 #endif /* HAVE_SOCKETS && HAVE_INET_SOCKETS */


[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

bug#6963: More usecases, patch attached

[Thierry Volpiatto]

Wolfgang Schnerring <wosc@wosc.de> writes:

> emacsclient is such a usefull tool. But that it does not signal error
> situations via its exit status severely hinders its usage in shell
> scripts or in integration with other programs.
>
> For example, I was bit by this rather badly when I tried to use
> emacsclient to start an ediff session, to use as a merge tool for the
> Mercurial SCM.
> I may have made a typo in configuring said ediff command or for
> whatever other reason, Emacs returned an error. But Mercurial was
> blissfully unaware of this, since emacsclient returns 0 no matter
> what. End result: Mercurial committed a broken merge that I had to
> spend at least two hours some days later to debug.
>
> I've attached a patch that makes emacsclient exit with nonzero status
> when it receives an error message from Emacs -- it already *prints*
> "*ERROR*" in these cases, so I feel this makes a lot of sense.
>
> Thanks,
> Wolfgang
>

Did you use your own script or the mercurial script to use
emacsclient/ediff?

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#6963: More usecases, patch attached

[Juanma Barranquero]

On Wed, Sep 29, 2010 at 08:09, Wolfgang Schnerring <wosc@wosc.de> wrote:

> But that it does not signal error
> situations via its exit status

It does in most cases.

> since emacsclient returns 0 no matter what.

That's not accurate. When emacsclient detects a problem with itself
(missing server file, error opening socket, etc.) it returns
EXIT_FAILURE (1). What it currently does not do, however, is returning
EXIT_FAILURE for errors passed back from Emacs.

> I've attached a patch that makes emacsclient exit with nonzero status
> when it receives an error message from Emacs -- it already *prints*
> "*ERROR*" in these cases, so I feel this makes a lot of sense.

Yes, it makes sense, because after -error the client's process is
deleted, so emacsclient exits immediately.

So, two questions remain:

- should this be considered a bug, and installed on emacs-23, or a new
feature, for the trunk?
- should emacsclient return 1 for these new errors, or a new error
code (2) to allow distinguishing them?

    Juanma

bug#6963: More usecases, patch attached

[Wolfgang Schnerring]

* Juanma Barranquero <lekktu@gmail.com> [2010-09-29 16:42]:
> On Wed, Sep 29, 2010 at 08:09, Wolfgang Schnerring <wosc@wosc.de> wrote:
> > since emacsclient returns 0 no matter what.
> That's not accurate. When emacsclient detects a problem with itself
> (missing server file, error opening socket, etc.) it returns
> EXIT_FAILURE (1). What it currently does not do, however, is returning
> EXIT_FAILURE for errors passed back from Emacs.

Right. Your description is accurate, I was glossing over a bit too
much, sorry.
 
> - should this be considered a bug, and installed on emacs-23, or a new
> feature, for the trunk?

I personally consider it a bug, since everything else about "errors
from Emacs" is already there, only the exit status is missing. And
that really does not play nice with others.
(That said, this is a gray area, and one could probably also just as
well argue the opposite.)

> - should emacsclient return 1 for these new errors, or a new error
> code (2) to allow distinguishing them?

It probably makes sense to distinguish these, since they are different
types of errors, technical ("communication with Emacs failed") vs.
semantic ("Emacs said boo!").
One might want to invoke something that has known error cases, which
would benefit from being treated differently than a socket error or
somesuch.

Wolfgang

bug#6963: More usecases, patch attached

[Juanma Barranquero]

On Wed, Sep 29, 2010 at 17:08, Wolfgang Schnerring <wosc@wosc.de> wrote:

> I personally consider it a bug, since everything else about "errors
> from Emacs" is already there, only the exit status is missing. And
> that really does not play nice with others.
> (That said, this is a gray area, and one could probably also just as
> well argue the opposite.)

I tend to agree with you about it being a bug, but OTOH I try to be
extremely conservative about committing changes into the release
branch, other than typos, docfixes, regression fixes and
very-obviously-correct bugfixes. That's the stated (though not always
followed) policy, and I think it's a very good one.

In this case, the patch changes the behavior of emacsclient, so even
if it fixes a bug, it could still affect users of emacsclient in
non-obvious ways (because, as you do, they could be using emacsclient
from scripts, tools, etc.). So I'd say it's best to keep this for the
trunk.

> It probably makes sense to distinguish these, since they are different
> types of errors, technical ("communication with Emacs failed") vs.
> semantic ("Emacs said boo!").
> One might want to invoke something that has known error cases, which
> would benefit from being treated differently than a socket error or
> somesuch.

I agree, and it doesn't make the patch more complicate (just one more line).

    Juanma

bug#6963: More usecases, patch attached

[Wolfgang Schnerring]

* Juanma Barranquero <lekktu@gmail.com> [2010-09-29 17:43]:
> In this case, the patch changes the behavior of emacsclient, so even
> if it fixes a bug, it could still affect users of emacsclient in
> non-obvious ways (because, as you do, they could be using emacsclient
> from scripts, tools, etc.). So I'd say it's best to keep this for the
> trunk.

Yes, this change is "backwards incompatible", since, well, Emacs
errors will then return nonzero, duh ;-), and that might not match
current expectations. Anyway, it's your call, not mine.

Wolfgang

bug#6963: More usecases, patch attached

[Juanma Barranquero]

On Wed, Sep 29, 2010 at 16:42, Juanma Barranquero <lekktu@gmail.com> wrote:

> Yes, it makes sense, because after -error the client's process is
> deleted, so emacsclient exits immediately.

Hmm.

There are two cases:

1) The emacs server sends -error XXX, and closes the connection.
emacsclient prints the error and exits.
2) The emacs server sends an unknown command; emacsclient prints an
error and continues.

With your patch, the case 2) would return EXIT_FAILURE (or the new
exit code we discussed) even if subsequent commands are dealt with
correctly.

IIRC, receiving an unknown command from Emacs shouldn't be a fatal
error; it can happen if you use an older emacsclient to connect to a
more recent server.el with new functionality. Or should it?

Comments anyone?

    Juanma

bug#6963: More usecases, patch attached

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

> 1) The emacs server sends -error XXX, and closes the connection.
> emacsclient prints the error and exits.
> 2) The emacs server sends an unknown command; emacsclient prints an
> error and continues.
>
> With your patch, the case 2) would return EXIT_FAILURE (or the new
> exit code we discussed) even if subsequent commands are dealt with
> correctly.
>
> IIRC, receiving an unknown command from Emacs shouldn't be a fatal
> error; it can happen if you use an older emacsclient to connect to a
> more recent server.el with new functionality. Or should it?

I agree, the second case should not lead to EXIT_FAILURE.  I've checked
the patch, with this and a couple of other corrections, into the trunk.

bug#6963: More usecases, patch attached

[Juanma Barranquero]

On Sun, Oct 3, 2010 at 02:00, Chong Yidong <cyd@stupidchicken.com> wrote:

> I agree, the second case should not lead to EXIT_FAILURE.  I've checked
> the patch, with this and a couple of other corrections, into the trunk.

OK, though I agree with Wolfgang that the first case would be more
useful returning a new EXIT_EMACSERROR (2) instead of EXIT_FAILURE
(1).

    Juanma

bug#6963: More usecases, patch attached

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Sun, Oct 3, 2010 at 02:00, Chong Yidong <cyd@stupidchicken.com> wrote:
>
>> I agree, the second case should not lead to EXIT_FAILURE.  I've checked
>> the patch, with this and a couple of other corrections, into the trunk.
>
> OK, though I agree with Wolfgang that the first case would be more
> useful returning a new EXIT_EMACSERROR (2) instead of EXIT_FAILURE
> (1).

I'm ambivalent.  This argument implies that we should give different
exit code for everything that could lead to failure in emacsclient, and
there are 18 separate cases in emacsclient.c.  It seems to me that, in
practice, people don't bother looking up the exit code, since there's no
established convention (other than 0 for success and 1 for failure);
they just look at the error message.

bug#6963: More usecases, patch attached

[Juanma Barranquero]

On Sun, Oct 3, 2010 at 06:55, Chong Yidong <cyd@stupidchicken.com> wrote:

> I'm ambivalent.  This argument implies that we should give different
> exit code for everything that could lead to failure in emacsclient, and
> there are 18 separate cases in emacsclient.c.

Well, I don't think so. All 18 cases are clearly separated into:

 - emacsclient had a problem (unable to create socket, bad
authorization file, etc.)
 - emacsclient worked fine, but Emacs had a problem.

but I suppose there's no harm in waiting for some use case before
going that route.

    Juanma

bug#6963: Thank you!

[Arne Babenhauserheide]

Many thanks for including the patch (and for writing it)!