[PATCH] gitweb: continue consolidation of URL generation.

[PATCH] gitweb: continue consolidation of URL generation.

[Martin Waitz]

Further use href() instead of URL generation by string concatenation.
Almost all functions are converted now.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |   68 +++++++++++++++++++++++++++-------------------------
 1 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 37a6284..72e687e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -204,7 +204,9 @@ sub href(%) {
 
 	my $href = "$my_uri?";
 	$href .= esc_param( join(";",
-		map { "$mapping{$_}=$params{$_}" } keys %params
+		map {
+			"$mapping{$_}=$params{$_}" if $params{$_}
+		} keys %params
 	) );
 
 	return $href;
@@ -1047,7 +1049,7 @@ sub git_footer_html {
 		}
 		print $cgi->a({-href => href(action=>"rss"), -class => "rss_logo"}, "RSS") . "\n";
 	} else {
-		print $cgi->a({-href => "$my_uri?" . esc_param("a=opml"), -class => "rss_logo"}, "OPML") . "\n";
+		print $cgi->a({-href => href(action=>"opml"), -class => "rss_logo"}, "OPML") . "\n";
 	}
 	print "</div>\n" .
 	      "</body>\n" .
@@ -1252,7 +1254,7 @@ sub git_difftree_body {
 			      "<td class=\"link\">" .
 				$cgi->a({-href => href(action=>"blob", hash=>$to_id, hash_base=>$hash, file_name=>$file)}, "blob");
 			if ($to_id ne $from_id) { # modified
-				print $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$file)}, "diff");
+				print " | " . $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$file)}, "diff");
 			}
 			print " | " . $cgi->a({-href => href(action=>"history", hash_base=>$hash, file_name=>$file)}, "history") . "\n";
 			print "</td>\n";
@@ -1663,16 +1665,16 @@ sub git_project_list {
 			print "<tr class=\"light\">\n";
 		}
 		$alternate ^= 1;
-		print "<td>" . $cgi->a({-href => "$my_uri?" . esc_param("p=$pr->{'path'};a=summary"),
+		print "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list"}, esc_html($pr->{'path'})) . "</td>\n" .
 		      "<td>" . esc_html($pr->{'descr'}) . "</td>\n" .
 		      "<td><i>" . chop_str($pr->{'owner'}, 15) . "</i></td>\n";
 		print "<td class=\"". age_class($pr->{'commit'}{'age'}) . "\">" .
 		      $pr->{'commit'}{'age_string'} . "</td>\n" .
 		      "<td class=\"link\">" .
-		      $cgi->a({-href => "$my_uri?" . esc_param("p=$pr->{'path'};a=summary")}, "summary")   . " | " .
-		      $cgi->a({-href => "$my_uri?" . esc_param("p=$pr->{'path'};a=shortlog")}, "shortlog") . " | " .
-		      $cgi->a({-href => "$my_uri?" . esc_param("p=$pr->{'path'};a=log")}, "log") .
+		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary")}, "summary")   . " | " .
+		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"shortlog")}, "shortlog") . " | " .
+		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") .
 		      "</td>\n" .
 		      "</tr>\n";
 	}
@@ -2036,11 +2038,11 @@ sub git_tree {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $hash_base);
 	git_header_html();
-	my $base_key = "";
+	my %base_key = ();
 	my $base = "";
 	my $have_blame = git_get_project_config_bool ('blame');
 	if (defined $hash_base && (my %co = parse_commit($hash_base))) {
-		$base_key = ";hb=$hash_base";
+		$base_key{hash_base} = $hash_base;
 		git_print_page_nav('tree','', $hash_base);
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
 	} else {
@@ -2071,23 +2073,23 @@ sub git_tree {
 		print "<td class=\"mode\">" . mode_str($t_mode) . "</td>\n";
 		if ($t_type eq "blob") {
 			print "<td class=\"list\">" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$t_hash$base_key;f=$base$t_name"), -class => "list"}, esc_html($t_name)) .
+			      $cgi->a({-href => href(action=>"blob", hash=>$t_hash, file_name=>"$base$t_name", %base_key), -class => "list"}, esc_html($t_name)) .
 			      "</td>\n" .
 			      "<td class=\"link\">" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$t_hash$base_key;f=$base$t_name")}, "blob");
+			      $cgi->a({-href => href(action=>"blob", hash=>$t_hash, file_name=>"$base$t_name", %base_key)}, "blob");
 			if ($have_blame) {
-				print " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blame;h=$t_hash$base_key;f=$base$t_name")}, "blame");
+				print " | " . $cgi->a({-href => href(action=>"blame", hash=>$t_hash, file_name=>"$base$t_name", %base_key)}, "blame");
 			}
