[PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Christian Couder]

This patch implements reading values from "man.<tool>.path"
configuration variables, and using these values as pathes to
the man viewer <tool>s when lauching them.

This makes it possible to use different version of the tools
than the one on the current PATH, or maybe a custom script.

In this patch we also try to launch "konqueror" using
"kfmclient" even if a path to a konqueror binary is given
in "man.konqueror.path".

And we add warnings after "exec" calls in case of exec errors. 

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 help.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 94 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index ecaca77..fd88c22 100644
--- a/help.c
+++ b/help.c
@@ -15,6 +15,12 @@ static struct man_viewer_list {
 	struct man_viewer_list *next;
 } *man_viewer_list;
 
+static struct man_viewer_info_list {
+	struct man_viewer_info_list *next;
+	const char *info;
+	char name[FLEX_ARRAY];
+} *man_viewer_info_list;
+
 enum help_format {
 	HELP_FORMAT_MAN,
 	HELP_FORMAT_INFO,
@@ -48,6 +54,18 @@ static enum help_format parse_help_format(const char *format)
 	die("unrecognized help format '%s'", format);
 }
 
+static const char *get_man_viewer_info(const char *name)
+{
+	struct man_viewer_info_list *viewer;
+
+	for (viewer = man_viewer_info_list; viewer; viewer = viewer->next)
+	{
+		if (!strcasecmp(name, viewer->name))
+			return viewer->info;
+	}
+	return NULL;
+}
+
 static int check_emacsclient_version(void)
 {
 	struct strbuf buffer = STRBUF_INIT;
@@ -99,8 +117,13 @@ static void exec_woman_emacs(const char *page)
 	if (!check_emacsclient_version()) {
 		/* This works only with emacsclient version >= 22. */
 		struct strbuf man_page = STRBUF_INIT;
+		const char *path = get_man_viewer_info("woman");
+
+		if (!path)
+			path = "emacsclient";
 		strbuf_addf(&man_page, "(woman \"%s\")", page);
-		execlp("emacsclient", "emacsclient", "-e", man_page.buf, NULL);
+		execlp(path, "emacsclient", "-e", man_page.buf, NULL);
+		warning("failed to exec '%s': %s", path, strerror(errno));
 	}
 }
 
@@ -109,14 +132,35 @@ static void exec_man_konqueror(const char *page)
 	const char *display = getenv("DISPLAY");
 	if (display && *display) {
 		struct strbuf man_page = STRBUF_INIT;
+		const char *path = get_man_viewer_info("konqueror");
+
+		/* It's simpler to launch konqueror using kfmclient. */
+		if (path) {
+			const char *file = strrchr(path, '/') + 1;
+			if (!strcmp(file, "konqueror")) {
+				char *new = xstrdup(path);
+				char *dest = strrchr(new, '/') + 1;
+
+				/* strlen("konqueror") == strlen("kfmclient") */
+				strcpy(dest, "kfmclient");
+				path = new;
+			}
+		} else
+			path = "kfmclient";
 		strbuf_addf(&man_page, "man:%s(1)", page);
-		execlp("kfmclient", "kfmclient", "newTab", man_page.buf, NULL);
+		execlp(path, "kfmclient", "newTab", man_page.buf, NULL);
+		warning("failed to exec '%s': %s", path, strerror(errno));
 	}
 }
 
 static void exec_man_man(const char *page)
 {
-	execlp("man", "man", page, NULL);
+	const char *path = get_man_viewer_info("man");
+
+	if (!path)
+		path = "man";
+	execlp(path, "man", page, NULL);
+	warning("failed to exec '%s': %s", path, strerror(errno));
 }
 
 static void do_add_man_viewer(void (*exec)(const char *))
@@ -144,6 +188,50 @@ static int add_man_viewer(const char *value)
 	return 0;
 }
 
