From: Junio C Hamano <junkio@cox.net>
Date: Fri, 30 Sep 2005 10:02:57 GMT
Subject: Re: First cut at git port to Cygwin
Message-ID: <7v4q826ffy.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <433B3B10.5050407@zytor.com>

"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]"