[PATCH 0/3] Interix support

[PATCH 0/3] Interix support

[Jonathan Callen]

This series of patches adds support for building git on Interix.

Interix is an interesting system, as it lacks some functions that are
normally present on POSIX systems, such as poll().  As I did not have
time to implement poll() on top of select(), these patches simply
disable the three commands that require poll() to be present:
git-daemon, git-upload-archive, and git-upload-pack.

Jonathan Callen (3):
      Support building on systems without poll(2)
      Support building without inttypes.h
      Add Interix support

 Makefile          |   40 +++++++++++++++++++++++++++++++++++-----
 builtin.h         |    2 ++
 git-compat-util.h |    6 ++++++
 git.c             |    2 ++
 4 files changed, 45 insertions(+), 5 deletions(-)

[PATCH 1/3] Support building on systems without poll(2)

[Jonathan Callen]

Some systems do not have sys/poll.h or poll(2).  Don't build
git-daemon, git-upload-archive, or git-upload-pack on such systems.

Signed-off-by: Jonathan Callen <abcd@gentoo.org>
---
 Makefile          |   21 ++++++++++++++++-----
 builtin.h         |    2 ++
 git-compat-util.h |    2 ++
 git.c             |    2 ++
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 07cab8f..4b36534 100644
--- a/Makefile
+++ b/Makefile
@@ -62,6 +62,8 @@ all::
 #
 # Define NO_MKSTEMPS if you don't have mkstemps in the C library.
 #
+# Define NO_POLL if you don't have poll in the C library, or it does not work.
+#
 # Define NO_LIBGEN_H if you don't have libgen.h.
 #
 # Define NEEDS_LIBGEN if your libgen needs -lgen when linking
@@ -386,7 +388,6 @@ PROGRAM_OBJS += fast-import.o
 PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += shell.o
 PROGRAM_OBJS += show-index.o
-PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += http-backend.o
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
@@ -434,9 +435,7 @@ OTHER_PROGRAMS = git$X
 
 # what test wrappers are needed and 'install' will install, in bindir
 BINDIR_PROGRAMS_NEED_X += git
-BINDIR_PROGRAMS_NEED_X += git-upload-pack
 BINDIR_PROGRAMS_NEED_X += git-receive-pack
-BINDIR_PROGRAMS_NEED_X += git-upload-archive
 BINDIR_PROGRAMS_NEED_X += git-shell
 
 BINDIR_PROGRAMS_NO_X += git-cvsserver
@@ -722,7 +721,6 @@ BUILTIN_OBJS += builtin/unpack-objects.o
 BUILTIN_OBJS += builtin/update-index.o
 BUILTIN_OBJS += builtin/update-ref.o
 BUILTIN_OBJS += builtin/update-server-info.o
-BUILTIN_OBJS += builtin/upload-archive.o
 BUILTIN_OBJS += builtin/var.o
 BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
@@ -1162,8 +1160,17 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz
 
+ifndef NO_POLL
+	BUILTIN_OBJS += builtin/upload-archive.o
+	PROGRAM_OBJS += upload-pack.o
+	BINDIR_PROGRAMS_NEED_X += git-upload-archive
+	BINDIR_PROGRAMS_NEED_X += git-upload-pack
+endif
+
 ifndef NO_POSIX_ONLY_PROGRAMS
-	PROGRAM_OBJS += daemon.o
+	ifndef NO_POLL
+		PROGRAM_OBJS += daemon.o
+	endif
 endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
@@ -1322,6 +1329,10 @@ ifdef OLD_ICONV
 	BASIC_CFLAGS += -DOLD_ICONV
 endif
 
+ifdef NO_POLL
+	BASIC_CFLAGS += -DNO_POLL
+endif
+
 ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif
diff --git a/builtin.h b/builtin.h
index 5c887ef..165a748 100644
--- a/builtin.h
+++ b/builtin.h
@@ -127,7 +127,9 @@ extern int cmd_unpack_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_update_index(int argc, const char **argv, const char *prefix);
 extern int cmd_update_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_update_server_info(int argc, const char **argv, const char *prefix);
+#ifdef NO_POLL
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
+#endif
 extern int cmd_upload_tar(int argc, const char **argv, const char *prefix);
 extern int cmd_var(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
diff --git a/git-compat-util.h b/git-compat-util.h
index edf352d..c5188e5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -94,7 +94,9 @@
 #include <utime.h>
 #ifndef __MINGW32__
 #include <sys/wait.h>
+#ifndef NO_POLL
 #include <sys/poll.h>
+#endif
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <termios.h>
diff --git a/git.c b/git.c
index 99f0363..8c081db 100644
--- a/git.c
+++ b/git.c
@@ -390,7 +390,9 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "update-index", cmd_update_index, RUN_SETUP },
 		{ "update-ref", cmd_update_ref, RUN_SETUP },
 		{ "update-server-info", cmd_update_server_info, RUN_SETUP },
+#ifndef NO_POLL
 		{ "upload-archive", cmd_upload_archive },
+#endif
 		{ "var", cmd_var },
 		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 		{ "version", cmd_version },
-- 
1.7.1

Re: [PATCH 1/3] Support building on systems without poll(2)

[Sverre Rabbelier]

Heya,

On Thu, May 27, 2010 at 10:19, Jonathan Callen <abcd@gentoo.org> wrote:
> Some systems do not have sys/poll.h or poll(2).  Don't build
> git-daemon, git-upload-archive, or git-upload-pack on such systems.

I thought git-upload-pack is required for push support in git?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 1/3] Support building on systems without poll(2)

[Jeff King]

On Thu, May 27, 2010 at 10:43:35AM +0200, Sverre Rabbelier wrote:

> On Thu, May 27, 2010 at 10:19, Jonathan Callen <abcd@gentoo.org> wrote:
> > Some systems do not have sys/poll.h or poll(2).  Don't build
> > git-daemon, git-upload-archive, or git-upload-pack on such systems.
> 
> I thought git-upload-pack is required for push support in git?

No, that's send-pack. Upload-pack is the server side of fetching. And
yes, these names are confusing. Before I worked a lot on the send-pack
code, I used to mix them up all the time. :)

So without upload-pack, one cannot be the server side of a fetch. Which
means you can still work as a client, but it does mean that even local
clones won't work (actually, the clone will work due to the local
hardlink optimization, but further fetches will fail).

-Peff

Re: [PATCH 1/3] Support building on systems without poll(2)

[Michael J Gruber]

Jonathan Callen venit, vidit, dixit 27.05.2010 10:19:
> Some systems do not have sys/poll.h or poll(2).  Don't build
> git-daemon, git-upload-archive, or git-upload-pack on such systems.
> 
> Signed-off-by: Jonathan Callen <abcd@gentoo.org>
> ---
>  Makefile          |   21 ++++++++++++++++-----
>  builtin.h         |    2 ++
>  git-compat-util.h |    2 ++
>  git.c             |    2 ++
>  4 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 07cab8f..4b36534 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -62,6 +62,8 @@ all::
>  #
>  # Define NO_MKSTEMPS if you don't have mkstemps in the C library.
>  #
> +# Define NO_POLL if you don't have poll in the C library, or it does not work.
> +#
>  # Define NO_LIBGEN_H if you don't have libgen.h.
>  #
>  # Define NEEDS_LIBGEN if your libgen needs -lgen when linking
> @@ -386,7 +388,6 @@ PROGRAM_OBJS += fast-import.o
>  PROGRAM_OBJS += imap-send.o
>  PROGRAM_OBJS += shell.o
>  PROGRAM_OBJS += show-index.o
> -PROGRAM_OBJS += upload-pack.o
>  PROGRAM_OBJS += http-backend.o
>  
>  PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
> @@ -434,9 +435,7 @@ OTHER_PROGRAMS = git$X
>  
>  # what test wrappers are needed and 'install' will install, in bindir
>  BINDIR_PROGRAMS_NEED_X += git
> -BINDIR_PROGRAMS_NEED_X += git-upload-pack
>  BINDIR_PROGRAMS_NEED_X += git-receive-pack
> -BINDIR_PROGRAMS_NEED_X += git-upload-archive
>  BINDIR_PROGRAMS_NEED_X += git-shell
>  
>  BINDIR_PROGRAMS_NO_X += git-cvsserver
> @@ -722,7 +721,6 @@ BUILTIN_OBJS += builtin/unpack-objects.o
>  BUILTIN_OBJS += builtin/update-index.o
>  BUILTIN_OBJS += builtin/update-ref.o
>  BUILTIN_OBJS += builtin/update-server-info.o
> -BUILTIN_OBJS += builtin/upload-archive.o
>  BUILTIN_OBJS += builtin/var.o
>  BUILTIN_OBJS += builtin/verify-pack.o
>  BUILTIN_OBJS += builtin/verify-tag.o
> @@ -1162,8 +1160,17 @@ ifdef ZLIB_PATH
>  endif
>  EXTLIBS += -lz
>  
> +ifndef NO_POLL
> +	BUILTIN_OBJS += builtin/upload-archive.o
> +	PROGRAM_OBJS += upload-pack.o
> +	BINDIR_PROGRAMS_NEED_X += git-upload-archive
> +	BINDIR_PROGRAMS_NEED_X += git-upload-pack
> +endif
> +
>  ifndef NO_POSIX_ONLY_PROGRAMS
> -	PROGRAM_OBJS += daemon.o
> +	ifndef NO_POLL
> +		PROGRAM_OBJS += daemon.o
> +	endif
>  endif
>  ifndef NO_OPENSSL
>  	OPENSSL_LIBSSL = -lssl
> @@ -1322,6 +1329,10 @@ ifdef OLD_ICONV
>  	BASIC_CFLAGS += -DOLD_ICONV
>  endif
>  
> +ifdef NO_POLL
> +	BASIC_CFLAGS += -DNO_POLL
> +endif
> +
>  ifdef NO_DEFLATE_BOUND
>  	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
>  endif
> diff --git a/builtin.h b/builtin.h
> index 5c887ef..165a748 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -127,7 +127,9 @@ extern int cmd_unpack_objects(int argc, const char **argv, const char *prefix);
>  extern int cmd_update_index(int argc, const char **argv, const char *prefix);
>  extern int cmd_update_ref(int argc, const char **argv, const char *prefix);
>  extern int cmd_update_server_info(int argc, const char **argv, const char *prefix);
> +#ifdef NO_POLL
>  extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
> +#endif