+static void do_add_man_viewer_info(const char *name,
+				   size_t len,
+				   const char *value)
+{
+	struct man_viewer_info_list *new = xcalloc(1, sizeof(*new) + len + 1);
+
+	strncpy(new->name, name, len);
+	new->info = xstrdup(value);
+	new->next = man_viewer_info_list;
+	man_viewer_info_list = new;
+}
+
+static int add_man_viewer_path(const char *name,
+			       size_t len,
+			       const char *value)
+{
+	if (!strncasecmp("man", name, len) ||
+	    !strncasecmp("woman", name, len) ||
+	    !strncasecmp("konqueror", name, len))
+		do_add_man_viewer_info(name, len, value);
+	else
+		warning("'%s': path for unsupported man viewer.", name);
+
+	return 0;
+}
+
+static int add_man_viewer_info(const char *var, const char *value)
+{
+	const char *name = var + 4;
+	const char *subkey = strrchr(name, '.');
+
+	if (!subkey)
+		return error("Config with no key for man viewer: %s", name);
+
+	if (!strcmp(subkey, ".path")) {
+		if (!value)
+			return config_error_nonbool(var);
+		return add_man_viewer_path(name, subkey - name, value);
+	}
+
+	warning("'%s': unsupported man viewer sub key.", subkey);
+	return 0;
+}
+
 static int git_help_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "help.format")) {
@@ -157,6 +245,9 @@ static int git_help_config(const char *var, const char *value)
 			return config_error_nonbool(var);
 		return add_man_viewer(value);
 	}
+	if (!prefixcmp(var, "man."))
+		return add_man_viewer_info(var, value);
+
 	return git_default_config(var, value);
 }
 
-- 
1.5.4.4.685.g3070a.dirty

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> This makes it possible to use different version of the tools
> than the one on the current PATH, or maybe a custom script.
>
> In this patch we also try to launch "konqueror" using
> "kfmclient" even if a path to a konqueror binary is given
> in "man.konqueror.path".

It may be true that allowing customizable paths may be more useful than
not allowing them, so I do not have fundamental objection to this
enhancement.  However, I doubt this s/konqueror/kfmclient/ is a good idea.

As a general rule, if you allow the user to explicitly say "instead of
what you would normally use, use _this_", you should not try to outsmart
the user by using something else that you derived from that "_this_" the
user gave you.

If the user wants to use kfmclient, then the user can say so.  If the user
wants to really launch konq instead of using kfmclient for whatever
reason, the outsmarting code will interfere and make it impossible.

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Christian Couder]

Le mardi 18 mars 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > This makes it possible to use different version of the tools
> > than the one on the current PATH, or maybe a custom script.
> >
> > In this patch we also try to launch "konqueror" using
> > "kfmclient" even if a path to a konqueror binary is given
> > in "man.konqueror.path".
>
> It may be true that allowing customizable paths may be more useful than
> not allowing them, so I do not have fundamental objection to this
> enhancement.  However, I doubt this s/konqueror/kfmclient/ is a good
> idea.
>
> As a general rule, if you allow the user to explicitly say "instead of
> what you would normally use, use _this_", you should not try to outsmart
> the user by using something else that you derived from that "_this_" the
> user gave you.
>
> If the user wants to use kfmclient, then the user can say so.  

Yes, but if the user just wants to use a konq that is not in the path, then 
the konq specified with "man.konqueror.path" should behave the same as when 
using the konq in the path. That means that we should also try to open a 
new tab on an existing konq, and this will not be the case if we 
use "/path/konqueror URL" instead of "/path/kfmclient newTab URL".

> If the
> user wants to really launch konq instead of using kfmclient for whatever
> reason, the outsmarting code will interfere and make it impossible.

I think it will still be possible using custom commands. I am working on the 
patch. It should be ready in a few days.

Thanks,
Christian.

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Yes, but if the user just wants to use a konq that is not in the path, then 
> the konq specified with "man.konqueror.path" should behave the same as when 
> using the konq in the path. That means that we should also try to open a 
> new tab on an existing konq, and this will not be the case if we 
> use "/path/konqueror URL" instead of "/path/kfmclient newTab URL".

