Re: [PATCH] gitweb: new cgi parameter: option

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 12 Jul 2007, Miklos Vajna wrote:
> Currently the only supported value is '--no-merges' for the 'rss', 'atom',
> 'log', 'shortlog' and 'history' actions, but it can be easily extended to allow
> other parameters for other actions.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>

Micronit: it is unwritten (as of yet) requirement to word wrap commit
message at 80 columns or less.

> ---
> 
> Na Wed, Jul 11, 2007 at 11:19:41PM +0200, Jakub Narebski <jnareb@gmail.com> pisal(a):
>> Miklos Vajna wrote:
>>
>>> +((defined $filter and $filter == "nomerges") ? ("--no-merges") : ()),
>>
>> Shouldn't it be '$filter eq "nomerges"' instead?
> 
> Yes, that works too (I'm not a perl addict :) )

By the way, there is t9500-gitweb-standalone-no-errors.sh test script to
check if gitweb doesn't give any Perl warnings or errors. Please try to
use it; it should at least find errors about undefined values and such.
But it has the disadvantage of requiring git to be build (compiled),
even if theoretically testing gitweb doesn't require it.

You would see something like

  Argument "nomerges" isn't numeric in numeric eq (==)

when running this test with --debug, I think.

>> Besides, I'd rather have generalized way to provide additional options
>> to git commands, like '--no-merges' for RSS and Atom feeds, log, shortlog
>> and history views, '-C' for commitdiff view, '--remove-empty' for history
>> view for a file, perhaps even '-c' or '--cc' for commitdiff for merges
>> instead of abusing 'hp' argument for that.

Now I'm not so sure about using 'option' for selecting between
combined ('-c') and compressed combined ('--cc') formats for commitdiff
for merges. The '-c' and '--cc' (or '-m') must be used with only one
commit-ish[*1*], so they can take place of the second commit-ish, i.e.
'hp' (hash_parent) parameter. What do the list think about it?

[*1*] At least in gitweb. If I understand correctly, you can use
"git diff --cc tree1 tree2 tree2 ..." to get combined diff of specified
tree-ish; I'm not sure if git-diff-tree support this. And I know that
gitweb does not support this... at least for now. Would this be useful,
I wonder?

>> But that doesn't mean that this patch should be not applied... it doesn't
>> mean it should be applied neither ;-)
> 
> What about this one?

See comments below.


> +my %options = (
> +	"--no-merges" => [('rss', 'atom', 'log', 'shortlog', 'history')],
> +);

First, you don't need inner () parentheses to delimit/create list, as
anonymous array reference constructor [] works like it. So it could be
written simply as

  +my %options = (
  +	"--no-merges" => ['rss', 'atom', 'log', 'shortlog', 'history'],
  +);

Second, instead of quoting each word by hand, we can use handy Perl
quoting operator, qw(), i.e. 'word list' operator, like below. 
See perlop(1), "Quote and Quote-like Operators" subsection

  +my %options = (
  +	"--no-merges" => [ qw(rss atom log shortlog history) ],
  +); 

> +our $option = $cgi->param('option');
> +if (defined $option) {
> +	if (not grep(/^$option$/, keys %options)) {
> +		die_error(undef, "Invalid option parameter");
> +	}
> +	if (not grep(/^$action$/, @{$options{$option}})) {
> +		die_error(undef, "Invalid option parameter for this action");
> +	}
> +}

I'd rather make it possible to pass multiple additional options, for
example both '--remove-empty' (to speed up) and '--no-merges' for the
history view. So I'd use

  +our @options = $cgi->param('option');

instead. This would make option validation bit harder, but I think not that
harder. But it would also make using extra options easier: just @options
instead of (defined $option ? $option : ()).

I'd also use @extra_options, or @act_opts instead of @options as
a variable name to be more descriptive.

I'm also not sure if invalid option parameter for action should return
error, or be simply ignored. This allow to hand-edit URL, changing for
example action from 'commitdiff' to 'commit', not worrying about spurious
parameters.

By the way, gitweb uses shortened names for paramaters. Perhaps 'opt'
or 'op' instead of 'options' here and in href subroutine (below)?

> @@ -534,6 +548,7 @@ sub href(%) {
[...]
> +		option => "option",


> @@ -1770,6 +1785,7 @@ sub parse_commits {
>  		($arg ? ($arg) : ()),
>  		("--max-count=" . $maxcount),
>  		("--skip=" . $skip),
> +		((defined $option) ? ($option) : ()),
>  		$commit_id,
>  		"--",
>  		($filename ? ($filename) : ())

Very clever to put this in parse_commits subroutine...

-- 
Jakub Narebski
Poland