Shouldn't this be "ifndef"? makes me wonder how the test compile worked...

>  extern int cmd_upload_tar(int argc, const char **argv, const char *prefix);
>  extern int cmd_var(int argc, const char **argv, const char *prefix);
>  extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index edf352d..c5188e5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -94,7 +94,9 @@
>  #include <utime.h>
>  #ifndef __MINGW32__
>  #include <sys/wait.h>
> +#ifndef NO_POLL
>  #include <sys/poll.h>
> +#endif
>  #include <sys/socket.h>
>  #include <sys/ioctl.h>
>  #include <termios.h>
> diff --git a/git.c b/git.c
> index 99f0363..8c081db 100644
> --- a/git.c
> +++ b/git.c
> @@ -390,7 +390,9 @@ static void handle_internal_command(int argc, const char **argv)
>  		{ "update-index", cmd_update_index, RUN_SETUP },
>  		{ "update-ref", cmd_update_ref, RUN_SETUP },
>  		{ "update-server-info", cmd_update_server_info, RUN_SETUP },
> +#ifndef NO_POLL
>  		{ "upload-archive", cmd_upload_archive },
> +#endif
>  		{ "var", cmd_var },
>  		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
>  		{ "version", cmd_version },

Re: [PATCH 1/3] Support building on systems without poll(2)

[Jonathan Callen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 05/27/2010 04:51 AM, Michael J Gruber wrote:
> Jonathan Callen venit, vidit, dixit 27.05.2010 10:19:
>> +#ifdef NO_POLL
>>  extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
>> +#endif
> 
> Shouldn't this be "ifndef"? makes me wonder how the test compile worked...
> 

Yeah, that's a mistake.  It should be #ifndef. The test compile worked
because cmd_upload_archive is never called when NO_POLL is defined (and
I didn't test on a system where it wasn't).

- -- 
Jonathan Callen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.15 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBCAAGBQJL/jfNAAoJELHSF2kinlg4avYP/3CxgTwg1Wcraf0JLxtg590T
r0VPk/FJfN70kCwch1107qrXFH2Lm5j57R2qcJRPbO4xLg3/7SVDo8uh5srRVBwh
jHV/lVHiDofzAwogv0T1dAd02/WrbpBKM8Wt6sT0fz9Y3yMGPfSuYCazQ0uiH6NA
I/Rk2CcTelNNSo4h8+XRTdMwiEVIehw1zUNX9HhH1SYWastMCgoh2PS+cpeGot+q
IvInTpcOJfOsREXTxjer9zf/GeZsTiEVLgDWZG6Uc0JOEvLTKOqk/BcSeOn4L2zv
lHz14vMT8NS68U/rfPvTjscgczyS2E47XHrvM0+1bswLuNVrilHdzgcnH2rhWFFP
tU/dA6qzzFPvpqLwhXOwxECUsnJ8km5i8oR1914Qwfxr88KVR8syYyDl4eSI3qMg
MbFRTAzRC+4KD1FcGaQ7f7f2J/2Um4wtTb5SrKUJVKk2ROI/2KfBFthI4SbIonOf
NYJp+2XEpc3vbwE6YbiFzXBkP2z/F4fFvorYi8epa1neX7YKF7Hp+U5UJIFBb8de
KMKCoX1ClXjsTQ3B72M29D/o52L67/xcyoPDIfmnjuxeBcrJBO82z8pqMCyF4nh+
xa7mU4rrk6D8ZHxv1uULf8Bqz29Eq6dzzeuNkoV/5plLC2XjWJ2qsMxvZY9dED4h
6SUO3TH9eJJW6eAZG9bB
=RkSh
-----END PGP SIGNATURE-----

[PATCH] compat: Add another rudimentary poll() emulation

[Jonathan Nieder]

Implement the subset of poll() semantics needed by git in terms of
select(), for use by the Interix port.  Inspired by commit 6ed807f
(Windows: A rudimentary poll() emulation, 2007-12-01).

Cc: Johannes Sixt <j6t@kdbg.org>
Cc: Jonathan Callen <abcd@gentoo.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Callen wrote:

> Some systems do not have sys/poll.h or poll(2).

Maybe this could help.  Thanks to DrNick on IRC for the suggestion.

Warning: untested.

 Makefile          |    8 ++++++++
 compat/poll.c     |   33 +++++++++++++++++++++++++++++++++
 compat/poll.h     |   13 +++++++++++++
 git-compat-util.h |    6 +++++-
 4 files changed, 59 insertions(+), 1 deletions(-)
 create mode 100644 compat/poll.c
 create mode 100644 compat/poll.h

diff --git a/Makefile b/Makefile
index 2f5d631..6715528 100644
--- a/Makefile
+++ b/Makefile
@@ -111,6 +111,9 @@ all::
 #
 # Define NO_PTHREADS if you do not have or do not want to use Pthreads.
 #
+# Define NO_POLL if you have a problem with the poll() system call (e.g.
+# Interix).
+#
 # Define NO_PREAD if you have a problem with pread() system call (e.g.
 # cygwin1.dll before v1.5.22).
 #
@@ -470,6 +473,7 @@ LIB_H += commit.h
 LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
+LIB_H += compat/poll.h
 LIB_H += compat/win32/pthread.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
@@ -1284,6 +1288,10 @@ endif
 ifdef OBJECT_CREATION_USES_RENAMES
 	COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
 endif
+ifdef NO_POLL
+	BASIC_CFLAGS += -DNO_POLL
+	COMPAT_OBJS += compat/poll.o
+endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
diff --git a/compat/poll.c b/compat/poll.c
new file mode 100644
index 0000000..33c6ae0
--- /dev/null
+++ b/compat/poll.c
@@ -0,0 +1,33 @@
+#include "../git-compat-util.h"
+
+int git_poll(struct git_pollfd *ufds, int nfds, int timeout)
+{
+	int i, maxfd, result;
+	fd_set fds;
+
+	if (timeout >= 0) {
+		if (nfds == 0) {
+			sleep(timeout);
+			return 0;
+		}
+		return errno = EINVAL, error("poll timeout not supported");
+	}
+
+	FD_ZERO(&fds);
+	maxfd = 0;
+	for (i = 0; i < nfds; i++) {
+		if (ufds[i].events != POLLIN)
+			return errno = EINVAL, error("poll: unsupported events");
+		maxfd = (ufds[i].fd > maxfd) ? ufds[i].fd : maxfd;
+		FD_SET(ufds[i].fd, &fds);
+	}
+
+	result = select(maxfd + 1, &fds, NULL, NULL, NULL);
+	if (result == -1)
+		return result;
+	for (i = 0; i < nfds; i++) {
+		if (FD_ISSET(ufds[i].fd, &fds))
+			ufds[i].revents |= POLLIN;
+	}
+	return result;
+}
diff --git a/compat/poll.h b/compat/poll.h
new file mode 100644
index 0000000..65775ab
--- /dev/null
+++ b/compat/poll.h
@@ -0,0 +1,13 @@
+#ifndef POLLIN
+#define POLLIN 1
+#define POLLHUP 2
+#endif
+
+#define pollfd git_pollfd
+#define poll git_poll
+struct git_pollfd {
+	int fd;		/* file descriptor */
+	short events;	/* requested events */
+	short revents;	/* returned events */
+};
+extern int git_poll(struct git_pollfd *fds, int nfds, int timeout);
diff --git a/git-compat-util.h b/git-compat-util.h
index 824c175..2494378 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -94,13 +94,17 @@
 #include <utime.h>
 #ifndef __MINGW32__
 #include <sys/wait.h>
-#include <sys/poll.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <termios.h>
 #ifndef NO_SYS_SELECT_H
 #include <sys/select.h>
 #endif
+#ifdef NO_POLL
+#include "compat/poll.h"
+#else
+#include <sys/poll.h>
+#endif
 #include <netinet/in.h>
 #include <netinet/tcp.h>
 #include <arpa/inet.h>
-- 
1.7.1

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Implement the subset of poll() semantics needed by git in terms of
> select(), for use by the Interix port.  Inspired by commit 6ed807f
> (Windows: A rudimentary poll() emulation, 2007-12-01).
>

A possible problem with this approach is that the maximum number of
file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
maximum number of file descriptors select can handle is limited by
FD_SETSIZE.

I don't think this is a big problem in reality, though - both values
seem to be pretty high in most implementations. And IIRC git-daemon is
the only one who needs more than 2, and it doesn't even check
RLIMIT_NOFILE.

If we decide to go this route, perhaps it'd make sense to change to
this code for Windows also? Our Windows-implementation of poll() has
some annoying limitations...

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Marko Kreen]

