Subject: Re: [RFC] gitweb: Add committags support (take 2)

From: Jakub Narebski <[email-masked]>
Date: Mon, 4 Dec 2006 12:33:19 +0100

Dnia poniedziałek 4. grudnia 2006 11:53, Junio C Hamano napisał:
> Jakub Narebski <[email-masked]> writes:
> 
>> I have one gripe about "git-cat-file -t". I'd like it to have 
>> -q/--quiet, -s/--silent, --hush (or --dont-spew-errors-on-stdout)
>> which would prohibit writing "object not found" errors on stderr
>> (and in gitweb case to webserver logs). I know I can use
>> "git-cat-file -e" to check if object exists, or modify git_get_type
>> subroutine 
>>
>>   # get type of given object
>>   sub git_get_type {
>> 	my $hash = shift;
>>   	my $git_command = git_cmd_str();
>>   
>>   	open my $fd, "-|",
>>   		"$git_command cat-file -t $hash 2>/dev/null"
>>   		or return '';
> 
> It is one thing if you tend to randomly throw garbage at this
> function and use it to check for object's existence, but I hope
> you are already checking the user input (which is what $hash is,
> I think, here), and the object is supposed to exist in the
> repository you are looking at.  In such a case, I think you and
> your server administrator have right to know about that
> situation; I do not see why you would want to squelch it.

I'm sorry, I should mention that this "quiet" mode of operation is 
needed _only_ for committags support, for example by using
git_get_type($hash_text, -quiet=>1) in 'sha1' committag subroutine.

You might have sha1 ids in commit message which no longer point to valid 
(existing) object, for example commit which is result of 
"git cherry-pick -x" from no longer existing temporary branch, or commit 
which is result of "git revert" on a branch which got rebased (but not 
reorganized), or shortened sha1 which is no longer unique. This should 
not cause errors to be written to webserver log.

By the way, is it better to use anonymous subroutines for committags 
subs, or use explicit subroutines?

>>> I do not know how this %committags{} is used per project.  With
>>> a setting like repo.or.cz, it is likely that one instance of
>>> gitweb is serving unrelated projects that have their issue
>>> tracker at different locations using different "committags"
>>> convention.  Is the idea to eventually allow enabling/disabling
>>> elements from the global %committags per repository somehow
>>> (perhaps not just enable/disable but even overriding patterns or
>>> parameters)?
>>
>> I have thought about putting %committags and @committags before
>> loading config file
>>   do $GITWEB_CONFIG if -e $GITWEB_CONFIG;
>> which can load config file depending on the project, but perhaps
>> it is too complicated solution.
> 
> I think you are talking about a gitweb-instance wide
> customization, but that's not what I meant.  I meant per-project
> configuration where w/git-gui.git and w/git.git are served by
> the same instance of gitweb but have pointers to different issue
> trackers.

You can always use $ENV{PATH_INFO} in $GITWEB_CONFIG value,
or check out path_info and/or $cgi->params('p') in the config file.
Or perhaps other way to set-up per repository config file.
But this is a bit complicated to set up.

I don't have definite answer about how configure committags
(both the committags enabled and sequence of committags, and
committags parameters) per repository. We can use gitweb.committags 
config variable for committags enables/sequence, but how configure 
committags? gitweb.committag.<name>?

BTW in some cases (e.g. xmms2 projects) issue tracker is common for
all projects hosted.

>>>> 3. To not split message into many fragments we concatenate strings
>>>> if possible.
>>> 
>>> I do not know why "avoiding splits" is needed, if it raises 
>>> issues that you need to ask the list about in a message like
>>> this...
>>
>> "Avoiding splits" is needed first for performance, and second to
>> avoid situation where pattern would match on the boundary between
>> two strings in a list of tokens to process.
> 
> I wouldn't know if constantly splitting and then concatenating
> is faster than just concatenatting once before output without
> benchmarking, so I'd refrain from talking about performance.
> Two string case may be a valid concern, though.

With current implementation it is very easy to switch this one and off.
You simply either use push, or push_or_append (or make push_or_append
do just push).

Previous (not published) version used $acc variable to concatenate 
strings, but I think this solution is better (and simpler).

-- 
Jakub Narebski