threads / discuss / 1969

First cut at git port to Cygwin

Subject: First cut at git port to Cygwin

## tl;dr

62 messages between Sep 29, 2005 and Oct 10, 2005.

replies: 61people: 14as markdown or json

H. Peter Anvin· Sep 29, 2005, 00:53 UTC · lore

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· Sep 29, 2005, 04:30 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

"H. Peter Anvin" <hpa@zytor.com> 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.

H. Peter Anvin· Sep 29, 2005, 05:07 UTC · re: Junio C Hamano · lore

Re: First cut at git port to Cygwin

Junio C Hamano wrote:
Show 13 quoted lines
> "H. Peter Anvin" <hpa@zytor.com> 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
Martin Langhoff· Sep 29, 2005, 04:46 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

On 9/29/05, H. Peter Anvin <hpa@zytor.com> 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
Junio C Hamano· Sep 29, 2005, 05:13 UTC · re: Martin Langhoff · lore

Re: First cut at git port to Cygwin

Martin Langhoff <martin.langhoff@gmail.com> 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· Sep 29, 2005, 06:19 UTC · re: Martin Langhoff · lore

Re: First cut at git port to Cygwin

Martin Langhoff wrote:
Show 8 quoted lines
> 
>>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· Sep 29, 2005, 08:46 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

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· Sep 29, 2005, 16:11 UTC · re: Johannes Schindelin · lore

Re: First cut at git port to Cygwin

Johannes Schindelin wrote:
Show 13 quoted lines
> 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· Sep 29, 2005, 17:25 UTC · re: Johannes Schindelin · lore

Re: First cut at git port to Cygwin

Johannes Schindelin wrote:
Show 13 quoted lines
> 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· Sep 30, 2005, 10:02 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

"H. Peter Anvin" <hpa@zytor.com> 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 <alloca.h>
Why?  I do not see any use of alloca in the added code...
        +#include <sys/poll.h>

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· Sep 30, 2005, 17:01 UTC · re: Junio C Hamano · lore

Re: First cut at git port to Cygwin

Junio C Hamano wrote:
> 
> Could you do update-server-info there, please?
> 
Done...
Show 9 quoted lines
> 
> 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.
Show 25 quoted lines
>          # 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.

Show 9 quoted lines
>         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 <alloca.h>
> 
> 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.

Show 7 quoted lines
>         +#include <sys/poll.h>
> 
> 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.
Show 7 quoted lines
>                 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!
Show 34 quoted lines
> 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
Alex Riesen· Oct 4, 2005, 12:31 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

On 9/29/05, H. Peter Anvin <hpa@zytor.com> wrote:
Show 8 quoted lines
> 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· Oct 4, 2005, 13:06 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

On 10/4/05, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 16 quoted lines
> On 9/29/05, H. Peter Anvin <hpa@zytor.com> 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· Oct 4, 2005, 14:06 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

Alex Riesen wrote:
Show 9 quoted lines
> 
> 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
Christopher Faylor· Oct 5, 2005, 03:15 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

On Tue, Oct 04, 2005 at 07:06:29AM -0700, H. Peter Anvin wrote:
Show 16 quoted lines
>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.

H. Peter Anvin· Oct 4, 2005, 15:03 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

Alex Riesen wrote:
Show 9 quoted lines
> 
> 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· Oct 5, 2005, 03:16 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

On Tue, Oct 04, 2005 at 08:03:55AM -0700, H. Peter Anvin wrote:
Show 16 quoted lines
>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.

Alex Riesen· Oct 5, 2005, 11:24 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

On 10/4/05, H. Peter Anvin <hpa@zytor.com> wrote:
Show 11 quoted lines
> > 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.

Alex Riesen· Oct 5, 2005, 15:46 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

On 10/5/05, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 21 quoted lines
> On 10/4/05, H. Peter Anvin <hpa@zytor.com> 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.
Christopher Faylor· Oct 5, 2005, 15:54 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

On Wed, Oct 05, 2005 at 05:46:08PM +0200, Alex Riesen wrote:
Show 28 quoted lines
>On 10/5/05, Alex Riesen <raa.lkml@gmail.com> wrote:
>> On 10/4/05, H. Peter Anvin <hpa@zytor.com> 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· Oct 5, 2005, 16:09 UTC · re: Christopher Faylor · lore

Re: First cut at git port to Cygwin