On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  > Implement the subset of poll() semantics needed by git in terms of
>  > select(), for use by the Interix port.  Inspired by commit 6ed807f
>  > (Windows: A rudimentary poll() emulation, 2007-12-01).
>  >
>
>
> A possible problem with this approach is that the maximum number of
>  file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>  maximum number of file descriptors select can handle is limited by
>  FD_SETSIZE.
>
>  I don't think this is a big problem in reality, though - both values
>  seem to be pretty high in most implementations. And IIRC git-daemon is
>  the only one who needs more than 2, and it doesn't even check
>  RLIMIT_NOFILE.
>
>  If we decide to go this route, perhaps it'd make sense to change to
>  this code for Windows also? Our Windows-implementation of poll() has
>  some annoying limitations...

Example of poll() compat without FD_SETSIZE limit:

  http://github.com/markokr/plproxy-dev/blob/master/src/poll_compat.c

-- 
marko

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 1:39 PM, Marko Kreen <markokr@gmail.com> wrote:
> On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>> On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  > Implement the subset of poll() semantics needed by git in terms of
>>  > select(), for use by the Interix port.  Inspired by commit 6ed807f
>>  > (Windows: A rudimentary poll() emulation, 2007-12-01).
>>  >
>>
>>
>> A possible problem with this approach is that the maximum number of
>>  file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>>  maximum number of file descriptors select can handle is limited by
>>  FD_SETSIZE.
>>
>>  I don't think this is a big problem in reality, though - both values
>>  seem to be pretty high in most implementations. And IIRC git-daemon is
>>  the only one who needs more than 2, and it doesn't even check
>>  RLIMIT_NOFILE.
>>
>>  If we decide to go this route, perhaps it'd make sense to change to
>>  this code for Windows also? Our Windows-implementation of poll() has
>>  some annoying limitations...
>
> Example of poll() compat without FD_SETSIZE limit:
>
>  http://github.com/markokr/plproxy-dev/blob/master/src/poll_compat.c
>

How does this code convince FD_SET() that the buffer has increased? It
looks to me like it depends on a specific FD_SET() implementation...
For instance, Windows' FD_SET() implementation is like this:

#define FD_SET(fd, set) do { \
    if (((fd_set FAR *)(set))->fd_count < FD_SETSIZE) \
        ((fd_set FAR *)(set))->fd_array[((fd_set FAR
*)(set))->fd_count++]=(fd);\
} while(0)

...so unless another set is passed in, it won't add any more fds once
fd_count reaches FD_SETSIZE.

Also, FD_SETSIZE is 64 on Windows. IIRC it's 1024 on Linux, so it is
much more likely that we encounter this issue on Windows than on
Linux, at least ;)

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Marko Kreen]

On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 1:39 PM, Marko Kreen <markokr@gmail.com> wrote:
>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>  >> On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  >>  > Implement the subset of poll() semantics needed by git in terms of
>  >>  > select(), for use by the Interix port.  Inspired by commit 6ed807f
>  >>  > (Windows: A rudimentary poll() emulation, 2007-12-01).
>  >>  >
>  >>
>  >>
>  >> A possible problem with this approach is that the maximum number of
>  >>  file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>  >>  maximum number of file descriptors select can handle is limited by
>  >>  FD_SETSIZE.
>  >>
>  >>  I don't think this is a big problem in reality, though - both values
>  >>  seem to be pretty high in most implementations. And IIRC git-daemon is
>  >>  the only one who needs more than 2, and it doesn't even check
>  >>  RLIMIT_NOFILE.
>  >>
>  >>  If we decide to go this route, perhaps it'd make sense to change to
>  >>  this code for Windows also? Our Windows-implementation of poll() has
>  >>  some annoying limitations...
>  >
>  > Example of poll() compat without FD_SETSIZE limit:
>  >
>  >  http://github.com/markokr/plproxy-dev/blob/master/src/poll_compat.c
>  >
>
>
> How does this code convince FD_SET() that the buffer has increased? It
>  looks to me like it depends on a specific FD_SET() implementation...
>  For instance, Windows' FD_SET() implementation is like this:
>
>  #define FD_SET(fd, set) do { \
>     if (((fd_set FAR *)(set))->fd_count < FD_SETSIZE) \
>         ((fd_set FAR *)(set))->fd_array[((fd_set FAR
>  *)(set))->fd_count++]=(fd);\
>  } while(0)
>
>  ...so unless another set is passed in, it won't add any more fds once
>  fd_count reaches FD_SETSIZE.
>
>  Also, FD_SETSIZE is 64 on Windows. IIRC it's 1024 on Linux, so it is
>  much more likely that we encounter this issue on Windows than on
>  Linux, at least ;)

Hm, good catch.  Seems such compat poll() cannot be done without
OS-specific hacks.

Do you know perhaps what other OS-es have non-bitmap fd_set?

-- 
marko

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 2:36 PM, Marko Kreen <markokr@gmail.com> wrote:
> On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>> On Thu, May 27, 2010 at 1:39 PM, Marko Kreen <markokr@gmail.com> wrote:
>>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>>  >> On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  >>  > Implement the subset of poll() semantics needed by git in terms of
>>  >>  > select(), for use by the Interix port.  Inspired by commit 6ed807f
>>  >>  > (Windows: A rudimentary poll() emulation, 2007-12-01).
>>  >>  >
>>  >>
>>  >>
>>  >> A possible problem with this approach is that the maximum number of
>>  >>  file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>>  >>  maximum number of file descriptors select can handle is limited by
>>  >>  FD_SETSIZE.
>>  >>
>>  >>  I don't think this is a big problem in reality, though - both values
>>  >>  seem to be pretty high in most implementations. And IIRC git-daemon is
>>  >>  the only one who needs more than 2, and it doesn't even check
>>  >>  RLIMIT_NOFILE.
>>  >>
>>  >>  If we decide to go this route, perhaps it'd make sense to change to
>>  >>  this code for Windows also? Our Windows-implementation of poll() has
>>  >>  some annoying limitations...
>>  >
>>  > Example of poll() compat without FD_SETSIZE limit:
>>  >
>>  >  http://github.com/markokr/plproxy-dev/blob/master/src/poll_compat.c
>>  >
>>
>>
>> How does this code convince FD_SET() that the buffer has increased? It
>>  looks to me like it depends on a specific FD_SET() implementation...
>>  For instance, Windows' FD_SET() implementation is like this:
>>
>>  #define FD_SET(fd, set) do { \
>>     if (((fd_set FAR *)(set))->fd_count < FD_SETSIZE) \
>>         ((fd_set FAR *)(set))->fd_array[((fd_set FAR
>>  *)(set))->fd_count++]=(fd);\
>>  } while(0)
>>
>>  ...so unless another set is passed in, it won't add any more fds once
>>  fd_count reaches FD_SETSIZE.
>>
>>  Also, FD_SETSIZE is 64 on Windows. IIRC it's 1024 on Linux, so it is
>>  much more likely that we encounter this issue on Windows than on
>>  Linux, at least ;)
>
> Hm, good catch.  Seems such compat poll() cannot be done without
> OS-specific hacks.
>

Perhaps getrlimit() could overridden to return FD_SETSIZE for both the
soft and hard limit when asking about RLIMIT_NOFILE? In such cases,
anyone who passes nfds above FD_SETSIZE hasn't consulted
RLIMIT_NOFILE, and should be outside the standard. But your point
might have been about a limitless poll()-implementation, like the code
you linked tried to achieve. In that context, no. I doubt it's
possible to do in a robust fashion.

For git, I don't think this is necessary, though. As said, I think
git-daemon is the only call-site for poll where nfds can be above 2.
And git-daemon's default max-connection is 32, which shouldn't cause
much problems. There's the theoretical problem of someone setting
--max-connections above their platform's limit, but I don't think we
need to touch that ;)

> Do you know perhaps what other OS-es have non-bitmap fd_set?

No, I don't have much low-level file-descriptor knowledge about other
OS'es than Windows, really.

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] Re: [PATCH] compat: Add another rudimentary poll() emulation

[Albert Dvornik]

On Thu, May 27, 2010 at 8:57 AM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 2:36 PM, Marko Kreen <markokr@gmail.com> wrote:
[...]
>> Hm, good catch.  Seems such compat poll() cannot be done without
>> OS-specific hacks.
>>
>
> Perhaps getrlimit() could overridden to return FD_SETSIZE for both the
> soft and hard limit when asking about RLIMIT_NOFILE? In such cases,
> anyone who passes nfds above FD_SETSIZE hasn't consulted
> RLIMIT_NOFILE, and should be outside the standard. But your point
> might have been about a limitless poll()-implementation, like the code
> you linked tried to achieve. In that context, no. I doubt it's
> possible to do in a robust fashion.

