Re: [PATCH] Show html help with git-help --html

["Nguyen Thai Ngoc Duy" <pclouds@gmail.com>]

On 6/4/07, Junio C Hamano <junkio@cox.net> wrote:
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
> > On 6/4/07, Junio C Hamano <gitster@pobox.com> wrote:
> >> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> >> > index 3d8f03d..2ec8545 100644
> >> > --- a/Documentation/config.txt
> >> > +++ b/Documentation/config.txt
> >> > @@ -261,6 +261,18 @@ core.excludeFile::
> >> > ...
> >> > +core.htmlprogram::
> >> > +     Specify the program used to open html help files when 'git-help'
> >> > +     is called with option --html or core.help is other than 'man'.
> >> > +     By default, xdg-open will be used.
> >>
> >> Is the program's calling convention something that needs to be
> >> customizable for this to be useful?
> >
> > At first I thought xdg-open would be flexible enough for most Linux
> > systems because it will choose the best browser you have. But I now
> > recall that Git does not only run on Linux.  Will make it a parameter
> > in config.mak.in
>
> Actually that was not what I meant.  I thought we might need to
> support something like:
>
>         core.htmlprogram = grand-unified-help %b
>
> for a program that knows where in the system the distribution
> keeps help HTML files, and add prefix (/usr/share/html-help/,
> perhaps) and suffix (obviously, .html) to the basename of the
> command (e.g. "grand-unified-help git-rebase" ends up running
> "lynx file:///usr/share/html-help/git-rebase.html"), or perhaps
>
>         core.htmlprogram = firefox /usr/share/html-help/%f
>
> and we substitute '%f' with "git-rebase.html".

Ok. Now I get it.

>
> >> ...
> >> > +     if (prefixcmp(git_cmd, "git"))
> >> > +             strcat(p,"git-");
> >> > +     strcat(p,git_cmd);
> >> > +     strcat(p,".html");
> >> > +
> >> > +     ret = system(p);
> >>
> >> This is sloppy in the presense of potentially unsafe characters...
> >
> > I personally think users will not shoot themselves with "git help
> > --html ';rm -rf'" but again scripts can. Thank you for pointing out.
> > Will add check for file existence before calling system().
>
> File existence?  I am not sure what you mean by that, sorry.

Something like this on top of the last patch.

diff --git a/help.c b/help.c
index e3e705b..6f5c340 100644
--- a/help.c
+++ b/help.c
@@ -191,7 +191,8 @@ static void show_html_page(const char *git_cmd)
 {
        const char *html_dir;
        int i,len,ret;
-       char *p;
+       char *p,*pathname;
+       struct stat html_stat;

        html_dir = HTML_DIR;
        if (!html_help_program)
@@ -203,13 +204,19 @@ static void show_html_page(const char *git_cmd)

        strcpy(p, html_help_program);
        strcat(p," ");
+       pathname = p + strlen(p);
        strcat(p,html_dir);
        if (prefixcmp(git_cmd, "git"))
                strcat(p,"git-");
        strcat(p,git_cmd);
        strcat(p,".html");

-       ret = system(p);
+       if (!stat(pathname, &html_stat))
+               ret = system(p);
+       else {
+               error("%s not found",pathname);
+               ret = -1;
+       }

        /* fallback to man pages */
        if (show_html_help > 1 && (ret == -1 || ret > 0))
-- 
Duy