On 10/5/05, Alex Riesen <raa.lkml@gmail.com> 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· Oct 5, 2005, 16:15 UTC · re: Davide Libenzi · lore

Re: First cut at git port to Cygwin

On Wed, Oct 05, 2005 at 09:09:49AM -0700, Davide Libenzi wrote:
Show 12 quoted lines
>On 10/5/05, Alex Riesen <raa.lkml@gmail.com> 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· Oct 5, 2005, 16:23 UTC · re: Christopher Faylor · lore

Re: First cut at git port to Cygwin

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· Oct 5, 2005, 16:28 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

On Wed, Oct 05, 2005 at 09:23:36AM -0700, H. Peter Anvin wrote:
Show 7 quoted lines
>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.

Davide Libenzi· Oct 5, 2005, 17:29 UTC · re: Christopher Faylor · lore

Re: First cut at git port to Cygwin

On Wed, 5 Oct 2005, Christopher Faylor wrote:
Show 17 quoted lines
> On Wed, Oct 05, 2005 at 09:09:49AM -0700, Davide Libenzi wrote:
>> On 10/5/05, Alex Riesen <raa.lkml@gmail.com> 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· Oct 5, 2005, 19:17 UTC · re: Christopher Faylor · lore

Re: First cut at git port to Cygwin

Christopher Faylor, Wed, Oct 05, 2005 17:54:57 +0200:
Show 13 quoted lines
> >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· Oct 5, 2005, 20:29 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

On Wed, Oct 05, 2005 at 09:17:41PM +0200, Alex Riesen wrote:
Show 17 quoted lines
>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· Oct 6, 2005, 09:05 UTC · re: Christopher Faylor · lore

Re: First cut at git port to Cygwin

On 10/5/05, Christopher Faylor <me@cgf.cx> wrote:
Show 5 quoted lines
> >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· Oct 6, 2005, 10:07 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

On 10/6/05, Alex Riesen <raa.lkml@gmail.com> 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· Oct 7, 2005, 12:44 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

