{"thread":{"id":"56685","subject":"[PATCH] http-backend: remove a duplicated code branch","startedAt":"2021-10-11T19:26:03Z","lastAt":"2021-10-26T19:05:44Z","messageCount":9,"participants":["Robin Dupret","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"438482","messageId":"20211011192546.1571-1-robin.dupret@hey.com","threadId":"56685","inReplyTo":null,"subject":"[PATCH] http-backend: remove a duplicated code branch","fromName":"Robin Dupret","fromEmail":"robin.dupret@gmail.com","sentAt":"2021-10-11T19:25:45Z","receivedAt":"2021-10-11T19:26:03Z","isPatch":true,"sender":{"key":"robin.dupret@gmail.com","avatar":"https://avatars.githubusercontent.com/u/354185?v=4"},"body":"\n---\n\nHello,\n\nThis patch simply factorize two code paths into a single one.\n\nJust wanted to say thanks for this piece of software and all\nthe work you are doing on it ; thank you very much !\n\nI hope I properly followed the contributing guidelines ; if not,\nsorry.\n\nHave a nice day.\n\n"},{"id":"438483","messageId":"20211011192546.1571-2-robin.dupret@hey.com","threadId":"56685","inReplyTo":"20211011192546.1571-1-robin.dupret@hey.com","subject":"[PATCH] http-backend: remove a duplicated code branch","fromName":"Robin Dupret","fromEmail":"robin.dupret@gmail.com","sentAt":"2021-10-11T19:25:46Z","receivedAt":"2021-10-11T19:26:16Z","isPatch":true,"sender":{"key":"robin.dupret@gmail.com","avatar":"https://avatars.githubusercontent.com/u/354185?v=4"},"body":"Signed-off-by: Robin Dupret \n---\n http-backend.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e7c0eeab23..3d6e2ff17f 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n \tssize_t req_len = get_content_length();\n \n-\tif (encoding && !strcmp(encoding, \"gzip\"))\n-\t\tgzipped_request = 1;\n-\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n+\tif (encoding && (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\")))\n \t\tgzipped_request = 1;\n \n \tif (!user || !*user)\n-- \n2.30.1 (Apple Git-130)\n\n"},{"id":"438515","messageId":"YWTyr6joJlyi1TPe@coredump.intra.peff.net","threadId":"56685","inReplyTo":"20211011192546.1571-2-robin.dupret@hey.com","subject":"Re: [PATCH] http-backend: remove a duplicated code branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-12T02:27:59Z","receivedAt":"2021-10-12T02:28:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 11, 2021 at 09:25:46PM +0200, Robin Dupret wrote:\n\n> Signed-off-by: Robin Dupret \n\nYou signed-off, which is good (and necessary for contributing a patch).\nThis is a good place to say \"why\". Even if it is \"because it makes the\ncode more readable\", it is good to say that rather than leave readers\nguessing (though of course people won't necessarily agree ;) ).\n\n> ---\n> http-backend.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n> \n> diff --git a/http-backend.c b/http-backend.c\n> index e7c0eeab23..3d6e2ff17f 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)\n> \tstruct child_process cld = CHILD_PROCESS_INIT;\n> \tssize_t req_len = get_content_length();\n> \n> -\tif (encoding && !strcmp(encoding, \"gzip\"))\n> -\t\tgzipped_request = 1;\n> -\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n> +\tif (encoding && (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\")))\n> \t\tgzipped_request = 1;\n\nI think this conversion is correct, and I do find the resulting slightly\neasier to read. I wondered if the two conditions might have come\nseparately, but no, they were both there in the initial 556cfa3b6d\n(Smart fetch and push over HTTP: server side, 2009-10-30).\n\nWe do frown a bit on making small style changes like this. This kind of\nchurn isn't dramatically improving the quality of the code, and it\ncarries the risk of regression (if there is something subtle that you or\nthe reviewers missed) and creates a maintenance burden (it may conflict\nwith other patches, though I doubt it in this case, and it creates work\nfor reviewers and the maintainer to apply).\n\nSo...I dunno. I don't mind it, but it is not a pattern we like to\nencourage in general. Let's see what Junio thinks.\n\n-Peff\n"},{"id":"438522","messageId":"8735p6li20.fsf@evledraar.gmail.com","threadId":"56685","inReplyTo":"YWTyr6joJlyi1TPe@coredump.intra.peff.net","subject":"Re: [PATCH] http-backend: remove a duplicated code branch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-12T09:07:55Z","receivedAt":"2021-10-12T09:16:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 11 2021, Jeff King wrote:\n\n> On Mon, Oct 11, 2021 at 09:25:46PM +0200, Robin Dupret wrote:\n>\n>> Signed-off-by: Robin Dupret \n>\n> You signed-off, which is good (and necessary for contributing a patch).\n> This is a good place to say \"why\". Even if it is \"because it makes the\n> code more readable\", it is good to say that rather than leave readers\n> guessing (though of course people won't necessarily agree ;) ).\n>\n>> ---\n>> http-backend.c | 4 +---\n>> 1 file changed, 1 insertion(+), 3 deletions(-)\n>> \n>> diff --git a/http-backend.c b/http-backend.c\n>> index e7c0eeab23..3d6e2ff17f 100644\n>> --- a/http-backend.c\n>> +++ b/http-backend.c\n>> @@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)\n>> \tstruct child_process cld = CHILD_PROCESS_INIT;\n>> \tssize_t req_len = get_content_length();\n>> \n>> -\tif (encoding && !strcmp(encoding, \"gzip\"))\n>> -\t\tgzipped_request = 1;\n>> -\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n>> +\tif (encoding && (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\")))\n>> \t\tgzipped_request = 1;\n>\n> I think this conversion is correct, and I do find the resulting slightly\n> easier to read. I wondered if the two conditions might have come\n> separately, but no, they were both there in the initial 556cfa3b6d\n> (Smart fetch and push over HTTP: server side, 2009-10-30).\n>\n> We do frown a bit on making small style changes like this. This kind of\n> churn isn't dramatically improving the quality of the code, and it\n> carries the risk of regression (if there is something subtle that you or\n> the reviewers missed) and creates a maintenance burden (it may conflict\n> with other patches, though I doubt it in this case, and it creates work\n> for reviewers and the maintainer to apply).\n>\n> So...I dunno. I don't mind it, but it is not a pattern we like to\n> encourage in general. Let's see what Junio thinks.\n\nMaybe the existence of this discussion is also why we frown on churn :)\nBut not being able to resist: FWIW I think if this were refactored the\nbelow makes more sense (untested etc.):\n\nBecause the pattern in that function is to:\n\n 1. Get params via getenv\n 2. Provide defaults in case those are NULL\n 3. Set resulting structs/variables, use them\n\nThe \"encoding\" and \"gzipped_request\" are the odd ones out there, this\nmakes them consistent with the rest.\n\nIt also has the effect of column-aligning the two strcmps, which along\nwith avoiding indentation is why I general is why I've sometimes used\nthis pattern of:\n\n if (a && b)\n ;\n else if (a && c)\n ;\n\nOver the obvious simplification of:\n\n if (a && (b || c))\n ;\n\nAlthough due to the \"if\" / \"else if\" in the pre-image that didn't apply\nhere...\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e7c0eeab230..13bc421b4e8 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -462,19 +462,19 @@ static void run_service(const char **argv, int buffer_input)\n \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n \tconst char *user = getenv(\"REMOTE_USER\");\n \tconst char *host = getenv(\"REMOTE_ADDR\");\n-\tint gzipped_request = 0;\n+\tint gzipped_request;\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n \tssize_t req_len = get_content_length();\n \n-\tif (encoding && !strcmp(encoding, \"gzip\"))\n-\t\tgzipped_request = 1;\n-\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n-\t\tgzipped_request = 1;\n-\n \tif (!user || !*user)\n \t\tuser = \"anonymous\";\n \tif (!host || !*host)\n \t\thost = \"(none)\";\n+\tif (!encoding)\n+\t\tencoding = \"\";\n+\n+\tgzipped_request = (!strcmp(encoding, \"gzip\") ||\n+\t\t\t !strcmp(encoding, \"x-gzip\"))\n \n \tif (!getenv(\"GIT_COMMITTER_NAME\"))\n \t\tstrvec_pushf(&cld.env_array, \"GIT_COMMITTER_NAME=%s\", user);\n"},{"id":"438570","messageId":"xmqqfst65fmn.fsf@gitster.g","threadId":"56685","inReplyTo":"YWTyr6joJlyi1TPe@coredump.intra.peff.net","subject":"Re: [PATCH] http-backend: remove a duplicated code branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-12T17:15:12Z","receivedAt":"2021-10-12T17:15:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> diff --git a/http-backend.c b/http-backend.c\n>> index e7c0eeab23..3d6e2ff17f 100644\n>> --- a/http-backend.c\n>> +++ b/http-backend.c\n>> @@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)\n>> \tstruct child_process cld = CHILD_PROCESS_INIT;\n>> \tssize_t req_len = get_content_length();\n>> \n>> -\tif (encoding && !strcmp(encoding, \"gzip\"))\n>> -\t\tgzipped_request = 1;\n>> -\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n>> +\tif (encoding && (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\")))\n>> \t\tgzipped_request = 1;\n>\n> I think this conversion is correct, and I do find the resulting slightly\n> easier to read. I wondered if the two conditions might have come\n> separately, but no, they were both there in the initial 556cfa3b6d\n> (Smart fetch and push over HTTP: server side, 2009-10-30).\n>\n> We do frown a bit on making small style changes like this. This kind of\n> churn isn't dramatically improving the quality of the code, and it\n> carries the risk of regression (if there is something subtle that you or\n> the reviewers missed) and creates a maintenance burden (it may conflict\n> with other patches, though I doubt it in this case, and it creates work\n> for reviewers and the maintainer to apply).\n>\n> So...I dunno. I don't mind it, but it is not a pattern we like to\n> encourage in general. Let's see what Junio thinks.\n\nThis particular conversion is mostly \"Meh\", although if it were done\nin this way:\n\n\tif (encoding) {\n\t\tif (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\"))\n\t\t\tgzipped_request = 1;\n\t}\n\nthe result may be a bit less \"meh\" by making it easier to adjust the\ncode for other kinds of encoding in the future.\n\nMy usual rule of thumb for such a single-hunk clean-up patch that is\n\"obviously not incorrect\" is to take it only once or twice per new\ncontributor. At some point, new contributors gain enough experience\nto know better, at which point I'd start frowning at them.\n\nThanks.\n"},{"id":"438571","messageId":"xmqqbl3u5fhy.fsf@gitster.g","threadId":"56685","inReplyTo":"8735p6li20.fsf@evledraar.gmail.com","subject":"Re: [PATCH] http-backend: remove a duplicated code branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-12T17:18:01Z","receivedAt":"2021-10-12T17:18:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/http-backend.c b/http-backend.c\n> index e7c0eeab230..13bc421b4e8 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -462,19 +462,19 @@ static void run_service(const char **argv, int buffer_input)\n> \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n> \tconst char *user = getenv(\"REMOTE_USER\");\n> \tconst char *host = getenv(\"REMOTE_ADDR\");\n> -\tint gzipped_request = 0;\n> +\tint gzipped_request;\n> \tstruct child_process cld = CHILD_PROCESS_INIT;\n> \tssize_t req_len = get_content_length();\n> \n> -\tif (encoding && !strcmp(encoding, \"gzip\"))\n> -\t\tgzipped_request = 1;\n> -\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n> -\t\tgzipped_request = 1;\n> -\n> \tif (!user || !*user)\n> \t\tuser = \"anonymous\";\n> \tif (!host || !*host)\n> \t\thost = \"(none)\";\n> +\tif (!encoding)\n> +\t\tencoding = \"\";\n> +\n> +\tgzipped_request = (!strcmp(encoding, \"gzip\") ||\n> +\t\t\t !strcmp(encoding, \"x-gzip\"))\n\nIn general, I'd frown upon such a conversion. In the the current\ncode, \"encoding\" might go dead after this point, but we are losing\ninformation for no good reason by making !encoding and !*encoding\nindistinguisable after this point. Compared to that, the rewrite in\nthe patch that started this discussion would be more preferrable.\n\nThanks.\n\n"},{"id":"439488","messageId":"20211024162859.6295-2-robin.dupret@hey.com","threadId":"56685","inReplyTo":"20211024162859.6295-1-robin.dupret@hey.com","subject":"[PATCH] http-backend: remove a duplicated code branch","fromName":"Robin Dupret","fromEmail":"robin.dupret@gmail.com","sentAt":"2021-10-24T16:28:59Z","receivedAt":"2021-10-24T16:29:34Z","isPatch":true,"sender":{"key":"robin.dupret@gmail.com","avatar":"https://avatars.githubusercontent.com/u/354185?v=4"},"body":"Try to make reading the computation of the gzipped flag a bit more\nnatural.\n\nSigned-off-by: Robin Dupret \n---\n http-backend.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e7c0eeab23..3d6e2ff17f 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n \tssize_t req_len = get_content_length();\n \n-\tif (encoding && !strcmp(encoding, \"gzip\"))\n-\t\tgzipped_request = 1;\n-\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n+\tif (encoding && (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\")))\n \t\tgzipped_request = 1;\n \n \tif (!user || !*user)\n-- \n2.30.1 (Apple Git-130)\n\n"},{"id":"439550","messageId":"xmqqsfwp2j6o.fsf@gitster.g","threadId":"56685","inReplyTo":"20211024162859.6295-2-robin.dupret@hey.com","subject":"Re: [PATCH] http-backend: remove a duplicated code branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-25T15:55:11Z","receivedAt":"2021-10-25T15:55:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Dupret writes:\n\n> Try to make reading the computation of the gzipped flag a bit more\n> natural.\n\n... and did you succeed? ;-)\n\nWe are all imperfect human and anything we do is merely \"trying to\",\nso let's not say \"try to\", unless the change is so involved that it\nis hard to judge if the result has succeeded in trying.\n\nDid you mean to send the same patch text (but with an updated log\nmessage)? Just making sure.\n\nThanks; will queue.\n\n> Signed-off-by: Robin Dupret \n> ---\n> http-backend.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/http-backend.c b/http-backend.c\n> index e7c0eeab23..3d6e2ff17f 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)\n> \tstruct child_process cld = CHILD_PROCESS_INIT;\n> \tssize_t req_len = get_content_length();\n> \n> -\tif (encoding && !strcmp(encoding, \"gzip\"))\n> -\t\tgzipped_request = 1;\n> -\telse if (encoding && !strcmp(encoding, \"x-gzip\"))\n> +\tif (encoding && (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\")))\n> \t\tgzipped_request = 1;\n> \n> \tif (!user || !*user)\n"},{"id":"439664","messageId":"CA+G124Xci1bGG1uxt35gr9ede11ix10CtLmmWcT+eYNCDSN6Xw@mail.gmail.com","threadId":"56685","inReplyTo":"xmqqsfwp2j6o.fsf@gitster.g","subject":"Re: [PATCH] http-backend: remove a duplicated code branch","fromName":"Robin Dupret","fromEmail":"robin.dupret@gmail.com","sentAt":"2021-10-26T19:04:25Z","receivedAt":"2021-10-26T19:05:44Z","isPatch":true,"sender":{"key":"robin.dupret@gmail.com","avatar":"https://avatars.githubusercontent.com/u/354185?v=4"},"body":"> Did you mean to send the same patch text (but with an updated log\n> message)? Just making sure.\n\nYes, exactly.\n\n> We are all imperfect human and anything we do is merely \"trying to\"\n\nFair point. :-) Do you want me to resend the patch without this part\nor is it ok ?\n\nThank you !\n\nLe lun. 25 oct. 2021 à 17:55, Junio C Hamano a écrit :\n>\n> Robin Dupret writes:\n>\n> > Try to make reading the computation of the gzipped flag a bit more\n> > natural.\n>\n> ... and did you succeed? ;-)\n>\n> We are all imperfect human and anything we do is merely \"trying to\",\n> so let's not say \"try to\", unless the change is so involved that it\n> is hard to judge if the result has succeeded in trying.\n>\n> Did you mean to send the same patch text (but with an updated log\n> message)? Just making sure.\n>\n> Thanks; will queue.\n>\n> > Signed-off-by: Robin Dupret \n> > ---\n> > http-backend.c | 4 +---\n> > 1 file changed, 1 insertion(+), 3 deletions(-)\n> >\n> > diff --git a/http-backend.c b/http-backend.c\n> > index e7c0eeab23..3d6e2ff17f 100644\n> > --- a/http-backend.c\n> > +++ b/http-backend.c\n> > @@ -466,9 +466,7 @@ static void run_service(const char **argv, int buffer_input)\n> > struct child_process cld = CHILD_PROCESS_INIT;\n> > ssize_t req_len = get_content_length();\n> >\n> > - if (encoding && !strcmp(encoding, \"gzip\"))\n> > - gzipped_request = 1;\n> > - else if (encoding && !strcmp(encoding, \"x-gzip\"))\n> > + if (encoding && (!strcmp(encoding, \"gzip\") || !strcmp(encoding, \"x-gzip\")))\n> > gzipped_request = 1;\n> >\n> > if (!user || !*user)\n"}]}