[PATCH] gitweb: New cgi parameter: filter

[PATCH] gitweb: New cgi parameter: filter

[Miklos Vajna]

Currently the only supported value is "nomerges". This allows one to filter
merges from many actions, like rss, log and shortlog.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 gitweb/gitweb.perl |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dc609f4..ce592cf 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -383,6 +383,13 @@ if (defined $hash_base) {
 	}
 }
 
+our $filter = $cgi->param('filter');
+if (defined $filter) {
+	if ($filter != "nomerges") {
+		die_error(undef, "Invalid filter parameter");
+	}
+}
+
 our $hash_parent_base = $cgi->param('hpb');
 if (defined $hash_parent_base) {
 	if (!validate_refname($hash_parent_base)) {
@@ -534,6 +541,7 @@ sub href(%) {
 		action => "a",
 		file_name => "f",
 		file_parent => "fp",
+		filter => "filter",
 		hash => "h",
 		hash_parent => "hp",
 		hash_base => "hb",
@@ -1770,6 +1778,7 @@ sub parse_commits {
 		($arg ? ($arg) : ()),
 		("--max-count=" . $maxcount),
 		("--skip=" . $skip),
+		((defined $filter and $filter == "nomerges") ? ("--no-merges") : ()),
 		$commit_id,
 		"--",
 		($filename ? ($filename) : ())
-- 
1.5.3.rc0.39.g46f7-dirty

Re: [PATCH] gitweb: New cgi parameter: filter

[VMiklos]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

Hello,

Na Sun, Jul 08, 2007 at 03:35:43AM +0200, Miklos Vajna <vmiklos@frugalware.org> pisal(a):
> Currently the only supported value is "nomerges". This allows one to filter
> merges from many actions, like rss, log and shortlog.
> 
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>

What do you think about this patch? Is it something that useful only for
me?

Some Frugalware users claimed about that most git-log-like command have
a --no-merges opcion but the rss feed, that's why i added such a
feature.

Thanks,

- VMiklos

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] gitweb: New cgi parameter: filter

[Jakub Narebski]

[Cc: git@vger.kernel.org]

Miklos Vajna wrote:

> +               ((defined $filter and $filter == "nomerges") ? ("--no-merges") : ()),

Shouldn't it be '$filter eq "nomerges"' instead?

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.

But that doesn't mean that this patch should be not applied... it doesn't
mean it should be applied neither ;-)
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH] gitweb: new cgi parameter: option

[Miklos Vajna]

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

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

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

 gitweb/gitweb.perl |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dc609f4..f3530ba 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -383,6 +383,20 @@ if (defined $hash_base) {
 	}
 }
 
+my %options = (
+	"--no-merges" => [('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");
+	}
+}
+
 our $hash_parent_base = $cgi->param('hpb');
 if (defined $hash_parent_base) {
 	if (!validate_refname($hash_parent_base)) {
@@ -534,6 +548,7 @@ sub href(%) {
 		action => "a",
 		file_name => "f",
 		file_parent => "fp",
+		option => "option",
 		hash => "h",
 		hash_parent => "hp",
 		hash_base => "hb",
@@ -1770,6 +1785,7 @@ sub parse_commits {
 		($arg ? ($arg) : ()),
 		("--max-count=" . $maxcount),
 		("--skip=" . $skip),
+		((defined $option) ? ($option) : ()),
 		$commit_id,
 		"--",
 		($filename ? ($filename) : ())
-- 
1.5.3.rc0.39.g46f7-dirty

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

[Jakub Narebski]

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

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

[Johannes Schindelin]

Hi,

On Thu, 12 Jul 2007, Jakub Narebski wrote:

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

It's not really difficult:

- snipsnap -
[PATCH] SubmittingPatches: mention line wrap

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

 Documentation/SubmittingPatches |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 01354c2..066e284 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -9,6 +9,8 @@ Checklist (and a short version for the impatient):
 	- provide a meaningful commit message
 	- the first line of the commit message should be a short
 	  description and should skip the full stop
+	- please use no lines longer than 76 characters, since you
+	  want to send this message as email
 	- if you want your work included in git.git, add a
 	  "Signed-off-by: Your Name <your@email.com>" line to the
 	  commit message (or just use the option "-s" when

[PATCH] gitweb: new cgi parameter: opt

[Miklos Vajna]

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

Hello,

Na Thu, Jul 12, 2007 at 12:11:32PM +0200, Jakub Narebski <jnareb@gmail.com> pisal(a):
> 'log', 'shortlog' and 'history' actions, but it can be easily extended to allow
> Micronit: it is unwritten (as of yet) requirement to word wrap commit
> message at 80 columns or less.

OK, though I think 79 is valid for "80 or less" :-)

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

As far as i see the test passes without any such warning ATM.

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

Fixed.

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

Done.

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

Changed. Also renamed %options to %allowed_options.

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

I did this way because gitweb also dies for invalid actions but it can
be removed if you don't like it.

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

Changed to 'opt'.

 gitweb/gitweb.perl |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 27580b5..13134fe 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -386,6 +386,23 @@ if (defined $hash_base) {
 	}
 }
 
+my %allowed_options = (
+	"--no-merges" => [ qw(rss atom log shortlog history) ],
+);
+
+our @extra_options = $cgi->param('opt');
+if (defined @extra_options) {
+	foreach(@extra_options)
+	{
+		if (not grep(/^$_$/, keys %allowed_options)) {
+			die_error(undef, "Invalid option parameter");
+		}
+		if (not grep(/^$action$/, @{$allowed_options{$_}})) {
+			die_error(undef, "Invalid option parameter for this action");
+		}
+	}
+}
+
 our $hash_parent_base = $cgi->param('hpb');
 if (defined $hash_parent_base) {
 	if (!validate_refname($hash_parent_base)) {
@@ -537,6 +554,7 @@ sub href(%) {
 		action => "a",
 		file_name => "f",
 		file_parent => "fp",
+		extra_options => "opt",
 		hash => "h",
 		hash_parent => "hp",
 		hash_base => "hb",
@@ -1773,6 +1791,7 @@ sub parse_commits {
 		($arg ? ($arg) : ()),
 		("--max-count=" . $maxcount),
 		("--skip=" . $skip),
+		@extra_options,
 		$commit_id,
 		"--",
 		($filename ? ($filename) : ())
-- 
1.5.3.rc0.39.g46f7-dirty

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> [*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?

I would say that would only be useful to satisfy curiosity.
Luckily or unluckily I have not had real life use of that
multiple tree comparison feature that is supported by "git
diff" (multiple blob comparison is also available, which is
mildly useful).

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

"!exists $options{$option}" ?

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

Good point.

> I'm also not sure if invalid option parameter for action should return
> error, or be simply ignored.

I'm mildly against "simply ignoring".

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

Or even 'o' ;-).

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

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:

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

'o' is used for 'order'/'ordering', to sort tables for some actions.
-- 
Jakub Narebski
Poland