If that inconsistency bothers you, you probably should rename the built-in
konqueror support to "kfmclient", which is more honest approach, I would
think.

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Xavier Maillard]

   As a general rule, if you allow the user to explicitly say "instead of
   what you would normally use, use _this_", you should not try to outsmart
   the user by using something else that you derived from that "_this_" the
   user gave you.

I second that.

	Xavier
-- 
http://www.gnu.org
http://www.april.org
http://www.lolica.org

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Christian Couder]

Le jeudi 20 mars 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Yes, but if the user just wants to use a konq that is not in the path,
> > then the konq specified with "man.konqueror.path" should behave the
> > same as when using the konq in the path. That means that we should also
> > try to open a new tab on an existing konq, and this will not be the
> > case if we use "/path/konqueror URL" instead of "/path/kfmclient newTab
> > URL".
>
> If that inconsistency bothers you, you probably should rename the
> built-in konqueror support to "kfmclient", which is more honest approach,
> I would think.

It's perhaps more honest, but kfmclient is not as well known as konqueror.

Isn't a documentation patch like this enough:

------8<---------

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 4c6cb21..0ece412 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -87,7 +87,8 @@ is chosen. Only the following values are currently 
supported:
 * "man": use the 'man' program as usual,
 * "woman": use 'emacsclient' to launch the "woman" mode in emacs
 (this only works starting with emacsclient versions 22),
-* "konqueror": use a man KIO slave in konqueror.
+* "konqueror": use kfmclient to open the man page in a new konqueror
+tab.

 Multiple values may be given to this configuration variable. Their
 corresponding programs will be tried in the order listed in the

------8<---------

I also wonder if you want some changes in "git-web--browse.sh" as there is 
the same logic ?

Thanks,
Christian.

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Le jeudi 20 mars 2008, Junio C Hamano a écrit :
> ...
>> If that inconsistency bothers you, you probably should rename the
>> built-in konqueror support to "kfmclient", which is more honest approach,
>> I would think.
>
> It's perhaps more honest, but kfmclient is not as well known as konqueror.
>
> Isn't a documentation patch like this enough:

I dunno.  I am not a KDE use to begin with.

But I am somewhat uncomfortable with this kind of magic, and very much
more so with basing the magic on the name of a binary.

For example, if a distro offers two versions of konq to co-exist on the
same system as konqueror-3 and konqueror-4 (with the usual symlink tricks
/etc/alternatives/konqueror -> konqueror-3 and /usr/bin/konqueror ->
/etc/alternatives/konqueror to make one version the systemwide default),
people who want a particular version may say /usr/bin/konqueror-4 and
would get frustrated to see kfmclient magic would not kick in.  By taking
honest route without magic, you would not have to worry about such
potential confusion.

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Xavier Maillard]

   Le jeudi 20 mars 2008, Junio C Hamano a écrit :
   > Christian Couder <chriscool@tuxfamily.org> writes:
   > > Yes, but if the user just wants to use a konq that is not in the path,
   > > then the konq specified with "man.konqueror.path" should behave the
   > > same as when using the konq in the path. That means that we should also
   > > try to open a new tab on an existing konq, and this will not be the
   > > case if we use "/path/konqueror URL" instead of "/path/kfmclient newTab
   > > URL".
   >
   > If that inconsistency bothers you, you probably should rename the
   > built-in konqueror support to "kfmclient", which is more honest approach,
   > I would think.

   It's perhaps more honest, but kfmclient is not as well known as konqueror.

I am not a KDE user at all and it is true that outside from here,
konqueror is a well known program whereas kfmlient is not (by the
way what is exactly kfmclient ?).

   Isn't a documentation patch like this enough:

   ------8<---------

   diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
   index 4c6cb21..0ece412 100644
   --- a/Documentation/git-help.txt
   +++ b/Documentation/git-help.txt
   @@ -87,7 +87,8 @@ is chosen. Only the following values are currently 
   supported:
    * "man": use the 'man' program as usual,
    * "woman": use 'emacsclient' to launch the "woman" mode in emacs
    (this only works starting with emacsclient versions 22),
   -* "konqueror": use a man KIO slave in konqueror.
   +* "konqueror": use kfmclient to open the man page in a new konqueror
   +tab.

