Kevin Cernekee <cernekee@xxxxxxxxx> 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];
[...]
> 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
> <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.
>
> Signed-off-by: Kevin Cernekee <cernekee@xxxxxxxxx>
> ---
> 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
> 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'.
>
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html