-			print " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=history;h=$t_hash;hb=$hash_base;f=$base$t_name")}, "history") .
-			      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob_plain;h=$t_hash;f=$base$t_name")}, "raw") .
+			print " | " . $cgi->a({-href => href(action=>"history", hash=>$t_hash, hash_base=>$hash_base, file_name=>"$base$t_name")}, "history") .
+			      " | " . $cgi->a({-href => href(action=>"blob_plain", hash=>$t_hash, file_name=>"$base$t_name")}, "raw") .
 			      "</td>\n";
 		} elsif ($t_type eq "tree") {
 			print "<td class=\"list\">" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$t_hash$base_key;f=$base$t_name")}, esc_html($t_name)) .
+			      $cgi->a({-href => href(action=>"tree", hash=>$t_hash, file_name=>"$base$t_name", %base_key)}, esc_html($t_name)) .
 			      "</td>\n" .
 			      "<td class=\"link\">" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$t_hash$base_key;f=$base$t_name")}, "tree") .
-			      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=history;hb=$hash_base;f=$base$t_name")}, "history") .
+			      $cgi->a({-href => href(action=>"tree", hash=>$t_hash, file_name=>"$base$t_name", %base_key)}, "tree") .
+			      " | " . $cgi->a({-href => href(action=>"history", hash_base=>$hash_base, file_name=>"$base$t_name")}, "history") .
 			      "</td>\n";
 		}
 		print "</tr>\n";
@@ -2276,7 +2278,7 @@ sub git_blobdiff {
 	git_header_html();
 	if (defined $hash_base && (my %co = parse_commit($hash_base))) {
 		my $formats_nav =
-			$cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blobdiff_plain;h=$hash;hp=$hash_parent")}, "plain");
+			$cgi->a({-href => href(action=>"blobdiff_plain", hash=>$hash, hash_parent=>$hash_parent)}, "plain");
 		git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
 		git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
 	} else {
@@ -2287,9 +2289,9 @@ sub git_blobdiff {
 	git_print_page_path($file_name, "blob");
 	print "<div class=\"page_body\">\n" .
 	      "<div class=\"diff_info\">blob:" .
-	      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$hash_parent;hb=$hash_base;f=$file_name")}, $hash_parent) .
+	      $cgi->a({-href => href(action=>"blob", hash=>$hash_parent, hash_base=>$hash_base, file_name=>$file_name)}, $hash_parent) .
 	      " -> blob:" .
-	      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$hash;hb=$hash_base;f=$file_name")}, $hash) .
+	      $cgi->a({-href => href(action=>"blob", hash=>$hash, hash_base=>$hash_base, file_name=>$file_name)}, $hash) .
 	      "</div>\n";
 	git_diff_print($hash_parent, $file_name || $hash_parent, $hash, $file_name || $hash);
 	print "</div>";
@@ -2324,7 +2326,7 @@ sub git_commitdiff {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $co{'id'});
 	my $formats_nav =
-		$cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff_plain;h=$hash;hp=$hash_parent")}, "plain");
+		$cgi->a({-href => href(action=>"commitdiff_plain", hash=>$hash, hash_parent=>$hash_parent)}, "plain");
 	git_header_html(undef, $expires);
 	git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
 	git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
@@ -2367,22 +2369,22 @@ sub git_commitdiff {
 		my $file = validate_input(unquote($6));
 		if ($status eq "A") {
 			print "<div class=\"diff_info\">" . file_type($to_mode) . ":" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$to_id;hb=$hash;f=$file")}, $to_id) . "(new)" .
+			      $cgi->a({-href => href(action=>"blob", hash=>$to_id, hash_base=>$hash, file_name=>$file)}, $to_id) . "(new)" .
 			      "</div>\n";
 			git_diff_print(undef, "/dev/null", $to_id, "b/$file");
 		} elsif ($status eq "D") {
 			print "<div class=\"diff_info\">" . file_type($from_mode) . ":" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$from_id;hb=$hash_parent;f=$file")}, $from_id) . "(deleted)" .
+			      $cgi->a({-href => href(action=>"blob", hash=>$from_id, hash_base=>$hash_parent, file_name=>$file)}, $from_id) . "(deleted)" .
 			      "</div>\n";
 			git_diff_print($from_id, "a/$file", undef, "/dev/null");
 		} elsif ($status eq "M") {
 			if ($from_id ne $to_id) {
 				print "<div class=\"diff_info\">" .
 				      file_type($from_mode) . ":" .
-				      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$from_id;hb=$hash_parent;f=$file")}, $from_id) .
+				      $cgi->a({-href => href(action=>"blob", hash=>$from_id, hash_base=>$hash_parent, file_name=>$file)}, $from_id) .
 				      " -> " .
 				      file_type($to_mode) . ":" .
-				      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$to_id;hb=$hash;f=$file")}, $to_id);
+				      $cgi->a({-href => href(action=>"blob", hash=>$to_id, hash_base=>$hash, file_name=>$file)}, $to_id) .
 				print "</div>\n";
 				git_diff_print($from_id, "a/$file",  $to_id, "b/$file");
 			}
@@ -2544,7 +2546,7 @@ sub git_search {
 			print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
 			      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 5)) . "</i></td>\n" .
 			      "<td>" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$co{'id'}"), -class => "list"}, "<b>" . esc_html(chop_str($co{'title'}, 50)) . "</b><br/>");
+			      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}) -class => "list"}, "<b>" . esc_html(chop_str($co{'title'}, 50)) . "</b><br/>");
 			my $comment = $co{'comment'};
 			foreach my $line (@$comment) {
 				if ($line =~ m/^(.*)($searchtext)(.*)$/i) {
@@ -2559,8 +2561,8 @@ sub git_search {
 			}
 			print "</td>\n" .
 			      "<td class=\"link\">" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$co{'id'}")}, "commit") .
-			      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$co{'tree'};hb=$co{'id'}")}, "tree");
+			      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
+			      " | " . $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$co{'id'})}, "tree");
 			print "</td>\n" .
 			      "</tr>\n";
 		}
