# First cut at git port to Cygwin 62 messages from 2005-09-29 to 2005-10-10. Participants: H. Peter Anvin, Junio C Hamano, Martin Langhoff, Johannes Schindelin, Alex Riesen, Christopher Faylor, Jonas Fonseca, Davide Libenzi, Linus Torvalds, Chuck Lever, Elfyn McBratney, Matthias Urlichs, Daniel Barkalow. Thread: https://gitlist.dev/t/1969 ## H. Peter Anvin, 2005-09-29 00:53 Subject: First cut at git port to Cygwin Message-ID: <433B3B10.5050407@zytor.com> URL: https://gitlist.dev/e/433B3B10.5050407%40zytor.com ``` I have made a first cut at a git port to Cygwin. It looks like the "git-diff-tree -p" problem has been resolved independently, or at least I can't reproduce it on a fresh Cygwin install (running on XP Home), but I have added support for running without the IPv6 and the getaddrinfo() API. There are still funnies. In particular, Cygwin and Samba handle symlinks differently, so you can't trivially share a repository via Samba. Linus' "symbolic refs" changes should eventually take care of that. Another funny which I haven't been able to figure out yet is that 'gitk' scrunches all its output up into a few pixels at the top of the window. If I maximize the window, I can manually resize most of the panes and the output looks correct, but the highlighted text in the top panes show up in black on a really really dark blue background and is thus illegible. I have set up a git-on-Cygwin temporary tree at: http://www.kernel.org/pub/scm/git/git-cygwin.git -hpa ``` ## Junio C Hamano, 2005-09-29 04:30 Subject: Re: First cut at git port to Cygwin Message-ID: <7v64skpkbb.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v64skpkbb.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <433B3B10.5050407@zytor.com> ``` "H. Peter Anvin" writes: > There are still funnies. In particular, Cygwin and Samba handle > symlinks differently, so you can't trivially share a repository via > Samba. Linus' "symbolic refs" changes should eventually take care of that. I just sent out "The other side of Linus' symbolic refs" patch, saying that Cygwin capable of doing symlink would probably made it irrelevant. But it may not be a waste after all, considering what you said above. ``` ## Martin Langhoff, 2005-09-29 04:46 Subject: Re: First cut at git port to Cygwin Message-ID: <46a038f905092821462d08f86a@mail.gmail.com> URL: https://gitlist.dev/e/46a038f905092821462d08f86a%40mail.gmail.com In-Reply-To: <433B3B10.5050407@zytor.com> ``` On 9/29/05, H. Peter Anvin wrote: > Another funny which I haven't been able to figure out yet is that 'gitk' > scrunches all its output up into a few pixels at the top of the window. > If I maximize the window, I can manually resize most of the panes and > the output looks correct This is visible on OSX too, and someone's mentioned it's a Tk oddity with rootless X. Does it do the same if you run X with a root window? > I have set up a git-on-Cygwin temporary tree at: > > http://www.kernel.org/pub/scm/git/git-cygwin.git Getting a 404 on that. Doesn't show up on gitweb either. I guess I have to wait... Is there a way to get gitweb to "compare" branches using git-cherry? I often look at branches via git-web and it's impossible to tell what makes them unique... cheers, martin ``` ## H. Peter Anvin, 2005-09-29 05:07 Subject: Re: First cut at git port to Cygwin Message-ID: <433B767E.7050704@zytor.com> URL: https://gitlist.dev/e/433B767E.7050704%40zytor.com In-Reply-To: <7v64skpkbb.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > "H. Peter Anvin" writes: > > >>There are still funnies. In particular, Cygwin and Samba handle >>symlinks differently, so you can't trivially share a repository via >>Samba. Linus' "symbolic refs" changes should eventually take care of that. > > > I just sent out "The other side of Linus' symbolic refs" patch, > saying that Cygwin capable of doing symlink would probably made > it irrelevant. But it may not be a waste after all, considering > what you said above. > After looking at it some more, what Samba does when talking to a host that doesn't support Unix extensions is that it simply resolves the symlink, in effect turning it into a hard link. That might be all git needs. The reverse still doesn't work, though. -hpa ``` ## Junio C Hamano, 2005-09-29 05:13 Subject: Re: First cut at git port to Cygwin Message-ID: <7vd5mso3ql.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vd5mso3ql.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <46a038f905092821462d08f86a@mail.gmail.com> ``` Martin Langhoff writes: > Is there a way to get gitweb to "compare" branches using git-cherry? I > often look at branches via git-web and it's impossible to tell what > makes them unique... Now that you mention it, I felt that too. Maybe git-show-branch output could help somehow? ``` ## H. Peter Anvin, 2005-09-29 06:19 Subject: Re: First cut at git port to Cygwin Message-ID: <433B877B.6010004@zytor.com> URL: https://gitlist.dev/e/433B877B.6010004%40zytor.com In-Reply-To: <46a038f905092821462d08f86a@mail.gmail.com> ``` Martin Langhoff wrote: > >>I have set up a git-on-Cygwin temporary tree at: >> >>http://www.kernel.org/pub/scm/git/git-cygwin.git > > Getting a 404 on that. Doesn't show up on gitweb either. I guess I > have to wait... > Well, it's up there now. -hpa ``` ## Johannes Schindelin, 2005-09-29 08:46 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0509291043580.20717%40wgmdd8.biozentrum.uni-wuerzburg.de In-Reply-To: <433B3B10.5050407@zytor.com> ``` Hi, On Wed, 28 Sep 2005, H. Peter Anvin wrote: > Another funny which I haven't been able to figure out yet is that 'gitk' > scrunches all its output up into a few pixels at the top of the window. See my mail about rootless X11. I went about working around that particular Tk bug by specifying the dimensions of the panes explicitely. However, I was not especially happy with my workaround, since it did not reproduce the layout exactly after a restart. Maybe you can figure it out how to do that. Ciao, Dscho ``` ## H. Peter Anvin, 2005-09-29 16:11 Subject: Re: First cut at git port to Cygwin Message-ID: <433C122B.3050509@zytor.com> URL: https://gitlist.dev/e/433C122B.3050509%40zytor.com In-Reply-To: ``` Johannes Schindelin wrote: > Hi, > > On Wed, 28 Sep 2005, H. Peter Anvin wrote: > >>Another funny which I haven't been able to figure out yet is that 'gitk' >>scrunches all its output up into a few pixels at the top of the window. > > See my mail about rootless X11. I went about working around that > particular Tk bug by specifying the dimensions of the panes explicitely. > However, I was not especially happy with my workaround, since it did not > reproduce the layout exactly after a restart. Maybe you can figure it out > how to do that. > Unlikely, since I'm a complete Tcl/Tk illiterate. -hpa ``` ## H. Peter Anvin, 2005-09-29 17:25 Subject: Re: First cut at git port to Cygwin Message-ID: <433C237A.20401@zytor.com> URL: https://gitlist.dev/e/433C237A.20401%40zytor.com In-Reply-To: ``` Johannes Schindelin wrote: > Hi, > > On Wed, 28 Sep 2005, H. Peter Anvin wrote: > >>Another funny which I haven't been able to figure out yet is that 'gitk' >>scrunches all its output up into a few pixels at the top of the window. > > See my mail about rootless X11. I went about working around that > particular Tk bug by specifying the dimensions of the panes explicitely. > However, I was not especially happy with my workaround, since it did not > reproduce the layout exactly after a restart. Maybe you can figure it out > how to do that. > It looks like this isn't a rootless *X* thing; it looks like the wish that is included with Cygwin actually opens native Win32 windows; even when run from inside a rooted X session it still opens an external window. I also tried using the wish from the latest ActiveState distribution; it exhibits the same problem although with slightly different geometries. -hpa ``` ## Junio C Hamano, 2005-09-30 10:02 Subject: Re: First cut at git port to Cygwin Message-ID: <7v4q826ffy.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v4q826ffy.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <433B3B10.5050407@zytor.com> ``` "H. Peter Anvin" writes: > I have set up a git-on-Cygwin temporary tree at: > > http://www.kernel.org/pub/scm/git/git-cygwin.git : siamese; git clone http://kernel.org/pub/scm/git/git-cygwin.git/ git-cygwin defaulting to local storage area Cannot get remote repository information. Perhaps git-update-server-info needs to be run there? Could you do update-server-info there, please? hera$ cd /pub/scm/git/git-cygwin.git hera$ GIT_DIR=. git-update-server-info Knowing nothing about Cygwin environment, here are some comments. +# Define NO_IPV6 if you lack IPv6 support and getaddrinfo(). This part probably is applicable outside Cygwin. At some point, can we have it in the mainline please? # The ones that do not have to link with lcrypto nor lz. SIMPLE_PROGRAMS = \ - git-get-tar-commit-id git-mailinfo git-mailsplit git-stripspace \ - git-daemon git-var + git-get-tar-commit-id$(X) git-mailinfo$(X) git-mailsplit$(X) \ + git-stripspace$(X) git-var$(X) git-daemon$(X) I have seen these $(X) in other programs' ports and found them quite distasteful. Since I do not have immediate suggestions for improvements, I do not have rights to complain, though. Spelling it $X is a bit less distracting but not that much better. Maybe "SIMPLE_PROGRAM_NAMES = git-foo git-bar" and "SIMPLE_PROGRAMS = $(patsubst %,%$X,$(SIMPLE_PROGRAM_NAMES))"... but that would not help bits like this: - PROGRAMS += git-http-fetch + PROGRAMS += git-http-fetch$(X) or this: -git-%: %.o $(LIB_FILE) +git-%$(X): %.o $(LIB_FILE) ... so I'd shut up about this part. diff --git a/daemon.c b/daemon.c --- a/daemon.c +++ b/daemon.c @@ -1,9 +1,11 @@ #include "cache.h" #include "pkt-line.h" +#include Why? I do not see any use of alloca in the added code... +#include Is poll preferrable over select in general? Some may have only select available and others may have only poll available, perhaps? In any case, this is probably relevant to wider audience than just Cygwin; please give it to mainline at some point, perhaps conditionally allowing either/both. + *socklist_p = malloc(sizeof(int)); + pfd = calloc(socknum, sizeof(struct pollfd)); Please use xmalloc and xcalloc just for consistency. test -x $path/git-$cmd && exec $path/git-$cmd "$@" ;; + + # In case we're running on Cygwin... + test -x $path/git-$cmd.exe && exec $path/git-$cmd.exe "$@" ;; esac Hmph, I think you forgot to drop double semicolon there. The git.sh script is munged by Makefile so presumably we could fix this part up there, like: git: git.sh Makefile rm -f $@+ $@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH)|' \ -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ -e 's/@@X@@/$X/g' <$@.sh >$@+ chmod +x $@+ mv $@+ $@ And then (a patch on top of your "master"): diff --git a/git.sh b/git.sh --- a/git.sh +++ b/git.sh @@ -12,10 +12,14 @@ case "$#" in exit 0 ;; esac - test -x $path/git-$cmd && exec $path/git-$cmd "$@" ;; + test -x $path/git-$cmd && exec $path/git-$cmd "$@" - # In case we're running on Cygwin... - test -x $path/git-$cmd.exe && exec $path/git-$cmd.exe "$@" ;; + case '@@X@@' in + '') + ;; + *) + test -x $path/git-$cmd@@X@@ && exec $path/git-$cmd@@X@@ "$@" ;; + esac esac echo "Usage: git COMMAND [OPTIONS] [TARGET]" ``` ## H. Peter Anvin, 2005-09-30 17:01 Subject: Re: First cut at git port to Cygwin Message-ID: <433D6F62.3030906@zytor.com> URL: https://gitlist.dev/e/433D6F62.3030906%40zytor.com In-Reply-To: <7v4q826ffy.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > > Could you do update-server-info there, please? > Done... > > Knowing nothing about Cygwin environment, here are some > comments. > > +# Define NO_IPV6 if you lack IPv6 support and getaddrinfo(). > > This part probably is applicable outside Cygwin. At some point, > can we have it in the mainline please? > Well, I would hope that all the changes could eventually be merged. > # The ones that do not have to link with lcrypto nor lz. > SIMPLE_PROGRAMS = \ > - git-get-tar-commit-id git-mailinfo git-mailsplit git-stripspace \ > - git-daemon git-var > + git-get-tar-commit-id$(X) git-mailinfo$(X) git-mailsplit$(X) \ > + git-stripspace$(X) git-var$(X) git-daemon$(X) > > I have seen these $(X) in other programs' ports and found them > quite distasteful. Since I not have immediate suggestions > for improvements, I do not have rights to complain, though. > > Spelling it $X is a bit less distracting but not that much > better. Maybe "SIMPLE_PROGRAM_NAMES = git-foo git-bar" and > "SIMPLE_PROGRAMS = $(patsubst %,%$X,$(SIMPLE_PROGRAM_NAMES))"... > but that would not help bits like this: > > - PROGRAMS += git-http-fetch > + PROGRAMS += git-http-fetch$(X) > > or this: > > -git-%: %.o $(LIB_FILE) > +git-%$(X): %.o $(LIB_FILE) > > ... so I'd shut up about this part. My first cut had PROGRAMS_X and SIMPLE_PROGRAMS_X being patsubst of the original versions, but in the end I decided it was even uglier, because these patterns were needed elsewhere. I'll change them to $X except where the parens are needed. > diff --git a/daemon.c b/daemon.c > --- a/daemon.c > +++ b/daemon.c > @@ -1,9 +1,11 @@ > #include "cache.h" > #include "pkt-line.h" > +#include > > Why? I do not see any use of alloca in the added code... I originally used alloca() before changing my mind and using calloc(); I think there might be platforms without alloca out there. > +#include > > Is poll preferrable over select in general? Some may have only > select available and others may have only poll available, > perhaps? In any case, this is probably relevant to wider > audience than just Cygwin; please give it to mainline at some > point, perhaps conditionally allowing either/both. The main reason I switched to poll() is that I believe all platforms that are even remotely relevant have both these days, and forming a poll list is so much cleaner than forming a select set. What makes forming a select set even remotely bearable is the invalid assumption that the number of file descriptors is bounded at compile time and therefore that fdset_t can be statically allocated. We've had problems in the past with that assumption on Linux, and I've tried to avoid select since then. > + *socklist_p = malloc(sizeof(int)); > + pfd = calloc(socknum, sizeof(struct pollfd)); > > Please use xmalloc and xcalloc just for consistency. Check. > test -x $path/git-$cmd && exec $path/git-$cmd "$@" ;; > + > + # In case we're running on Cygwin... > + test -x $path/git-$cmd.exe && exec $path/git-$cmd.exe "$@" ;; > esac > > Hmph, I think you forgot to drop double semicolon there. D'oh! > The git.sh script is munged by Makefile so presumably we could > fix this part up there, like: > > git: git.sh Makefile > rm -f $@+ $@ > sed -e '1s|#!.*/sh|#!$(SHELL_PATH)|' \ > -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ > -e 's/@@X@@/$X/g' <$@.sh >$@+ > chmod +x $@+ > mv $@+ $@ > > And then (a patch on top of your "master"): > > diff --git a/git.sh b/git.sh > --- a/git.sh > +++ b/git.sh > @@ -12,10 +12,14 @@ case "$#" in > exit 0 ;; > esac > > - test -x $path/git-$cmd && exec $path/git-$cmd "$@" ;; > + test -x $path/git-$cmd && exec $path/git-$cmd "$@" > > - # In case we're running on Cygwin... > - test -x $path/git-$cmd.exe && exec $path/git-$cmd.exe "$@" ;; > + case '@@X@@' in > + '') > + ;; > + *) > + test -x $path/git-$cmd@@X@@ && exec $path/git-$cmd@@X@@ "$@" ;; > + esac > esac > > echo "Usage: git COMMAND [OPTIONS] [TARGET]" That wouldn't work, because the shell scripts don't get the .exe extension. However, I can figure out something equivalent. -hpa ``` ## H. Peter Anvin, 2005-09-30 19:08 Subject: Re: First cut at git port to Cygwin Message-ID: <433D8D1A.9010904@zytor.com> URL: https://gitlist.dev/e/433D8D1A.9010904%40zytor.com In-Reply-To: <433D6F62.3030906@zytor.com> ``` Okay, I have updated the git-cygwin repository with the changes proposed, and they seem to work. -hpa ``` ## Alex Riesen, 2005-10-04 12:31 Subject: Re: First cut at git port to Cygwin Message-ID: <81b0412b0510040531m441ca759k6d1f3fbf0cd248ce@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510040531m441ca759k6d1f3fbf0cd248ce%40mail.gmail.com In-Reply-To: <433B3B10.5050407@zytor.com> ``` On 9/29/05, H. Peter Anvin wrote: > I have made a first cut at a git port to Cygwin. It looks like the > "git-diff-tree -p" problem has been resolved independently, or at least > I can't reproduce it on a fresh Cygwin install (running on XP Home), but > I have added support for running without the IPv6 and the getaddrinfo() API. > > There are still funnies. In particular, Cygwin and Samba handle > symlinks differently, so you can't trivially share a repository via > Samba. Linus' "symbolic refs" changes should eventually take care of that. I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove the target and returns an error (probably EPERM, but I have reasons not to trust strerror on that thing). The repository was on FAT. Taking "rename(2)" from cygwin's libiberty solved this (they unlink if link(2) returns EEXIST). PS: Does broken rename(2) qualify a system "not worthy to support"? ``` ## Alex Riesen, 2005-10-04 13:06 Subject: Re: First cut at git port to Cygwin Message-ID: <81b0412b0510040606n41a8bec6o24b47d65ff1c050b@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510040606n41a8bec6o24b47d65ff1c050b%40mail.gmail.com In-Reply-To: <81b0412b0510040531m441ca759k6d1f3fbf0cd248ce@mail.gmail.com> ``` On 10/4/05, Alex Riesen wrote: > On 9/29/05, H. Peter Anvin wrote: > > I have made a first cut at a git port to Cygwin. It looks like the > > "git-diff-tree -p" problem has been resolved independently, or at least > > I can't reproduce it on a fresh Cygwin install (running on XP Home), but > > I have added support for running without the IPv6 and the getaddrinfo() API. > > > > There are still funnies. In particular, Cygwin and Samba handle > > symlinks differently, so you can't trivially share a repository via > > Samba. Linus' "symbolic refs" changes should eventually take care of that. > > I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove the > target and returns an error (probably EPERM, but I have reasons not to trust > strerror on that thing). > The repository was on FAT. > Taking "rename(2)" from cygwin's libiberty solved this (they unlink if link(2) > returns EEXIST). I think I have to clarify: I copied the function (like in strcasestr case) into compat/ ``` ## H. Peter Anvin, 2005-10-04 14:06 Subject: Re: First cut at git port to Cygwin Message-ID: <43428C65.3040205@zytor.com> URL: https://gitlist.dev/e/43428C65.3040205%40zytor.com In-Reply-To: <81b0412b0510040531m441ca759k6d1f3fbf0cd248ce@mail.gmail.com> ``` Alex Riesen wrote: > > I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove the > target and returns an error (probably EPERM, but I have reasons not to trust > strerror on that thing). > The repository was on FAT. > Taking "rename(2)" from cygwin's libiberty solved this (they unlink if link(2) > returns EEXIST). > > PS: Does broken rename(2) qualify a system "not worthy to support"? In this case a better way would be to just add -liberty to all link lines if necessary, but I would expect the core cygwin code to do this. -hpa ``` ## H. Peter Anvin, 2005-10-04 15:03 Subject: Re: First cut at git port to Cygwin Message-ID: <434299DB.7020805@zytor.com> URL: https://gitlist.dev/e/434299DB.7020805%40zytor.com In-Reply-To: <81b0412b0510040531m441ca759k6d1f3fbf0cd248ce@mail.gmail.com> ``` Alex Riesen wrote: > > I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove the > target and returns an error (probably EPERM, but I have reasons not to trust > strerror on that thing). > The repository was on FAT. > Taking "rename(2)" from cygwin's libiberty solved this (they unlink if link(2) > returns EEXIST). > > PS: Does broken rename(2) qualify a system "not worthy to support"? I just tried this with Cygwin 1.5.18-1 and didn't have any such problems. I tried it on NTFS, FAT and Samba, using WinXP. -hpa ``` ## Christopher Faylor, 2005-10-05 03:15 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005031547.GC1393@trixie.casa.cgf.cx> URL: https://gitlist.dev/e/20051005031547.GC1393%40trixie.casa.cgf.cx In-Reply-To: <43428C65.3040205@zytor.com> ``` On Tue, Oct 04, 2005 at 07:06:29AM -0700, H. Peter Anvin wrote: >Alex Riesen wrote: >> >>I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove >>the >>target and returns an error (probably EPERM, but I have reasons not to >>trust >>strerror on that thing). >>The repository was on FAT. >>Taking "rename(2)" from cygwin's libiberty solved this (they unlink if >>link(2) >>returns EEXIST). >> >>PS: Does broken rename(2) qualify a system "not worthy to support"? > >In this case a better way would be to just add -liberty to all link >lines if necessary, but I would expect the core cygwin code to do this. AFAIK, cygwin has a working rename(). Many packages rely on it. If rename() is not working then a bug report with a test case would be appreciated. -- Christopher Faylor spammer? -> aaaspam@sourceware.org Cygwin Co-Project Leader aaaspam@duffek.com TimeSys, Inc. ``` ## Christopher Faylor, 2005-10-05 03:16 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005031642.GD1393@trixie.casa.cgf.cx> URL: https://gitlist.dev/e/20051005031642.GD1393%40trixie.casa.cgf.cx In-Reply-To: <434299DB.7020805@zytor.com> ``` On Tue, Oct 04, 2005 at 08:03:55AM -0700, H. Peter Anvin wrote: >Alex Riesen wrote: >> >>I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove >>the >>target and returns an error (probably EPERM, but I have reasons not to >>trust >>strerror on that thing). >>The repository was on FAT. >>Taking "rename(2)" from cygwin's libiberty solved this (they unlink if >>link(2) >>returns EEXIST). >> >>PS: Does broken rename(2) qualify a system "not worthy to support"? > >I just tried this with Cygwin 1.5.18-1 and didn't have any such >problems. I tried it on NTFS, FAT and Samba, using WinXP. That's a relief. Btw, AFAIK, strerror is working correctly under Cygwin also. -- Christopher Faylor spammer? -> aaaspam@sourceware.org Cygwin Co-Project Leader aaaspam@duffek.com TimeSys, Inc. ``` ## H. Peter Anvin, 2005-10-05 05:25 Subject: Re: First cut at git port to Cygwin Message-ID: <434363C2.2040501@zytor.com> URL: https://gitlist.dev/e/434363C2.2040501%40zytor.com In-Reply-To: <20051005031642.GD1393@trixie.casa.cgf.cx> ``` Christopher Faylor wrote: > That's a relief. Btw, AFAIK, strerror is working correctly under > Cygwin also. Now if we can only figure out why gitk is messed up... -hpa ``` ## Alex Riesen, 2005-10-05 11:24 Subject: Re: First cut at git port to Cygwin Message-ID: <81b0412b0510050424h21fc06bav7677911f52b38426@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510050424h21fc06bav7677911f52b38426%40mail.gmail.com In-Reply-To: <434299DB.7020805@zytor.com> ``` On 10/4/05, H. Peter Anvin wrote: > > I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove the > > target and returns an error (probably EPERM, but I have reasons not to trust > > strerror on that thing). > > The repository was on FAT. > > Taking "rename(2)" from cygwin's libiberty solved this (they unlink if link(2) > > returns EEXIST). > > > > PS: Does broken rename(2) qualify a system "not worthy to support"? > > I just tried this with Cygwin 1.5.18-1 and didn't have any such > problems. I tried it on NTFS, FAT and Samba, using WinXP. It's on Win2k, there was multiple cygwin installations in path, the other one supposedly is 1.5.5 (it's from QNX Momentics installation). I had that old "cygwin1.dll" renamed into "cygwin1.dll-disabled" long ago, though... I can't reproduce this out of GIT context, and the error is not reproducable after I removed the other cygwin installation out of PATH. Anyway, sorry, I should have tried this before posting. ``` ## Jonas Fonseca, 2005-10-05 13:16 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005131631.GA9442@diku.dk> URL: https://gitlist.dev/e/20051005131631.GA9442%40diku.dk In-Reply-To: <433B3B10.5050407@zytor.com> ``` I have a few things I experienced with the merged cygwin stuff. Sorry I haven't investigated it further, but there should be enough for a few fixes. When I ... user@machine /usr/local/dev/git/git $ make prefix=/usr/local install install -d -m755 /usr/local/bin install git-apply.exe [...] sh ./cmd-rename.sh /usr/local/bin ln: creating symbolic link `/usr/local/bin/git-http-pull.exe' to `git-http-fetch.exe': File exists make: *** [install] Error 1 Can be fixed by the patch below. I don't know if it would be cleaner to pass cmd-rename.sh "$X" as a second argument from the Makefile. --- cmd-rename.sh 2005-10-05 14:42:00.000000000 +0200 +++ cmd-rename.sh-orig 2005-10-05 14:43:48.000000000 +0200 @@ -3,7 +3,7 @@ test -d "$d" || exit while read old new do - rm -f "$d/$old" "$d/$old.exe" + rm -f "$d/$old" if test -f "$d/$new" then ln -s "$new" "$d/$old" || exit Some other obscurities ... user@machine /usr/local/dev/git/git $ git-log fatal: Not a git repository user@machine /usr/local/dev/git/git $ GIT_DIR=.git git-log | wc -l 26094 and I cannot rebuild the index file with git-reset. First time I run it it creates the index.lock file and errors out when writing. The second time it errors out because the lock file was not removed in the first case. user@machine /usr/local/dev/git/git $ GIT_DIR=.git git-reset fatal: unable to write new index file user@machine /usr/local/dev/git/git $ GIT_DIR=.git git-reset fatal: unable to create new cachefile user@machine /usr/local/dev/git/git $ uname -a CYGWIN_NT-5.1 antimatter 1.5.18(0.132/4/2) 2005-07-02 20:30 i686 unknown unknown Cygwin -- Jonas Fonseca ``` ## Johannes Schindelin, 2005-10-05 13:58 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510051556320.14244%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <20051005131631.GA9442@diku.dk> ``` Hi, On Wed, 5 Oct 2005, Jonas Fonseca wrote: > user@machine /usr/local/dev/git/git > $ git-log > fatal: Not a git repository > > user@machine /usr/local/dev/git/git > $ GIT_DIR=.git git-log | wc -l > 26094 That could have its cause in your .git/HEAD being no symlink. That happens when rsync´ing the .git directory. The other errors could also stem from the fact that quite a few places expect HEAD to be a symlink. Ciao, Dscho ``` ## Alex Riesen, 2005-10-05 15:46 Subject: Re: First cut at git port to Cygwin Message-ID: <81b0412b0510050846l2258775co117bada2d2b5a1ad@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510050846l2258775co117bada2d2b5a1ad%40mail.gmail.com In-Reply-To: <81b0412b0510050424h21fc06bav7677911f52b38426@mail.gmail.com> ``` On 10/5/05, Alex Riesen wrote: > On 10/4/05, H. Peter Anvin wrote: > > > I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove the > > > target and returns an error (probably EPERM, but I have reasons not to trust > > > strerror on that thing). > > > The repository was on FAT. > > > Taking "rename(2)" from cygwin's libiberty solved this (they unlink if link(2) > > > returns EEXIST). > > > > > > PS: Does broken rename(2) qualify a system "not worthy to support"? > > > > I just tried this with Cygwin 1.5.18-1 and didn't have any such > > problems. I tried it on NTFS, FAT and Samba, using WinXP. > > It's on Win2k, there was multiple cygwin installations in path, the other one > supposedly is 1.5.5 (it's from QNX Momentics installation). > I had that old "cygwin1.dll" renamed into "cygwin1.dll-disabled" long > ago, though... > I can't reproduce this out of GIT context, and the error is not > reproducable after > I removed the other cygwin installation out of PATH. > Anyway, sorry, I should have tried this before posting. Still does not work for me. I cannot isolate the problem out of git, but at the moment the only way for me to make commit_index_file to work is to put unlink(indexfile) before rename(cf->lockfile, indexfile). For everyone interested, I attach cygwin's strace output here. ``` ## Jonas Fonseca, 2005-10-05 15:52 Subject: [PATCH] Fix symbolic ref validation Message-ID: <20051005155212.GA16391@diku.dk> URL: https://gitlist.dev/e/20051005155212.GA16391%40diku.dk In-Reply-To: ``` Johannes Schindelin wrote Wed, Oct 05, 2005: > Hi, Hello, > On Wed, 5 Oct 2005, Jonas Fonseca wrote: > > > user@machine /usr/local/dev/git/git > > $ git-log > > fatal: Not a git repository > > > > user@machine /usr/local/dev/git/git > > $ GIT_DIR=.git git-log | wc -l > > 26094 > > That could have its cause in your .git/HEAD being no symlink. That happens > when rsync´ing the .git directory. Yes, used rsync when I cloned. Seems validate_symref() was buggy. > The other errors could also stem from the fact that quite a few places > expect HEAD to be a symlink. git-reset still error out ... --- Use the correct buffer when validating 'ref: refs/...' Signed-off-by: Jonas Fonseca --- diff --git a/refs.c b/refs.c --- a/refs.c +++ b/refs.c @@ -46,7 +46,7 @@ int validate_symref(const char *path) len -= 4; while (len && isspace(*buf)) buf++, len--; - if (len >= 5 && !memcmp("refs/", buffer, 5)) + if (len >= 5 && !memcmp("refs/", buf, 5)) return 0; return -1; } -- Jonas Fonseca ``` ## Christopher Faylor, 2005-10-05 15:54 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005155457.GA30303@trixie.casa.cgf.cx> URL: https://gitlist.dev/e/20051005155457.GA30303%40trixie.casa.cgf.cx In-Reply-To: <81b0412b0510050846l2258775co117bada2d2b5a1ad@mail.gmail.com> ``` On Wed, Oct 05, 2005 at 05:46:08PM +0200, Alex Riesen wrote: >On 10/5/05, Alex Riesen wrote: >> On 10/4/05, H. Peter Anvin wrote: >> > > I noticed that rename(2) in my copy of cygwin (1.5.18-1) does not remove the >> > > target and returns an error (probably EPERM, but I have reasons not to trust >> > > strerror on that thing). >> > > The repository was on FAT. >> > > Taking "rename(2)" from cygwin's libiberty solved this (they unlink if link(2) >> > > returns EEXIST). >> > > >> > > PS: Does broken rename(2) qualify a system "not worthy to support"? >> > >> > I just tried this with Cygwin 1.5.18-1 and didn't have any such >> > problems. I tried it on NTFS, FAT and Samba, using WinXP. >> >> It's on Win2k, there was multiple cygwin installations in path, the other one >> supposedly is 1.5.5 (it's from QNX Momentics installation). >> I had that old "cygwin1.dll" renamed into "cygwin1.dll-disabled" long >> ago, though... >> I can't reproduce this out of GIT context, and the error is not >> reproducable after >> I removed the other cygwin installation out of PATH. >> Anyway, sorry, I should have tried this before posting. > >Still does not work for me. I cannot isolate the problem out of git, >but at the moment the only way for me to make commit_index_file to work >is to put unlink(indexfile) before rename(cf->lockfile, indexfile). > >For everyone interested, I attach cygwin's strace output here. I'm sorry that I missed this thread. I'm usually pretty alert to the word "cygwin" showing up in a subject. I'll go back and read the archives to catch up but, at the risk of making an observation that has already been made, under windows you can't always rename a file that is open. Is that what's happening here? -- Christopher Faylor spammer? -> aaaspam@sourceware.org Cygwin Co-Project Leader aaaspam@duffek.com TimeSys, Inc. ``` ## Davide Libenzi, 2005-10-05 16:09 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510050902430.28854%40localhost.localdomain In-Reply-To: <20051005155457.GA30303@trixie.casa.cgf.cx> ``` On 10/5/05, Alex Riesen wrote: > Still does not work for me. I cannot isolate the problem out of git, > but at the moment the only way for me to make commit_index_file to work > is to put unlink(indexfile) before rename(cf->lockfile, indexfile). I don't know how Cygwin implemented rename(), but if they used MoveFile() they broke the POSIX rename() since MoveFile() fails if destination already exists. They should have used MoveFileEx(MOVEFILE_REPLACE_EXISTING) instead, to guarantee POSIX semantics. The symptoms you're experiencing make me think this might be the case (even if it is strange, since other Unix software would fail the same way). - Davide ``` ## Christopher Faylor, 2005-10-05 16:15 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005161546.GB30303@trixie.casa.cgf.cx> URL: https://gitlist.dev/e/20051005161546.GB30303%40trixie.casa.cgf.cx In-Reply-To: ``` On Wed, Oct 05, 2005 at 09:09:49AM -0700, Davide Libenzi wrote: >On 10/5/05, Alex Riesen wrote: >>Still does not work for me. I cannot isolate the problem out of git, >>but at the moment the only way for me to make commit_index_file to work >>is to put unlink(indexfile) before rename(cf->lockfile, indexfile). > >I don't know how Cygwin implemented rename(), but if they used >MoveFile() they broke the POSIX rename() since MoveFile() fails if >destination already exists. They should have used >MoveFileEx(MOVEFILE_REPLACE_EXISTING) instead, to guarantee POSIX >semantics. The symptoms you're experiencing make me think this might >be the case (even if it is strange, since other Unix software would >fail the same way). Cygwin's rename is much more than just a simple wrapper around MoveFile or MoveFileEx. It tries hard to guarantee POSIX semantics within the strictures imposed by Windows. -- Christopher Faylor spammer? -> aaaspam@sourceware.org Cygwin Co-Project Leader aaaspam@duffek.com TimeSys, Inc. ``` ## H. Peter Anvin, 2005-10-05 16:23 Subject: Re: First cut at git port to Cygwin Message-ID: <4343FE08.2080607@zytor.com> URL: https://gitlist.dev/e/4343FE08.2080607%40zytor.com In-Reply-To: <20051005161546.GB30303@trixie.casa.cgf.cx> ``` Christopher Faylor wrote: > Cygwin's rename is much more than just a simple wrapper around MoveFile > or MoveFileEx. It tries hard to guarantee POSIX semantics within the > strictures imposed by Windows. Certainly, but presumably different versions of Win32 have different limitations, no? -hpa ``` ## Christopher Faylor, 2005-10-05 16:28 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005162801.GC30303@trixie.casa.cgf.cx> URL: https://gitlist.dev/e/20051005162801.GC30303%40trixie.casa.cgf.cx In-Reply-To: <4343FE08.2080607@zytor.com> ``` On Wed, Oct 05, 2005 at 09:23:36AM -0700, H. Peter Anvin wrote: >Christopher Faylor wrote: >>Cygwin's rename is much more than just a simple wrapper around MoveFile >>or MoveFileEx. It tries hard to guarantee POSIX semantics within the >>strictures imposed by Windows. > >Certainly, but presumably different versions of Win32 have different >limitations, no? Yes, definitely. That's why the Cygwin rename code (and unlink code for that matter) is so headache-inducing. FWIW, I just looked at the source and, AFAICT, if rename() is setting EPERM on Windows NT/2000/XP, it should be the result of a failing MoveFileEx (old, new, MOVEFILE_REPLACE_EXISTING). -- Christopher Faylor spammer? -> aaaspam@sourceware.org Cygwin Co-Project Leader aaaspam@duffek.com TimeSys, Inc. ``` ## Junio C Hamano, 2005-10-05 16:54 Subject: Re: [PATCH] Fix symbolic ref validation Message-ID: <7vk6grykdo.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk6grykdo.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20051005155212.GA16391@diku.dk> ``` Jonas Fonseca writes: > Yes, used rsync when I cloned. Seems validate_symref() was buggy. > >> The other errors could also stem from the fact that quite a few places >> expect HEAD to be a symlink. > > git-reset still error out ... > > --- > > Use the correct buffer when validating 'ref: refs/...' > > Signed-off-by: Jonas Fonseca > > --- > diff --git a/refs.c b/refs.c Thanks. One request, not just to Jonas. Please do not use '^---$' to separate the introductory discussion and the real commit log message. Linus style (recently the kernel list had a thread on this as well) is to have the commit log upfront with signoff, three-dash line, optional discussion and diffstat, and then diff. I do not mind seeing discussion upfront personally [*1*], but the thing is the tool treats everything after the first '^---$' something to be fed to patch, and does not treat it as the commit log message. [Footnote] *1* ...but remember, Linus does. ``` ## Davide Libenzi, 2005-10-05 17:29 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510051028570.29565%40localhost.localdomain In-Reply-To: <20051005161546.GB30303@trixie.casa.cgf.cx> ``` On Wed, 5 Oct 2005, Christopher Faylor wrote: > On Wed, Oct 05, 2005 at 09:09:49AM -0700, Davide Libenzi wrote: >> On 10/5/05, Alex Riesen wrote: >>> Still does not work for me. I cannot isolate the problem out of git, >>> but at the moment the only way for me to make commit_index_file to work >>> is to put unlink(indexfile) before rename(cf->lockfile, indexfile). >> >> I don't know how Cygwin implemented rename(), but if they used >> MoveFile() they broke the POSIX rename() since MoveFile() fails if >> destination already exists. They should have used >> MoveFileEx(MOVEFILE_REPLACE_EXISTING) instead, to guarantee POSIX >> semantics. The symptoms you're experiencing make me think this might >> be the case (even if it is strange, since other Unix software would >> fail the same way). > > Cygwin's rename is much more than just a simple wrapper around MoveFile > or MoveFileEx. It tries hard to guarantee POSIX semantics within the > strictures imposed by Windows. Ouch, IC: http://cygwin.com/cgi-bin/cvsweb.cgi/~checkout~/src/winsup/cygwin/syscalls.cc?rev=1.390&content-type=text/plain&cvsroot=src That's quite some code. - Davide ``` ## Alex Riesen, 2005-10-05 19:17 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005191741.GA25493@steel.home> URL: https://gitlist.dev/e/20051005191741.GA25493%40steel.home In-Reply-To: <20051005155457.GA30303@trixie.casa.cgf.cx> ``` Christopher Faylor, Wed, Oct 05, 2005 17:54:57 +0200: > >Still does not work for me. I cannot isolate the problem out of git, > >but at the moment the only way for me to make commit_index_file to work > >is to put unlink(indexfile) before rename(cf->lockfile, indexfile). > > > >For everyone interested, I attach cygwin's strace output here. > > I'm sorry that I missed this thread. I'm usually pretty alert to the word > "cygwin" showing up in a subject. > > I'll go back and read the archives to catch up but, at the risk of > making an observation that has already been made, under windows you > can't always rename a file that is open. Is that what's happening here? > Don't think so, but will check in about 10 hrs. The code in question is in index.c, commit_index_file. ``` ## Christopher Faylor, 2005-10-05 20:29 Subject: Re: First cut at git port to Cygwin Message-ID: <20051005202947.GA6184@trixie.casa.cgf.cx> URL: https://gitlist.dev/e/20051005202947.GA6184%40trixie.casa.cgf.cx In-Reply-To: <20051005191741.GA25493@steel.home> ``` On Wed, Oct 05, 2005 at 09:17:41PM +0200, Alex Riesen wrote: >Christopher Faylor, Wed, Oct 05, 2005 17:54:57 +0200: >> >Still does not work for me. I cannot isolate the problem out of git, >> >but at the moment the only way for me to make commit_index_file to work >> >is to put unlink(indexfile) before rename(cf->lockfile, indexfile). >> > >> >For everyone interested, I attach cygwin's strace output here. >> >> I'm sorry that I missed this thread. I'm usually pretty alert to the word >> "cygwin" showing up in a subject. >> >> I'll go back and read the archives to catch up but, at the risk of >> making an observation that has already been made, under windows you >> can't always rename a file that is open. Is that what's happening here? >> > >Don't think so, but will check in about 10 hrs. The code in question >is in index.c, commit_index_file. Ok. Looks pretty simple. FWIW, I've just built git on windows and I don't see this behavior. For the most part, it "just works". I do see the gitk behavior but I'm tk illiterate too, unfortunately, so I can't help there. Cygwin's tcl/tk is a funny beast. It is primarily included (by me) to allow for the operation of the insight debugger and is more windows than POSIX. As was noted, it doesn't interact with Cygwin/X but just draws windows using the Windows API. So, tcl/tk are just barely supported in Cygwin currently. I'm actually a little surprised that it works as well as it does with gitk. -- Christopher Faylor spammer? -> aaaspam@sourceware.org Cygwin Co-Project Leader aaaspam@duffek.com TimeSys, Inc. ``` ## Alex Riesen, 2005-10-06 09:05 Subject: Re: First cut at git port to Cygwin Message-ID: <81b0412b0510060205v4cd510c9wb4b06a3ed9242c8@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510060205v4cd510c9wb4b06a3ed9242c8%40mail.gmail.com In-Reply-To: <20051005202947.GA6184@trixie.casa.cgf.cx> ``` On 10/5/05, Christopher Faylor wrote: > >Don't think so, but will check in about 10 hrs. The code in question > >is in index.c, commit_index_file. > > Ok. Looks pretty simple. FWIW, I've just built git on windows and I > don't see this behavior. For the most part, it "just works". Thanks for the hint. There are open files involved (both index.lock and index). I attach the patch which closes index.lock (this is not really needed, btw: rename works even without closing index.lock) and unmaps the index (a bit too intrusive). The patch fixes only update-index.c (the one I had problems with), there probably are other places were the situation is alike. I don't like the patch (and win32 at all; hence the offending comment), so use it only unless there is no other possibility to workaround. I specifically do not request its inclusion into official branch (even though Junio is cc'ed). ``` ## Alex Riesen, 2005-10-06 10:07 Subject: Re: First cut at git port to Cygwin Message-ID: <81b0412b0510060307q431b64edt4196553bce28346c@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510060307q431b64edt4196553bce28346c%40mail.gmail.com In-Reply-To: <81b0412b0510060205v4cd510c9wb4b06a3ed9242c8@mail.gmail.com> ``` On 10/6/05, Alex Riesen wrote: > (a bit too intrusive). The patch fixes only update-index.c (the one I had > problems with), there probably are other places were the situation is alike. of course there are "other places". Please, try the attached patch instead. For the record: the patch is supposed to help people with "Unable to write new cachefile" kind of errors. ``` ## Alex Riesen, 2005-10-07 12:44 Subject: Re: First cut at git port to Cygwin Message-ID: <81b0412b0510070544v3e7cf0b4n521db8ff7e4e335a@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0510070544v3e7cf0b4n521db8ff7e4e335a%40mail.gmail.com In-Reply-To: <81b0412b0510060307q431b64edt4196553bce28346c@mail.gmail.com> ``` On 10/6/05, Alex Riesen wrote: > > (a bit too intrusive). The patch fixes only update-index.c (the one I had > > problems with), there probably are other places were the situation is alike. > > of course there are "other places". Please, try the attached patch instead. > > For the record: the patch is supposed to help people with > "Unable to write new cachefile" kind of errors. Just as I thought the situation improved (by closing index.lock and unmapping index), it suddenly get worse: now I'm stuck on git-pull. git-merge-index (called at some point by git-pull) maps the index in, and starts git-merge-one-file for each (or the given) entry in the index. git-merge-one-file calls git-update-index, which wants to update the index. Which doesn't work, because it's locked by that piece of s$%^. The only working walkaround for me atm is unlinking indexfile before rename in commit_index_file :( ``` ## Linus Torvalds, 2005-10-07 15:34 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0510070828270.31407%40g5.osdl.org In-Reply-To: <81b0412b0510070544v3e7cf0b4n521db8ff7e4e335a@mail.gmail.com> ``` On Fri, 7 Oct 2005, Alex Riesen wrote: > > it suddenly get worse: now I'm stuck on git-pull. > > git-merge-index (called at some point by git-pull) maps the index in, and starts > git-merge-one-file for each (or the given) entry in the index. > git-merge-one-file > calls git-update-index, which wants to update the index. Which doesn't work, > because it's locked by that piece of s$%^. NOTE! git doesn't use mmap() because it _needs_ to use mmap(), but because it was simple to do that way, and it's a total idiosyncracy of mine that I often try to mmap the data. I often also tend to do my own allocators instead of using malloc() (see my "sparse" project in case you're interested in other idiosyncracies of mine - macros to do list traversal etc). The fact is, "mmap()" isn't really any better than "read()": it has some advantages wrt memory management for the kernel, which is probably one big reason why I do it, but quite frankly, if you were to change every single mmap() to be a "map_file()" instead, and made it optional whether it used mmap() or "malloc + read()", I personally don't think it would be horrible. And it might make things much simpler for portability. The "use mmap" approach is very much a unixism, particularly the way unix people do it (mmap followed by close, making the file descriptor "go away"). Sure, other OS's have mmap too, but I think on them it tends to be less commonly used. Linus ``` ## Alex Riesen, 2005-10-07 20:54 Subject: Re: First cut at git port to Cygwin Message-ID: <20051007205450.GA14827@steel.home> URL: https://gitlist.dev/e/20051007205450.GA14827%40steel.home In-Reply-To: ``` Linus Torvalds, Fri, Oct 07, 2005 17:34:19 +0200: > > it suddenly get worse: now I'm stuck on git-pull. > > > > git-merge-index (called at some point by git-pull) maps the index > > in, and starts git-merge-one-file for each (or the given) entry in > > the index. git-merge-one-file calls git-update-index, which wants > > to update the index. Which doesn't work, because it's locked by > > that piece of s$%^. > > NOTE! git doesn't use mmap() because it _needs_ to use mmap(), but because > it was simple to do that way, and it's a total idiosyncracy of mine that I > often try to mmap the data. I often also tend to do my own allocators > instead of using malloc() (see my "sparse" project in case you're > interested in other idiosyncracies of mine - macros to do list traversal > etc). > > The fact is, "mmap()" isn't really any better than "read()": it has some > advantages wrt memory management for the kernel, which is probably one big > reason why I do it, but quite frankly, if you were to change every single > mmap() to be a "map_file()" instead, and made it optional whether it used > mmap() or "malloc + read()", I personally don't think it would be > horrible. > > And it might make things much simpler for portability. The "use mmap" > approach is very much a unixism, particularly the way unix people do it > (mmap followed by close, making the file descriptor "go away"). Sure, > other OS's have mmap too, but I think on them it tends to be less commonly > used. "Sounds like a thinly veiled threat or a very effective prodding" 8) --- Make read_cache copy the index into memory, to improve portability on other OS's which have mmap too, tend to use it less commonly. Signed-off-by: Alex Riesen diff --git a/read-cache.c b/read-cache.c --- a/read-cache.c +++ b/read-cache.c @@ -497,9 +497,11 @@ int read_cache(void) offset = sizeof(*hdr); for (i = 0; i < active_nr; i++) { struct cache_entry *ce = map + offset; - offset = offset + ce_size(ce); - active_cache[i] = ce; + size_t size = ce_size(ce); + offset = offset + size; + active_cache[i] = malloc(ce, size); } + munmap(map, size); return active_nr; unmap: ``` ## Alex Riesen, 2005-10-07 21:22 Subject: Re: First cut at git port to Cygwin Message-ID: <20051007212250.GA1423@steel.home> URL: https://gitlist.dev/e/20051007212250.GA1423%40steel.home In-Reply-To: <20051007205450.GA14827@steel.home> ``` Alex Riesen, Fri, Oct 07, 2005 22:54:50 +0200: > Linus Torvalds, Fri, Oct 07, 2005 17:34:19 +0200: > > > it suddenly get worse: now I'm stuck on git-pull. > > > > > > git-merge-index (called at some point by git-pull) maps the index > > > in, and starts git-merge-one-file for each (or the given) entry in > > > the index. git-merge-one-file calls git-update-index, which wants > > > to update the index. Which doesn't work, because it's locked by > > > that piece of s$%^. > > > > NOTE! git doesn't use mmap() because it _needs_ to use mmap(), but because > > it was simple to do that way, and it's a total idiosyncracy of mine that I > > often try to mmap the data. I often also tend to do my own allocators > > instead of using malloc() (see my "sparse" project in case you're > > interested in other idiosyncracies of mine - macros to do list traversal > > etc). > > > > The fact is, "mmap()" isn't really any better than "read()": it has some > > advantages wrt memory management for the kernel, which is probably one big > > reason why I do it, but quite frankly, if you were to change every single > > mmap() to be a "map_file()" instead, and made it optional whether it used > > mmap() or "malloc + read()", I personally don't think it would be > > horrible. > > > > And it might make things much simpler for portability. The "use mmap" > > approach is very much a unixism, particularly the way unix people do it > > (mmap followed by close, making the file descriptor "go away"). Sure, > > other OS's have mmap too, but I think on them it tends to be less commonly > > used. > > "Sounds like a thinly veiled threat or a very effective prodding" 8) > Junio C Hamano, Fri, Oct 07, 2005 23:00:02 +0200: > Huh? where is your memcpy? Unbelievable... I actually tested the change! But not _the_ patch. Thanks. Next time, hit me :) --- Make read_cache copy the index into memory, to improve portability on other OS's which have mmap too, tend to use it less commonly. Signed-off-by: Alex Riesen diff --git a/read-cache.c b/read-cache.c --- a/read-cache.c +++ b/read-cache.c @@ -497,9 +497,12 @@ int read_cache(void) offset = sizeof(*hdr); for (i = 0; i < active_nr; i++) { struct cache_entry *ce = map + offset; - offset = offset + ce_size(ce); - active_cache[i] = ce; + size_t size = ce_size(ce); + struct cache_entry *newce = malloc(size); + offset = offset + size; + active_cache[i] = memcpy(newce, ce, size); } + munmap(map, size); return active_nr; unmap: ``` ## Chuck Lever, 2005-10-07 21:29 Subject: Re: First cut at git port to Cygwin Message-ID: <4346E8AC.5030503@citi.umich.edu> URL: https://gitlist.dev/e/4346E8AC.5030503%40citi.umich.edu In-Reply-To: <20051007212250.GA1423@steel.home> ``` Alex Riesen wrote: > Make read_cache copy the index into memory, to improve portability on > other OS's which have mmap too, tend to use it less commonly. > > Signed-off-by: Alex Riesen > > diff --git a/read-cache.c b/read-cache.c > --- a/read-cache.c > +++ b/read-cache.c > @@ -497,9 +497,12 @@ int read_cache(void) > offset = sizeof(*hdr); > for (i = 0; i < active_nr; i++) { > struct cache_entry *ce = map + offset; > - offset = offset + ce_size(ce); > - active_cache[i] = ce; > + size_t size = ce_size(ce); > + struct cache_entry *newce = malloc(size); > + offset = offset + size; > + active_cache[i] = memcpy(newce, ce, size); > } > + munmap(map, size); > return active_nr; > > unmap: s/malloc/xmalloc/ begin:vcard fn:Chuck Lever n:Lever;Charles org:Network Appliance, Incorporated;Linux NFS Client Development adr:535 West William Street, Suite 3100;;Center for Information Technology Integration;Ann Arbor;MI;48103-4943;USA email;internet:cel@citi.umich.edu title:Member of Technical Staff tel;work:+1 734 763 4415 tel;fax:+1 734 763 4434 tel;home:+1 734 668 1089 x-mozilla-html:FALSE url:http://www.monkey.org/~cel/ version:2.1 end:vcard ``` ## Alex Riesen, 2005-10-07 21:39 Subject: Re: First cut at git port to Cygwin Message-ID: <20051007213952.GA8821@steel.home> URL: https://gitlist.dev/e/20051007213952.GA8821%40steel.home In-Reply-To: <4346E8AC.5030503@citi.umich.edu> ``` Chuck Lever, Fri, Oct 07, 2005 23:29:16 +0200: > s/malloc/xmalloc/ It's not that funny after second repost... --- Make read_cache copy the index into memory, to improve portability on other OS's which have mmap too, tend to use it less commonly. Signed-off-by: Alex Riesen diff --git a/read-cache.c b/read-cache.c --- a/read-cache.c +++ b/read-cache.c @@ -497,9 +497,12 @@ int read_cache(void) offset = sizeof(*hdr); for (i = 0; i < active_nr; i++) { struct cache_entry *ce = map + offset; - offset = offset + ce_size(ce); - active_cache[i] = ce; + size_t size = ce_size(ce); + struct cache_entry *newce = xmalloc(size); + offset = offset + size; + active_cache[i] = memcpy(newce, ce, size); } + munmap(map, size); return active_nr; unmap: ``` ## Alex Riesen, 2005-10-07 23:45 Subject: Re: First cut at git port to Cygwin Message-ID: <20051007234547.GC8893@steel.home> URL: https://gitlist.dev/e/20051007234547.GC8893%40steel.home In-Reply-To: <7vfyrdyre5.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano, Fri, Oct 07, 2005 23:00:02 +0200: > > "Sounds like a thinly veiled threat or a very effective prodding" 8) > > --- > > > > Make read_cache copy the index into memory, to improve portability on > > other OS's which have mmap too, tend to use it less commonly. > > > > Huh? where is your memcpy? > Junio, unless there already are pressing reasons to put the patch in GIT, could you postpone its inclusion (if you ever considered)? Or at least put "#ifdef __cygwin" (I hope this is the define) around it? It just so ugly... And besides, GIT reportedly works without problems for many people even without it. Anyway, the patch is out, so anyone with the problems can just patch their copy to workaround this specific win2k problem. Thanks, Alex ``` ## Elfyn McBratney, 2005-10-08 01:00 Subject: Re: First cut at git port to Cygwin Message-ID: <20051008010021.GA29261@gentoo.org> URL: https://gitlist.dev/e/20051008010021.GA29261%40gentoo.org In-Reply-To: <20051007234547.GC8893@steel.home> ``` On Sat, Oct 08, 2005 at 01:45:47AM +0200, Alex Riesen wrote: > Junio C Hamano, Fri, Oct 07, 2005 23:00:02 +0200: > > > "Sounds like a thinly veiled threat or a very effective prodding" 8) > > > --- > > > > > > Make read_cache copy the index into memory, to improve portability on > > > other OS's which have mmap too, tend to use it less commonly. > > > > > > > Huh? where is your memcpy? > > > > Junio, unless there already are pressing reasons to put the patch in > GIT, could you postpone its inclusion (if you ever considered)? Or at > least put "#ifdef __cygwin" (I hope this is the define) around it? Close ;) - the define is "__CYGWIN__". Best, Elfyn -- Elfyn McBratney Gentoo Developer/Perl Team Lead beu/irc.freenode.net http://dev.gentoo.org/~beu/ +------------O.o--------------------- http://dev.gentoo.org/~beu/pubkey.asc PGP Key ID: 0x69DF17AD PGP Key Fingerprint: DBD3 B756 ED58 B1B4 47B9 B3BD 8D41 E597 69DF 17AD ``` ## Linus Torvalds, 2005-10-08 16:11 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0510080900510.31407%40g5.osdl.org In-Reply-To: <20051007213952.GA8821@steel.home> ``` On Fri, 7 Oct 2005, Alex Riesen wrote: > > Make read_cache copy the index into memory, to improve portability on > other OS's which have mmap too, tend to use it less commonly. I really think that you should just get rid of the mmap. As it is, you're just slowing the code down on sane architectures. That's not good. So I'd suggest something like this instead. Totally untested, of course. Linus ---- diff --git a/read-cache.c b/read-cache.c --- a/read-cache.c +++ b/read-cache.c @@ -454,13 +454,39 @@ static int verify_hdr(struct cache_heade return 0; } +static void *map_index_file(int fd, size_t size) +{ + void *map; +#ifdef NO_MMAP + map = malloc(size); + if (!map) + die("Unable to allocate index file mapping"); + if (read(fd, map, size) != size) + die("Unable to read %z bytes from inde +#else + map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); + if (map == MAP_FAILED) + die("index file mmap failed (%s)", strerror(errno)); +#endif + return map; +} + +static void unmap_index_file(void *map, size_t size) +{ +#ifdef NO_MMAP + free(map); +#else + munmap(map, size); +#endif +} + int read_cache(void) { int fd, i; struct stat st; unsigned long size, offset; - void *map; struct cache_header *hdr; + void *map; errno = EBUSY; if (active_cache) @@ -475,16 +501,15 @@ int read_cache(void) } size = 0; // avoid gcc warning - map = MAP_FAILED; - if (!fstat(fd, &st)) { - size = st.st_size; - errno = EINVAL; - if (size >= sizeof(struct cache_header) + 20) - map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); - } + if (fstat(fd, &st)) + die("unable to fstat index file"); + + size = st.st_size; + errno = EINVAL; + if (size < sizeof(struct cache_header) + 20) + goto corrupt; + map = map_index_file(fd, size); close(fd); - if (map == MAP_FAILED) - die("index file mmap failed (%s)", strerror(errno)); hdr = map; if (verify_hdr(hdr, size) < 0) @@ -503,8 +528,9 @@ int read_cache(void) return active_nr; unmap: - munmap(map, size); + unmap_index_file(map, size); errno = EINVAL; +corrupt: die("index file corrupt"); } ``` ## Elfyn McBratney, 2005-10-08 17:38 Subject: Re: First cut at git port to Cygwin Message-ID: <20051008173812.GA20870@gentoo.org> URL: https://gitlist.dev/e/20051008173812.GA20870%40gentoo.org In-Reply-To: ``` On Sat, Oct 08, 2005 at 09:11:03AM -0700, Linus Torvalds wrote: > > On Fri, 7 Oct 2005, Alex Riesen wrote: > > > > Make read_cache copy the index into memory, to improve portability on > > other OS's which have mmap too, tend to use it less commonly. > > I really think that you should just get rid of the mmap. > > As it is, you're just slowing the code down on sane architectures. That's > not good. > > So I'd suggest something like this instead. > > Totally untested, of course. > > Linus Slightly adjusted diff below so it compiles ;) (Note: only the second die() un hunk #1 was changed.) Best, Elfyn ---- diff --git a/read-cache.c b/read-cache.c --- a/read-cache.c +++ b/read-cache.c @@ -454,13 +454,39 @@ static int verify_hdr(struct cache_heade return 0; } +static void *map_index_file(int fd, size_t size) +{ + void *map; +#ifdef NO_MMAP + map = malloc(size); + if (!map) + die("Unable to allocate index file mapping"); + if (read(fd, map, size) != size) + die("Unable to read %z bytes from index", size); +#else + map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); + if (map == MAP_FAILED) + die("index file mmap failed (%s)", strerror(errno)); +#endif + return map; +} + +static void unmap_index_file(void *map, size_t size) +{ +#ifdef NO_MMAP + free(map); +#else + munmap(map, size); +#endif +} + int read_cache(void) { int fd, i; struct stat st; unsigned long size, offset; - void *map; struct cache_header *hdr; + void *map; errno = EBUSY; if (active_cache) @@ -475,16 +501,15 @@ int read_cache(void) } size = 0; // avoid gcc warning - map = MAP_FAILED; - if (!fstat(fd, &st)) { - size = st.st_size; - errno = EINVAL; - if (size >= sizeof(struct cache_header) + 20) - map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); - } + if (fstat(fd, &st)) + die("unable to fstat index file"); + + size = st.st_size; + errno = EINVAL; + if (size < sizeof(struct cache_header) + 20) + goto corrupt; + map = map_index_file(fd, size); close(fd); - if (map == MAP_FAILED) - die("index file mmap failed (%s)", strerror(errno)); hdr = map; if (verify_hdr(hdr, size) < 0) @@ -503,8 +528,9 @@ int read_cache(void) return active_nr; unmap: - munmap(map, size); + unmap_index_file(map, size); errno = EINVAL; +corrupt: die("index file corrupt"); } -- Elfyn McBratney Gentoo Developer/Perl Team Lead beu/irc.freenode.net http://dev.gentoo.org/~beu/ +------------O.o--------------------- http://dev.gentoo.org/~beu/pubkey.asc PGP Key ID: 0x69DF17AD PGP Key Fingerprint: DBD3 B756 ED58 B1B4 47B9 B3BD 8D41 E597 69DF 17AD ``` ## Elfyn McBratney, 2005-10-08 17:43 Subject: Re: First cut at git port to Cygwin Message-ID: <20051008174306.GB20870@gentoo.org> URL: https://gitlist.dev/e/20051008174306.GB20870%40gentoo.org In-Reply-To: ``` Er, apologies for the dups - postfix crapped itself :/ *goes and stands in the corner donning the 'D' hat* -- Elfyn McBratney Gentoo Developer/Perl Team Lead beu/irc.freenode.net http://dev.gentoo.org/~beu/ +------------O.o--------------------- http://dev.gentoo.org/~beu/pubkey.asc PGP Key ID: 0x69DF17AD PGP Key Fingerprint: DBD3 B756 ED58 B1B4 47B9 B3BD 8D41 E597 69DF 17AD ``` ## Johannes Schindelin, 2005-10-08 18:27 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510082023130.25971%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` Hi, On Sat, 8 Oct 2005, Linus Torvalds wrote: > I really think that you should just get rid of the mmap. > > As it is, you're just slowing the code down on sane architectures. That's > not good. > > So I'd suggest something like this instead. > > Totally untested, of course. Am I missing something? I don't see where the changes are written back to the fd. After all, mmap() is called with PROT_WRITE... *shameless plug* Of course, this problem does not come up with my NO_MMAP patch. Ciao, Dscho ``` ## Junio C Hamano, 2005-10-08 18:44 Subject: Re: First cut at git port to Cygwin Message-ID: <7vr7avrgr2.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr7avrgr2.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: > Am I missing something? I don't see where the changes are written back to > the fd. After all, mmap() is called with PROT_WRITE... PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall correctly we do not write file via mmap -- at least we do not intend to. - index file is mapped for reading, long ago it was mapped read-only but these days we do PROT_WRITE, but updates are done via opening a new file and writing afresh. - objects are mapped for reading, but, never updated once created. Creation side is regular open - write - close. - diff reads original by mapping, but obviously has no business writing. - local-fetch reads original by mapping for copying. > *shameless plug* Of course, this problem does not come up with > my NO_MMAP patch. Yes. It might have been overkill that you supported writing changes back, though. . ``` ## Alex Riesen, 2005-10-08 18:49 Subject: Re: First cut at git port to Cygwin Message-ID: <20051008184925.GA6347@steel.home> URL: https://gitlist.dev/e/20051008184925.GA6347%40steel.home In-Reply-To: ``` Johannes Schindelin, Sat, Oct 08, 2005 20:27:06 +0200: > > I really think that you should just get rid of the mmap. > > > > As it is, you're just slowing the code down on sane architectures. That's > > not good. > > > > So I'd suggest something like this instead. > > > > Totally untested, of course. > > Am I missing something? I don't see where the changes are written back to > the fd. After all, mmap() is called with PROT_WRITE... It's just becase the file is open for reading only. Also, it is not an mmap/unmap implementation. Just reading cache in. ``` ## Johannes Schindelin, 2005-10-08 19:04 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510082100020.26626%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vr7avrgr2.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Sat, 8 Oct 2005, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Am I missing something? I don't see where the changes are written back to > > the fd. After all, mmap() is called with PROT_WRITE... > > PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall > correctly we do not write file via mmap -- at least we do not > intend to. Ahh! Reading the man page helps! > Yes. It might have been overkill that you supported writing > changes back, though. Sure. Something like this? diff --git a/compat/mmap.c b/compat/mmap.c index fca6321..fda39fc 100644 --- a/compat/mmap.c +++ b/compat/mmap.c @@ -49,7 +49,7 @@ void *gitfakemmap(void *start, size_t le n += count; } - if(prot & PROT_WRITE) { + if((prot & PROT_WRITE) && !(flags & MAP_PRIVATE)) { fakemmapwritable *next = xmalloc(sizeof(fakemmapwritable)); next->start = start; next->length = length; ``` ## Junio C Hamano, 2005-10-08 21:10 Subject: Re: First cut at git port to Cygwin Message-ID: <7vk6gnpvf9.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk6gnpvf9.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: >> PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall >> correctly we do not write file via mmap -- at least we do not >> intend to. > > Ahh! Reading the man page helps! > >> Yes. It might have been overkill that you supported writing >> changes back, though. > > Sure. Something like this? Not, really. What I meant was to rip out the writing out altogether, and perhaps making sure that the caller never calls us without MAP_PRIVATE. ``` ## Johannes Schindelin, 2005-10-08 22:06 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510090004370.30110%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vk6gnpvf9.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Sat, 8 Oct 2005, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Sure. Something like this? > > Not, really. What I meant was to rip out the writing out > altogether, and perhaps making sure that the caller never calls > us without MAP_PRIVATE. How about this, then? [PATCH] If NO_MMAP is defined, fake mmap() and munmap() Since some platforms do not support mmap() at all, and others do only just so, this patch introduces the option to fake mmap() and munmap() by malloc()ing the region explicitely. Signed-off-by: Johannes Schindelin --- Makefile | 6 ++++++ cache.h | 16 ++++++++++++++++ compat/mmap.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ mailsplit.c | 1 - 4 files changed, 73 insertions(+), 1 deletions(-) create mode 100644 compat/mmap.c applies-to: 274542bcbc891cca353c2728ac4075df3d1d2c0d ed334e3e2276fe9d41ed78917544ef6a3fa87eb7 diff --git a/Makefile b/Makefile index 1bdf4de..7ca77cf 100644 --- a/Makefile +++ b/Makefile @@ -27,6 +27,8 @@ # Define NEEDS_SOCKET if linking with libc is not enough (SunOS, # Patrick Mauritz). # +# Define NO_MMAP if you want to avoid mmap. +# # Define WITH_OWN_SUBPROCESS_PY if you want to use with python 2.3. # # Define NO_IPV6 if you lack IPv6 support and getaddrinfo(). @@ -258,6 +260,10 @@ ifdef NO_STRCASESTR DEFINES += -Dstrcasestr=gitstrcasestr LIB_OBJS += compat/strcasestr.o endif +ifdef NO_MMAP + DEFINES += -Dmmap=gitfakemmap -Dmunmap=gitfakemunmap -DNO_MMAP + LIB_OBJS += compat/mmap.o +endif ifdef NO_IPV6 DEFINES += -DNO_IPV6 -Dsockaddr_storage=sockaddr_in endif diff --git a/cache.h b/cache.h index 514adb8..5987d4c 100644 --- a/cache.h +++ b/cache.h @@ -11,7 +11,9 @@ #include #include #include +#ifndef NO_MMAP #include +#endif #include #include #include @@ -356,4 +358,18 @@ extern void packed_object_info_detail(st /* Dumb servers support */ extern int update_server_info(int); +#ifdef NO_MMAP + +#ifndef PROT_READ +#define PROT_READ 1 +#define PROT_WRITE 2 +#define MAP_PRIVATE 1 +#define MAP_FAILED ((void*)-1) +#endif + +extern void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset); +extern int gitfakemunmap(void *start, size_t length); + +#endif + #endif /* CACHE_H */ diff --git a/compat/mmap.c b/compat/mmap.c new file mode 100644 index 0000000..3f035a0 --- /dev/null +++ b/compat/mmap.c @@ -0,0 +1,51 @@ +#include +#include +#include +#include +#include "../cache.h" + +void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset) +{ + int n = 0; + + if(start != NULL || !(flags & MAP_PRIVATE)) + die("Invalid usage of gitfakemmap."); + + if(lseek(fd, offset, SEEK_SET)<0) { + errno = EINVAL; + return MAP_FAILED; + } + + start = xmalloc(length); + if(start == NULL) { + errno = ENOMEM; + return MAP_FAILED; + } + + while(n < length) { + int count = read(fd, start+n, length-n); + + if(count == 0) { + memset(start+n, 0, length-n); + break; + } + + if(count < 0) { + free(start); + errno = EACCES; + return MAP_FAILED; + } + + n += count; + } + + return start; +} + +int gitfakemunmap(void *start, size_t length) +{ + free(start); + + return 0; +} + diff --git a/mailsplit.c b/mailsplit.c index 7981f87..0f8100d 100644 --- a/mailsplit.c +++ b/mailsplit.c @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include --- 0.99.8.GIT ``` ## Matthias Urlichs, 2005-10-09 20:40 Subject: Commit text BEFORE the dashes (Re: First cut at git port to Cygwin) Message-ID: URL: https://gitlist.dev/e/pan.2005.10.09.20.39.59.765531%40smurf.noris.de In-Reply-To: <20051007212250.GA1423@steel.home> ``` Hi, Alex Riesen wrote: > [ some text ] > --- > [ the actual commit text ] REMINDER: These need to be swapped. -- Matthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de ``` ## H. Peter Anvin, 2005-10-10 18:43 Subject: Re: First cut at git port to Cygwin Message-ID: <434AB663.8050205@zytor.com> URL: https://gitlist.dev/e/434AB663.8050205%40zytor.com In-Reply-To: <7vr7avrgr2.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > > PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall > correctly we do not write file via mmap -- at least we do not > intend to. > Then PROT_READ probably makes more sense? > > Yes. It might have been overkill that you supported writing > changes back, though. Not just overkill; if we do MAP_PRIVATE it's actively WRONG. -hpa ``` ## H. Peter Anvin, 2005-10-10 18:45 Subject: Re: First cut at git port to Cygwin Message-ID: <434AB6AD.9090107@zytor.com> URL: https://gitlist.dev/e/434AB6AD.9090107%40zytor.com In-Reply-To: <20051008010021.GA29261@gentoo.org> ``` Elfyn McBratney wrote: > > > > Junio, unless there already are pressing reasons to put the patch in > > GIT, could you postpone its inclusion (if you ever considered)? Or at > > least put "#ifdef __cygwin" (I hope this is the define) around it? > > Close ;) - the define is "__CYGWIN__". > This should be a feature-control macro in the Makefile. ``` ## Johannes Schindelin, 2005-10-10 19:01 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510102100010.7688%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <434AB663.8050205@zytor.com> ``` Hi, On Mon, 10 Oct 2005, H. Peter Anvin wrote: > Junio C Hamano wrote: > > > > PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall > > correctly we do not write file via mmap -- at least we do not > > intend to. > > > > Then PROT_READ probably makes more sense? Not necessarily. Sometimes you need to annotate the data from the index, and this does not need to be written back to the index file. > > Yes. It might have been overkill that you supported writing > > changes back, though. > > Not just overkill; if we do MAP_PRIVATE it's actively WRONG. See above. BTW, is there a mechanism to make sure that the index file is locked between reading and writing? Ciao, Dscho ``` ## H. Peter Anvin, 2005-10-10 19:26 Subject: Re: First cut at git port to Cygwin Message-ID: <434AC058.60803@zytor.com> URL: https://gitlist.dev/e/434AC058.60803%40zytor.com In-Reply-To: ``` Johannes Schindelin wrote: > >>Junio C Hamano wrote: >> >>>PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall >>>correctly we do not write file via mmap -- at least we do not >>>intend to. >>> >> >>Then PROT_READ probably makes more sense? > > Not necessarily. Sometimes you need to annotate the data from the index, > and this does not need to be written back to the index file. > In the above sentence, emphasis on "at least we do not intend to." If writes are done legitimately then that's fine, but we shouldn't have "accidental writes" -- those would be program bugs! > >>>Yes. It might have been overkill that you supported writing >>>changes back, though. >> >>Not just overkill; if we do MAP_PRIVATE it's actively WRONG. > > See above. > Eh? If we MAP_PRIVATE, *and* we (intentionally) write to it, we *BETTER* not write anything back. -hpa ``` ## Johannes Schindelin, 2005-10-10 19:42 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510102139540.7861%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <434AC058.60803@zytor.com> ``` Hi, On Mon, 10 Oct 2005, H. Peter Anvin wrote: > Johannes Schindelin wrote: > > > > > Junio C Hamano wrote: > > > > > > > PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall > > > > correctly we do not write file via mmap -- at least we do not > > > > intend to. > > > > > > > > > > Then PROT_READ probably makes more sense? > > > > Not necessarily. Sometimes you need to annotate the data from the index, and > > this does not need to be written back to the index file. > > > > In the above sentence, emphasis on "at least we do not intend to." If writes > are done legitimately then that's fine, but we shouldn't have "accidental > writes" -- those would be program bugs! Yes, those would be bugs. However, if I understood the man page for mmap() correctly, then PROT_WRITE && MAP_PRIVATE makes the data copy-on-write, which means that those bugs would have been found (because the changes would no longer be present when git was called the next time). And I checked: all mmap() calls in git are MAP_PRIVATE. > > > > Yes. It might have been overkill that you supported writing > > > > changes back, though. > > > > > > Not just overkill; if we do MAP_PRIVATE it's actively WRONG. > > > > See above. > > > > Eh? If we MAP_PRIVATE, *and* we (intentionally) write to it, we *BETTER* not > write anything back. Yes. That was *my* mistake. Ciao, Dscho ``` ## Junio C Hamano, 2005-10-10 20:21 Subject: Re: First cut at git port to Cygwin Message-ID: <7vr7at3yyd.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr7at3yyd.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <434AC058.60803@zytor.com> ``` "H. Peter Anvin" writes: > Eh? If we MAP_PRIVATE, *and* we (intentionally) write to it, we > *BETTER* not write anything back. Correct. It has already been fixed by Johannes last week and I merged it over the weekend if not earlier if I recall correctly. ``` ## Daniel Barkalow, 2005-10-10 20:27 Subject: Re: First cut at git port to Cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0510101620290.23242%40iabervon.org In-Reply-To: ``` On Mon, 10 Oct 2005, Johannes Schindelin wrote: > Hi, > > On Mon, 10 Oct 2005, H. Peter Anvin wrote: > > > Junio C Hamano wrote: > > > > > > PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall > > > correctly we do not write file via mmap -- at least we do not > > > intend to. > > > > > > > Then PROT_READ probably makes more sense? > > Not necessarily. Sometimes you need to annotate the data from the index, > and this does not need to be written back to the index file. In fact, it is intentional that we open the file O_RDONLY, and mmap it PROT_READ | PROT_WRITE, MAP_PRIVATE. We prepare the next index in the memory where we mapped the old index, but we don't want to change what's on the disk using the mapping; we write that later to a different file using write(). > > > Yes. It might have been overkill that you supported writing > > > changes back, though. > > > > Not just overkill; if we do MAP_PRIVATE it's actively WRONG. > > See above. > > BTW, is there a mechanism to make sure that the index file is locked > between reading and writing? There's definitely locking; the new file is written to "(filename).lock", which is openned O_CREAT | O_EXCL, and is moved to the destination when it's complete. I believe everything that intends to write a new index gets the lock before reading the old index, although I haven't actually checked. -Daniel *This .sig left intentionally blank* ``` ## Junio C Hamano, 2005-10-10 20:34 Subject: Re: First cut at git port to Cygwin Message-ID: <7vhdbp3yd7.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vhdbp3yd7.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <434AC058.60803@zytor.com> ``` "H. Peter Anvin" writes: >>>Junio C Hamano wrote: >>> >>>>PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall >>>>correctly we do not write file via mmap -- at least we do not >>>>intend to. >>>> > > In the above sentence, emphasis on "at least we do not intend to." If > writes are done legitimately then that's fine, but we shouldn't have > "accidental writes" -- those would be program bugs! What I meant to say was "we do not intend to write back the changes by expecting the modification on mapped area are written back by mmap() mechanism -- the updates to index file is done by creat - write - close - rename". So your saying "the overkill being actively wrong" was technically correct, but that wrongly written data was renamed out anyway and no real harm was done. ``` ## H. Peter Anvin, 2005-10-10 20:52 Subject: Re: First cut at git port to Cygwin Message-ID: <434AD48B.8070305@zytor.com> URL: https://gitlist.dev/e/434AD48B.8070305%40zytor.com In-Reply-To: <7vhdbp3yd7.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > "H. Peter Anvin" writes: > > >>>>Junio C Hamano wrote: >>>> >>>> >>>>>PROT_WRITE is true, but we do MAP_PRIVATE, and if I recall >>>>>correctly we do not write file via mmap -- at least we do not >>>>>intend to. >>>>> >> >>In the above sentence, emphasis on "at least we do not intend to." If >>writes are done legitimately then that's fine, but we shouldn't have >>"accidental writes" -- those would be program bugs! > > > What I meant to say was "we do not intend to write back the > changes by expecting the modification on mapped area are written > back by mmap() mechanism -- the updates to index file is done by > creat - write - close - rename". So your saying "the overkill > being actively wrong" was technically correct, but that wrongly > written data was renamed out anyway and no real harm was done. Well, it broke the atomicity of an operation, which *is* a real problem. Anyway, malloc+read is a dead ringer for MAP_PRIVATE with PROT_WRITE, so that makes it even easier to mimic. -hpa ```