[PATCH] gitweb: action in path with use_pathinfo

[PATCH] gitweb: action in path with use_pathinfo

[Giuseppe Bilotta]

When using path info, reduce the number of CGI parameters by embedding
the action in the path. The typicial cgiweb path is thus
$project/$action/$hash_base:$file_name or $project/$action/$hash

This is mostly backwards compatible with the old-style gitweb paths,
except when $project/$branch was used to access a branch whose name
matches a gitweb action.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---

I've been thinking a long time about the best way to get rid of most
CGI parameters when using path info, and I think that the attached
patch solves the problem pretty well, since it makes most simple
(one-paramater) actions accessible as plain paths.

The only weak point I see in my approach is with -replay=>1. Since in
our case there are no CGI parameters to be replayed, I use the
corresponding variables. An alternative, safer method would probably
be to store the various request parameters in a separate hash,
obtained by merging the CGI data with the one extracted from the
request path. I didn't follow the latter route because the simpler
approach implemented in this patch seems to work without problems.

 gitweb/gitweb.perl |  111 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 73 insertions(+), 38 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index aa8c27c..d4ed401 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -512,6 +512,37 @@ if (defined $searchtext) {
 	$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
 }
 
+# dispatch
+my %actions = (
+	"blame" => \&git_blame,
+	"blobdiff" => \&git_blobdiff,
+	"blobdiff_plain" => \&git_blobdiff_plain,
+	"blob" => \&git_blob,
+	"blob_plain" => \&git_blob_plain,
+	"commitdiff" => \&git_commitdiff,
+	"commitdiff_plain" => \&git_commitdiff_plain,
+	"commit" => \&git_commit,
+	"forks" => \&git_forks,
+	"heads" => \&git_heads,
+	"history" => \&git_history,
+	"log" => \&git_log,
+	"rss" => \&git_rss,
+	"atom" => \&git_atom,
+	"search" => \&git_search,
+	"search_help" => \&git_search_help,
+	"shortlog" => \&git_shortlog,
+	"summary" => \&git_summary,
+	"tag" => \&git_tag,
+	"tags" => \&git_tags,
+	"tree" => \&git_tree,
+	"snapshot" => \&git_snapshot,
+	"object" => \&git_object,
+	# those below don't need $project
+	"opml" => \&git_opml,
+	"project_list" => \&git_project_list,
+	"project_index" => \&git_project_index,
+);
+
 # now read PATH_INFO and use it as alternative to parameters
 sub evaluate_path_info {
 	return if defined $project;
@@ -536,6 +568,16 @@ sub evaluate_path_info {
 	# do not change any parameters if an action is given using the query string
 	return if $action;
 	$path_info =~ s,^\Q$project\E/*,,;
+
+	# next comes the action
+	$action = $path_info;
+	$action =~ s,/.*$,,;
+	if (exists $actions{$action}) {
+		$path_info =~ s,^\Q$action\E/*,,;
+	} else {
+		$action  = undef;
+	}
+
 	my ($refname, $pathname) = split(/:/, $path_info, 2);
 	if (defined $pathname) {
 		# we got "project.git/branch:filename" or "project.git/branch:dir/"
@@ -549,10 +591,12 @@ sub evaluate_path_info {
 		}
 		$hash_base ||= validate_refname($refname);
 		$file_name ||= validate_pathname($pathname);
+		$hash      ||= git_get_hash_by_path($hash_base, $file_name);
 	} elsif (defined $refname) {
 		# we got "project.git/branch"
-		$action ||= "shortlog";
-		$hash   ||= validate_refname($refname);
+		$action    ||= "shortlog";
+		$hash      ||= validate_refname($refname);
+		$hash_base ||= validate_refname($refname);
 	}
 }
 evaluate_path_info();
@@ -561,37 +605,6 @@ evaluate_path_info();
 our $git_dir;
 $git_dir = "$projectroot/$project" if $project;
 
-# dispatch
-my %actions = (
-	"blame" => \&git_blame,
-	"blobdiff" => \&git_blobdiff,
-	"blobdiff_plain" => \&git_blobdiff_plain,
-	"blob" => \&git_blob,
-	"blob_plain" => \&git_blob_plain,
-	"commitdiff" => \&git_commitdiff,
-	"commitdiff_plain" => \&git_commitdiff_plain,
-	"commit" => \&git_commit,
-	"forks" => \&git_forks,
-	"heads" => \&git_heads,
-	"history" => \&git_history,
-	"log" => \&git_log,
-	"rss" => \&git_rss,
-	"atom" => \&git_atom,
-	"search" => \&git_search,
-	"search_help" => \&git_search_help,
-	"shortlog" => \&git_shortlog,
-	"summary" => \&git_summary,
-	"tag" => \&git_tag,
-	"tags" => \&git_tags,
-	"tree" => \&git_tree,
-	"snapshot" => \&git_snapshot,
-	"object" => \&git_object,
-	# those below don't need $project
-	"opml" => \&git_opml,
-	"project_list" => \&git_project_list,
-	"project_index" => \&git_project_index,
-);
-
 if (!defined $action) {
 	if (defined $hash) {
 		$action = git_get_type($hash);
@@ -649,8 +661,13 @@ sub href (%) {
 	if ($params{-replay}) {
 		while (my ($name, $symbol) = each %mapping) {
 			if (!exists $params{$name}) {
-				# to allow for multivalued params we use arrayref form
-				$params{$name} = [ $cgi->param($symbol) ];
+				if ($cgi->param($symbol)) {
+					# to allow for multivalued params we use arrayref form
+					$params{$name} = [ $cgi->param($symbol) ];
+				} else {
+					no strict 'refs';
+					$params{$name} = $$name if $$name;
+				}
 			}
 		}
 	}
@@ -661,10 +678,28 @@ sub href (%) {
 		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
 		delete $params{'project'};
 
-		# Summary just uses the project path URL
-		if (defined $params{'action'} && $params{'action'} eq 'summary') {
+		# Summary just uses the project path URL, any other action come next
+		# in the URL
+		if (defined $params{'action'}) {
+			$href .= "/".esc_url($params{'action'}) unless $params{'action'} eq 'summary';
 			delete $params{'action'};
 		}
+
+		# next, we put either hash_base:file_name or hash
+		if (defined $params{'hash_base'}) {
+			$href .= "/".esc_url($params{'hash_base'});
+			if (defined $params{'file_name'}) {
+				$href .= ":".esc_url($params{'file_name'});
+				delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});
+				delete $params{'file_name'};
+			} else {
+				delete $params{'hash'} if $params{'hash'} eq $params{'hash_base'};
+			}
+			delete $params{'hash_base'};
+		} elsif (defined $params{'hash'}) {
+			$href .= "/".esc_url($params{'hash'});
+			delete $params{'hash'};
+		}
 	}
 
 	# now encode the parameters explicitly
-- 
1.5.6.3

gitweb pathinfo improvements

[Giuseppe Bilotta]

The following patchset improves on gitweb's support for PATH_INFO
by supporting paths in the form project/action/[parent..]hash,
both in generating them and in accepting them. The old path info
style project/hash is still supported as long as it doesn't
conflict with the new style

For those that prefer git trees to patch bombs, my git tree is
available for gitweb browsing at http://git.oblomov.eu/git and for
git cloning at git://git.oblomov.eu/git/git

The changes are very local to the PATH_INFO parsing and creation,
so I hope they don't conflict with Lea's cache work.

[PATCH] gitweb: action in path with use_pathinfo

[Giuseppe Bilotta]

When using path info, reduce the number of CGI parameters by embedding
the action in the path. The typicial cgiweb path is thus
$project/$action/$hash_base:$file_name or $project/$action/$hash

This is mostly backwards compatible with the old-style gitweb paths,
except when $project/$branch was used to access a branch whose name
matches a gitweb action.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |  109 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 72 insertions(+), 37 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 29e2156..496dce4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -488,6 +488,37 @@ if (defined $searchtext) {
 	$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
 }
 
+# dispatch
+my %actions = (
+	"blame" => \&git_blame,
+	"blobdiff" => \&git_blobdiff,
+	"blobdiff_plain" => \&git_blobdiff_plain,
+	"blob" => \&git_blob,
+	"blob_plain" => \&git_blob_plain,
+	"commitdiff" => \&git_commitdiff,
+	"commitdiff_plain" => \&git_commitdiff_plain,
+	"commit" => \&git_commit,
+	"forks" => \&git_forks,
+	"heads" => \&git_heads,
+	"history" => \&git_history,
+	"log" => \&git_log,
+	"rss" => \&git_rss,
+	"atom" => \&git_atom,
+	"search" => \&git_search,
+	"search_help" => \&git_search_help,
+	"shortlog" => \&git_shortlog,
+	"summary" => \&git_summary,
+	"tag" => \&git_tag,
+	"tags" => \&git_tags,
+	"tree" => \&git_tree,
+	"snapshot" => \&git_snapshot,
+	"object" => \&git_object,
+	# those below don't need $project
+	"opml" => \&git_opml,
+	"project_list" => \&git_project_list,
+	"project_index" => \&git_project_index,
+);
+
 # now read PATH_INFO and use it as alternative to parameters
 sub evaluate_path_info {
 	return if defined $project;
@@ -512,6 +543,16 @@ sub evaluate_path_info {
 	# do not change any parameters if an action is given using the query string
 	return if $action;
 	$path_info =~ s,^\Q$project\E/*,,;
+
+	# next comes the action
+	$action = $path_info;
+	$action =~ s,/.*$,,;
+	if (exists $actions{$action}) {
+		$path_info =~ s,^\Q$action\E/*,,;
+	} else {
+		$action  = undef;
+	}
+
 	my ($refname, $pathname) = split(/:/, $path_info, 2);
 	if (defined $pathname) {
 		# we got "project.git/branch:filename" or "project.git/branch:dir/"
@@ -525,10 +566,12 @@ sub evaluate_path_info {
 		}
 		$hash_base ||= validate_refname($refname);
 		$file_name ||= validate_pathname($pathname);
+		$hash      ||= git_get_hash_by_path($hash_base, $file_name);
 	} elsif (defined $refname) {
 		# we got "project.git/branch"
-		$action ||= "shortlog";
-		$hash   ||= validate_refname($refname);
+		$action    ||= "shortlog";
+		$hash      ||= validate_refname($refname);
+		$hash_base ||= validate_refname($refname);
 	}
 }
 evaluate_path_info();
@@ -537,37 +580,6 @@ evaluate_path_info();
 our $git_dir;
 $git_dir = "$projectroot/$project" if $project;
 
-# dispatch
-my %actions = (
-	"blame" => \&git_blame,
-	"blobdiff" => \&git_blobdiff,
-	"blobdiff_plain" => \&git_blobdiff_plain,
-	"blob" => \&git_blob,
-	"blob_plain" => \&git_blob_plain,
-	"commitdiff" => \&git_commitdiff,
-	"commitdiff_plain" => \&git_commitdiff_plain,
-	"commit" => \&git_commit,
-	"forks" => \&git_forks,
-	"heads" => \&git_heads,
-	"history" => \&git_history,
-	"log" => \&git_log,
-	"rss" => \&git_rss,
-	"atom" => \&git_atom,
-	"search" => \&git_search,
-	"search_help" => \&git_search_help,
-	"shortlog" => \&git_shortlog,
-	"summary" => \&git_summary,
-	"tag" => \&git_tag,
-	"tags" => \&git_tags,
-	"tree" => \&git_tree,
-	"snapshot" => \&git_snapshot,
-	"object" => \&git_object,
-	# those below don't need $project
-	"opml" => \&git_opml,
-	"project_list" => \&git_project_list,
-	"project_index" => \&git_project_index,
-);
-
 if (!defined $action) {
 	if (defined $hash) {
 		$action = git_get_type($hash);
@@ -624,8 +636,13 @@ sub href (%) {
 	if ($params{-replay}) {
 		while (my ($name, $symbol) = each %mapping) {
 			if (!exists $params{$name}) {
-				# to allow for multivalued params we use arrayref form
-				$params{$name} = [ $cgi->param($symbol) ];
+				if ($cgi->param($symbol)) {
+					# to allow for multivalued params we use arrayref form
+					$params{$name} = [ $cgi->param($symbol) ];
+				} else {
+					no strict 'refs';
+					$params{$name} = $$name if $$name;
+				}
 			}
 		}
 	}
@@ -636,10 +653,28 @@ sub href (%) {
 		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
 		delete $params{'project'};
 
-		# Summary just uses the project path URL
-		if (defined $params{'action'} && $params{'action'} eq 'summary') {
+		# Summary just uses the project path URL, any other action come next
+		# in the URL
+		if (defined $params{'action'}) {
+			$href .= "/".esc_url($params{'action'}) unless $params{'action'} eq 'summary';
 			delete $params{'action'};
 		}
+
+		# next, we put either hash_base:file_name or hash
+		if (defined $params{'hash_base'}) {
+			$href .= "/".esc_url($params{'hash_base'});
+			if (defined $params{'file_name'}) {
+				$href .= ":".esc_url($params{'file_name'});
+				delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});
+				delete $params{'file_name'};
+			} else {
+				delete $params{'hash'} if $params{'hash'} eq $params{'hash_base'};
+			}
+			delete $params{'hash_base'};
+		} elsif (defined $params{'hash'}) {
+			$href .= "/".esc_url($params{'hash'});
+			delete $params{'hash'};
+		}
 	}
 
 	# now encode the parameters explicitly
-- 
1.5.6.5

[PATCH] gitweb: use_pathinfo filenames start with /

[Giuseppe Bilotta]

When using path info, make filenames start with a / (right after the :
that separates them from the hash base). This minimal change allows
relative navigation to work properly when viewing HTML files.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 496dce4..9d4952f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -664,7 +664,7 @@ sub href (%) {
 		if (defined $params{'hash_base'}) {
 			$href .= "/".esc_url($params{'hash_base'});
 			if (defined $params{'file_name'}) {
-				$href .= ":".esc_url($params{'file_name'});
+				$href .= ":/".esc_url($params{'file_name'});
 				delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});
 				delete $params{'file_name'};
 			} else {
-- 
1.5.6.5

[PATCH] gitweb: parse parent..current syntax from pathinfo

[Giuseppe Bilotta]

This makes it possible to use an URL such as
$project/somebranch..otherbranch:/filename to get a diff between
different version of a file. Paths like
$project/$action/somebranch:/somefile..otherbranch:/otherfile are parsed
as well.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9d4952f..3d21624 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -553,7 +553,9 @@ sub evaluate_path_info {
 		$action  = undef;
 	}
 
-	my ($refname, $pathname) = split(/:/, $path_info, 2);
+	$path_info =~ /^((.+?)(:(.+))?\.\.)?(.+?)(:(.+))?$/;
+	my ($parentrefname, $parentpathname, $refname, $pathname) = (
+		$2, $4, $5, $7);
 	if (defined $pathname) {
 		# we got "project.git/branch:filename" or "project.git/branch:dir/"
 		# we could use git_get_type(branch:pathname), but it needs $git_dir
@@ -562,7 +564,11 @@ sub evaluate_path_info {
 			$action  ||= "tree";
 			$pathname =~ s,/$,,;
 		} else {
-			$action  ||= "blob_plain";
+			if ($parentrefname) {
+				$action ||= "blobdiff_plain";
+			} else {
+				$action  ||= "blob_plain";
+			}
 		}
 		$hash_base ||= validate_refname($refname);
 		$file_name ||= validate_pathname($pathname);
@@ -573,6 +579,22 @@ sub evaluate_path_info {
 		$hash      ||= validate_refname($refname);
 		$hash_base ||= validate_refname($refname);
 	}
+	# the parent part might be missing the pathname, in which case we use the $file_name, if present
+	if (defined $parentrefname) {
+		$hash_parent_base ||= validate_refname($parentrefname);
+		if ($parentpathname) {
+			$parentpathname =~ s,^/+,,;
+			$parentpathname =~ s,/$,,;
+			$file_parent ||= validate_pathname($parentpathname);
+		} else {
+			$file_parent ||= $file_name
+		}
+		if (defined $file_parent) {
+			$hash_parent ||= git_get_hash_by_path($hash_parent_base, $file_parent);
+		} else {
+			$hash_parent ||= validate_refname($parentrefname);
+		}
+	}
 }
 evaluate_path_info();
 
-- 
1.5.6.5

[PATCH] gitweb: use_pathinfo creates parent..current paths

[Giuseppe Bilotta]

If use_pathinfo is enabled, href now creates links that contain paths in
the form $project/$action/oldhash:/oldname..newhash:/newname for actions
that use hash_parent etc.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   26 +++++++++++++++++++++++---
 1 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3d21624..573b416 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -682,9 +682,29 @@ sub href (%) {
 			delete $params{'action'};
 		}
 
-		# next, we put either hash_base:file_name or hash
+		# next, we put hash_parent_base:/file_parent..hash_base:/file_name, stripping nonexistent or useless pieces
+		$href .= "/" if ($params{'hash_base'} || $params{'hash_parent_base'} || $params{'hash_parent'} || $params{'hash'});
 		if (defined $params{'hash_base'}) {
-			$href .= "/".esc_url($params{'hash_base'});
+			if (defined $params{'hash_parent_base'}) {
+				$href .= esc_url($params{'hash_parent_base'});
+				if (defined $params{'file_parent'}) {
+					$href .= ":/".esc_url($params{'file_parent'}) unless $params{'file_parent'} eq $params{'file_name'};
+					delete $params{'hash_parent'} if $params{'hash_parent'} eq git_get_hash_by_path($params{'hash_parent_base'},$params{'file_parent'});
+					delete $params{'file_parent'};
+				} else {
+					delete $params{'hash_parent'} if $params{'hash_parent'} eq $params{'hash_parent_base'};
+					if ($params{'file_name'}) {
+						delete $params{'hash_parent'} if $params{'hash_parent'} eq git_get_hash_by_path($params{'hash_parent_base'},$params{'file_name'});
+					}
+				}
+				$href .= "..";
+				delete $params{'hash_parent_base'};
+			} elsif (defined $params{'hash_parent'}) {
+				$href .= esc_url($params{'hash_parent'}). "..";
+				delete $params{'hash_parent'};
+			}
+
+			$href .= esc_url($params{'hash_base'});
 			if (defined $params{'file_name'}) {
 				$href .= ":/".esc_url($params{'file_name'});
 				delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});
@@ -694,7 +714,7 @@ sub href (%) {
 			}
 			delete $params{'hash_base'};
 		} elsif (defined $params{'hash'}) {
-			$href .= "/".esc_url($params{'hash'});
+			$href .= esc_url($params{'hash'});
 			delete $params{'hash'};
 		}
 	}
-- 
1.5.6.5

[PATCH] gitweb: remove PATH_INFO from $my_url and $my_uri

[Giuseppe Bilotta]

This patch (combined with appropriate server configuration) allows usage
of the gitweb CGI script as DirectoryIndex for the server root even when
the pathinfo feature is enabled.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 573b416..99c891e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -26,6 +26,10 @@ our $cgi = new CGI;
 our $version = "++GIT_VERSION++";
 our $my_url = $cgi->url();
 our $my_uri = $cgi->url(-absolute => 1);
+if (my $path_info = $ENV{"PATH_INFO"}) {
+	$my_url =~ s,$path_info$,,;
+	$my_uri =~ s,$path_info$,,;
+}
 
 # core git executable to use
 # this can just be "git" if your webserver has a sensible PATH
-- 
1.5.6.5

Re: gitweb pathinfo improvements

[Giuseppe Bilotta]

Damn, I forgot the patch sequence numbers.

On Wednesday 03 September 2008 11:57, Giuseppe Bilotta wrote:
> The following patchset improves on gitweb's support for PATH_INFO
> by supporting paths in the form project/action/[parent..]hash,
> both in generating them and in accepting them. The old path info
> style project/hash is still supported as long as it doesn't
> conflict with the new style
> 
> For those that prefer git trees to patch bombs, my git tree is
> available for gitweb browsing at http://git.oblomov.eu/git and for
> git cloning at git://git.oblomov.eu/git/git
> 
> The changes are very local to the PATH_INFO parsing and creation,
> so I hope they don't conflict with Lea's cache work.

-- 
Giuseppe "Oblomov" Bilotta

Re: gitweb pathinfo improvements

[Junio C Hamano]

Please don't top post ;-).

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> Damn, I forgot the patch sequence numbers.

And please don't send a message without useful contents.  Instead of
cursing alone, after that sentence you could have said:

    Each of them is a reply to the previous one in the sequence so if your
    MUA threads correctly that is the order of the patches in the sequence,
    but in case yours doesn't, they should apply in this order:

        [PATCH 1/5] gitweb: action in path with use_pathinfo
        [PATCH 2/5] gitweb: use_pathinfo filenames start with /
        [PATCH 3/5] gitweb: parse parent..current syntax from pathinfo
        [PATCH 4/5] gitweb: use_pathinfo creates parent..current paths
        [PATCH 5/5] gitweb: remove PATH_INFO from $my_url and $my_uri

Re: gitweb pathinfo improvements

[Jakub Narebski]

Hi, Giuseppe!

I'm sorry for the delay in reviewing this series

On Wed, 3 September 2008, Giuseppe Bilotta wrote:
> 
> The following patchset improves on gitweb's support for PATH_INFO
> by supporting paths in the form project/action/[parent..]hash,
> both in generating them and in accepting them. The old path info
> style project/hash is still supported as long as it doesn't
> conflict with the new style
> 
> For those that prefer git trees to patch bombs, my git tree is
> available for gitweb browsing at http://git.oblomov.eu/git and for
> git cloning at git://git.oblomov.eu/git/git
> 
> The changes are very local to the PATH_INFO parsing and creation,
> so I hope they don't conflict with Lea's cache work.

Let me comment first on _intent_ of the patches, and on series as
a whole, before examining individual patches in detail.

Below there is table of contents / shortlog of this series, which I 
think is a good practice to include in cover letter describing the 
series:

Table of contents:
==================
 * [PATCH 1/5] gitweb: action in path with use_pathinfo
 * [PATCH 2/5] gitweb: use_pathinfo filenames start with /
 * [PATCH 3/5] gitweb: parse parent..current syntax from pathinfo
 * [PATCH 4/5] gitweb: use_pathinfo creates parent..current paths
 * [PATCH 5/5] gitweb: remove PATH_INFO from $my_url and $my_uri

This series of patches deal with moving more parameters from CGI query 
to pathinfo (beside 2nd and 5th, which are about relative navigation in 
HTML files ('blob_plain' view) and ability to act as DirectoryIndex, 
which means that they are about using gitweb as web server).

There are two sides of those pathinfo improvement: first is to be able 
to get more parameters from pathinfo, second is to use pathinfo-URLs
in gitweb links if requested/configured.

The problem with fitting more parameters in pathinfo is first backwards 
compatibility (this is not strict requirement, but we would rather not 
make existing bookmarked pathinfo URLs invalid), and second with 
ordering those parameters and detecting when one parameter ends and 
next one begins (which is made more complicated by the fact that some 
parameters, like action or hash/hash_base can be skipped).

This trouble with fitting parameters in pathinfo creates some 
limitations and tradeoffs. For example (optionally!) embedding
the action in the pathinfo, by putting it after project and before 
hash/hash_base (usually refname) and filename makes old-style 
$project/$branch lead to incorrect view. This also means that we have 
to be careful about creating pathinfo links, either by always putting 
an action, or using full ref name (which I think we do anyway to avoid 
tag/head ambiguities); or doing it only in the case of possible 
conflict i.e. branch named like one of gitweb actions.

Using ':' or ':/' to separate branch name (hash or hash_base) from 
filename doesn't lead to problems, as pathinfo is split on _first_ 
occurrence of ':', and refnames cannot contain ':'. Using '..' to 
separate $hash_parent_base:$file_parent from $hash_base:$filename
is IMVHO a very good idea... but when creating pathinfo links we have
to consider filenames with '..' in them; an example is there in git 
repository: "t/t5515/refs.master_.._.git" file. Then we probably want 
to fallback on CGI query/CGI parameters. NOTE: I have not read the 
patch yet, perhaps it does this.


By the way, this is perhaps slightly outside the issue of this series, 
but having a..b syntax would tempt to handcraft gitweb URLs for 
equivalents of "git log a..b", "git log a...b" and "git diff a...b",
neither of which works yet.

-- 
Jakub Narebski
Poland

Re: gitweb pathinfo improvements

[Giuseppe Bilotta]

On Sat, Sep 6, 2008 at 1:22 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Below there is table of contents / shortlog of this series, which I
> think is a good practice to include in cover letter describing the
> series:
>
> Table of contents:
> ==================
>  * [PATCH 1/5] gitweb: action in path with use_pathinfo
>  * [PATCH 2/5] gitweb: use_pathinfo filenames start with /
>  * [PATCH 3/5] gitweb: parse parent..current syntax from pathinfo
>  * [PATCH 4/5] gitweb: use_pathinfo creates parent..current paths
>  * [PATCH 5/5] gitweb: remove PATH_INFO from $my_url and $my_uri

Yes, I really have to learn proper behaviour when sending a patchest.
I'll make treasure of yours and Junio's hints on the matter 8-)

I'll probably have to resend this patch series anyway, since I've
already found a quirk for which an additional patch is ready, and the
double-dot-filename thing you mention below needs fixing as well.

BTW, is there a way to automate this summary generation when using
format-patch or send-email?

> The problem with fitting more parameters in pathinfo is first backwards
> compatibility (this is not strict requirement, but we would rather not
> make existing bookmarked pathinfo URLs invalid), and second with
> ordering those parameters and detecting when one parameter ends and
> next one begins (which is made more complicated by the fact that some
> parameters, like action or hash/hash_base can be skipped).
>
> This trouble with fitting parameters in pathinfo creates some
> limitations and tradeoffs. For example (optionally!) embedding
> the action in the pathinfo, by putting it after project and before
> hash/hash_base (usually refname) and filename makes old-style
> $project/$branch lead to incorrect view. This also means that we have
> to be careful about creating pathinfo links, either by always putting
> an action, or using full ref name (which I think we do anyway to avoid
> tag/head ambiguities); or doing it only in the case of possible
> conflict i.e. branch named like one of gitweb actions.

Yes, this was something that got me thinking for a while. I've tried
as hard as possible to preserve backwards compatibility, and old-style
paths should still work correctly except for projects whose branched
are named exactly like gitweb actions.

> Using ':' or ':/' to separate branch name (hash or hash_base) from
> filename doesn't lead to problems, as pathinfo is split on _first_
> occurrence of ':', and refnames cannot contain ':'. Using '..' to
> separate $hash_parent_base:$file_parent from $hash_base:$filename
> is IMVHO a very good idea... but when creating pathinfo links we have
> to consider filenames with '..' in them; an example is there in git
> repository: "t/t5515/refs.master_.._.git" file. Then we probably want
> to fallback on CGI query/CGI parameters. NOTE: I have not read the
> patch yet, perhaps it does this.

Actually, this was not a case I had taken into consideration (a
filename with two dots). It should be straightforward to change the
link-creation code to switch to CGI parameters in this case. Should I
change the corresponding patch, or would it be enough to add a patch
to the series clearing this issue?

> By the way, this is perhaps slightly outside the issue of this series,
> but having a..b syntax would tempt to handcraft gitweb URLs for
> equivalents of "git log a..b", "git log a...b" and "git diff a...b",
> neither of which works yet.

I do have a patch to that effect for the shortlog action:
http://git.oblomov.eu/git/commitdiff/refs/heads/gitweb/shortlog and
you can see it in effect on my gitweb with links such as
http://git.oblomov.eu/git/master..gitweb/pathinfo.


-- 
Giuseppe "Oblomov" Bilotta

Re: gitweb pathinfo improvements

[Jakub Narebski]

On Sat, 6 Sep 2008, Giuseppe Bilotta wrote:
> On Sat, Sep 6, 2008, Jakub Narebski <jnareb@gmail.com> wrote:

> > Below there is table of contents / shortlog of this series, which I
> > think is a good practice to include in cover letter describing the
> > series:
> >
> > Table of contents:
> > ==================
> >  * [PATCH 1/5] gitweb: action in path with use_pathinfo
> >  * [PATCH 2/5] gitweb: use_pathinfo filenames start with /
> >  * [PATCH 3/5] gitweb: parse parent..current syntax from pathinfo
> >  * [PATCH 4/5] gitweb: use_pathinfo creates parent..current paths
> >  * [PATCH 5/5] gitweb: remove PATH_INFO from $my_url and $my_uri
> 
> Yes, I really have to learn proper behaviour when sending a patchest.
> I'll make treasure of yours and Junio's hints on the matter 8-)
> 
> I'll probably have to resend this patch series anyway, since I've
> already found a quirk for which an additional patch is ready, and the
> double-dot-filename thing you mention below needs fixing as well.
> 
> BTW, is there a way to automate this summary generation when using
> format-patch or send-email?

Yes, if you have new enough git.

First, to have numbered patches (just in case mail threading screws up,
for example if emails come out of order, or if you don't use chain
reply to), you can use -n / --numbered option, or format.numbered=auto
(I think that now auto numbering is the default).

To generate template for cover letter you can use new --cover-letter
option. I don't think that there is configuration option for that.

Additionally when sending series of patches I usually output patches to 
separate directory (mdir.1, mdir.2,...) using -o <directory> option,
but that is just my workflow.

> > Using ':' or ':/' to separate branch name (hash or hash_base) from
> > filename doesn't lead to problems, as pathinfo is split on _first_
> > occurrence of ':', and refnames cannot contain ':'. Using '..' to
> > separate $hash_parent_base:$file_parent from $hash_base:$filename
> > is IMVHO a very good idea... but when creating pathinfo links we
> > have to consider filenames with '..' in them; an example is there
> > in git repository: "t/t5515/refs.master_.._.git" file. Then we
> > probably want to fallback on CGI query/CGI parameters. 
> > NOTE: I have not read the patch yet, perhaps it does this.
> 
> Actually, this was not a case I had taken into consideration (a
> filename with two dots). It should be straightforward to change the
> link-creation code to switch to CGI parameters in this case. Should I
> change the corresponding patch, or would it be enough to add a patch
> to the series clearing this issue?

I think it would be better to have it correct the first time;
especially if you are resending series anyway.

> > By the way, this is perhs slightly outside the issue of this series,
> > but having a..b syntax would tempt to handcraft gitweb URLs for
> > equivalents of "git log a..b", "git log a...b" and "git diff a...b",
> > neither of which works yet.
> 
> I do have a patch to that effect for the shortlog action:
> http://git.oblomov.eu/git/commitdiff/refs/heads/gitweb/shortlog and
> you can see it in effect on my gitweb with links such as
> http://git.oblomov.eu/git/master..gitweb/pathinfo.

I think that it would be good idea to first refactor generation of 
log-like views in git (log, shortlog, history, search, feed) to 
consolidate them somehow, so revision limiting would work on _all_ 
log-like views.

This is in my TODO list, but I didn't even began implementing it.

-- 
Jakub Narebski
Poland