gitweb: Make the Git logo link target to point to the homepage

RFC PATCH In review Petr Baudis 4 participants 25 messages Jun 15, 2016

Share & tools

Petr Baudis

author

2016-06-15 22:42 UTC #0 lore raw

RFC PATCH 1/1 gitweb: Make the Git logo link target to point to the homepage

It provides more useful information for causual Git users than the Git docs (especially about where to get Git and such).

Signed-off-by Petr Baudis <[email protected]>

---

 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +1 -1

index c77270c..7ecd7df 100755
@@ -1312,7 +1312,7 @@ EOF
 	print "</head>\n" .
 	      "<body>\n" .
 	      "<div class=\"page_header\">\n" .
-	      "<a href=\"http://www.kernel.org/pub/software/scm/git/docs/\" title=\"git documentation\">" .
+	      "<a href=\"http://git.or.cz/\" title=\"Git homepage\">" .
 	      "<img src=\"$logo\" width=\"72\" height=\"27\" alt=\"git\" style=\"float:right; border-width:0px;\"/>" .
 	      "</a>\n";
 	print $cgi->a({-href => esc_param($home_link)}, $home_link_str) . " / ";

Petr Baudis

author

2016-06-15 22:42 UTC #1 lore raw

Dear diary, on Tue, Sep 19, 2006 at 11:27:25PM CEST, I got a letter where Petr Baudis <[email protected]> said that...

Quoted 4 lines

It provides more useful information for causual Git users than the Git docs (especially about where to get Git and such).
Signed-off-by Petr Baudis <[email protected]>

Ping? This is the only gitweb patch still in my stg stack. I guess noone really cares strongly either way since there were no comments.

Signature · 5 lines

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Junio C Hamano

2016-06-15 22:42 UTC #2 lore raw

Quoted 9 lines · Petr Baudis

Dear diary, on Tue, Sep 19, 2006 at 11:27:25PM CEST, I got a letter where Petr Baudis <[email protected]> said that...
Quoted 4 lines
It provides more useful information for causual Git users than the Git docs (especially about where to get Git and such).
Signed-off-by Petr Baudis <[email protected]>
Ping? This is the only gitweb patch still in my stg stack. I guess noone really cares strongly either way since there were no comments.

I did not care either way, but I did not like either of these hardcoded strings in the code, and felt that if we are touching that part of the code we also should be making real improvement at the same time ;-).

Doing something like this would let us update it easier, and let people override with GITWEB_CONFIG if they want to.

---

gitweb/gitweb.perl +6 -1

index 3d06181..ce90178 100755
@@ -51,6 +51,9 @@ our $logo = "++GITWEB_LOGO++";
 # URI of GIT favicon, assumed to be image/png type
 our $favicon = "++GITWEB_FAVICON++";
 
+our $githelp_url = "http://git.or.cz/";
+our $githelp_label = "git homepage";
+
 # source of projects list
 our $projects_list = "++GITWEB_LIST++";

@@ -1335,7 +1338,9 @@ EOF
 	print "</head>\n" .
 	      "<body>\n" .
 	      "<div class=\"page_header\">\n" .
-	      "<a href=\"http://www.kernel.org/pub/software/scm/git/docs/\" title=\"git documentation\">" .
+	      "<a href=\"" . esc_html($githelp_url) .
+	      "\" title=\"" . esc_html($githelp_label) .
+	      "\">" .
 	      "<img src=\"$logo\" width=\"72\" height=\"27\" alt=\"git\" style=\"float:right; border-width:0px;\"/>" .
 	      "</a>\n";
 	print $cgi->a({-href => esc_param($home_link)}, $home_link_str) . " / ";

Petr Baudis

author Acked-by

2016-06-15 22:42 UTC #3 lore raw

Dear diary, on Sat, Sep 23, 2006 at 09:36:01PM CEST, I got a letter where Junio C Hamano <[email protected]> said that...

Quoted 12 lines

Quoted 9 lines · Petr Baudis
Dear diary, on Tue, Sep 19, 2006 at 11:27:25PM CEST, I got a letter where Petr Baudis <[email protected]> said that...
Quoted 4 lines
It provides more useful information for causual Git users than the Git docs (especially about where to get Git and such).
Signed-off-by Petr Baudis <[email protected]>
Ping? This is the only gitweb patch still in my stg stack. I guess noone really cares strongly either way since there were no comments.

