mike@xxxxxxx (mike dalessio) writes:
> diff --git a/Documentation/git-instaweb.txt b/Documentation/git-instaweb.txt
> index cec60ee..914fc4c 100644
> --- a/Documentation/git-instaweb.txt
> +++ b/Documentation/git-instaweb.txt
> @@ -27,7 +27,8 @@ OPTIONS
> The HTTP daemon command-line that will be executed.
> Command-line options may be specified here, and the
> configuration file will be added at the end of the command-line.
> - Currently, lighttpd and apache2 are the only supported servers.
> + Currently, lighttpd, apache2 and webrick are the only supported
> + servers.
> (Default: lighttpd)
Perhaps we can start thinking about rewording "are the only
suported servers" to "are supported".
> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index b79c6b6..803a754 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -37,7 +37,9 @@ start_httpd () {
> else
> # many httpds are installed in /usr/sbin or /usr/local/sbin
> # these days and those are not in most users $PATHs
> - for i in /usr/local/sbin /usr/sbin
> + # in addition, we may have generated a server script
> + # in $fqgitdir/gitweb.
> + for i in /usr/local/sbin /usr/sbin $fqgitdir/gitweb
> do
> if test -x "$i/$httpd_only"
> then
I do not think this hunk belongs to this patch. It alone would
be a useful addition to the program even without the rest of
your patch, wouldn't it? Imagine a case where I automatically
would reject all patches that have "Ruby" in it for some unknown
reason. Do we want to drop this hunk in such a case?
> + # generate a shell script to invoke the above ruby script,
> + # which assumes _ruby_ is in the user's $PATH. that's _one_
> + # portable way to run ruby, which could be installed anywhere,
> + # really.
No games with env, type, nor which. Good.
Just a few style things (-) and one concern (+):
> + cat > "$fqgitdir/gitweb/$httpd" <<EOF
> +#! /bin/sh
> +ruby $fqgitdir/gitweb/$httpd.rb \$*
> +EOF
- I do not like extra whitespace between she-bang #! and the
path. Looks very ugly.
- I do not like extra whitespace between redirection > and
redirected filename either. Looks very ugly.
- I prefer such wrapper to exec the command, like this:
#!/bin/sh
exec ruby ...
+ fqgitdir and httpd need to be shell quoted. I do not think
anybody is stupid enough to have his GIT_DIR set to something
like "/opt/funny/; rm -f / nuke-me/.git/" but you would see
spaces and single quotes in pathnames in odd environments.
I wonder how popular instaweb is and how widely it is used.
I've actually wondering if we should demote it to contrib/
somewhere, but it gets occasional updates so people must be
using it...
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
