Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO

From: Petr Baudis <pasky@suse.cz>
Date: Oct 2, 2008, 20:56 UTC
Message-ID: <20081002205603.GW10360@machine.or.cz>
In-Reply-To: <cb7bb73a0810021230u2ec512c0l577b3146cffccb3e@mail.gmail.com>

On Thu, Oct 02, 2008 at 09:30:42PM +0200, Giuseppe Bilotta wrote:

Show 18 quoted lines

> On Thu, Oct 2, 2008 at 5:34 PM, Petr Baudis <pasky@suse.cz> wrote:
> 
> > Just nits...
> >> +     }
> >> +
> >>       my ($refname, $pathname) = split(/:/, $path_info, 2);
> >>       if (defined $pathname) {
> >> -             # we got "project.git/branch:filename" or "project.git/branch:dir/"
> >> +             # we got "project.git/action/branch:filename" or "project.git/action/branch:dir/"
> >>               # we could use git_get_type(branch:pathname), but it needs $git_dir
> >>               $pathname =~ s,^/+,,;
> >>               if (!$pathname || substr($pathname, -1) eq "/") {
> >
> > But the old variant is still possible, maybe the comments should
> > indicate that the action/ part is optional.
> 
> I put the action/ part in square brackets. (e.g.
> project.git/[action/]branch:filename). Is this good enough?

That's perfect.

Show 20 quoted lines

> >> @@ -534,8 +575,9 @@ sub evaluate_path_info {
> >>               $file_name ||= validate_pathname($pathname);
> >>       } elsif (defined $refname) {
> >>               # we got "project.git/branch"
> >> -             $action ||= "shortlog";
> >> -             $hash   ||= validate_refname($refname);
> >> +             $action    ||= "shortlog";
> >> +             $hash      ||= validate_refname($refname);
> >> +             $hash_base ||= validate_refname($refname);
> >>       }
> >>  }
> >>  evaluate_path_info();
> >
> > What is this good for?
> 
> The purpose of what? setting both $hash and $hash_base was something
> that I found was needed in some extreme cases, as discussed with
> Jakub. Proposals for recommended cleaner but equally fast way to
> handle it. If you're referring to the whitespace, I was just lining up
> the entries. Should I do it in a separate patch?

I refer to the setting of $hash_base (I'm not huge fan of the whitespace aligning, but I don't really care). What extreme cases are these? I think you should describe that in the code since it's not really obvious. Maybe I could find it in older threads but I should understand the code just from reading it. :-)

-- 
				Petr "Pasky" Baudis
People who take cold baths never have rheumatism, but they have
cold baths.