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

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 3 Oct 2008, Giuseppe Bilotta wrote:
> On Fri, Oct 3, 2008 at 2:48 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:

>>> 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?
> 
> Because cleaning it up depends on the refactoring, and the refactoring
> is much cleaner when path_info already handles $action too.

Hmmm... it looks like after refactoring you don't have to change href()
at all when adding $action to parameters gitweb can get from path_info;
all that having refactoring (cleanup) upfront changes in/for this patch
is a bit different saving action (also in %input_params) and lack of
this ugly and I think subtly wrong added code for href(...,-replay=>1).
 
>> 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.
> 
> Ah the issue of the default action is something I hadn't taken into
> consideration, actually. Now the question is, should replay keep
> default -> default, or should it go with default -> last incantation?

I think we should use default -> default.

Besides I think there can also be an issue ("can" because I am not sure
if in practice it is a problem) the fact that gitweb sometimes expand
parameters to sha-1, for example setting $head to git_get_head_hash()
if it is not set (default param value), and not to 'HEAD'. This perhaps
should be changed, but to be on safer side better not to use 'action
variables' because some code treats them as temporary variables.

>> 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.
> 
> Yes, but it would muddy the waters about 'where did this parameter
> come from' in case we ever need to know that.

True. Like for example implementing -faithful_replay where if parameter
was passed through path_info it is replayed through path_info, and if
it was passed through query string it is replayed as CGI query param.

After thinking a bit about that I think that %input_params idea is
superior to both $cgi->params(-name=>..., -value=>...) and to have
either no strict refs $$name or name to action variable ref hash.
See also my comment for the refactoring patch.

-- 
Jakub Narebski
Poland