On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
> On Thu, Oct 2, 2008 at 10:59 AM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
>> Giuseppe Bilotta wrote:
>>> - $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?
Well, it is either checking $action and setting either $hash or
$hash_base, or setting both, with some comments on why and when it is
needed (as discussed on #git). IIUC $hash_base is needed only for
filename-taking tree actions which acts on top-tree, and therefore
don't need $file_name, like 'project/tree/branch' or related
'project/history/branch' (the latter is practically almost equivalent
to 'project/shortlog/branch' or 'project/branch').
I'm not sure if it wouldn't be better to call validate_refname($refname)
once, either as:
$hash_base ||= $hash ||= validate_refname($refname);
but that might be incorrect in the obscure case of setting $hash via 'h'
CGI query parameter, and letting gitweb to set-up $hash_base via
path_info, so perhaps ($refname is local to evaluate_path_info, IIRC)
$refname = validate_refname($refname);
$hash ||= $refname;
$hash_base ||= $refname;
But that is just nitpicking this fragment of code to death. In short:
either check which of $hash and $hash_base to set in this branch of
conditional, or explain why setting both $hash and $hash_base is needed,
and why it is acceptable, either as comments, or in commit message.
>>> @@ -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.
Why not cleanup first?
When implementing href(..., -replay=>1) I have forgot that some of
gitweb parameters are implicitly passed ($project, because it is needed
in most gitweb links), and some can be passed via path_info ($hash
and/or $hash_base, $file_name). Your code adds $action to the mix, but
it doesn't change the fact that 1.) even before your code -replay case
was incorrect for some path_info links (handcrafted, as gitweb generates
only $project via path_info); 2.) code you have added is a bit ugly.
Besides using variables change a little meaning of -replay, namely
in your code gitweb always sets action, even for non-path_name links
when we started from "default action" (i.e. without action set) links.
I guess this is mainly theoretical issue, as I don't think that default
views use many -replay links.
P.S. with the idea of pushing parameters obtained not from CGI query
string to $cgi->param() via "$cgi->param($name, $value);" or in named
params form "$cgi->(-name=>$name, -value=>$value);" you would not need
to change (a bit hacky, admittedly) href(...,-replay=>1) code.
--
Jakub Narebski
Poland
--
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
