Re: [PATCH] full featured formating function of the --{base,user}_path arguments,
- From
Pierre Habouzit <madcoder@debian.org>
- Date
- Aug 28, 2006, 06:37 UTC
- Message-ID
- <200608280837.47870.madcoder@debian.org>
- In-Reply-To
- <7vd5altob5.fsf@assigned-by-dhcp.cox.net>
Le lun 28 août 2006 07:35, Junio C Hamano a écrit :
Show 30 quoted lines
> 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.
Show 20 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.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.
Show 16 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?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.
Show 33 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.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