Re: [PATCH] Add virtualization support to git-daemon
- From
Junio C Hamano <junkio@cox.net>
- Date
- Sep 17, 2006, 10:07 UTC
- Message-ID
- <7vvenm4xi6.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <E1GOint-00048W-2A@jdl.com>
I have a few stylistic comments but the contents look quite sane and fine.
Show 9 quoted lines
> I use an inetd invocation like this example: > > git stream tcp nowait nobody /usr/bin/git-daemon git-daemon --inetd > --verbose --syslog > --export-all --interpolated-path=/pub/%H/%D > /pub/software /software > /pub/www.example.com/software > /pub/www.example.org/software > /pub
This would be nice to have as an example in the documentation.
Show 19 quoted lines
> @@ -45,6 +51,23 @@ static const char *user_path;
> static unsigned int timeout;
> static unsigned int init_timeout;
>
> +/*
> + * Static table for now. Ugh.
> + * Feel free to make dynamic as needed.
> + */
> +#define INTERP_SLOT_HOST (0)
> +#define INTERP_SLOT_DIR (1)
> +#define INTERP_SLOT_PERCENT (2)
> +
> +struct interp interp_table[] = {
> + { "%H", 0},
> + { "%D", 0},
> + { "%%", "%"},
> +};
> +
> +#define N_INTERPS (sizeof(interp_table) / sizeof(struct interp))Shouldn't this variable static to the file?
We seem to use ARRAY_SIZE() macro, which is not particularly my favorite but other places already use it so probably we would want to keep consistency.
Show 9 quoted lines
> + loginfo("Before interpolation '%s'", dir);
> + loginfo("Interp slot 0 (%s,%s)",
> + interp_table[0].name, interp_table[0].value);
> + loginfo("Interp slot 1 (%s,%s)",
> + interp_table[1].name, interp_table[1].value);
> + interpolate(interp_path, PATH_MAX, interpolated_path,
> + interp_table, N_INTERPS);
> + loginfo("After interpolation '%s'", interp_path);
> + dir = interp_path;Do we really want to see each step of interpolation in the log? Sounds more like logdebug than loginfo to me.
Show 5 quoted lines
> + snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
> + loginfo("dir was %s", dir);
> + loginfo("base_path is %s", base_path);
> + loginfo("rpath now %s", rpath);
> + dir = rpath;Likewise.
Show 5 quoted lines
> @@ -351,6 +398,29 @@ static void make_service_overridable(con
> die("No such service %s", name);
> }
>
> +void parse_extra_args(char *extra_args, int buflen)static?
Show 8 quoted lines
> @@ -391,13 +461,19 @@ #endif
> if (len && line[len-1] == '\n')
> line[--len] = 0;
>
> + if (len != pktlen) {
> + parse_extra_args(line + len + 1, pktlen - len - 1);
> + }
> +No { } around single statement.Show 5 quoted lines
> +int
> +interpolate(char *result, int reslen,
> + char *orig,
> + struct interp *interps, int ninterps)
> +{"int interpolate(..."; the return type and function name on the same line (again, not my preference but just doing as Romans do).
> + bzero(result, reslen);
memset(result, 0, reslen);
Show 6 quoted lines
> diff --git a/interpolate.h b/interpolate.h > new file mode 100644 > index 0000000..241af7c > --- /dev/null > +++ b/interpolate.h > @@ -0,0 +1,13 @@
#ifndef INTERPOLATE_H
#define INTERPOLATE_H
...
#endif> +/* > + * Copyright 2006 Jon Loeliger > + */
Show 10 quoted lines
> +
> +struct interp {
> + char *name;
> + char *value;
> +};
> +
> +extern int interpolate(char *result, int reslen,
> + char *orig,
> + struct interp *interps, int ninterps);
> +