Re: [PATCH/RESEND] gitweb: Fix snapshots requested via PATH_INFO

Holger Weiß [off-list ref] writes:

Fix the detection of the requested snapshot format, which failed for
PATH_INFO URLs since the references to the hashes which describe the
supported snapshot formats weren't dereferenced appropriately.

Signed-off-by: Holger Weiß <redacted>
---
I guess this one got lost.  Without this patch, snapshots won't work if
Gitweb is configured to generate PATH_INFO URLs.  (Original Message-ID:
[off-list ref]).

The patch looks obviously correct; "our %known_snapshort_formats" maps a
name to a hashref, but the current code makes a nonsense assignment,
essentialy doing ($fmt, %opt) = ($name, $hashref), but what would I
know...  I am not using gitweb actively.

These lines come from 1ec2fb5 (gitweb: retrieve snapshot format from
PATH_INFO, 2008-11-02) by Guiseppe.

Judging from the "git shortlog -n -s --grep=PATH_INFO gitweb" output, I
think I should have heard from either Guiseppe and Jakub by now if this
patch is desired.  Pinging them...

On Wed, Apr 15, 2009 at 8:40 AM, Junio C Hamano [off-list ref] wrote:

Holger Weiß [off-list ref] writes:

quoted

Fix the detection of the requested snapshot format, which failed for
PATH_INFO URLs since the references to the hashes which describe the
supported snapshot formats weren't dereferenced appropriately.

Signed-off-by: Holger Weiß <redacted>
---
I guess this one got lost.  Without this patch, snapshots won't work if
Gitweb is configured to generate PATH_INFO URLs.  (Original Message-ID:
[off-list ref]).

The patch looks obviously correct; "our %known_snapshort_formats" maps a
name to a hashref, but the current code makes a nonsense assignment,
essentialy doing ($fmt, %opt) = ($name, $hashref), but what would I
know...  I am not using gitweb actively.

My gitweb over at http://git.oblomov.eu/ supports snapshots with
PATH_INFO just fine even without the need for this patch. Could this
be a perl version issue? My apache is using mod_perl version 2.0.4,
and I have a perl 5.10 on my system.

-- 
Giuseppe "Oblomov" Bilotta

* Giuseppe Bilotta [off-list ref] [2009-04-15 11:33]:

On Wed, Apr 15, 2009 at 8:40 AM, Junio C Hamano [off-list ref] wrote:

quoted

Holger Weiß [off-list ref] writes:

quoted

Fix the detection of the requested snapshot format, which failed for
PATH_INFO URLs since the references to the hashes which describe the
supported snapshot formats weren't dereferenced appropriately.

Signed-off-by: Holger Weiß <redacted>
---
I guess this one got lost.  Without this patch, snapshots won't work if
Gitweb is configured to generate PATH_INFO URLs.  (Original Message-ID:
[off-list ref]).

The patch looks obviously correct; "our %known_snapshort_formats" maps a
name to a hashref, but the current code makes a nonsense assignment,
essentialy doing ($fmt, %opt) = ($name, $hashref), but what would I
know...  I am not using gitweb actively.

My gitweb over at http://git.oblomov.eu/ supports snapshots with
PATH_INFO just fine even without the need for this patch.

Really?  If I try to download a snapshot from your site, I get an empty
tarball (and the server appends an additional ".tar.gz" suffix to the
filename within the "Content-disposition" header).  For example:

$ wget -q -O - http://git.oblomov.eu/git/snapshot/v1.6.2.3.tar.gz | gzip -d | tar -t | wc -l
0

That's the bug which is fixed by my patch.

Holger

2009/4/15 Holger Weiß [off-list ref]:

* Giuseppe Bilotta [off-list ref] [2009-04-15 11:33]:

quoted

My gitweb over at http://git.oblomov.eu/ supports snapshots with
PATH_INFO just fine even without the need for this patch.

Really?  If I try to download a snapshot from your site, I get an empty
tarball (and the server appends an additional ".tar.gz" suffix to the
filename within the "Content-disposition" header).  For example:

$ wget -q -O - http://git.oblomov.eu/git/snapshot/v1.6.2.3.tar.gz | gzip -d | tar -t | wc -l
0

That's the bug which is fixed by my patch.

