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

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 4527 bytes --]

Le lun 28 août 2006 07:35, Junio C Hamano a écrit :
> 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?

I think that merging the structs is indeed good, but that I do not like 
to break (even non common) setups when avoidable. but well, I've no 
*very* strong opinion on this, and if the consensus is that extending 
the argument with a format is good enough, I'll go for it, it would 
simplify the code a bit, and remove two arguments, both are good.

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

uuhhh, I've always thought that passing NULL to a %s format was allowed 
by the C standard ... I've no copy here but one at work, I will verify 
it (for my peace of mind) but I agree it's not pretty.

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

well, it was here before, so I left it, but I do not liked it much 
either, so if it's ok to remove it, I'd be glad to.

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

hmmm, lack of host= is only logged in the git_path_fmt function atm. Its 
presence is logged though. But I could obviously log that it's missing 
while parsing the extending attributes, and then only need to barf when 
the %h is actually needed in the format string. That seems indeed 
easier. Will do.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]