[PATCH] More flexible URL patterns for gitweb

[PATCH] More flexible URL patterns for gitweb

[Damien Tournoud]

@git_base_url_list hardcodes URLs in the form "$git_base_url/$project",
which makes impossible to generate proper URLs for SSH access scenarios.

This patch implements a more flexible replacement scheme, using a
simple placeholder for the project path, and fixes the associated
documentation.

Signed-off-by: Damien Tournoud <damien@tournoud.net>
---
 gitweb/README      |   13 ++++++-------
 gitweb/gitweb.perl |    2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index ad6a04c..f16729c 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -197,13 +197,12 @@ not include variables usually directly set during build):
    full description is available as 'title' attribute (usually shown on
    mouseover).  By default set to 25, which might be too small if you
    use long project descriptions.
- * @git_base_url_list
-   List of git base URLs used for URL to where fetch project from, shown
-   in project summary page.  Full URL is "$git_base_url/$project".
-   You can setup multiple base URLs (for example one for  git:// protocol
-   access, and one for http:// "dumb" protocol access).  Note that per
-   repository configuration in 'cloneurl' file, or as values of gitweb.url
-   project config.
+ * @git_base_url_patterns
+   List of git base URLs patterns used to build the URLs displayed in the
+   project summary page. Examples include "git://git.example.com/%project",
+   "http://git.example.com/%project" and "git@git.example.com:%project".
+   Note that per repository configuration in a 'cloneurl' file, or configuration
+   values of gitweb.url in the project config take precedence over this.
  * $default_blob_plain_mimetype
    Default mimetype for blob_plain (raw) view, if mimetype checking
    doesn't result in some other type; by default 'text/plain'.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c356e95..0fc5957 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4904,7 +4904,7 @@ sub git_summary {
 	# or make project git URL from git base URL and project name
 	my $url_tag = "URL";
 	my @url_list = git_get_project_url_list($project);
-	@url_list = map { "$_/$project" } @git_base_url_list unless @url_list;
+	@url_list = map { my $url = $_; $url =~ s/%project/$project/g; $url } @git_base_url_list unless @url_list;
 	foreach my $git_url (@url_list) {
 		next unless $git_url;
 		print "<tr class=\"metadata_url\"><td>$url_tag</td><td>$git_url</td></tr>\n";
-- 
1.6.5.7

Re: [PATCH] More flexible URL patterns for gitweb

[Jakub Narebski]

Damien Tournoud <damien@tournoud.net> writes:

> @git_base_url_list hardcodes URLs in the form "$git_base_url/$project",
> which makes impossible to generate proper URLs for SSH access scenarios.

As I understand it, current way of creating project's URLs from
@git_base_url_list and $project (and project name), via joining
with '/' as separator in the form of "$git_base_url/$project"
does not work for a class of scp-like relative "URLs".  You can't
create 'git@git.example.com:path/to/project' URL using current form
of @git_base_url_list support.

Current form of @git_base_url_list support works well both for
absolute scp-like URLs for SSH sccess, for example
'git@git.example.com:/srv/git' + 'path/to/project', and also for
"ssh://" URLs like 'ssh://git.example.com/srv/git' or 
'ssh://git@git.example.com/~' + 'path/to/project'.

> 
> This patch implements a more flexible replacement scheme, using a
> simple placeholder for the project path, and fixes the associated
> documentation.

NAK, at least in this form, for it breaks backwards compatibility.
We do not want to have upgrading gitweb break one's existing gitweb
configuration.  But see below for proposed amendment of this patch.
 
> Signed-off-by: Damien Tournoud <damien@tournoud.net>
> ---
>  gitweb/README      |   13 ++++++-------
>  gitweb/gitweb.perl |    2 +-
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/gitweb/README b/gitweb/README
> index ad6a04c..f16729c 100644
> --- a/gitweb/README
> +++ b/gitweb/README
> @@ -197,13 +197,12 @@ not include variables usually directly set during build):
>     full description is available as 'title' attribute (usually shown on
>     mouseover).  By default set to 25, which might be too small if you
>     use long project descriptions.
> - * @git_base_url_list
> -   List of git base URLs used for URL to where fetch project from, shown
> -   in project summary page.  Full URL is "$git_base_url/$project".
> -   You can setup multiple base URLs (for example one for  git:// protocol
> -   access, and one for http:// "dumb" protocol access).  Note that per
> -   repository configuration in 'cloneurl' file, or as values of gitweb.url
> -   project config.
> + * @git_base_url_patterns
> +   List of git base URLs patterns used to build the URLs displayed in the
> +   project summary page. Examples include "git://git.example.com/%project",
> +   "http://git.example.com/%project" and "git@git.example.com:%project".
> +   Note that per repository configuration in a 'cloneurl' file, or configuration
> +   values of gitweb.url in the project config take precedence over this.

Here it looks like you would be using @git_base_url_patterns for the
new mechanism, while below one can see that you re-use @git_base_url_list...
and break backwards compatibility with existing gitweb configuration using
@git_base_url_list.

Also, you use %project as placeholder... while %project is valid path part
of URLs (although unlikely one).

>   * $default_blob_plain_mimetype
>     Default mimetype for blob_plain (raw) view, if mimetype checking
>     doesn't result in some other type; by default 'text/plain'.
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index c356e95..0fc5957 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4904,7 +4904,7 @@ sub git_summary {
>  	# or make project git URL from git base URL and project name
>  	my $url_tag = "URL";
>  	my @url_list = git_get_project_url_list($project);
> -	@url_list = map { "$_/$project" } @git_base_url_list unless @url_list;
> +	@url_list = map { my $url = $_; $url =~ s/%project/$project/g; $url } @git_base_url_list unless @url_list;
>  	foreach my $git_url (@url_list) {
>  		next unless $git_url;
>  		print "<tr class=\"metadata_url\"><td>$url_tag</td><td>$git_url</td></tr>\n";

To not break backward compatibility, wouldn't it be better to check if
elements of @git_base_url_list end with ':' or '/', and join base with
project path depending on this condition, i.e.:

+	@url_list = map { m/[/:]$/ ? "$_$project" : "$_/$project" } @git_base_url_list
+               unless @url_list;

This means: if base ends with colon ':' or slash '/', concatenate base
and project path, otherwise join them using '/' as field separator.
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] More flexible URL patterns for gitweb

[Damien Tournoud]

On Sun, Apr 11, 2010 at 7:33 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> To not break backward compatibility, wouldn't it be better to check if
> elements of @git_base_url_list end with ':' or '/', and join base with
> project path depending on this condition, i.e.:
>
> +       @url_list = map { m/[/:]$/ ? "$_$project" : "$_/$project" } @git_base_url_list
> +               unless @url_list;
>
> This means: if base ends with colon ':' or slash '/', concatenate base
> and project path, otherwise join them using '/' as field separator.

Thanks for the review. Indeed this way sounds better.

All this is new to me so I'm not sure what is the way forward. Should
I publish another patch or would you?

Damien Tournoud

Re: [PATCH] More flexible URL patterns for gitweb

[Jakub Narebski]

On Sun, 11 Apr 2010, Damien Tournoud wrote:
> On Sun, Apr 11, 2010 at 7:33 PM, Jakub Narebski <jnareb@gmail.com> wrote:

> > To not break backward compatibility, wouldn't it be better to check if
> > elements of @git_base_url_list end with ':' or '/', and join base with
> > project path depending on this condition, i.e.:
> >
> > +       @url_list = map { m/[/:]$/ ? "$_$project" : "$_/$project" } @git_base_url_list
> > +               unless @url_list;
> >
> > This means: if base ends with colon ':' or slash '/', concatenate base
> > and project path, otherwise join them using '/' as field separator.
> 
> Thanks for the review. Indeed this way sounds better.
> 
> All this is new to me so I'm not sure what is the way forward. Should
> I publish another patch or would you?

If you don't mind, I'd rather you send updated patch (perhaps as response
to this email, or staring a separate thread).

Note that you would need to:
1. Perhaps update first line of commit message, i.e. email subject.
   But current version might be good enough; if you don't change the
   subject you should use [PATCH v2] rather than [PATCH] in it.

2. Update commit message, to say that you are modifying the way project
   URLs shown in gitweb are composed out of bases in @git_base_url_list
   and $project, to allow possibility of generating scp-like relative
   URL for SSH access, e.g. 'git@git.example.com:user/repo.git'

3. You might want to put comment about what changed from previous
   version of the patch between "---\n" and diffstat.

4. You would need to update description of @git_base_url_list in
   gitweb/README, e.g. providing example what to put in @git_base_url_list
   to get 'git@git.example.com:user/repo.git': it is 'git@git.example.com:'

5. Update comment in gitweb.perl above declaration of @git_base_url_list
   to follow modified way of getting URL from base and $project

6. Make above change in git_summary.


P.S. Thank you for contributing to git.
-- 
Jakub Narebski
Poland