This is getting far off-topic, but I do think it's possible to do in a
robust fashion if you are willing to sacrifice some performance [i.e.
we should not consider this for Git =) ], by creating multiple fd_set
structures and only shoving up to FD_SETSIZE descriptors into each.
Cycling between the structures would cause some extra latency and a
LOT more code complexity, but it should work.

--bert

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Paolo Bonzini]

On 05/27/2010 02:36 PM, Marko Kreen wrote:
> Hm, good catch.  Seems such compat poll() cannot be done without
> OS-specific hacks.
>
> Do you know perhaps what other OS-es have non-bitmap fd_set?

I don't think any does.  Winsock needed that because its fd_sets host 
handles instead of small integers.

Paolo

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Implement the subset of poll() semantics needed by git in terms of
>> select(), for use by the Interix port.  Inspired by commit 6ed807f
>> (Windows: A rudimentary poll() emulation, 2007-12-01).
>>
>
> A possible problem with this approach is that the maximum number of
> file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
> maximum number of file descriptors select can handle is limited by
> FD_SETSIZE.
>
> I don't think this is a big problem in reality, though - both values
> seem to be pretty high in most implementations. And IIRC git-daemon is
> the only one who needs more than 2, and it doesn't even check
> RLIMIT_NOFILE.
>

To be clear: I think this strategy is the best option (at least for
non-Windows, where select() might be our only option).

But perhaps you should include a check along the lines of this:

if (nfds > FD_SETSIZE)
	return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);

Just so we can know when the code fails :)

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Marko Kreen]

On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>  <kusmabite@googlemail.com> wrote:
>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  >> Implement the subset of poll() semantics needed by git in terms of
>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>  >>
>  >
>  > A possible problem with this approach is that the maximum number of
>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>  > maximum number of file descriptors select can handle is limited by
>  > FD_SETSIZE.
>  >
>  > I don't think this is a big problem in reality, though - both values
>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>  > the only one who needs more than 2, and it doesn't even check
>  > RLIMIT_NOFILE.
>  >
>
>
> To be clear: I think this strategy is the best option (at least for
>  non-Windows, where select() might be our only option).
>
>  But perhaps you should include a check along the lines of this:
>
>  if (nfds > FD_SETSIZE)
>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>
>  Just so we can know when the code fails :)

Well, per your own FD_SET example, the FD_SETSIZE on windows
means different thing than FD_SETSIZE on old-style bitmap-based
select() implementation.

On Unix, it's max fd number + 1, on windows it's max count.

Thus on windows the nfds can safely be larger than FD_SETSIZE,
assuming the actual count is smaller.



RLIMIT_NOFILE - I don't see why it is relevant here, as you
get error on open() when you cross the limit.  So how can you
pass more than that meny fds to select()/poll()?

-- 
marko

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
> On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>>  <kusmabite@googlemail.com> wrote:
>>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  >> Implement the subset of poll() semantics needed by git in terms of
>>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>>  >>
>>  >
>>  > A possible problem with this approach is that the maximum number of
>>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>>  > maximum number of file descriptors select can handle is limited by
>>  > FD_SETSIZE.
>>  >
>>  > I don't think this is a big problem in reality, though - both values
>>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>>  > the only one who needs more than 2, and it doesn't even check
>>  > RLIMIT_NOFILE.
>>  >
>>
>>
>> To be clear: I think this strategy is the best option (at least for
>>  non-Windows, where select() might be our only option).
>>
>>  But perhaps you should include a check along the lines of this:
>>
>>  if (nfds > FD_SETSIZE)
>>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>>
>>  Just so we can know when the code fails :)
>
> Well, per your own FD_SET example, the FD_SETSIZE on windows
> means different thing than FD_SETSIZE on old-style bitmap-based
> select() implementation.
>
> On Unix, it's max fd number + 1, on windows it's max count.
>

Are you sure this applies for all Unix, not just some given Unix-y system?

> RLIMIT_NOFILE - I don't see why it is relevant here, as you
> get error on open() when you cross the limit.  So how can you
> pass more than that meny fds to select()/poll()?
>

Good point, this was my bad.

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Marko Kreen]

On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>  >> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>  >>  <kusmabite@googlemail.com> wrote:
>  >>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  >>  >> Implement the subset of poll() semantics needed by git in terms of
>  >>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>  >>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>  >>  >>
>  >>  >
>  >>  > A possible problem with this approach is that the maximum number of
>  >>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>  >>  > maximum number of file descriptors select can handle is limited by
>  >>  > FD_SETSIZE.
>  >>  >
>  >>  > I don't think this is a big problem in reality, though - both values
>  >>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>  >>  > the only one who needs more than 2, and it doesn't even check
>  >>  > RLIMIT_NOFILE.
>  >>  >
>  >>
>  >>
>  >> To be clear: I think this strategy is the best option (at least for
>  >>  non-Windows, where select() might be our only option).
>  >>
>  >>  But perhaps you should include a check along the lines of this:
>  >>
>  >>  if (nfds > FD_SETSIZE)
>  >>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>  >>
>  >>  Just so we can know when the code fails :)
>  >
>  > Well, per your own FD_SET example, the FD_SETSIZE on windows
>  > means different thing than FD_SETSIZE on old-style bitmap-based
>  > select() implementation.
>  >
>  > On Unix, it's max fd number + 1, on windows it's max count.
>  >
>
>
> Are you sure this applies for all Unix, not just some given Unix-y system?

Not sure.  Just pointing out that the above check is not
universal enough.

-- 
marko

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 3:58 PM, Marko Kreen <markokr@gmail.com> wrote:
> On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>> On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
>>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>>  >> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>>  >>  <kusmabite@googlemail.com> wrote:
>>  >>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  >>  >> Implement the subset of poll() semantics needed by git in terms of
>>  >>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>>  >>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>>  >>  >>
>>  >>  >
>>  >>  > A possible problem with this approach is that the maximum number of
>>  >>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>>  >>  > maximum number of file descriptors select can handle is limited by
>>  >>  > FD_SETSIZE.
>>  >>  >
>>  >>  > I don't think this is a big problem in reality, though - both values
>>  >>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>>  >>  > the only one who needs more than 2, and it doesn't even check
>>  >>  > RLIMIT_NOFILE.
>>  >>  >
>>  >>
>>  >>
>>  >> To be clear: I think this strategy is the best option (at least for
>>  >>  non-Windows, where select() might be our only option).
>>  >>
>>  >>  But perhaps you should include a check along the lines of this:
>>  >>
>>  >>  if (nfds > FD_SETSIZE)
>>  >>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>>  >>
>>  >>  Just so we can know when the code fails :)
>>  >
>>  > Well, per your own FD_SET example, the FD_SETSIZE on windows
>>  > means different thing than FD_SETSIZE on old-style bitmap-based
>>  > select() implementation.
>>  >
>>  > On Unix, it's max fd number + 1, on windows it's max count.
>>  >
>>
>>
>> Are you sure this applies for all Unix, not just some given Unix-y system?
>
> Not sure.  Just pointing out that the above check is not
> universal enough.
>

Isn't it? How could one possibly pass more than max fd number + 1 file
descriptors, since they start at 0? I guess one could specify a given
fd more than once, but that'd be kind of redundant... and also very
unlikely in our case ;)

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Marko Kreen]

On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 3:58 PM, Marko Kreen <markokr@gmail.com> wrote:
>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>  >> On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
>  >>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>  >>  >> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>  >>  >>  <kusmabite@googlemail.com> wrote:
>  >>  >>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  >>  >>  >> Implement the subset of poll() semantics needed by git in terms of
>  >>  >>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>  >>  >>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>  >>  >>  >>
>  >>  >>  >
>  >>  >>  > A possible problem with this approach is that the maximum number of
>  >>  >>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>  >>  >>  > maximum number of file descriptors select can handle is limited by
>  >>  >>  > FD_SETSIZE.
>  >>  >>  >
>  >>  >>  > I don't think this is a big problem in reality, though - both values
>  >>  >>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>  >>  >>  > the only one who needs more than 2, and it doesn't even check
>  >>  >>  > RLIMIT_NOFILE.
>  >>  >>  >
>  >>  >>
>  >>  >>
>  >>  >> To be clear: I think this strategy is the best option (at least for
>  >>  >>  non-Windows, where select() might be our only option).
>  >>  >>
>  >>  >>  But perhaps you should include a check along the lines of this:
>  >>  >>
>  >>  >>  if (nfds > FD_SETSIZE)
>  >>  >>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>  >>  >>
>  >>  >>  Just so we can know when the code fails :)
>  >>  >
>  >>  > Well, per your own FD_SET example, the FD_SETSIZE on windows
>  >>  > means different thing than FD_SETSIZE on old-style bitmap-based
>  >>  > select() implementation.
>  >>  >
>  >>  > On Unix, it's max fd number + 1, on windows it's max count.
>  >>  >
>  >>
>  >>
>  >> Are you sure this applies for all Unix, not just some given Unix-y system?
>  >
>  > Not sure.  Just pointing out that the above check is not
>  > universal enough.
>  >
>
>
> Isn't it? How could one possibly pass more than max fd number + 1 file
>  descriptors, since they start at 0? I guess one could specify a given
>  fd more than once, but that'd be kind of redundant... and also very
>  unlikely in our case ;)

