Re: [PATCH 1/2] gitweb: fix #patchNN anchors when path_info is enabled

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 17, 2011, 10:43 UTC
Message-ID: <m3hbb258pw.fsf@localhost.localdomain>
In-Reply-To: <3ef1af6874437043a4451bfbcae59b2b@localhost>

Kevin Cernekee <cernekee@gmail.com> writes:

> My configuration is as follows:

Very minor issue: Documentation/SubmittingPatches states the following:

    - describe changes in imperative mood, e.g. "make xyzzy do frotz"
      instead of "[This patch] makes xyzzy do frotz" or "[I] changed
      xyzzy to do frotz", as if you are giving orders to the codebase
      to change its behaviour.

I think this also means trying to avoid "My configuration..." and "My solution..." etc. in commit message. But this is just a side issue, not worth worrying over in my opinion; perhaps something to think about in the future.

> $feature{'pathinfo'}{'default'} = [1];

[...]

Show 9 quoted lines

> The problem is that in this configuration, PATH_INFO is used to set the
> base URL:
> 
> <base href="http://git.example.com/gitweb.cgi">
> 
> This breaks the "patch" anchor links seen on the commitdiff pages,
> because they are computed relative to the base URL:
> 
> http://git.example.com/gitweb.cgi#patch1

I think that the above configuration is enough to trigger bug / errorneous behavior that you describe, isn't it? It is better to try to find minimal way to reproduce a bug when describing it.

I guess that

Show 12 quoted lines

> <Location /gitweb>
>         Options ExecCGI
>         SetHandler cgi-script
> </Location>
> 
> GITWEB_{JS,CSS,LOGO,...} all start with gitweb-static/
> 
> gitweb.cgi renamed to /var/www/html/gitweb
> 
> This gives me simple, easy-to-read URLs that look like:
> 
> http://HOST/gitweb/myproject.git/commitdiff/0faa4a6ef921d8a233f30d66f9a3e1b24e8ec906

 
is not strictly necessary to trigger a bug.

> 
> My solution is to add an "anchor" parameter to href(), so that the full
> path is included in the patchNN links.

This is a very good idea. Thank you very much for sending this patch, and contributing to gitweb.

Its implemetation could be though improved a bit; see below.

Show 15 quoted lines

> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  gitweb/gitweb.perl |   31 +++++++++++++++++++++++++------
>  1 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1b9369d..3b6a90d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1199,6 +1199,7 @@ if (defined caller) {
>  # -full => 0|1      - use absolute/full URL ($my_uri/$my_url as base)
>  # -replay => 1      - start from a current view (replay with modifications)
>  # -path_info => 0|1 - don't use/use path_info URL (if possible)
> +# -anchor           - add #ANCHOR to end of URL

Shouldn't it be:

  +# -anchor => ANCHOR - add #ANCHOR to end of URL

Show 13 quoted lines

>  sub href {
>  	my %params = @_;
>  	# default is to use -absolute url() i.e. $my_uri
> @@ -1314,6 +1315,10 @@ sub href {
>  	# final transformation: trailing spaces must be escaped (URI-encoded)
>  	$href =~ s/(\s+)$/CGI::escape($1)/e;
>  
> +	if (defined($params{'anchor'})) {
> +		$href .= "#".esc_param($params{'anchor'});
> +	}
> +
>  	return $href;
>  }

Here you have slight mismatch between description, which uses '-anchor', and code, which uses 'anchor'.

Show 11 quoted lines

>  
> @@ -4334,8 +4339,10 @@ sub git_difftree_body {
>  			if ($action eq 'commitdiff') {
>  				# link to patch
>  				$patchno++;
> -				print "<td class=\"link\">" .
> -				      $cgi->a({-href => "#patch$patchno"}, "patch") .
> +				print $cgi->a({-href =>
> +				      href(action=>"commitdiff",
> +				      hash=>$hash, anchor=>"patch$patchno")},
> +				      "patch") .

It would be better (less error prone) and easier to use '-replay' option to href(), i.e. write

> +				print $cgi->a({-href => href(-replay=>1, -anchor=>"patch$patchno")},
> +				              "patch") .

or even make it so 'href(-anchor=>"ANCHOR")' implies '-replay => 1'. The href() part of patch would then look something like this:

  @@ -1199,6 +1199,7 @@ if (defined caller) {
   # -full => 0|1      - use absolute/full URL ($my_uri/$my_url as base)
   # -replay => 1      - start from a current view (replay with modifications)
   # -path_info => 0|1 - don't use/use path_info URL (if possible)
  +# -anchor => ANCHOR - add #ANCHOR to end of URL, implies -replay if used alone
   sub href {
   	my %params = @_;
   	# default is to use -absolute url() i.e. $my_uri
  @@ -1310,6 +1310,7 @@ sub href {
  
  	$params{'project'} = $project unless exists $params{'project'};
  
 -	if ($params{-replay}) {
 +	if ($params{-replay} ||
 +	    ($params{-anchor} && keys %params == 1)) {
  		while (my ($name, $symbol) = each %cgi_param_mapping) {
  			if (!exists $params{$name}) {
  				$params{$name} = $input_params{$name};
  @@ -1314,6 +1315,10 @@ sub href {
   	# final transformation: trailing spaces must be escaped (URI-encoded)
   	$href =~ s/(\s+)$/CGI::escape($1)/e;
   
  +	if (defined($params{'anchor'})) {
  +		$href .= "#".esc_param($params{'anchor'});
  +	}
  +
   	return $href;
   }

Do you want to resend patch with those corrections yourself, or should I do this?

-- 
Jakub Narebski
Poland
ShadeHawk on #git