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

From: Jakub Narebski <jnareb@gmail.com>
Date: Oct 3, 2008, 10:31 UTC
Message-ID: <200810031231.40146.jnareb@gmail.com>
In-Reply-To: <cb7bb73a0810022304r11d2ad87q7691213ff67f7e4c@mail.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:

Show 9 quoted lines

>>> 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).

Show 17 quoted lines

>> 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.

Show 7 quoted lines

>> 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