@@ -2597,18 +2599,18 @@ sub git_search {
 					print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
 					      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 5)) . "</i></td>\n" .
 					      "<td>" .
-					      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$co{'id'}"), -class => "list"}, "<b>" .
+					      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}), -class => "list"}, "<b>" .
 					      esc_html(chop_str($co{'title'}, 50)) . "</b><br/>");
 					while (my $setref = shift @files) {
 						my %set = %$setref;
-						print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$set{'id'};hb=$co{'id'};f=$set{'file'}"), class => "list"},
+						print $cgi->a({-href => href(action=>"blob", hash=>$set{'id'}, hash_base=>$co{'id'}, file_name=>$set{'file'}), class => "list"},
 						      "<span class=\"match\">" . esc_html($set{'file'}) . "</span>") .
 						      "<br/>\n";
 					}
 					print "</td>\n" .
 					      "<td class=\"link\">" .
-					      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$co{'id'}")}, "commit") .
-					      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$co{'tree'};hb=$co{'id'}")}, "tree");
+					      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
+					      " | " . $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$co{'id'})}, "tree");
 					print "</td>\n" .
 					      "</tr>\n";
 				}
@@ -2641,7 +2643,7 @@ sub git_shortlog {
 	my $next_link = '';
 	if ($#revlist >= (100 * ($page+1)-1)) {
 		$next_link =
-			$cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=shortlog;h=$hash;pg=" . ($page+1)),
+			$cgi->a({-href => href(action=>"shortlog", hash=>$hash, page=>$page+1),
 			         -title => "Alt-n"}, "next");
 	}
 
-- 
1.4.2.rc2.gf055

[PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Martin Waitz]

The "o" (list order) parameter was handled special.
Now it is generated with the common href() function and parsed together with
all other parameters.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 72e687e..ed6bd0e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -91,6 +91,11 @@ if (defined $action) {
 	}
 }
 
