From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Robin Dupret <robin.dupret@gmail.com>,
git@vger.kernel.org, Robin Dupret <robin.dupret@hey.com>
Subject: Re: [PATCH] http-backend: remove a duplicated code branch
Date: Tue, 12 Oct 2021 11:07:55 +0200 [thread overview]
Message-ID: <8735p6li20.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YWTyr6joJlyi1TPe@coredump.intra.peff.net>
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@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);
next prev parent reply other threads:[~2021-10-12 9:16 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-11 19:25 [PATCH] http-backend: remove a duplicated code branch Robin Dupret
2021-10-11 19:25 ` Robin Dupret
2021-10-12 2:27 ` Jeff King
2021-10-12 9:07 ` Ævar Arnfjörð Bjarmason [this message]
2021-10-12 17:18 ` Junio C Hamano
2021-10-24 16:28 ` Robin Dupret
2021-10-24 16:28 ` [PATCH] http-backend: remove a duplicated code branch Robin Dupret
2021-10-25 15:55 ` Junio C Hamano
2021-10-26 19:04 ` Robin Dupret
2021-10-12 17:15 ` Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2021-10-24 17:23 Robin Dupret
2021-10-24 17:29 ` Robin Dupret
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8735p6li20.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=robin.dupret@gmail.com \
--cc=robin.dupret@hey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).