Quoted 7 lines

I did not care either way, but I did not like either of these hardcoded strings in the code, and felt that if we are touching that part of the code we also should be making real improvement at the same time ;-).
Doing something like this would let us update it easier, and let people override with GITWEB_CONFIG if they want to.

Reasonable.

Acked-by Petr Baudis <[email protected]>

Signature · 5 lines

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Petr Baudis

author

2016-06-15 22:42 UTC #4 lore raw

Dear diary, on Sat, Sep 23, 2006 at 09:46:43PM CEST, I got a letter where Petr Baudis <[email protected]> said that...

Quoted 19 lines

Dear diary, on Sat, Sep 23, 2006 at 09:36:01PM CEST, I got a letter where Junio C Hamano <[email protected]> said that...
Quoted 17 lines
Quoted 9 lines · Petr Baudis
Dear diary, on Tue, Sep 19, 2006 at 11:27:25PM CEST, I got a letter where Petr Baudis <[email protected]> said that...
Quoted 4 lines
It provides more useful information for causual Git users than the Git docs (especially about where to get Git and such).
Signed-off-by Petr Baudis <[email protected]>
Ping? This is the only gitweb patch still in my stg stack. I guess noone really cares strongly either way since there were no comments.
I did not care either way, but I did not like either of these hardcoded strings in the code, and felt that if we are touching that part of the code we also should be making real improvement at the same time ;-).

Quoted 6 lines

Quoted 2 lines
Doing something like this would let us update it easier, and let people override with GITWEB_CONFIG if they want to.
Reasonable.
Acked-by Petr Baudis <[email protected]>

So, what happenned to this patch?

Thanks,

Signature · 5 lines

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Junio C Hamano

2016-06-15 22:42 UTC #5 lore raw

Quoted 21 lines · Petr Baudis

Dear diary, on Sat, Sep 23, 2006 at 09:46:43PM CEST, I got a letter where Petr Baudis <[email protected]> said that...
Quoted 19 lines
Dear diary, on Sat, Sep 23, 2006 at 09:36:01PM CEST, I got a letter where Junio C Hamano <[email protected]> said that...
Quoted 17 lines
Quoted 9 lines · Petr Baudis
Dear diary, on Tue, Sep 19, 2006 at 11:27:25PM CEST, I got a letter where Petr Baudis <[email protected]> said that...
Quoted 4 lines
It provides more useful information for causual Git users than the Git docs (especially about where to get Git and such).
Signed-off-by Petr Baudis <[email protected]>
Ping? This is the only gitweb patch still in my stg stack. I guess noone really cares strongly either way since there were no comments.
I did not care either way, but I did not like either of these hardcoded strings in the code, and felt that if we are touching that part of the code we also should be making real improvement at the same time ;-).

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Doing something like this would let us update it easier, and let people override with GITWEB_CONFIG if they want to.
Reasonable.
Acked-by Petr Baudis <[email protected]>
So, what happenned to this patch?

Did anybody send in an applicable patch?

Junio C Hamano

2016-06-15 22:42 UTC #6 lore raw

Quoted 13 lines · Junio C Hamano

Quoted 11 lines · Petr Baudis
Quoted 11 lines
Quoted 10 lines
Quoted 2 lines
Ping? This is the only gitweb patch still in my stg stack. I guess noone really cares strongly either way since there were no comments.
I did not care either way, but I did not like either of these hardcoded strings in the code, and felt that if we are touching that part of the code we also should be making real improvement at the same time ;-).
Doing something like this would let us update it easier, and let people override with GITWEB_CONFIG if they want to.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Reasonable.
Acked-by Petr Baudis <[email protected]>
So, what happenned to this patch?
Did anybody send in an applicable patch?

Ah, I found it; you do not have to re-send it. But do you still want it?

Petr Baudis

author

2016-06-15 22:42 UTC #7 lore raw

Dear diary, on Thu, Oct 05, 2006 at 11:34:42PM CEST, I got a letter where Junio C Hamano <[email protected]> said that...