+our $order = $cgi->param('o');
+if (defined $order && $order !~ m/project|descr|owner|age/) {
+	die_error(undef, "Unknown order parameter");
+}
+
 our $project = ($cgi->param('p') || $ENV{'PATH_INFO'});
 if (defined $project) {
 	$project =~ s|^/||;
@@ -197,6 +202,7 @@ sub href(%) {
 		hash_base => "hb",
 		page => "pg",
 		searchtext => "s",
+		order => "o",
 	);
 
 	my %params = @_;
@@ -1577,11 +1583,6 @@ ## =====================================
 ## actions
 
 sub git_project_list {
-	my $order = $cgi->param('o');
-	if (defined $order && $order !~ m/project|descr|owner|age/) {
-		die_error(undef, "Unknown order parameter");
-	}
-
 	my @list = git_get_projects_list();
 	my @projects;
 	if (!@list) {
@@ -1624,7 +1625,7 @@ sub git_project_list {
 		print "<th>Project</th>\n";
 	} else {
 		print "<th>" .
-		      $cgi->a({-href => "$my_uri?" . esc_param("o=project"),
+		      $cgi->a({-href => href(order=>"project"),
 		               -class => "header"}, "Project") .
 		      "</th>\n";
 	}
@@ -1633,7 +1634,7 @@ sub git_project_list {
 		print "<th>Description</th>\n";
 	} else {
 		print "<th>" .
-		      $cgi->a({-href => "$my_uri?" . esc_param("o=descr"),
+		      $cgi->a({-href => href(order=>"descr"),
 		               -class => "header"}, "Description") .
 		      "</th>\n";
 	}
@@ -1642,7 +1643,7 @@ sub git_project_list {
 		print "<th>Owner</th>\n";
 	} else {
 		print "<th>" .
-		      $cgi->a({-href => "$my_uri?" . esc_param("o=owner"),
+		      $cgi->a({-href => href(order=>"owner"),
 		               -class => "header"}, "Owner") .
 		      "</th>\n";
 	}
@@ -1651,7 +1652,7 @@ sub git_project_list {
 		print "<th>Last Change</th>\n";
 	} else {
 		print "<th>" .
-		      $cgi->a({-href => "$my_uri?" . esc_param("o=age"),
+		      $cgi->a({-href => href(order=>"age"),
 		               -class => "header"}, "Last Change") .
 		      "</th>\n";
 	}
-- 
1.4.2.rc2.gf055

[PATCH] gitweb: support for "fp" parameter.

[Martin Waitz]

The "fp" (file name parent) parameter was previously generated for
blob diffs of renamed files.  However, it was not used in any code.

Now href() can generate "fp" parameters and they are used by the
blobdiff code to show the correct file name.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ed6bd0e..a022569 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -124,6 +124,13 @@ if (defined $file_name) {
 	}
 }
 
+our $file_parent = $cgi->param('fp');
+if (defined $file_parent) {
+	if (!validate_input($file_parent)) {
+		die_error(undef, "Invalid file parent parameter");
+	}
+}
+
 our $hash = $cgi->param('h');
 if (defined $hash) {
 	if (!validate_input($hash)) {
@@ -197,6 +204,7 @@ sub href(%) {
 		action => "a",
 		project => "p",
 		file_name => "f",
+		file_parent => "fp",
 		hash => "h",
 		hash_parent => "hp",
 		hash_base => "hb",
@@ -1282,8 +1290,7 @@ sub git_difftree_body {
 			      $cgi->a({-href => href(action=>"blob", hash=>$to_id, hash_base=>$hash, file_name=>$to_file)}, "blob");
 			if ($to_id ne $from_id) {
 				print " | " .
-				      $cgi->a({-href => "$my_uri?" .
-				              esc_param("p=$project;a=blobdiff;h=$to_id;hp=$from_id;hb=$hash;f=$to_file;fp=$from_file")}, "diff");
+				      $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$to_file, file_parent=>$from_file)}, "diff");
 			}
 			print "</td>\n";
 
@@ -1304,8 +1311,7 @@ sub git_difftree_body {
 			      $cgi->a({-href => href(action=>"blob", hash=>$to_id, hash_base=>$hash, file_name=>$to_file)}, "blob");
 			if ($to_id ne $from_id) {
 				print " | " .
-				      $cgi->a({-href => "$my_uri?" .
-				              esc_param("p=$project;a=blobdiff;h=$to_id;hp=$from_id;hb=$hash;f=$to_file;fp=$from_file")}, "diff");
+				      $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$to_file, file_parent=>$from_file)}, "diff");
 			}
 			print "</td>\n";
 		} # we should not encounter Unmerged (U) or Unknown (X) status
@@ -2290,7 +2296,7 @@ sub git_blobdiff {
 	git_print_page_path($file_name, "blob");
 	print "<div class=\"page_body\">\n" .
 	      "<div class=\"diff_info\">blob:" .
-	      $cgi->a({-href => href(action=>"blob", hash=>$hash_parent, hash_base=>$hash_base, file_name=>$file_name)}, $hash_parent) .
+	      $cgi->a({-href => href(action=>"blob", hash=>$hash_parent, hash_base=>$hash_base, file_name=>($file_parent || $file_name))}, $hash_parent) .
 	      " -> blob:" .
 	      $cgi->a({-href => href(action=>"blob", hash=>$hash, hash_base=>$hash_base, file_name=>$file_name)}, $hash) .
 	      "</div>\n";
-- 
1.4.2.rc2.gf055

[PATCH] gitweb: support for / as home_link.

[Martin Waitz]

If the webserver is configured to use gitweb even for the root directory
of the site, then my_uri is empty which leads to a non-functional home
link.  Fix that by defaulting to "/" in this case.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a022569..0a0b37f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -34,7 +34,7 @@ # location for temporary files needed fo
 our $git_temp = "/tmp/gitweb";
 
 # target of the home link on top of all pages
-our $home_link = $my_uri;
+our $home_link = $my_uri || "/";
 
 # string of the home link on top of all pages
 our $home_link_str = "++GITWEB_HOME_LINK_STR++";
-- 
1.4.2.rc2.gf055

[PATCH] gitweb: fix project list if PATH_INFO=="/".

[Martin Waitz]

The project list now uses several common header / footer generation functions.
These functions only check for "defined $project", but when PATH_INFO just
contains a "/" (which is often generated by web servers), then this test
fails.

Now explicitly undef $project if there is none so that the tests in other
gitweb parts work again.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0a0b37f..3b6333c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -100,8 +100,9 @@ our $project = ($cgi->param('p') || $ENV
 if (defined $project) {
 	$project =~ s|^/||;
 	$project =~ s|/$||;
+	$project = undef unless $project;
 }
-if (defined $project && $project) {
+if (defined $project) {
 	if (!validate_input($project)) {
 		die_error(undef, "Invalid project parameter");
 	}
-- 
1.4.2.rc2.gf055

[PATCH] gitweb: use action dispatcher for non-project actions, too.

[Martin Waitz]

Allow to use the global action dispatcher for all actions by introducing
"/actions".  If no project directory is given then a / is prepended to the
action and the normal dispatcher is called.
Project list and OPML generation are now hooked into the list as "/summary"
and "/opml".

As "/" is not an allowed character in actions accepted through the CGI
parameters, it is still ensured that no normal action can be called without
giving a valid project directory.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3b6333c..9a7faab 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -84,11 +84,6 @@ if (defined $action) {
 	if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
 		die_error(undef, "Invalid action parameter");
 	}
-	# action which does not check rest of parameters
-	if ($action eq "opml") {
-		git_opml();
-		exit;
-	}
 }
 
 our $order = $cgi->param('o');
@@ -113,9 +108,6 @@ if (defined $project) {
 		die_error(undef, "No such project");
 	}
 	$ENV{'GIT_DIR'} = "$projectroot/$project";
-} else {
-	git_project_list();
-	exit;
 }
 
 our $file_name = $cgi->param('f');
@@ -185,12 +177,19 @@ my %actions = (
 	"search" => \&git_search,
 	"shortlog" => \&git_shortlog,
 	"summary" => \&git_summary,
+	"/summary" => \&git_project_list,
+	"/opml" => \&git_opml,
 	"tag" => \&git_tag,
 	"tags" => \&git_tags,
 	"tree" => \&git_tree,
 );
 
-$action = 'summary' if (!defined($action));
+# default action
+$action ||= 'summary';
+
+#only allow non-/ actions if we have a project directory
+$action = "/$action" if !defined($project);
+
 if (!defined($actions{$action})) {
 	die_error(undef, "Unknown action");
 }
-- 
1.4.2.rc2.gf055

Re: [PATCH] gitweb: continue consolidation of URL generation.

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> Further use href() instead of URL generation by string concatenation.
> Almost all functions are converted now.
>
> Signed-off-by: Martin Waitz <tali@admingilde.org>
> ---
>  gitweb/gitweb.perl |   68 +++++++++++++++++++++++++++-------------------------
>  1 files changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 37a6284..72e687e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -204,7 +204,9 @@ sub href(%) {
>  
>  	my $href = "$my_uri?";
>  	$href .= esc_param( join(";",
> -		map { "$mapping{$_}=$params{$_}" } keys %params
> +		map {
> +			"$mapping{$_}=$params{$_}" if $params{$_}
> +		} keys %params
>  	) );
>  
>  	return $href;

This is not about "further use href()".

I think "if $params{$_}" is unsafe here.  Can you guarantee that
we will never want to pass 0 or an empty string as an parameter
value both in the current and the future code?

I think it needs to be "if defined $params{$_}", at least.

> @@ -1252,7 +1254,7 @@ sub git_difftree_body {
>  			      "<td class=\"link\">" .
>  				$cgi->a({-href => href(action=>"blob", hash=>$to_id, hash_base=>$hash, file_name=>$file)}, "blob");
>  			if ($to_id ne $from_id) { # modified
> -				print $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$file)}, "diff");
> +				print " | " . $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$file)}, "diff");
>  			}
>  			print " | " . $cgi->a({-href => href(action=>"history", hash_base=>$hash, file_name=>$file)}, "history") . "\n";
>  			print "</td>\n";

Well spotted.

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> Allow to use the global action dispatcher for all actions by introducing
> "/actions".  If no project directory is given then a / is prepended to the
> action and the normal dispatcher is called.
> Project list and OPML generation are now hooked into the list as "/summary"
> and "/opml".
>
> As "/" is not an allowed character in actions accepted through the CGI
> parameters, it is still ensured that no normal action can be called without
> giving a valid project directory.
>
> Signed-off-by: Martin Waitz <tali@admingilde.org>
> ---
>  gitweb/gitweb.perl |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)

You completely lost me.  What are you trying to achieve here?

I suspect this kind of funkies (and PATH_INFO) might be easier
to do in mod_rewrite() outside gitweb, but that is probably just
me.

Re: [PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Jakub Narebski]

Martin Waitz wrote:

> The "o" (list order) parameter was handled special.
> Now it is generated with the common href() function and parsed together with
> all other parameters.
 
This was because "o" (sort table by) parameter was used only
in one place, and valid values of order parameter depends on
the place it is used.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Jakub Narebski]

Martin Waitz wrote:

> Project list and OPML generation are now hooked into the list as
> "/summary" and "/opml".

It would be better to use "/list" or "/projects", although empty or undef or
"/" action would work as well.

Having both "/summary" (for projects list) and "summary" (for project
summary) is bad, bad idea.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Carl Worth]

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

On Wed, 16 Aug 2006 19:06:02 -0700, Junio C Hamano wrote:
>
> You completely lost me.  What are you trying to achieve here?

Presumably the goal is to take better advantage of the hierarchical
namespace provided with the path component of the URL before dropping
to the query string.

> I suspect this kind of funkies (and PATH_INFO) might be easier
> to do in mod_rewrite() outside gitweb, but that is probably just
> me.

Personally, I disagree, (at least as far as PATH_INFO---my feelings
aren't as strong for the action parameters).

A current problem I have with git is the difficulty of publishing a
new repository to the world. With our current setup on
freedesktop.org, for example, I have to publish 3 separate URLs
depending on how users might decide to interact with my repository,
(one for gitweb browsing and two separate URLs for doing git
clone/fetch/pull over either git: or http:). Much of that can now be
fixed with configuration changes on our server---but anything git can
do to simplify the amount of configuration is definitely appreciated.

-Carl

PS. A secondary problem is the difficulty of publishing a new
branch. But if I can get the 3 URLs above down to 1, then I could
probably live with the user having to figure out how to combine the
repository URL with the branch name, (in different ways depending on
what the user is doing).

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

Re: [PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Martin Waitz]

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

hoi :)

On Thu, Aug 17, 2006 at 11:35:18AM +0200, Jakub Narebski wrote:
> > The "o" (list order) parameter was handled special.
> > Now it is generated with the common href() function and parsed together with
> > all other parameters.
>  
> This was because "o" (sort table by) parameter was used only
> in one place, and valid values of order parameter depends on
> the place it is used.

But I'd really like to have all the parameter parsing and generation
code in one place.

Perhaps we can agree that only the validation should be coupled with the
actual user?  E.g. use normal validate_input() for it and then check
for actual values inside git_project_list (which is already done now).

-- 
Martin Waitz

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

Re: [PATCH] gitweb: continue consolidation of URL generation.

[Martin Waitz]

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

hoi :)

On Wed, Aug 16, 2006 at 06:59:15PM -0700, Junio C Hamano wrote:
> This is not about "further use href()".

Right.

I thought that it was needed in order to generate links to the
main project list page.  But as this is only done via $home_link
this hunk is not needed.  I just forgot to remove it again.

Should I send another patch or do you want to remove it yourself?

-- 
Martin Waitz

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

Re: [PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Jakub Narebski]

On Thu, Aug 17, 2006 at 21:13, Martin Waitz wrote:
> On Thu, Aug 17, 2006 at 11:35:18AM +0200, Jakub Narebski wrote:
>>> The "o" (list order) parameter was handled special.
>>> Now it is generated with the common href() function and parsed together with
>>> all other parameters.
>>  
>> This was because "o" (sort table by) parameter was used only
>> in one place, and valid values of order parameter depends on
>> the place it is used.
> 
> But I'd really like to have all the parameter parsing and generation
> code in one place.
> 
> Perhaps we can agree that only the validation should be coupled with the
> actual user?  E.g. use normal validate_input() for it and then check
> for actual values inside git_project_list (which is already done now).

The validate_input() function has too generic name and is too widely used:
it should be split into validate_ref() and validate_path(); perhaps "o"
should be validate with $order =~ m/^[a-zA-Z]$/ 

But I was thinking about moving parameter parsing to the "action" functions
which use them, the opposite of what you want to do...

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Martin Waitz]

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

hoi :)

