Re: [PATCH] help: introduce man.viewer = eman

[John Keeping <john@keeping.me.uk>]

On Sat, Jun 22, 2013 at 05:13:29PM +0530, Ramkumar Ramachandra wrote:
> Corresponding to woman.
> 
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-help.txt |  3 +++
>  builtin/help.c             | 11 ++++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index b21e9d7..0cb4c46 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -104,6 +104,9 @@ The 'man.viewer' config variable will be checked if the 'man' format
>  is chosen. The following values are currently supported:
>  
>  * "man": use the 'man' program as usual,
> +* "eman": use 'emacsclient' to launch the "man" mode in emacs
> +(this only works starting with emacsclient versions 22), on systems
> +with man,
>  * "woman": use 'emacsclient' to launch the "woman" mode in emacs
>  (this only works starting with emacsclient versions 22),
>  * "konqueror": use 'kfmclient' to open the man page in a new konqueror
> diff --git a/builtin/help.c b/builtin/help.c
> index 062957f..7cb44e0 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -120,7 +120,7 @@ static int check_emacsclient_version(void)
>  	return 0;
>  }
>  
> -static void exec_woman_emacs(const char *path, const char *page)
> +static void exec_woman_emacs(const char *path, const char *page, int eman)
>  {
>  	if (!check_emacsclient_version()) {
>  		/* This works only with emacsclient version >= 22. */
> @@ -128,7 +128,10 @@ static void exec_woman_emacs(const char *path, const char *page)
>  
>  		if (!path)
>  			path = "emacsclient";
> -		strbuf_addf(&man_page, "(woman \"%s\")", page);
> +		if (eman)
> +			strbuf_addf(&man_page, "(man \"%s\")", page);
> +		else
> +			strbuf_addf(&man_page, "(woman \"%s\")", page);

Would it be nicer to pass a string in here instead of a flag?  Then this
becomes:

	strbuf_addf(&man_page, "(%s \"%s\")", command, page);

You should probably also rename this function to "exec_emacsclient" or
something as well now that it doesn't just launch woman.

>  		execlp(path, "emacsclient", "-e", man_page.buf, (char *)NULL);
>  		warning(_("failed to exec '%s': %s"), path, strerror(errno));
>  	}
> @@ -341,8 +344,10 @@ static void exec_viewer(const char *name, const char *page)
>  
>  	if (!strcasecmp(name, "man"))
>  		exec_man_man(info, page);
> +	else if (!strcasecmp(name, "eman"))
> +		exec_woman_emacs(info, page, 1);
>  	else if (!strcasecmp(name, "woman"))
> -		exec_woman_emacs(info, page);
> +		exec_woman_emacs(info, page, 0);
>  	else if (!strcasecmp(name, "konqueror"))
>  		exec_man_konqueror(info, page);
>  	else if (info)
> -- 
> 1.8.3.1.487.g3e7a5b4.dirty