On 10/6/05, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 7 quoted lines
> > (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· Oct 7, 2005, 15:34 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

On Fri, 7 Oct 2005, Alex Riesen wrote:
Show 8 quoted lines
>
> 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· Oct 7, 2005, 20:54 UTC · re: Linus Torvalds · lore

Re: First cut at git port to Cygwin

Linus Torvalds, Fri, Oct 07, 2005 17:34:19 +0200:
Show 27 quoted lines
> > 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 <raa.lkml@gmail.com>
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· Oct 7, 2005, 21:22 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

Alex Riesen, Fri, Oct 07, 2005 22:54:50 +0200:
Show 31 quoted lines
> 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 <raa.lkml@gmail.com>
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· Oct 7, 2005, 21:29 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

Alex Riesen wrote:
Show 23 quoted lines
> 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 <raa.lkml@gmail.com>
> 
> 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· Oct 7, 2005, 21:39 UTC · re: Chuck Lever · lore

Re: First cut at git port to Cygwin

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 <raa.lkml@gmail.com>
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:
Linus Torvalds· Oct 8, 2005, 16:11 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

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· Oct 8, 2005, 17:38 UTC · re: Linus Torvalds · lore

Re: First cut at git port to Cygwin

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· Oct 8, 2005, 17:43 UTC · re: Linus Torvalds · lore

Re: First cut at git port to Cygwin

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· Oct 8, 2005, 18:27 UTC · re: Linus Torvalds · lore

Re: First cut at git port to Cygwin

Hi,
On Sat, 8 Oct 2005, Linus Torvalds wrote:
Show 8 quoted lines
> 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· Oct 8, 2005, 18:44 UTC · re: Johannes Schindelin · lore

Re: First cut at git port to Cygwin

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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. .

Johannes Schindelin· Oct 8, 2005, 19:04 UTC · re: Junio C Hamano · lore

Re: First cut at git port to Cygwin

Hi,
On Sat, 8 Oct 2005, Junio C Hamano wrote:
Show 8 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Oct 8, 2005, 21:10 UTC · re: Johannes Schindelin · lore

Re: First cut at git port to Cygwin

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
>> 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· Oct 8, 2005, 22:06 UTC · re: Junio C Hamano · lore

Re: First cut at git port to Cygwin

Hi,
On Sat, 8 Oct 2005, Junio C Hamano wrote:
Show 7 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 <Johannes.Schindelin@gmx.de>
---
 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 <string.h>
 #include <errno.h>
 #include <limits.h>
+#ifndef NO_MMAP
 #include <sys/mman.h>
+#endif
 #include <sys/param.h>
 #include <netinet/in.h>
 #include <sys/types.h>
@@ -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 <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#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 <fcntl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <sys/mman.h>
 #include <string.h>
 #include <stdio.h>
 #include <ctype.h>
---
0.99.8.GIT
H. Peter Anvin· Oct 10, 2005, 18:43 UTC · re: Junio C Hamano · lore

Re: First cut at git port to Cygwin

Junio C Hamano wrote:
Show 5 quoted lines
> 
> 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
Johannes Schindelin· Oct 10, 2005, 19:01 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

Hi,
On Mon, 10 Oct 2005, H. Peter Anvin wrote:
Show 8 quoted lines
> 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· Oct 10, 2005, 19:26 UTC · re: Johannes Schindelin · lore

Re: First cut at git port to Cygwin

Johannes Schindelin wrote:
Show 13 quoted lines
> 
>>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!

Show 8 quoted lines
> 
>>>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· Oct 10, 2005, 19:42 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

Hi,
On Mon, 10 Oct 2005, H. Peter Anvin wrote:
Show 18 quoted lines
> 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.

Show 10 quoted lines
> > > > 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· Oct 10, 2005, 20:21 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

"H. Peter Anvin" <hpa@zytor.com> 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.

Junio C Hamano· Oct 10, 2005, 20:34 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

"H. Peter Anvin" <hpa@zytor.com> writes:
Show 10 quoted lines
>>>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· Oct 10, 2005, 20:52 UTC · re: Junio C Hamano · lore

Re: First cut at git port to Cygwin

Junio C Hamano wrote:
Show 22 quoted lines
> "H. Peter Anvin" <hpa@zytor.com> 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
Daniel Barkalow· Oct 10, 2005, 20:27 UTC · re: Johannes Schindelin · lore

Re: First cut at git port to Cygwin

On Mon, 10 Oct 2005, Johannes Schindelin wrote:
Show 15 quoted lines
> 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().

Show 9 quoted lines
> > > 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*
Alex Riesen· Oct 8, 2005, 18:49 UTC · re: Johannes Schindelin · lore

Re: First cut at git port to Cygwin

Johannes Schindelin, Sat, Oct 08, 2005 20:27:06 +0200:
Show 11 quoted lines
> > 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.

Matthias Urlichs· Oct 9, 2005, 20:40 UTC · re: Alex Riesen · lore

Commit text BEFORE the dashes (Re: First cut at git port to Cygwin)

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
Jonas Fonseca· Oct 5, 2005, 13:16 UTC · re: H. Peter Anvin · lore

Re: First cut at git port to Cygwin

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· Oct 5, 2005, 13:58 UTC · re: Jonas Fonseca · lore

Re: First cut at git port to Cygwin

Hi,
On Wed, 5 Oct 2005, Jonas Fonseca wrote:
Show 7 quoted lines
>   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

Jonas Fonseca· Oct 5, 2005, 15:52 UTC · re: Johannes Schindelin · lore

[PATCH] Fix symbolic ref validation

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote Wed, Oct 05, 2005:
> Hi,
Hello,
Show 12 quoted lines
> 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 <fonseca@diku.dk>
---
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
Junio C Hamano· Oct 5, 2005, 16:54 UTC · re: Jonas Fonseca · lore

Re: [PATCH] Fix symbolic ref validation

Jonas Fonseca <fonseca@diku.dk> writes:
Show 15 quoted lines
> 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 <fonseca@diku.dk>
>
> ---
> 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.
Alex Riesen· Oct 7, 2005, 23:45 UTC · lore

Re: First cut at git port to Cygwin

Junio C Hamano, Fri, Oct 07, 2005 23:00:02 +0200:
Show 9 quoted lines
> > "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· Oct 8, 2005, 01:00 UTC · re: Alex Riesen · lore

Re: First cut at git port to Cygwin

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
H. Peter Anvin· Oct 10, 2005, 18:45 UTC · re: Elfyn McBratney · lore

Re: First cut at git port to Cygwin

Elfyn McBratney wrote:
Show 7 quoted lines
>  > 
>  > 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.

← back to recent threads