On Wed, Aug 16, 2006 at 07:06:02PM -0700, Junio C Hamano wrote:
> Martin Waitz <tali@admingilde.org> writes:
> 
> > Allow to use the global action dispatcher for all actions by introducing
> > "/actions".  If no project directory is given then a / is prepended to the
> > action and the normal dispatcher is called.
> > Project list and OPML generation are now hooked into the list as "/summary"
> > and "/opml".
> >
> > As "/" is not an allowed character in actions accepted through the CGI
> > parameters, it is still ensured that no normal action can be called without
> > giving a valid project directory.
> >
> > Signed-off-by: Martin Waitz <tali@admingilde.org>
> > ---
> >  gitweb/gitweb.perl |   17 ++++++++---------
> >  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> You completely lost me.  What are you trying to achieve here?

I just want to separate all the URL generation/parsing from the rest
of the code.
So I removed the opml/project list calls from the parsing code.

The target is to be able to be able to replace it without influencing
other parts of the code.

> I suspect this kind of funkies (and PATH_INFO) might be easier
> to do in mod_rewrite() outside gitweb, but that is probably just
> me.

Well, I think that the URL shown in the browser is one important
part of the user interface of a web page. So we should not rely
on the system administrator to implement it on his own.

-- 
Martin Waitz

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

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Martin Waitz]

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