Pass one fd with value 70 it.  Check returns error, although
everything would work.

-- 
marko

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 4:11 PM, Marko Kreen <markokr@gmail.com> wrote:
> On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>> On Thu, May 27, 2010 at 3:58 PM, Marko Kreen <markokr@gmail.com> wrote:
>>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>>  >> On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
>>  >>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>>  >>  >> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>>  >>  >>  <kusmabite@googlemail.com> wrote:
>>  >>  >>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  >>  >>  >> Implement the subset of poll() semantics needed by git in terms of
>>  >>  >>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>>  >>  >>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>>  >>  >>  >>
>>  >>  >>  >
>>  >>  >>  > A possible problem with this approach is that the maximum number of
>>  >>  >>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>>  >>  >>  > maximum number of file descriptors select can handle is limited by
>>  >>  >>  > FD_SETSIZE.
>>  >>  >>  >
>>  >>  >>  > I don't think this is a big problem in reality, though - both values
>>  >>  >>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>>  >>  >>  > the only one who needs more than 2, and it doesn't even check
>>  >>  >>  > RLIMIT_NOFILE.
>>  >>  >>  >
>>  >>  >>
>>  >>  >>
>>  >>  >> To be clear: I think this strategy is the best option (at least for
>>  >>  >>  non-Windows, where select() might be our only option).
>>  >>  >>
>>  >>  >>  But perhaps you should include a check along the lines of this:
>>  >>  >>
>>  >>  >>  if (nfds > FD_SETSIZE)
>>  >>  >>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>>  >>  >>
>>  >>  >>  Just so we can know when the code fails :)
>>  >>  >
>>  >>  > Well, per your own FD_SET example, the FD_SETSIZE on windows
>>  >>  > means different thing than FD_SETSIZE on old-style bitmap-based
>>  >>  > select() implementation.
>>  >>  >
>>  >>  > On Unix, it's max fd number + 1, on windows it's max count.
>>  >>  >
>>  >>
>>  >>
>>  >> Are you sure this applies for all Unix, not just some given Unix-y system?
>>  >
>>  > Not sure.  Just pointing out that the above check is not
>>  > universal enough.
>>  >
>>
>>
>> Isn't it? How could one possibly pass more than max fd number + 1 file
>>  descriptors, since they start at 0? I guess one could specify a given
>>  fd more than once, but that'd be kind of redundant... and also very
>>  unlikely in our case ;)
>
> Pass one fd with value 70 it.  Check returns error, although
> everything would work.
>

No, not with the check I posted. I checked nfds, not the value of the
fds themselves. nfds is clearly the size of the fds array -- if it
wasn't, it'd be impossible for the poll-implementation to know how big
the array is!

-- 
Erik "kusma" Faye-Lund

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Marko Kreen]

On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 4:11 PM, Marko Kreen <markokr@gmail.com> wrote:
>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>  >> On Thu, May 27, 2010 at 3:58 PM, Marko Kreen <markokr@gmail.com> wrote:
>  >>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>  >>  >> On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
>  >>  >>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>  >>  >>  >> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>  >>  >>  >>  <kusmabite@googlemail.com> wrote:
>  >>  >>  >>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  >>  >>  >>  >> Implement the subset of poll() semantics needed by git in terms of
>  >>  >>  >>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>  >>  >>  >>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>  >>  >>  >>  >>
>  >>  >>  >>  >
>  >>  >>  >>  > A possible problem with this approach is that the maximum number of
>  >>  >>  >>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>  >>  >>  >>  > maximum number of file descriptors select can handle is limited by
>  >>  >>  >>  > FD_SETSIZE.
>  >>  >>  >>  >
>  >>  >>  >>  > I don't think this is a big problem in reality, though - both values
>  >>  >>  >>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>  >>  >>  >>  > the only one who needs more than 2, and it doesn't even check
>  >>  >>  >>  > RLIMIT_NOFILE.
>  >>  >>  >>  >
>  >>  >>  >>
>  >>  >>  >>
>  >>  >>  >> To be clear: I think this strategy is the best option (at least for
>  >>  >>  >>  non-Windows, where select() might be our only option).
>  >>  >>  >>
>  >>  >>  >>  But perhaps you should include a check along the lines of this:
>  >>  >>  >>
>  >>  >>  >>  if (nfds > FD_SETSIZE)
>  >>  >>  >>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>  >>  >>  >>
>  >>  >>  >>  Just so we can know when the code fails :)
>  >>  >>  >
>  >>  >>  > Well, per your own FD_SET example, the FD_SETSIZE on windows
>  >>  >>  > means different thing than FD_SETSIZE on old-style bitmap-based
>  >>  >>  > select() implementation.
>  >>  >>  >
>  >>  >>  > On Unix, it's max fd number + 1, on windows it's max count.
>  >>  >>  >
>  >>  >>
>  >>  >>
>  >>  >> Are you sure this applies for all Unix, not just some given Unix-y system?
>  >>  >
>  >>  > Not sure.  Just pointing out that the above check is not
>  >>  > universal enough.
>  >>  >
>  >>
>  >>
>  >> Isn't it? How could one possibly pass more than max fd number + 1 file
>  >>  descriptors, since they start at 0? I guess one could specify a given
>  >>  fd more than once, but that'd be kind of redundant... and also very
>  >>  unlikely in our case ;)
>  >
>  > Pass one fd with value 70 it.  Check returns error, although
>  > everything would work.
>  >
>
>
> No, not with the check I posted. I checked nfds, not the value of the
>  fds themselves. nfds is clearly the size of the fds array -- if it
>  wasn't, it'd be impossible for the poll-implementation to know how big
>  the array is!

Ah, ok.  I though this is the nfds for select().  In this case it can
still fail, as nfds=1 for poll() can still go over FD_SETSIZE for
select() if the fd value is big enough.  On non-windows, that is.

Dunno if this matters for git-daemon..

-- 
marko

Re: [PATCH] compat: Add another rudimentary poll() emulation

[Erik Faye-Lund]

On Thu, May 27, 2010 at 4:32 PM, Marko Kreen <markokr@gmail.com> wrote:
> On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>> On Thu, May 27, 2010 at 4:11 PM, Marko Kreen <markokr@gmail.com> wrote:
>>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>>  >> On Thu, May 27, 2010 at 3:58 PM, Marko Kreen <markokr@gmail.com> wrote:
>>  >>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>>  >>  >> On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
>>  >>  >>  > On 5/27/10, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>>  >>  >>  >> On Thu, May 27, 2010 at 1:00 PM, Erik Faye-Lund
>>  >>  >>  >>  <kusmabite@googlemail.com> wrote:
>>  >>  >>  >>  > On Thu, May 27, 2010 at 12:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  >>  >>  >>  >> Implement the subset of poll() semantics needed by git in terms of
>>  >>  >>  >>  >> select(), for use by the Interix port.  Inspired by commit 6ed807f
>>  >>  >>  >>  >> (Windows: A rudimentary poll() emulation, 2007-12-01).
>>  >>  >>  >>  >>
>>  >>  >>  >>  >
>>  >>  >>  >>  > A possible problem with this approach is that the maximum number of
>>  >>  >>  >>  > file descriptors poll can handle limited by RLIMIT_NOFILE, whereas the
>>  >>  >>  >>  > maximum number of file descriptors select can handle is limited by
>>  >>  >>  >>  > FD_SETSIZE.
>>  >>  >>  >>  >
>>  >>  >>  >>  > I don't think this is a big problem in reality, though - both values
>>  >>  >>  >>  > seem to be pretty high in most implementations. And IIRC git-daemon is
>>  >>  >>  >>  > the only one who needs more than 2, and it doesn't even check
>>  >>  >>  >>  > RLIMIT_NOFILE.
>>  >>  >>  >>  >
>>  >>  >>  >>
>>  >>  >>  >>
>>  >>  >>  >> To be clear: I think this strategy is the best option (at least for
>>  >>  >>  >>  non-Windows, where select() might be our only option).
>>  >>  >>  >>
>>  >>  >>  >>  But perhaps you should include a check along the lines of this:
>>  >>  >>  >>
>>  >>  >>  >>  if (nfds > FD_SETSIZE)
>>  >>  >>  >>         return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>>  >>  >>  >>
>>  >>  >>  >>  Just so we can know when the code fails :)
>>  >>  >>  >
>>  >>  >>  > Well, per your own FD_SET example, the FD_SETSIZE on windows
>>  >>  >>  > means different thing than FD_SETSIZE on old-style bitmap-based
>>  >>  >>  > select() implementation.
>>  >>  >>  >
>>  >>  >>  > On Unix, it's max fd number + 1, on windows it's max count.
>>  >>  >>  >
>>  >>  >>
>>  >>  >>
>>  >>  >> Are you sure this applies for all Unix, not just some given Unix-y system?
>>  >>  >
>>  >>  > Not sure.  Just pointing out that the above check is not
>>  >>  > universal enough.
>>  >>  >
>>  >>
>>  >>
>>  >> Isn't it? How could one possibly pass more than max fd number + 1 file
>>  >>  descriptors, since they start at 0? I guess one could specify a given
>>  >>  fd more than once, but that'd be kind of redundant... and also very
>>  >>  unlikely in our case ;)
>>  >
>>  > Pass one fd with value 70 it.  Check returns error, although
>>  > everything would work.
>>  >
>>
>>
>> No, not with the check I posted. I checked nfds, not the value of the
>>  fds themselves. nfds is clearly the size of the fds array -- if it
>>  wasn't, it'd be impossible for the poll-implementation to know how big
>>  the array is!
>
> Ah, ok.  I though this is the nfds for select().  In this case it can
> still fail, as nfds=1 for poll() can still go over FD_SETSIZE for
> select() if the fd value is big enough.  On non-windows, that is.
>

