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

[Christian Couder <chriscool@tuxfamily.org>]

Le samedi 2 février 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > +	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.

Yeah, I could have explained some parts of the patch more.
I will try to do better.

> 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.

Great commit message indeed! Though I fear that such long messages (for a 
not very long patch) could take precious screen real estate when using "git 
log" or otherwise bother some other readers.

Anyway do you want me to resend the patch or the series with improved commit 
messages ?

>     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),

You definitely deserve it. Thanks for your great release and maintainer 
work.

> but I think overall the series makes sense.

Thanks,
Christian.