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

From: Junio C Hamano <junkio@cox.net>
Date: Aug 28, 2006, 05:35 UTC
Message-ID: <7vd5altob5.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11566787581030-git-send-email-madcoder@debian.org>

Pierre Habouzit <madcoder@debian.org> writes:

Show 14 quoted lines

> +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?

Show 14 quoted lines

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

Show 11 quoted lines

> +	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?

Show 24 quoted lines

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