I don't quite get where one would get a fd that's value is FD_SETSIZE
or above from. I mean, FD_SETSIZE would have to be the maximum value
of a file descriptor, or else you risk open() or socket() returning
file descriptors that cannot be select()'ed. Which would just be
insane, no?

-- 
Erik "kusma" Faye-Lund

RE: [PATCH] compat: Add another rudimentary poll() emulation

[Peter Kjellerstedt]

> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Erik Faye-Lund
> Sent: den 27 maj 2010 16:44
> To: Marko Kreen
> Cc: Jonathan Nieder; Jonathan Callen; git@vger.kernel.org;
> mduft@gentoo.org; Sverre Rabbelier; Michael J Gruber; Johannes Sixt;
> msysGit
> Subject: Re: [PATCH] compat: Add another rudimentary poll() emulation
> 
> On Thu, May 27, 2010 at 4:32 PM, Marko Kreen <markokr@gmail.com> wrote:

[cut]

> > Ah, ok.  I though this is the nfds for select().  In this case it can
> > still fail, as nfds=1 for poll() can still go over FD_SETSIZE for
> > select() if the fd value is big enough.  On non-windows, that is.
> 
> I don't quite get where one would get a fd that's value is FD_SETSIZE
> or above from. I mean, FD_SETSIZE would have to be the maximum value
> of a file descriptor, or else you risk open() or socket() returning
> file descriptors that cannot be select()'ed. Which would just be
> insane, no?
> 
> --
> Erik "kusma" Faye-Lund

That is exactly how e.g. Linux works. If your application can have more 
than 1024 file descriptors open simultaneously, you are restricted to 
use poll() as you risk having a file descriptor with a value >= 1024.
And using FD_SET() with a file descriptor >= 1024 will trash memory...

//Peter

Re: [msysGit] Re: [PATCH] compat: Add another rudimentary poll() emulation

[Albert Dvornik]

On Thu, May 27, 2010 at 9:46 AM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> On Thu, May 27, 2010 at 3:29 PM, Marko Kreen <markokr@gmail.com> wrote:
[...]
>> Well, per your own FD_SET example, the FD_SETSIZE on windows
>> means different thing than FD_SETSIZE on old-style bitmap-based
>> select() implementation.

Yes, it sure does.  Once again, Windows seems to be a special, unique
snowflake. =)

>> On Unix, it's max fd number + 1, on windows it's max count.
>>
>
> Are you sure this applies for all Unix, not just some given Unix-y system?

While I can't make statements about "all UNIX" since there are many
obscure flavors, this is true on Linux, Solaris and various BSD
derivatives.  It's also what's specified by POSIX:

http://www.opengroup.org/onlinepubs/000095399/functions/select.html

--bert

Re: [msysGit] Re: [PATCH] compat: Add another rudimentary poll() emulation

[Albert Dvornik]

On Thu, May 27, 2010 at 9:06 AM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
[...]
> But perhaps you should include a check along the lines of this:
>
> if (nfds > FD_SETSIZE)
>        return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
>
> Just so we can know when the code fails :)

If you're checking against FD_SETSIZE (which is IMO a good idea), you
should consider that
(a) on the one system I'm aware of where fd_set doesn't use a bitmap
(Windows), FD_SETSIZE is a limit on the number of descriptors added to
the set, but
(b) on systems where fd_set uses a bitmap (i.e. Linux, perhaps all
UNIXes, etc), FD_SETSIZE is a limit on *each descriptor value*.  This
is also what POSIX says.

So on the latter systems, we want something like this before each FD_SET:

if (ufds[i].fd >= FD_SETSIZE) {
    errno = EINVAL;
    return error("poll: each fd must be below %d", FD_SETSIZE);
}

(The reason to have it in the loop, rather than just check maxfd
afterwards, is that FD_SET with an argument that's too big can trash
the stack.)

Of course, on Windows this would impose a limitation that all
descriptors be < 64, which is probably crazy.  Which means that you'd
have to actually distinguish the two types.  Sigh.

--bert

[PATCH v2] compat: Add another rudimentary poll() emulation

[Jonathan Nieder]

Implement the subset of poll() semantics needed by git in terms of
select(), for use by the Interix port.  Inspired by commit 6ed807f
(Windows: A rudimentary poll() emulation, 2007-12-01).

This will only poll on descriptors with value <= the maximum fd that
select() can portably handle, FD_SETSIZE - 1.  On Windows (and not on
Unix), in principle git could permit higher fds as long as the number
of descriptors used in a single select() call was limited to
FD_SETSIZE; but such a facility would be rarely used, since the number
of sockets the ‘git daemon’ parent process keeps open tends to be very
small.

Helped-by: Erik Faye-Lund <kusmabite@googlemail.com>
Cc: Johannes Sixt <j6t@kdbg.org>
Cc: Jonathan Callen <abcd@gentoo.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Erik Faye-Lund wrote:

> But perhaps you should include a check along the lines of this:
> 
> if (nfds > FD_SETSIZE)
> 	return errno = EINVAL, error("poll: nfds must be below %d", FD_SETSIZE);
> 
> Just so we can know when the code fails :)

Good catch, thanks.  I opted to check each fd instead so the NO_POLL
code can be safely used on Unix for debugging.

Other changes from v1:

 . use COMPAT_CFLAGS instead of BASIC_CFLAGS
 . timeout is a number of milliseconds, not seconds
 . lightly tested on Linux

I would be interested to hear whether this works on msysgit and Interix.
Thanks again for the help.

 Makefile          |    8 +++++++
 compat/poll.c     |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat/poll.h     |   13 ++++++++++++
 git-compat-util.h |    6 ++++-
 4 files changed, 82 insertions(+), 1 deletions(-)
 create mode 100644 compat/poll.c
 create mode 100644 compat/poll.h

diff --git a/Makefile b/Makefile
index 2f5d631..ed9f03b 100644
--- a/Makefile
+++ b/Makefile
@@ -111,6 +111,9 @@ all::
 #
 # Define NO_PTHREADS if you do not have or do not want to use Pthreads.
 #
+# Define NO_POLL if you have a problem with the poll() system call (e.g.
+# Interix).
+#
 # Define NO_PREAD if you have a problem with pread() system call (e.g.
 # cygwin1.dll before v1.5.22).
 #
@@ -470,6 +473,7 @@ LIB_H += commit.h
 LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
+LIB_H += compat/poll.h
 LIB_H += compat/win32/pthread.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
@@ -1284,6 +1288,10 @@ endif
 ifdef OBJECT_CREATION_USES_RENAMES
 	COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
 endif
+ifdef NO_POLL
+	COMPAT_CFLAGS += -DNO_POLL
+	COMPAT_OBJS += compat/poll.o
+endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
diff --git a/compat/poll.c b/compat/poll.c
new file mode 100644
index 0000000..0b3c2ae
--- /dev/null
+++ b/compat/poll.c
@@ -0,0 +1,56 @@
+#include "../git-compat-util.h"
+
+static int msleep(int timeout)
+{
+	struct timeval tv;
+	tv.tv_sec = 0;
+	tv.tv_usec = 1000 * timeout;
+	return select(0, NULL, NULL, NULL, &tv);
+}
+
+static int validate_fd(const struct git_pollfd *ufd)
+{
+	const int fd = ufd->fd;
+	if (ufd->events != POLLIN) {
+		errno = EINVAL;
+		return error("poll: unsupported events");
+	}
+	if (fd >= FD_SETSIZE) {
+		errno = EINVAL;
+		return error("poll: fd must be below %d", FD_SETSIZE);
+	}
+	return fd;
+}
+
+int git_poll(struct git_pollfd *ufds, int nfds, int timeout)
+{
+	int i, maxfd, result;
+	fd_set fds;
+
+	if (timeout >= 0) {
+		if (nfds != 0) {
+			errno = EINVAL;
+			return error("poll timeout not supported");
+		}
+		return msleep(timeout);
+	}
+
+	FD_ZERO(&fds);
+	maxfd = -1;
+	for (i = 0; i < nfds; i++) {
+		const int fd = validate_fd(&ufds[i]);
+		if (fd < 0)
+			return fd;
+		maxfd = (fd > maxfd) ? fd : maxfd;
+		FD_SET(fd, &fds);
+	}
+
+	result = select(maxfd + 1, &fds, NULL, NULL, NULL);
+	if (result < 0)
+		return result;
+	for (i = 0; i < nfds; i++) {
+		if (FD_ISSET(ufds[i].fd, &fds))
+			ufds[i].revents |= POLLIN;
+	}
+	return result;
+}
diff --git a/compat/poll.h b/compat/poll.h
new file mode 100644
index 0000000..65775ab
--- /dev/null
+++ b/compat/poll.h
@@ -0,0 +1,13 @@
+#ifndef POLLIN
+#define POLLIN 1
+#define POLLHUP 2
+#endif
+
+#define pollfd git_pollfd
+#define poll git_poll
+struct git_pollfd {
+	int fd;		/* file descriptor */
+	short events;	/* requested events */
+	short revents;	/* returned events */
+};
+extern int git_poll(struct git_pollfd *fds, int nfds, int timeout);
diff --git a/git-compat-util.h b/git-compat-util.h
index 824c175..2494378 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -94,13 +94,17 @@
 #include <utime.h>
 #ifndef __MINGW32__
 #include <sys/wait.h>
