{"thread":{"id":"47021","subject":"[PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","startedAt":"2017-10-24T15:27:36Z","lastAt":"2017-10-31T09:09:26Z","messageCount":22,"participants":["Andrey Okoshkin","Stefan Beller","Eric Sunshine","Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"330918","messageId":"3aed764b-388c-d163-08fc-32b294c6b9d3@samsung.com","threadId":"47021","inReplyTo":"CGME20171024152727epcas2p4fb7dcf147e44aadf7733098151d469a5@epcas2p4.samsung.com","subject":"[PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-24T15:27:24Z","receivedAt":"2017-10-24T15:27:36Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in\ninit_merge_options().\nConsequential call of getenv() may return NULL pointer and strtol() crashes.\nHowever the stored pointer to the obtained getenv() result may be invalidated\nby some other getenv() call from another thread as getenv() is not thread-safe.\n\nSigned-off-by: Andrey Okoshkin \n---\n merge-recursive.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1494ffdb8..eaac98145 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n \n void init_merge_options(struct merge_options *o)\n {\n+\tconst char *merge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tmemset(o, 0, sizeof(struct merge_options));\n \to->verbosity = 2;\n \to->buffer_output = 1;\n@@ -2171,9 +2172,8 @@ void init_merge_options(struct merge_options *o)\n \to->renormalize = 0;\n \to->detect_rename = 1;\n \tmerge_recursive_config(o);\n-\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n-\t\to->verbosity =\n-\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n+\tif (merge_verbosity)\n+\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n \tif (o->verbosity >= 5)\n \t\to->buffer_output = 0;\n \tstrbuf_init(&o->obuf, 0);\n-- \n2.14.3\n"},{"id":"330924","messageId":"CAGZ79kaSZG9WriqX0SLbTnEDRr-4YdFRuK17+9wnxyrftfnMmg@mail.gmail.com","threadId":"47021","inReplyTo":"3aed764b-388c-d163-08fc-32b294c6b9d3@samsung.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-24T16:28:31Z","receivedAt":"2017-10-24T16:28:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin wrote:\n> Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in\n> init_merge_options().\n> Consequential call of getenv() may return NULL pointer and strtol() crashes.\n> However the stored pointer to the obtained getenv() result may be invalidated\n> by some other getenv() call from another thread as getenv() is not thread-safe.\n\nBut do we have other threads running at the time?\nInspecting the four callsites:\n* sequencer.c:\n The prior lines to hold the index lock suggests we're not in a threaded\n environment\n* builtin/merge-recursive.c:\n In cmd_merge_recursive and we're fiddling with argv/argc, which\n suggests we in a main function, not having threads around\n* builtin/am.c: fall_back_threeway called by am_run.\n (am is not threaded)\n* builtin/merge:\n In try_merge_strategy called from the main function, also index locking\n* builtin/checkout.c:\n merge_working_tree also locks the index.\n\nSo I think this function is never called from within a threaded environment\nin git.\n\n>\n> Signed-off-by: Andrey Okoshkin \n> ---\n> merge-recursive.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 1494ffdb8..eaac98145 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n>\n> void init_merge_options(struct merge_options *o)\n> {\n> + const char *merge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n\nDespite not being in a threaded environment, I wonder if we want to\nminimize the time between calling getenv and the use of the result,\ni.e. declare merge_verbosity here, but assign it later, just before the\ncondition?\n\n(The compiler may shuffle stuff around anyway, so this is a\nmoot suggestion; It gears mostly towards making the code more\nreadable/maintainable when presenting this part of the code\nto the user.)\n\nWith or without this change:\nReviewed-by: Stefan Beller \n\n\n> memset(o, 0, sizeof(struct merge_options));\n> o->verbosity = 2;\n> o->buffer_output = 1;\n> @@ -2171,9 +2172,8 @@ void init_merge_options(struct merge_options *o)\n> o->renormalize = 0;\n> o->detect_rename = 1;\n> merge_recursive_config(o);\n> - if (getenv(\"GIT_MERGE_VERBOSITY\"))\n> - o->verbosity =\n> - strtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n> + if (merge_verbosity)\n> + o->verbosity = strtol(merge_verbosity, NULL, 10);\n> if (o->verbosity >= 5)\n> o->buffer_output = 0;\n> strbuf_init(&o->obuf, 0);\n> --\n> 2.14.3\n"},{"id":"330926","messageId":"CAPig+cRTL2amjsgdp1=T3GMZLa=favugOfnQw9XjWzC+U=v5Sw@mail.gmail.com","threadId":"47021","inReplyTo":"CAGZ79kaSZG9WriqX0SLbTnEDRr-4YdFRuK17+9wnxyrftfnMmg@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-24T16:45:14Z","receivedAt":"2017-10-24T16:45:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote:\n> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin wrote:\n>> Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in\n>> init_merge_options().\n>> Consequential call of getenv() may return NULL pointer and strtol() crashes.\n>> However the stored pointer to the obtained getenv() result may be invalidated\n>> by some other getenv() call from another thread as getenv() is not thread-safe.\n>\n> But do we have other threads running at the time?\n\nI feel uncomfortable about this change, not due to lack of thread\nsafety, but due to the distance between the getenv() invocation and\nits point of use. See below for more detail.\n\n>> Signed-off-by: Andrey Okoshkin \n>> ---\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n>> void init_merge_options(struct merge_options *o)\n>> {\n>> + const char *merge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n>\n> Despite not being in a threaded environment, I wonder if we want to\n> minimize the time between calling getenv and the use of the result,\n> i.e. declare merge_verbosity here, but assign it later, just before the\n> condition?\n>\n> With or without this change:\n> Reviewed-by: Stefan Beller \n\nThe distance between getenv() and the point where the value is\nactually used is a big concern due to not knowing what is or might be\ngoing on in called functions between the two points. According to [1],\nthe value returned by getenv() could be invalidated by another call to\ngetenv() (or setenv() or unsetenv() or putenv()), and we don't have\nguarantee that we're safe from such invalidation considering that this\nfunction calls out to others. For instance, after getenv() but before\nthe value is used, init_merge_options() calls merge_recursive_config()\nwhich calls git_config() which calls git_xmerge_config(), and so on.\n\nFor this reason, I have difficulty endorsing this change as-is.\n\n[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html\n\n>> memset(o, 0, sizeof(struct merge_options));\n>> o->verbosity = 2;\n>> o->buffer_output = 1;\n>> @@ -2171,9 +2172,8 @@ void init_merge_options(struct merge_options *o)\n>> o->renormalize = 0;\n>> o->detect_rename = 1;\n>> merge_recursive_config(o);\n>> - if (getenv(\"GIT_MERGE_VERBOSITY\"))\n>> - o->verbosity =\n>> - strtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n>> + if (merge_verbosity)\n>> + o->verbosity = strtol(merge_verbosity, NULL, 10);\n>> if (o->verbosity >= 5)\n>> o->buffer_output = 0;\n>> strbuf_init(&o->obuf, 0);\n>> --\n>> 2.14.3\n"},{"id":"330928","messageId":"CAN0heSp7b_6n3y=s4++oWhkPUuM=s9L7LWVx5vn8o=5aH6DKKw@mail.gmail.com","threadId":"47021","inReplyTo":"CAPig+cRTL2amjsgdp1=T3GMZLa=favugOfnQw9XjWzC+U=v5Sw@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-24T17:11:24Z","receivedAt":"2017-10-24T17:11:33Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 24 October 2017 at 18:45, Eric Sunshine wrote:\n> On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote:\n>> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin wrote:\n>>> Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in\n>>> init_merge_options().\n>>> Consequential call of getenv() may return NULL pointer and strtol() crashes.\n>>> However the stored pointer to the obtained getenv() result may be invalidated\n>>> by some other getenv() call from another thread as getenv() is not thread-safe.\n\nI'm having trouble wrapping my head around this. Under which\ncircumstances could the second call in the current code return NULL, but\nthe code after your patch behave in a well-defined (and correct) way?\n\n> The distance between getenv() and the point where the value is\n> actually used is a big concern due to not knowing what is or might be\n> going on in called functions between the two points. According to [1],\n> the value returned by getenv() could be invalidated by another call to\n> getenv() (or setenv() or unsetenv() or putenv()), and we don't have\n> guarantee that we're safe from such invalidation considering that this\n> function calls out to others. For instance, after getenv() but before\n> the value is used, init_merge_options() calls merge_recursive_config()\n> which calls git_config() which calls git_xmerge_config(), and so on.\n>\n> For this reason, I have difficulty endorsing this change as-is.\n\nYeah. The call should be immediately before `merge_verbosity` is used.\nThen, if a compiler wants to move the call, it has to do the work and\nprove that it's ok.\n"},{"id":"330960","messageId":"20171024195221.gqgtibwjaztgeel6@sigill.intra.peff.net","threadId":"47021","inReplyTo":"CAN0heSp7b_6n3y=s4++oWhkPUuM=s9L7LWVx5vn8o=5aH6DKKw@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T19:52:21Z","receivedAt":"2017-10-24T19:52:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 24, 2017 at 07:11:24PM +0200, Martin Ågren wrote:\n\n> On 24 October 2017 at 18:45, Eric Sunshine wrote:\n> > On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote:\n> >> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin wrote:\n> >>> Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in\n> >>> init_merge_options().\n> >>> Consequential call of getenv() may return NULL pointer and strtol() crashes.\n> >>> However the stored pointer to the obtained getenv() result may be invalidated\n> >>> by some other getenv() call from another thread as getenv() is not thread-safe.\n> \n> I'm having trouble wrapping my head around this. Under which\n> circumstances could the second call in the current code return NULL, but\n> the code after your patch behave in a well-defined (and correct) way?\n\nYeah, it's not at all clear to me this is solving a real problem. I know\nAndrey mentioned playing around with fault injection in an earlier\nthread, so I'm wondering if there is an artificial fault being injected\ninto the second getenv() call. Which does not seem like something that\nshould be possible in the real world.\n\nI definitely agree with the sentiment that as few things as possible\nshould happen between calling getenv() and using its result. I've seen\nreal bugs there from unexpected invalidation of the static buffer.\n\n-Peff\n"},{"id":"330979","messageId":"xmqq8tg0j8vb.fsf@gitster.mtv.corp.google.com","threadId":"47021","inReplyTo":"20171024195221.gqgtibwjaztgeel6@sigill.intra.peff.net","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-25T01:48:08Z","receivedAt":"2017-10-25T01:48:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I definitely agree with the sentiment that as few things as possible\n> should happen between calling getenv() and using its result. I've seen\n> real bugs there from unexpected invalidation of the static buffer.\n\nSure. I do not think we mind xstrdup()ing the result and freeing\nafter we are done, though ;-)\n"},{"id":"330984","messageId":"CAPig+cSjQd=p1CdizU5oUaz91z=j02UnWLtTguWzvkjS+v6ETA@mail.gmail.com","threadId":"47021","inReplyTo":"xmqq8tg0j8vb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-25T04:07:12Z","receivedAt":"2017-10-25T04:07:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 24, 2017 at 9:48 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>> I definitely agree with the sentiment that as few things as possible\n>> should happen between calling getenv() and using its result. I've seen\n>> real bugs there from unexpected invalidation of the static buffer.\n>\n> Sure. I do not think we mind xstrdup()ing the result and freeing\n> after we are done, though ;-)\n\nMore specifically, xstrdup_or_null(getenv(...)), if that route is chosen.\n"},{"id":"330998","messageId":"20171025072717.7svdq4kqlfxlwszi@sigill.intra.peff.net","threadId":"47021","inReplyTo":"CAPig+cSjQd=p1CdizU5oUaz91z=j02UnWLtTguWzvkjS+v6ETA@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-25T07:27:18Z","receivedAt":"2017-10-25T07:27:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 25, 2017 at 12:07:12AM -0400, Eric Sunshine wrote:\n\n> On Tue, Oct 24, 2017 at 9:48 PM, Junio C Hamano wrote:\n> > Jeff King writes:\n> >> I definitely agree with the sentiment that as few things as possible\n> >> should happen between calling getenv() and using its result. I've seen\n> >> real bugs there from unexpected invalidation of the static buffer.\n> >\n> > Sure. I do not think we mind xstrdup()ing the result and freeing\n> > after we are done, though ;-)\n> \n> More specifically, xstrdup_or_null(getenv(...)), if that route is chosen.\n\nThat would be the way to do it, but I do not see thta we need to record\nthe string at all. The current code is calling strtol on it on it\nimmediately.\n\nSo the options are:\n\n 1. Save the result of getenv() in a variable. If it is non-NULL, then\n immediately call strtol() on it.\n\n 2. Do nothing. The double-call to getenv() is actually fine in the\n real world as it will return consistent results.\n\nBut the patch under discussion, which calls getenv() then expects it\nto be correct after a call to merge_recursive_config(), introduces a\nproblem.\n\n-Peff\n"},{"id":"331005","messageId":"b92619c8-8ae1-1270-ba7e-bfd0afd3055f@samsung.com","threadId":"47021","inReplyTo":"CAGZ79kaSZG9WriqX0SLbTnEDRr-4YdFRuK17+9wnxyrftfnMmg@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-25T10:49:33Z","receivedAt":"2017-10-25T10:49:52Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Thanks for your review.\n\n24.10.2017 19:28, Stefan Beller wrote:\n> So I think this function is never called from within a threaded environment\n> in git.\nYou are right, it's just a hypothetic case.\n \n> Despite not being in a threaded environment, I wonder if we want to\n> minimize the time between calling getenv and the use of the result,\n> i.e. declare merge_verbosity here, but assign it later, just before the\n> condition?\n> \n> (The compiler may shuffle stuff around anyway, so this is a\n> moot suggestion; It gears mostly towards making the code more\n> readable/maintainable when presenting this part of the code\n> to the user.)\n> \n> With or without this change:\n> Reviewed-by: Stefan Beller \nYes, in current situation it's more for readability. And I'll make the usage\nof merge_verbosity just after the assignment.\n\n-- \nBest regards,\nAndrey Okoshkin\n"},{"id":"331006","messageId":"41b8510f-d71b-9de6-9199-9e355f373af2@samsung.com","threadId":"47021","inReplyTo":"CAPig+cRTL2amjsgdp1=T3GMZLa=favugOfnQw9XjWzC+U=v5Sw@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-25T10:49:50Z","receivedAt":"2017-10-25T10:50:00Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"24.10.2017 19:45, Eric Sunshine wrote:\n> I feel uncomfortable about this change, not due to lack of thread\n> safety, but due to the distance between the getenv() invocation and\n> its point of use. See below for more detail.\n\nThanks, the usage must be just after the assignment.\n\n-- \nBest regards,\nAndrey Okoshkin\n"},{"id":"331007","messageId":"c7b82a8b-a133-6310-22d1-428dbf483a24@samsung.com","threadId":"47021","inReplyTo":"20171024195221.gqgtibwjaztgeel6@sigill.intra.peff.net","subject":"Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-25T11:13:20Z","receivedAt":"2017-10-25T11:13:32Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"\n24.10.2017 22:52, Jeff King wrote:\n> On Tue, Oct 24, 2017 at 07:11:24PM +0200, Martin Ågren wrote:\n> \n>> On 24 October 2017 at 18:45, Eric Sunshine wrote:\n>>> On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote:\n>>>> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin wrote:\n>>>>> Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in\n>>>>> init_merge_options().\n>>>>> Consequential call of getenv() may return NULL pointer and strtol() crashes.\n>>>>> However the stored pointer to the obtained getenv() result may be invalidated\n>>>>> by some other getenv() call from another thread as getenv() is not thread-safe.\n>>\n>> I'm having trouble wrapping my head around this. Under which\n>> circumstances could the second call in the current code return NULL, but\n>> the code after your patch behave in a well-defined (and correct) way?\n> \n> Yeah, it's not at all clear to me this is solving a real problem. I know\n> Andrey mentioned playing around with fault injection in an earlier\n> thread, so I'm wondering if there is an artificial fault being injected\n> into the second getenv() call. Which does not seem like something that\n> should be possible in the real world.\n> \n> I definitely agree with the sentiment that as few things as possible\n> should happen between calling getenv() and using its result. I've seen\n> real bugs there from unexpected invalidation of the static buffer.\n> \n> -Peff\n\nThanks for your comments.\n\nJeff is right: there were some artificial fault injections imitating valid failures\nof different functions (syscalls, libc and so on).\nAnd yes - in the real life there is no problems with current code as there are no other\nthreads running.\nHowever it's not a good practice to double call getenv() with the same argument:\n* Code readability.\n* Still no guaranty that the second call will be valid: some linked library may be\ncompromised or LD_PRELOADed with the aim to create a race with getenv(). I believe\nthere is no profit doing it here but it's just an explanation.\n\nIn my opinion, here it's ok to save the pointer returned from the single getnev() call\ndoing as few actions as possible between getenv() and strtol() calls.\nI'll change the patch.\n\n-- \nBest regards,\nAndrey Okoshkin\n"},{"id":"331008","messageId":"38a80069-abdb-0646-a20c-eca39dd4f519@samsung.com","threadId":"47021","inReplyTo":"20171025072717.7svdq4kqlfxlwszi@sigill.intra.peff.net","subject":"[PATCH v2] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-25T11:39:17Z","receivedAt":"2017-10-25T11:39:30Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Check 'GIT_MERGE_VERBOSITY' environment variable only once in\ninit_merge_options().\nConsequential call of getenv() may return NULL pointer.\nHowever the stored pointer to the obtained getenv() result may be invalidated\nby some other getenv() call as getenv() is not thread-safe.\n\nSigned-off-by: Andrey Okoshkin \nReviewed-by: Stefan Beller \n---\nChanges since the previous patch:\n* no actions are taken between the merge_verbosity assignment and check.\n merge-recursive.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1494ffdb8..60084e3a0 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n \n void init_merge_options(struct merge_options *o)\n {\n+\tconst char *merge_verbosity;\n \tmemset(o, 0, sizeof(struct merge_options));\n \to->verbosity = 2;\n \to->buffer_output = 1;\n@@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n \to->renormalize = 0;\n \to->detect_rename = 1;\n \tmerge_recursive_config(o);\n-\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n-\t\to->verbosity =\n-\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n+\tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n+\tif (merge_verbosity)\n+\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n \tif (o->verbosity >= 5)\n \t\to->buffer_output = 0;\n \tstrbuf_init(&o->obuf, 0);\n-- \n2.14.3\n"},{"id":"331010","messageId":"CAPig+cRq1AEOgDoXeH-hDMvhEMnfiNK5CuSBbbio-mbHros=QQ@mail.gmail.com","threadId":"47021","inReplyTo":"38a80069-abdb-0646-a20c-eca39dd4f519@samsung.com","subject":"Re: [PATCH v2] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-25T11:53:07Z","receivedAt":"2017-10-25T11:53:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 25, 2017 at 7:39 AM, Andrey Okoshkin wrote:\n> Check 'GIT_MERGE_VERBOSITY' environment variable only once in\n> init_merge_options().\n> Consequential call of getenv() may return NULL pointer.\n\nIt would be particularly nice to have a more detailed explanation in\nthe commit message of the potential problem this patch is trying to\nsolve. Given the amount of discussion, thus far, surrounding such a\nsimple patch, this cryptic warning about getenv() returning NULL upon\nsecond invocation is insufficient to explain why this patch is\ndesirable; it merely leads to a lot of head-scratching.\n\n> However the stored pointer to the obtained getenv() result may be invalidated\n> by some other getenv() call as getenv() is not thread-safe.\n\nThis is even more cryptic, as it appears to be arguing for or against\n_something_ (it's not clear what) and it seems to be talking about a\nfacet of the existing code, rather than explaining why the updated\ncode consumes its 'merge_verbosity' value as early as possible after\nbeing assigned. Perhaps this part could be reworded something like\nthis:\n\n Instead, call getenv() only once, storing its value and\n consulting it as many times as needed. This update takes care\n to consume the value returned by getenv() without any\n intervening calls to getenv(), setenv(), unsetenv(), or\n putenv(), any of which might invalidate the pointer returned\n by the initial call.\n\n> Signed-off-by: Andrey Okoshkin \n> Reviewed-by: Stefan Beller \n\nAs this patch is semantically quite different from the original to\nwhich Stefan gave his Reviewed-by: (and which other people argued\nagainst), it might be better omit this footer and let him re-give it\nif he so desires.\n\n> ---\n> Changes since the previous patch:\n> * no actions are taken between the merge_verbosity assignment and check.\n> merge-recursive.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 1494ffdb8..60084e3a0 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n>\n> void init_merge_options(struct merge_options *o)\n> {\n> + const char *merge_verbosity;\n> memset(o, 0, sizeof(struct merge_options));\n> o->verbosity = 2;\n> o->buffer_output = 1;\n> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n> o->renormalize = 0;\n> o->detect_rename = 1;\n> merge_recursive_config(o);\n> - if (getenv(\"GIT_MERGE_VERBOSITY\"))\n> - o->verbosity =\n> - strtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n> + merge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n> + if (merge_verbosity)\n> + o->verbosity = strtol(merge_verbosity, NULL, 10);\n> if (o->verbosity >= 5)\n> o->buffer_output = 0;\n> strbuf_init(&o->obuf, 0);\n> --\n> 2.14.3\n"},{"id":"331014","messageId":"84d89e54-4e72-9938-4d71-e24a0617fe82@samsung.com","threadId":"47021","inReplyTo":"CAPig+cRq1AEOgDoXeH-hDMvhEMnfiNK5CuSBbbio-mbHros=QQ@mail.gmail.com","subject":"Re: [PATCH v2] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-25T12:27:44Z","receivedAt":"2017-10-25T12:27:56Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Thanks, Eric, indeed it's better to change the commit message.\n\n25.10.2017 14:53, Eric Sunshine wrote:\n> On Wed, Oct 25, 2017 at 7:39 AM, Andrey Okoshkin wrote:\n>> Check 'GIT_MERGE_VERBOSITY' environment variable only once in\n>> init_merge_options().\n>> Consequential call of getenv() may return NULL pointer.\n> \n> It would be particularly nice to have a more detailed explanation in\n> the commit message of the potential problem this patch is trying to\n> solve. Given the amount of discussion, thus far, surrounding such a\n> simple patch, this cryptic warning about getenv() returning NULL upon\n> second invocation is insufficient to explain why this patch is\n> desirable; it merely leads to a lot of head-scratching.\n> \n>> However the stored pointer to the obtained getenv() result may be invalidated\n>> by some other getenv() call as getenv() is not thread-safe.\n> \n> This is even more cryptic, as it appears to be arguing for or against\n> _something_ (it's not clear what) and it seems to be talking about a\n> facet of the existing code, rather than explaining why the updated\n> code consumes its 'merge_verbosity' value as early as possible after\n> being assigned. Perhaps this part could be reworded something like\n> this:\n> \n> Instead, call getenv() only once, storing its value and\n> consulting it as many times as needed. This update takes care\n> to consume the value returned by getenv() without any\n> intervening calls to getenv(), setenv(), unsetenv(), or\n> putenv(), any of which might invalidate the pointer returned\n> by the initial call.\n> \n>> Signed-off-by: Andrey Okoshkin \n>> Reviewed-by: Stefan Beller \n> \n> As this patch is semantically quite different from the original to\n> which Stefan gave his Reviewed-by: (and which other people argued\n> against), it might be better omit this footer and let him re-give it\n> if he so desires.\n> \n>> ---\n>> Changes since the previous patch:\n>> * no actions are taken between the merge_verbosity assignment and check.\n>> merge-recursive.c | 7 ++++---\n>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 1494ffdb8..60084e3a0 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n>>\n>> void init_merge_options(struct merge_options *o)\n>> {\n>> + const char *merge_verbosity;\n>> memset(o, 0, sizeof(struct merge_options));\n>> o->verbosity = 2;\n>> o->buffer_output = 1;\n>> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n>> o->renormalize = 0;\n>> o->detect_rename = 1;\n>> merge_recursive_config(o);\n>> - if (getenv(\"GIT_MERGE_VERBOSITY\"))\n>> - o->verbosity =\n>> - strtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n>> + merge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n>> + if (merge_verbosity)\n>> + o->verbosity = strtol(merge_verbosity, NULL, 10);\n>> if (o->verbosity >= 5)\n>> o->buffer_output = 0;\n>> strbuf_init(&o->obuf, 0);\n>> --\n>> 2.14.3\n> \n> \n> \n\n-- \nBest regards,\nAndrey Okoshkin\n"},{"id":"331016","messageId":"bd7eb593-75f9-0dd1-9dff-9dc420532217@samsung.com","threadId":"47021","inReplyTo":"CAPig+cRq1AEOgDoXeH-hDMvhEMnfiNK5CuSBbbio-mbHros=QQ@mail.gmail.com","subject":"Re: [PATCH v3] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-25T13:03:06Z","receivedAt":"2017-10-25T13:03:23Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Get 'GIT_MERGE_VERBOSITY' environment variable only once in\ninit_merge_options() and store the pointer to its value for the further check.\nNo intervening calls to getenv(), putenv(), setenv() or unsetenv() are done\nbetween the initial getenv() call and the consequential result pass to strtol()\nas these environment related functions could modify the string pointer returned\nby the initial getenv() call.\n\nSigned-off-by: Andrey Okoshkin \n---\nI tried to rework the commit message.\n merge-recursive.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1494ffdb8..60084e3a0 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n \n void init_merge_options(struct merge_options *o)\n {\n+\tconst char *merge_verbosity;\n \tmemset(o, 0, sizeof(struct merge_options));\n \to->verbosity = 2;\n \to->buffer_output = 1;\n@@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n \to->renormalize = 0;\n \to->detect_rename = 1;\n \tmerge_recursive_config(o);\n-\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n-\t\to->verbosity =\n-\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n+\tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n+\tif (merge_verbosity)\n+\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n \tif (o->verbosity >= 5)\n \t\to->buffer_output = 0;\n \tstrbuf_init(&o->obuf, 0);\n-- \n2.14.3\n\n"},{"id":"331180","messageId":"CAGZ79kZ9EV=qaYyuA3kfuQ04EhLWax52MhtkmGJto2Lommc_SQ@mail.gmail.com","threadId":"47021","inReplyTo":"bd7eb593-75f9-0dd1-9dff-9dc420532217@samsung.com","subject":"Re: [PATCH v3] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-27T17:29:01Z","receivedAt":"2017-10-27T17:29:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 25, 2017 at 6:03 AM, Andrey Okoshkin wrote:\n> Get 'GIT_MERGE_VERBOSITY' environment variable only once in\n> init_merge_options() and store the pointer to its value for the further check.\n> No intervening calls to getenv(), putenv(), setenv() or unsetenv() are done\n> between the initial getenv() call and the consequential result pass to strtol()\n> as these environment related functions could modify the string pointer returned\n> by the initial getenv() call.\n>\n> Signed-off-by: Andrey Okoshkin \n\nThis is\nReviewed-by: Stefan Beller \n"},{"id":"331335","messageId":"2b507ce0-0d18-d9d6-11ba-6fb0d4f4a473@samsung.com","threadId":"47021","inReplyTo":"CAGZ79kZ9EV=qaYyuA3kfuQ04EhLWax52MhtkmGJto2Lommc_SQ@mail.gmail.com","subject":"[PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-30T07:42:33Z","receivedAt":"2017-10-30T07:42:48Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Get 'GIT_MERGE_VERBOSITY' environment variable only once in\ninit_merge_options() and store the pointer to its value for the further check.\nNo intervening calls to getenv(), putenv(), setenv() or unsetenv() are done\nbetween the initial getenv() call and the consequential result pass to strtol()\nas these environment related functions could modify the string pointer returned\nby the initial getenv() call.\n\nSigned-off-by: Andrey Okoshkin \nReviewed-by: Stefan Beller \n---\nAdded 'reviewed-by' field.\n merge-recursive.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1494ffdb8..60084e3a0 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n \n void init_merge_options(struct merge_options *o)\n {\n+\tconst char *merge_verbosity;\n \tmemset(o, 0, sizeof(struct merge_options));\n \to->verbosity = 2;\n \to->buffer_output = 1;\n@@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n \to->renormalize = 0;\n \to->detect_rename = 1;\n \tmerge_recursive_config(o);\n-\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n-\t\to->verbosity =\n-\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n+\tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n+\tif (merge_verbosity)\n+\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n \tif (o->verbosity >= 5)\n \t\to->buffer_output = 0;\n \tstrbuf_init(&o->obuf, 0);\n-- \n2.14.3\n"},{"id":"331437","messageId":"xmqqd1543xfc.fsf@gitster.mtv.corp.google.com","threadId":"47021","inReplyTo":"2b507ce0-0d18-d9d6-11ba-6fb0d4f4a473@samsung.com","subject":"Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-31T01:42:31Z","receivedAt":"2017-10-31T01:42:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrey Okoshkin writes:\n\n> Get 'GIT_MERGE_VERBOSITY' environment variable only once in\n> init_merge_options() and store the pointer to its value for the further check.\n\nOK, that is \"what this thing does\" description.\n\n> No intervening calls to getenv(), putenv(), setenv() or unsetenv() are done\n> between the initial getenv() call and the consequential result pass to strtol()\n> as these environment related functions could modify the string pointer returned\n> by the initial getenv() call.\n\nThat holds true for the code before or after this patch equally. In\nother words, that sounds like a justification for rejecting this\npatch (i.e. explanation of why this change is not needed).\n\nIf we are worried about another thread calling these functions after\nthe time we call getenv() and before the time we pass the result to\nstrtol(), then I do not think this patch gives a better protection\nagainst such race, so I do not think that is why you are doing this.\n\nSo... why do we want to do this change? I am puzzled.\n\n\n>\n> Signed-off-by: Andrey Okoshkin \n> Reviewed-by: Stefan Beller \n> ---\n> Added 'reviewed-by' field.\n> merge-recursive.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 1494ffdb8..60084e3a0 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n> \n> void init_merge_options(struct merge_options *o)\n> {\n> +\tconst char *merge_verbosity;\n> \tmemset(o, 0, sizeof(struct merge_options));\n> \to->verbosity = 2;\n> \to->buffer_output = 1;\n> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n> \to->renormalize = 0;\n> \to->detect_rename = 1;\n> \tmerge_recursive_config(o);\n> -\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n> -\t\to->verbosity =\n> -\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n> +\tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n> +\tif (merge_verbosity)\n> +\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n> \tif (o->verbosity >= 5)\n> \t\to->buffer_output = 0;\n> \tstrbuf_init(&o->obuf, 0);\n"},{"id":"331440","messageId":"xmqq37603vek.fsf@gitster.mtv.corp.google.com","threadId":"47021","inReplyTo":"xmqqd1543xfc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-31T02:26:11Z","receivedAt":"2017-10-31T02:26:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> That holds true for the code before or after this patch equally. In\n> other words, that sounds like a justification for rejecting this\n> patch (i.e. explanation of why this change is not needed).\n>\n> If we are worried about another thread calling these functions after\n> the time we call getenv() and before the time we pass the result to\n> strtol(), then I do not think this patch gives a better protection\n> against such race, so I do not think that is why you are doing this.\n>\n> So... why do we want to do this change? I am puzzled.\n\nFor the sake of fairness, I would say that the resulting code may be\neasier to follow and has one less instance of a constant string that\nthe compiler cannot statically check if we made a typo. That's the\nonly benefit in this patch as far as I can see.\n\nThe original makes a call to see if the result is NULL, and then\nmakes the same call, expecting that we get the same result (not just\nthat it is not NULL, but it is the same verbosity request the end\nuser made via the environment as the one we checked earlier), and I\ncan understand that it feels a bit redundant and ugly.\n\n\n>> Signed-off-by: Andrey Okoshkin \n>> Reviewed-by: Stefan Beller \n>> ---\n>> Added 'reviewed-by' field.\n>> merge-recursive.c | 7 ++++---\n>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 1494ffdb8..60084e3a0 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n>> \n>> void init_merge_options(struct merge_options *o)\n>> {\n>> +\tconst char *merge_verbosity;\n>> \tmemset(o, 0, sizeof(struct merge_options));\n>> \to->verbosity = 2;\n>> \to->buffer_output = 1;\n>> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n>> \to->renormalize = 0;\n>> \to->detect_rename = 1;\n>> \tmerge_recursive_config(o);\n>> -\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n>> -\t\to->verbosity =\n>> -\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n>> +\tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n>> +\tif (merge_verbosity)\n>> +\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n>> \tif (o->verbosity >= 5)\n>> \t\to->buffer_output = 0;\n>> \tstrbuf_init(&o->obuf, 0);\n"},{"id":"331461","messageId":"ecb12f21-f26f-6905-1fdc-60d7c282cfe3@samsung.com","threadId":"47021","inReplyTo":"xmqq37603vek.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-31T07:13:00Z","receivedAt":"2017-10-31T07:13:18Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"31.10.2017 05:26, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> That holds true for the code before or after this patch equally. In\n>> other words, that sounds like a justification for rejecting this\n>> patch (i.e. explanation of why this change is not needed).\n>>\n>> If we are worried about another thread calling these functions after\n>> the time we call getenv() and before the time we pass the result to\n>> strtol(), then I do not think this patch gives a better protection\n>> against such race, so I do not think that is why you are doing this.\n>>\n>> So... why do we want to do this change? I am puzzled.\n\nI think, the main benefits are:\n* Code is more readable, no duplicated calls with the same constant string\nargument.\n* Code is potentially safer, the second getenv() call may return another\npointer value which could be NULL (and yes, this is an arguable point as it\ncan be done only artificially).\n\n> For the sake of fairness, I would say that the resulting code may be\n> easier to follow and has one less instance of a constant string that\n> the compiler cannot statically check if we made a typo. That's the\n> only benefit in this patch as far as I can see.\n> \n> The original makes a call to see if the result is NULL, and then\n> makes the same call, expecting that we get the same result (not just\n> that it is not NULL, but it is the same verbosity request the end\n> user made via the environment as the one we checked earlier), and I\n> can understand that it feels a bit redundant and ugly.\n\nYes, you absolutely right.\nI believe this patch makes code more beautiful :-)\n\n>>> Signed-off-by: Andrey Okoshkin \n>>> Reviewed-by: Stefan Beller \n>>> ---\n>>> Added 'reviewed-by' field.\n>>> merge-recursive.c | 7 ++++---\n>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/merge-recursive.c b/merge-recursive.c\n>>> index 1494ffdb8..60084e3a0 100644\n>>> --- a/merge-recursive.c\n>>> +++ b/merge-recursive.c\n>>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n>>> \n>>> void init_merge_options(struct merge_options *o)\n>>> {\n>>> +\tconst char *merge_verbosity;\n>>> \tmemset(o, 0, sizeof(struct merge_options));\n>>> \to->verbosity = 2;\n>>> \to->buffer_output = 1;\n>>> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n>>> \to->renormalize = 0;\n>>> \to->detect_rename = 1;\n>>> \tmerge_recursive_config(o);\n>>> -\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n>>> -\t\to->verbosity =\n>>> -\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n>>> +\tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n>>> +\tif (merge_verbosity)\n>>> +\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n>>> \tif (o->verbosity >= 5)\n>>> \t\to->buffer_output = 0;\n>>> \tstrbuf_init(&o->obuf, 0);\n> \n> \n> \n\n-- \nBest regards,\nAndrey Okoshkin\n"},{"id":"331463","messageId":"xmqqwp3byeai.fsf@gitster.mtv.corp.google.com","threadId":"47021","inReplyTo":"ecb12f21-f26f-6905-1fdc-60d7c282cfe3@samsung.com","subject":"Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-31T07:20:05Z","receivedAt":"2017-10-31T07:20:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrey Okoshkin writes:\n\n> I think, the main benefits are:\n> * Code is more readable, no duplicated calls with the same constant string\n> argument.\n> * Code is potentially safer, the second getenv() call may return another\n> pointer value which could be NULL (and yes, this is an arguable point as it\n> can be done only artificially).\n>\n>> For the sake of fairness, I would say that the resulting code may be\n>> easier to follow and has one less instance of a constant string that\n>> the compiler cannot statically check if we made a typo. That's the\n>> only benefit in this patch as far as I can see.\n>> \n>> The original makes a call to see if the result is NULL, and then\n>> makes the same call, expecting that we get the same result (not just\n>> that it is not NULL, but it is the same verbosity request the end\n>> user made via the environment as the one we checked earlier), and I\n>> can understand that it feels a bit redundant and ugly.\n>\n> Yes, you absolutely right.\n\nI am absolutely right when I say your \"code is potentially safer\" is\ntotal BS. The result from first getenv() call may be pointing at an\ninvalid piece of memory by the time it is used, if you are in a\nsituation in which not having the second getenv() matters\n(i.e. somebody else is also mucking with getenv() at the same time).\n\nSo please update the log message so that the patch is not sold on\nthat basis.\n\nThanks.\n"},{"id":"331466","messageId":"f0d63948-80f3-b2d8-8623-49ae13ca87bf@samsung.com","threadId":"47021","inReplyTo":"xmqqwp3byeai.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v5] merge-recursive: check GIT_MERGE_VERBOSITY only once","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-31T09:09:13Z","receivedAt":"2017-10-31T09:09:26Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Get rid of the duplicated getenv('GIT_MERGE_VERBOSITY') calls with the same\nconstant string argument. This makes code more readable and prevents typo in\nthe further development.\n\nSigned-off-by: Andrey Okoshkin \nReviewed-by: Stefan Beller \n---\nCommit message is reworked according to the feedback.\n merge-recursive.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1494ffdb8..60084e3a0 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)\n \n void init_merge_options(struct merge_options *o)\n {\n+\tconst char *merge_verbosity;\n \tmemset(o, 0, sizeof(struct merge_options));\n \to->verbosity = 2;\n \to->buffer_output = 1;\n@@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)\n \to->renormalize = 0;\n \to->detect_rename = 1;\n \tmerge_recursive_config(o);\n-\tif (getenv(\"GIT_MERGE_VERBOSITY\"))\n-\t\to->verbosity =\n-\t\t\tstrtol(getenv(\"GIT_MERGE_VERBOSITY\"), NULL, 10);\n+\tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n+\tif (merge_verbosity)\n+\t\to->verbosity = strtol(merge_verbosity, NULL, 10);\n \tif (o->verbosity >= 5)\n \t\to->buffer_output = 0;\n \tstrbuf_init(&o->obuf, 0);\n-- \n2.14.3\n\n"}]}