On Wed, Jun 23, 2010 at 1:59 AM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> On Tue, 22 June 2010, Pavan Kumar Sunkara wrote:
>>
>> Yes, due to either unment dependency or circular dependency problem.
>> But no need to worry as they are very small.
>
> Could you _*LIST*_ those subroutines that you feel should belong in
> Gitweb::Config but could not be put in it, and write _why_ they could not
> be put (on what subroutines / variables do they depend that it makes
> impossible to move them)?
feature_* subs. They can't be put in it because they use
git_get_project_config from Gitweb::RepoConfig module
>>>
>>> What unment dependencies? Can it be worked around by changing their
>>> signatures to use additional parameters, e.g.:
>>>
>>> sub validate_action {
>>> my $input = shift || return undef;
>>> my $actions = shift || return undef;
>>>
>>> return undef unless exists $actions->{$input};
>>> return $input;
>>> }
>>
>> Yes. I think we can but don't you think that we need to import
>> %actions everytime we use validate_action.
>> So, I figured it would be better If i leave it untouched for now.
>
> Well, changing subroutines could be thought as being ot of scope of
> current series, which is about splitting gitweb and moving subroutines to
> appripriate modules... unless it would be not possible because of
> circular dependencies problem (but I guess it is not the case,
> ultimately).
>
>>
>>> But I guess it would be very hard to do the same with validate_project.
>>> Also such change might be out of scope for _this_ series.
>
> Perhaps it would be better then to move _all_ validate_* subroutines to
> separate Gitweb::Request::Validate module... unless they are used by some
> subroutine from Gitweb::Request.
or even better if we leave them untouched for now. (in gitweb.perl script)
and move them later along with the evaluate_validate_params function
>>>>
>>>> Subroutines moved:
>>>> evaluate_uri
>>>> evaluate_query_params
>>>> validate_pathname
>>>> validate_refname
>>>
>>> I'm not completly sure if validate_* subroutines should be not separate,
>>> as they do require more knowledge about setup, and about git repositories
>>> served, than the rest of Gitweb::Request subroutines...
>>
>> But they are being used in Gitweb::Request subroutines, so I included
>> them in there.
>
> Which Gitweb::Request subroutine uses validate_* subroutines? Neither
> evaluate_uri, nor evaluate_query_params, nor evaluate_path_info uses
> them. And evaluate_and_validate_params can be in Gitweb::Request::Validate.
>
> If there is circular dependency in reasoning given above, please explain
> it.
>
>>>> 6. gitweb: Create Gitweb::Escape module
>>>>
>>>> Create a Gitweb::Escape module in 'gitweb/lib/Gitweb/Escape.pm'
>>>> to store all the quoting/unquoting and escaping subroutines
>>>> regarding the gitweb.perl script.
>>>>
>>>> This module imports $fallback_encoding variable from
>>>> Gitweb::Config module to use it in sub 'to_utf8'
>>>>
>>>> Subroutines moved:
> [...]
>>>> unquote
>>>
>>> Shouldn't unquote be in Gitweb::Parse, as contrary to the rest of
>>> subroutines is is not about gitweb output escaping/quoting, but
>>> about processing (parsing) output of git commands? Perhaps it
>>> could even be provate to Gitweb::Parse (i.e. not exported by
>>> default)...
>>
>> It would result in circular dependency. So, Escape module is best for
>> it's place.
>
> Errr... how? If unquote is used only by subroutines in Gitweb::Parse
> (and I think it is), it could be local to Gitweb::Parse, not even
> exported (by default). Please explain.
Oh! I didn't know that.
I will change it.
>>>> 7. gitweb: Create Gitweb::RepoConfig module
>>>>
>>>> Create a Gitweb::RepoConfig module in 'gitweb/lib/Gitweb/RepoConfig.pm'
>>>> to store and handle all the configuration and subroutines
>>>> related to a single repository regarding the gitweb.perl script.
>>>>
>>>> This module depend on several other modules like Git.pm,
>>>> Config.pm, Request.pm and Escape.pm.
>>>>
>>>> It also include subroutines regarding project_list and
>>>> it's handling.
>>>
>>> Why? Is it to not have too many tiny modules, or is simply there no
>>> single good place where to put this subroutine?
>>
>> There is no single good place. Anyhow, as they are regarding project
>> configuration, I thought it would be better place for it.
>
> O.K.
>
>>>> 8. gitweb: Create Gitweb::View module
>>>>
>>>> Create Gitweb::View module in 'gitweb/lib/Gitweb/View.pm'
>>>> to store the subroutines related to the HTML output
>>>> for gitweb.
>>>
>>> I find that this module looks a bit like a collection of fairly unrelated
>>> subroutines at very different levels of abstractions.
>>>
>>> I guess that you don't want to split gitweb into too many modules,
>>> and splitting gitweb is more of one of steps to final goal of adding
>>> write functionality to gitweb, rather than the goal in and of itself.
>>> Nevertheless it would be good to be able to immediately know from the
>>> description of module what kind of subroutines should be there.
>
> Any comment on this, if you don't mind? Even "I don't want to think more
> about better split" would be all right for me.
I don't want to think more about better split :)
>>>> This module depends on Git.pm, Config.pm, Request.pm,
>>>> Escape.pm and RepoConfig.pm. Some subroutines which
>>>> output HTML but are not included in this module due
>>>> to unmet dependencies.
>>>
>>> Which subroutines and what unmet dependencies?
>>
>> action specific HTML divs. They include format_* and parse_* subs.
>
> Thanks for this info. It should be, IMHO, in the comit message.
>
> But... Shouldn't all format_* subs be in Gitweb::Format anyway?
> Shouldn't all parse_* subs be in Gitweb::Parse anyway? Which of format_*
> and parse_* do you feel that belong here?
Yes. I think u misunderstood me.
Gitweb::parse and Gitweb::Formst depend on Gitweb::View.
So, action specific HTML divs can't be placed in Gitweb::View because
they depend on Gitweb::Format and Gitweb::parse.
I think it's better if they are in GitwebAction::* modules
>>>> 9. gitweb: Create Gitweb::Util module
>>>>
>>>> Create Gitweb::Util module in 'gitweb/lib/Gitweb/Util.pm'
>>>> to store the git utility subroutines related to gitweb.
>>>>
>>>> This module include subroutines in various categories
>>>> such as git utility subs invoking git commands, git
>>>> utility subs accessing git repository, mimetype related
>>>> subs and HTML output utility subs.
>>>>
>>>> Subroutines moved:
>>>> git_get_head_hash
> [...]
>>>> is_patch_split
>>>
>>> O.K.
>>>
>>>> run_highlighter
>>>
>>> _Perhaps_ with exception of this subroutine.
>>
>> Well. It was in the utility category in gitweb.perl script. So, I
>> added it in here without giving much thought.
>
> Well, it is good explanation. All right, then.
>
>>>> 10. gitweb: Create Gitweb::Format module
>>>>
>>>> Create Gitweb::Format module in 'gitweb/lib/Gitweb/Format.pm'
>>>> to store the subroutines related to formatting of HTML
>>>> fragments required for gitweb.
>>>>
>>>> This module depends on Config.pm, View.pm, Escape.pm,
>>>> Util.pm and Request.pm. It mainly contain functions returning
>>>> short HTML fragments or transforming HTML fragments. Also
>>>> include subroutines regarding avatar formatting.
>>>
>>> Why it depends on Gitweb::View, and not the reverse? I understand why
>>> it depends on Gitweb::Config and Gitweb::Escape, and I guess it needs
>>> $cgi from Gitweb::Request (or is it something more?).
>>
>> $hash, $hash_parent also.
>
> Why it depends on Gitweb::View? Is it because of ubiquitous and
> troublesome :-) die_error?
>
>>>> 11. gitweb: Create Gitweb::Parse module
>>>>
>>>> Create Gitweb::Parse module in 'gitweb/lib/Gitweb/Parse.pm'
>>>> to store the subroutines which related to parsing functions
>>>> required for gitweb.
>>>>
>>>> This module depends on Git.pm, Escape.pm, View.pm and Util.pm.
>>>
>>> Why it depends on Gitweb::View? It is a strange dependency.
>>> I understand depending on Gitweb::Git to some extent, I'm not sure
>>> if we shouldn't simply move unescape to it instead of requiring
>>> Gitweb::Escape (unless there is more: to_utf8 perhaps?), and
>>> I understand that Gitweb::Util has some required subroutines.
>>
>> for die_error.
>
> Ah, I se that any run of git command is protected with
> 'or die_error(...)'. Hmmm, not very good.
>
> Well, in the future we could think about decoupling Gitweb::Parse from
> other modules, but for now it should be enough to mention in commit
> message that Gitweb::Parse depends on Gitweb::View because of die_error.
> Good for now. O.K.
>
> Perhaps die_error should be in Gitweb::Carp? But it neds git_header_html
> etc. anyway. Ugh.
>
> --
> Jakub Narebski
> Poland
>
Thanks,
Pavan.
--
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