Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-25 Thread Andrey Okoshkin
24.10.2017 22:52, Jeff King wrote: > On Tue, Oct 24, 2017 at 07:11:24PM +0200, Martin Ågren wrote: > >> On 24 October 2017 at 18:45, Eric Sunshine wrote: >>> On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote: On Tue, Oct 24, 2017 at 8:27

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-25 Thread Andrey Okoshkin
Thanks for your review. 24.10.2017 19:28, Stefan Beller wrote: > So I think this function is never called from within a threaded environment > in git. You are right, it's just a hypothetic case. > Despite not being in a threaded environment, I wonder if we want to > minimize the time between

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-25 Thread Andrey Okoshkin
24.10.2017 19:45, Eric Sunshine wrote: > I feel uncomfortable about this change, not due to lack of thread > safety, but due to the distance between the getenv() invocation and > its point of use. See below for more detail. Thanks, the usage must be just after the assignment. -- Best regards,

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-25 Thread Jeff King
On Wed, Oct 25, 2017 at 12:07:12AM -0400, Eric Sunshine wrote: > On Tue, Oct 24, 2017 at 9:48 PM, Junio C Hamano wrote: > > Jeff King writes: > >> I definitely agree with the sentiment that as few things as possible > >> should happen between calling getenv()

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-24 Thread Eric Sunshine
On Tue, Oct 24, 2017 at 9:48 PM, Junio C Hamano wrote: > Jeff King writes: >> I definitely agree with the sentiment that as few things as possible >> should happen between calling getenv() and using its result. I've seen >> real bugs there from unexpected

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-24 Thread Junio C Hamano
Jeff King writes: > I definitely agree with the sentiment that as few things as possible > should happen between calling getenv() and using its result. I've seen > real bugs there from unexpected invalidation of the static buffer. Sure. I do not think we mind xstrdup()ing the

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-24 Thread Jeff King
On Tue, Oct 24, 2017 at 07:11:24PM +0200, Martin Ågren wrote: > On 24 October 2017 at 18:45, Eric Sunshine wrote: > > On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote: > >> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-24 Thread Martin Ågren
On 24 October 2017 at 18:45, Eric Sunshine wrote: > On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote: >> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin >> wrote: >>> Add check of 'GIT_MERGE_VERBOSITY' environment

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-24 Thread Eric Sunshine
On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote: > On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin > wrote: >> Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in >> init_merge_options(). >> Consequential call of getenv() may

Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-24 Thread Stefan Beller
On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin wrote: > Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in > init_merge_options(). > Consequential call of getenv() may return NULL pointer and strtol() crashes. > However the stored pointer to the

[PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

2017-10-24 Thread Andrey Okoshkin
Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in init_merge_options(). Consequential call of getenv() may return NULL pointer and strtol() crashes. However the stored pointer to the obtained getenv() result may be invalidated by some other getenv() call from another thread as