# [RFC/PATCH 1/3] gitweb: make suspenders more useful 5 messages from 2009-08-23 to 2009-08-24. Participants: Mark Rada, Sam Vilain. Thread: https://gitlist.dev/t/20710 ## Mark Rada, 2009-08-23 21:53 Subject: [RFC/PATCH 1/3] gitweb: make suspenders more useful Message-ID: <4A91BA66.5050108@mailservices.uwaterloo.ca> URL: https://gitlist.dev/e/4A91BA66.5050108%40mailservices.uwaterloo.ca ``` In the first block of checks to validate a snapshot request, the last check is never executed because the second last check is a superset of the last check. This change will switch the order of the last two checks, it has the advantage of giving clients a more specific reason why they cannot get a specific snapshot format instead of giving them the more generic response. Signed-off-by: Mark Rada --- gitweb/gitweb.perl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 4a42f61..7068db2 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -5174,10 +5174,10 @@ sub git_snapshot { die_error(400, "Invalid snapshot format parameter"); } elsif (!exists($known_snapshot_formats{$format})) { die_error(400, "Unknown snapshot format"); - } elsif (!grep($_ eq $format, @snapshot_fmts)) { - die_error(403, "Unsupported snapshot format"); } elsif ($known_snapshot_formats{$format}{'disabled'}) { die_error(403, "Snapshot format not allowed"); + } elsif (!grep($_ eq $format, @snapshot_fmts)) { + die_error(403, "Unsupported snapshot format"); } if (!defined $hash) { -- Mark A Rada (ferrous26) marada@uwaterloo.ca ``` ## Sam Vilain, 2009-08-23 22:10 Subject: Re: [RFC/PATCH 1/3] gitweb: make suspenders more useful Message-ID: <4A91BE50.7070103@vilain.net> URL: https://gitlist.dev/e/4A91BE50.7070103%40vilain.net In-Reply-To: <4A91BA66.5050108@mailservices.uwaterloo.ca> ``` Mark Rada wrote: > Subject: [RFC/PATCH 1/3] gitweb: make suspenders more useful Suspenders? Really? Sam ``` ## Mark Rada, 2009-08-23 22:39 Subject: Re: [RFC/PATCH 1/3] gitweb: make suspenders more useful Message-ID: <4A91C512.7000707@mailservices.uwaterloo.ca> URL: https://gitlist.dev/e/4A91C512.7000707%40mailservices.uwaterloo.ca In-Reply-To: <4A91BE50.7070103@vilain.net> ``` On 23/08/09 6:10 PM, Sam Vilain wrote: > Mark Rada wrote: >> Subject: [RFC/PATCH 1/3] gitweb: make suspenders more useful > > Suspenders? Really? > > Sam > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Context: On 21/08/09 5:43 PM, Junio C Hamano wrote: > Mark A Rada writes: > >> Unless I missed a case, the tests show that the extra condition check >> that was added in the &git_snapshot routine is never actually executed, >> because a disabled snapshot format is not added to @snapshot_fmts, which >> is checked first. >> >> snippet: >> 5178 } elsif (!grep($_ eq $format, @snapshot_fmts)) { >> 5179 die_error(403, "Unsupported snapshot format"); >> 5180 } elsif ($known_snapshot_formats{$format}{'disabled'}) { >> 5181 die_error(403, "Snapshot format not allowed"); >> 5182 } >> 5183 > > True; filter_snapshot_fmts looks at 'disabled' first. > > I do not mind keeping these two lines as belt-and-suspender, though. > Yup...I couldn't think of an appropriate title and I expect this patch to be edited (for another reason) or thrown out anyways. -- Mark A Rada (ferrous26) marada@uwaterloo.ca ``` ## Sam Vilain, 2009-08-23 22:48 Subject: Re: [RFC/PATCH 1/3] gitweb: make suspenders more useful Message-ID: <4A91C74D.1080908@vilain.net> URL: https://gitlist.dev/e/4A91C74D.1080908%40vilain.net In-Reply-To: <4A91C512.7000707@mailservices.uwaterloo.ca> ``` Mark Rada wrote: >>> Subject: [RFC/PATCH 1/3] gitweb: make suspenders more useful >>> >> Suspenders? Really? >> > Context: > > On 21/08/09 5:43 PM, Junio C Hamano wrote: > >> I do not mind keeping these two lines as belt-and-suspender, though. >> > Yup...I couldn't think of an appropriate title and I expect this > patch to be edited (for another reason) or thrown out anyways. > Please don't do that. Use of metaphors makes the code harder to follow, especially for non-native speakers. In this case you confused me, a British/NZ native speaker because you only used half of the idiom; so all I was left with was a word which in the English where I speak refers to a piece of clothing worn most often by prostitutes and people at the Rocky Horror Picture Show. Can I suggest "snapshot error handling" as a more neutral term... Sam ``` ## Mark Rada, 2009-08-24 03:09 Subject: [RFC/PATCH 1/3] gitweb: improve snapshot error handling Message-ID: <4A920470.1090709@mailservices.uwaterloo.ca> URL: https://gitlist.dev/e/4A920470.1090709%40mailservices.uwaterloo.ca In-Reply-To: <4A91C74D.1080908@vilain.net> ``` Ok, this is just a resend of the first patch in my set from earlier, but with some of the commit message reworded, namely the subject (as suggested by Sam Vilain). -- Mark Rada (ferrous26) marada@uwaterloo.ca --->8--- In the second block of checks to validate a snapshot request, the last check is never executed because the second last check is a superset of the last check. This change will switch the order of the last two checks, it has the advantage of giving clients a more specific reason why they cannot get a snapshot format if the format they have chosen is disabled. Signed-off-by: Mark Rada --- gitweb/gitweb.perl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 4a42f61..7068db2 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -5174,10 +5174,10 @@ sub git_snapshot { die_error(400, "Invalid snapshot format parameter"); } elsif (!exists($known_snapshot_formats{$format})) { die_error(400, "Unknown snapshot format"); - } elsif (!grep($_ eq $format, @snapshot_fmts)) { - die_error(403, "Unsupported snapshot format"); } elsif ($known_snapshot_formats{$format}{'disabled'}) { die_error(403, "Snapshot format not allowed"); + } elsif (!grep($_ eq $format, @snapshot_fmts)) { + die_error(403, "Unsupported snapshot format"); } if (!defined $hash) { -- 1.6.4 ```