-#include <sys/poll.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <termios.h>
 #ifndef NO_SYS_SELECT_H
 #include <sys/select.h>
 #endif
+#ifdef NO_POLL
+#include "compat/poll.h"
+#else
+#include <sys/poll.h>
+#endif
 #include <netinet/in.h>
 #include <netinet/tcp.h>
 #include <arpa/inet.h>
-- 
1.7.1

Re: [PATCH v2] compat: Add another rudimentary poll() emulation

[Johannes Sixt]

On Sonntag, 30. Mai 2010, Jonathan Nieder wrote:
> I would be interested to hear whether this works on msysgit and Interix.

It cannot work on msysgit because

- on Windows, select() works only for sockets, but we poll() on pipes, too;

- in our emulation layer, fds that are sockets must be unpacked with 
_get_osfhandle() before they can be passed to FD_SET() because (as Paolo has 
pointed out) FD_SET() must be populated with handles, not fds.

-- Hannes

Re: [PATCH v2] compat: Add another rudimentary poll() emulation

[Jonathan Nieder]

Johannes Sixt wrote:

> It cannot work on msysgit because
> 
> - on Windows, select() works only for sockets, but we poll() on pipes, too;
> 
> - in our emulation layer, fds that are sockets must be unpacked with 
> _get_osfhandle() before they can be passed to FD_SET() because (as Paolo has 
> pointed out) FD_SET() must be populated with handles, not fds.

Thanks for the explanation.  Alas.

>From <http://www.suacommunity.com/man/2/select.2.html> I get the
impression that FD_SETSIZE on Interix might not be so small after all
(good).  Apparently older versions of Interix (5.3?) do not obey
timeouts properly, so the poll() in compat/poll.c should probably use
usleep() instead (meh).

Anyway, I leave it to developers on that platform to take it from here
if they so wish.

Ciao,
Jonathan

Re: [PATCH v2] compat: Add another rudimentary poll() emulation

[Joshua Juran]

On May 29, 2010, at 5:37 PM, Jonathan Nieder wrote:

> Implement the subset of poll() semantics needed by git in terms of
> select(), for use by the Interix port.  Inspired by commit 6ed807f
> (Windows: A rudimentary poll() emulation, 2007-12-01).
>
> I would be interested to hear whether this works on msysgit and  
> Interix.

For what it's worth, after creating a poll_compat library for Lamp  
(Lamp ain't Mac POSIX) from the original plproxy code and linking git  
against it, I'm able to serve fetch requests from git daemon.  I  
haven't tried simultaneous connections, though.

http://github.com/jjuran/git/

Josh

Mac OS 9 (Lamp) port

[Jonathan Nieder]

[new thread, cleared cc list]

Hi Josh,

Joshua Juran wrote:

> http://github.com/jjuran/git.git

I assume the lamp-git branch is the interesting one.  Some of
these patches (e.g., a3b378781) look like they might be of general
interest, while others (e.g., 6e6b106d) reveal Metrowerks to be
a bit braindead.

Have you thought about getting git to compile in C++ mode, where
Metrowerks might be a little more sane[1]?  Sure, this runs into
basically all the major incompatibilities between C and C++[2], but
that might not be insurmountable:

 . No implicit conversion from void* to other: don’t use void *,
   then.  With type-safe interfaces like

#define typed_malloc(size, type) (type *)xmalloc(size)
#define malloc_many(nmemb, type) typed_malloc((nmemb) * sizeof(type), type)

   one can take advantage of type checking without the annoyance of
   casts at the call site.

   Another place git uses void * is for low-level access to the
   object database, because it is not obvious whether objects data
   should use char * or unsigned char *.  unsigned char * should
   be fine.

 . Use of C++ keywords:

#ifdef __cplusplus
#define template git_template
#define typename git_typename
#endif

   It is not like this is an actual C++ program.

 . Assignment of ints to enums is forbidden: okay, this one is
   not worth working around.  Does Metrowerks have an option to turn
   off this piece of C++ insanity?

Curious,
Jonathan

[1] e.g., to solve the problem you described before:
http://thread.gmane.org/gmane.comp.version-control.git/115301

[2] http://www2.research.att.com/~bs/bs_faq.html#merge

Re: Mac OS 9 (Lamp) port

[Joshua Juran]

On May 30, 2010, at 8:19 PM, Jonathan Nieder wrote:

> Joshua Juran wrote:
>
>> http://github.com/jjuran/git.git

Apparently <http://github.com/jjuran/git> works better.

> I assume the lamp-git branch is the interesting one.  Some of

The lamp-git branch is the one you'd checkout to build git.  It  
merges the interesting ones.

> these patches (e.g., a3b378781) look like they might be of general

$ git show a3b378781
commit a3b378781228d59a13ee055d06c8f5465b543178
Author: Joshua Juran <jjuran@metamage.com>
Date:   Sun Nov 29 03:57:54 2009 -0800

     Avoid pointless use of one-celled array (which also confounds  
Metrowerks C).

     Pass the address of fetch_refspec_str directly to  
parse_refspec_internal()
     instead of copying fetch_refspec_str to an array and passing that.

     Metrowerks C won't initialize aggregates with values determined  
at runtime, and
     we can't use the C++ translator for parse_refspec_internal()  
because it uses the
     same identifier (refspec) for a variable as an existing struct  
type.

     That, and there's no purpose to the array in the first place.

diff --git a/remote.c b/remote.c
index c70181c..ade0424 100644
--- a/remote.c
+++ b/remote.c
@@ -657,10 +657,9 @@ static struct refspec *parse_refspec_internal 
(int nr_refspec, const char **refsp

  int valid_fetch_refspec(const char *fetch_refspec_str)
  {
-       const char *fetch_refspec[] = { fetch_refspec_str };
         struct refspec *refspec;

-       refspec = parse_refspec_internal(1, fetch_refspec, 1, 1);
+       refspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);
         free_refspecs(refspec, 1);
         return !!refspec;
  }

Yes, this is the patch I suggested to Gary V. Vaughan.

> interest, while others (e.g., 6e6b106d) reveal Metrowerks to be
> a bit braindead.

$ git show 6e6b106d
commit 6e6b106dd6c561db726f90e2db07beeef1f716f9
Author: Joshua Juran <jjuran@metamage.com>
Date:   Wed Apr 8 01:51:04 2009 -0700

     Cast compare_info() arguments to (const struct pack_info *const *).

     Otherwise, Metrowerks C reports "illegal implicit conversion  
from 'const void *'
     to 'struct pack_info *const *'".

diff --git a/server-info.c b/server-info.c
index 4098ca2..a9a0188 100644
--- a/server-info.c
+++ b/server-info.c
@@ -132,8 +132,8 @@ static int read_pack_info_file(const char *infofile)

  static int compare_info(const void *a_, const void *b_)
  {
-       struct pack_info *const *a = a_;
-       struct pack_info *const *b = b_;
+       const struct pack_info *const *a = a_;
+       const struct pack_info *const *b = b_;

         if (0 <= (*a)->old_num && 0 <= (*b)->old_num)
                 /* Keep the order in the original */

I certainly won't defend Metrowerks C 2.4.1, but the patch is  
harmless enough.

> Have you thought about getting git to compile in C++ mode, where

If the Git maintainers are interested in this, I'm glad to do it.  I  
expect it to require generally less maintenance than my c++- 
initializer-fix branch, which is rather ugly to boot.

> Metrowerks might be a little more sane[1]?  Sure, this runs into

Well, I wasn't going to suggest general C++ compatibility just for my  
port, but if there's interest, then yes, that would obviate my (by  
far) largest and nastiest patch.

> basically all the major incompatibilities between C and C++[2], but
> that might not be insurmountable:

Writing code which is both valid C and valid C++ is not difficult.

>  . No implicit conversion from void* to other: don’t use void *,
>    then.  With type-safe interfaces like
>
> #define typed_malloc(size, type) (type *)xmalloc(size)
> #define malloc_many(nmemb, type) typed_malloc((nmemb) * sizeof 
> (type), type)
>
>    one can take advantage of type checking without the annoyance of
>    casts at the call site.

Replacing xmalloc(size) with typed_malloc(size, struct foo) instead  
of (struct foo *)xmalloc(size) still feels like a cast to me.  IMHO  
it adds no safety and reduces readability.

