threads / patch / 5299

patchgitweb: continue consolidation of URL generation.

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

## tl;dr

25 messages between Aug 16, 2006 and Aug 19, 2006. Diffs are folded; open one to read it.

replies: 24people: 5as markdown or json

Martin Waitz· Aug 16, 2006, 22:28 UTC · lore

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(-)
Show changes to gitweb/gitweb.perl +35 −33
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
Martin Waitz· Aug 16, 2006, 22:28 UTC · re: Martin Waitz · lore

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

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(-)
Show changes to gitweb/gitweb.perl +10 −9
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
Martin Waitz· Aug 16, 2006, 22:28 UTC · re: Martin Waitz · lore

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

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(-)
Show changes to gitweb/gitweb.perl +11 −5
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
Martin Waitz· Aug 16, 2006, 22:28 UTC · re: Martin Waitz · lore

[PATCH] gitweb: support for / as home_link.

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(-)
Show changes to gitweb/gitweb.perl +1 −1
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
Martin Waitz· Aug 16, 2006, 22:28 UTC · re: Martin Waitz · lore

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

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(-)
Show changes to gitweb/gitweb.perl +2 −1
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
Martin Waitz· Aug 16, 2006, 22:28 UTC · re: Martin Waitz · lore

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

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(-)
Show changes to gitweb/gitweb.perl +8 −9
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
Junio C Hamano· Aug 17, 2006, 02:06 UTC · re: Martin Waitz · lore

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

Martin Waitz <tali@admingilde.org> writes:
Show 14 quoted lines
> 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.

Carl Worth· Aug 17, 2006, 15:00 UTC · re: Junio C Hamano · lore

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

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

Petr Baudis· Aug 18, 2006, 13:16 UTC · re: Carl Worth · lore

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

Dear diary, on Thu, Aug 17, 2006 at 05:00:11PM CEST, I got a letter where Carl Worth <cworth@cworth.org> said that...

Show 5 quoted lines
> 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
Carl Worth· Aug 18, 2006, 14:03 UTC · re: Petr Baudis · lore

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

On Fri, 18 Aug 2006 15:16:57 +0200, Petr Baudis wrote:
Show 11 quoted lines
>
> 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
Jakub Narebski· Aug 18, 2006, 14:22 UTC · re: Carl Worth · lore

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

Carl Worth wrote:
Show 6 quoted lines
> * 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
Petr Baudis· Aug 18, 2006, 15:40 UTC · re: Carl Worth · lore

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

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

Show 6 quoted lines
> * 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
Martin Waitz· Aug 17, 2006, 19:43 UTC · re: Junio C Hamano · lore

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

hoi :)
On Wed, Aug 16, 2006 at 07:06:02PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> 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
Jakub Narebski· Aug 17, 2006, 09:41 UTC · re: Martin Waitz · lore

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

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
Martin Waitz· Aug 17, 2006, 19:49 UTC · re: Jakub Narebski · lore

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

hoi :)
On Thu, Aug 17, 2006 at 11:41:43AM +0200, Jakub Narebski wrote:
Show 8 quoted lines
> > 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
Jakub Narebski· Aug 17, 2006, 20:00 UTC · re: Martin Waitz · lore

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

Martin Waitz wrote:
Show 15 quoted lines
> 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
Jakub Narebski· Aug 17, 2006, 09:35 UTC · re: Martin Waitz · lore

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

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
Martin Waitz· Aug 17, 2006, 19:13 UTC · re: Jakub Narebski · lore

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

hoi :)
On Thu, Aug 17, 2006 at 11:35:18AM +0200, Jakub Narebski wrote:
Show 7 quoted lines
> > 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
Jakub Narebski· Aug 17, 2006, 19:34 UTC · re: Martin Waitz · lore

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

On Thu, Aug 17, 2006 at 21:13, Martin Waitz wrote:
Show 15 quoted lines
> 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
Martin Waitz· Aug 18, 2006, 20:20 UTC · re: Jakub Narebski · lore

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

hoi :)
On Thu, Aug 17, 2006 at 09:34:38PM +0200, Jakub Narebski wrote:
Show 7 quoted lines
> > 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
Jakub Narebski· Aug 19, 2006, 10:55 UTC · re: Martin Waitz · lore

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

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.
Show 7 quoted lines
> 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
Martin Waitz· Aug 19, 2006, 18:33 UTC · re: Jakub Narebski · lore

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

hoi :)
On Sat, Aug 19, 2006 at 12:55:57PM +0200, Jakub Narebski wrote:
Show 5 quoted lines
> > 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
Jakub Narebski· Aug 19, 2006, 21:44 UTC · re: Martin Waitz · lore

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

Martin Waitz wrote:
Show 15 quoted lines
> 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
Junio C Hamano· Aug 17, 2006, 01:59 UTC · re: Martin Waitz · lore

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

Martin Waitz <tali@admingilde.org> writes:
Show 23 quoted lines
> 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.
Show 9 quoted lines
> @@ -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.
Martin Waitz· Aug 17, 2006, 19:32 UTC · re: Junio C Hamano · lore

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

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

← back to recent threads