This gets weirder and weirder. I'm seeing the same behaviour with your
patch. I think I busted something the upgrade I was running this
morning. Even git archive on the command line gives me an empty tar.

I have no idea what happened. I rebuilt and reinstalled git and now
it's working again, and I see the problem too, and yes your patch
fixes it.

-- 
Giuseppe "Oblomov" Bilotta

On Wed, 15 April 2009, Junio C Hamano wrote:

Holger Weiß [off-list ref] writes:

quoted

Fix the detection of the requested snapshot format, which failed for
PATH_INFO URLs since the references to the hashes which describe the
supported snapshot formats weren't dereferenced appropriately.

Signed-off-by: Holger Weiß <redacted>
---
I guess this one got lost.  Without this patch, snapshots won't work if
Gitweb is configured to generate PATH_INFO URLs.  (Original Message-ID:
[off-list ref]).

The patch looks obviously correct; "our %known_snapshort_formats" maps a
name to a hashref, but the current code makes a nonsense assignment,
essentialy doing ($fmt, %opt) = ($name, $hashref), but what would I
know...  I am not using gitweb actively.

These lines come from 1ec2fb5 (gitweb: retrieve snapshot format from
PATH_INFO, 2008-11-02) by Guiseppe.

Judging from the "git shortlog -n -s --grep=PATH_INFO gitweb" output, I
think I should have heard from either Guiseppe and Jakub by now if this
patch is desired.  Pinging them...

This change looks correct, and is very much desired.  Thanks for
catching this.

By the way, if there was check added for full path_info snapshot URL in
existing t/t9500-gitweb-standalone-no-errors.sh it would caught this
bug thanks to the
  "Odd number of elements in hash assignment ..."
warning that Perl throws in this case. 

quoted

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 33ef190..3f99361 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -688,10 +688,10 @@ sub evaluate_path_info {
 		# extensions. Allowed extensions are both the defined suffix
 		# (which includes the initial dot already) and the snapshot
 		# format key itself, with a prepended dot
-		while (my ($fmt, %opt) = each %known_snapshot_formats) {
+		while (my ($fmt, $opt) = each %known_snapshot_formats) {
 			my $hash = $refname;
 			my $sfx;
-			$hash =~ s/(\Q$opt{'suffix'}\E|\Q.$fmt\E)$//;
+			$hash =~ s/(\Q$opt->{'suffix'}\E|\Q.$fmt\E)$//;
 			next unless $sfx = $1;
 			# a valid suffix was found, so set the snapshot format
 			# and reset the hash parameter

-- 
1.6.2.1

-- 
Jakub Narebski
Poland

I'm sorry for resend, but I forgot to quote non-ASCII in 'Cc:'
and vger anti-SPAM filter rejected message...

Jakub Narebski [off-list ref] writes:

On Wed, 15 April 2009, Junio C Hamano wrote:

quoted

Holger Weiß [off-list ref] writes:

quoted

Fix the detection of the requested snapshot format, which failed for
PATH_INFO URLs since the references to the hashes which describe the
supported snapshot formats weren't dereferenced appropriately.

Signed-off-by: Holger Weiß <redacted>
---
I guess this one got lost.  Without this patch, snapshots won't work if
Gitweb is configured to generate PATH_INFO URLs.  (Original Message-ID:
[off-list ref]).

The patch looks obviously correct; "our %known_snapshort_formats" maps a
name to a hashref, but the current code makes a nonsense assignment,
essentialy doing ($fmt, %opt) = ($name, $hashref), but what would I
know...  I am not using gitweb actively.

These lines come from 1ec2fb5 (gitweb: retrieve snapshot format from
PATH_INFO, 2008-11-02) by Guiseppe.

Judging from the "git shortlog -n -s --grep=PATH_INFO gitweb" output, I
think I should have heard from either Guiseppe and Jakub by now if this
patch is desired.  Pinging them...

This change looks correct, and is very much desired.  Thanks for
catching this.

Ping!  This is quite straighforward bugfix for a new feature...

By the way, if there was check added for full path_info snapshot URL in
existing t/t9500-gitweb-standalone-no-errors.sh it would caught this
bug thanks to the
  "Odd number of elements in hash assignment ..."
warning that Perl throws in this case. 

... or are you waiting for test case?

-- 
Jakub Narebski
Poland
ShadeHawk on #git