Date: Wed, 28 Dec 2011 21:24:19 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: David Bremner <david@tethera.net>
Subject: Re: have "notmuch help" call man?
Message-ID: <20111229022419.GI30150@mit.edu>
References: <87zkescxp5.fsf@zancas.localnet> <87iplbyj0w.fsf@zancas.localnet>
	<87ehvxtgoi.fsf@zancas.localnet>
	<87k45pqkaz.fsf@servo.finestructure.net>
	<8739c6yhp0.fsf@zancas.localnet>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8739c6yhp0.fsf@zancas.localnet>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: Notmuch Mail <notmuch@notmuchmail.org>
Precedence: list

Quoth David Bremner on Dec 26 at  8:35 pm:
> The following changes since commit d61cef374b3234c2f1327fa74b612b40c196a605:
> 
>   show: Rewrite show_message_body to use the MIME tree interface. (2011-12-25 22:23:15 -0400)
> 
> are available in the git repository at:
>   git://pivot.cs.unb.ca/notmuch split-man
> 
> HEAD is at 8eaee754e48dc630d83e2f7369bdadde0f34af84

Some comments on "notmuch: replace built-in help with exec of man"

> @@ -534,38 +126,20 @@ notmuch_help_command (unused (void *ctx), int argc, char *argv[])
>      for (i = 0; i < ARRAY_SIZE (commands); i++) {
>  	command = &commands[i];
>  
> -	if (strcmp (argv[0], command->name) == 0) {
> -	    printf ("Help for \"notmuch %s\":\n\n", argv[0]);
> -	    if (command->arguments)
> -		printf ("%s %s\n\n\t%s\n\n%s\n\n",
> -			command->name, command->arguments,
> -			command->summary, command->documentation);
> -	    else
> -		printf ("%s\t%s\n\n%s\n\n", command->name,
> -			command->summary, command->documentation);
> -	    return 0;
> +	if (strncmp (argv[0], command->name, strlen (argv[0])) == 0) {

Shouldn't this be strcmp?

> +	    char *page = malloc (strlen (argv[0]) + strlen ("notmuch-") + 1);
> +	    strcpy (page, "notmuch-");
> +	    strcat (page, command->name);

This is fine, but could be cleaner with talloc:
  char *page = talloc_asprintf (ctx, "notmuch-%s", command->name);

> +
> +	    execlp ("man", "man", page, (char *) NULL);
> +	    /* NOTREACHED */

This certainly *can* be reached if the exec fails.  You should print a
helpful error message and exit, maybe something like
  fprintf (stderr, "failed to exec man: %s\n", strerror (errno));
  exit (1);

>  	}
>      }
>  
> -    if (strcmp (argv[0], "search-terms") == 0) {
> -	printf ("Help for <%s>\n\n", argv[0]);
> -	for (i = 0; i < ARRAY_SIZE (commands); i++) {
> -	    command = &commands[i];
> -
> -	    if (command->arguments &&
> -		strstr (command->arguments, "search-terms"))
> -	    {
> -		printf ("\t%s\t%s\n",
> -			command->name, command->arguments);
> -	    }
> -	}
> -	printf ("\n");
> -	printf (search_terms_help);
> -	return 0;
> -    } else if (strcmp (argv[0], "hooks") == 0) {
> -	printf ("Help for <%s>\n\n", argv[0]);
> -	printf (hooks_help);
> -	return 0;
> +    if (strncmp (argv[0], "search-terms", strlen (argv[0])) == 0) {
> +	execlp ("man", "man", "notmuch-search-terms", (char *) NULL);
> +    } else if (strncmp (argv[0], "hooks", strlen (argv[0])) == 0) {
> +	execlp ("man", "man", "notmuch-hooks", (char *) NULL);

Same comments about strncmp and exec failing for the above four lines.
It might make sense to merge all of the exec calls in to one to
centralize the error handling.

>      }
>  
>      fprintf (stderr,