{"thread":{"id":"12633","subject":"[PATCH] help: implement multi-valued \"man.viewer\" config option","startedAt":"2008-03-11T07:51:12Z","lastAt":"2008-03-15T13:00:27Z","messageCount":6,"participants":["Christian Couder","Xavier Maillard"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"71664","messageId":"20080311085113.176df1af.chriscool@tuxfamily.org","threadId":"12633","inReplyTo":null,"subject":"[PATCH] help: implement multi-valued \"man.viewer\" config option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-03-11T07:51:12Z","receivedAt":"2008-03-11T07:51:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Junio suggested:\n> How about allowing multi-valued man.viewer like this:\n>\n>        [man]\n>                viewer = woman\n>                viewer = konqueror\n>                viewer = man\n>\n> and have:\n>\n>        static struct man_viewer {\n>                char *name;\n>                void (*exec)(const char *);\n>        } viewers[] = {\n>                { \"woman\", exec_woman },\n>                { \"konqueror\", exec_konqueror },\n>                { \"man\", exec_man },\n>                { NULL, },\n>        };\n>\n> Then you can iterate the man.viewer values, ask the viewer's\n> exec() function to show the page (or return when it is not\n> in an environment that it can be useful).\n>\n> show_man_page() would become:\n>\n>        for (each viewer in user's config)\n>                viewer.exec(page); /* will return when unable */\n>        die(\"no man viewer handled the request\");\n\nThis patch implements the above using a list of exec functions that\nis filled when reading the config.\n\nTo do that the exec functions have been moved before reading the\nconfig. This makes the patch much longer than it would be otherwise.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n help.c |  191 ++++++++++++++++++++++++++++++++++++++--------------------------\n 1 files changed, 113 insertions(+), 78 deletions(-)\n\n\tThis is on top of my previous \"man.viewer\" patches:\n\n\t[PATCH 1/2] help: add \"man.viewer\" config var to use \"woman\" or \"konqueror\"\n\t[PATCH 2/2] Documentation: help: describe 'man.viewer' config variable\n\ndiff --git a/help.c b/help.c\nindex 2cb152d..5da8c9c 100644\n--- a/help.c\n+++ b/help.c\n@@ -10,7 +10,10 @@\n #include \"parse-options.h\"\n #include \"run-command.h\"\n \n-static const char *man_viewer;\n+static struct man_viewer_list {\n+\tvoid (*exec)(const char *);\n+\tstruct man_viewer_list *next;\n+} *man_viewer_list;\n \n enum help_format {\n \tHELP_FORMAT_MAN,\n@@ -45,6 +48,102 @@ static enum help_format parse_help_format(const char *format)\n \tdie(\"unrecognized help format '%s'\", format);\n }\n \n+static int check_emacsclient_version(void)\n+{\n+\tstruct strbuf buffer = STRBUF_INIT;\n+\tstruct child_process ec_process;\n+\tconst char *argv_ec[] = { \"emacsclient\", \"--version\", NULL };\n+\tint version;\n+\n+\t/* emacsclient prints its version number on stderr */\n+\tmemset(&ec_process, 0, sizeof(ec_process));\n+\tec_process.argv = argv_ec;\n+\tec_process.err = -1;\n+\tec_process.stdout_to_stderr = 1;\n+\tif (start_command(&ec_process)) {\n+\t\tfprintf(stderr, \"Failed to start emacsclient.\\n\");\n+\t\treturn -1;\n+\t}\n+\tstrbuf_read(&buffer, ec_process.err, 20);\n+\tclose(ec_process.err);\n+\n+\t/*\n+\t * Don't bother checking return value, because \"emacsclient --version\"\n+\t * seems to always exits with code 1.\n+\t */\n+\tfinish_command(&ec_process);\n+\n+\tif (prefixcmp(buffer.buf, \"emacsclient\")) {\n+\t\tfprintf(stderr, \"Failed to parse emacsclient version.\\n\");\n+\t\tstrbuf_release(&buffer);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_remove(&buffer, 0, strlen(\"emacsclient\"));\n+\tversion = atoi(buffer.buf);\n+\n+\tif (version < 22) {\n+\t\tfprintf(stderr,\n+\t\t\t\"emacsclient version '%d' too old (< 22).\\n\",\n+\t\t\tversion);\n+\t\tstrbuf_release(&buffer);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_release(&buffer);\n+\treturn 0;\n+}\n+\n+static void exec_woman_emacs(const char *page)\n+{\n+\tif (!check_emacsclient_version()) {\n+\t\t/* This works only with emacsclient version >= 22. */\n+\t\tstruct strbuf man_page = STRBUF_INIT;\n+\t\tstrbuf_addf(&man_page, \"(woman \\\"%s\\\")\", page);\n+\t\texeclp(\"emacsclient\", \"emacsclient\", \"-e\", man_page.buf, NULL);\n+\t}\n+}\n+\n+static void exec_man_konqueror(const char *page)\n+{\n+\tconst char *display = getenv(\"DISPLAY\");\n+\tif (display && *display) {\n+\t\tstruct strbuf man_page = STRBUF_INIT;\n+\t\tstrbuf_addf(&man_page, \"man:%s(1)\", page);\n+\t\texeclp(\"kfmclient\", \"kfmclient\", \"newTab\", man_page.buf, NULL);\n+\t}\n+}\n+\n+static void exec_man_man(const char *page)\n+{\n+\texeclp(\"man\", \"man\", page, NULL);\n+}\n+\n+static void do_add_man_viewer(void (*exec)(const char *))\n+{\n+\tstruct man_viewer_list **p = &man_viewer_list;\n+\n+\twhile (*p)\n+\t\tp = &((*p)->next);\n+\t*p = xmalloc(sizeof(**p));\n+\t(*p)->next = NULL;\n+\t(*p)->exec = exec;\n+}\n+\n+static int add_man_viewer(const char *value)\n+{\n+\tif (!strcasecmp(value, \"man\"))\n+\t\tdo_add_man_viewer(exec_man_man);\n+\telse if (!strcasecmp(value, \"woman\"))\n+\t\tdo_add_man_viewer(exec_woman_emacs);\n+\telse if (!strcasecmp(value, \"konqueror\"))\n+\t\tdo_add_man_viewer(exec_man_konqueror);\n+\telse\n+\t\treturn error(\"'%s': unsupported man viewer.\", value);\n+\n+\treturn 0;\n+}\n+\n static int git_help_config(const char *var, const char *value)\n {\n \tif (!strcmp(var, \"help.format\")) {\n@@ -53,8 +152,11 @@ static int git_help_config(const char *var, const char *value)\n \t\thelp_format = parse_help_format(value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(var, \"man.viewer\"))\n-\t\treturn git_config_string(&man_viewer, var, value);\n+\tif (!strcmp(var, \"man.viewer\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\treturn add_man_viewer(value);\n+\t}\n \treturn git_default_config(var, value);\n }\n \n@@ -350,85 +452,18 @@ static void setup_man_path(void)\n \tstrbuf_release(&new_path);\n }\n \n-static int check_emacsclient_version(void)\n-{\n-\tstruct strbuf buffer = STRBUF_INIT;\n-\tstruct child_process ec_process;\n-\tconst char *argv_ec[] = { \"emacsclient\", \"--version\", NULL };\n-\tint version;\n-\n-\t/* emacsclient prints its version number on stderr */\n-\tmemset(&ec_process, 0, sizeof(ec_process));\n-\tec_process.argv = argv_ec;\n-\tec_process.err = -1;\n-\tec_process.stdout_to_stderr = 1;\n-\tif (start_command(&ec_process)) {\n-\t\tfprintf(stderr, \"Failed to start emacsclient.\\n\");\n-\t\treturn -1;\n-\t}\n-\tstrbuf_read(&buffer, ec_process.err, 20);\n-\tclose(ec_process.err);\n-\n-\t/*\n-\t * Don't bother checking return value, because \"emacsclient --version\"\n-\t * seems to always exits with code 1.\n-\t */\n-\tfinish_command(&ec_process);\n-\n-\tif (prefixcmp(buffer.buf, \"emacsclient\")) {\n-\t\tfprintf(stderr, \"Failed to parse emacsclient version.\\n\");\n-\t\tstrbuf_release(&buffer);\n-\t\treturn -1;\n-\t}\n-\n-\tstrbuf_remove(&buffer, 0, strlen(\"emacsclient\"));\n-\tversion = atoi(buffer.buf);\n-\n-\tif (version < 22) {\n-\t\tfprintf(stderr,\n-\t\t\t\"emacsclient version '%d' too old (< 22).\\n\",\n-\t\t\tversion);\n-\t\tstrbuf_release(&buffer);\n-\t\treturn -1;\n-\t}\n-\n-\tstrbuf_release(&buffer);\n-\treturn 0;\n-}\n-\n-static void exec_woman_emacs(const char *page)\n-{\n-\tif (!check_emacsclient_version()) {\n-\t\t/* This works only with emacsclient version >= 22. */\n-\t\tstruct strbuf man_page = STRBUF_INIT;\n-\t\tstrbuf_addf(&man_page, \"(woman \\\"%s\\\")\", page);\n-\t\texeclp(\"emacsclient\", \"emacsclient\", \"-e\", man_page.buf, NULL);\n-\t} else\n-\t\texeclp(\"man\", \"man\", page, NULL);\n-}\n-\n-static void exec_man_konqueror(const char *page)\n-{\n-\tconst char *display = getenv(\"DISPLAY\");\n-\tif (display && *display) {\n-\t\tstruct strbuf man_page = STRBUF_INIT;\n-\t\tstrbuf_addf(&man_page, \"man:%s(1)\", page);\n-\t\texeclp(\"kfmclient\", \"kfmclient\", \"newTab\", man_page.buf, NULL);\n-\t} else\n-\t\texeclp(\"man\", \"man\", page, NULL);\n-}\n-\n static void show_man_page(const char *git_cmd)\n {\n+\tstruct man_viewer_list *viewer;\n \tconst char *page = cmd_to_page(git_cmd);\n+\n \tsetup_man_path();\n-\tif (!man_viewer || !strcmp(man_viewer, \"man\"))\n-\t\texeclp(\"man\", \"man\", page, NULL);\n-\tif (!strcmp(man_viewer, \"woman\"))\n-\t\texec_woman_emacs(page);\n-\tif (!strcmp(man_viewer, \"konqueror\"))\n-\t\texec_man_konqueror(page);\n-\tdie(\"'%s': unsupported man viewer.\", man_viewer);\n+\tfor (viewer = man_viewer_list; viewer; viewer = viewer->next)\n+\t{\n+\t\tviewer->exec(page); /* will return when unable */\n+\t}\n+\texec_man_man(page);\n+\tdie(\"no man viewer handled the request\");\n }\n \n static void show_info_page(const char *git_cmd)\n-- \n1.5.4.4.595.g9c65\n"},{"id":"71759","messageId":"200803120100.m2C105YM010496@localhost.localdomain","threadId":"12633","inReplyTo":"20080311085113.176df1af.chriscool@tuxfamily.org","subject":"Re: [PATCH] help: implement multi-valued \"man.viewer\" config option","fromName":"Xavier Maillard","fromEmail":"xma@gnu.org","sentAt":"2008-03-12T01:00:06Z","receivedAt":"2008-03-12T01:00:06Z","isPatch":true,"sender":{"key":"xma@gnu.org","avatar":null},"body":"\n   Junio suggested:\n   > How about allowing multi-valued man.viewer like this:\n   >\n   >        [man]\n   >                viewer = woman\n   >                viewer = konqueror\n   >                viewer = man\n   >\n   > and have:\n   >\n   >        static struct man_viewer {\n   >                char *name;\n   >                void (*exec)(const char *);\n   >        } viewers[] = {\n   >                { \"woman\", exec_woman },\n   >                { \"konqueror\", exec_konqueror },\n   >                { \"man\", exec_man },\n   >                { NULL, },\n   >        };\n   >\n   > Then you can iterate the man.viewer values, ask the viewer's\n   > exec() function to show the page (or return when it is not\n   > in an environment that it can be useful).\n   >\n   > show_man_page() would become:\n   >\n   >        for (each viewer in user's config)\n   >                viewer.exec(page); /* will return when unable */\n   >        die(\"no man viewer handled the request\");\n\n   This patch implements the above using a list of exec functions that\n   is filled when reading the config.\n\n   To do that the exec functions have been moved before reading the\n   config. This makes the patch much longer than it would be otherwise.\n\n   Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n\nTested-by: Xavier Maillard <xma@gnu.org>\n\nThough, I thought that when one entry had failed we would have\nswitched to the next until none could be found thus \n\nI (voluntary) made a typo in my .git/config file as reflected by:\n\n[xma@localhost 23:57:18 git]$ git config --get-all man.viewer\nwoma  <- TYPO HERE\nkonqueror\nman\n\nand I then tried git config --help. I thought it would have tried\nall entries and as a last resort would have failed back to man\nbut it did not act like this:\n\n[xma@localhost 23:57:11 git]$ git config --help\nerror: 'woma': unsupported man viewer.\nfatal: bad config file line 16 in .git/config\n\nOk, woma in not supported here and it is reported like this but\nwould it be possible to just throw an error on stdout and try\nanother viewer ? We could even imagine something even more\ngeneral like the possibility for the user to write his own man\nviewer (a bash script for example) and set it as a candidate.\n\nBy the way, I do not see any reason to put man as a candidate.\n\"man\" should be the default when nothing is specified or when all\ncandidates have failed.\n\nAnyway, thank you for this implementation.\n\n\tXavier\n-- \nhttp://www.gnu.org\nhttp://www.april.org\nhttp://www.lolica.org\n"},{"id":"71789","messageId":"200803120823.38100.chriscool@tuxfamily.org","threadId":"12633","inReplyTo":"200803120100.m2C105YM010496@localhost.localdomain","subject":"Re: [PATCH] help: implement multi-valued \"man.viewer\" config option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-03-12T07:23:37Z","receivedAt":"2008-03-12T07:23:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le mercredi 12 mars 2008, Xavier Maillard a écrit :\n>\n> Tested-by: Xavier Maillard <xma@gnu.org>\n\nThanks.\n\n> Though, I thought that when one entry had failed we would have\n> switched to the next until none could be found thus\n>\n> I (voluntary) made a typo in my .git/config file as reflected by:\n>\n> [xma@localhost 23:57:18 git]$ git config --get-all man.viewer\n> woma  <- TYPO HERE\n> konqueror\n> man\n>\n> and I then tried git config --help. I thought it would have tried\n> all entries and as a last resort would have failed back to man\n> but it did not act like this:\n>\n> [xma@localhost 23:57:11 git]$ git config --help\n> error: 'woma': unsupported man viewer.\n> fatal: bad config file line 16 in .git/config\n>\n> Ok, woma in not supported here and it is reported like this but\n> would it be possible to just throw an error on stdout and try\n> another viewer ? \n\nYes, with the following patch on top:\n\ndiff --git a/help.c b/help.c\nindex 5da8c9c..ecaca77 100644\n--- a/help.c\n+++ b/help.c\n@@ -139,7 +139,7 @@ static int add_man_viewer(const char *value)\n        else if (!strcasecmp(value, \"konqueror\"))\n                do_add_man_viewer(exec_man_konqueror);\n        else\n-               return error(\"'%s': unsupported man viewer.\", value);\n+               warning(\"'%s': unsupported man viewer.\", value);\n\n        return 0;\n }\n\n> We could even imagine something even more \n> general like the possibility for the user to write his own man\n> viewer (a bash script for example) and set it as a candidate.\n\nI will do that in a latter patch, it has been suggested a lot of times \nalready.\n\n> By the way, I do not see any reason to put man as a candidate.\n> \"man\" should be the default when nothing is specified or when all\n> candidates have failed.\n\nIt may be more explicit.\n\nThanks,\nChristian.\n\n> Anyway, thank you for this implementation.\n>\n> \tXavier\n"},{"id":"72019","messageId":"200803140100.m2E105o5004664@localhost.localdomain","threadId":"12633","inReplyTo":"200803120823.38100.chriscool@tuxfamily.org","subject":"Re: [PATCH] help: implement multi-valued \"man.viewer\" config option","fromName":"Xavier Maillard","fromEmail":"xma@gnu.org","sentAt":"2008-03-14T01:00:05Z","receivedAt":"2008-03-14T01:00:05Z","isPatch":true,"sender":{"key":"xma@gnu.org","avatar":null},"body":"\n   > Ok, woma in not supported here and it is reported like this but\n   > would it be possible to just throw an error on stdout and try\n   > another viewer ? \n\n   Yes, with the following patch on top:\n\nSee my \"tested-by\" message.\n\n   > We could even imagine something even more \n   > general like the possibility for the user to write his own man\n   > viewer (a bash script for example) and set it as a candidate.\n\n   I will do that in a latter patch, it has been suggested a lot of times \n   already.\n\nGlad to read that !\n\n   > By the way, I do not see any reason to put man as a candidate.\n   > \"man\" should be the default when nothing is specified or when all\n   > candidates have failed.\n\n   It may be more explicit.\n\nWell, I do not buy this argument and I am pretty sure that a\nsimple note into the manual would suffice but, that's me :)\n\n\tXavier\n-- \nhttp://www.gnu.org\nhttp://www.april.org\nhttp://www.lolica.org\n"},{"id":"72038","messageId":"200803140626.16075.chriscool@tuxfamily.org","threadId":"12633","inReplyTo":"200803140100.m2E105o5004664@localhost.localdomain","subject":"Re: [PATCH] help: implement multi-valued \"man.viewer\" config option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-03-14T05:26:15Z","receivedAt":"2008-03-14T05:26:15Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 14 mars 2008, Xavier Maillard a écrit :\n>    > Ok, woma in not supported here and it is reported like this but\n>    > would it be possible to just throw an error on stdout and try\n>    > another viewer ?\n>\n>    Yes, with the following patch on top:\n>\n> See my \"tested-by\" message.\n\nThank you Xavier for this message.\n\n>    > We could even imagine something even more\n>    > general like the possibility for the user to write his own man\n>    > viewer (a bash script for example) and set it as a candidate.\n>\n>    I will do that in a latter patch, it has been suggested a lot of times\n>    already.\n>\n> Glad to read that !\n\nI just sent a patch to do that in \"git-web--browse.sh\" and I will soon work \non the same stuff for man viewing.\n\n>    > By the way, I do not see any reason to put man as a candidate.\n>    > \"man\" should be the default when nothing is specified or when all\n>    > candidates have failed.\n>\n>    It may be more explicit.\n>\n> Well, I do not buy this argument and I am pretty sure that a\n> simple note into the manual would suffice but, that's me :)\n\nAs my patch is now on next and as I am not sure to understand exactly what \nyou want, I can only suggest to send a patch if you really care.\n\nThanks,\nChristian.\n"},{"id":"72184","messageId":"200803151300.m2FD0RHF003225@localhost.localdomain","threadId":"12633","inReplyTo":"200803140626.16075.chriscool@tuxfamily.org","subject":"Re: [PATCH] help: implement multi-valued \"man.viewer\" config option","fromName":"Xavier Maillard","fromEmail":"xma@gnu.org","sentAt":"2008-03-15T13:00:27Z","receivedAt":"2008-03-15T13:00:27Z","isPatch":true,"sender":{"key":"xma@gnu.org","avatar":null},"body":"\n   >    I will do that in a latter patch, it has been suggested a lot of times\n   >    already.\n   >\n   > Glad to read that !\n\n   I just sent a patch to do that in \"git-web--browse.sh\" and I will soon work \n   on the same stuff for man viewing.\n\nSeen it and I am about to test it ;)\n\n   > Well, I do not buy this argument and I am pretty sure that a\n   > simple note into the manual would suffice but, that's me :)\n\n   As my patch is now on next and as I am not sure to understand exactly what \n   you want, I can only suggest to send a patch if you really care.\n\nI could but I won't: no time and most importantly, who really\ncare except me ? :) It is okay for me as is (I can certainly live\nwith that).\n\nRegards\n\n\tXavier\n-- \nhttp://www.gnu.org\nhttp://www.april.org\nhttp://www.lolica.org\n"}]}