hoi :)

On Thu, Aug 17, 2006 at 11:41:43AM +0200, Jakub Narebski wrote:
> > Project list and OPML generation are now hooked into the list as
> > "/summary" and "/opml".
> 
> It would be better to use "/list" or "/projects", although empty or undef or
> "/" action would work as well.
> 
> Having both "/summary" (for projects list) and "summary" (for project
> summary) is bad, bad idea.

well, it was a little trick so that only one default action "summary"
is needed.  I have no strong feelings about it so any other name is
good for me, too.

-- 
Martin Waitz

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

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Jakub Narebski]

Martin Waitz wrote:

> hoi :)
> 
> On Thu, Aug 17, 2006 at 11:41:43AM +0200, Jakub Narebski wrote:
>> > Project list and OPML generation are now hooked into the list as
>> > "/summary" and "/opml".
>> 
>> It would be better to use "/list" or "/projects", although empty or 
>> undef or "/" action would work as well.
>> 
>> Having both "/summary" (for projects list) and "summary" (for project
>> summary) is bad, bad idea.
> 
> well, it was a little trick so that only one default action "summary"
> is needed.  I have no strong feelings about it so any other name is
> good for me, too.

if (defined $project) { # with the patch that undef $project when empty
        $action ||= "summary";
} else {
        $action ||= "/projects_list"; # or simply "projects_list", or "/"
}

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Petr Baudis]

