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

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

[Junio C Hamano]

Holger Weiß <holger@zedat.fu-berlin.de> 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ß <holger@zedat.fu-berlin.de>
> ---
> 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:
> <20090331161636.GV30233737@CIS.FU-Berlin.DE>).

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...

>  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

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

[Giuseppe Bilotta]

On Wed, Apr 15, 2009 at 8:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Holger Weiß <holger@zedat.fu-berlin.de> 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ß <holger@zedat.fu-berlin.de>
>> ---
>> 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:
>> <20090331161636.GV30233737@CIS.FU-Berlin.DE>).
>
> 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

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

[Holger Weiß]

* Giuseppe Bilotta <giuseppe.bilotta@gmail.com> [2009-04-15 11:33]:
> On Wed, Apr 15, 2009 at 8:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Holger Weiß <holger@zedat.fu-berlin.de> 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ß <holger@zedat.fu-berlin.de>
> >> ---
> >> 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:
> >> <20090331161636.GV30233737@CIS.FU-Berlin.DE>).
> >
> > 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

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

[Giuseppe Bilotta]

2009/4/15 Holger Weiß <holger@zedat.fu-berlin.de>:
> * Giuseppe Bilotta <giuseppe.bilotta@gmail.com> [2009-04-15 11:33]:

>> 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

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

[Jakub Narebski]

On Wed, 15 April 2009, Junio C Hamano wrote:
> Holger Weiß <holger@zedat.fu-berlin.de> 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ß <holger@zedat.fu-berlin.de>
> > ---
> > 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:
> > <20090331161636.GV30233737@CIS.FU-Berlin.DE>).
> 
> 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. 

> >  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

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

[Jakub Narebski]

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

Jakub Narebski <jnareb@gmail.com> writes:
> On Wed, 15 April 2009, Junio C Hamano wrote:
>> Holger Weiß <holger@zedat.fu-berlin.de> 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ß <holger@zedat.fu-berlin.de>
>>> ---
>>> 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:
>>> <20090331161636.GV30233737@CIS.FU-Berlin.DE>).
>> 
>> 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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> 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?

Yeah, I somehow misinterpreted that you are saying you will follow up with
an additional test to cover this, and while I was waiting I got busy and
forgot.