Re: [PATCH] error messages

[Thorsten Wißmann <re06huxa _at_ cip _dot_ cs _dot_ fau _dot_ de>]

Hi Florian,

thanks for your effort you put into that large patch set. I merged most
of them (M-M-M-Monster-Merge), with some modifications or additions as
listed below.

On Sun, Nov 18, 2012 at 12:56:01AM +0100, Florian Bruhin wrote:
> * Florian Bruhin <me _at_ the _minus_ compiler _dot_ org> [2012-11-18 00:53:01 +0100]:
> Subject: [PATCH 01/25] Use HERBST_NEED_MORE_ARGS if a cmd needs more args
> diff --git a/ipc-client/main.c b/ipc-client/main.c
> index 14567ab..3f57db7 100644
> -        fputs(output->str, stdout);
> +        if (command_status == 0) { // success, output to stdout
> +            fputs(output->str, stdout);
> +        } else if (command_status == HERBST_NEED_MORE_ARGS) { // needs more arguments
> +            fputs(output->str, stderr);
> +            fprintf(stderr, "%s: not enough arguments\n", argv[arg_index]); // first argument == cmd
> +        } else { // other error, output to stderr
> +            fputs(output->str, stderr);
> +        }
>          if (g_ensure_newline) {
>              if (output->len > 0 && output->str[output->len - 1] != '\n') {
>                  fputs("\n", stdout);

Of course, you must output the newline on the same file as the
output->str. I fixed this in an extra commit.

c870389 client: Print \n to the same file as the output

> Subject: [PATCH 02/25] Output error message if command is not found

Totally correct.

> -int jumpto_command(int argc, char** argv) {
> +int jumpto_command(int argc, char** argv, GString* output) {
>      HSClient* client = NULL;
>      string_to_client((argc > 1) ? argv[1] : "", &client);
>      if (client) {
>          focus_window(client->window, true, true);
> +        return 0;
> +    } else {
> +        g_string_append_printf(output,
> +            "%s: error: Could not find client", argv[0]);
> +        if (argc > 1) {
> +            g_string_append_printf(output, " \"%s\".\n", argv[1]);
> +        } else {
> +            g_string_append(output, ".\n");
> +        }
> +        return HERBST_INVALID_ARGUMENT;
>      }
> -    return 0;
>  }

That's not perfect yet, because now, the exit status for a single
"jumpto" depends on, if a client or an empty frame is focused. I don't
think there is any real use case for this, so we can discuss about a
clean long-term solution and merge this for now.

>  int getenv_command(int argc, char** argv, GString* output) {
> @@ -363,28 +381,34 @@ int getenv_command(int argc, char** argv, GString* output) {
>      }
>      char* envvar = getenv(argv[1]);
>      if (envvar == NULL) {
> +        g_string_append_printf(output,
> +            "Environment variable \"%s\" is not set!\n", argv[1]);
>          return HERBST_ENV_UNSET;
>      }
>      g_string_append_printf(output, "%s\n", envvar);
>      return 0;
>  }

An unset environment variable in that context is not an error, I want
the behaviour of "herbstclient get foo" is similar to "echo $foo". And
one still can check if the variable was set or not by checking the error
message. That's also why the exit status is HERBST_ENV_UNSET and not
just "invalid ...". An intended usage is calling that in if-clauses (if
! hc get SSH_AGENT_PID ; then ...). So I removed that from your patch!

> -int detect_monitors_command(int argc, char **argv) {
> +int detect_monitors_command(int argc, char **argv, GString* output) {
>      MonitorDetection detect[] = {
>          detect_monitors_xinerama,
>          detect_monitors_simple,
> @@ -923,6 +947,10 @@ int detect_monitors_command(int argc, char **argv) {
>      // apply it
>      int ret = set_monitor_rects(monitors, count);
>      g_free(monitors);
> +    if (ret == HERBST_TAG_IN_USE && output != NULL) {
> +        g_string_append_printf(output,
> +            "%s: there are not enough free tags!\n", argv[0]);
> +    }
>      return ret;
>  }

Why do you check output here? You can use it unchecked, just like
anywhere else (or like argv).

> diff --git a/src/rules.c b/src/rules.c
>          else {
> -            fprintf(stderr, "rule: unknown argument \"%s\"\n", *argv);
> +            // need to hardcode "rule:" here because args are shifted
> +            g_string_append_printf(output,
> +                "rule: unknown argument \"%s\"\n", *argv);
>              return HERBST_INVALID_ARGUMENT;

Or remember it as char* cmdname = argv[0]; before the first SHIFT

> +    if (output->len > 0) { // if there was output
> +        g_string_prepend(output, "rule: ");
> +    }
>      g_queue_push_tail(&g_rules, rule);
>      return 0;

I removed that hunk because: 1. I can't see how this can be triggered
(because you return on any error before). 2. I currently want the
rule-command to output anything if it returns 0, because I plan to add
IDs for rules that are printed on creation if the "printid"-flag is
passed. (The ID then can be used to delete that single rule).

> Subject: [PATCH 04/25] Check if the rule command has enough arguments

OK.

> Subject: [PATCH 05/25] Return status code in key_remove_bind_with_keysym

If it's not a command, then the status code meaning isn't that
intuitive, so I prefer a bool here, so we know true is good and false is
bad. I just amended your commit. Which also affects the following
commit:

> Subject: [PATCH 06/25] Add error message when an unbound key is unbinded

> Subject: [PATCH 07/25] split: print error message if alignment is invalid

> Subject: [PATCH 08/25] Print error when frame neighbours are missing

That's OK, as there is no error message visible if the user calls that
command with a key binding. It should always be OK to press (even
useless) key-combinations on any layout tree.

> Subject: [PATCH 09/25] bring: add error message when client is invalid

> Subject: [PATCH 10/25] Make arguments mandatory in jumpto/bring
>  int raise_command(int argc, char** argv) {
> +    if (argc < 2) {
> +        return HERBST_NEED_MORE_ARGS;
> +    }
>      Window win;
>      HSClient* client = NULL;
> -    win = string_to_client((argc > 1) ? argv[1] : "", &client);
> +    win = string_to_client(argv[1], &client);
>      if (client) {
>          client_raise(client);
>      } else {

Actually we said, that it makes sense for raise. So i removed that hunk
and corrected it in the documentation.

> Subject: [PATCH 11/25] Print help when there are no hc-arguments

That's fine. I extended your code by a commit that prints the help to
stderr for that case.

> Subject: [PATCH 12/25] Cleanup of error messages
> Subject: [PATCH 13/25] Add an error message if "raise" can't find client

That's fine. (After some manual patch fixing ...). The following patches
could be applied without modification.

> Subject: [PATCH 14/25] Add error messages for remove_monitor
> Subject: [PATCH 15/25] Add error message for set_monitors
> Subject: [PATCH 16/25] Add error message for toggle for non-ints
> Subject: [PATCH 17/25] Add error message when setting a wrong type

> Subject: [PATCH 18/25] prepend "load: " to msgs in load_command
> +    if (output->len > 0) {
> +        g_string_prepend(output, "load: ");
> +    }

That's very hacky and only works because the load command normally does
not output anything. But I currently neither have a better idea nor
plans for a possible output of the "load" command.

> Subject: [PATCH 19/25] Avoid shifting in monitor commands

Why? IMO shifting the command name away make indices much more readable.
So I'm skipping this patch.

> Subject: [PATCH 20/25] rule: handle function name for error msgs better

Because you're only add fuzzy "command: "-prepending and remove a hunk I
already removed from the patc that adds it (see note above), I'll skip
this patch.

> Subject: [PATCH 21/25] Handle argv-shifting correctly in settings
>  int settings_cycle_value(int argc, char** argv, GString* output) {
> +    char* cmd_name = argv[0];
> +    char* setting_name = argv[1]; // save this before shifting
>      if (argc < 3) {
>          return HERBST_NEED_MORE_ARGS;
>      }

I moved the the assignments after the argc-check.

> Subject: [PATCH 22/25] Add error message if tag can't be switched
> +            g_string_append_printf(output,
> +                "%s: Could not change tag (maybe monitor is locked?)\n", argv[0]);
> +        }

Here I also like the monitor_set_tag function to return a bool
indicating the success/failure. And don't ask the user those question
you (in the role hlwm) can answer! That definitely must be fixed, I
merged it anyway to simplify the remaining patches.

> Subject: [PATCH 23/25] dump: More / clearer error messages
> Subject: [PATCH 24/25] tag_status: CLAMP correctly

Both are OK.

> Subject: [PATCH 25/25] Update MIGRATION

I replaced the german "When"s by "If"s. So the final list of commits
merged to master looks as follows:

91e2f6d Use HERBST_NEED_MORE_ARGS if a cmd needs more args
c870389 client: Print \n to the same file as the output
9aaa25b Output error message if command is not found
3dafb09 Add error output for nearly every command
6aea4e1 Check if the rule command has enough arguments
770319d Return status in key_remove_bind_with_keysym
921b8ce Add error message when an unbound key is unbinded
cde90ec split: print error message if alignment is invalid
72c0d49 Print error when frame neighbours are missing
c92084d bring: add error message when client is invalid
22b1bab Make arguments mandatory in jumpto/bring
ba2603f Document the argument for raise as optional
9d49fc8 Print help when there are no hc-arguments
b4bc24a Print help to stderr if hc arguments are missing
2c3ed85 Cleanup of error messages
44c6478 Add an error message if "raise" can't find client
e827cd8 Add error messages for remove_monitor
f4fa981 Add error message for set_monitors
3a6244b Add error message for toggle for non-ints
cb11de0 Add error message when setting a wrong type
b3e50c8 prepend "load: " to msgs in load_command
963d08a Handle argv-shifting correctly in settings
c9f18c9 Add error message if tag can't be switched
b6cae13 dump: More / clearer error messages
547d591 tag_status: CLAMP correctly
7baa11c Update MIGRATION

Regards,
Thorsten