threads / rfc / 20710

RFC patch, 3 partsgitweb: make suspenders more useful

Subject: [RFC/PATCH 1/3] gitweb: make suspenders more useful

## tl;dr

5 messages between Aug 23, 2009 and Aug 24, 2009. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Mark Rada· Aug 23, 2009, 21:53 UTC · lore

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 <marada@uwaterloo.ca>
---
 gitweb/gitweb.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to gitweb/gitweb.perl +2 −2
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
Mark Rada· Aug 23, 2009, 22:39 UTC · re: Sam Vilain · lore

Re: [RFC/PATCH 1/3] gitweb: make suspenders more useful

On 23/08/09 6:10 PM, Sam Vilain wrote:
Show 10 quoted lines
> 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:
Show 19 quoted lines
> Mark A Rada <marada@uwaterloo.ca> 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· Aug 23, 2009, 22:48 UTC · re: Mark Rada · lore

Re: [RFC/PATCH 1/3] gitweb: make suspenders more useful

Mark Rada wrote:
Show 13 quoted lines
>>> 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· Aug 24, 2009, 03:09 UTC · re: Sam Vilain · lore

[RFC/PATCH 1/3] gitweb: improve snapshot error handling

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 <marada@uwaterloo.ca>
---
 gitweb/gitweb.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to gitweb/gitweb.perl +2 −2
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

← back to recent threads