Re: [PATCH] http-backend: remove a duplicated code branch

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 12, 2021, 09:07 UTC
Message-ID: <8735p6li20.fsf@evledraar.gmail.com>
In-Reply-To: <YWTyr6joJlyi1TPe@coredump.intra.peff.net>

On Mon, Oct 11 2021, Jeff King wrote:

Show 41 quoted lines

> On Mon, Oct 11, 2021 at 09:25:46PM +0200, Robin Dupret wrote:
>
>> Signed-off-by: Robin Dupret <robin.dupret@hey.com>
>
> You signed-off, which is good (and necessary for contributing a patch).
> This is a good place to say "why". Even if it is "because it makes the
> code more readable", it is good to say that rather than leave readers
> guessing (though of course people won't necessarily agree ;) ).
>
>> ---
>>  http-backend.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/http-backend.c b/http-backend.c
>> index e7c0eeab23..3d6e2ff17f 100644
>> --- a/http-backend.c
>> +++ b/http-backend.c
>> @@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)
>>  	struct child_process cld = CHILD_PROCESS_INIT;
>>  	ssize_t req_len = get_content_length();
>>  
>> -	if (encoding && !strcmp(encoding, "gzip"))
>> -		gzipped_request = 1;
>> -	else if (encoding && !strcmp(encoding, "x-gzip"))
>> +	if (encoding && (!strcmp(encoding, "gzip") || !strcmp(encoding, "x-gzip")))
>>  		gzipped_request = 1;
>
> I think this conversion is correct, and I do find the resulting slightly
> easier to read. I wondered if the two conditions might have come
> separately, but no, they were both there in the initial 556cfa3b6d
> (Smart fetch and push over HTTP: server side, 2009-10-30).
>
> We do frown a bit on making small style changes like this. This kind of
> churn isn't dramatically improving the quality of the code, and it
> carries the risk of regression (if there is something subtle that you or
> the reviewers missed) and creates a maintenance burden (it may conflict
> with other patches, though I doubt it in this case, and it creates work
> for reviewers and the maintainer to apply).
>
> So...I dunno. I don't mind it, but it is not a pattern we like to
> encourage in general. Let's see what Junio thinks.

Maybe the existence of this discussion is also why we frown on churn :) But not being able to resist: FWIW I think if this were refactored the below makes more sense (untested etc.):

Because the pattern in that function is to:

 1. Get params via getenv
 2. Provide defaults in case those are NULL
 3. Set resulting structs/variables, use them

The "encoding" and "gzipped_request" are the odd ones out there, this makes them consistent with the rest.

It also has the effect of column-aligning the two strcmps, which along with avoiding indentation is why I general is why I've sometimes used this pattern of:

    if (a && b)
        ;
    else if (a && c)
        ;

Over the obvious simplification of:

    if (a && (b || c))
        ;

Although due to the "if" / "else if" in the pre-image that didn't apply here...

diff --git a/http-backend.c b/http-backend.c
index e7c0eeab230..13bc421b4e8 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -462,19 +462,19 @@ static void run_service(const char **argv, int buffer_input)
 	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
 	const char *user = getenv("REMOTE_USER");
 	const char *host = getenv("REMOTE_ADDR");
-	int gzipped_request = 0;
+	int gzipped_request;
 	struct child_process cld = CHILD_PROCESS_INIT;
 	ssize_t req_len = get_content_length();
 
-	if (encoding && !strcmp(encoding, "gzip"))
-		gzipped_request = 1;
-	else if (encoding && !strcmp(encoding, "x-gzip"))
-		gzipped_request = 1;
-
 	if (!user || !*user)
 		user = "anonymous";
 	if (!host || !*host)
 		host = "(none)";
+	if (!encoding)
+		encoding = "";
+
+	gzipped_request = (!strcmp(encoding, "gzip") ||
+			   !strcmp(encoding, "x-gzip"))
 
 	if (!getenv("GIT_COMMITTER_NAME"))
 		strvec_pushf(&cld.env_array, "GIT_COMMITTER_NAME=%s", user);