Maybe you could have:

	#define alloc_n(type, n)  ((type*)xmalloc(sizeof (type) * n))

to factor the multiplication out of the calling code.  But for a  
plain data buffer, nothing is shorter or clearer than a simple  
(char*) cast.

>    Another place git uses void * is for low-level access to the
>    object database, because it is not obvious whether objects data
>    should use char * or unsigned char *.  unsigned char * should
>    be fine.

If there were a configurable typedef defining this, I'd use char*  
since the Metrowerks debugger by default displays it as a C string  
(as opposed to a Pascal string for unsigned char* -- it's a Mac OS  
convention).  But that's a minor point and not one I'd spend time  
advocating.

>  . Use of C++ keywords:
>
> #ifdef __cplusplus
> #define template git_template
> #define typename git_typename
> #endif
>
>    It is not like this is an actual C++ program.

My c++initializer-fix patch does this locally in a few places.  I  
suppose it makes sense to nail them all at once in git-compat-util.h.

>  . Assignment of ints to enums is forbidden: okay, this one is
>    not worth working around.  Does Metrowerks have an option to turn
>    off this piece of C++ insanity?

It's a feature.  :-)  I use enums for type-safety in my C++ POSIX  
wrapper library.  p7::wait() returns p7::wait_t, and if you try to  
pass that to p7::exit() (which takes p7::exit_t) you get a compile  
error (instead of a program that 'works' when the wait status is zero).

(No, I'm not aware of an option to disable this restriction.)

If you want the type-safety, you have to pay for it.  Otherwise,  
declare variables and parameters as integral types instead.  I don't  
care one way or the other -- I just want git to work on Lamp, and if  
there's community interest in C++ compatibility then I can assist  
with that.

Josh

P.S.  Every patch in my git repository is Signed-off-by: Joshua Juran  
<jjuran@gmail.com> (or s/gmail/metamage/).

Re: Mac OS 9 (Lamp) port

[Jonathan Nieder]

Joshua Juran wrote:

> diff --git a/server-info.c b/server-info.c
> index 4098ca2..a9a0188 100644
> --- a/server-info.c
> +++ b/server-info.c
> @@ -132,8 +132,8 @@ static int read_pack_info_file(const char *infofile)
> 
>  static int compare_info(const void *a_, const void *b_)
>  {
> -       struct pack_info *const *a = a_;
> -       struct pack_info *const *b = b_;
> +       const struct pack_info *const *a = a_;
> +       const struct pack_info *const *b = b_;
> 
>         if (0 <= (*a)->old_num && 0 <= (*b)->old_num)
>                 /* Keep the order in the original */
> 
> I certainly won't defend Metrowerks C 2.4.1, but the patch is
> harmless enough.

Yes, it is even a good change.

It just emphasized for me that Metrowerks is a little confused
about type qualifiers.

> 	#define alloc_n(type, n)  ((type*)xmalloc(sizeof (type) * n))
> 
> to factor the multiplication out of the calling code.

Maybe, imitating the signature of calloc:

 #define malloc_n(n, type) ((type *)xmalloc((n) * sizeof(type)))

For structs with variable-length arrays at the end:

 #define malloc_plus(type, extra) ((type *)xmalloc(sizeof(type) + (extra)))

That should cover most of the calls in git.

> I just want git to work on Lamp, and if
> there's community interest in C++ compatibility then I can assist
> with that.

There probably is not much direct interest.  But if in the process we
gain a little type-safety and portability, that would be nice. :)

Thanks for your hard work,
Jonathan

Re: [PATCH v2] compat: Add another rudimentary poll() emulation

[Albert Dvornik]

On Sat, May 29, 2010 at 8:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
[,,,]
> +static int msleep(int timeout)
> +{
> +       struct timeval tv;
> +       tv.tv_sec = 0;
> +       tv.tv_usec = 1000 * timeout;
> +       return select(0, NULL, NULL, NULL, &tv);
> +}

This code will do the right thing only when timeout is < 1000.  (This
is probably true for us in practice, so this is likely just
nitpicking.)  For the general case, you'd want
       tv.tv_sec = timeout / 1000;
       tv.tv_usec = 1000 * (timeout % 1000);

--bert

Re: [PATCH v2] compat: Add another rudimentary poll() emulation

[Jonathan Nieder]

Albert Dvornik wrote:

> For the general case, you'd want
>        tv.tv_sec = timeout / 1000;
>        tv.tv_usec = 1000 * (timeout % 1000);

Thanks for the catch.  Actually, it is not so unlikely that someone
would ask the autocorrect to wait longer than a second.

On Linux this is not an issue, but maybe Interix cares.  Posix is
vague and only says "the timeout period is given in seconds and
microseconds", staying silent on what the range of valid values is.

Jonathan

Re: [PATCH v2] compat: Add another rudimentary poll() emulation

[Albert Dvornik]

On Mon, May 31, 2010 at 8:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Albert Dvornik wrote:
>
>> For the general case, you'd want
>>        tv.tv_sec = timeout / 1000;
>>        tv.tv_usec = 1000 * (timeout % 1000);
>
> Thanks for the catch.  Actually, it is not so unlikely that someone
> would ask the autocorrect to wait longer than a second.

Good point, I take back what I said. =)

> On Linux this is not an issue, but maybe Interix cares.  Posix is
> vague and only says "the timeout period is given in seconds and
> microseconds", staying silent on what the range of valid values is.

Some traditional UNIXes are actually picky about the usec range, or at
least used to be (I seem to recall BSD and/or Solaris, but I really
don't remember for sure, this was a long time ago).

--bert

[PATCH 2/3] Support building without inttypes.h

[Jonathan Callen]

Some systems, such as Interix, do not have a inttypes.h header.
Attempt to use stdint.h instead.

Signed-off-by: Jonathan Callen <abcd@gentoo.org>
---
 Makefile          |    6 ++++++
 git-compat-util.h |    4 ++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 4b36534..1722bf0 100644
--- a/Makefile
+++ b/Makefile
@@ -70,6 +70,8 @@ all::
 #
 # Define NO_SYS_SELECT_H if you don't have sys/select.h.
 #
+# Define NO_INTTYPES_H if you don't have inttypes.h.
+#
 # Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.
 # Enable it on Windows.  By default, symrefs are still used.
 #
@@ -1333,6 +1335,10 @@ ifdef NO_POLL
 	BASIC_CFLAGS += -DNO_POLL
 endif
 
+ifdef NO_INTTYPES_H
+	BASIC_CFLAGS += -DNO_INTTYPES_H
+endif
+
 ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif
diff --git a/git-compat-util.h b/git-compat-util.h
index c5188e5..6f2aaca 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -108,7 +108,11 @@
 #include <arpa/inet.h>
 #include <netdb.h>
 #include <pwd.h>
+#ifdef NO_INTTYPES_H
+#include <stdint.h>
+#else
 #include <inttypes.h>
+#endif
 #if defined(__CYGWIN__)
 #undef _XOPEN_SOURCE
 #include <grp.h>
-- 
1.7.1

[PATCH 3/3] Add Interix support

[Jonathan Callen]

Add an Interix clause in the makefile.  Interix's libc does not have
support for many of the functions that are generally taken for granted.

Signed-off-by: Jonathan Callen <abcd@gentoo.org>
---
 Makefile |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 1722bf0..563eec2 100644
--- a/Makefile
+++ b/Makefile
@@ -1071,6 +1071,19 @@ else
 	NO_CURL = YesPlease
 endif
 endif
+ifeq ($(uname_S),Interix)
+	NO_IPV6 = YesPlease
+	NO_MEMMEM = YesPlease
+	NO_MKDTEMP = YesPlease
+	NO_STRTOUMAX = YesPlease
+	NO_STRTOULL = YesPlease
+	NO_INET_NTOP = YesPlease
+	NO_INET_PTON = YesPlease
+	NO_NSEC = YesPlease
+	NO_MKSTEMPS = YesPlease
+	NO_POLL = YesPlease
+	NO_INTTYPES_H = YesPlease
+endif
 
 -include config.mak.autogen
 -include config.mak
-- 
1.7.1

Re: [PATCH 0/3] Interix support

[Jakub Narebski]

Jonathan Callen <abcd@gentoo.org> writes:

> This series of patches adds support for building git on Interix.
> 
> Interix is an interesting system, as it lacks some functions that are
> normally present on POSIX systems, such as poll().  As I did not have
> time to implement poll() on top of select(), these patches simply
> disable the three commands that require poll() to be present:
> git-daemon, git-upload-archive, and git-upload-pack.
> 
> Jonathan Callen (3):
>       Support building on systems without poll(2)
>       Support building without inttypes.h
>       Add Interix support
> 
>  Makefile          |   40 +++++++++++++++++++++++++++++++++++-----
>  builtin.h         |    2 ++
>  git-compat-util.h |    6 ++++++
>  git.c             |    2 ++
>  4 files changed, 45 insertions(+), 5 deletions(-)

Could you please, if you can, add support for detecting existence of
poll(2) and existence of inttypes.h to configure.ac, to make it
automatically detected by

  $ make configure
  $ ./configure [options]

Thanks in advance.

-- 
Jakub Narebski
Poland
ShadeHawk on #git