Even if we make that change to use TLS for this case, the implementation of pread() will still change in such a way that the semantics of pread() are different on Windows. Is that ok?
Just to summarize, here's the viable approaches I've seen discussed so far: 1) Use _WINVER at compile time to select either a thread-safe or non-thread-safe implementation of pread. This is the easiest possible code change, but would necessitate 2 binary distributions of git for windows. 2) Use TLS as you suggest and have one fd per pack thread. Probably the most complicated code change (at least for me, being a first-time contributor) 3) Use Karsten's suggested implementation from earlier in the thread. Seems to work, but it's a little confusing from a readability standpoint since the implementation is not-thread safe except in this specific usage context. On Fri, Feb 14, 2014 at 4:56 PM, Duy Nguyen <pclo...@gmail.com> wrote: > On Sat, Feb 15, 2014 at 7:50 AM, Stefan Zager <sza...@google.com> wrote: >> On Fri, Feb 14, 2014 at 4:45 PM, Duy Nguyen <pclo...@gmail.com> wrote: >>> On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner <ztur...@chromium.org> >>> wrote: >>>> (Gah, sorry if you're receiving multiple emails to your personal >>>> addresses, I need to get used to manually setting Plain-text mode >>>> every time I send a message). >>>> >>>> For the mixed read, we wouldn't be looking for another caller of >>>> pread() (since it doesn't care what the file pointer is), but instead >>>> a caller of read() or lseek() (since those do depend on the current >>>> file pointer). In index-pack.c, I see two possible culprits: >>>> >>>> 1) A call to xread() from inside fill() >>>> 2) A call to lseek in parse_pack_objects() >>>> >>>> Do you think these could be related? If so, maybe that opens up some >>>> other solutions? >>> >>> For index-pack alone, what's wrong with open one file handle per thread? >> >> Nothing wrong with that, except that it would mean either using >> thread-local storage (which the code doesn't currently use); or >> plumbing pack_fd through the call stack, which doesn't sound very fun. > > Current code does use thread-local storage (struct thread_local and > get_thread_data). Adding a new file handle when NO_THREAD_SAFE_PREAD > is defined is simpler imo. > -- > Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
