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

On Mon, Oct 11 2021, Jeff King wrote:

On Mon, Oct 11, 2021 at 09:25:46PM +0200, Robin Dupret wrote:

quoted

Signed-off-by: Robin Dupret <redacted>

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

quoted

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