Re: [PATCH v5 2/5] gitweb: add project_filter to limit project list to a subdirectory

"Bernhard R. Link" [off-list ref] writes:

I'll resend the series as replies to this mail.

Thanks; I'll queue them in 'pu' for now (if Jakub wants to Ack the pieces,
I'll amend them).

Regarding the first patch in the series, while it may be a valid perl to
introduce a new variable, assign to it and then munge its contents with
s///, all inside a parameter list of a function call, it is doing a bit
too much and makes it hard to see if the variable may or may not later be
used in the same scope (in this case, it is not).

I am tempted to squash the following in.

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b764d51..f215eaa 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -6003,7 +6003,8 @@ sub git_forks {
 		die_error(400, "Unknown order parameter");
 	}
 
-	my @list = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
+	my ($filter = $project) =~ s/\.git$//;
+	my @list = git_get_projects_list($filter);
 	if (!@list) {
 		die_error(404, "No forks found");
 	}

@@ -6062,7 +6063,8 @@ sub git_summary {
 
 	if ($check_forks) {
 		# find forks of a project
-		@forklist = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
+		my ($filter = $project) =~ s/\.git$//;
+		@forklist = git_get_projects_list($filter);
 		# filter out forks of forks
 		@forklist = filter_forks_from_projects_list(\@forklist)
 			if (@forklist);

-- 
1.7.9.154.g413bff

On Mon, 30 Jan 2012, Junio C Hamano wrote:

"Bernhard R. Link" [off-list ref] writes:

quoted

I'll resend the series as replies to this mail.

Thanks; I'll queue them in 'pu' for now (if Jakub wants to Ack the pieces,
I'll amend them).

You can add Ack from me for the whole series.

This doesn't work: it is syntax error:

  Can't declare scalar assignment in "my"
  
It has to be either

 +	(my $filter = $project) =~ s/\.git$//;

or

 +	my $filter = $project;
 +	$filter =~ s/\.git$//;

-- 
Jakub Narebski
Poland

* Junio C Hamano [off-list ref] [120130 21:34]:

"Bernhard R. Link" [off-list ref] writes:
Regarding the first patch in the series, while it may be a valid perl to
introduce a new variable, assign to it and then munge its contents with
s///, all inside a parameter list of a function call, it is doing a bit
too much and makes it hard to see if the variable may or may not later be
used in the same scope (in this case, it is not).

I'm fine either way.
I had interpreted [off-list ref] to be meant this
way, but rereading it I am not sure it was meant this way at all.
I thought this was to express that those variables are not used outside
this scope.

        Bernhard R. Link

* Junio C Hamano [off-list ref] [120130 21:34]:

Thanks; I'll queue them in 'pu' for now (if Jakub wants to Ack the pieces,
I'll amend them).

Regarding the first patch in the series, while it may be a valid perl to
introduce a new variable, assign to it and then munge its contents with
s///, all inside a parameter list of a function call, it is doing a bit
too much and makes it hard to see if the variable may or may not later be
used in the same scope (in this case, it is not).

I am tempted to squash the following in.

Look liks a change like that is actually needed. I made the mistake of
assuming
  (my $filter = $project) =~ s/\.git$//;
was the same like
  $project =~ s/\.git$//r;
but the latter returns the changed string, the former returns the number
of arguments. (So it looks for forks in a directory named '1').

(Should have tested it again after this last change)...

Can you squash it in (with the correction of Jakub Narebski), or do you
prefer a new patch?

        Bernhard R. Link