Re: [PATCH] help: implement multi-valued "man.viewer" config option

   Junio suggested:
   > How about allowing multi-valued man.viewer like this:
   >
   >        [man]
   >                viewer = woman
   >                viewer = konqueror
   >                viewer = man
   >
   > and have:
   >
   >        static struct man_viewer {
   >                char *name;
   >                void (*exec)(const char *);
   >        } viewers[] = {
   >                { "woman", exec_woman },
   >                { "konqueror", exec_konqueror },
   >                { "man", exec_man },
   >                { NULL, },
   >        };
   >
   > Then you can iterate the man.viewer values, ask the viewer's
   > exec() function to show the page (or return when it is not
   > in an environment that it can be useful).
   >
   > show_man_page() would become:
   >
   >        for (each viewer in user's config)
   >                viewer.exec(page); /* will return when unable */
   >        die("no man viewer handled the request");

   This patch implements the above using a list of exec functions that
   is filled when reading the config.

   To do that the exec functions have been moved before reading the
   config. This makes the patch much longer than it would be otherwise.

   Signed-off-by: Christian Couder [off-list ref]

Tested-by: Xavier Maillard <redacted>

Though, I thought that when one entry had failed we would have
switched to the next until none could be found thus 

I (voluntary) made a typo in my .git/config file as reflected by:

[xma@localhost 23:57:18 git]$ git config --get-all man.viewer
woma  <- TYPO HERE
konqueror
man

and I then tried git config --help. I thought it would have tried
all entries and as a last resort would have failed back to man
but it did not act like this:

[xma@localhost 23:57:11 git]$ git config --help
error: 'woma': unsupported man viewer.
fatal: bad config file line 16 in .git/config

Ok, woma in not supported here and it is reported like this but
would it be possible to just throw an error on stdout and try
another viewer ? We could even imagine something even more
general like the possibility for the user to write his own man
viewer (a bash script for example) and set it as a candidate.

By the way, I do not see any reason to put man as a candidate.
"man" should be the default when nothing is specified or when all
candidates have failed.

Anyway, thank you for this implementation.

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

Le mercredi 12 mars 2008, Xavier Maillard a écrit :

Tested-by: Xavier Maillard <redacted>

Thanks.

Though, I thought that when one entry had failed we would have
switched to the next until none could be found thus

I (voluntary) made a typo in my .git/config file as reflected by:

[xma@localhost 23:57:18 git]$ git config --get-all man.viewer
woma  <- TYPO HERE
konqueror
man

and I then tried git config --help. I thought it would have tried
all entries and as a last resort would have failed back to man
but it did not act like this:

[xma@localhost 23:57:11 git]$ git config --help
error: 'woma': unsupported man viewer.
fatal: bad config file line 16 in .git/config

Ok, woma in not supported here and it is reported like this but
would it be possible to just throw an error on stdout and try
another viewer ? 

Yes, with the following patch on top:

diff --git a/help.c b/help.c
index 5da8c9c..ecaca77 100644
--- a/help.c
+++ b/help.c

@@ -139,7 +139,7 @@ static int add_man_viewer(const char *value)
        else if (!strcasecmp(value, "konqueror"))
                do_add_man_viewer(exec_man_konqueror);
        else
-               return error("'%s': unsupported man viewer.", value);
+               warning("'%s': unsupported man viewer.", value);

        return 0;
 }

We could even imagine something even more 
general like the possibility for the user to write his own man
viewer (a bash script for example) and set it as a candidate.

I will do that in a latter patch, it has been suggested a lot of times 
already.

By the way, I do not see any reason to put man as a candidate.
"man" should be the default when nothing is specified or when all
candidates have failed.

It may be more explicit.

Thanks,
Christian.

Anyway, thank you for this implementation.

	Xavier