Christian Couder <chriscool@tuxfamily.org> writes:

quoted text
> By moving some help specific stuff from 'git-help--browse.sh'
> into 'help.c', we make it possible to use 'git-help--browse'
> outside 'git-help'.

I was initially puzzled why Eric was CC'ed while reading this
first patch in the 4-patch series.  It would have been nicer to
start the series by mentioning the ultimate goal of the series
upfront.  You are not making it usable just by anybody, but have
a specific goal of sharing the mechanism to launch user's web
browser for both help and instaweb.

quoted text
> @@ -71,6 +66,8 @@ do
>      shift
>  done
>  
> +test $# = 0 && usage
> +
>  if test -z "$browser"
>  then
>      for opt in "help.browser" "web.browser"
> @@ -113,16 +110,13 @@ else
>      fi
>  fi
>  
> -pages=$(for p in "$@"; do echo "$html_dir/$p.html" ; done)
> -test -z "$pages" && pages="$html_dir/git.html"
> -

When the helper was run without specifying any page, it used to
show the toplevel "git" documentation.  If your eventual goal is
to allow general browsing, obviously you do not want such a
logic here, and it needs to migrate to the caller.  So far, it
makes perfect sense. Let's read on.

quoted text
> diff --git a/help.c b/help.c
> index 1302a61..b929899 100644
> --- a/help.c
> +++ b/help.c
> @@ -328,10 +328,26 @@ static void show_info_page(const char *git_cmd)
>  	execlp("info", "info", "gitman", page, NULL);
>  }
>  
> +static void get_html_page_path(struct strbuf *page_path, const char *page)
> +{
> +	struct stat st;
> +
> +	/* Check that we have a git documentation directory. */
> +	if (stat(GIT_HTML_PATH "/git.html", &st) || !S_ISREG(st.st_mode))
> +		die("'%s': not a documentation directory.", GIT_HTML_PATH);
> +
> +	strbuf_init(page_path, 0);
> +	strbuf_addf(page_path, GIT_HTML_PATH "/%s.html", page);
> +}

Gets a "page", makes page_path by concatenating the manual page
location.  Looks Ok.

quoted text
>  static void show_html_page(const char *git_cmd)
>  {
>  	const char *page = cmd_to_page(git_cmd);
> -	execl_git_cmd("help--browse", page, NULL);

So this function used to give whatever "page" it got back from
cmd_to_page().  Maybe it could have been NULL but that would
have been handled by the browser helper just fine.

A reviewer would be left wondering if this means that you lost
the fallback to the git top page.

quoted text
> +	struct strbuf page_path; /* it leaks but we exec bellow */
> +
> +	get_html_page_path(&page_path, page);
> +
> +	execl_git_cmd("help--browse", page_path.buf, NULL);
>  }

And this part makes the reviewer even more worried. If page
could be NULL, then get_html_page_path() would be fed a NULL
pointer, which is given to strbuf_addf()!  Ugh.

Then the reviewer would find out that cmd_to_page() would never
return NULL, as it has its own NULL-to-"git" fallback logic.

I think the code is good, but the proposed commit log message
has some room for improvements.  

Something like...

    [PATCH 1/4] help: make 'git-help--browse' usable outside 'git-help'

    "git-help--browse" helper is to launch a browser of the
    user's choice to view the HTML version of git documentation
    for a given command.  It used to take the name of a command,
    convert it to the path of the documentation by prefixing the
    directory name and appending the ".html" suffix, and start
    the browser on the path.

    This updates the division of labor between the caller in
    help.c and git-help--browser helper.  The helper is now
    responsible for launching a browser of the user's choice
    on given URLs, and it is the caller's responsibility to
    tell it the paths to documentation files.

    This is in preparation to reuse the logic to choose
    user's preferred browser in instaweb.

    The helper had a provision for running it without any
    command name, in which case it showed the toplevel "git(7)"
    documentation, but the caller in help.c never makes such a
    call.  The helper now exits with a usage message when no
    path is given.

    Signed-off-by: ...
    ---

     * Eric is CC'ed because the ultimate goal of this
       series is to get rid of the duplicated logic between
       help--browse and instaweb.

     Makefile            |    2 +-
     git-help--browse.sh |   24 +++++++++---------------
    ...

I have given only a cursory look at the remainder of the series
(I'll hopefully be in a mini vacation mode after the release),
but I think overall the series makes sense.

Thanks.

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html