On Mon, Oct 11 2021, Jeff King wrote:
> On Mon, Oct 11, 2021 at 09:25:46PM +0200, Robin Dupret wrote:
>
>> Signed-off-by: Robin Dupret <robin.dupret@xxxxxxx>
>
> 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);