Dear diary, on Thu, Aug 17, 2006 at 05:00:11PM CEST, I got a letter
where Carl Worth <cworth@cworth.org> said that...
> PS. A secondary problem is the difficulty of publishing a new
> branch. But if I can get the 3 URLs above down to 1, then I could
> probably live with the user having to figure out how to combine the
> repository URL with the branch name, (in different ways depending on
> what the user is doing).

Could you be more specific here, please? Is it just that Git won't
automagically pick up new branches published on the server?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Carl Worth]

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

On Fri, 18 Aug 2006 15:16:57 +0200, Petr Baudis wrote:
>
> Dear diary, on Thu, Aug 17, 2006 at 05:00:11PM CEST, I got a letter
> where Carl Worth <cworth@cworth.org> said that...
> > PS. A secondary problem is the difficulty of publishing a new
> > branch. But if I can get the 3 URLs above down to 1, then I could
> > probably live with the user having to figure out how to combine the
> > repository URL with the branch name, (in different ways depending on
> > what the user is doing).
>
> Could you be more specific here, please? Is it just that Git won't
> automagically pick up new branches published on the server?

The best I've been able to come up with is:

	See my new branch <branch> at <URL>

Then it's up to the user to do one of the following (depending on what
they want):

1. For gitweb, either:

  a. Browse to <URL> and manually find <branch> in the list (ugh!)

  b. Manually construct a branch-specific URL: <URL>;h=<branch>

2. For git fetch/clone:

  Construct a command line looking something like this:

	git fetch <URL> <branch>:tracking-branch

  and optionally do some manual editing of a config file if they want
  to easily get at this branch again in the future.

And that assumes I can give out a single <URL> in the first place, of
course.

Beyond that, here are some of the things I would like:

* A single thingy I could publish for <URL> and <branch>. Ideally this
  would have no whitespace in it and would be directly
  cut-and-paste-able for use in either gitweb or the git command-line.

* A mechanism for not requiring me to invent tracking-branch names.  I
  end up manually doing a scheme with short prefixes for any given
  repository. It would be nice if I could configure that (in
  .git/remotes/<something> say) once so that any new branch I pulled
  would get its properly named tracking branch. And git could find the
  right remotes file by matching up the URL.

* An easy way, (maybe a one-letter command-line option), for stashing
  a branch I'm fetching off into .git/remotes/<something> where again,
  it would find the right file by matching the URL. This option might
  also let me/help me name <something> if this is the first time I'm
  fetching from a particular URL.

-Carl

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

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Jakub Narebski]

Carl Worth wrote:

> * A mechanism for not requiring me to invent tracking-branch names.  I
>   end up manually doing a scheme with short prefixes for any given
>   repository. It would be nice if I could configure that (in
>   .git/remotes/<something> say) once so that any new branch I pulled
>   would get its properly named tracking branch. And git could find the
>   right remotes file by matching up the URL.

This mechanism is called 'git clone --use-separate-remote'.

  --use-separate-remote
     Save remotes  heads under $GIT_DIR/remotes/origin/ instead  of
     $GIT_DIR/refs/heads/. Only the master branch is saved in the latter.
     (as origin).

