On Thu, Oct 2, 2008 at 10:59 AM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> Giuseppe Bilotta wrote:
>>
>> +# dispatch
>> +my %actions = (
>> + "blame" => \&git_blame,
> [...]
>> +);
>
> I'm not sure if the '# dispatch' comment is correct here now that
> %actions hash is moved away from actual dispatch (selecting action
> to run)
Bingo.
>> @@ -519,9 +550,19 @@ sub evaluate_path_info {
>> # do not change any parameters if an action is given using the query string
>> return if $action;
>> $path_info =~ s,^\Q$project\E/*,,;
>> +
>> + # next comes the action
>> + $action = $path_info;
>> + $action =~ s,/.*$,,;
>
> I would use here pattern matching, but your code is also good and
> doesn't need changing; just for completeness below there is alternate
> solution:
>
> + $path_info =~ m,^(.*?)/,;
> + $action = $1;
Yeah, I just followed the existing code style.
>> @@ -534,8 +575,9 @@ sub evaluate_path_info {
>> $file_name ||= validate_pathname($pathname);
>> } elsif (defined $refname) {
>> # we got "project.git/branch"
>
> You meant here
>
> # we got "project.git/branch" or "project.git/action/branch"
Yes I do.
>> - $action ||= "shortlog";
>> - $hash ||= validate_refname($refname);
>> + $action ||= "shortlog";
>> + $hash ||= validate_refname($refname);
>> + $hash_base ||= validate_refname($refname);
>> }
>> }
>
> This hunk is IMHO incorrect. First, $refname is _either_ $hash, or
> $hash_base; it cannot be both. Second, in most cases (like the case
> of 'shortlog' action, either explicit or implicit) it is simply $hash;
> I think it can be $hash_base when $file_name is not set only in
> singular exception case of 'tree' view for the top tree (lack of
> filename is not an error, but is equivalent to $file_name='/').
OTOH, while setting both $hash and $hash_base has worked fine for me
so far (because the right one is automatically used and apparently
setting the other doesn't hurt), choosing which one to set is a much
hairier case. Do you have suggestions for a better way to always make
it work?
>> @@ -544,37 +586,6 @@ evaluate_path_info();
>> our $git_dir;
>> $git_dir = "$projectroot/$project" if $project;
>>
>> -# dispatch
>> -my %actions = (
> [...]
>> -);
>> -
>> if (!defined $action) {
>> if (defined $hash) {
>> $action = git_get_type($hash);
>
> I _think_ the '# dispatch' comment should be left here, and not moved
> with the %actions hash.
I agree.
>> @@ -631,8 +642,15 @@ sub href (%) {
>> if ($params{-replay}) {
>> while (my ($name, $symbol) = each %mapping) {
>> if (!exists $params{$name}) {
>> - # to allow for multivalued params we use arrayref form
>> - $params{$name} = [ $cgi->param($symbol) ];
>> + # the parameter we want to recycle may be either part of the
>> + # list of CGI parameter, or recovered from PATH_INFO
>> + if ($cgi->param($symbol)) {
>> + # to allow for multivalued params we use arrayref form
>> + $params{$name} = [ $cgi->param($symbol) ];
>> + } else {
>> + no strict 'refs';
>> + $params{$name} = $$name if $$name;
>
> I would _perhaps_ add here comment that multivalued parameters can come
> only from CGI query string, so there is no need for something like:
>
> + $params{$name} = (ref($$name) ? @$name : $$name) if $$name;
>
>> + }
>> }
>> }
>> }
>
> This fragment is a bit of ugly code, hopefully corrected in later patch.
> I think it would be better to have 'refactor parsing/validation of input
> parameters' to be very fist patch in series; I am not sure but I suspect
> that is a kind of bugfix for current "$project/$hash" ('shortlog' view)
> and "$project/$hash_base:$file_name" ('blob_plain' and 'tree' view)
> path_info.
But implementing the path_info parsing first makes the input param
refactoring SO much nicer that I would rather put a comment here
saying "this code sucks: we should rather collect all input
parameters" and then clean it up on the subsequent patch.
--
Giuseppe "Oblomov" Bilotta
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