Quoted 17 lines

Quoted 15 lines · Junio C Hamano
Quoted 13 lines · Petr Baudis
Quoted 13 lines
Quoted 10 lines
Quoted 2 lines
Ping? This is the only gitweb patch still in my stg stack. I guess noone really cares strongly either way since there were no comments.
I did not care either way, but I did not like either of these hardcoded strings in the code, and felt that if we are touching that part of the code we also should be making real improvement at the same time ;-).
Doing something like this would let us update it easier, and let people override with GITWEB_CONFIG if they want to.
Reasonable.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Acked-by Petr Baudis <[email protected]>
So, what happenned to this patch?
Did anybody send in an applicable patch?
Ah, I found it; you do not have to re-send it. But do you still want it?

Yes. :-) (I wouldn't prod about it otherwise.)

Signature · 5 lines

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Jakub Narebski

2016-06-15 22:42 UTC #8 lore raw

In reply to Junio C Hamano

Quoted 4 lines · Junio C Hamano

-        "<a href=\"http://www.kernel.org/pub/software/scm/git/docs/\" title=\"git documentation\">" . +        "<a href=\"" . esc_html($githelp_url) .
+        "\" title=\"" . esc_html($githelp_label) .
+        "\">" .

Why not use $cgi->a_begin({-href=>esc_param($githelp_url), -title=>$githelp_label}); or just plain $cgi->a and $cgi->img?

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Junio C Hamano

2016-06-15 22:42 UTC #9 lore raw

Quoted 9 lines · Jakub Narebski

Quoted 4 lines · Junio C Hamano
-        "<a href=\"http://www.kernel.org/pub/software/scm/git/docs/\" title=\"git documentation\">" . +        "<a href=\"" . esc_html($githelp_url) .
+        "\" title=\"" . esc_html($githelp_label) .
+        "\">" .
Why not use $cgi->a_begin({-href=>esc_param($githelp_url), -title=>$githelp_label}); or just plain $cgi->a and $cgi->img?

Be my guest and send in a proper patch please.

I was merely demonstrating my preference on how definition of default values and actual use of them are separated.

Jakub Narebski

2016-06-15 22:42 UTC #10 lore raw

PATCH 1/1 gitweb: Cleanup Git logo and Git logo target generation

Rename $githelp_url and $githelp_label to $logo_url and $logo_label to be more obvious what they refer to; while at it add commented out previous contents (git documentation at kernel.org). Add comment about logo size.

Use $cgi->a(...) to generate Git logo link; it automatically escapes attribute values when it is needed. Escape href attribute using esc_url instead of (incorrect!) esc_html.

Move styling of git logo <img> element from "style" attribute to CSS via setting class to "logo". Perhaps we should set it by id rather than by class.

Signed-off-by Jakub Narebski <[email protected]>

---

Quoted 11 lines · Junio C Hamano

Quoted 6 lines
Quoted 3 lines
+ "<a href=\"" . esc_html($githelp_url) . + "\" title=\"" . esc_html($githelp_label) . + "\">" .
Why not use $cgi->a_begin({-href=>esc_param($githelp_url), -title=>$githelp_label}); or just plain $cgi->a and $cgi->img?
Be my guest and send in a proper patch please.
I was merely demonstrating my preference on how definition of default values and actual use of them are separated.

And here it is. By the way, esc_html was incorrect, I think.

 gitweb/gitweb.css  |    5 +++++
 gitweb/gitweb.perl |   17 +++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

gitweb/gitweb.css +5 -0

index 668e69a..3f62b6d 100644
@@ -16,6 +16,11 @@ a:hover, a:visited, a:active {
 	color: #880000;
 }
 
+img.logo {
+	float: right;
+	border-width: 0px;
+}
+
 div.page_header {
 	height: 25px;
 	padding: 8px;

gitweb/gitweb.perl +9 -8

index 3320069..a966f9f 100755
@@ -46,13 +46,16 @@ our $home_text = "++GITWEB_HOMETEXT++";
 
 # URI of default stylesheet
 our $stylesheet = "++GITWEB_CSS++";
-# URI of GIT logo
+# URI of GIT logo (72x27 size)
 our $logo = "++GITWEB_LOGO++";
 # URI of GIT favicon, assumed to be image/png type
 our $favicon = "++GITWEB_FAVICON++";
 
-our $githelp_url = "http://git.or.cz/";
-our $githelp_label = "git homepage";
+# URI and label (title) of GIT logo link
+#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/";
+#our $logo_label = "git documentation";
+our $logo_url = "http://git.or.cz/";
+our $logo_label = "git homepage";
 
 # source of projects list
 our $projects_list = "++GITWEB_LIST++";

@@ -1376,11 +1379,9 @@ EOF
 	print "</head>\n" .
 	      "<body>\n" .
 	      "<div class=\"page_header\">\n" .
-	      "<a href=\"" . esc_html($githelp_url) .
-	      "\" title=\"" . esc_html($githelp_label) .
-	      "\">" .
-	      "<img src=\"$logo\" width=\"72\" height=\"27\" alt=\"git\" style=\"float:right; border-width:0px;\"/>" .
-	      "</a>\n";
+	      $cgi->a({-href => esc_url($logo_url),
+	               -title => $logo_label},
+	              qq(<img src="$logo" width="72" height="27" alt="git" class="logo"/>));
 	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
 		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));

