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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 12, 2007, 10:11 UTC
Message-ID: <200707121211.32813.jnareb@gmail.com>
In-Reply-To: <20070711230038.GN19386@genesis.frugalware.org>

On Thu, 12 Jul 2007, Miklos Vajna wrote:

Show 5 quoted lines

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

Show 10 quoted lines

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

Show 5 quoted lines

>> 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) ],
  +);

Show 9 quoted lines

> +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",

Show 8 quoted lines

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