(Shouldn't it be $GIT_DIT/refs/remotes/<repository>/ instead, BTW?)
And of course you can configure pull lines in the following way:

  Pull: refs/heads/<branch>:refs/remotes/<repository>/<branch>

Unfortunately, there is no way to automatically pull new refs and tags
in such a way.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: use action dispatcher for non-project actions, too.

[Petr Baudis]

Dear diary, on Fri, Aug 18, 2006 at 04:03:29PM CEST, I got a letter
where Carl Worth <cworth@cworth.org> said that...
> * A single thingy I could publish for <URL> and <branch>. Ideally this
>   would have no whitespace in it and would be directly
>   cut-and-paste-able for use in either gitweb or the git command-line.

Perhaps you could paste a fetch URL to some gitweb inputbox at the top
of every page and it would show the right thing, sort of like mldonkey
with ed2k URLs...

> * A mechanism for not requiring me to invent tracking-branch names.  I
>   end up manually doing a scheme with short prefixes for any given
>   repository. It would be nice if I could configure that (in
>   .git/remotes/<something> say) once so that any new branch I pulled
>   would get its properly named tracking branch. And git could find the
>   right remotes file by matching up the URL.

We already have .git/refs/remote/ where the tracking branches are just
the same name as on the other side, we only need to start really using
it.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam

Re: [PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Martin Waitz]

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

hoi :)

On Thu, Aug 17, 2006 at 09:34:38PM +0200, Jakub Narebski wrote:
> > Perhaps we can agree that only the validation should be coupled with the
> > actual user?  E.g. use normal validate_input() for it and then check
> > for actual values inside git_project_list (which is already done now).
> 
> The validate_input() function has too generic name and is too widely used:
> it should be split into validate_ref() and validate_path(); perhaps "o"
> should be validate with $order =~ m/^[a-zA-Z]$/ 

agreed.

> But I was thinking about moving parameter parsing to the "action" functions
> which use them, the opposite of what you want to do...

but only short-term.

I think we both agree on the same target:
we need some simple to pass parameters to a function which should only
be called if the user clicks on a specific link.

Now lets talk about the interface to do this.

We need one interface for generating the URL (stub in RPC talk)
and one for calling the function once the link is clicked (skeleton).
We now have the href() function which is not so bad for the stub side.
We now need a nice generic skeleton.

Perhaps introduce a new function which is used to access the parameters?
This new function could check the URL or CGI->param or whatever and then
return the requested value.

Then the action functions could get all parameters they need, validate
them themselves and then act on them.
This would suit my "break out parameter parsing from actions" and your
"validate parameters in the action function".
(And I really interpret your sentence in such a way that you only want
to move the _validation_, not the actual parsing (which is done inside
CGI->param at the moment.)

-- 
Martin Waitz

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

Re: [PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Jakub Narebski]

Martin Waitz wrote:

> Perhaps introduce a new function which is used to access the parameters?
> This new function could check the URL or CGI->param or whatever and then
> return the requested value.

CGI->param. There is no reason to duplicate CGI module.

> Then the action functions could get all parameters they need, validate
> them themselves and then act on them.
> This would suit my "break out parameter parsing from actions" and your
> "validate parameters in the action function".
> (And I really interpret your sentence in such a way that you only want
> to move the _validation_, not the actual parsing (which is done inside
> CGI->param at the moment.)

Validation and accessing. Parsing via CGI->param.

There is another reason I want to move accessing and validation of
parameters to the "action" function (perhaps, as you suggested, via
"wrapper" function), namely removing some global variables. Perhaps it is
unnecessary, but I wonder how well that works with mod_perl. Other global
variables are configuration and do not change form execution to execution
of script...

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Martin Waitz]

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

hoi :)

On Sat, Aug 19, 2006 at 12:55:57PM +0200, Jakub Narebski wrote:
> > Perhaps introduce a new function which is used to access the parameters?
> > This new function could check the URL or CGI->param or whatever and then
> > return the requested value.
> 
> CGI->param. There is no reason to duplicate CGI module.

yes there is.
using CGI->param it is not possible to use nice URLs ala
http://git.site.org/projectpath.git

I would really appreciate to be able to use nice URLs in gitweb that
correspond to the repository URL.

-- 
Martin Waitz

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

Re: [PATCH] gitweb: use common parameter parsing and generation for "o", too.

[Jakub Narebski]

Martin Waitz wrote:

> hoi :)
> 
> On Sat, Aug 19, 2006 at 12:55:57PM +0200, Jakub Narebski wrote:
>>> Perhaps introduce a new function which is used to access the parameters?
>>> This new function could check the URL or CGI->param or whatever and then
>>> return the requested value.
>> 
>> CGI->param. There is no reason to duplicate CGI module.
> 
> yes there is.
> using CGI->param it is not possible to use nice URLs ala
> http://git.site.org/projectpath.git
> 
> I would really appreciate to be able to use nice URLs in gitweb that
> correspond to the repository URL.

As far as I know it is now possible (of course if your web server is
configured to redirect / to gitweb.cgi, e.g. for Apache using ScriptAlias
directive) via PATH_INFO support. At least it is what is intended.

Gitweb doesn't generate links in such a way, yet, but this should be fairly
easy now, changing only href() subroutine.

But apart from this one parameter the rest have to be passed as CGI
parameters,
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git