threads / patch / 18876

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

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

## tl;dr

7 messages between Apr 15, 2009 and Apr 20, 2009. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Junio C Hamano· Apr 15, 2009, 06:40 UTC · lore
Holger Weiß <holger@zedat.fu-berlin.de> writes:
Show 9 quoted lines
> 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...

Show 22 quoted lines
>  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
Giuseppe Bilotta· Apr 15, 2009, 09:33 UTC · re: Junio C Hamano · lore
On Wed, Apr 15, 2009 at 8:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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
Holger Weiß· Apr 15, 2009, 10:09 UTC · re: Giuseppe Bilotta · lore
* Giuseppe Bilotta <giuseppe.bilotta@gmail.com> [2009-04-15 11:33]:
Show 19 quoted lines
> 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
Giuseppe Bilotta· Apr 15, 2009, 11:29 UTC · re: Holger Weiß · lore
2009/4/15 Holger Weiß <holger@zedat.fu-berlin.de>:
> * Giuseppe Bilotta <giuseppe.bilotta@gmail.com> [2009-04-15 11:33]:
Show 11 quoted lines
>> 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
Jakub Narebski· Apr 15, 2009, 17:34 UTC · re: Junio C Hamano · lore
On Wed, 15 April 2009, Junio C Hamano wrote:
Show 23 quoted lines
> 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. 
Show 23 quoted lines
> >  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
Jakub Narebski· Apr 20, 2009, 09:52 UTC · re: Jakub Narebski · lore

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:
Show 27 quoted lines
> 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...
Show 5 quoted lines
> 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
Junio C Hamano· Apr 20, 2009, 10:41 UTC · re: Jakub Narebski · lore
Jakub Narebski <jnareb@gmail.com> writes:
Show 9 quoted lines
> 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.

← back to recent threads