* [PATCH] http-backend: remove a duplicated code branch @ 2021-10-11 19:25 Robin Dupret 2021-10-11 19:25 ` Robin Dupret 0 siblings, 1 reply; 12+ messages in thread From: Robin Dupret @ 2021-10-11 19:25 UTC (permalink / raw) To: git --- Hello, This patch simply factorize two code paths into a single one. Just wanted to say thanks for this piece of software and all the work you are doing on it ; thank you very much ! I hope I properly followed the contributing guidelines ; if not, sorry. Have a nice day. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] http-backend: remove a duplicated code branch 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 0 siblings, 1 reply; 12+ messages in thread From: Robin Dupret @ 2021-10-11 19:25 UTC (permalink / raw) To: git; +Cc: Robin Dupret Signed-off-by: Robin Dupret <robin.dupret@hey.com> --- 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; if (!user || !*user) -- 2.30.1 (Apple Git-130) ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch 2021-10-11 19:25 ` Robin Dupret @ 2021-10-12 2:27 ` Jeff King 2021-10-12 9:07 ` Ævar Arnfjörð Bjarmason 2021-10-12 17:15 ` Junio C Hamano 0 siblings, 2 replies; 12+ messages in thread From: Jeff King @ 2021-10-12 2:27 UTC (permalink / raw) To: Robin Dupret; +Cc: git, Robin Dupret 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. -Peff ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch 2021-10-12 2:27 ` Jeff King @ 2021-10-12 9:07 ` Ævar Arnfjörð Bjarmason 2021-10-12 17:18 ` Junio C Hamano 2021-10-12 17:15 ` Junio C Hamano 1 sibling, 1 reply; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-12 9:07 UTC (permalink / raw) To: Jeff King; +Cc: Robin Dupret, git, Robin Dupret 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); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch 2021-10-12 9:07 ` Ævar Arnfjörð Bjarmason @ 2021-10-12 17:18 ` Junio C Hamano 2021-10-24 16:28 ` Robin Dupret 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2021-10-12 17:18 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Jeff King, Robin Dupret, git, Robin Dupret Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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")) In general, I'd frown upon such a conversion. In the the current code, "encoding" might go dead after this point, but we are losing information for no good reason by making !encoding and !*encoding indistinguisable after this point. Compared to that, the rewrite in the patch that started this discussion would be more preferrable. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* (no subject) 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 0 siblings, 1 reply; 12+ messages in thread From: Robin Dupret @ 2021-10-24 16:28 UTC (permalink / raw) To: git --- Hello, Sorry for the late reply, I'm not really familiar with mailing lists. Here's the patch with an ammended commit message. Actually, the goal is not to add any maintainance burden. I just thought the original prose was not really intentional. Feel free to discard this patch if you doubt it brings any improvement to the code. I'd be happy to send a more substantial patch later if the opportunity arrises. :-) (Thanks for your comments and again for all your work on this great tool !) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] http-backend: remove a duplicated code branch 2021-10-24 16:28 ` Robin Dupret @ 2021-10-24 16:28 ` Robin Dupret 2021-10-25 15:55 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Robin Dupret @ 2021-10-24 16:28 UTC (permalink / raw) To: git; +Cc: Robin Dupret Try to make reading the computation of the gzipped flag a bit more natural. Signed-off-by: Robin Dupret <robin.dupret@hey.com> --- 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; if (!user || !*user) -- 2.30.1 (Apple Git-130) ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch 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 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2021-10-25 15:55 UTC (permalink / raw) To: Robin Dupret; +Cc: git, Robin Dupret Robin Dupret <robin.dupret@gmail.com> writes: > Try to make reading the computation of the gzipped flag a bit more > natural. ... and did you succeed? ;-) We are all imperfect human and anything we do is merely "trying to", so let's not say "try to", unless the change is so involved that it is hard to judge if the result has succeeded in trying. Did you mean to send the same patch text (but with an updated log message)? Just making sure. Thanks; will queue. > Signed-off-by: Robin Dupret <robin.dupret@hey.com> > --- > 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; > > if (!user || !*user) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch 2021-10-25 15:55 ` Junio C Hamano @ 2021-10-26 19:04 ` Robin Dupret 0 siblings, 0 replies; 12+ messages in thread From: Robin Dupret @ 2021-10-26 19:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Robin Dupret > Did you mean to send the same patch text (but with an updated log > message)? Just making sure. Yes, exactly. > We are all imperfect human and anything we do is merely "trying to" Fair point. :-) Do you want me to resend the patch without this part or is it ok ? Thank you ! Le lun. 25 oct. 2021 à 17:55, Junio C Hamano <gitster@pobox.com> a écrit : > > Robin Dupret <robin.dupret@gmail.com> writes: > > > Try to make reading the computation of the gzipped flag a bit more > > natural. > > ... and did you succeed? ;-) > > We are all imperfect human and anything we do is merely "trying to", > so let's not say "try to", unless the change is so involved that it > is hard to judge if the result has succeeded in trying. > > Did you mean to send the same patch text (but with an updated log > message)? Just making sure. > > Thanks; will queue. > > > Signed-off-by: Robin Dupret <robin.dupret@hey.com> > > --- > > 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; > > > > if (!user || !*user) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch 2021-10-12 2:27 ` Jeff King 2021-10-12 9:07 ` Ævar Arnfjörð Bjarmason @ 2021-10-12 17:15 ` Junio C Hamano 1 sibling, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2021-10-12 17:15 UTC (permalink / raw) To: Jeff King; +Cc: Robin Dupret, git, Robin Dupret Jeff King <peff@peff.net> writes: >> 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. This particular conversion is mostly "Meh", although if it were done in this way: if (encoding) { if (!strcmp(encoding, "gzip") || !strcmp(encoding, "x-gzip")) gzipped_request = 1; } the result may be a bit less "meh" by making it easier to adjust the code for other kinds of encoding in the future. My usual rule of thumb for such a single-hunk clean-up patch that is "obviously not incorrect" is to take it only once or twice per new contributor. At some point, new contributors gain enough experience to know better, at which point I'd start frowning at them. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch @ 2021-10-24 17:23 Robin Dupret 2021-10-24 17:29 ` Robin Dupret 0 siblings, 1 reply; 12+ messages in thread From: Robin Dupret @ 2021-10-24 17:23 UTC (permalink / raw) To: git In-Reply-To: xmqqbl3u5fhy.fsf@gitster.g Cc: Jeff King <peff@peff.net>, Robin Dupret <robin.dupret@gmail.com>, git@vger.kernel.org, Robin Dupret <robin.dupret@hey.com>, junio@pobox.com Hello, Sorry for the late reply, I'm not really familiar with mailing lists. I resent the patch with an ammended commit message. Actually, the goal is not to add any maintainance burden. I just thought the original prose was not really intentional. Feel free to discard this patch if you doubt it brings any improvement to the code. I'd be happy to send a more substantial patch later if the opportunity arrises. :-) (Thanks for your comments and again for all your work on this great tool !) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] http-backend: remove a duplicated code branch 2021-10-24 17:23 Robin Dupret @ 2021-10-24 17:29 ` Robin Dupret 0 siblings, 0 replies; 12+ messages in thread From: Robin Dupret @ 2021-10-24 17:29 UTC (permalink / raw) To: git; +Cc: peff, junio, avarab Hello, Sorry for the late reply, I'm not really familiar with mailing lists. I resent the patch with an ammended commit message. Actually, the goal is not to add any maintainance burden. I just thought the original prose was not really intentional. Feel free to discard this patch if you doubt it brings any improvement to the code. I'd be happy to send a more substantial patch later if the opportunity arrises. :-) (Thanks for your comments and again for all your work on this great tool !) Le dim. 24 oct. 2021 à 19:25, Robin Dupret <robin.dupret@gmail.com> a écrit : > > In-Reply-To: xmqqbl3u5fhy.fsf@gitster.g > Cc: Jeff King <peff@peff.net>, Robin Dupret <robin.dupret@gmail.com>, git@vger.kernel.org, Robin Dupret <robin.dupret@hey.com>, junio@pobox.com > > Hello, > > Sorry for the late reply, I'm not really familiar with mailing lists. > I resent the patch with an ammended commit message. > > Actually, the goal is not to add any maintainance burden. I just thought > the original prose was not really intentional. Feel free to discard this > patch if you doubt it brings any improvement to the code. > > I'd be happy to send a more substantial patch later if the opportunity > arrises. :-) > > (Thanks for your comments and again for all your work on this great > tool !) ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-26 19:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).