I find this clearer at user point of view than the older entry.

Regards,

	Xavier
-- 
http://www.gnu.org
http://www.april.org
http://www.lolica.org

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Christian Couder]

Le vendredi 21 mars 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> >
> > It's perhaps more honest, but kfmclient is not as well known as
> > konqueror.
> >
> > Isn't a documentation patch like this enough:
>
> I dunno.  I am not a KDE use to begin with.
>
> But I am somewhat uncomfortable with this kind of magic, and very much
> more so with basing the magic on the name of a binary.

I think in this case it should be ok if it's properly documented.

It's good to have a consistent user interface with meaningfull names for the 
available options. And we are right to try to do the same thing as when 
browsing an HTML man page.

In the documentation patch for "man.<tool>.cmd" that I will send just after 
this email, there is also enough information to understand what is going on 
and to find a good way around the magic if needed.

I can send the same kind of documentation patch for git-web--browse too, so 
we are completely consistent.

> For example, if a distro offers two versions of konq to co-exist on the
> same system as konqueror-3 and konqueror-4 (with the usual symlink tricks
> /etc/alternatives/konqueror -> konqueror-3 and /usr/bin/konqueror ->
> /etc/alternatives/konqueror to make one version the systemwide default),
> people who want a particular version may say /usr/bin/konqueror-4 and
> would get frustrated to see kfmclient magic would not kick in.  By taking
> honest route without magic, you would not have to worry about such
> potential confusion.

In my experience KDE people have never changed the binary names like that. 
They just use (or advice users to use) different directory names and change 
environment variables (QTDIR, KDEDIR and maybe others) to point to the new 
dirs.

Thanks,
Christian.

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Le vendredi 21 mars 2008, Junio C Hamano a écrit :
> ...
>> For example, if a distro offers two versions of konq to co-exist on the
>> same system as konqueror-3 and konqueror-4 (with the usual symlink tricks
>> /etc/alternatives/konqueror -> konqueror-3 and /usr/bin/konqueror ->
>> /etc/alternatives/konqueror to make one version the systemwide default),
>> people who want a particular version may say /usr/bin/konqueror-4 and
>> would get frustrated to see kfmclient magic would not kick in.  By taking
>> honest route without magic, you would not have to worry about such
>> potential confusion.
>
> In my experience KDE people have never changed the binary names like that. 
> They just use (or advice users to use) different directory names and change 
> environment variables (QTDIR, KDEDIR and maybe others) to point to the new 
> dirs.

I was more worried about what distro people do, not "KDE people".  For
example, Contents-i386.gz file from an unnamed distribution lists a
handful /usr/bin/k*[0-9] files with their counterparts without the
trailing digit.

Re: [PATCH 1/2] help: use man viewer path from "man.<tool>.path" config var

[Christian Couder]

Le mardi 25 mars 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> >
> > In my experience KDE people have never changed the binary names like
> > that. They just use (or advice users to use) different directory names
> > and change environment variables (QTDIR, KDEDIR and maybe others) to
> > point to the new dirs.
>
> I was more worried about what distro people do, not "KDE people".  For
> example, Contents-i386.gz file from an unnamed distribution lists a
> handful /usr/bin/k*[0-9] files with their counterparts without the
> trailing digit.

You are right. This is probably because of the current switch from KDE3 to 
KDE4. But as I could find no "konqueror4", we are probably safe for at 
least a few years.

By the way I just sent a documentation patch to add a "Note about konqueror" 
in "Documentation/git-web--browse.txt", to be consistent with the note 
in "Documentation/git-help.txt".

Thanks,
Christian.