Signature · 1 line

1.4.2.1

Junio C Hamano

2016-06-15 22:42 UTC #11 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 8 lines · Jakub Narebski

Rename $githelp_url and $githelp_label to $logo_url and $logo_label to be more obvious what they refer to; while at it add commented out previous contents (git documentation at kernel.org). Add comment about logo size.
Use $cgi->a(...) to generate Git logo link; it automatically escapes attribute values when it is needed. Escape href attribute using esc_url instead of (incorrect!) esc_html.

In principle, I am not sure renaming the variables is a good idea in general. Renaming them means you are burning people who have custom help link defined in their gitweb_config.perl file, so unless the new variable names are ten times better than the current one I do not think it is worth doing.

These variables, however, have never been in any official release nor -rc yet, and have only been there for exactly two weeks, so if we are going to rename them, we'd better do so now. And the variables are about the same thing described by $logo variable, so the new names probably make more sense.

Having said that, I am wondering if it is worth doing something like this:

sub img_button {
	my $it = $_[0];
	my @attr = ();
        for my $attr (qw(src width height alt class id)) {
		next unless exists $it->{$attr};
                push @attr, "$attr=\"" . esc_attr($it->{$attr}) . '"';
	}
	my $img = '<img ' . join(' ', @attr) . ' />';
	print $cgi->a({ -href => esc_url($it->{'url'}),
        		-title => esc_attr($it->{'title'}),
                      }, $img);
}

our %logo = (

	# logo image button
	src => '++GITWEB_LOGO++',
        width => 72,
        height => 27,
	alt => 'git logo',

	# where that link leads to
        url => 'http:/git.or.cz/',
	title => 'git homepage',
);

Jakub Narebski

2016-06-15 22:42 UTC #12 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 24 lines · Junio C Hamano

Having said that, I am wondering if it is worth doing something like this:

sub img_button {
	my $it = $_[0];
	my @attr = ();
        for my $attr (qw(src width height alt class id)) {
		next unless exists $it->{$attr};
                push @attr, "$attr=\"" . esc_attr($it->{$attr}) . '"';
	}
	my $img = '<img ' . join(' ', @attr) . ' />';
	print $cgi->a({ -href => esc_url($it->{'url'}),
        		-title => esc_attr($it->{'title'}),
                      }, $img);
}

our %logo = (

	# logo image button
	src => '++GITWEB_LOGO++',
        width => 72,
        height => 27,
	alt => 'git logo',

Quoted 4 lines

	# where that link leads to
        url => 'http:/git.or.cz/',
	title => 'git homepage',
);

You forgot to add style, i.e. either id => 'logo' or class => 'logo'.

I'm not sure if it is worth complication. We have similar situation (although without image) with $home_link and $home_link_str (as with $logo, $logo_url, $logo_label).

I wonder how many people use non-standard logo image, or non standard favicon...

Signature · 2 lines

Jakub Narebski
Poland

