Re: [PATCH] full featured formating function of the --{base,user}_path arguments,

[Junio C Hamano <junkio@cox.net>]

Pierre Habouzit <madcoder@debian.org> writes:

> +static struct {
> +	const char *path;
> +	int use_as_fmt;
> +} base_path;
>  
>  /* If defined, ~user notation is allowed and the string is inserted
>   * after ~user/.  E.g. a request to git://host/~alice/frotz would
>   * go to /home/alice/pub_git/frotz with --user-path=pub_git.
>   */
> -static const char *user_path;
> +static struct {
> +	const char *path;
> +	int use_as_fmt;
> +} user_path;

Maybe it does not matter much, but I wonder if we want to keep
two structs the same type, like:

        static struct {
                const char *path;
                int use_as_fmt;
        } base_path, user_path;

I also wondered if we can just extend the semantics of base_path
and user_path to autodetect the fmt-ness of them, but that means
we would break existing setups that uses per-cent in the
pathname.  Arguably that would not be so common and we may not
need to worry about such an installation, though.  What do you
think?

> @@ -174,24 +285,45 @@ static char *path_ok(char *dir)
>  				slash = dir + restlen;
>  			namlen = slash - dir;
>  			restlen -= namlen;
> +
> +			if (user_path.use_as_fmt) {
> +				loginfo("host <%s>, "
> +					"userpathfmt <%s>, request <%s>, "
> +					"namlen %d, restlen %d, slash <%s>",
> +					vhost,
> +					user_path.path, dir,
> +					namlen, restlen, slash);
> +				dir = git_path_fmt(rpath, user_path.path, vhost,
> +						   slash, dir + 1, namlen - 1);

When vhost is NULL you would feed it to "%s", which I think
glibc works around with (null) fine but other C libraries would
not like it.  git_path_fmt()'s logging does not have this
problem, though.

> +	else if (base_path.path) {
>  		if (*dir != '/') {
>  			/* Allow only absolute */
>  			logerror("'%s': Non-absolute path denied (base-path active)", dir);
>  			return NULL;
>  		}
> +
> +		if (base_path.use_as_fmt) {
> +			dir = git_base_path_fmt(rpath, base_path.path, vhost, dir);
> +		} else {
> +			snprintf(rpath, PATH_MAX, "%s%s", base_path.path, dir);

The level of logging in this branch and in user_path.use_as_fmt
branch are inconsistent.  Maybe the more detailed one above I
commented about vhost==NULL case was primarily meant for
debugging and you forgot to remove it?

> @@ -274,6 +406,7 @@ static int execute(struct sockaddr *addr
> @@ -303,15 +436,30 @@ #endif
>  	alarm(0);
>  
>  	len = strlen(line);
> +
> +	if (pktlen != len) {
> +		int arg_pos = len + 1;
> +
>  		loginfo("Extended attributes (%d bytes) exist <%.*s>",
>  			(int) pktlen - len,
> +			(int) pktlen - len, line + arg_pos);
> +
> +		while (arg_pos < pktlen) {
> +			int arg_len = strlen(line + arg_pos);
> +
> +			if (!strncmp("host=", line + arg_pos, 5)) {
> +				vhost = line + arg_pos + 5;
> +			}
> +
> +			arg_pos += arg_len + 1;
> +		}
> +	}
> +

I think it is easier to do:

	if (!vhost)
        	vhost = default_host;

and have git_base_path_fmt() barf if the format calls for %h and
vhost passed to it is NULL.  Lack of "host=" in the request is
logged here already.