{"thread":{"id":"11828","subject":"[PATCH 1/4] help: make 'git-help--browse' usable outside 'git-help'.","startedAt":"2008-02-02T06:32:33Z","lastAt":"2008-02-03T05:00:29Z","messageCount":3,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"67083","messageId":"20080202073233.7a656fa8.chriscool@tuxfamily.org","threadId":"11828","inReplyTo":null,"subject":"[PATCH 1/4] help: make 'git-help--browse' usable outside 'git-help'.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-02T06:32:33Z","receivedAt":"2008-02-02T06:32:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"By moving some help specific stuff from 'git-help--browse.sh'\ninto 'help.c', we make it possible to use 'git-help--browse'\noutside 'git-help'.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Makefile            |    2 +-\n git-help--browse.sh |   24 +++++++++---------------\n help.c              |   18 +++++++++++++++++-\n 3 files changed, 27 insertions(+), 17 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 92341c4..a03fd2e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -819,6 +819,7 @@ git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)\n \n help.o: help.c common-cmds.h GIT-CFLAGS\n \t$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) \\\n+\t\t'-DGIT_HTML_PATH=\"$(htmldir_SQ)\"' \\\n \t\t'-DGIT_MAN_PATH=\"$(mandir_SQ)\"' \\\n \t\t'-DGIT_INFO_PATH=\"$(infodir_SQ)\"' $<\n \n@@ -839,7 +840,6 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh\n \t    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n \t    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n \t    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \\\n-\t    -e 's|@@HTMLDIR@@|$(htmldir_SQ)|g' \\\n \t    $@.sh >$@+ && \\\n \tchmod +x $@+ && \\\n \tmv $@+ $@\ndiff --git a/git-help--browse.sh b/git-help--browse.sh\nindex 10b0a36..8189c08 100755\n--- a/git-help--browse.sh\n+++ b/git-help--browse.sh\n@@ -16,18 +16,13 @@\n # git maintainer.\n #\n \n-USAGE='[--browser=browser|--tool=browser] [cmd to display] ...'\n+USAGE='[--browser=browser|--tool=browser] url/file ...'\n \n # This must be capable of running outside of git directory, so\n # the vanilla git-sh-setup should not be used.\n NONGIT_OK=Yes\n . git-sh-setup\n \n-# Install data.\n-html_dir=\"@@HTMLDIR@@\"\n-\n-test -f \"$html_dir/git.html\" || die \"No documentation directory found.\"\n-\n valid_tool() {\n \tcase \"$1\" in\n \t\tfirefox | iceweasel | konqueror | w3m | links | lynx | dillo)\n@@ -71,6 +66,8 @@ do\n     shift\n done\n \n+test $# = 0 && usage\n+\n if test -z \"$browser\"\n then\n     for opt in \"help.browser\" \"web.browser\"\n@@ -113,16 +110,13 @@ else\n     fi\n fi\n \n-pages=$(for p in \"$@\"; do echo \"$html_dir/$p.html\" ; done)\n-test -z \"$pages\" && pages=\"$html_dir/git.html\"\n-\n case \"$browser\" in\n     firefox|iceweasel)\n \t# Check version because firefox < 2.0 does not support \"-new-tab\".\n \tvers=$(expr \"$($browser_path -version)\" : '.* \\([0-9][0-9]*\\)\\..*')\n \tNEWTAB='-new-tab'\n \ttest \"$vers\" -lt 2 && NEWTAB=''\n-\tnohup \"$browser_path\" $NEWTAB $pages &\n+\tnohup \"$browser_path\" $NEWTAB \"$@\" &\n \t;;\n     konqueror)\n \tcase \"$(basename \"$browser_path\")\" in\n@@ -130,20 +124,20 @@ case \"$browser\" in\n \t\t# It's simpler to use kfmclient to open a new tab in konqueror.\n \t\tbrowser_path=\"$(echo \"$browser_path\" | sed -e 's/konqueror$/kfmclient/')\"\n \t\ttype \"$browser_path\" > /dev/null 2>&1 || die \"No '$browser_path' found.\"\n-\t\teval \"$browser_path\" newTab $pages\n+\t\teval \"$browser_path\" newTab \"$@\"\n \t\t;;\n \t    kfmclient)\n-\t\teval \"$browser_path\" newTab $pages\n+\t\teval \"$browser_path\" newTab \"$@\"\n \t\t;;\n \t    *)\n-\t        nohup \"$browser_path\" $pages &\n+\t        nohup \"$browser_path\" \"$@\" &\n \t\t;;\n \tesac\n \t;;\n     w3m|links|lynx)\n-\teval \"$browser_path\" $pages\n+\teval \"$browser_path\" \"$@\"\n \t;;\n     dillo)\n-\tnohup \"$browser_path\" $pages &\n+\tnohup \"$browser_path\" \"$@\" &\n \t;;\n esac\ndiff --git a/help.c b/help.c\nindex 1302a61..b929899 100644\n--- a/help.c\n+++ b/help.c\n@@ -328,10 +328,26 @@ static void show_info_page(const char *git_cmd)\n \texeclp(\"info\", \"info\", \"gitman\", page, NULL);\n }\n \n+static void get_html_page_path(struct strbuf *page_path, const char *page)\n+{\n+\tstruct stat st;\n+\n+\t/* Check that we have a git documentation directory. */\n+\tif (stat(GIT_HTML_PATH \"/git.html\", &st) || !S_ISREG(st.st_mode))\n+\t\tdie(\"'%s': not a documentation directory.\", GIT_HTML_PATH);\n+\n+\tstrbuf_init(page_path, 0);\n+\tstrbuf_addf(page_path, GIT_HTML_PATH \"/%s.html\", page);\n+}\n+\n static void show_html_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n-\texecl_git_cmd(\"help--browse\", page, NULL);\n+\tstruct strbuf page_path; /* it leaks but we exec bellow */\n+\n+\tget_html_page_path(&page_path, page);\n+\n+\texecl_git_cmd(\"help--browse\", page_path.buf, NULL);\n }\n \n void help_unknown_cmd(const char *cmd)\n-- \n1.5.4.rc5.25.g7a831-dirty\n"},{"id":"67086","messageId":"7vmyqj4xxw.fsf@gitster.siamese.dyndns.org","threadId":"11828","inReplyTo":"20080202073233.7a656fa8.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/4] help: make 'git-help--browse' usable outside 'git-help'.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-02T07:30:51Z","receivedAt":"2008-02-02T07:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> By moving some help specific stuff from 'git-help--browse.sh'\n> into 'help.c', we make it possible to use 'git-help--browse'\n> outside 'git-help'.\n\nI was initially puzzled why Eric was CC'ed while reading this\nfirst patch in the 4-patch series.  It would have been nicer to\nstart the series by mentioning the ultimate goal of the series\nupfront.  You are not making it usable just by anybody, but have\na specific goal of sharing the mechanism to launch user's web\nbrowser for both help and instaweb.\n\n> @@ -71,6 +66,8 @@ do\n>      shift\n>  done\n>  \n> +test $# = 0 && usage\n> +\n>  if test -z \"$browser\"\n>  then\n>      for opt in \"help.browser\" \"web.browser\"\n> @@ -113,16 +110,13 @@ else\n>      fi\n>  fi\n>  \n> -pages=$(for p in \"$@\"; do echo \"$html_dir/$p.html\" ; done)\n> -test -z \"$pages\" && pages=\"$html_dir/git.html\"\n> -\n\nWhen the helper was run without specifying any page, it used to\nshow the toplevel \"git\" documentation.  If your eventual goal is\nto allow general browsing, obviously you do not want such a\nlogic here, and it needs to migrate to the caller.  So far, it\nmakes perfect sense. Let's read on.\n\n> diff --git a/help.c b/help.c\n> index 1302a61..b929899 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -328,10 +328,26 @@ static void show_info_page(const char *git_cmd)\n>  \texeclp(\"info\", \"info\", \"gitman\", page, NULL);\n>  }\n>  \n> +static void get_html_page_path(struct strbuf *page_path, const char *page)\n> +{\n> +\tstruct stat st;\n> +\n> +\t/* Check that we have a git documentation directory. */\n> +\tif (stat(GIT_HTML_PATH \"/git.html\", &st) || !S_ISREG(st.st_mode))\n> +\t\tdie(\"'%s': not a documentation directory.\", GIT_HTML_PATH);\n> +\n> +\tstrbuf_init(page_path, 0);\n> +\tstrbuf_addf(page_path, GIT_HTML_PATH \"/%s.html\", page);\n> +}\n\nGets a \"page\", makes page_path by concatenating the manual page\nlocation.  Looks Ok.\n\n>  static void show_html_page(const char *git_cmd)\n>  {\n>  \tconst char *page = cmd_to_page(git_cmd);\n> -\texecl_git_cmd(\"help--browse\", page, NULL);\n\nSo this function used to give whatever \"page\" it got back from\ncmd_to_page().  Maybe it could have been NULL but that would\nhave been handled by the browser helper just fine.\n\nA reviewer would be left wondering if this means that you lost\nthe fallback to the git top page.\n\n> +\tstruct strbuf page_path; /* it leaks but we exec bellow */\n> +\n> +\tget_html_page_path(&page_path, page);\n> +\n> +\texecl_git_cmd(\"help--browse\", page_path.buf, NULL);\n>  }\n\nAnd this part makes the reviewer even more worried. If page\ncould be NULL, then get_html_page_path() would be fed a NULL\npointer, which is given to strbuf_addf()!  Ugh.\n\nThen the reviewer would find out that cmd_to_page() would never\nreturn NULL, as it has its own NULL-to-\"git\" fallback logic.\n\nI think the code is good, but the proposed commit log message\nhas some room for improvements.  \n\nSomething like...\n\n    [PATCH 1/4] help: make 'git-help--browse' usable outside 'git-help'\n\n    \"git-help--browse\" helper is to launch a browser of the\n    user's choice to view the HTML version of git documentation\n    for a given command.  It used to take the name of a command,\n    convert it to the path of the documentation by prefixing the\n    directory name and appending the \".html\" suffix, and start\n    the browser on the path.\n\n    This updates the division of labor between the caller in\n    help.c and git-help--browser helper.  The helper is now\n    responsible for launching a browser of the user's choice\n    on given URLs, and it is the caller's responsibility to\n    tell it the paths to documentation files.\n\n    This is in preparation to reuse the logic to choose\n    user's preferred browser in instaweb.\n\n    The helper had a provision for running it without any\n    command name, in which case it showed the toplevel \"git(7)\"\n    documentation, but the caller in help.c never makes such a\n    call.  The helper now exits with a usage message when no\n    path is given.\n\n    Signed-off-by: ...\n    ---\n\n     * Eric is CC'ed because the ultimate goal of this\n       series is to get rid of the duplicated logic between\n       help--browse and instaweb.\n\n     Makefile            |    2 +-\n     git-help--browse.sh |   24 +++++++++---------------\n    ...\n\nI have given only a cursory look at the remainder of the series\n(I'll hopefully be in a mini vacation mode after the release),\nbut I think overall the series makes sense.\n\nThanks.\n"},{"id":"67183","messageId":"200802030600.30074.chriscool@tuxfamily.org","threadId":"11828","inReplyTo":"7vmyqj4xxw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/4] help: make 'git-help--browse' usable outside 'git-help'.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-03T05:00:29Z","receivedAt":"2008-02-03T05:00:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le samedi 2 février 2008, Junio C Hamano a écrit :\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> > +\tstruct strbuf page_path; /* it leaks but we exec bellow */\n> > +\n> > +\tget_html_page_path(&page_path, page);\n> > +\n> > +\texecl_git_cmd(\"help--browse\", page_path.buf, NULL);\n> >  }\n>\n> And this part makes the reviewer even more worried. If page\n> could be NULL, then get_html_page_path() would be fed a NULL\n> pointer, which is given to strbuf_addf()!  Ugh.\n>\n> Then the reviewer would find out that cmd_to_page() would never\n> return NULL, as it has its own NULL-to-\"git\" fallback logic.\n>\n> I think the code is good, but the proposed commit log message\n> has some room for improvements.\n\nYeah, I could have explained some parts of the patch more.\nI will try to do better.\n\n> Something like...\n>\n>     [PATCH 1/4] help: make 'git-help--browse' usable outside 'git-help'\n>\n>     \"git-help--browse\" helper is to launch a browser of the\n>     user's choice to view the HTML version of git documentation\n>     for a given command.  It used to take the name of a command,\n>     convert it to the path of the documentation by prefixing the\n>     directory name and appending the \".html\" suffix, and start\n>     the browser on the path.\n>\n>     This updates the division of labor between the caller in\n>     help.c and git-help--browser helper.  The helper is now\n>     responsible for launching a browser of the user's choice\n>     on given URLs, and it is the caller's responsibility to\n>     tell it the paths to documentation files.\n>\n>     This is in preparation to reuse the logic to choose\n>     user's preferred browser in instaweb.\n>\n>     The helper had a provision for running it without any\n>     command name, in which case it showed the toplevel \"git(7)\"\n>     documentation, but the caller in help.c never makes such a\n>     call.  The helper now exits with a usage message when no\n>     path is given.\n\nGreat commit message indeed! Though I fear that such long messages (for a \nnot very long patch) could take precious screen real estate when using \"git \nlog\" or otherwise bother some other readers.\n\nAnyway do you want me to resend the patch or the series with improved commit \nmessages ?\n\n>     Signed-off-by: ...\n>     ---\n>\n>      * Eric is CC'ed because the ultimate goal of this\n>        series is to get rid of the duplicated logic between\n>        help--browse and instaweb.\n>\n>      Makefile            |    2 +-\n>      git-help--browse.sh |   24 +++++++++---------------\n>     ...\n>\n> I have given only a cursory look at the remainder of the series\n> (I'll hopefully be in a mini vacation mode after the release),\n\nYou definitely deserve it. Thanks for your great release and maintainer \nwork.\n\n> but I think overall the series makes sense.\n\nThanks,\nChristian.\n"}]}