Alan Chandler

2016-06-15 22:42 UTC #13 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 2 lines · Jakub Narebski

I wonder how many people use non-standard logo image, or non standard favicon...

I'm using a non standard favicon to make it consistent with the rest of my site.

That said, I like the revised form.

Signature · 2 lines

Alan Chandler
http://www.chandlerfamily.org.uk

Jakub Narebski

2016-06-15 22:42 UTC #14 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

In reply to Jakub Narebski

Was this patch dropped? Postponed to after release? Waiting for better patch (but this correct esc_html-ing URL...)?

Signature · 1 line

Jakub Narebski

Junio C Hamano

2016-06-15 22:42 UTC #15 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 2 lines · Jakub Narebski

Was this patch dropped? Postponed to after release? Waiting for better patch (but this correct esc_html-ing URL...)?

Pretty much the last one -- waiting for a patch directly applicable from my e-mail client. As far as I can recall there has been none in the thread.

Petr Baudis

author

2016-06-15 22:42 UTC #16 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Dear diary, on Sun, Oct 08, 2006 at 09:47:22PM CEST, I got a letter where Junio C Hamano <[email protected]> said that...

Quoted 8 lines

Quoted 2 lines · Jakub Narebski
Was this patch dropped? Postponed to after release? Waiting for better patch (but this correct esc_html-ing URL...)?
Pretty much the last one -- waiting for a patch directly applicable from my e-mail client. As far as I can recall there has been none in the thread.

Is there a problem with taking <[email protected]>?

I think it's currently not worth the complexity and breakage of backwards compatibility to do the more elaborate form you proposed.

Signature · 5 lines

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Junio C Hamano

2016-06-15 22:42 UTC #17 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 4 lines · Petr Baudis

Is there a problem with taking <[email protected]>?
I think it's currently not worth the complexity and breakage of backwards compatibility to do the more elaborate form you proposed.

I agree with that, except that the 72x27 dimension bit troubles me.

Jakub Narebski

2016-06-15 22:42 UTC #18 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 9 lines · Junio C Hamano

Quoted 4 lines · Petr Baudis
Is there a problem with taking <[email protected]>?
I think it's currently not worth the complexity and breakage of backwards compatibility to do the more elaborate form you proposed.
I agree with that, except that the 72x27 dimension bit troubles me.

First, that's the problem for the future. The 72x27 was there, I have not introduced this.

Second, 72x27 is size override, so although logo would better be 72x27, but if it is not it will be scaled to given size.
http://www.w3.org/TR/html401/struct/objects.html#visual

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Junio C Hamano

2016-06-15 22:42 UTC #19 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 6 lines · Jakub Narebski

Quoted 4 lines · Junio C Hamano
Quoted 2 lines · Petr Baudis
Is there a problem with taking <[email protected]>?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
I think it's currently not worth the complexity and breakage of backwards compatibility to do the more elaborate form you proposed.
I agree with that, except that the 72x27 dimension bit troubles me.
First, that's the problem for the future. The 72x27 was there, I have not introduced this.

That's exactly my point. This is not a "placing blame" game.

It just feels wrong to update only two things when we already know there are others that need fixing in a very similar way.

Jakub Narebski

2016-06-15 22:42 UTC #20 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 14 lines · Junio C Hamano

Quoted 12 lines · Jakub Narebski
Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Petr Baudis
Is there a problem with taking <[email protected]>?
I think it's currently not worth the complexity and breakage of backwards compatibility to do the more elaborate form you proposed.
I agree with that, except that the 72x27 dimension bit troubles me.

Quoted 7 lines

Quoted 2 lines
First, that's the problem for the future. The 72x27 was there, I have not introduced this.
That's exactly my point. This is not a "placing blame" game.
It just feels wrong to update only two things when we already know there are others that need fixing in a very similar way.

Well, I'd rather have it corrected, than wait for generalized version which would allow to set dimensions and stuff... which is most probably not needed.

Partial improvement is better than no improvement (especially as it corrects 'href="'.esc_html($githelp_url).'"' bug).

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:42 UTC #21 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 1 line · Jakub Narebski

Partial improvement is better than no improvement.

