Re: Problem with Integrated Vim Editor on Win 10
I dug into this some more, and as surprising as it is, I believe the release of Git 2.8.0 is just busted. I had an installer for 2.7.0 lying around, so after uninstalling 2.8.0 and re-installing 2.7.0, everything works fine. I'm not terribly active in the Git community so I don't know what the procedure is for things like this, but this seems like a fairly serious regression. Suggestions on how to proceed? On Wed, Mar 30, 2016 at 5:07 PM, Zachary Turner wrote: > Hi, just recently I installed the latest build of Windows 10 of my > machine. This is my second Win10 machine. On the other I am using git > 2.7.0.windows.1 and everything is working just fine. > > On the second machine I am using git 2.8.0.windows.1 and vim does not > work. I sent a bug report to b...@vim.org, but frankly I don't know whose > bug this is, so I'm including it here as well. > > The problem is that vim is just a black screen when git launches it. If I > mash enough keys eventually I see something that resembles vim output at > the bottom, but I can't actually use it. > > I tried going into program files\git\usr\bin and just running vim.exe. > Again, black screen. If I press enter about 10 times I can see the > introduction screen. Then if I press : about 10-20 times it will go into > command mode and a single : appears. after pressing a few more keys all > the rest of the :s appear. Basically, everything is completely unusable. > > I tried downloading vim 7.4 from www.vim.org, and low and behold, it > works. For now I've replaced the copy of vim.exe that ships with git with > the copy from www.vim.org. But this leaves me nervous that something is > seriously wrong. > > Has anyone seen anything like this before, or have any ideas how I might > better diagnose this? -- 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
Problem with Integrated Vim Editor on Win 10
Hi, just recently I installed the latest build of Windows 10 of my machine. This is my second Win10 machine. On the other I am using git 2.7.0.windows.1 and everything is working just fine. On the second machine I am using git 2.8.0.windows.1 and vim does not work. I sent a bug report to b...@vim.org, but frankly I don't know whose bug this is, so I'm including it here as well. The problem is that vim is just a black screen when git launches it. If I mash enough keys eventually I see something that resembles vim output at the bottom, but I can't actually use it. I tried going into program files\git\usr\bin and just running vim.exe. Again, black screen. If I press enter about 10 times I can see the introduction screen. Then if I press : about 10-20 times it will go into command mode and a single : appears. after pressing a few more keys all the rest of the :s appear. Basically, everything is completely unusable. I tried downloading vim 7.4 from www.vim.org, and low and behold, it works. For now I've replaced the copy of vim.exe that ships with git with the copy from www.vim.org. But this leaves me nervous that something is seriously wrong. Has anyone seen anything like this before, or have any ideas how I might better diagnose this? -- 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
Re: Make the git codebase thread-safe
It shouldn't be hard for us to run some tests with this patch applied. Will report back in a day or two. On Tue, Feb 18, 2014 at 9:55 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Sat, Feb 15, 2014 at 8:15 AM, Zachary Turner wrote: >> ... >>> 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) >> >> It's not so complicated. I suggested a patch [1] before (surprise!). >> ... >> [1] http://article.gmane.org/gmane.comp.version-control.git/196042 > > That message is at the tail end of the discussion. I wonder why > nothing came out of it back then. > > While I do not see anything glaringly wrong with the change from a > quick glance over it, it would be nice to hear how well it performs > on their platform from Windows folks. > > Thanks. -- 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
Re: Make the git codebase thread-safe
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 wrote: > On Sat, Feb 15, 2014 at 7:50 AM, Stefan Zager wrote: >> On Fri, Feb 14, 2014 at 4:45 PM, Duy Nguyen wrote: >>> On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner >>> 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
Re: Make the git codebase thread-safe
(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? BTW, the version you posted isn't thread safe. Suppose thread A and thread B execute this function at the same time. A executes through the ReadFile(), but does not yet reset the second lseek64. B then executes the first lseek64(), storing off the modified file pointer. Then A finishes, then B finishes. At the end, the file pointer is still modified. On Fri, Feb 14, 2014 at 11:15 AM, Zachary Turner wrote: > 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(). 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? > > BTW, the version you posted isn't thread safe. Suppose thread A and thread > B execute this function at the same time. A executes through the > ReadFile(), but does not yet reset the second lseek64. B then executes the > first lseek64(), storing off the modified file pointer. Then A finishes, > then B finishes. At the end, the file pointer is still modified. > > > > On Fri, Feb 14, 2014 at 11:04 AM, Karsten Blees > wrote: >> >> Am 14.02.2014 00:09, schrieb Zachary Turner: >> > To elaborate a little bit more, you can verify with a sample program >> > that ReadFile with OVERLAPPED does in fact modify the HANDLE's file >> > position. The documentation doesn't actually state one way or >> > another. My original attempt at a patch didn't have the ReOpenFile, >> > and we experienced regular read corruption. We scratched our heads >> > over it for a bit, and then hypothesized that someone must be mixing >> > read styles, which led to this ReOpenFile workaround, which >> > incidentally also solved the corruption problems. We wrote a similar >> > sample program to verify that when using ReOpenHandle, and changing >> > the file pointer of the duplicated handle, that the file pointer of >> > the original handle is not modified. >> > >> > We did not actually try to identify the source of the mixed read >> > styles, but it seems like the only possible explanation. >> > >> > On Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager wrote: >> >> On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees >> >> wrote: >> >>> Am 13.02.2014 19:38, schrieb Zachary Turner: >> >>> >> >>>> The only reason ReOpenFile is necessary at >> >>>> all is because some code somewhere is mixing read-styles against the >> >>>> same >> >>>> fd. >> >>>> >> >>> >> >>> I don't understand...ReadFile with OVERLAPPED parameter doesn't modify >> >>> the HANDLE's file position, so you should be able to mix read()/pread() >> >>> however you like (as long as read() is only called from one thread). >> >> >> >> That is, apparently, a bald-faced lie in the ReadFile API doc. First >> >> implementation didn't use ReOpenFile, and it crashed all over the >> >> place. ReOpenFile fixed it. >> >> >> >> Stefan >> >> Damn...you're right, multi-threaded git-index-pack works fine, but some >> tests fail badly. Mixed reads would have to be from git_mmap, which is the >> only other caller of pread(). >> >> A simple alternative to ReOpenHandle is to reset the file pointer to its >> original position, as in compat/pread.c::git_pread. Thus single-theaded code >> can mix read()/pread() at will, but multi-threaded code has to use pread() >> exclusively (which is usually the case anyway). A main thread using read() >> and background threads using pread() (which is technically allowed by POSIX) >> will fail with this solution. >> >> This version passes the test suite on msysgit: >> >> 8< >> ssize_t mingw_pread(int fd, void *b
Re: Make the git codebase thread-safe
To elaborate a little bit more, you can verify with a sample program that ReadFile with OVERLAPPED does in fact modify the HANDLE's file position. The documentation doesn't actually state one way or another. My original attempt at a patch didn't have the ReOpenFile, and we experienced regular read corruption. We scratched our heads over it for a bit, and then hypothesized that someone must be mixing read styles, which led to this ReOpenFile workaround, which incidentally also solved the corruption problems. We wrote a similar sample program to verify that when using ReOpenHandle, and changing the file pointer of the duplicated handle, that the file pointer of the original handle is not modified. We did not actually try to identify the source of the mixed read styles, but it seems like the only possible explanation. On Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager wrote: > On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees > wrote: >> Am 13.02.2014 19:38, schrieb Zachary Turner: >> >>> The only reason ReOpenFile is necessary at >>> all is because some code somewhere is mixing read-styles against the same >>> fd. >>> >> >> I don't understand...ReadFile with OVERLAPPED parameter doesn't modify the >> HANDLE's file position, so you should be able to mix read()/pread() however >> you like (as long as read() is only called from one thread). > > That is, apparently, a bald-faced lie in the ReadFile API doc. First > implementation didn't use ReOpenFile, and it crashed all over the > place. ReOpenFile fixed it. > > Stefan -- 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
Re: Make the git codebase thread-safe
Karsten Blees gmail.com> writes: > > Am 12.02.2014 19:37, schrieb Erik Faye-Lund: > > On Wed, Feb 12, 2014 at 7:34 PM, Stefan Zager google.com> wrote: > >> On Wed, Feb 12, 2014 at 10:27 AM, Erik Faye-Lund gmail.com> wrote: > >>> On Wed, Feb 12, 2014 at 7:20 PM, Stefan Zager google.com> wrote: > > I don't want to steal the thunder of my coworker, who wrote the > implementation. He plans to submit it upstream soon-ish. It relies > on using the lpOverlapped argument to ReadFile(), with some additional > tomfoolery to make sure that the implicit position pointer for the > file descriptor doesn't get modified. > >>> > >>> Is the code available somewhere? I'm especially interested in the > >>> "additional tomfoolery to make sure that the implicit position pointer > >>> for the file descriptor doesn't get modified"-part, as this was what I > >>> ended up butting my head into when trying to do this myself. > >> > >> https://chromium-review.googlesource.com/#/c/186104/ > > > > ReOpenFile, that's fantastic. Thanks a lot! > > ...but should be loaded dynamically via GetProcAddress, or are we ready to drop XP support? > > Original patch author here. In trying to prepare this patch to use GetProcAddress to load dynamically, I've run into a bit of a snag. NO_THREAD_SAFE_PREAD is a compile-time flag, which will be incompatible with any attempt to make this a runtime decision a la LoadLibrary / GetProcAddress. On XP, we would need to fallback to the single-threaded path, and on Vista+ we would use the thread-able path, and obviously this decision could not be made until runtime. If MinGW were the only configuration using NO_THREAD_SAFE_PREAD, I would just remove it entirely, but it appears Cygwin configuration uses it also. Suggestions? One possibility is to disallow (by convention, perhaps), the use of pread() and read() against the same fd. The only reason ReOpenFile is necessary at all is because some code somewhere is mixing read-styles against the same fd. -- 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