Not necessarily. As the maintainer, I found that when we say "we will fix it later", later tend to never come, and one effective way to fight that tendency is to prod the contributors a bit harder, which worked reasonably well so far.

Jakub Narebski

2016-06-15 22:42 UTC #22 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

Quoted 8 lines · Junio C Hamano

Quoted 1 line · Jakub Narebski
Partial improvement is better than no improvement.
Not necessarily. As the maintainer, I found that when we say "we will fix it later", later tend to never come, and one effective way to fight that tendency is to prod the contributors a bit harder, which worked reasonably well so far.

Well. Perhaps.

But first, it is also bugfix for esc_html on URL, and second I though you rather have one feature per patch; this one has two - cleanup of logo generation, and moving style to CSS. "Better" patch would have three...

Signature · 2 lines

Jakub Narebski
Poland

Petr Baudis

author

2016-06-15 22:42 UTC #23 lore raw

Re: [PATCH] gitweb: Cleanup Git logo and Git logo target generation

In reply to Junio C Hamano

Dear diary, on Mon, Oct 09, 2006 at 11:00:50AM CEST, I got a letter where Junio C Hamano <[email protected]> said that...

Quoted 8 lines

Quoted 1 line · Jakub Narebski
Partial improvement is better than no improvement.
Not necessarily. As the maintainer, I found that when we say "we will fix it later", later tend to never come, and one effective way to fight that tendency is to prod the contributors a bit harder, which worked reasonably well so far.

If later tends to never come, it means apparently noone really needs it. :-)

Signature · 5 lines

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Jakub Narebski

2016-06-15 22:42 UTC #24 lore raw

PATCH 1/1 gitweb: Cleanup Git logo and Git logo target generation

In reply to Junio C Hamano

Use $cgi->a(...) to generate Git logo link; it automatically escapes attribute values when it is needed. Escape href attribute using esc_url instead of (incorrect!) esc_html.

Move styling of git logo <img> element from "style" attribute to CSS via setting class to "logo". Perhaps we should set it by id rather than by class.

Signed-off-by Jakub Narebski <[email protected]>

---
I hope that this version is applicable...

 gitweb/gitweb.css  |    5 +++++
 gitweb/gitweb.perl |   17 +++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

gitweb/gitweb.css +5 -0

index 668e69a..3f62b6d 100644
@@ -16,6 +16,11 @@ a:hover, a:visited, a:active {
 	color: #880000;
 }
 
+img.logo {
+	float: right;
+	border-width: 0px;
+}
+
 div.page_header {
 	height: 25px;
 	padding: 8px;

gitweb/gitweb.perl +9 -8

index 3320069..a966f9f 100755
@@ -46,13 +46,16 @@ our $home_text = "++GITWEB_HOMETEXT++";
 
 # URI of default stylesheet
 our $stylesheet = "++GITWEB_CSS++";
-# URI of GIT logo
+# URI of GIT logo (72x27 size)
 our $logo = "++GITWEB_LOGO++";
 # URI of GIT favicon, assumed to be image/png type
 our $favicon = "++GITWEB_FAVICON++";
 
-our $githelp_url = "http://git.or.cz/";
-our $githelp_label = "git homepage";
+# URI and label (title) of GIT logo link
+#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/";
+#our $logo_label = "git documentation";
+our $logo_url = "http://git.or.cz/";
+our $logo_label = "git homepage";
 
 # source of projects list
 our $projects_list = "++GITWEB_LIST++";

@@ -1376,11 +1379,9 @@ EOF
 	print "</head>\n" .
 	      "<body>\n" .
 	      "<div class=\"page_header\">\n" .
-	      "<a href=\"" . esc_html($githelp_url) .
-	      "\" title=\"" . esc_html($githelp_label) .
-	      "\">" .
-	      "<img src=\"$logo\" width=\"72\" height=\"27\" alt=\"git\" style=\"float:right; border-width:0px;\"/>" .
-	      "</a>\n";
+	      $cgi->a({-href => esc_url($logo_url),
+	               -title => $logo_label},
+	              qq(<img src="$logo" width="72" height="27" alt="git" class="logo"/>));
 	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
 		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));

Signature · 1 line

1.4.2.1