[PATCH 00/40] MinGW port

[PATCH 00/40] MinGW port

[Johannes Sixt]

So here it is, the MinGW port of git.
This series is also available from

git://repo.or.cz/git/mingw/j6t.git upstream
http://repo.or.cz/w/git/mingw/j6t.git?a=shortlog;h=upstream

Junio, please do *not* pull/apply this series into your tree(s) until we
have verified that forks on repo.or.cz do not lose objects anymore when
branches are rewound.


What this series achieves:

- An almost complete set of git tools running natively on Windows.

- Functional git-gui and gitk.


What is missing:

- perl scripts don't work. We have a hack in the port tree, but I haven't
  yet taken the time to clean it up enough.

- The test suite does not pass because of missing features like symbolic
  links. Again we have adjusted the tests in the port tree so that the
  test suite can be completed unattended.


I've arranged the patches in a few subsets. They are different from what
Dscho had proposed some weeks ago when I announced the series for the
first time because I felt this topical partitioning makes more sense.


* Part 1: Get it going
01/40 Add compat/regex.[ch] and compat/fnmatch.[ch].
02/40 Compile some programs only conditionally.
03/40 Add target architecture MinGW.

With these patches we have a working git.exe that can successfully run
those builtins that need only read-access of the repository, like
git log, git diff, etc.

 7 files changed, 6222 insertions(+), 5 deletions(-)


* Part 2: Working locally is possible
04/40 Windows: Use the Windows style PATH separator ';'.
05/40 Windows: Strip ".exe" from the program name.
06/40 Windows: Implement a wrapper of the open() function.
07/40 Windows: A minimal implemention of getpwuid().
08/40 Windows: always chmod(, 0666) before unlink().
09/40 Windows: Work around misbehaved rename().
10/40 Windows: Treat Windows style path names.
11/40 Windows: Handle absolute paths in safe_create_leading_directories().
12/40 Windows: Implement gettimeofday().
13/40 Windows: Fix PRIuMAX definition.
14/40 Windows: Implement setitimer() and sigaction().
15/40 Windows: A work-around for a misbehaved vsnprintf.

Now builtins that write to the repository work as long as they don't need
to spawn secondary processes.

 10 files changed, 347 insertions(+), 20 deletions(-)


* Part 3: Shell scripts and start_command()
16/40 Windows: Wrap execve so that shell scripts can be invoked.
17/40 Windows: A pipe() replacement whose ends are not inherited to children.
18/40 Windows: Implement start_command().
19/40 Windows: Change the name of hook scripts to make them not executable.

Now scripted tools and the builtins and standalone programs work that
depend on start_command().

 5 files changed, 391 insertions(+), 16 deletions(-)


* Part 4: Remote connections
20/40 Windows: A rudimentary poll() emulation.
21/40 Windows: Disambiguate DOS style paths from SSH URLs.
22/40 Windows: Implement asynchronous functions as threads.
23/40 Windows: Local clone must use the drive letter in absolute paths.
24/40 Windows: Work around incompatible sort and find.
25/40 Windows: Implement a cpio emulation in git-clone.sh.
26/40 Windows: Implement wrappers for gethostbyname(), socket(), and connect().

Remote connections, both push and fetch, using local transport as well as
connections via ssh and native git protocol work.

 9 files changed, 233 insertions(+), 3 deletions(-)


* Part 5: Optimizations
27/40 Windows: Implement a custom spawnve().
28/40 Windows: Add a new lstat and fstat implementation based on Win32 API.
29/40 Windows: Use a customized struct stat that also has the st_blocks member.

 3 files changed, 370 insertions(+), 10 deletions(-)


* Part 6: Tie up loose ends
30/40 Turn builtin_exec_path into a function.
31/40 Compute the ultimate fallback for exec_path from the program invocation.
32/40 Windows: Use a relative default template_dir and ETC_GITCONFIG
33/40 When installing, be prepared that template_dir may be relative.
34/40 Windows: Make the pager work.
35/40 Windows: Work around an oddity when a pipe with no reader is written to.
36/40 Avoid the "dup dance" in wt_status_print_verbose() when possible.
37/40 Windows: Make 'git help -a' work.
38/40 Windows: TMP and TEMP environment variables specify a temporary directory.
39/40 Windows: Fix ntohl() related warnings about printf formatting
40/40 compat/pread.c: Add foward decl to fix warning

This fixes some final remaining oddities, minor missing features, and
works around differences between POSIX and Windows.

 11 files changed, 154 insertions(+), 21 deletions(-)


* Diffstat excluding 01/40 (which adds 5989 lines)

 Documentation/git.txt |    6 +-
 Makefile              |   48 +++-
 cache.h               |    4 +
 compat/mingw.c        | 1018 +++++++++++++++++++++++++++++++++++++++++++++++++
 connect.c             |    8 +-
 date.c                |    2 +-
 exec_cmd.c            |   43 ++-
 git-clone.sh          |   52 +++-
 git-compat-util.h     |  221 +++++++++++-
 git-sh-setup.sh       |   17 +
 git.c                 |   14 +
 gitk-git/Makefile     |    4 +
 gitk-git/gitk         |    1 +
 help.c                |   38 ++-
 pager.c               |   40 ++-
 path.c                |    7 +
 run-command.c         |  118 ++++++-
 run-command.h         |    5 +
 setup.c               |   59 +++-
 sha1_file.c           |   18 +-
 t/test-lib.sh         |   13 +
 templates/Makefile    |   16 +-
 upload-pack.c         |    2 +
 write_or_die.c        |    7 +-
 wt-status.c           |   12 +-
 25 files changed, 1713 insertions(+), 60 deletions(-)

-- Hannes

[PATCH 01/40] Add compat/regex.[ch] and compat/fnmatch.[ch].

[Johannes Sixt]

We don't have fnmatch and regular expressions on Windows. We borrow
fnmatch.[ch] from the GNU C library (license is LGPL 2 or later) and
GNU regexp (regexp.c[ch], license is GPL 2 or later). Note that regexp.c
was changed slightly to avoid warnings with gcc.

We make the addition of these files an extra commit so as not to clutter
the next commits.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 This patch is intentially broken.  There's no need to comment on the
 details of the files that it adds.

 -- Hannes

 compat/fnmatch.c |  488 ++++++
 compat/fnmatch.h |   84 +
 compat/regex.c   | 4927 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat/regex.h   |  490 ++++++
 4 files changed, 5989 insertions(+), 0 deletions(-)
 create mode 100644 compat/fnmatch.c
 create mode 100644 compat/fnmatch.h
 create mode 100644 compat/regex.c
 create mode 100644 compat/regex.h

diff --git a/compat/fnmatch.c b/compat/fnmatch.c
new file mode 100644
index 0000000..1f4ead5
--- /dev/null
+++ b/compat/fnmatch.c
@@ -0,0 +1,488 @@
This patch is intentionally broken.
diff --git a/compat/fnmatch.h b/compat/fnmatch.h
new file mode 100644
index 0000000..cc3ec37
--- /dev/null
+++ b/compat/fnmatch.h
@@ -0,0 +1,84 @@
This patch is intentionally broken.
diff --git a/compat/regex.c b/compat/regex.c
new file mode 100644
index 0000000..1d39e08
--- /dev/null
+++ b/compat/regex.c
@@ -0,0 +1,4927 @@
This patch is intentionally broken.
diff --git a/compat/regex.h b/compat/regex.h
new file mode 100644
index 0000000..408dd21
--- /dev/null
+++ b/compat/regex.h
@@ -0,0 +1,490 @@
This patch is intentionally broken.
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 01/40] Add compat/regex.[ch] and compat/fnmatch.[ch].

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

>  This patch is intentially broken.  There's no need to comment on the
>  details of the files that it adds.

Heh.

You mean to say: "all bugs you find here are _not_ ours!" ;-)

Ciao,
Dscho

[PATCH 02/40] Compile some programs only conditionally.

[Johannes Sixt]

On MinGW, we won't compile some programs.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d33a556..870a862 100644
--- a/Makefile
+++ b/Makefile
@@ -255,7 +255,6 @@ PROGRAMS = \
 	git-fetch-pack$X \
 	git-hash-object$X git-index-pack$X \
 	git-fast-import$X \
-	git-daemon$X \
 	git-merge-index$X git-mktag$X git-mktree$X git-patch-id$X \
 	git-receive-pack$X \
 	git-send-pack$X git-shell$X \
@@ -264,7 +263,7 @@ PROGRAMS = \
 	git-update-server-info$X \
 	git-upload-pack$X \
 	git-pack-redundant$X git-var$X \
-	git-merge-tree$X git-imap-send$X \
+	git-merge-tree$X \
 	git-merge-recursive$X \
 	$(EXTRA_PROGRAMS)
 
@@ -583,6 +582,11 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz
 
+ifndef NO_EXTRA_PROGRAMS
+	EXTRA_PROGRAMS += \
+		git-daemon$X \
+		git-imap-send$X
+endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 02/40] Compile some programs only conditionally.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> On MinGW, we won't compile some programs.

This explanation is good (even if I recall that we compile git-daemon.exe 
just fine now).

> @@ -264,7 +263,7 @@ PROGRAMS = \
>  	git-update-server-info$X \
>  	git-upload-pack$X \
>  	git-pack-redundant$X git-var$X \
> -	git-merge-tree$X git-imap-send$X \
> +	git-merge-tree$X \

We do not compile git-imap-send because MinGW lacks socketpair() and 
getpass().  Why not say it explicitely, and have a NO_SOCKETPAIR variable?  
Likewise, for git-daemon we should say NO_SYSLOG.

> +ifndef NO_EXTRA_PROGRAMS

The name NO_EXTRA_PROGRAMS is definitely not illustrating the reasons why 
we exclude those programs, so I'd rather not have this patch as-is.

NOTE: I think that both programs are fixable, but only git-daemon is 
important enough that I would like to see patch of the "daemon" branch in 
4msysgit.git cherry picked into this series (maybe we should implement a 
compat/syslog.c using the ReportEvent() function of the win32 API?).

Ciao,
Dscho

Re: [PATCH 02/40] Compile some programs only conditionally.

[Johannes Sixt]

On Thursday 28 February 2008 12:57, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > On MinGW, we won't compile some programs.
>
> This explanation is good (even if I recall that we compile git-daemon.exe
> just fine now).
>
> > @@ -264,7 +263,7 @@ PROGRAMS = \
> >  	git-update-server-info$X \
> >  	git-upload-pack$X \
> >  	git-pack-redundant$X git-var$X \
> > -	git-merge-tree$X git-imap-send$X \
> > +	git-merge-tree$X \
>
> We do not compile git-imap-send because MinGW lacks socketpair() and
> getpass().  Why not say it explicitely, and have a NO_SOCKETPAIR variable?
> Likewise, for git-daemon we should say NO_SYSLOG.

Where are NO_WNOHANG, NO_GETPASS, NO_SELECT, NO_SIGCHLD, NO_EXECVE etc in your 
list? A condition on MINGW32 is certainly sufficient.

> > +ifndef NO_EXTRA_PROGRAMS
>
> The name NO_EXTRA_PROGRAMS is definitely not illustrating the reasons why
> we exclude those programs, so I'd rather not have this patch as-is.

I agree.

> NOTE: I think that both programs are fixable, but only git-daemon is
> important enough that I would like to see patch of the "daemon" branch in
> 4msysgit.git cherry picked into this series (maybe we should implement a
> compat/syslog.c using the ReportEvent() function of the win32 API?).

openlog() etc. is only one problem in git-daemon. It also depends on SIGCHLD, 
a non-blocking waitpid, and a lot more. The patch that is in 4msysgit.git 
allows only a single connection, IIRC, after which it terminates. But I also 
think that git-daemon can be made more complete on Windows, but it certainly 
requires a major surgery.

-- Hannes

Re: [PATCH 02/40] Compile some programs only conditionally.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 12:57, Johannes Schindelin wrote:
>
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > > On MinGW, we won't compile some programs.
> >
> > This explanation is good (even if I recall that we compile 
> > git-daemon.exe just fine now).
> >
> > > @@ -264,7 +263,7 @@ PROGRAMS = \
> > >  	git-update-server-info$X \
> > >  	git-upload-pack$X \
> > >  	git-pack-redundant$X git-var$X \
> > > -	git-merge-tree$X git-imap-send$X \
> > > +	git-merge-tree$X \
> >
> > We do not compile git-imap-send because MinGW lacks socketpair() and 
> > getpass().  Why not say it explicitely, and have a NO_SOCKETPAIR 
> > variable? Likewise, for git-daemon we should say NO_SYSLOG.
> 
> Where are NO_WNOHANG, NO_GETPASS, NO_SELECT, NO_SIGCHLD, NO_EXECVE etc 
> in your list? A condition on MINGW32 is certainly sufficient.

Just pick the most difficult one.  But just saying "this does not work on 
MinGW32" does not help people who want to help.

> > NOTE: I think that both programs are fixable, but only git-daemon is 
> > important enough that I would like to see patch of the "daemon" branch 
> > in 4msysgit.git cherry picked into this series (maybe we should 
> > implement a compat/syslog.c using the ReportEvent() function of the 
> > win32 API?).
> 
> openlog() etc. is only one problem in git-daemon. It also depends on 
> SIGCHLD, a non-blocking waitpid, and a lot more. The patch that is in 
> 4msysgit.git allows only a single connection, IIRC, after which it 
> terminates. But I also think that git-daemon can be made more complete 
> on Windows, but it certainly requires a major surgery.

Okay, I did not look closely enough.  But even a single-connection daemon 
is better than no daemon, no?

Ciao,
Dscho

Re: [PATCH 02/40] Compile some programs only conditionally.

[Johannes Sixt]

On Friday 29 February 2008 01:47, Johannes Schindelin wrote:
> On Thu, 28 Feb 2008, Johannes Sixt wrote:
> > openlog() etc. is only one problem in git-daemon. It also depends on
> > SIGCHLD, a non-blocking waitpid, and a lot more. The patch that is in
> > 4msysgit.git allows only a single connection, IIRC, after which it
> > terminates. But I also think that git-daemon can be made more complete
> > on Windows, but it certainly requires a major surgery.
>
> Okay, I did not look closely enough.  But even a single-connection daemon
> is better than no daemon, no?

In principle, yes. But:

- git-fetch needs two connections if there are new tags in the source repo.

- The daemon patches in 4msysgit.git could be prettier, they certainly would 
not pass your scrutiny.

git-daemon is not my itch at this time, so I won't scratch it.

-- Hannes

Re: [PATCH 02/40] Compile some programs only conditionally.

[Johannes Schindelin]

Hi,

On Fri, 29 Feb 2008, Johannes Sixt wrote:

> On Friday 29 February 2008 01:47, Johannes Schindelin wrote:
> > On Thu, 28 Feb 2008, Johannes Sixt wrote:
> > > openlog() etc. is only one problem in git-daemon. It also depends on
> > > SIGCHLD, a non-blocking waitpid, and a lot more. The patch that is in
> > > 4msysgit.git allows only a single connection, IIRC, after which it
> > > terminates. But I also think that git-daemon can be made more complete
> > > on Windows, but it certainly requires a major surgery.
> >
> > Okay, I did not look closely enough.  But even a single-connection daemon
> > is better than no daemon, no?
> 
> In principle, yes. But:
> 
> - git-fetch needs two connections if there are new tags in the source repo.
> 
> - The daemon patches in 4msysgit.git could be prettier, they certainly would 
> not pass your scrutiny.

Right.

> git-daemon is not my itch at this time, so I won't scratch it.

Right.  I will mail the correct person.

Ciao,
Dscho

[PATCH 03/40] Add target architecture MinGW.

[Johannes Sixt]

With this change GIT can be compiled and linked using MinGW. Builtins
that only read the repository such as the log family and grep already
work.

Simple stubs are provided for a number of functions that the Windows C
runtime does not offer. They will be completed in later patches.

Dmitry Kakurin pointed out that access(..., X_OK) would always fails on
Vista and suggested the -D__USE_MINGW_ACCESS workaround.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile          |   25 +++++++++
 compat/mingw.c    |   57 ++++++++++++++++++++
 git-compat-util.h |  148 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 227 insertions(+), 3 deletions(-)
 create mode 100644 compat/mingw.c

diff --git a/Makefile b/Makefile
index 870a862..bc8a487 100644
--- a/Makefile
+++ b/Makefile
@@ -522,6 +522,31 @@ ifeq ($(uname_S),HP-UX)
 	NO_HSTRERROR = YesPlease
 	NO_SYS_SELECT_H = YesPlease
 endif
+ifneq (,$(findstring MINGW,$(uname_S)))
+	NO_MMAP = YesPlease
+	NO_PREAD = YesPlease
+	NO_OPENSSL = YesPlease
+	NO_CURL = YesPlease
+	NO_SYMLINK_HEAD = YesPlease
+	NO_IPV6 = YesPlease
+	NO_SETENV = YesPlease
+	NO_UNSETENV = YesPlease
+	NO_STRCASESTR = YesPlease
+	NO_STRLCPY = YesPlease
+	NO_MEMMEM = YesPlease
+	NEEDS_LIBICONV = YesPlease
+	OLD_ICONV = YesPlease
+	NO_C99_FORMAT = YesPlease
+	NO_STRTOUMAX = YesPlease
+	NO_MKDTEMP = YesPlease
+	NO_SVN_TESTS = YesPlease
+	NO_PERL_MAKEMAKER = YesPlease
+	NO_EXTRA_PROGRAMS = YesPlease
+	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
+	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o
+	EXTLIBS += -lws2_32
+	X = .exe
+endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
 endif
diff --git a/compat/mingw.c b/compat/mingw.c
new file mode 100644
index 0000000..0c87e43
--- /dev/null
+++ b/compat/mingw.c
@@ -0,0 +1,57 @@
+#include "../git-compat-util.h"
+
+unsigned int _CRT_fmode = _O_BINARY;
+
+unsigned int sleep (unsigned int seconds)
+{
+	Sleep(seconds*1000);
+	return 0;
+}
+
+int mkstemp(char *template)
+{
+	char *filename = mktemp(template);
+	if (filename == NULL)
+		return -1;
+	return open(filename, O_RDWR | O_CREAT, 0600);
+}
+
+int gettimeofday(struct timeval *tv, void *tz)
+{
+	return 0;
+}
+
+int poll(struct pollfd *ufds, unsigned int nfds, int timeout)
+{
+	return 0;
+}
+
+struct tm *gmtime_r(const time_t *timep, struct tm *result)
+{
+	/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */
+	memcpy(result, gmtime(timep), sizeof(struct tm));
+	return result;
+}
+
+struct tm *localtime_r(const time_t *timep, struct tm *result)
+{
+	/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */
+	memcpy(result, localtime(timep), sizeof(struct tm));
+	return result;
+}
+
+struct passwd *getpwuid(int uid)
+{
+	static struct passwd p;
+	return &p;
+}
+
+int setitimer(int type, struct itimerval *in, struct itimerval *out)
+{
+	return 0;
+}
+
+int sigaction(int sig, struct sigaction *in, struct sigaction *out)
+{
+	return 0;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 2a40703..b1f3f92 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -63,16 +63,17 @@
 #include <sys/time.h>
 #include <time.h>
 #include <signal.h>
-#include <sys/wait.h>
 #include <fnmatch.h>
+#include <assert.h>
+#include <regex.h>
+#ifndef __MINGW32__
+#include <sys/wait.h>
 #include <sys/poll.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #ifndef NO_SYS_SELECT_H
 #include <sys/select.h>
 #endif
-#include <assert.h>
-#include <regex.h>
 #include <netinet/in.h>
 #include <netinet/tcp.h>
 #include <arpa/inet.h>
@@ -88,6 +89,7 @@
 #include <grp.h>
 #define _ALL_SOURCE 1
 #endif
+#endif	/* !__MINGW32__ */
 
 #ifndef NO_ICONV
 #include <iconv.h>
@@ -357,6 +359,10 @@ static inline FILE *xfdopen(int fd, const char *mode)
 	return stream;
 }
 
+#ifdef __MINGW32__
+int mkstemp(char *template);
+#endif
+
 static inline int xmkstemp(char *template)
 {
 	int fd;
@@ -437,4 +443,140 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 #define qsort git_qsort
 #endif
 
+#ifdef __MINGW32__
+
+#include <winsock2.h>
+
+/*
+ * things that are not available in header files
+ */
+
+typedef int pid_t;
+#define hstrerror strerror
+
+#define S_IFLNK    0120000 /* Symbolic link */
+#define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
+#define S_ISSOCK(x) 0
+#define S_IRGRP 0
+#define S_IWGRP 0
+#define S_IXGRP 0
+#define S_ISGID 0
+#define S_IROTH 0
+#define S_IXOTH 0
+
+#define WIFEXITED(x) ((unsigned)(x) < 259)	/* STILL_ACTIVE */
+#define WEXITSTATUS(x) ((x) & 0xff)
+#define WIFSIGNALED(x) ((unsigned)(x) > 259)
+
+#define SIGKILL 0
+#define SIGCHLD 0
+#define SIGPIPE 0
+#define SIGALRM 100
+
+#define F_GETFD 1
+#define F_SETFD 2
+#define FD_CLOEXEC 0x1
+
+struct passwd {
+	char *pw_name;
+	char *pw_gecos;
+	char *pw_dir;
+};
+
+struct pollfd {
+	int fd;           /* file descriptor */
+	short events;     /* requested events */
+	short revents;    /* returned events */
+};
+#define POLLIN 1
+#define POLLHUP 2
+
+typedef void (__cdecl *sig_handler_t)(int);
+struct sigaction {
+	sig_handler_t sa_handler;
+	unsigned sa_flags;
+};
+#define sigemptyset(x) (void)0
+#define SA_RESTART 0
+
+struct itimerval {
+	struct timeval it_value, it_interval;
+};
+#define ITIMER_REAL 0
+
+#define st_blocks st_size/512	/* will be cleaned up later */
+#define lstat stat
+
+/*
+ * trivial stubs
+ */
+
+static inline int readlink(const char *path, char *buf, size_t bufsiz)
+{ errno = ENOSYS; return -1; }
+static inline int symlink(const char *oldpath, const char *newpath)
+{ errno = ENOSYS; return -1; }
+static inline int link(const char *oldpath, const char *newpath)
+{ errno = ENOSYS; return -1; }
+static inline int fchmod(int fildes, mode_t mode)
+{ errno = ENOSYS; return -1; }
+static inline int fork(void)
+{ errno = ENOSYS; return -1; }
+static inline unsigned int alarm(unsigned int seconds)
+{ return 0; }
+static inline int fsync(int fd)
+{ return 0; }
+static inline int getppid(void)
+{ return 1; }
+static inline void sync(void)
+{}
+static inline int getuid()
+{ return 1; }
+static inline struct passwd *getpwnam(const char *name)
+{ return NULL; }
+static inline int fcntl(int fd, int cmd, long arg)
+{
+	if (cmd == F_GETFD || cmd == F_SETFD)
+		return 0;
+	errno = EINVAL;
+	return -1;
+}
+
+/*
+ * simple adaptors
+ */
+
+static inline int mingw_mkdir(const char *path, int mode)
+{
+	return mkdir(path);
+}
+#define mkdir mingw_mkdir
+
+static inline int waitpid(pid_t pid, unsigned *status, unsigned options)
+{
+	if (options == 0)
+		return _cwait(status, pid, 0);
+	errno = EINVAL;
+	return -1;
+}
+
+
+static inline int pipe(int filedes[2])
+{ return _pipe(filedes, 8192, 0); }
+
+/*
+ * implementations of missing functions
+ */
+
+unsigned int sleep (unsigned int seconds);
+int gettimeofday(struct timeval *tv, void *tz);
+int poll(struct pollfd *ufds, unsigned int nfds, int timeout);
+struct tm *gmtime_r(const time_t *timep, struct tm *result);
+struct tm *localtime_r(const time_t *timep, struct tm *result);
+int getpagesize(void);	/* defined in MinGW's libgcc.a */
+struct passwd *getpwuid(int uid);
+int setitimer(int type, struct itimerval *in, struct itimerval *out);
+int sigaction(int sig, struct sigaction *in, struct sigaction *out);
+
+#endif /* __MINGW32__ */
+
 #endif
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> +int gettimeofday(struct timeval *tv, void *tz)
> +{
> +	return 0;
> +}

Should it not return -1, for failure?  (I know, I know, probably a few 
programs do not work, then, but it is not correct that it succeeded.)

The same goes for a few other functions.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 2a40703..b1f3f92 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -357,6 +359,10 @@ static inline FILE *xfdopen(int fd, const char *mode)
>  	return stream;
>  }
>  
> +#ifdef __MINGW32__
> +int mkstemp(char *template);
> +#endif
> +
>  static inline int xmkstemp(char *template)
>  {
>  	int fd;

Could we have this...

> @@ -437,4 +443,140 @@ void git_qsort(void *base, size_t nmemb, size_t size,
>  #define qsort git_qsort
>  #endif
>  
> +#ifdef __MINGW32__
> +
> +#include <winsock2.h>
> +
> +/*
> + * things that are not available in header files
> + */
> +
> [...]

... and this in compat/mingw.h?  And then, we'd only have

#ifdef __MINGW32__
#include "mingw.h"
#endif

in git-compat-util.h?

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Paolo Bonzini]

Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> 
>> +int gettimeofday(struct timeval *tv, void *tz)
>> +{
>> +	return 0;
>> +}
> 
> Should it not return -1, for failure?  (I know, I know, probably a few 
> programs do not work, then, but it is not correct that it succeeded.)

FWIW, this is overwritten by later patches in the series anyway.

All the other suggestions that Dscho wrote about, make a lot of sense to 
me too.

Paolo

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Paolo Bonzini wrote:

> Johannes Schindelin wrote:
> 
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > 
> > > +int gettimeofday(struct timeval *tv, void *tz)
> > > +{
> > > +	return 0;
> > > +}
> > 
> > Should it not return -1, for failure?  (I know, I know, probably a few
> > programs do not work, then, but it is not correct that it succeeded.)
> 
> FWIW, this is overwritten by later patches in the series anyway.

Yes, I know that.  But the whole point of having a nice patch series is to 
have proper revisions, and IMO returning 0 here is not correct.

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Sixt]

On Thursday 28 February 2008 13:05, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > +int gettimeofday(struct timeval *tv, void *tz)
> > +{
> > +	return 0;
> > +}
>
> Should it not return -1, for failure?  (I know, I know, probably a few
> programs do not work, then, but it is not correct that it succeeded.)
>
> The same goes for a few other functions.

Agreed. The return value of these functions should not make a difference at 
this stage in the patch series anyway.

> > +#ifdef __MINGW32__
> > +int mkstemp(char *template);
> > +#endif
> > +
> >  static inline int xmkstemp(char *template)
> >  {
> >  	int fd;
>
> Could we have this...

No, becauser xmkstemp needs the forward declaration of mkstemp(). But we could 
make it unconditional.

> ... and this in compat/mingw.h?  And then, we'd only have
>
> #ifdef __MINGW32__
> #include "mingw.h"
> #endif
>
> in git-compat-util.h?

I thought about this, but I decided against it:  git-compat-util.h is the 
place to look for compatibility functions. A file compat/mingw.h only 
introduces an extra indirection and only *hides* stuff instead of making it 
obvious.

-- Hannes

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 13:05, Johannes Schindelin wrote:
> >
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > 
> > > +#ifdef __MINGW32__
> > > +int mkstemp(char *template);
> > > +#endif
> > > +
> > >  static inline int xmkstemp(char *template)
> > >  {
> > >  	int fd;
> >
> > Could we have this...
> 
> No, becauser xmkstemp needs the forward declaration of mkstemp(). But we 
> could make it unconditional.

Actually, I was thinking of including it at the same spot where you 
declare mkstemp conditionally.  (Of course, since I spotted very well that 
you need that declaration for xmkstemp().)

> I thought about this, but I decided against it:  git-compat-util.h is 
> the place to look for compatibility functions. A file compat/mingw.h 
> only introduces an extra indirection and only *hides* stuff instead of 
> making it obvious.

Well, the thing is, there are quite a few definitions and declarations 
that are _only_ _ever_ interesting if you are on MinGW32.

So maybe it is a good idea to hide it to all the other users.

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Sixt]

On Friday 29 February 2008 02:07, Johannes Schindelin wrote:
> On Thu, 28 Feb 2008, Johannes Sixt wrote:
> > I thought about this, but I decided against it:  git-compat-util.h is
> > the place to look for compatibility functions. A file compat/mingw.h
> > only introduces an extra indirection and only *hides* stuff instead of
> > making it obvious.
>
> Well, the thing is, there are quite a few definitions and declarations
> that are _only_ _ever_ interesting if you are on MinGW32.
>
> So maybe it is a good idea to hide it to all the other users.

You get compat/mingw.h, but you don't get a zillion NO_<insert feature here>. 
The two are pretty much exclusive anyway. Deal?

-- Hannes

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Fri, 29 Feb 2008, Johannes Sixt wrote:

> On Friday 29 February 2008 02:07, Johannes Schindelin wrote:
> > On Thu, 28 Feb 2008, Johannes Sixt wrote:
> > > I thought about this, but I decided against it:  git-compat-util.h is
> > > the place to look for compatibility functions. A file compat/mingw.h
> > > only introduces an extra indirection and only *hides* stuff instead of
> > > making it obvious.
> >
> > Well, the thing is, there are quite a few definitions and declarations
> > that are _only_ _ever_ interesting if you are on MinGW32.
> >
> > So maybe it is a good idea to hide it to all the other users.
> 
> You get compat/mingw.h, but you don't get a zillion NO_<insert feature here>. 
> The two are pretty much exclusive anyway. Deal?

Deal.

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Sixt]

On Thursday 28 February 2008 13:05, Johannes Schindelin wrote:
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -357,6 +359,10 @@ static inline FILE *xfdopen(int fd, const char
> > *mode) return stream;
> >  }
> >
> > +#ifdef __MINGW32__
> > +int mkstemp(char *template);
> > +#endif
> > +
> >  static inline int xmkstemp(char *template)
> >  {
> >  	int fd;
>
> Could we have this...
>
> > @@ -437,4 +443,140 @@ void git_qsort(void *base, size_t nmemb, size_t
> > size, #define qsort git_qsort
> >  #endif
> >
> > +#ifdef __MINGW32__
> > +
> > +#include <winsock2.h>
> > +
> > +/*
> > + * things that are not available in header files
> > + */
> > +
> > [...]
>
> ... and this in compat/mingw.h?  And then, we'd only have
>
> #ifdef __MINGW32__
> #include "mingw.h"
> #endif
>
> in git-compat-util.h?

I've now created compat/mingw.h. Below is the interdiff. Of course, it mostly 
only moves code around. But you might want to look at at mkstemp, PATH_SEP, 
PRIuMAX, and has_dos_drive_prefix. Notice also that I include compat/mingw.h 
early in git-compat-util.h instead of late.

STRIP_EXTENSION could also be moved into compat/mingw.h, but I decided not to 
do so because I think that macros that code outside of git-compat-util.h 
checks for using #ifdef should be found in Makefile.

-- Hannes

diff --git a/Makefile b/Makefile
index 88a52bf..af74566 100644
--- a/Makefile
+++ b/Makefile
@@ -548,9 +548,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_PERL_MAKEMAKER = YesPlease
 	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
-	COMPAT_CFLAGS += -DPATH_SEP="';'"
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-	COMPAT_CFLAGS += -DPRIuMAX=\"I64u\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o
 	EXTLIBS += -lws2_32
 	X = .exe
diff --git a/compat/mingw.h b/compat/mingw.h
new file mode 100644
index 0000000..d92c631
--- /dev/null
+++ b/compat/mingw.h
@@ -0,0 +1,210 @@
+#include <winsock2.h>
+
+/*
+ * things that are not available in header files
+ */
+
+typedef int pid_t;
+#define hstrerror strerror
+
+#define S_IFLNK    0120000 /* Symbolic link */
+#define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
+#define S_ISSOCK(x) 0
+#define S_IRGRP 0
+#define S_IWGRP 0
+#define S_IXGRP 0
+#define S_ISGID 0
+#define S_IROTH 0
+#define S_IXOTH 0
+
+#define WIFEXITED(x) ((unsigned)(x) < 259)	/* STILL_ACTIVE */
+#define WEXITSTATUS(x) ((x) & 0xff)
+#define WIFSIGNALED(x) ((unsigned)(x) > 259)
+
+#define SIGKILL 0
+#define SIGCHLD 0
+#define SIGPIPE 0
+#define SIGALRM 100
+
+#define F_GETFD 1
+#define F_SETFD 2
+#define FD_CLOEXEC 0x1
+
+struct passwd {
+	char *pw_name;
+	char *pw_gecos;
+	char *pw_dir;
+};
+
+struct pollfd {
+	int fd;           /* file descriptor */
+	short events;     /* requested events */
+	short revents;    /* returned events */
+};
+#define POLLIN 1
+#define POLLHUP 2
+
+typedef void (__cdecl *sig_handler_t)(int);
+struct sigaction {
+	sig_handler_t sa_handler;
+	unsigned sa_flags;
+};
+#define sigemptyset(x) (void)0
+#define SA_RESTART 0
+
+struct itimerval {
+	struct timeval it_value, it_interval;
+};
+#define ITIMER_REAL 0
+
+/*
+ * trivial stubs
+ */
+
+static inline int readlink(const char *path, char *buf, size_t bufsiz)
+{ errno = ENOSYS; return -1; }
+static inline int symlink(const char *oldpath, const char *newpath)
+{ errno = ENOSYS; return -1; }
+static inline int link(const char *oldpath, const char *newpath)
+{ errno = ENOSYS; return -1; }
+static inline int fchmod(int fildes, mode_t mode)
+{ errno = ENOSYS; return -1; }
+static inline int fork(void)
+{ errno = ENOSYS; return -1; }
+static inline unsigned int alarm(unsigned int seconds)
+{ return 0; }
+static inline int fsync(int fd)
+{ return 0; }
+static inline int getppid(void)
+{ return 1; }
+static inline void sync(void)
+{}
+static inline int getuid()
+{ return 1; }
+static inline struct passwd *getpwnam(const char *name)
+{ return NULL; }
+static inline int fcntl(int fd, int cmd, long arg)
+{
+	if (cmd == F_GETFD || cmd == F_SETFD)
+		return 0;
+	errno = EINVAL;
+	return -1;
+}
+
+/*
+ * simple adaptors
+ */
+
+static inline int mingw_mkdir(const char *path, int mode)
+{
+	return mkdir(path);
+}
+#define mkdir mingw_mkdir
+
+static inline int mingw_unlink(const char *pathname)
+{
+	/* read-only files cannot be removed */
+	chmod(pathname, 0666);
+	return unlink(pathname);
+}
+#define unlink mingw_unlink
+
+static inline int waitpid(pid_t pid, unsigned *status, unsigned options)
+{
+	if (options == 0)
+		return _cwait(status, pid, 0);
+	errno = EINVAL;
+	return -1;
+}
+
+/*
+ * implementations of missing functions
+ */
+
+int pipe(int filedes[2]);
+unsigned int sleep (unsigned int seconds);
+int mkstemp(char *template);
+int gettimeofday(struct timeval *tv, void *tz);
+int poll(struct pollfd *ufds, unsigned int nfds, int timeout);
+struct tm *gmtime_r(const time_t *timep, struct tm *result);
+struct tm *localtime_r(const time_t *timep, struct tm *result);
+int getpagesize(void);	/* defined in MinGW's libgcc.a */
+struct passwd *getpwuid(int uid);
+int setitimer(int type, struct itimerval *in, struct itimerval *out);
+int sigaction(int sig, struct sigaction *in, struct sigaction *out);
+
+/*
+ * replacements of existing functions
+ */
+
+int mingw_open (const char *filename, int oflags, ...);
+#define open mingw_open
+
+char *mingw_getcwd(char *pointer, int len);
+#define getcwd mingw_getcwd
+
+struct hostent *mingw_gethostbyname(const char *host);
+#define gethostbyname mingw_gethostbyname
+
+int mingw_socket(int domain, int type, int protocol);
+#define socket mingw_socket
+
+int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz);
+#define connect mingw_connect
+
+int mingw_rename(const char*, const char*);
+#define rename mingw_rename
+
+/* Use mingw_lstat() instead of lstat()/stat() and
+ * mingw_fstat() instead of fstat() on Windows.
+ * struct stat is redefined because it lacks the st_blocks member.
+ */
+struct mingw_stat {
+	unsigned st_mode;
+	time_t st_mtime, st_atime, st_ctime;
+	unsigned st_dev, st_ino, st_uid, st_gid;
+	size_t st_size;
+	size_t st_blocks;
+};
+int mingw_lstat(const char *file_name, struct mingw_stat *buf);
+int mingw_fstat(int fd, struct mingw_stat *buf);
+#define fstat mingw_fstat
+#define lstat mingw_lstat
+#define stat mingw_stat
+static inline int mingw_stat(const char *file_name, struct mingw_stat *buf)
+{ return mingw_lstat(file_name, buf); }
+
+int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
+#define vsnprintf mingw_vsnprintf
+
+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
+void mingw_execvp(const char *cmd, char *const *argv);
+#define execvp mingw_execvp
+
+static inline unsigned int git_ntohl(unsigned int x)
+{ return (unsigned int)ntohl(x); }
+#define ntohl git_ntohl
+
+sig_handler_t mingw_signal(int sig, sig_handler_t handler);
+#define signal mingw_signal
+
+/*
+ * git specific compatibility
+ */
+
+static inline int has_dos_drive_prefix(const char *path)
+{
+	return isalpha(*path) && path[1] == ':';
+}
+#define has_dos_drive_prefix has_dos_drive_prefix
+
+#define PATH_SEP ';'
+#define PRIuMAX "I64u"
+
+/*
+ * helpers
+ */
+
+char **copy_environ(void);
+void free_environ(char **env);
+char **env_setenv(char **env, const char *name);
diff --git a/git-compat-util.h b/git-compat-util.h
index 3ea0d91..1ee7357 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -89,7 +89,10 @@
 #include <grp.h>
 #define _ALL_SOURCE 1
 #endif
-#endif	/* !__MINGW32__ */
+#else 	/* __MINGW32__ */
+/* pull in Windows compatibility stuff */
+#include "compat/mingw.h"
+#endif	/* __MINGW32__ */
 
 #ifndef NO_ICONV
 #include <iconv.h>
@@ -110,6 +113,10 @@
 #define PATH_SEP ':'
 #endif
 
+#ifndef has_dos_drive_prefix
+#define has_dos_drive_prefix(path) 0
+#endif
+
 #ifdef __GNUC__
 #define NORETURN __attribute__((__noreturn__))
 #else
@@ -369,10 +376,6 @@ static inline FILE *xfdopen(int fd, const char *mode)
 	return stream;
 }
 
-#ifdef __MINGW32__
-int mkstemp(char *template);
-#endif
-
 static inline int xmkstemp(char *template)
 {
 	int fd;
@@ -453,217 +456,4 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 #define qsort git_qsort
 #endif
 
-#ifdef __MINGW32__
-
-#include <winsock2.h>
-
-/*
- * things that are not available in header files
- */
-
-typedef int pid_t;
-#define hstrerror strerror
-
-#define S_IFLNK    0120000 /* Symbolic link */
-#define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
-#define S_ISSOCK(x) 0
-#define S_IRGRP 0
-#define S_IWGRP 0
-#define S_IXGRP 0
-#define S_ISGID 0
-#define S_IROTH 0
-#define S_IXOTH 0
-
-#define WIFEXITED(x) ((unsigned)(x) < 259)	/* STILL_ACTIVE */
-#define WEXITSTATUS(x) ((x) & 0xff)
-#define WIFSIGNALED(x) ((unsigned)(x) > 259)
-
-#define SIGKILL 0
-#define SIGCHLD 0
-#define SIGPIPE 0
-#define SIGALRM 100
-
-#define F_GETFD 1
-#define F_SETFD 2
-#define FD_CLOEXEC 0x1
-
-struct passwd {
-	char *pw_name;
-	char *pw_gecos;
-	char *pw_dir;
-};
-
-struct pollfd {
-	int fd;           /* file descriptor */
-	short events;     /* requested events */
-	short revents;    /* returned events */
-};
-#define POLLIN 1
-#define POLLHUP 2
-
-typedef void (__cdecl *sig_handler_t)(int);
-struct sigaction {
-	sig_handler_t sa_handler;
-	unsigned sa_flags;
-};
-#define sigemptyset(x) (void)0
-#define SA_RESTART 0
-
-struct itimerval {
-	struct timeval it_value, it_interval;
-};
-#define ITIMER_REAL 0
-
-/*
- * trivial stubs
- */
-
-static inline int readlink(const char *path, char *buf, size_t bufsiz)
-{ errno = ENOSYS; return -1; }
-static inline int symlink(const char *oldpath, const char *newpath)
-{ errno = ENOSYS; return -1; }
-static inline int link(const char *oldpath, const char *newpath)
-{ errno = ENOSYS; return -1; }
-static inline int fchmod(int fildes, mode_t mode)
-{ errno = ENOSYS; return -1; }
-static inline int fork(void)
-{ errno = ENOSYS; return -1; }
-static inline unsigned int alarm(unsigned int seconds)
-{ return 0; }
-static inline int fsync(int fd)
-{ return 0; }
-static inline int getppid(void)
-{ return 1; }
-static inline void sync(void)
-{}
-static inline int getuid()
-{ return 1; }
-static inline struct passwd *getpwnam(const char *name)
-{ return NULL; }
-static inline int fcntl(int fd, int cmd, long arg)
-{
-	if (cmd == F_GETFD || cmd == F_SETFD)
-		return 0;
-	errno = EINVAL;
-	return -1;
-}
-
-/*
- * simple adaptors
- */
-
-static inline int mingw_mkdir(const char *path, int mode)
-{
-	return mkdir(path);
-}
-#define mkdir mingw_mkdir
-
-static inline int mingw_unlink(const char *pathname)
-{
-	/* read-only files cannot be removed */
-	chmod(pathname, 0666);
-	return unlink(pathname);
-}
-#define unlink mingw_unlink
-
-static inline int waitpid(pid_t pid, unsigned *status, unsigned options)
-{
-	if (options == 0)
-		return _cwait(status, pid, 0);
-	errno = EINVAL;
-	return -1;
-}
-
-/*
- * implementations of missing functions
- */
-
-int pipe(int filedes[2]);
-unsigned int sleep (unsigned int seconds);
-int gettimeofday(struct timeval *tv, void *tz);
-int poll(struct pollfd *ufds, unsigned int nfds, int timeout);
-struct tm *gmtime_r(const time_t *timep, struct tm *result);
-struct tm *localtime_r(const time_t *timep, struct tm *result);
-int getpagesize(void);	/* defined in MinGW's libgcc.a */
-struct passwd *getpwuid(int uid);
-int setitimer(int type, struct itimerval *in, struct itimerval *out);
-int sigaction(int sig, struct sigaction *in, struct sigaction *out);
-
-/*
- * replacements of existing functions
- */
-
-int mingw_open (const char *filename, int oflags, ...);
-#define open mingw_open
-
-char *mingw_getcwd(char *pointer, int len);
-#define getcwd mingw_getcwd
-
-struct hostent *mingw_gethostbyname(const char *host);
-#define gethostbyname mingw_gethostbyname
-
-int mingw_socket(int domain, int type, int protocol);
-#define socket mingw_socket
-
-int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz);
-#define connect mingw_connect
-
-int mingw_rename(const char*, const char*);
-#define rename mingw_rename
-
-/* Use mingw_lstat() instead of lstat()/stat() and
- * mingw_fstat() instead of fstat() on Windows.
- * struct stat is redefined because it lacks the st_blocks member.
- */
-struct mingw_stat {
-	unsigned st_mode;
-	time_t st_mtime, st_atime, st_ctime;
-	unsigned st_dev, st_ino, st_uid, st_gid;
-	size_t st_size;
-	size_t st_blocks;
-};
-int mingw_lstat(const char *file_name, struct mingw_stat *buf);
-int mingw_fstat(int fd, struct mingw_stat *buf);
-#define fstat mingw_fstat
-#define lstat mingw_lstat
-#define stat mingw_stat
-static inline int mingw_stat(const char *file_name, struct mingw_stat *buf)
-{ return mingw_lstat(file_name, buf); }
-
-int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
-#define vsnprintf mingw_vsnprintf
-
-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
-void mingw_execvp(const char *cmd, char *const *argv);
-#define execvp mingw_execvp
-
-static inline unsigned int git_ntohl(unsigned int x)
-{ return (unsigned int)ntohl(x); }
-#define ntohl git_ntohl
-
-sig_handler_t mingw_signal(int sig, sig_handler_t handler);
-#define signal mingw_signal
-
-/*
- * helpers
- */
-
-char **copy_environ(void);
-void free_environ(char **env);
-char **env_setenv(char **env, const char *name);
-
-static inline int has_dos_drive_prefix(const char *path)
-{
-	return isalpha(*path) && path[1] == ':';
-}
-
-#else /* __MINGW32__ */
-
-static inline int has_dos_drive_prefix(const char *path)
-{
-	return 0;
-}
-
-#endif /* __MINGW32__ */

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Wed, 5 Mar 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 13:05, Johannes Schindelin wrote:
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > > --- a/git-compat-util.h
> > > +++ b/git-compat-util.h
> > > @@ -357,6 +359,10 @@ static inline FILE *xfdopen(int fd, const char
> > > *mode) return stream;
> > >  }
> > >
> > > +#ifdef __MINGW32__
> > > +int mkstemp(char *template);
> > > +#endif
> > > +
> > >  static inline int xmkstemp(char *template)
> > >  {
> > >  	int fd;
> >
> > Could we have this...
> >
> > > @@ -437,4 +443,140 @@ void git_qsort(void *base, size_t nmemb, size_t
> > > size, #define qsort git_qsort
> > >  #endif
> > >
> > > +#ifdef __MINGW32__
> > > +
> > > +#include <winsock2.h>
> > > +
> > > +/*
> > > + * things that are not available in header files
> > > + */
> > > +
> > > [...]
> >
> > ... and this in compat/mingw.h?  And then, we'd only have
> >
> > #ifdef __MINGW32__
> > #include "mingw.h"
> > #endif
> >
> > in git-compat-util.h?
> 
> I've now created compat/mingw.h. Below is the interdiff. Of course, it 
> mostly only moves code around. But you might want to look at at mkstemp, 
> PATH_SEP, PRIuMAX, and has_dos_drive_prefix. Notice also that I include 
> compat/mingw.h early in git-compat-util.h instead of late.

Thanks!

With this, I think I have nothing to add to your series (except maybe an 
Acked-by: where it applies, or a Reviewed-by:, but I think that this 
would only be a burden on our maintainer).

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Thanks!
>
> With this, I think I have nothing to add to your series (except maybe an 
> Acked-by: where it applies, or a Reviewed-by:, but I think that this 
> would only be a burden on our maintainer).

I think Reviewed-by: would indeed be a very good addition to our
patch flow convention, borrowing from the kernel folks.

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Wed, 5 Mar 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Thanks!
> >
> > With this, I think I have nothing to add to your series (except maybe an 
> > Acked-by: where it applies, or a Reviewed-by:, but I think that this 
> > would only be a burden on our maintainer).
> 
> I think Reviewed-by: would indeed be a very good addition to our
> patch flow convention, borrowing from the kernel folks.

You mean you have more people to blame, then? ;-)

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I think Reviewed-by: would indeed be a very good addition to our
>> patch flow convention, borrowing from the kernel folks.
>
> You mean you have more people to blame, then? ;-)

No.  The procedure would help me keep my impatience from making
me merge patches that have not been adequately reviewed on the
list.

Recently, I ended up wasting two nights because I was not
careful enough earlier, when I was short of time and apparent
backlog was beginning to get larger and larger.

I queued some patches from the backlog to 'next' saying "ah,
they look good enough, people will notice breakages anyway," but
the breakage was not caught until 'master' got broken.

Not good.  And the list is not to blame.

By merging to 'next' I am sending a message that I think they
have been adequately reviewed (either by me or by people whose
judgement I trust), so I shouldn't have applied them to 'next'
in the first place.  I instead should have ignored them, and
waited until I had enough time and concentration to properly
review them.  Or until somebody else did --- by that time,
hopefully other people might have commented on them, saying
"these look all ok to me", or "ah that's crap".

These wasted two nights was all my fault, and as a result, there
are more patches on the list archive that I have seen (notice I
did not say "have read") that are unapplied.

As to those "more patches on the list that are unapplied", I'll
keep them unapplied for now, until there are positive feedbacks
on them.

The positive feedback may come from myself.  I am not saying I
will stop reviewing and/or applying patches nobody else
commented on.

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Wed, 5 Mar 2008, Junio C Hamano wrote:

> Recently, I ended up wasting two nights because I was not careful enough 
> earlier, when I was short of time and apparent backlog was beginning to 
> get larger and larger.
> 
> I queued some patches from the backlog to 'next' saying "ah, they look 
> good enough, people will notice breakages anyway," but the breakage was 
> not caught until 'master' got broken.
> 
> Not good.  And the list is not to blame.

Ah, but I think that you are too harsh onto yourself.  Recently, there was 
a surge of patches, mainly because 1.5.4 was held of -- but for a good 
reason: 1.5.4 was not ready before the point in time where you decided to 
release it.

If at all, the list is to blame, for just sending patches, but not 
reviewing them.

Now, personally I know that I am not half as good a reviewer as you are, 
since you catch way more bugs than me, just by looking at the patch.

But still, "many eyes make bugs shallow" is a principle to be heeded 
_everywhere_.

So I'd say: if you think that you are short of time, and patches have not 
been reviewed properly, do not assume it _your sole_ responsibility to 
review the patches.  Make it known that other people have to step in (even 
if it is a mediocre reviewer like me).  Do not overload yourself.

So my comment about the "blame" was really tongue-in-cheek.  Please do not 
take it for anything but a joke.

And when I say that I think you are a kick-ass maintainer, I _mean_ it.

'nuff said,
Dscho

[PATCH 1/2] Add strbuf_initf()

[Johannes Schindelin]

The most common use of addf() was to init a strbuf and addf() right away.
Since it is so common, it makes sense to have a function strbuf_initf()
to wrap both calls into one.

Unfortunately, C (and cpp) has no way to make this easy without code
duplication, as we need to va_init() in strbuf_addf() possibly a few
times.  So the code for addf() is copied.  Fortunately, the code is
pretty short, so not too much had to be copied as-is.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Wed, 5 Mar 2008, Johannes Schindelin wrote:

	> On Wed, 5 Mar 2008, Junio C Hamano wrote:
	> 
	> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> > 
	> > > [... I'd submit my ] a Reviewed-by:, but I think that this 
	> > > would only be a burden on our maintainer.
	> > 
	> > I think Reviewed-by: would indeed be a very good addition to 
	> > our patch flow convention, borrowing from the kernel folks.
	> 
	> You mean you have more people to blame, then? ;-)

	Well, it was meant as a joke...

	Anyway, here is a start of a patch series that should help the 
	King Penguin...

	strbuf_initf() is something I long planned to do; Kristian just 
	pushed me over the edge.

 strbuf.c |   23 +++++++++++++++++++++++
 strbuf.h |    2 ++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 5afa8f3..067d55a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -147,6 +147,29 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+void strbuf_initf(struct strbuf *sb, const char *fmt, ...)
+{
+	int len;
+	va_list ap;
+
+	strbuf_init(sb, strlen(fmt) + 64);
+	va_start(ap, fmt);
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	va_end(ap);
+	if (len < 0)
+		die("your vsnprintf is broken");
+	if (len > strbuf_avail(sb)) {
+		strbuf_grow(sb, len);
+		va_start(ap, fmt);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+		va_end(ap);
+		if (len > strbuf_avail(sb)) {
+			die("this should not happen, your snprintf is broken");
+		}
+	}
+	strbuf_setlen(sb, sb->len + len);
+}
+
 void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 		   void *context)
 {
diff --git a/strbuf.h b/strbuf.h
index faec229..eaf2409 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -108,6 +108,8 @@ extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 
 __attribute__((format(printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+__attribute__((format(printf,2,3)))
+extern void strbuf_initf(struct strbuf *sb, const char *fmt, ...);
 
 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
-- 
1.5.4.3.571.g9aec3

Re: [PATCH 1/2] Add strbuf_initf()

[Mike Hommey]

On Thu, Mar 06, 2008 at 02:14:43AM +0100, Johannes Schindelin wrote:
> 
> The most common use of addf() was to init a strbuf and addf() right away.
> Since it is so common, it makes sense to have a function strbuf_initf()
> to wrap both calls into one.
> 
> Unfortunately, C (and cpp) has no way to make this easy without code
> duplication, as we need to va_init() in strbuf_addf() possibly a few
> times.  So the code for addf() is copied.  Fortunately, the code is
> pretty short, so not too much had to be copied as-is.

The problem with code duplication is not about code size, but more about
not forgetting to fix bugs in all incarnations of the duplicated code.

Is it so ugly to use a macro ?

Mike

Re: [PATCH 1/2] Add strbuf_initf()

[Reece Dunn]

On 06/03/2008, Mike Hommey <mh@glandium.org> wrote:
> On Thu, Mar 06, 2008 at 02:14:43AM +0100, Johannes Schindelin wrote:
>  >
>  > The most common use of addf() was to init a strbuf and addf() right away.
>  > Since it is so common, it makes sense to have a function strbuf_initf()
>  > to wrap both calls into one.
>  >
>  > Unfortunately, C (and cpp) has no way to make this easy without code
>  > duplication, as we need to va_init() in strbuf_addf() possibly a few
>  > times.  So the code for addf() is copied.  Fortunately, the code is
>  > pretty short, so not too much had to be copied as-is.
>
>
> The problem with code duplication is not about code size, but more about
>  not forgetting to fix bugs in all incarnations of the duplicated code.
>
>  Is it so ugly to use a macro ?

Why not have a strbuf_vaddf and strbuf_vinitf that take a va_arg as a
parameter. This would mean that you don't have code duplication, and
it is flexible enough if you want to add more customisations in the
future. No macro needed. This is what the printf/scanf family of
functions do.

- Reece

Re: [PATCH 1/2] Add strbuf_initf()

[Johannes Schindelin]

Hi,

On Thu, 6 Mar 2008, Reece Dunn wrote:

> On 06/03/2008, Mike Hommey <mh@glandium.org> wrote:
> > On Thu, Mar 06, 2008 at 02:14:43AM +0100, Johannes Schindelin wrote:
> >  >
> >  > The most common use of addf() was to init a strbuf and addf() right 
> >  > away. Since it is so common, it makes sense to have a function 
> >  > strbuf_initf() to wrap both calls into one.
> >  >
> >  > Unfortunately, C (and cpp) has no way to make this easy without 
> >  > code duplication, as we need to va_init() in strbuf_addf() possibly 
> >  > a few times.  So the code for addf() is copied.  Fortunately, the 
> >  > code is pretty short, so not too much had to be copied as-is.
> >
> >
> > The problem with code duplication is not about code size, but more 
> > about not forgetting to fix bugs in all incarnations of the duplicated 
> > code.
> >
> > Is it so ugly to use a macro ?
> 
> Why not have a strbuf_vaddf and strbuf_vinitf that take a va_arg as a 
> parameter. This would mean that you don't have code duplication, and it 
> is flexible enough if you want to add more customisations in the future. 
> No macro needed. This is what the printf/scanf family of functions do.

The problem is that we have to restart va_list() if the buffer was too 
small.

So your suggestion is out, unless you suggest to implement the whole 
printf mechanism... which I hope you're not.

Ciao,
Dscho

Re: [PATCH 1/2] Add strbuf_initf()

[Reece Dunn]

On 06/03/2008, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>  On Thu, 6 Mar 2008, Reece Dunn wrote:
>
>  > On 06/03/2008, Mike Hommey <mh@glandium.org> wrote:
>  > > On Thu, Mar 06, 2008 at 02:14:43AM +0100, Johannes Schindelin wrote:
>  > >  >
>  > >  > The most common use of addf() was to init a strbuf and addf() right
>  > >  > away. Since it is so common, it makes sense to have a function
>  > >  > strbuf_initf() to wrap both calls into one.
>  > >  >
>  > >  > Unfortunately, C (and cpp) has no way to make this easy without
>  > >  > code duplication, as we need to va_init() in strbuf_addf() possibly
>  > >  > a few times.  So the code for addf() is copied.  Fortunately, the
>  > >  > code is pretty short, so not too much had to be copied as-is.
>  > >
>  > >
>  > > The problem with code duplication is not about code size, but more
>  > > about not forgetting to fix bugs in all incarnations of the duplicated
>  > > code.
>  > >
>  > > Is it so ugly to use a macro ?
>  >
>  > Why not have a strbuf_vaddf and strbuf_vinitf that take a va_arg as a
>  > parameter. This would mean that you don't have code duplication, and it
>  > is flexible enough if you want to add more customisations in the future.
>  > No macro needed. This is what the printf/scanf family of functions do.
>
>
> The problem is that we have to restart va_list() if the buffer was too
>  small.

Ok.

>  So your suggestion is out, unless you suggest to implement the whole
>  printf mechanism... which I hope you're not.

No, that was not my intention. My intention was that they were simple
forwarding functions that handled the va_start and va_end calls.

Is it possible to pass a void * to a strbuf_vaddf function that you
can pass to va_start, so you can then restart the va_list?

- Reece

Re: [PATCH 1/2] Add strbuf_initf()

[Johannes Schindelin]

Hi,

On Thu, 6 Mar 2008, Reece Dunn wrote:

> On 06/03/2008, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> >  On Thu, 6 Mar 2008, Reece Dunn wrote:
> >
> >  > On 06/03/2008, Mike Hommey <mh@glandium.org> wrote:
> >  > > On Thu, Mar 06, 2008 at 02:14:43AM +0100, Johannes Schindelin wrote:
> >  > >  >
> >  > >  > The most common use of addf() was to init a strbuf and addf() 
> >  > >  > right away. Since it is so common, it makes sense to have a 
> >  > >  > function strbuf_initf() to wrap both calls into one.
> >  > >  >
> >  > >  > Unfortunately, C (and cpp) has no way to make this easy 
> >  > >  > without code duplication, as we need to va_init() in 
> >  > >  > strbuf_addf() possibly a few times.  So the code for addf() is 
> >  > >  > copied.  Fortunately, the code is pretty short, so not too 
> >  > >  > much had to be copied as-is.
> >  >
> >  > Why not have a strbuf_vaddf and strbuf_vinitf that take a va_arg as 
> >  > a parameter. This would mean that you don't have code duplication, 
> >  > and it is flexible enough if you want to add more customisations in 
> >  > the future. No macro needed. This is what the printf/scanf family 
> >  > of functions do.
> 
> Is it possible to pass a void * to a strbuf_vaddf function that you can 
> pass to va_start, so you can then restart the va_list?

AFAIU va_list() works on the stack (on less register-challenged systems 
than x86, the first parameters are possibly passed as registers, too, but 
it has to resort to the stack at a certain number of parameters).

Since the stack is also used (at least on register-challenged yadda yadda) 
to store the return address, va_list() would most likely pick that up, 
too.

Now, that just _might_ still work, since the printf() family determines 
the number of arguments from the format string.

But then there are machines which are _not_ completely stack-based, for 
example SPARC.  IIRC the first 8 parameters are passed by a so-called 
register window, which changes with each function call.

So no, I think there is no portable way to pass them around.

Of course, having a simple implementation for addf() _not_ using 
vsnprintf() could help, too (and make the process more efficient, 
probably).

I thought.

The formats we'd have to support are:

$ git grep strbuf_addf |
	sed -e 's/^[^"]*"\([^"]*\)".*$/\1/' -e 's/%%//g' -e 's/%/\
&/g' | sed -n -e 's/\(%[^a-zA-Z]*[a-zA-Z]\).*/\1/p' | sort | uniq

%02X
%06o
%c
%d
%l
%o
%s
%.*s
%u

So it does not look too bad.  'X', 'o', 'd', 'l' and 'u', possibly with a 
size specifier (left 0-padded), 'c' and 's' (possibly with a length 
parameter).

The implementation wouldn't be _too_ short, I'd say, but doable.

Ciao,
Dscho

[PATCH 1/2 v2] Add strbuf_vaddf(), use it in strbuf_addf(), and add strbuf_initf()

[Johannes Schindelin]

The most common use of addf() was to init a strbuf and addf() right away.
Since it is so common, it makes sense to have a function strbuf_initf() to
wrap both calls into one.

To do that, we implement a (really minimal) vaddf() lookalike to
vsprintf().

At the moment, it only handles %u, %i, %d, %l, %o, %x and %X with size
indicators '<number>', ' <number>' and '0<number>', as well as %c and %s,
the latter with size indicators '.*' in addition to the same size
indicators as for numbers.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Thu, 6 Mar 2008, Johannes Schindelin wrote:

	> Of course, having a simple implementation for addf() _not_ using 
	> vsnprintf() could help, too (and make the process more efficient, 
	> probably).

	Having thought about this a bit, I came up with this 
	implementation (replacing my earlier PATCH 1/2).  It is by far not 
	complete, but that is exactly the idea: we do not _need_ a fully-fledged 
	printf() implementation.

	Besides, it was fun.

 .gitignore       |    1 +
 Makefile         |    4 +-
 strbuf.c         |  134 +++++++++++++++++++++++++++++++++++++++++++++++------
 strbuf.h         |    3 +
 t/t0000-basic.sh |    8 +++
 test-strbuf.c    |   17 +++++++
 6 files changed, 150 insertions(+), 17 deletions(-)
 create mode 100644 test-strbuf.c

diff --git a/.gitignore b/.gitignore
index 219759f..c0ecd41 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@ test-genrandom
 test-match-trees
 test-parse-options
 test-sha1
+test-strbuf
 common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index cd0b294..58f6238 100644
--- a/Makefile
+++ b/Makefile
@@ -1052,7 +1052,7 @@ endif
 
 ### Testing rules
 
-TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X test-absolute-path$X test-parse-options$X
+TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X test-absolute-path$X test-parse-options$X test-strbuf$X
 
 all:: $(TEST_PROGRAMS)
 
@@ -1071,6 +1071,8 @@ test-delta$X: diff-delta.o patch-delta.o
 
 test-parse-options$X: parse-options.o
 
+test-strbuf$X: strbuf.o
+
 .PRECIOUS: $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
 
 test-%$X: test-%.o $(GITLIBS)
diff --git a/strbuf.c b/strbuf.c
index 5afa8f3..5ad89a8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -123,28 +123,130 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+static int number_length(unsigned long number, long base)
+{
+	int length = 1;
+	while (number >= base) {
+		number /= base;
+		length++;
+	}
+	return length;
+}
+
+/*
+ * Only supports %u, %i, %d, %l, %o, %x and %X with size indicators
+ *   '<number>', '0<number>', and ' <number>',
+ * as well as %c,
+ * and %s with size indicators '<number>', ' <number>' and '.*'.
+ */
+void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
+{
+	while (*fmt) {
+		char fill = '\0';
+		int size = -1, max_size = -1;
+		char *p = (char *)fmt;
+
+		if (*p != '%' || *(++p) == '%') {
+			strbuf_addch(sb, *p++);
+			fmt = p;
+			continue;
+		}
+		if (*p == ' ' || *p == '0')
+			fill = *p++;
+		if (isdigit(*p))
+			size = (int)strtol(p, &p, 10);
+		else if (!prefixcmp(p, ".*")) {
+			max_size = va_arg(ap, int);
+			p += 2;
+		}
+		switch (*p) {
+		case 's': {
+			const char *s = va_arg(ap, const char *);
+			if (fill) {
+				int len = size - strlen(s);
+				while (len-- > 0)
+					strbuf_addch(sb, fill);
+			}
+			while (*s && max_size--)
+				strbuf_addch(sb, *s++);
+			break;
+		}
+		case 'c':
+			strbuf_addch(sb, va_arg(ap, int));
+			break;
+		case 'u':
+		case 'i':
+		case 'l':
+		case 'd':
+		case 'o':
+		case 'x':
+		case 'X': {
+			int base = *p == 'x' || *p == 'X' ? 16 :
+				*p == 'o' ? 8 : 10;
+			int negative = 0, len;
+			unsigned long number, power;
+
+			if (*p == 'u')
+				number = va_arg(ap, unsigned int);
+			else {
+				long signed_number;
+				if (*p == 'l')
+					signed_number = va_arg(ap, long);
+				else
+					signed_number = va_arg(ap, int);
+				if (signed_number < 0) {
+					negative = 1;
+					number = -signed_number;
+				} else
+					number = signed_number;
+			}
+
+			/* pad */
+			len = number_length(number, base);
+			while (size-- > len + negative)
+				strbuf_addch(sb, fill ? fill : ' ');
+			if (negative)
+				strbuf_addch(sb, '-');
+
+			/* output number */
+			power = 1;
+			while (len-- > 1)
+				power *= base;
+			while (power) {
+				int digit = number / power;
+				strbuf_addch(sb, digit < 10 ? '0' + digit
+					: *p + 'A' - 'X' + digit - 10);
+				number -= digit * power;
+				power /= base;
+			}
+
+			break;
+		}
+		default:
+			/* unknown / invalid format: copy verbatim */
+			strbuf_insert(sb, sb->len, fmt, p - fmt + 1);
+		}
+		fmt = p + (*p != '\0');
+	}
+}
+
 void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 {
-	int len;
 	va_list ap;
 
-	if (!strbuf_avail(sb))
-		strbuf_grow(sb, 64);
 	va_start(ap, fmt);
-	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	strbuf_vaddf(sb, fmt, ap);
+	va_end(ap);
+}
+
+void strbuf_initf(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list ap;
+
+	strbuf_init(sb, strlen(fmt) + 64);
+	va_start(ap, fmt);
+	strbuf_vaddf(sb, fmt, ap);
 	va_end(ap);
-	if (len < 0)
-		die("your vsnprintf is broken");
-	if (len > strbuf_avail(sb)) {
-		strbuf_grow(sb, len);
-		va_start(ap, fmt);
-		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-		va_end(ap);
-		if (len > strbuf_avail(sb)) {
-			die("this should not happen, your snprintf is broken");
-		}
-	}
-	strbuf_setlen(sb, sb->len + len);
 }
 
 void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
diff --git a/strbuf.h b/strbuf.h
index faec229..d7c7aaf 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -106,8 +106,11 @@ extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
 typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
 extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);
 
+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list args);
 __attribute__((format(printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+__attribute__((format(printf,2,3)))
+extern void strbuf_initf(struct strbuf *sb, const char *fmt, ...);
 
 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 27b54cb..c1d4639 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -311,6 +311,14 @@ test_expect_success 'absolute path works as expected' '
 	test "$sym" = "$(test-absolute-path $dir2/syml)"
 '
 
+test_expect_success 'strbuf_initf() works as expected' '
+
+	eval $(test-strbuf) &&
+	test ! -z "$result" &&
+	test "$result" = "$expect"
+
+'
+
 test_expect_success 'very long name in the index handled sanely' '
 
 	a=a && # 1
diff --git a/test-strbuf.c b/test-strbuf.c
new file mode 100644
index 0000000..479fa08
--- /dev/null
+++ b/test-strbuf.c
@@ -0,0 +1,17 @@
+#include "cache.h"
+#include "strbuf.h"
+
+int main(int argc, char **argv)
+{
+	struct strbuf buf;
+#define TEST_FORMAT \
+	"'%%%.*s,%x,%05X,%u,%i,% 4d,%3d,%c,%3d'", \
+	5, "Hello, World!", 27, 27, -1, -1, 1, 5, ':', 1234
+
+	strbuf_initf(&buf, TEST_FORMAT);
+	printf("result=%s\n", buf.buf);
+	printf("expect=" TEST_FORMAT);
+	strbuf_release(&buf);
+
+	return 0;
+}
-- 
1.5.4.3.571.g9aec3.dirty

Re: [PATCH 1/2 v2] Add strbuf_vaddf(), use it in strbuf_addf(), and add strbuf_initf()

[Johannes Sixt]

Johannes Schindelin schrieb:
> The most common use of addf() was to init a strbuf and addf() right away.
> Since it is so common, it makes sense to have a function strbuf_initf() to
> wrap both calls into one.
> 
> To do that, we implement a (really minimal) vaddf() lookalike to
> vsprintf().
> 
> At the moment, it only handles %u, %i, %d, %l, %o, %x and %X with size
> indicators '<number>', ' <number>' and '0<number>', as well as %c and %s,
> the latter with size indicators '.*' in addition to the same size
> indicators as for numbers.

WTF?

You did not cater for PRIuMAX, which is %llu except on Windows, where it
is %I64u. We can make it %llu on Windows only if we can ensure that none
of its uses ends up in a regular *printf function!

-- Hannes

Re: [PATCH 1/2 v2] Add strbuf_vaddf(), use it in strbuf_addf(), and add strbuf_initf()

[Johannes Sixt]

Johannes Sixt schrieb:
> You did not cater for PRIuMAX, which is %llu except on Windows, where it
> is %I64u. We can make it %llu on Windows only if we can ensure that none
> of its uses ends up in a regular *printf function!

Oh, well, PRIuMAX is *only* used in regular *printf and in die() calls.
That's fine, too.

-- Hannes

Re: [PATCH 1/2 v2] Add strbuf_vaddf(), use it in strbuf_addf(), and add strbuf_initf()

[Johannes Schindelin]

Hi,

On Thu, 6 Mar 2008, Johannes Sixt wrote:

> Johannes Sixt schrieb:
> > You did not cater for PRIuMAX, which is %llu except on Windows, where 
> > it is %I64u. We can make it %llu on Windows only if we can ensure that 
> > none of its uses ends up in a regular *printf function!
> 
> Oh, well, PRIuMAX is *only* used in regular *printf and in die() calls. 
> That's fine, too.

Hey, no problem.  I used my earlier analysis to find the _minimal_ set of 
formats needed to get Git running without that vsnprintf().

The idea is, of course, to enhance vaddf() as needed (if at all).

Note: I did not even bother trying to convert any vsnprintf() users except 
for strbuf_addf() to that interface: it should be relatively easy, but 
there are quite a few sites (even using strbuf, but not addf() because 
only a va_list is available), so it is left as an exercise to the reader 
;-)

Ciao,
Dscho

Re: [PATCH 1/2] Add strbuf_initf()

[Kristian Høgsberg]

On Thu, 2008-03-06 at 11:55 +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 6 Mar 2008, Reece Dunn wrote:
> 
> > On 06/03/2008, Mike Hommey <mh@glandium.org> wrote:
> > > On Thu, Mar 06, 2008 at 02:14:43AM +0100, Johannes Schindelin wrote:
> > >  >
> > >  > The most common use of addf() was to init a strbuf and addf() right 
> > >  > away. Since it is so common, it makes sense to have a function 
> > >  > strbuf_initf() to wrap both calls into one.
> > >  >
> > >  > Unfortunately, C (and cpp) has no way to make this easy without 
> > >  > code duplication, as we need to va_init() in strbuf_addf() possibly 
> > >  > a few times.  So the code for addf() is copied.  Fortunately, the 
> > >  > code is pretty short, so not too much had to be copied as-is.
> > >
> > >
> > > The problem with code duplication is not about code size, but more 
> > > about not forgetting to fix bugs in all incarnations of the duplicated 
> > > code.
> > >
> > > Is it so ugly to use a macro ?
> > 
> > Why not have a strbuf_vaddf and strbuf_vinitf that take a va_arg as a 
> > parameter. This would mean that you don't have code duplication, and it 
> > is flexible enough if you want to add more customisations in the future. 
> > No macro needed. This is what the printf/scanf family of functions do.
> 
> The problem is that we have to restart va_list() if the buffer was too 
> small.

I think we've spent more time debating va_copy than it would take for
somebody to just lift the implementation and checks from something like
glib.  But the recent patch for vsnprintf[1] doesn't actually fix the
problem of reusing va_args in the general case; the va_list argument,
ap, is undefined after the vsnprintf() call, yet it calls it in a loop.
Just bite the bullet and pull in va_copy.  Of course, I'm just adding
to the debate here and not sending patches :/

> So your suggestion is out, unless you suggest to implement the whole 
> printf mechanism... which I hope you're not.

It's not a terrible idea, honestly.  There are several mature vsnprintf
implementations out there under friendly licenses.  We could just stick
it in compat/.  It's the only way to do reliable, cross platform
vsnprintf, in my experience.  And the issue with %I64u vs %llu could be
handle by implementing both, as Wayne Davison suggests.

Kristian

[1] Message-Id: 200803051646.13343.michal.rokos@nextsoft.cz from Michal Rokos

Re: [PATCH 1/2] Add strbuf_initf()

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 811 bytes --]

Hi,

On Thu, 6 Mar 2008, Kristian Høgsberg wrote:

> On Thu, 2008-03-06 at 11:55 +0100, Johannes Schindelin wrote:
>
> > So your suggestion is out, unless you suggest to implement the whole 
> > printf mechanism... which I hope you're not.
> 
> It's not a terrible idea, honestly.

Heh.  Yeah, I actually posted a start...

> There are several mature vsnprintf implementations out there under 
> friendly licenses.  We could just stick it in compat/.  It's the only 
> way to do reliable, cross platform vsnprintf, in my experience.  And the 
> issue with %I64u vs %llu could be handle by implementing both, as Wayne 
> Davison suggests.

Sure, we could always copy from somewhere else.  My rationale was that I 
had fun writing it, and that we do not really need a full printf() 
implementation.

Ciao,
Dscho

Re: [PATCH 1/2] Add strbuf_initf()

[Kristian Høgsberg]

On Thu, 2008-03-06 at 19:26 +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 6 Mar 2008, Kristian Høgsberg wrote:
> 
> > On Thu, 2008-03-06 at 11:55 +0100, Johannes Schindelin wrote:
> >
> > > So your suggestion is out, unless you suggest to implement the whole 
> > > printf mechanism... which I hope you're not.
> > 
> > It's not a terrible idea, honestly.
> 
> Heh.  Yeah, I actually posted a start...
> 
> > There are several mature vsnprintf implementations out there under 
> > friendly licenses.  We could just stick it in compat/.  It's the only 
> > way to do reliable, cross platform vsnprintf, in my experience.  And the 
> > issue with %I64u vs %llu could be handle by implementing both, as Wayne 
> > Davison suggests.
> 
> Sure, we could always copy from somewhere else.  My rationale was that I 
> had fun writing it, and that we do not really need a full printf() 
> implementation.

That's how it always starts... "We just need %d and %s! It'll take me
half an hour!"  I know because, I've it a number of times myself ;)
Just pull in the rsync one, don't waste time on this.

Kristian

Re: [PATCH 1/2] Add strbuf_initf()

[Mike Hommey]

On Thu, Mar 06, 2008 at 07:26:50PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 6 Mar 2008, Kristian Høgsberg wrote:
> 
> > On Thu, 2008-03-06 at 11:55 +0100, Johannes Schindelin wrote:
> >
> > > So your suggestion is out, unless you suggest to implement the whole 
> > > printf mechanism... which I hope you're not.
> > 
> > It's not a terrible idea, honestly.
> 
> Heh.  Yeah, I actually posted a start...
> 
> > There are several mature vsnprintf implementations out there under 
> > friendly licenses.  We could just stick it in compat/.  It's the only 
> > way to do reliable, cross platform vsnprintf, in my experience.  And the 
> > issue with %I64u vs %llu could be handle by implementing both, as Wayne 
> > Davison suggests.
> 
> Sure, we could always copy from somewhere else.  My rationale was that I 
> had fun writing it, and that we do not really need a full printf() 
> implementation.

And vsnprintf is a mess accross systems, returning -1 when not expected.
I even had the surprise to get an interesting bug in liferea/libxml2 due
to glibc's vsnprintf returning -1 in some obscure cases.

Mike

Re: [PATCH 1/2] Add strbuf_initf()

[Johannes Schindelin]

Hi,

On Thu, 6 Mar 2008, Mike Hommey wrote:

> On Thu, Mar 06, 2008 at 02:14:43AM +0100, Johannes Schindelin wrote:
> > 
> > The most common use of addf() was to init a strbuf and addf() right 
> > away. Since it is so common, it makes sense to have a function 
> > strbuf_initf() to wrap both calls into one.
> > 
> > Unfortunately, C (and cpp) has no way to make this easy without code 
> > duplication, as we need to va_init() in strbuf_addf() possibly a few 
> > times.  So the code for addf() is copied.  Fortunately, the code is 
> > pretty short, so not too much had to be copied as-is.
> 
> The problem with code duplication is not about code size, but more about 
> not forgetting to fix bugs in all incarnations of the duplicated code.

Yes, I know.

> Is it so ugly to use a macro ?

AFAIR it is unportable to have a macro

#define strbuf_initf(buf, fmt, ...) \
	(strbuf_init(buf, strlen(fmt) + 64), strbuf_addf(buf, fmt, args);) 

(GNU cpp groks it, but what about the others?)

Or you mean the whole body of the implementation of strbuf_addf()?  Now, 
_that_ would be ugly.

Ciao,
Dscho

Re: [PATCH 1/2] Add strbuf_initf()

[Jeff King]

On Thu, Mar 06, 2008 at 11:53:28AM +0100, Johannes Schindelin wrote:

> AFAIR it is unportable to have a macro
> 
> #define strbuf_initf(buf, fmt, ...) \
> 	(strbuf_init(buf, strlen(fmt) + 64), strbuf_addf(buf, fmt, args);) 
> 
> (GNU cpp groks it, but what about the others?)

C99 has variable-argument macros, but C89 does not. I have no idea in
practice how many of the compilers git targets support them (in one form
or another).

-Peff

[PATCH 2/2] format-patch: add --reviewed-by=<ident>

[Johannes Schindelin]

Now you can conveniently add lines with format-patch, adding missing
Reviewed-by: lines (if it is already present at the end of the commit
message, it will not be repeated).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	And this is the actual patch...

	It is just a patch for format-patch... the patch for git-am should 
	be trivial.

 Documentation/git-format-patch.txt |    5 ++++
 builtin-log.c                      |    9 +++++++
 log-tree.c                         |   44 +++++++++++++++++++++++++++++++++++-
 revision.h                         |    1 +
 t/t4014-format-patch.sh            |   26 +++++++++++++++++++++
 5 files changed, 84 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index b5207b7..da52720 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git-format-patch' [-k] [-o <dir> | --stdout] [--thread]
 		   [--attach[=<boundary>] | --inline[=<boundary>]]
+		   [--reviewed-by=<ident>]
 		   [-s | --signoff] [<common diff options>]
 		   [-n | --numbered | -N | --no-numbered]
 		   [--start-number <n>] [--numbered-files]
@@ -96,6 +97,10 @@ include::diff-options.txt[]
 	Do not strip/add '[PATCH]' from the first line of the
 	commit log message.
 
+--reviewed-by=<ident>::
+	Add `Reviewed-by:` line to the commit message, using
+	the given identity.
+
 -s|--signoff::
 	Add `Signed-off-by:` line to the commit message, using
 	the committer identity of yourself.
diff --git a/builtin-log.c b/builtin-log.c
index fe8fc6f..90573b3 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -753,6 +753,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
 	char *add_signoff = NULL;
+	struct path_list add_reviewed_by = { NULL, 0, 0, 0 };
 	struct strbuf buf;
 
 	git_config(git_format_config);
@@ -821,6 +822,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				die("bogos committer info %s\n", committer);
 			add_signoff = xmemdupz(committer, endpos - committer + 1);
 		}
+		else if (!prefixcmp(argv[i], "--reviewed-by=")) {
+			struct strbuf buf;
+
+			strbuf_initf(&buf, "Reviewed-by: %s", argv[i] + 14);
+			path_list_append(strbuf_detach(&buf, NULL),
+					&add_reviewed_by);
+		}
 		else if (!strcmp(argv[i], "--attach")) {
 			rev.mime_boundary = git_version_string;
 			rev.no_inline = 1;
@@ -994,6 +1002,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		start_number--;
 	}
 	rev.add_signoff = add_signoff;
+	rev.add_reviewed_by = &add_reviewed_by;
 	while (0 <= --nr) {
 		int shown;
 		commit = list[nr];
diff --git a/log-tree.c b/log-tree.c
index 608f697..36621ab 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -3,6 +3,7 @@
 #include "commit.h"
 #include "log-tree.h"
 #include "reflog-walk.h"
+#include "path-list.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -79,6 +80,34 @@ static int detect_any_signoff(char *letter, int size)
 	return seen_head && seen_name;
 }
 
+static void append_reviewed_by(struct strbuf *sb, struct path_list *reviewed)
+{
+	struct path_list already_reviewed = { NULL, 0, 0, 0 };
+	int i, eol = sb->len + 1;
+
+	/* find the last Reviewed-by: lines */
+	for (i = eol - 2; i > 0; i--)
+		if (i == 0 || sb->buf[i - 1] == '\n') {
+			if (!prefixcmp(sb->buf + i, "Reviewed-by:"))
+				path_list_insert(xstrndup(sb->buf + i,
+							eol - i - 1),
+						&already_reviewed);
+			else if (eol - i > 1)
+				break;
+			eol = i;
+		}
+
+	for (i = 0; i < reviewed->nr; i++) {
+		const char *line = reviewed->items[i].path;
+		if (!path_list_has_path(&already_reviewed, line))
+			strbuf_addf(sb, "%s\n", line);
+	}
+
+	/* all strings were xstrndup()ed, clean them up */
+	already_reviewed.strdup_paths = 1;
+	path_list_clear(&already_reviewed, 0);
+}
+
 static void append_signoff(struct strbuf *sb, const char *signoff)
 {
 	static const char signed_off_by[] = "Signed-off-by: ";
@@ -137,6 +166,16 @@ static int has_non_ascii(const char *s)
 	return 0;
 }
 
+static int path_list_has_non_ascii(struct path_list *list)
+{
+	int i;
+
+	for (i = 0; list && i < list->nr; i++)
+		if (has_non_ascii(list->items[i].path))
+			return 1;
+	return 0;
+}
+
 void log_write_email_headers(struct rev_info *opt, const char *name,
 			     const char **subject_p, const char **extra_headers_p)
 {
@@ -301,8 +340,11 @@ void show_log(struct rev_info *opt, const char *sep)
 	strbuf_init(&msgbuf, 0);
 	pretty_print_commit(opt->commit_format, commit, &msgbuf,
 			    abbrev, subject, extra_headers, opt->date_mode,
-			    has_non_ascii(opt->add_signoff));
+			    has_non_ascii(opt->add_signoff) ||
+			    path_list_has_non_ascii(opt->add_reviewed_by));
 
+	if (opt->add_reviewed_by && opt->add_reviewed_by->nr)
+		append_reviewed_by(&msgbuf, opt->add_reviewed_by);
 	if (opt->add_signoff)
 		append_signoff(&msgbuf, opt->add_signoff);
 	if (opt->show_log_size)
diff --git a/revision.h b/revision.h
index c8b3b94..eb4cc3b 100644
--- a/revision.h
+++ b/revision.h
@@ -78,6 +78,7 @@ struct rev_info {
 	char		*message_id;
 	const char	*ref_message_id;
 	const char	*add_signoff;
+	struct path_list *add_reviewed_by;
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b2b7a8d..e2ff94f 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -230,4 +230,30 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' '
 
 '
 
+cat > expect << EOF
+
+Reviewed-by: Ken Robinson
+
+Reviewed-by: Sergey Rachmaninov
+
+Reviewed-by: Ronny O Sullivan
+Reviewed-by: Mickey Mouse
+Reviewed-by: Mahatma Gandhi
+EOF
+
+test_expect_success '--reviewed-by' '
+
+	echo reviewed > foo &&
+	test_tick &&
+	git commit -m "Reviewed" -m "Reviewed-by: Ken Robinson" \
+		-m "Reviewed-by: Sergey Rachmaninov" \
+		-m "Reviewed-by: Ronny O Sullivan" foo &&
+	git format-patch --reviewed-by="Mickey Mouse" \
+		--reviewed-by="Sergey Rachmaninov" \
+		--reviewed-by="Mahatma Gandhi" -1 HEAD &&
+	sed -e "1,/^Cc: /d" -e "/^---/,\$d" < 0001-Reviewed.patch > output &&
+	git diff expect output
+
+'
+
 test_done
-- 
1.5.4.3.571.g9aec3

Re: [PATCH 2/2] format-patch: add --reviewed-by=<ident>

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>  'git-format-patch' [-k] [-o <dir> | --stdout] [--thread]
>  		   [--attach[=<boundary>] | --inline[=<boundary>]]
> +		   [--reviewed-by=<ident>]
>  		   [-s | --signoff] [<common diff options>]
>  		   [-n | --numbered | -N | --no-numbered]
>  		   [--start-number <n>] [--numbered-files]

What's the expected workflow this patch intends to help?

 - You see a patch by somebody, you look at it deeply, you apply to your
   tree (presumably with your own Signed-off-by).

 - You inspect the result further, and decide it is good.

 - You format-patch with the option, which would now have a Reviewed-by:
   too.

 - You send it out.

If so, it might make sense to simply always use the committer ident.

If the person who adds the reviewed-by is trusted so much that her
reviewed-by counts, the commits might even be transfered with "Please
pull".  In such a case, the workflow might become:

 - You see a patch by somebody, you look at it deeply, you apply to your
   tree (presumably with your own Signed-off-by).

 - You inspect the result further, and decide it is good.

 - You run "rebase --add-reviewed-by" to prepare a series on a branch to
   be pulled from.

 - You send a request-pull.

In that workflow, it would also make sense to use the committer ident.

I am trying to come up with a plausible workflow that wants to add
somebody else's reviewed-by.

 - You send out your patch to the list.  People give comments, you reroll,
   you get more comments, eventually people say "Ah, that's good, Ack."
   and/or "I am not the primary person who knows this area, but I reviewed
   it and I know my reviewed-by would count, so here is my Ok".

 - You format-patch the final version, with Acked-by and Reviewed-by
   adding other people's names.

Then I think it makes sense to take names of other people if that is the
case.

You probably meant that that is the expected workflow, as you can give
more than one of these options.

But people who read the documentation should not have to guess.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index b2b7a8d..e2ff94f 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -230,4 +230,30 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' '
>  
>  '
>  
> +cat > expect << EOF
> +
> +Reviewed-by: Ken Robinson
> +
> +Reviewed-by: Sergey Rachmaninov
> +
> +Reviewed-by: Ronny O Sullivan
> +Reviewed-by: Mickey Mouse
> +Reviewed-by: Mahatma Gandhi
> +EOF
> +
> +test_expect_success '--reviewed-by' '
> +
> +	echo reviewed > foo &&
> +	test_tick &&
> +	git commit -m "Reviewed" -m "Reviewed-by: Ken Robinson" \
> +		-m "Reviewed-by: Sergey Rachmaninov" \
> +		-m "Reviewed-by: Ronny O Sullivan" foo &&
> +	git format-patch --reviewed-by="Mickey Mouse" \
> +		--reviewed-by="Sergey Rachmaninov" \
> +		--reviewed-by="Mahatma Gandhi" -1 HEAD &&
> +	sed -e "1,/^Cc: /d" -e "/^---/,\$d" < 0001-Reviewed.patch > output &&
> +	git diff expect output
> +
> +'

Why not use a single -m for the first three reviewed-bys, instead of
making them into separate paragraphs using multiple -m?

Re: [PATCH 2/2] format-patch: add --reviewed-by=<ident>

[Johannes Schindelin]

Hi,

On Wed, 5 Mar 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >  'git-format-patch' [-k] [-o <dir> | --stdout] [--thread]
> >  		   [--attach[=<boundary>] | --inline[=<boundary>]]
> > +		   [--reviewed-by=<ident>]
> >  		   [-s | --signoff] [<common diff options>]
> >  		   [-n | --numbered | -N | --no-numbered]
> >  		   [--start-number <n>] [--numbered-files]
> 
> What's the expected workflow this patch intends to help?
> 
> [...]
>
> I am trying to come up with a plausible workflow that wants to add 
> somebody else's reviewed-by.
> 
>  - You send out your patch to the list.  People give comments, you 
>    reroll, you get more comments, eventually people say "Ah, that's 
>    good, Ack."  and/or "I am not the primary person who knows this area, 
>    but I reviewed it and I know my reviewed-by would count, so here is 
>    my Ok".
> 
>  - You format-patch the final version, with Acked-by and Reviewed-by 
>    adding other people's names.
> 
> Then I think it makes sense to take names of other people if that is the
> case.

Yes, exactly.  That was the usage I had in mind.

The problem with the two others you suggested (basically that the reviewer 
herself sends out another patch series) is a little fragile, since a 
malicous reviewer could add/modify code, but for nice reviewers, it is 
better in that you know which code was reviewed.

So I'd rather have it not resent by the reviewer as a patch series, but I 
think that pulling it is fine, since it is much easier to verify (with log 
--cherry-pick) that no code was tampered with by the reviewer.

> You probably meant that that is the expected workflow, as you can give 
> more than one of these options.
> 
> But people who read the documentation should not have to guess.

Frankly, I did not make up my mind on the other workflows, I only wanted 
to make the maintainer's life (yours) easier.

I have no problem posting several revisions of this patch, or scrap it 
altogether... as somebody recently said, we even joke around here via 
patches ;-)

> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index b2b7a8d..e2ff94f 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -230,4 +230,30 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' '
> >  
> >  '
> >  
> > +cat > expect << EOF
> > +
> > +Reviewed-by: Ken Robinson
> > +
> > +Reviewed-by: Sergey Rachmaninov
> > +
> > +Reviewed-by: Ronny O Sullivan
> > +Reviewed-by: Mickey Mouse
> > +Reviewed-by: Mahatma Gandhi
> > +EOF
> > +
> > +test_expect_success '--reviewed-by' '
> > +
> > +	echo reviewed > foo &&
> > +	test_tick &&
> > +	git commit -m "Reviewed" -m "Reviewed-by: Ken Robinson" \
> > +		-m "Reviewed-by: Sergey Rachmaninov" \
> > +		-m "Reviewed-by: Ronny O Sullivan" foo &&
> > +	git format-patch --reviewed-by="Mickey Mouse" \
> > +		--reviewed-by="Sergey Rachmaninov" \
> > +		--reviewed-by="Mahatma Gandhi" -1 HEAD &&
> > +	sed -e "1,/^Cc: /d" -e "/^---/,\$d" < 0001-Reviewed.patch > output &&
> > +	git diff expect output
> > +
> > +'
> 
> Why not use a single -m for the first three reviewed-bys, instead of
> making them into separate paragraphs using multiple -m?

I am always unsure how to embed newlines in strings in a shell script 
(except with single-ticks), and besides, I do not like breaking the 
indentation.  But I could make it a temporary file... Hmm?

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Sixt]

On Wednesday 05 March 2008 23:18, Johannes Schindelin wrote:
> On Wed, 5 Mar 2008, Johannes Sixt wrote:
> > I've now created compat/mingw.h. Below is the interdiff. Of course, it
> > mostly only moves code around. But you might want to look at at mkstemp,
> > PATH_SEP, PRIuMAX, and has_dos_drive_prefix. Notice also that I include
> > compat/mingw.h early in git-compat-util.h instead of late.
>
> Thanks!
>
> With this, I think I have nothing to add to your series (except maybe an
> Acked-by: where it applies, or a Reviewed-by:, but I think that this
> would only be a burden on our maintainer).

Thank you!

I now wait for vsnprintf patches and the recent start_command patch to hit 
master, then I'll do a final rebase and fix-up before I consider the series 
ready for inclusion into git.git.

-- Hannes

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Sixt]

On Wednesday 05 March 2008 22:21, Johannes Sixt wrote:
> I've now created compat/mingw.h. Below is the interdiff. Of course, it
> mostly only moves code around. But you might want to look at at mkstemp,
> PATH_SEP, PRIuMAX, and has_dos_drive_prefix. Notice also that I include
> compat/mingw.h early in git-compat-util.h instead of late.

I must admit I was very sloppy with the previous round. I had to make 
has_dos_drive_prefix a macro; otherwise we would get numerous warnings 
about "undeclared function isalpha", because the declaration appears later in 
git-compat-util.h.

On the positive side, we can now reuse Michal's vsnprintf wrapper, which fixes 
snprintf, too, (which was not the case previously). Note that on Windows we 
have to adjust the size parameter.

There's also a change in the setup of stderr in start_command() that 
corresponds to ce2cf27adc. And I made is_dir_sep into a conditional macro 
similar to has_dos_drive_prefix to get rid of another #ifdef/#endif.

-- Hannes

Here's the interdiff:

diff --git a/Makefile b/Makefile
index 68d60e7..6619523 100644
--- a/Makefile
+++ b/Makefile
@@ -309,7 +309,7 @@ LIB_H = \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \
 	utf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \
 	mailmap.h remote.h parse-options.h transport.h diffcore.h hash.h fsck.h \
-	pack-revindex.h
+	pack-revindex.h compat/mingw.h
 
 DIFF_OBJS = \
 	diff.o diff-lib.o diffcore-break.o diffcore-order.o \
@@ -549,10 +549,12 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_C99_FORMAT = YesPlease
 	NO_STRTOUMAX = YesPlease
 	NO_MKDTEMP = YesPlease
+	SNPRINTF_RETURNS_BOGUS = YesPlease
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
+	COMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o
 	EXTLIBS += -lws2_32
diff --git a/compat/mingw.c b/compat/mingw.c
index 6733727..7c8fd0e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -847,40 +847,6 @@ int mingw_rename(const char *pold, const char *pnew)
 	return -1;
 }
 
-#undef vsnprintf
-/* Note that the size parameter specifies the available space, i.e.
- * includes the trailing NUL byte; but Windows's vsnprintf expects the
- * number of characters to write without the trailing NUL.
- */
-
-/* This is out of line because it uses alloca() behind the scenes,
- * which must not be called in a loop (alloca() reclaims the allocations
- * only at function exit).
- */
-static int try_vsnprintf(size_t size, const char *fmt, va_list args)
-{
-	char buf[size];	/* gcc-ism */
-	return vsnprintf(buf, size-1, fmt, args);
-}
-
-int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
-{
-	int len;
-	if (size > 0) {
-		len = vsnprintf(buf, size-1, fmt, args);
-		if (len >= 0)
-			return len;
-	}
-	/* ouch, buffer too small; need to compute the size */
-	if (size < 250)
-		size = 250;
-	do {
-		size *= 4;
-		len = try_vsnprintf(size, fmt, args);
-	} while (len < 0);
-	return len;
-}
-
 struct passwd *getpwuid(int uid)
 {
 	static char user_name[100];
diff --git a/compat/mingw.h b/compat/mingw.h
index d92c631..c7db345 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -174,9 +174,6 @@ int mingw_fstat(int fd, struct mingw_stat *buf);
 static inline int mingw_stat(const char *file_name, struct mingw_stat *buf)
 { return mingw_lstat(file_name, buf); }
 
-int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
-#define vsnprintf mingw_vsnprintf
-
 pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
 void mingw_execvp(const char *cmd, char *const *argv);
 #define execvp mingw_execvp
@@ -192,12 +189,8 @@ sig_handler_t mingw_signal(int sig, sig_handler_t 
handler);
  * git specific compatibility
  */
 
-static inline int has_dos_drive_prefix(const char *path)
-{
-	return isalpha(*path) && path[1] == ':';
-}
-#define has_dos_drive_prefix has_dos_drive_prefix
-
+#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 #define PATH_SEP ';'
 #define PRIuMAX "I64u"
 
diff --git a/compat/snprintf.c b/compat/snprintf.c
index dbfc2d6..480b66f 100644
--- a/compat/snprintf.c
+++ b/compat/snprintf.c
@@ -1,12 +1,21 @@
 #include "../git-compat-util.h"
 
+/*
+ * The size parameter specifies the available space, i.e. includes
+ * the trailing NUL byte; but Windows's vsnprintf expects the
+ * number of characters to write without the trailing NUL.
+ */
+#ifndef SNPRINTF_SIZE_CORR
+#define SNPRINTF_SIZE_CORR 0
+#endif
+
 #undef vsnprintf
 int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
 {
 	char *s;
 	int ret;
 
-	ret = vsnprintf(str, maxsize, format, ap);
+	ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
 	if (ret != -1)
 		return ret;
 
@@ -20,7 +29,7 @@ int git_vsnprintf(char *str, size_t maxsize, const char 
*format, va_list ap)
 		if (! str)
 			break;
 		s = str;
-		ret = vsnprintf(str, maxsize, format, ap);
+		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
 	}
 	free(s);
 	return ret;
diff --git a/git-compat-util.h b/git-compat-util.h
index 08f764e..2889146 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -117,6 +117,10 @@
 #define has_dos_drive_prefix(path) 0
 #endif
 
+#ifndef is_dir_sep
+#define is_dir_sep(c) ((c) == '/')
+#endif
+
 #ifdef __GNUC__
 #define NORETURN __attribute__((__noreturn__))
 #else
diff --git a/run-command.c b/run-command.c
index aba2bf2..2ce8c2b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -132,6 +132,14 @@ int start_command(struct child_process *cmd)
 		dup2(cmd->in, 0);
 	}
 
+	if (cmd->no_stderr) {
+		s2 = dup(2);
+		dup_devnull(2);
+	} else if (need_err) {
+		s2 = dup(2);
+		dup2(fderr[1], 2);
+	}
+
 	if (cmd->no_stdout) {
 		s1 = dup(1);
 		dup_devnull(1);
@@ -146,14 +154,6 @@ int start_command(struct child_process *cmd)
 		dup2(cmd->out, 1);
 	}
 
-	if (cmd->no_stderr) {
-		s2 = dup(2);
-		dup_devnull(2);
-	} else if (need_err) {
-		s2 = dup(2);
-		dup2(fderr[1], 2);
-	}
-
 	if (cmd->dir)
 		die("chdir in start_command() not implemented");
 	if (cmd->env) {
diff --git a/setup.c b/setup.c
index d1a862e..eea1038 100644
--- a/setup.c
+++ b/setup.c
@@ -4,12 +4,6 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
-#ifdef __MINGW32__
-static inline int is_dir_sep(char c) { return c == '/' || c == '\\'; }
-#else
-static inline int is_dir_sep(char c) { return c == '/'; }
-#endif
-
 static int sanitary_path_copy(char *dst, const char *src)
 {
 	char *dst0;

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Tue, 11 Mar 2008, Johannes Sixt wrote:

> On the positive side, we can now reuse Michal's vsnprintf wrapper, which 
> fixes snprintf, too, (which was not the case previously). Note that on 
> Windows we have to adjust the size parameter.

Actually, I have to admit that I like strbuf_vaddf() more and more.  
Anything you would wish me to implement there?

Ciao,
Dscho

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Sixt]

On Wednesday 12 March 2008 00:28, Johannes Schindelin wrote:
> On Tue, 11 Mar 2008, Johannes Sixt wrote:
> > On the positive side, we can now reuse Michal's vsnprintf wrapper, which
> > fixes snprintf, too, (which was not the case previously). Note that on
> > Windows we have to adjust the size parameter.
>
> Actually, I have to admit that I like strbuf_vaddf() more and more.
> Anything you would wish me to implement there?

Only that you keep an eye on PRIuMAX; if it is ever used in a format string of 
strbuf_vaddf() then you ought to implement %I64u as well as %llu. ;)

-- Hannes

Re: [PATCH 03/40] Add target architecture MinGW.

[Johannes Schindelin]

Hi,

On Wed, 12 Mar 2008, Johannes Sixt wrote:

> On Wednesday 12 March 2008 00:28, Johannes Schindelin wrote:
> > On Tue, 11 Mar 2008, Johannes Sixt wrote:
> > > On the positive side, we can now reuse Michal's vsnprintf wrapper, 
> > > which fixes snprintf, too, (which was not the case previously). Note 
> > > that on Windows we have to adjust the size parameter.
> >
> > Actually, I have to admit that I like strbuf_vaddf() more and more. 
> > Anything you would wish me to implement there?
> 
> Only that you keep an eye on PRIuMAX; if it is ever used in a format 
> string of strbuf_vaddf() then you ought to implement %I64u as well as 
> %llu. ;)

Sure!

Ciao,
Dscho

[PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Documentation/git.txt |    6 +++---
 exec_cmd.c            |    4 ++++
 sha1_file.c           |    4 ++++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index d57bed6..b2a5b60 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -382,9 +382,9 @@ git so take care if using Cogito etc.
 'GIT_ALTERNATE_OBJECT_DIRECTORIES'::
 	Due to the immutable nature of git objects, old objects can be
 	archived into shared, read-only directories. This variable
-	specifies a ":" separated list of git object directories which
-	can be used to search for git objects. New objects will not be
-	written to these directories.
+	specifies a ":" separated (on Windows ";" separated) list
+	of git object directories which can be used to search for git
+	objects. New objects will not be written to these directories.
 
 'GIT_DIR'::
 	If the 'GIT_DIR' environment variable is set then it
diff --git a/exec_cmd.c b/exec_cmd.c
index e189cac..343545d 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -37,7 +37,11 @@ static void add_path(struct strbuf *out, const char *path)
 		else
 			strbuf_addstr(out, make_absolute_path(path));
 
+#ifdef __MINGW32__
+		strbuf_addch(out, ';');
+#else
 		strbuf_addch(out, ':');
+#endif
 	}
 }
 
diff --git a/sha1_file.c b/sha1_file.c
index 1ddb96b..453bc43 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -389,7 +389,11 @@ void prepare_alt_odb(void)
 	if (!alt) alt = "";
 
 	alt_odb_tail = &alt_odb_list;
+#ifdef __MINGW32__
+	link_alt_odb_entries(alt, alt + strlen(alt), ';', NULL, 0);
+#else
 	link_alt_odb_entries(alt, alt + strlen(alt), ':', NULL, 0);
+#endif
 
 	read_info_alternates(get_object_directory(), 0);
 }
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Paolo Bonzini]

> +#ifdef __MINGW32__
> +		strbuf_addch(out, ';');
> +#else
>  		strbuf_addch(out, ':');
> +#endif

Why not adding a PATH_SEPARATOR #define?

Paolo

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Johannes Sixt]

On Thursday 28 February 2008 10:25, Paolo Bonzini wrote:
> > +#ifdef __MINGW32__
> > +		strbuf_addch(out, ';');
> > +#else
> >  		strbuf_addch(out, ':');
> > +#endif
>
> Why not adding a PATH_SEPARATOR #define?

Because IMO it is obfuscating:

1. When you read through the code and see PATH_SEPARATOR, you still have
to go look how it's defined. Why? Because you always will ask: Is this 
about ':' vs. ';' or '/' vs. '\\'?

2. When you look for where ":" or ";" are treated, you'll end up at the
#define. Then you need an extra step to search for PATH_SEPARATOR.

-- Hannes

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 10:25, Paolo Bonzini wrote:
> > > +#ifdef __MINGW32__
> > > +		strbuf_addch(out, ';');
> > > +#else
> > >  		strbuf_addch(out, ':');
> > > +#endif
> >
> > Why not adding a PATH_SEPARATOR #define?
> 
> Because IMO it is obfuscating:
> 
> 1. When you read through the code and see PATH_SEPARATOR, you still have
> to go look how it's defined. Why? Because you always will ask: Is this 
> about ':' vs. ';' or '/' vs. '\\'?
> 
> 2. When you look for where ":" or ";" are treated, you'll end up at the
> #define. Then you need an extra step to search for PATH_SEPARATOR.

IMO these are not good arguments.  If they were, we could not typedef 
size_t and socklen_t, either.

Ciao,
Dscho

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Paolo Bonzini]

Johannes Sixt wrote:
> On Thursday 28 February 2008 10:25, Paolo Bonzini wrote:
>>> +#ifdef __MINGW32__
>>> +		strbuf_addch(out, ';');
>>> +#else
>>>  		strbuf_addch(out, ':');
>>> +#endif
>> Why not adding a PATH_SEPARATOR #define?
> 
> Because IMO it is obfuscating:
> 
> 1. When you read through the code and see PATH_SEPARATOR, you still have
> to go look how it's defined. Why? Because you always will ask: Is this 
> about ':' vs. ';' or '/' vs. '\\'?

One is a DIR_SEPARATOR, the other is a PATH_SEPARATOR.  It's a matter of 
conventions.

Paolo

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Johannes Schindelin]

Hi,

On Fri, 29 Feb 2008, Paolo Bonzini wrote:

> Johannes Sixt wrote:
> > On Thursday 28 February 2008 10:25, Paolo Bonzini wrote:
> > > > +#ifdef __MINGW32__
> > > > +		strbuf_addch(out, ';');
> > > > +#else
> > > >  		strbuf_addch(out, ':');
> > > > +#endif
> > > Why not adding a PATH_SEPARATOR #define?
> > 
> > Because IMO it is obfuscating:
> > 
> > 1. When you read through the code and see PATH_SEPARATOR, you still have
> > to go look how it's defined. Why? Because you always will ask: Is this about
> > ':' vs. ';' or '/' vs. '\\'?
> 
> One is a DIR_SEPARATOR, the other is a PATH_SEPARATOR.  It's a matter of
> conventions.

I agree that it would be good, also for documentation purposes, to add the 
#defines.

However, there is something ugly waiting for us: we often have

	case '/':

and for Windows, this needs to add

	case '\\':

Now, we could #define CASE_DIR_SEPARATOR, which expands to "case '/': case 
'\\':" on Windows, but I am prepared to let Windows uglify our source code 
only so far *does the famous imitation of Linus where he puts two finger 
so close that you cannot see between them*.

Ciao,
Dscho

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Paolo Bonzini]

> However, there is something ugly waiting for us: we often have
> 
> 	case '/':
> 
> and for Windows, this needs to add
> 
> 	case '\\':

More often than in an "if" statement (so you can add IS_DIR_SEPARATOR 
which is much less ugly than CASE_DIR_SEPARATOR)?

Paolo

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Johannes Schindelin]

Hi,

On Fri, 29 Feb 2008, Paolo Bonzini wrote:

> > However, there is something ugly waiting for us: we often have
> > 
> > 	case '/':
> > 
> > and for Windows, this needs to add
> > 
> > 	case '\\':
> 
> More often than in an "if" statement (so you can add IS_DIR_SEPARATOR 
> which is much less ugly than CASE_DIR_SEPARATOR)?

I have not grepped through the patches (as yourself, I am too lazy), but I 
remember seeing the "case" case in two instances, and I do not remember 
seeing the "if" case.

In the end, I think suggesting something like PATH_SEPARATOR is good.  But 
insisting on it without working on it is less good.

So I think that it is up to Hannes now; he got enough input, and he has a 
fine sense of taste, so he will do the Right Thing.

Ciao,
Dscho

Re: [PATCH 04/40] Windows: Use the Windows style PATH separator ';'.

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> diff --git a/exec_cmd.c b/exec_cmd.c
> index e189cac..343545d 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -37,7 +37,11 @@ static void add_path(struct strbuf *out, const char *path)
>  		else
>  			strbuf_addstr(out, make_absolute_path(path));
>  
> +#ifdef __MINGW32__
> +		strbuf_addch(out, ';');
> +#else
>  		strbuf_addch(out, ':');
> +#endif
>  	}
>  }

If it were only these two places it might be Ok, but together
with 10/40 you may want to do

        #if WINDOWS
        #define PATH_SEP ';'
        #else
        #define PATH_SEP ':'
        #endif

in a header so that the C files do not have to suffer.

[PATCH 05/40] Windows: Strip ".exe" from the program name.

[Johannes Sixt]

Before we can successfully parse a builtin command from the program name
we must strip off unneeded parts, that is, the file extension.
Additionally, we must take Windows style path names into account.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile |    1 +
 git.c    |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index bc8a487..069939f 100644
--- a/Makefile
+++ b/Makefile
@@ -543,6 +543,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_PERL_MAKEMAKER = YesPlease
 	NO_EXTRA_PROGRAMS = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
+	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o
 	EXTLIBS += -lws2_32
 	X = .exe
diff --git a/git.c b/git.c
index 0cb8688..32166a4 100644
--- a/git.c
+++ b/git.c
@@ -372,6 +372,15 @@ static void handle_internal_command(int argc, const char **argv)
 	};
 	int i;
 
+#ifdef STRIP_EXTENSION
+	i = strlen(argv[0]) - strlen(STRIP_EXTENSION);
+	if (i > 0 && !strcmp(argv[0] + i, STRIP_EXTENSION)) {
+		char *argv0 = strdup(argv[0]);
+		argv[0] = cmd = argv0;
+		argv0[i] = '\0';
+	}
+#endif
+
 	/* Turn "git cmd --help" into "git help cmd" */
 	if (argc > 1 && !strcmp(argv[1], "--help")) {
 		argv[1] = argv[0];
@@ -398,6 +407,11 @@ int main(int argc, const char **argv)
 	 * name, and the dirname as the default exec_path
 	 * if we don't have anything better.
 	 */
+#ifdef __MINGW32__
+	char *bslash = strrchr(cmd, '\\');
+	if (!slash || (bslash && bslash > slash))
+		slash = bslash;
+#endif
 	if (slash) {
 		*slash++ = 0;
 		cmd_path = cmd;
-- 
1.5.4.1.126.ge5a7d

[PATCH 06/40] Windows: Implement a wrapper of the open() function.

[Johannes Sixt]

The wrapper does two things:
- Requests to open /dev/null are redirected to open the nul pseudo file.
- A request to open a file that currently exists as a directory on
  Windows fails with EACCES; this is changed to EISDIR.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |   20 ++++++++++++++++++++
 git-compat-util.h |    7 +++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 0c87e43..faa6df3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2,6 +2,26 @@
 
 unsigned int _CRT_fmode = _O_BINARY;
 
+#undef open
+int mingw_open (const char *filename, int oflags, ...)
+{
+	va_list args;
+	unsigned mode;
+	va_start(args, oflags);
+	mode = va_arg(args, int);
+	va_end(args);
+
+	if (!strcmp(filename, "/dev/null"))
+		filename = "nul";
+	int fd = open(filename, oflags, mode);
+	if (fd < 0 && (oflags & O_CREAT) && errno == EACCES) {
+		DWORD attrs = GetFileAttributes(filename);
+		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
+			errno = EISDIR;
+	}
+	return fd;
+}
+
 unsigned int sleep (unsigned int seconds)
 {
 	Sleep(seconds*1000);
diff --git a/git-compat-util.h b/git-compat-util.h
index b1f3f92..672c074 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -577,6 +577,13 @@ struct passwd *getpwuid(int uid);
 int setitimer(int type, struct itimerval *in, struct itimerval *out);
 int sigaction(int sig, struct sigaction *in, struct sigaction *out);
 
+/*
+ * replacements of existing functions
+ */
+
+int mingw_open (const char *filename, int oflags, ...);
+#define open mingw_open
+
 #endif /* __MINGW32__ */
 
 #endif
-- 
1.5.4.1.126.ge5a7d

[PATCH 07/40] Windows: A minimal implemention of getpwuid().

[Johannes Sixt]

getpwuid() is implemented just enough that GIT does not issue errors.
Since the information that it returns is not very useful, users are
required to set up user.name and user.email configuration.

All uses of getpwuid() are like getpwuid(getuid()), hence, the return value
of getuid() is irrelevant and the uid parameter is not even looked at.

Side note: getpwnam() is only used to resolve '~' and '~username' paths,
which is an idiom not known on Windows, hence, we don't implement it.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index faa6df3..0c1d0e4 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -62,7 +62,15 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
 
 struct passwd *getpwuid(int uid)
 {
+	static char user_name[100];
 	static struct passwd p;
+
+	DWORD len = sizeof(user_name);
+	if (!GetUserName(user_name, &len))
+		return NULL;
+	p.pw_name = user_name;
+	p.pw_gecos = "unknown";
+	p.pw_dir = NULL;
 	return &p;
 }
 
-- 
1.5.4.1.126.ge5a7d

[PATCH 08/40] Windows: always chmod(, 0666) before unlink().

[Johannes Sixt]

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>

On Windows, a read-only files cannot be deleted. To make sure that
deletion does not fail because of this, always call chmod() before
unlink().

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-compat-util.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 672c074..06ac2c1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -551,6 +551,14 @@ static inline int mingw_mkdir(const char *path, int mode)
 }
 #define mkdir mingw_mkdir
 
+static inline int mingw_unlink(const char *pathname)
+{
+	/* read-only files cannot be removed */
+	chmod(pathname, 0666);
+	return unlink(pathname);
+}
+#define unlink mingw_unlink
+
 static inline int waitpid(pid_t pid, unsigned *status, unsigned options)
 {
 	if (options == 0)
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 08/40] Windows: always chmod(, 0666) before unlink().

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> On Windows, a read-only files cannot be deleted. To make sure that
> deletion does not fail because of this, always call chmod() before
> unlink().

May I request that this embarrassing typo be fixed before applying? ("a 
read-only files" -> "read-only files".)

Thanks,
Dscho

[PATCH 09/40] Windows: Work around misbehaved rename().

[Johannes Sixt]

Windows's rename() is based on the MoveFile() API, which fails if the
destination exists. Here we work around the problem by using MoveFileEx().
Furthermore, the posixly correct error is returned if the destination is
a directory.

The implementation is still slightly incomplete, however, because of the
missing error code translation: We assume that the failure is due to
permissions.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |   25 +++++++++++++++++++++++++
 git-compat-util.h |    3 +++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 0c1d0e4..733ef87 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -60,6 +60,31 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
 	return result;
 }
 
+#undef rename
+int mingw_rename(const char *pold, const char *pnew)
+{
+	/*
+	 * Try native rename() first to get errno right.
+	 * It is based on MoveFile(), which cannot overwrite existing files.
+	 */
+	if (!rename(pold, pnew))
+		return 0;
+	if (errno != EEXIST)
+		return -1;
+	if (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))
+		return 0;
+	/* TODO: translate more errors */
+	if (GetLastError() == ERROR_ACCESS_DENIED) {
+		DWORD attrs = GetFileAttributes(pnew);
+		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY)) {
+			errno = EISDIR;
+			return -1;
+		}
+	}
+	errno = EACCES;
+	return -1;
+}
+
 struct passwd *getpwuid(int uid)
 {
 	static char user_name[100];
diff --git a/git-compat-util.h b/git-compat-util.h
index 06ac2c1..f44a287 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -592,6 +592,9 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out);
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
+int mingw_rename(const char*, const char*);
+#define rename mingw_rename
+
 #endif /* __MINGW32__ */
 
 #endif
-- 
1.5.4.1.126.ge5a7d

[PATCH 10/40] Windows: Treat Windows style path names.

[Johannes Sixt]

GIT's guts work with a forward slash as a path separators. We do not change
that. Rather we make sure that only "normalized" paths enter the depths
of the machinery.

We have to translate backslashes to forward slashes in the prefix and in
command line arguments. Fortunately, all of them are passed through
functions in setup.c.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 cache.h           |    4 +++
 compat/mingw.c    |   16 ++++++++++++++
 git-compat-util.h |    3 ++
 setup.c           |   59 +++++++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index e1000bc..3e4e10a 100644
--- a/cache.h
+++ b/cache.h
@@ -441,7 +441,11 @@ int safe_create_leading_directories(char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
+#ifndef __MINGW32__
 	return path[0] == '/';
+#else
+	return path[0] == '/' || (path[0] && path[1] == ':');
+#endif
 }
 const char *make_absolute_path(const char *path);
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 733ef87..71bca96 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -60,6 +60,22 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
 	return result;
 }
 
+#undef getcwd
+char *mingw_getcwd(char *pointer, int len)
+{
+	char *ret = getcwd(pointer, len);
+	if (!ret)
+		return ret;
+	if (pointer[0] != 0 && pointer[1] == ':') {
+		int i;
+		for (i = 2; pointer[i]; i++)
+			/* Thanks, Bill. You'll burn in hell for that. */
+			if (pointer[i] == '\\')
+				pointer[i] = '/';
+	}
+	return ret;
+}
+
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index f44a287..3b57464 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -592,6 +592,9 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out);
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
+char *mingw_getcwd(char *pointer, int len);
+#define getcwd mingw_getcwd
+
 int mingw_rename(const char*, const char*);
 #define rename mingw_rename
 
diff --git a/setup.c b/setup.c
index dc247a8..77cc461 100644
--- a/setup.c
+++ b/setup.c
@@ -4,13 +4,26 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
+#ifdef __MINGW32__
+static inline int is_dir_sep(char c) { return c == '/' || c == '\\'; }
+#else
+static inline int is_dir_sep(char c) { return c == '/'; }
+#endif
+
 static int sanitary_path_copy(char *dst, const char *src)
 {
 	char *dst0 = dst;
 
-	if (*src == '/') {
+#ifdef __MINGW32__
+	if (isalpha(*src) && src[1] == ':') {
+		*dst++ = *src++;
+		*dst++ = *src++;
+		dst0 = dst;
+	}
+#endif
+	if (is_dir_sep(*src)) {
 		*dst++ = '/';
-		while (*src == '/')
+		while (is_dir_sep(*src))
 			src++;
 	}
 
@@ -32,9 +45,12 @@ static int sanitary_path_copy(char *dst, const char *src)
 				src++;
 				break;
 			case '/':
+#ifdef __MINGW32__
+			case '\\':
+#endif
 				/* (2) */
 				src += 2;
-				while (*src == '/')
+				while (is_dir_sep(*src))
 					src++;
 				continue;
 			case '.':
@@ -44,9 +60,12 @@ static int sanitary_path_copy(char *dst, const char *src)
 					src += 2;
 					goto up_one;
 				case '/':
+#ifdef __MINGW32__
+				case '\\':
+#endif
 					/* (4) */
 					src += 3;
-					while (*src == '/')
+					while (is_dir_sep(*src))
 						src++;
 					goto up_one;
 				}
@@ -54,11 +73,11 @@ static int sanitary_path_copy(char *dst, const char *src)
 		}
 
 		/* copy up to the next '/', and eat all '/' */
-		while ((c = *src++) != '\0' && c != '/')
+		while ((c = *src++) != '\0' && !is_dir_sep(c))
 			*dst++ = c;
-		if (c == '/') {
-			*dst++ = c;
-			while (c == '/')
+		if (is_dir_sep(c)) {
+			*dst++ = '/';
+			while (is_dir_sep(c))
 				c = *src++;
 			src--;
 		} else if (!c)
@@ -77,7 +96,7 @@ static int sanitary_path_copy(char *dst, const char *src)
 			if (dst <= dst0)
 				break;
 			c = *dst--;
-			if (c == '/') {
+			if (c == '/') {	/* MinGW: cannot be '\\' anymore */
 				dst += 2;
 				break;
 			}
@@ -126,10 +145,23 @@ const char *prefix_path(const char *prefix, int len, const char *path)
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
 	static char path[PATH_MAX];
+#ifndef __MINGW32__
 	if (!pfx || !*pfx || is_absolute_path(arg))
 		return arg;
 	memcpy(path, pfx, pfx_len);
 	strcpy(path + pfx_len, arg);
+#else
+	char *p;
+	/* don't add prefix to absolute paths, but still replace '\' by '/' */
+	if (is_absolute_path(arg))
+		pfx_len = 0;
+	else
+		memcpy(path, pfx, pfx_len);
+	strcpy(path + pfx_len, arg);
+	for (p = path + pfx_len; *p; p++)
+		if (*p == '\\')
+			*p = '/';
+#endif
 	return path;
 }
 
@@ -322,6 +354,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
 	int len, offset;
+	int minoffset = 0;
 
 	/*
 	 * If GIT_DIR is set explicitly, we're not going
@@ -364,6 +397,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	if (!getcwd(cwd, sizeof(cwd)-1))
 		die("Unable to read current working directory");
+#ifdef __MINGW32__
+	if (cwd[1] == ':')
+		minoffset = 2;
+#endif
 
 	/*
 	 * Test in the following order (relative to the cwd):
@@ -388,7 +425,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		}
 		chdir("..");
 		do {
-			if (!offset) {
+			if (offset <= minoffset) {
 				if (nongit_ok) {
 					if (chdir(cwd))
 						die("Cannot come back to cwd");
@@ -397,7 +434,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				}
 				die("Not a git repository");
 			}
-		} while (cwd[--offset] != '/');
+		} while (offset > minoffset && cwd[--offset] != '/');
 	}
 
 	inside_git_dir = 0;
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 10/40] Windows: Treat Windows style path names.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> diff --git a/cache.h b/cache.h
> index e1000bc..3e4e10a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -441,7 +441,11 @@ int safe_create_leading_directories(char *path);
>  char *enter_repo(char *path, int strict);
>  static inline int is_absolute_path(const char *path)
>  {
> +#ifndef __MINGW32__
>  	return path[0] == '/';
> +#else
> +	return path[0] == '/' || (path[0] && path[1] == ':');

Maybe "(isalpha(path[0]) && ...)"?

And maybe make this an inline function in git-compat-util.h, like this?

static inline int has_dos_drive_prefix(const char *path)
{
#ifdef __MINGW32__
	return isalpha(*path) && path[1] == ':';
#else
	return 0;
#endif
}

> diff --git a/setup.c b/setup.c
> index dc247a8..77cc461 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -4,13 +4,26 @@
>  static int inside_git_dir = -1;
>  static int inside_work_tree = -1;
>  
> +#ifdef __MINGW32__
> +static inline int is_dir_sep(char c) { return c == '/' || c == '\\'; }
> +#else
> +static inline int is_dir_sep(char c) { return c == '/'; }
> +#endif

I think if you rename it to is_dir_separator(), you can put it into 
git-compat-util.h, right after the PATH_SEPARATOR Paolo suggested.

Thanks,
Dscho

[PATCH 11/40] Windows: Handle absolute paths in safe_create_leading_directories().

[Johannes Sixt]

In this function we must be careful to handle drive-local paths else there
is a danger that it runs into an infinite loop.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 sha1_file.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 453bc43..0c60849 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -83,14 +83,20 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)
 	return 0;
 }
 
+static inline int offset_1st_component(const char *path)
+{
+#ifdef __MINGW32__
+	if (isalpha(path[0]) && path[1] == ':')
+		return 2 + (path[2] == '/');
+#endif
+	return *path == '/';
+}
+
 int safe_create_leading_directories(char *path)
 {
-	char *pos = path;
+	char *pos = path + offset_1st_component(path);
 	struct stat st;
 
-	if (is_absolute_path(path))
-		pos++;
-
 	while (pos) {
 		pos = strchr(pos, '/');
 		if (!pos)
-- 
1.5.4.1.126.ge5a7d

[PATCH 12/40] Windows: Implement gettimeofday().

[Johannes Sixt]

For this purpose we make my_mktime of date.c public and use it to convert
a struct tm to a time_t.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c |   14 ++++++++++++++
 date.c         |    2 +-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 71bca96..24f783b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -38,6 +38,20 @@ int mkstemp(char *template)
 
 int gettimeofday(struct timeval *tv, void *tz)
 {
+	extern time_t my_mktime(struct tm *tm);
+	SYSTEMTIME st;
+	struct tm tm;
+	GetSystemTime(&st);
+	tm.tm_year = st.wYear-1900;
+	tm.tm_mon = st.wMonth-1;
+	tm.tm_mday = st.wDay;
+	tm.tm_hour = st.wHour;
+	tm.tm_min = st.wMinute;
+	tm.tm_sec = st.wSecond;
+	tv->tv_sec = my_mktime(&tm);
+	if (tv->tv_sec < 0)
+		return -1;
+	tv->tv_usec = st.wMilliseconds*1000;
 	return 0;
 }
 
diff --git a/date.c b/date.c
index 8f70500..9c16eae 100644
--- a/date.c
+++ b/date.c
@@ -6,7 +6,7 @@
 
 #include "cache.h"
 
-static time_t my_mktime(struct tm *tm)
+time_t my_mktime(struct tm *tm)
 {
 	static const int mdays[] = {
 	    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
-- 
1.5.4.1.126.ge5a7d

[PATCH 13/40] Windows: Fix PRIuMAX definition.

[Johannes Sixt]

Since GIT calls into Microsoft's MSVCRT.DLL, it must use the printf
format that this DLL uses for 64-bit integers, which is %I64u instead
of %llu.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-compat-util.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 3b57464..c576f5a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -103,7 +103,11 @@
 #endif
 
 #ifndef PRIuMAX
+#ifndef __MINGW32__
 #define PRIuMAX "llu"
+#else
+#define PRIuMAX "I64u"
+#endif
 #endif
 
 #ifdef __GNUC__
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 13/40] Windows: Fix PRIuMAX definition.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3b57464..c576f5a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -103,7 +103,11 @@
>  #endif
>  
>  #ifndef PRIuMAX
> +#ifndef __MINGW32__
>  #define PRIuMAX "llu"
> +#else
> +#define PRIuMAX "I64u"
> +#endif
>  #endif

Would this not be better as a patch to Makefile, extending the 
COMPAT_CFLAGS with -DPRIuMAX=\"I64u\"?

Ciao,
Dscho

Re: [PATCH 13/40] Windows: Fix PRIuMAX definition.

[Johannes Sixt]

On Thursday 28 February 2008 13:21, Johannes Schindelin wrote:
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> >  #ifndef PRIuMAX
> > +#ifndef __MINGW32__
> >  #define PRIuMAX "llu"
> > +#else
> > +#define PRIuMAX "I64u"
> > +#endif
> >  #endif
>
> Would this not be better as a patch to Makefile, extending the
> COMPAT_CFLAGS with -DPRIuMAX=\"I64u\"?

Me likes this!

-- Hannes

[PATCH 14/40] Windows: Implement setitimer() and sigaction().

[Johannes Sixt]

The timer is implemented using a thread that calls the signal handler
at regular intervals.

We also replace Windows's signal() function because we must intercept
that SIGALRM is set (which is used when a timer is canceled).

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 git-compat-util.h |    3 +
 2 files changed, 113 insertions(+), 1 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 24f783b..4888a03 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -129,12 +129,121 @@ struct passwd *getpwuid(int uid)
 	return &p;
 }
 
-int setitimer(int type, struct itimerval *in, struct itimerval *out)
+static HANDLE timer_event;
+static HANDLE timer_thread;
+static int timer_interval;
+static int one_shot;
+static sig_handler_t timer_fn = SIG_DFL;
+
+/* The timer works like this:
+ * The thread, ticktack(), is basically a trivial routine that most of the
+ * time only waits to receive the signal to terminate. The main thread
+ * tells the thread to terminate by setting the timer_event to the signalled
+ * state.
+ * But ticktack() does not wait indefinitely; instead, it interrupts the
+ * wait state every now and then, namely exactly after timer's interval
+ * length. At these opportunities it calls the signal handler.
+ */
+
+static __stdcall unsigned ticktack(void *dummy)
+{
+	while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
+		if (timer_fn == SIG_DFL)
+			die("Alarm");
+		if (timer_fn != SIG_IGN)
+			timer_fn(SIGALRM);
+		if (one_shot)
+			break;
+	}
+	return 0;
+}
+
+static int start_timer_thread(void)
 {
+	timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
+	if (timer_event) {
+		timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);
+		if (!timer_thread )
+			return errno = ENOMEM,
+				error("cannot start timer thread");
+	} else
+		return errno = ENOMEM,
+			error("cannot allocate resources timer");
 	return 0;
 }
 
+static void stop_timer_thread(void)
+{
+	if (timer_event)
+		SetEvent(timer_event);	/* tell thread to terminate */
+	if (timer_thread) {
+		int rc = WaitForSingleObject(timer_thread, 1000);
+		if (rc == WAIT_TIMEOUT)
+			error("timer thread did not terminate timely");
+		else if (rc != WAIT_OBJECT_0)
+			error("waiting for timer thread failed: %lu",
+			      GetLastError());
+		CloseHandle(timer_thread);
+	}
+	if (timer_event)
+		CloseHandle(timer_event);
+	timer_event = NULL;
+	timer_thread = NULL;
+}
+
+static inline int is_timeval_eq(const struct timeval *i1, const struct timeval *i2)
+{
+	return i1->tv_sec == i2->tv_sec && i1->tv_usec == i2->tv_usec;
+}
+
+int setitimer(int type, struct itimerval *in, struct itimerval *out)
+{
+	static const struct timeval zero;
+	static int atexit_done;
+
+	if (out != NULL)
+		return errno = EINVAL,
+			error("setitmer param 3 != NULL not implemented");
+	if (!is_timeval_eq(&in->it_interval, &zero) &&
+	    !is_timeval_eq(&in->it_interval, &in->it_value))
+		return errno = EINVAL,
+			error("setitmer: it_interval must be zero or eq it_value");
+
+	if (timer_thread)
+		stop_timer_thread();
+
+	if (is_timeval_eq(&in->it_value, &zero) &&
+	    is_timeval_eq(&in->it_interval, &zero))
+		return 0;
+
+	timer_interval = in->it_value.tv_sec * 1000 + in->it_value.tv_usec / 1000;
+	one_shot = is_timeval_eq(&in->it_interval, &zero);
+	if (!atexit_done) {
+		atexit(stop_timer_thread);
+		atexit_done = 1;
+	}
+	return start_timer_thread();
+}
+
 int sigaction(int sig, struct sigaction *in, struct sigaction *out)
 {
+	if (sig != SIGALRM)
+		return errno = EINVAL,
+			error("sigaction only implemented for SIGALRM");
+	if (out != NULL)
+		return errno = EINVAL,
+			error("sigaction: param 3 != NULL not implemented");
+
+	timer_fn = in->sa_handler;
 	return 0;
 }
+
+#undef signal
+sig_handler_t mingw_signal(int sig, sig_handler_t handler)
+{
+	if (sig != SIGALRM)
+		return signal(sig, handler);
+	sig_handler_t old = timer_fn;
+	timer_fn = handler;
+	return old;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index c576f5a..483ace2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -602,6 +602,9 @@ char *mingw_getcwd(char *pointer, int len);
 int mingw_rename(const char*, const char*);
 #define rename mingw_rename
 
+sig_handler_t mingw_signal(int sig, sig_handler_t handler);
+#define signal mingw_signal
+
 #endif /* __MINGW32__ */
 
 #endif
-- 
1.5.4.1.126.ge5a7d

[PATCH 15/40] Windows: A work-around for a misbehaved vsnprintf.

[Johannes Sixt]

On Windows, vsnprintf deviates in two regards from the "usual" behavior
that its callers in GIT expect:

- It returns -1 if the buffer is too small instead of the number of
  characters that the operation produces.

- The size parameter is the number of characters to write, which does not
  include the trailing NUL byte, instead of the available space.

This wrapper computes the needed buffer size by trying various sizes with
exponential growth. A large growth factor is used so as only few trials are
required if a really large result needs to be stored.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |   34 ++++++++++++++++++++++++++++++++++
 git-compat-util.h |    3 +++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 4888a03..77e4b83 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -115,6 +115,40 @@ int mingw_rename(const char *pold, const char *pnew)
 	return -1;
 }
 
+#undef vsnprintf
+/* Note that the size parameter specifies the available space, i.e.
+ * includes the trailing NUL byte; but Windows's vsnprintf expects the
+ * number of characters to write without the trailing NUL.
+ */
+
+/* This is out of line because it uses alloca() behind the scenes,
+ * which must not be called in a loop (alloca() reclaims the allocations
+ * only at function exit).
+ */
+static int try_vsnprintf(size_t size, const char *fmt, va_list args)
+{
+	char buf[size];	/* gcc-ism */
+	return vsnprintf(buf, size-1, fmt, args);
+}
+
+int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
+{
+	int len;
+	if (size > 0) {
+		len = vsnprintf(buf, size-1, fmt, args);
+		if (len >= 0)
+			return len;
+	}
+	/* ouch, buffer too small; need to compute the size */
+	if (size < 250)
+		size = 250;
+	do {
+		size *= 4;
+		len = try_vsnprintf(size, fmt, args);
+	} while (len < 0);
+	return len;
+}
+
 struct passwd *getpwuid(int uid)
 {
 	static char user_name[100];
diff --git a/git-compat-util.h b/git-compat-util.h
index 483ace2..6fa93b6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -602,6 +602,9 @@ char *mingw_getcwd(char *pointer, int len);
 int mingw_rename(const char*, const char*);
 #define rename mingw_rename
 
+int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
+#define vsnprintf mingw_vsnprintf
+
 sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 #define signal mingw_signal
 
-- 
1.5.4.1.126.ge5a7d

[PATCH 16/40] Windows: Wrap execve so that shell scripts can be invoked.

[Johannes Sixt]

When an external git command is invoked, it can be a Bourne shell script.
This patch looks into the command file to see whether it is one.
In this case, the command line is rearranged to invoke the shell
with the proper arguments.

With this change, scripted git commands work. Command line arguments
to those scripts cannot be complex (contain spaces or double-quotes), yet.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |  246 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |    3 +
 2 files changed, 249 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 77e4b83..9d03ea5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -90,6 +90,252 @@ char *mingw_getcwd(char *pointer, int len)
 	return ret;
 }
 
+static const char *parse_interpreter(const char *cmd)
+{
+	static char buf[100];
+	char *p, *opt;
+	int n, fd;
+
+	/* don't even try a .exe */
+	n = strlen(cmd);
+	if (n >= 4 && !strcasecmp(cmd+n-4, ".exe"))
+		return NULL;
+
+	fd = open(cmd, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+	n = read(fd, buf, sizeof(buf)-1);
+	close(fd);
+	if (n < 4)	/* at least '#!/x' and not error */
+		return NULL;
+
+	if (buf[0] != '#' || buf[1] != '!')
+		return NULL;
+	buf[n] = '\0';
+	p = strchr(buf, '\n');
+	if (!p)
+		return NULL;
+
+	*p = '\0';
+	if (!(p = strrchr(buf+2, '/')) && !(p = strrchr(buf+2, '\\')))
+		return NULL;
+	/* strip options */
+	if ((opt = strchr(p+1, ' ')))
+		*opt = '\0';
+	return p+1;
+}
+
+/*
+ * Splits the PATH into parts.
+ */
+static char **get_path_split(void)
+{
+	char *p, **path, *envpath = getenv("PATH");
+	int i, n = 0;
+
+	if (!envpath || !*envpath)
+		return NULL;
+
+	envpath = xstrdup(envpath);
+	p = envpath;
+	while (p) {
+		char *dir = p;
+		p = strchr(p, ';');
+		if (p) *p++ = '\0';
+		if (*dir) {	/* not earlier, catches series of ; */
+			++n;
+		}
+	}
+	if (!n)
+		return NULL;
+
+	path = xmalloc((n+1)*sizeof(char*));
+	p = envpath;
+	i = 0;
+	do {
+		if (*p)
+			path[i++] = xstrdup(p);
+		p = p+strlen(p)+1;
+	} while (i < n);
+	path[i] = NULL;
+
+	free(envpath);
+
+	return path;
+}
+
+static void free_path_split(char **path)
+{
+	if (!path)
+		return;
+
+	char **p = path;
+	while (*p)
+		free(*p++);
+	free(path);
+}
+
+/*
+ * exe_only means that we only want to detect .exe files, but not scripts
+ * (which do not have an extension)
+ */
+static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_only)
+{
+	char path[MAX_PATH];
+	snprintf(path, sizeof(path), "%s/%s.exe", dir, cmd);
+
+	if (!isexe && access(path, F_OK) == 0)
+		return xstrdup(path);
+	path[strlen(path)-4] = '\0';
+	if ((!exe_only || isexe) && access(path, F_OK) == 0)
+		return xstrdup(path);
+	return NULL;
+}
+
+/*
+ * Determines the absolute path of cmd using the the split path in path.
+ * If cmd contains a slash or backslash, no lookup is performed.
+ */
+static char *path_lookup(const char *cmd, char **path, int exe_only)
+{
+	char *prog = NULL;
+	int len = strlen(cmd);
+	int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe");
+
+	if (strchr(cmd, '/') || strchr(cmd, '\\'))
+		prog = xstrdup(cmd);
+
+	while (!prog && *path)
+		prog = lookup_prog(*path++, cmd, isexe, exe_only);
+
+	return prog;
+}
+
+static int try_shell_exec(const char *cmd, char *const *argv, char **env)
+{
+	const char *interpr = parse_interpreter(cmd);
+	char **path;
+	char *prog;
+	int pid = 0;
+
+	if (!interpr)
+		return 0;
+	path = get_path_split();
+	prog = path_lookup(interpr, path, 1);
+	if (prog) {
+		int argc = 0;
+		const char **argv2;
+		while (argv[argc]) argc++;
+		argv2 = xmalloc(sizeof(*argv) * (argc+2));
+		argv2[0] = (char *)interpr;
+		argv2[1] = (char *)cmd;	/* full path to the script file */
+		memcpy(&argv2[2], &argv[1], sizeof(*argv) * argc);
+		pid = spawnve(_P_NOWAIT, prog, argv2, (const char **)env);
+		if (pid >= 0) {
+			int status;
+			if (waitpid(pid, &status, 0) < 0)
+				status = 255;
+			exit(status);
+		}
+		pid = 1;	/* indicate that we tried but failed */
+		free(prog);
+		free(argv2);
+	}
+	free_path_split(path);
+	return pid;
+}
+
+static void mingw_execve(const char *cmd, char *const *argv, char *const *env)
+{
+	/* check if git_command is a shell script */
+	if (!try_shell_exec(cmd, argv, (char **)env)) {
+		int pid, status;
+
+		pid = spawnve(_P_NOWAIT, cmd, (const char **)argv, (const char **)env);
+		if (pid < 0)
+			return;
+		if (waitpid(pid, &status, 0) < 0)
+			status = 255;
+		exit(status);
+	}
+}
+
+void mingw_execvp(const char *cmd, char *const *argv)
+{
+	char **path = get_path_split();
+	char *prog = path_lookup(cmd, path, 0);
+
+	if (prog) {
+		mingw_execve(prog, argv, environ);
+		free(prog);
+	} else
+		errno = ENOENT;
+
+	free_path_split(path);
+}
+
+char **copy_environ()
+{
+	char **env;
+	int i = 0;
+	while (environ[i])
+		i++;
+	env = xmalloc((i+1)*sizeof(*env));
+	for (i = 0; environ[i]; i++)
+		env[i] = xstrdup(environ[i]);
+	env[i] = NULL;
+	return env;
+}
+
+void free_environ(char **env)
+{
+	int i;
+	for (i = 0; env[i]; i++)
+		free(env[i]);
+	free(env);
+}
+
+static int lookup_env(char **env, const char *name, size_t nmln)
+{
+	int i;
+
+	for (i = 0; env[i]; i++) {
+		if (0 == strncmp(env[i], name, nmln)
+		    && '=' == env[i][nmln])
+			/* matches */
+			return i;
+	}
+	return -1;
+}
+
+/*
+ * If name contains '=', then sets the variable, otherwise it unsets it
+ */
+char **env_setenv(char **env, const char *name)
+{
+	char *eq = strchrnul(name, '=');
+	int i = lookup_env(env, name, eq-name);
+
+	if (i < 0) {
+		if (*eq) {
+			for (i = 0; env[i]; i++)
+				;
+			env = xrealloc(env, (i+2)*sizeof(*env));
+			env[i] = xstrdup(name);
+			env[i+1] = NULL;
+		}
+	}
+	else {
+		free(env[i]);
+		if (*eq)
+			env[i] = xstrdup(name);
+		else
+			for (; env[i]; i++)
+				env[i] = env[i+1];
+	}
+	return env;
+}
+
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 6fa93b6..e191a46 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -605,6 +605,9 @@ int mingw_rename(const char*, const char*);
 int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
 #define vsnprintf mingw_vsnprintf
 
+void mingw_execvp(const char *cmd, char *const *argv);
+#define execvp mingw_execvp
+
 sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 #define signal mingw_signal
 
-- 
1.5.4.1.126.ge5a7d

[PATCH 17/40] Windows: A pipe() replacement whose ends are not inherited to children.

[Johannes Sixt]

On Unix the idiom to use a pipe is as follows:

    pipe(fd);
    pid = fork();
    if (!pid) {
        dup2(fd[1], 1);
        close(fd[1]);
        close(fd[0]);
        ...
     }
     close(fd[1]);

i.e. the child process closes the both pipe ends after duplicating one
to the file descriptors where they are needed.

On Windows, which does not have fork(), we never have an opportunity to
(1) duplicate a pipe end in the child, (2) close unused pipe ends. Instead,
we must use this idiom:

    save1 = dup(1);
    pipe(fd);
    dup2(fd[1], 1);
    spawn(...);
    dup2(save1, 1);
    close(fd[1]);

i.e. save away the descriptor at the destination slot, replace by the pipe
end, spawn process, restore the saved file.

But there is a problem: Notice that the child did not only inherit the
dup2()ed descriptor, but also *both* original pipe ends. Although the one
end that was dup()ed could be closed before the spawn(), we cannot close
the other end - the child inherits it, no matter what.

The solution is to generate non-inheritable pipes. At the first glance,
this looks strange: The purpose of pipes is usually to be inherited to
child processes. But notice that in the course of actions as outlined
above, the pipe descriptor that we want to inherit to the child is
dup2()ed, and as it so happens, Windows's dup2() creates inheritable
duplicates.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |   45 +++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |    5 +----
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 9d03ea5..fe6f6ce 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -55,6 +55,51 @@ int gettimeofday(struct timeval *tv, void *tz)
 	return 0;
 }
 
+int pipe(int filedes[2])
+{
+	int fd;
+	HANDLE h[2], parent;
+
+	if (_pipe(filedes, 8192, 0) < 0)
+		return -1;
+
+	parent = GetCurrentProcess();
+
+	if (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[0]),
+			parent, &h[0], 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+		close(filedes[0]);
+		close(filedes[1]);
+		return -1;
+	}
+	if (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[1]),
+			parent, &h[1], 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+		close(filedes[0]);
+		close(filedes[1]);
+		CloseHandle(h[0]);
+		return -1;
+	}
+	fd = _open_osfhandle((int)h[0], O_NOINHERIT);
+	if (fd < 0) {
+		close(filedes[0]);
+		close(filedes[1]);
+		CloseHandle(h[0]);
+		CloseHandle(h[1]);
+		return -1;
+	}
+	close(filedes[0]);
+	filedes[0] = fd;
+	fd = _open_osfhandle((int)h[1], O_NOINHERIT);
+	if (fd < 0) {
+		close(filedes[0]);
+		close(filedes[1]);
+		CloseHandle(h[1]);
+		return -1;
+	}
+	close(filedes[1]);
+	filedes[1] = fd;
+	return 0;
+}
+
 int poll(struct pollfd *ufds, unsigned int nfds, int timeout)
 {
 	return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index e191a46..170c279 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -571,14 +571,11 @@ static inline int waitpid(pid_t pid, unsigned *status, unsigned options)
 	return -1;
 }
 
-
-static inline int pipe(int filedes[2])
-{ return _pipe(filedes, 8192, 0); }
-
 /*
  * implementations of missing functions
  */
 
+int pipe(int filedes[2]);
 unsigned int sleep (unsigned int seconds);
 int gettimeofday(struct timeval *tv, void *tz);
 int poll(struct pollfd *ufds, unsigned int nfds, int timeout);
-- 
1.5.4.1.126.ge5a7d

[PATCH 18/40] Windows: Implement start_command().

[Johannes Sixt]

On Windows, we have spawnv() variants to run a child process instead of
fork()/exec(). In order to attach pipe ends to stdin, stdout, and stderr,
we have to use this idiom:

    save1 = dup(1);
    dup2(pipe[1], 1);
    spawnv();
    dup2(save1, 1);
    close(pipe[1]);

assuming that the descriptors created by pipe() are not inheritable.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-compat-util.h |    8 +++++
 run-command.c     |   89 +++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 87 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 170c279..046891d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -608,6 +608,14 @@ void mingw_execvp(const char *cmd, char *const *argv);
 sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 #define signal mingw_signal
 
+/*
+ * helpers
+ */
+
+char **copy_environ(void);
+void free_environ(char **env);
+char **env_setenv(char **env, const char *name);
+
 #endif /* __MINGW32__ */
 
 #endif
diff --git a/run-command.c b/run-command.c
index 476d00c..873f6d0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -53,17 +53,8 @@ int start_command(struct child_process *cmd)
 		cmd->err = fderr[0];
 	}
 
+#ifndef __MINGW32__
 	cmd->pid = fork();
-	if (cmd->pid < 0) {
-		if (need_in)
-			close_pair(fdin);
-		if (need_out)
-			close_pair(fdout);
-		if (need_err)
-			close_pair(fderr);
-		return -ERR_RUN_COMMAND_FORK;
-	}
-
 	if (!cmd->pid) {
 		if (cmd->no_stdin)
 			dup_devnull(0);
@@ -112,6 +103,84 @@ int start_command(struct child_process *cmd)
 		}
 		die("exec %s failed.", cmd->argv[0]);
 	}
+#else
+	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
+	const char *sargv0 = cmd->argv[0];
+	char **env = environ;
+	struct strbuf git_cmd;
+
+	if (cmd->no_stdin) {
+		s0 = dup(0);
+		dup_devnull(0);
+	} else if (need_in) {
+		s0 = dup(0);
+		dup2(fdin[0], 0);
+	} else if (cmd->in) {
+		s0 = dup(0);
+		dup2(cmd->in, 0);
+	}
+
+	if (cmd->no_stdout) {
+		s1 = dup(1);
+		dup_devnull(1);
+	} else if (cmd->stdout_to_stderr) {
+		s1 = dup(1);
+		dup2(2, 1);
+	} else if (need_out) {
+		s1 = dup(1);
+		dup2(fdout[1], 1);
+	} else if (cmd->out > 1) {
+		s1 = dup(1);
+		dup2(cmd->out, 1);
+	}
+
+	if (cmd->no_stderr) {
+		s2 = dup(2);
+		dup_devnull(2);
+	} else if (need_err) {
+		s2 = dup(2);
+		dup2(fderr[1], 2);
+	}
+
+	if (cmd->dir)
+		die("chdir in start_command() not implemented");
+	if (cmd->env) {
+		env = copy_environ();
+		for (; *cmd->env; cmd->env++)
+			env = env_setenv(env, *cmd->env);
+	}
+
+	if (cmd->git_cmd) {
+		strbuf_init(&git_cmd, 0);
+		strbuf_addf(&git_cmd, "git-%s", cmd->argv[0]);
+		cmd->argv[0] = git_cmd.buf;
+	}
+
+	cmd->pid = spawnvpe(_P_NOWAIT, cmd->argv[0], cmd->argv, (const char **)env);
+
+	if (cmd->env)
+		free_environ(env);
+	if (cmd->git_cmd)
+		strbuf_release(&git_cmd);
+
+	cmd->argv[0] = sargv0;
+	if (s0 >= 0)
+		dup2(s0, 0), close(s0);
+	if (s1 >= 0)
+		dup2(s1, 1), close(s1);
+	if (s2 >= 0)
+		dup2(s2, 2), close(s2);
+#endif
+
+	if (cmd->pid < 0) {
+		if (need_in)
+			close_pair(fdin);
+		if (need_out)
+			close_pair(fdout);
+		if (need_err)
+			close_pair(fderr);
+		return -ERR_RUN_COMMAND_FORK;
+	}
 
 	if (need_in)
 		close(fdin[0]);
-- 
1.5.4.1.126.ge5a7d

[PATCH 19/40] Windows: Change the name of hook scripts to make them not executable.

[Johannes Sixt]

Since on Windows there is no 'executable' bit whose absence would deny
execution of a script, we must change the hook scripts' names entirely
to inhibit that they can be invoked by the tools.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile           |    3 ++-
 templates/Makefile |    8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 069939f..7d4a591 100644
--- a/Makefile
+++ b/Makefile
@@ -547,6 +547,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o
 	EXTLIBS += -lws2_32
 	X = .exe
+	NOEXECTEMPL = .noexec
 endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
@@ -849,7 +850,7 @@ ifndef NO_TCLTK
 	$(QUIET_SUBDIR0)gitk-git $(QUIET_SUBDIR1) all
 endif
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' all
-	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1)
+	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) NOEXECTEMPL='$(NOEXECTEMPL)'
 
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
diff --git a/templates/Makefile b/templates/Makefile
index ebd3a62..b341105 100644
--- a/templates/Makefile
+++ b/templates/Makefile
@@ -10,6 +10,8 @@ RM ?= rm -f
 prefix ?= $(HOME)
 template_dir ?= $(prefix)/share/git-core/templates
 # DESTDIR=
+# set NOEXECTEMPL to non-empty to change the names of hook scripts
+# so that the tools will not find them
 
 # Shell quote (do not use $(call) to accommodate ancient setups);
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
@@ -32,7 +34,11 @@ boilerplates.made : $(bpsrc)
 		mkdir -p blt/$$dir && \
 		case "$$boilerplate" in \
 		*--) ;; \
-		*) cp $$boilerplate blt/$$dst ;; \
+		*) if test -n "$$(sed -ne '/^#!\//p' -e '1q' < "$$boilerplate")"; then \
+			cp "$$boilerplate" "blt/$${dst}$(NOEXECTEMPL)"; \
+		   else \
+			cp "$$boilerplate" "blt/$$dst"; \
+		   fi ;; \
 		esac || exit; \
 	done && \
 	date >$@
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 19/40] Windows: Change the name of hook scripts to make them not executable.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> -		*) cp $$boilerplate blt/$$dst ;; \
> +		*) if test -n "$$(sed -ne '/^#!\//p' -e '1q' < "$$boilerplate")"; then \
> +			cp "$$boilerplate" "blt/$${dst}$(NOEXECTEMPL)"; \
> +		   else \
> +			cp "$$boilerplate" "blt/$$dst"; \
> +		   fi ;; \

Why not just append .noexec to all of the hooks?

Ciao,
Dscho

Re: [PATCH 19/40] Windows: Change the name of hook scripts to make them not executable.

[Johannes Sixt]

On Thursday 28 February 2008 16:20, Johannes Schindelin wrote:
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > -		*) cp $$boilerplate blt/$$dst ;; \
> > +		*) if test -n "$$(sed -ne '/^#!\//p' -e '1q' < "$$boilerplate")"; then
> > \ +			cp "$$boilerplate" "blt/$${dst}$(NOEXECTEMPL)"; \
> > +		   else \
> > +			cp "$$boilerplate" "blt/$$dst"; \
> > +		   fi ;; \
>
> Why not just append .noexec to all of the hooks?

Because some of the files are not hooks. Or do you mean to check for the files 
hooks--* explicitly?

-- Hannes

Re: [PATCH 19/40] Windows: Change the name of hook scripts to make them not executable.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 16:20, Johannes Schindelin wrote:
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > > -		*) cp $$boilerplate blt/$$dst ;; \
> > > +		*) if test -n "$$(sed -ne '/^#!\//p' -e '1q' < "$$boilerplate")"; then
> > > \ +			cp "$$boilerplate" "blt/$${dst}$(NOEXECTEMPL)"; \
> > > +		   else \
> > > +			cp "$$boilerplate" "blt/$$dst"; \
> > > +		   fi ;; \
> >
> > Why not just append .noexec to all of the hooks?
> 
> Because some of the files are not hooks.

Then you have to make sure even more urgently that only the hooks are 
treated that way.

Ciao,
Dscho

[PATCH 20/40] Windows: A rudimentary poll() emulation.

[Johannes Sixt]

This emulation of poll() is by far not general. It assumes that the
fds that are to be waited for are connected to pipes. The pipes are
polled in a loop until data becomes available in at least one of them.
If only a single fd is waited for, the implementation actually does
not wait at all, but assumes that a subsequent read() will block.

In order not to needlessly burn CPU time, the CPU is yielded to other
processes before the next round in the poll loop using Sleep(0). Note that
any sleep timeout greater than zero will reduce the efficiency by a
magnitude.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index fe6f6ce..837d741 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -102,6 +102,61 @@ int pipe(int filedes[2])
 
 int poll(struct pollfd *ufds, unsigned int nfds, int timeout)
 {
+	int i, pending;
+
+	if (timeout != -1)
+		return errno = EINVAL, error("poll timeout not supported");
+
+	/* When there is only one fd to wait for, then we pretend that
+	 * input is available and let the actual wait happen when the
+	 * caller invokes read().
+	 */
+	if (nfds == 1) {
+		if (!(ufds[0].events & POLLIN))
+			return errno = EINVAL, error("POLLIN not set");
+		ufds[0].revents = POLLIN;
+		return 0;
+	}
+
+repeat:
+	pending = 0;
+	for (i = 0; i < nfds; i++) {
+		DWORD avail = 0;
+		HANDLE h = (HANDLE) _get_osfhandle(ufds[i].fd);
+		if (h == INVALID_HANDLE_VALUE)
+			return -1;	/* errno was set */
+
+		if (!(ufds[i].events & POLLIN))
+			return errno = EINVAL, error("POLLIN not set");
+
+		/* this emulation works only for pipes */
+		if (!PeekNamedPipe(h, NULL, 0, NULL, &avail, NULL)) {
+			int err = GetLastError();
+			if (err == ERROR_BROKEN_PIPE) {
+				ufds[i].revents = POLLHUP;
+				pending++;
+			} else {
+				errno = EINVAL;
+				return error("PeekNamedPipe failed,"
+					" GetLastError: %u", err);
+			}
+		} else if (avail) {
+			ufds[i].revents = POLLIN;
+			pending++;
+		} else
+			ufds[i].revents = 0;
+	}
+	if (!pending) {
+		/* The only times that we spin here is when the process
+		 * that is connected through the pipes is waiting for
+		 * its own input data to become available. But since
+		 * the process (pack-objects) is itself CPU intensive,
+		 * it will happily pick up the time slice that we are
+		 * relinguishing here.
+		 */
+		Sleep(0);
+		goto repeat;
+	}
 	return 0;
 }
 
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 20/40] Windows: A rudimentary poll() emulation.

[Paolo Bonzini]

Johannes Sixt wrote:
> This emulation of poll() is by far not general. It assumes that the
> fds that are to be waited for are connected to pipes. The pipes are
> polled in a loop until data becomes available in at least one of them.
> If only a single fd is waited for, the implementation actually does
> not wait at all, but assumes that a subsequent read() will block.

For the future, would it be better to first use WaitForMultipleObjects, 
and then use PeekNamedPipe to find which handles have data on it? 
That's how the mingw port of GNU Smalltalk does it.

Paolo

Re: [PATCH 20/40] Windows: A rudimentary poll() emulation.

[Johannes Sixt]

On Thursday 28 February 2008 10:36, Paolo Bonzini wrote:
> Johannes Sixt wrote:
> > This emulation of poll() is by far not general. It assumes that the
> > fds that are to be waited for are connected to pipes. The pipes are
> > polled in a loop until data becomes available in at least one of them.
> > If only a single fd is waited for, the implementation actually does
> > not wait at all, but assumes that a subsequent read() will block.
>
> For the future, would it be better to first use WaitForMultipleObjects,
> and then use PeekNamedPipe to find which handles have data on it?
> That's how the mingw port of GNU Smalltalk does it.

I tried but I failed. If you can show me code where WaitForMultipleObjects
works on handles that MSVCRT.DLL's open() created, I'll gladly accept it!

-- Hannes

Re: [PATCH 20/40] Windows: A rudimentary poll() emulation.

[Govind Salinas]

On 2/29/08, Johannes Sixt <johannes.sixt@telecom.at> wrote:
> On Friday 29 February 2008 05:19, Govind Salinas wrote:
>  > On Thu, Feb 28, 2008 at 2:49 PM, Johannes Sixt <johannes.sixt@telecom.at>
>  wrote:
>  > > On Thursday 28 February 2008 10:36, Paolo Bonzini wrote:
>
> > >  > For the future, would it be better to first use
>  > >  > WaitForMultipleObjects, and then use PeekNamedPipe to find which
>  > >  > handles have data on it? That's how the mingw port of GNU Smalltalk
>  > >  > does it.
>  > >
>  > >  I tried but I failed. If you can show me code where
>      ^^^^^^^^^^^^^^^^^^^^^
>  > > WaitForMultipleObjects works on handles that MSVCRT.DLL's open() created,
>  > > I'll gladly accept it!
>  >
>  > I haven't tried it myself, but you can look at _get_osfhandle
>  >
>  > http://msdn2.microsoft.com/en-us/library/ks2530z6(VS.71).aspx
>  >
>  > of course you would need to keep a mapping from the handle to the fd.
>  > Or _open_osfhandle might work the other way, I don't know if it will
>  > necessarily return the same descriptor.
>  >
>  > http://msdn2.microsoft.com/en-us/library/bdts1c9x(VS.71).aspx
>
>
> Fscking basics, this. How do you go from here to WaitForMultipleObjects?
>
Well, I could have sworn that named pipe handles were in the waitable
list, but after looking again, they are not.

However, I did find

http://msdn2.microsoft.com/en-us/library/aa365603(VS.85).aspx

which suggests you can pass these handles to ReadFile and if you include an
OVERLAPPED struct in the call, it will return an event handle to wait on which
you can then use to wait.

Sorry for the false lead.

HTH,
Govind.

Re: [PATCH 20/40] Windows: A rudimentary poll() emulation.

[Johannes Sixt]

On Friday 29 February 2008 22:47, Govind Salinas wrote:
> On 2/29/08, Johannes Sixt <johannes.sixt@telecom.at> wrote:
> > On Friday 29 February 2008 05:19, Govind Salinas wrote:
> >  > On Thu, Feb 28, 2008 at 2:49 PM, Johannes Sixt
> >  > <johannes.sixt@telecom.at>
> >
> >  wrote:
> >  > > On Thursday 28 February 2008 10:36, Paolo Bonzini wrote:
> > > >  > For the future, would it be better to first use
> > > >  >
> >  > >  > WaitForMultipleObjects, and then use PeekNamedPipe to find which
> >  > >  > handles have data on it? That's how the mingw port of GNU
> >  > >  > Smalltalk does it.
> >  > >
> >  > >  I tried but I failed. If you can show me code where
> >
> >      ^^^^^^^^^^^^^^^^^^^^^
> >
> >  > > WaitForMultipleObjects works on handles that MSVCRT.DLL's open()
> >  > > created, I'll gladly accept it!
> >  >
> >  > I haven't tried it myself, but you can look at _get_osfhandle
> >  >
> >  > http://msdn2.microsoft.com/en-us/library/ks2530z6(VS.71).aspx
> >  >
> >  > of course you would need to keep a mapping from the handle to the fd.
> >  > Or _open_osfhandle might work the other way, I don't know if it will
> >  > necessarily return the same descriptor.
> >  >
> >  > http://msdn2.microsoft.com/en-us/library/bdts1c9x(VS.71).aspx
> >
> > Fscking basics, this. How do you go from here to WaitForMultipleObjects?
>
> Well, I could have sworn that named pipe handles were in the waitable
> list, but after looking again, they are not.
>
> However, I did find
>
> http://msdn2.microsoft.com/en-us/library/aa365603(VS.85).aspx
>
> which suggests you can pass these handles to ReadFile and if you include an
> OVERLAPPED struct in the call, it will return an event handle to wait on
> which you can then use to wait.

I've read the documentation. Try it. Come back when you succeed.

-- Hannes

Re: [PATCH 20/40] Windows: A rudimentary poll() emulation.

[Brian Dessent]

Govind Salinas wrote:

> http://msdn2.microsoft.com/en-us/library/aa365603(VS.85).aspx
> 
> which suggests you can pass these handles to ReadFile and if you include an
> OVERLAPPED struct in the call, it will return an event handle to wait on which
> you can then use to wait.

That only applies if the handle has been opened with
FILE_FLAG_OVERLAPPED.

Brian

Re: [PATCH 20/40] Windows: A rudimentary poll() emulation.

[Robin Rosenberg]

Den Thursday 28 February 2008 21.49.36 skrev Johannes Sixt:
> On Thursday 28 February 2008 10:36, Paolo Bonzini wrote:
> > For the future, would it be better to first use WaitForMultipleObjects,
> > and then use PeekNamedPipe to find which handles have data on it?
> > That's how the mingw port of GNU Smalltalk does it.
>
> I tried but I failed. If you can show me code where WaitForMultipleObjects
> works on handles that MSVCRT.DLL's open() created, I'll gladly accept it!

Have you checked _get_osfhandle? There's a warning though in the knowledgebase
with number 99173, though. There's also another function that works the other 
way, _open_osfhandle.

-- robin

Re: [PATCH 20/40] Windows: A rudimentary poll() emulation.

[Johannes Sixt]

On Saturday 01 March 2008 16:48, Robin Rosenberg wrote:
> Den Thursday 28 February 2008 21.49.36 skrev Johannes Sixt:
> > On Thursday 28 February 2008 10:36, Paolo Bonzini wrote:
> > > For the future, would it be better to first use WaitForMultipleObjects,
> > > and then use PeekNamedPipe to find which handles have data on it?
> > > That's how the mingw port of GNU Smalltalk does it.
> >
> > I tried but I failed. If you can show me code where
> > WaitForMultipleObjects works on handles that MSVCRT.DLL's open() created,
> > I'll gladly accept it!
>
> Have you checked _get_osfhandle? There's a warning though in the
> knowledgebase with number 99173, though. There's also another function that
> works the other way, _open_osfhandle.

Handwaving you are! This stuff is well-known. Show me code.

-- Hannes

[PATCH 21/40] Windows: Disambiguate DOS style paths from SSH URLs.

[Johannes Sixt]

If on Windows a path is specified as C:/path, then this is also a valid
SSH URL. To disambiguate between the two interpretations we require that
a SSH URL must have a host name with at least two characters.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 connect.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/connect.c b/connect.c
index 5ac3572..7e18ac8 100644
--- a/connect.c
+++ b/connect.c
@@ -529,7 +529,13 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		end = host;
 
 	path = strchr(end, c);
-	if (path) {
+#ifdef __MINGW32__
+	/* host must have at least 2 chars to catch DOS C:/path */
+	if (path && path - end > 1)
+#else
+	if (path)
+#endif
+	{
 		if (c == ':') {
 			protocol = PROTO_SSH;
 			*path++ = '\0';
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 21/40] Windows: Disambiguate DOS style paths from SSH URLs.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> diff --git a/connect.c b/connect.c
> index 5ac3572..7e18ac8 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -529,7 +529,13 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
>  		end = host;
>  
>  	path = strchr(end, c);
> -	if (path) {
> +#ifdef __MINGW32__
> +	/* host must have at least 2 chars to catch DOS C:/path */
> +	if (path && path - end > 1)
> +#else
> +	if (path)
> +#endif
> +	{

This could be un-#ifdef'ed by using the has_dos_drive_prefix() function I 
suggested earlier.

Ciao,
Dscho

Re: [PATCH 21/40] Windows: Disambiguate DOS style paths from SSH URLs.

[Johannes Sixt]

On Thursday 28 February 2008 16:22, Johannes Schindelin wrote:
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > +#ifdef __MINGW32__
> > +	/* host must have at least 2 chars to catch DOS C:/path */
> > +	if (path && path - end > 1)
> > +#else
> > +	if (path)
> > +#endif
> > +	{
>
> This could be un-#ifdef'ed by using the has_dos_drive_prefix() function I
> suggested earlier.

Ah, great! I'm convinced.

-- Hannes

[PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Johannes Sixt]

In upload-pack we must explicitly close the output channel of rev-list.
(On Unix, the channel is closed automatically because process that runs
rev-list terminates.)

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 run-command.c |   29 ++++++++++++++++++++++++++++-
 run-command.h |    5 +++++
 upload-pack.c |    2 ++
 3 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 873f6d0..3834f86 100644
--- a/run-command.c
+++ b/run-command.c
@@ -276,13 +276,23 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
 	return run_command(&cmd);
 }
 
+#ifdef __MINGW32__
+static __stdcall unsigned run_thread(void *data)
+{
+	struct async *async = data;
+	return async->proc(async->fd_for_proc, async->data);
+}
+#endif
+
 int start_async(struct async *async)
 {
 	int pipe_out[2];
 
 	if (pipe(pipe_out) < 0)
 		return error("cannot create pipe: %s", strerror(errno));
+	async->out = pipe_out[0];
 
+#ifndef __MINGW32__
 	async->pid = fork();
 	if (async->pid < 0) {
 		error("fork (async) failed: %s", strerror(errno));
@@ -293,16 +303,33 @@ int start_async(struct async *async)
 		close(pipe_out[0]);
 		exit(!!async->proc(pipe_out[1], async->data));
 	}
-	async->out = pipe_out[0];
 	close(pipe_out[1]);
+#else
+	async->fd_for_proc = pipe_out[1];
+	async->tid = (HANDLE) _beginthreadex(NULL, 0, run_thread, async, 0, NULL);
+	if (!async->tid) {
+		error("cannot create thread: %s", strerror(errno));
+		close_pair(pipe_out);
+		return -1;
+	}
+#endif
 	return 0;
 }
 
 int finish_async(struct async *async)
 {
+#ifndef __MINGW32__
 	int ret = 0;
 
 	if (wait_or_whine(async->pid))
 		ret = error("waitpid (async) failed");
+#else
+	DWORD ret = 0;
+	if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0)
+		ret = error("waiting for thread failed: %lu", GetLastError());
+	else if (!GetExitCodeThread(async->tid, &ret))
+		ret = error("cannot get thread exit code: %lu", GetLastError());
+	CloseHandle(async->tid);
+#endif
 	return ret;
 }
diff --git a/run-command.h b/run-command.h
index 1fc781d..0bbac86 100644
--- a/run-command.h
+++ b/run-command.h
@@ -60,7 +60,12 @@ struct async {
 	int (*proc)(int fd, void *data);
 	void *data;
 	int out;	/* caller reads from here and closes it */
+#ifndef __MINGW32__
 	pid_t pid;
+#else
+	HANDLE tid;
+	int fd_for_proc;
+#endif
 };
 
 int start_async(struct async *async);
diff --git a/upload-pack.c b/upload-pack.c
index b26d053..3c99c8d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -133,6 +133,8 @@ static int do_rev_list(int fd, void *create_full_pack)
 		die("revision walk setup failed");
 	mark_edges_uninteresting(revs.commits, &revs, show_edge);
 	traverse_commit_list(&revs, show_commit, show_object);
+	fflush(pack_pipe);
+	fclose(pack_pipe);
 	return 0;
 }
 
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> In upload-pack we must explicitly close the output channel of rev-list. 
> (On Unix, the channel is closed automatically because process that runs 
> rev-list terminates.)

When I read this patch, my impression was that it litters the source code 
with #ifdef's.  IMO this makes the code less readable, and as a 
consequence easer to fsck up.

Unfortunately, I have no idea how to help that, other than implementing 
compat/thread.[ch], abstracting the thread functions, and introducing a 
NO_FORK Makefile variable and preprocessor constant.

Hmpf.

Ciao,
Dscho

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Paul Franz]

This reminds me of a crazy idea and I know it would be quite a bit of 
work. Do you think it would be possible to change git to use Apache's 
apr library? The benefit would be that the cross platform stuff is 
already written. I understand that it would not be tweaked to be 
optimized for git's usage. But it would allow people to focus on the 
heart of the git and not the way git interacts with the OS level stuff.  
Obviously, the scripts that make up part of git are not as portable but 
that is a different animal unless you want to either support a cross 
platform scripting language as part of git.

As I said it is a crazy idea.

Paul Franz

Johannes Schindelin wrote:
> Hi,
>
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
>
>   
>> In upload-pack we must explicitly close the output channel of rev-list. 
>> (On Unix, the channel is closed automatically because process that runs 
>> rev-list terminates.)
>>     
>
> When I read this patch, my impression was that it litters the source code 
> with #ifdef's.  IMO this makes the code less readable, and as a 
> consequence easer to fsck up.
>
> Unfortunately, I have no idea how to help that, other than implementing 
> compat/thread.[ch], abstracting the thread functions, and introducing a 
> NO_FORK Makefile variable and preprocessor constant.
>
> Hmpf.
>
> Ciao,
> Dscho
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   

-- 

-------------------------------------------

There are seven sins in the world.
     Wealth without work.
     Pleasure without conscience.
     Knowledge without character.
     Commerce without morality.
     Science without humanity.
     Worship without sacrifice.
     Politics without principle.

   -- Mohandas Gandhi

-------------------------------------------

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Paul Franz wrote:

> Do you think it would be possible to change git to use Apache's apr 
> library?

Possible?  Yes.

Sensible?  That's another issue.  For one, we already rely on a pretty 
good POSIX shell support.  That is not something helped by APR.

So if we already need a POSIX shell, we might just as well rely on a 
(kind of) POSIX environment.

Besides, I cannot say that I found compiling APR in msysGit _easy_.

So if there are as many disadvantages as I suspect, and as few advantages, 
as I suspect, too, I think it would be a waste of time to bother to port 
Git to APR.

Besides, I think there are -- again, as always when things seem to be only 
fun -- legal issues.  I do not know the details, but there are people who 
say that APL is incompatible with GPL (v2).

Not that I care too much about those bastards who devised licenses just to 
make my life miserable.  But them lawyer types have ways to _make_ my 
life miserable.  So I avoid those kind of problems.  I guess you could 
call this "collateral legal damage".

Ciao,
Dscho

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Paul Franz]

True there are the legal issues. And it is not known if it would help on 
other platforms. My may concern is Windows because if git could be made 
available without some of msysGit's requirements. It would definitely 
make it more palletable (sp?) to that community which is a large chunk 
of developers, including my own work environment. I would love to switch 
from ClearCase to git but there are issues. One of them is integration 
with the Windows environment.

Another concern is performance penalty of using non-native calls under 
Windows and that is one of the problems that I was hoping the using APR 
would solve. I figure if it is good enough to support the network stuff 
under Windows well maybe it would be good for git too.

Paul Franz

Johannes Schindelin wrote:
> Hi,
>
> On Thu, 28 Feb 2008, Paul Franz wrote:
>
>   
>> Do you think it would be possible to change git to use Apache's apr 
>> library?
>>     
>
> Possible?  Yes.
>
> Sensible?  That's another issue.  For one, we already rely on a pretty 
> good POSIX shell support.  That is not something helped by APR.
>
> So if we already need a POSIX shell, we might just as well rely on a 
> (kind of) POSIX environment.
>
> Besides, I cannot say that I found compiling APR in msysGit _easy_.
>
> So if there are as many disadvantages as I suspect, and as few advantages, 
> as I suspect, too, I think it would be a waste of time to bother to port 
> Git to APR.
>
> Besides, I think there are -- again, as always when things seem to be only 
> fun -- legal issues.  I do not know the details, but there are people who 
> say that APL is incompatible with GPL (v2).
>
> Not that I care too much about those bastards who devised licenses just to 
> make my life miserable.  But them lawyer types have ways to _make_ my 
> life miserable.  So I avoid those kind of problems.  I guess you could 
> call this "collateral legal damage".
>
> Ciao,
> Dscho
>
>
>   

-- 

-------------------------------------------

There are seven sins in the world.
     Wealth without work.
     Pleasure without conscience.
     Knowledge without character.
     Commerce without morality.
     Science without humanity.
     Worship without sacrifice.
     Politics without principle.

   -- Mohandas Gandhi

-------------------------------------------

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Johannes Schindelin]

Hi,

[top-posting?]

On Thu, 28 Feb 2008, Paul Franz wrote:

> True there are the legal issues. And it is not known if it would help on 
> other platforms. My may concern is Windows because if git could be made 
> available without some of msysGit's requirements. It would definitely 
> make it more palletable (sp?) to that community which is a large chunk 
> of developers, including my own work environment. I would love to switch 
> from ClearCase to git but there are issues. One of them is integration 
> with the Windows environment.

I think that you do not help the effort to bring Git to Windows, by 
suggesting even more work, for dubitable benefits.

And that is what I think of APR here.

NOTE: my only experience with APR on Windows is to get git-svn to compile, 
and there it was not only a PITA.  The result is _SLOW_.

> Another concern is performance penalty of using non-native calls under 
> Windows and that is one of the problems that I was hoping the using APR 
> would solve. I figure if it is good enough to support the network stuff 
> under Windows well maybe it would be good for git too.

The problems with Git and Windows are not network related.  They are with 
Windows' pitiful performance when it comes to spawn processes.

IMO your efforts to help Git on Windows would be better spent on the 
msysGit list, where you would learn about the real issues we face.

Ciao,
Dscho

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Paul Franz]

Johannes Schindelin wrote:
> Hi,
>
> [top-posting?]
>   

Is there something wrong with it?
> On Thu, 28 Feb 2008, Paul Franz wrote:
>
>   
>> True there are the legal issues. And it is not known if it would help on 
>> other platforms. My may concern is Windows because if git could be made 
>> available without some of msysGit's requirements. It would definitely 
>> make it more palletable (sp?) to that community which is a large chunk 
>> of developers, including my own work environment. I would love to switch 
>> from ClearCase to git but there are issues. One of them is integration 
>> with the Windows environment.
>>     
>
> I think that you do not help the effort to bring Git to Windows, by 
> suggesting even more work, for dubitable benefits.
>
> And that is what I think of APR here.
>
> NOTE: my only experience with APR on Windows is to get git-svn to compile, 
> and there it was not only a PITA.  The result is _SLOW_.
>   

Thanks for the info.
>   
>> Another concern is performance penalty of using non-native calls under 
>> Windows and that is one of the problems that I was hoping the using APR 
>> would solve. I figure if it is good enough to support the network stuff 
>> under Windows well maybe it would be good for git too.
>>     
>
> The problems with Git and Windows are not network related.  They are with 
> Windows' pitiful performance when it comes to spawn processes.
>   
Which makes me believe that it sounds like git depends on spawning 
processes being cheap.
> IMO your efforts to help Git on Windows would be better spent on the 
> msysGit list, where you would learn about the real issues we face.
>   

Excuse my ignorance,  could you tell me where the mysGit list is, so 
that I can subscribe?

> Ciao,
> Dscho
>
>
>   
-- 

-------------------------------------------

There are seven sins in the world.
     Wealth without work.
     Pleasure without conscience.
     Knowledge without character.
     Commerce without morality.
     Science without humanity.
     Worship without sacrifice.
     Politics without principle.

   -- Mohandas Gandhi

-------------------------------------------

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Junio C Hamano]

Paul Franz <thefranz@comcast.net> writes:

> Johannes Schindelin wrote:
>> Hi,
>>
>> [top-posting?]
>
> Is there something wrong with it?

Yes.  Please do not do it.

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Paul Franz]

Junio C Hamano wrote:
> Paul Franz <thefranz@comcast.net> writes:
>
>   
>> Johannes Schindelin wrote:
>>     
>>> Hi,
>>>
>>> [top-posting?]
>>>       
>> Is there something wrong with it?
>>     
>
> Yes.  Please do not do it.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   
Thanks. Noted for future reference.

-- 

-------------------------------------------

There are seven sins in the world.
     Wealth without work.
     Pleasure without conscience.
     Knowledge without character.
     Commerce without morality.
     Science without humanity.
     Worship without sacrifice.
     Politics without principle.

   -- Mohandas Gandhi

-------------------------------------------

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Paul Franz wrote:

> Johannes Schindelin wrote:
> 
> > [top-posting?]
> 
> Is there something wrong with it?

My favourite way to describe it (a quote):

+A: Because it messes up the order in which people normally read text.
+Q: Why is top-posting such a bad thing?
+A: Top-posting.
+Q: What is the most annoying thing on usenet and in e-mail?

> > The problems with Git and Windows are not network related.  They are 
> > with Windows' pitiful performance when it comes to spawn processes.
>
> Which makes me believe that it sounds like git depends on spawning 
> processes being cheap.

Yes, like everything that relies on being scriptable.

> > IMO your efforts to help Git on Windows would be better spent on the 
> > msysGit list, where you would learn about the real issues we face.
> 
> Excuse my ignorance, could you tell me where the mysGit list is, so that 
> I can subscribe?

It is referenced right on the first page on

	http://msysgit.googlecode.com/

under the keyword "Groups:".

Hth,
Dscho

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Johannes Sixt]

On Thursday 28 February 2008 16:28, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > In upload-pack we must explicitly close the output channel of rev-list.
> > (On Unix, the channel is closed automatically because process that runs
> > rev-list terminates.)
>
> When I read this patch, my impression was that it litters the source code
> with #ifdef's.  IMO this makes the code less readable, and as a
> consequence easer to fsck up.
>
> Unfortunately, I have no idea how to help that, other than implementing
> compat/thread.[ch], abstracting the thread functions, and introducing a
> NO_FORK Makefile variable and preprocessor constant.
>
> Hmpf.

The number of #ifdef/#endif is already at a minimum unless you are willing to 
have entire functions in separate #ifdef/#else/#endif branches. Whether to 
have compat/thread.[ch] or not is just a question of whether you want to have 
asynchronous functions in threads also on Unix or not.

-- Hannes

Re: [PATCH 22/40] Windows: Implement asynchronous functions as threads.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 16:28, Johannes Schindelin wrote:
>
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > > In upload-pack we must explicitly close the output channel of 
> > > rev-list. (On Unix, the channel is closed automatically because 
> > > process that runs rev-list terminates.)
> >
> > When I read this patch, my impression was that it litters the source 
> > code with #ifdef's.  IMO this makes the code less readable, and as a 
> > consequence easer to fsck up.
> >
> > Unfortunately, I have no idea how to help that, other than 
> > implementing compat/thread.[ch], abstracting the thread functions, and 
> > introducing a NO_FORK Makefile variable and preprocessor constant.
> >
> > Hmpf.
> 
> The number of #ifdef/#endif is already at a minimum unless you are 
> willing to have entire functions in separate #ifdef/#else/#endif 
> branches. Whether to have compat/thread.[ch] or not is just a question 
> of whether you want to have asynchronous functions in threads also on 
> Unix or not.

Well, my idea was that Git's code looks as good as it does now not only 
because the functions are well written, but because the 
conditionally-compiled code is not classified by _platform_ but by 
_reason_.

For example, we do not write that on FreeBSD, we do this and that, but 
we define for FreeBSD that there is no strlcpy(), and if NO_STRLCPY is 
defined, we implement it.

Likewise, I would prefer to have it in the code here _not_ that we do this 
and that on MinGW32, but that in the absence of a usable fork(), we 
require certain functions (declared in compat/thread.h, for example), and 
use them.

But I know this is a lot of work, so I hoped for a better solution, that 
still excites my "elegance neuron".

Ciao,
Dscho

[PATCH 23/40] Windows: Local clone must use the drive letter in absolute paths.

[Johannes Sixt]

The default of pwd of MSYS's bash and /bin/pwd are to use /c/rest/of/path
notation instead of c:/rest/of/path. But the former is not supported
by programs that use the standard C runtime (instead of MSYS's
runtime). Hence, we must make sure that only drive letter notations
are generated by using pwd's -W option.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-clone.sh |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index 0d686c3..640e29d 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -32,6 +32,25 @@ die() {
 	exit 1
 }
 
+# Fix some commands on Windows
+case $(uname -s) in
+*MINGW*)
+	# pwd must return a path with a drive letter
+	bin_pwd() {
+		# there are no symlinks to resolve: /bin/pwd is not needed
+		builtin pwd -W
+	}
+	pwd() {
+		builtin pwd -W
+	}
+	;;
+*)
+	bin_pwd() {
+		/bin/pwd
+	}
+	;;
+esac
+
 usage() {
 	exec "$0" -h
 }
@@ -40,7 +59,7 @@ eval "$(echo "$OPTIONS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)
 
 get_repo_base() {
 	(
-		cd "`/bin/pwd`" &&
+		cd "$(bin_pwd)" &&
 		cd "$1" || cd "$1.git" &&
 		{
 			cd .git
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 23/40] Windows: Local clone must use the drive letter in absolute paths.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> The default of pwd of MSYS's bash and /bin/pwd are to use 
> /c/rest/of/path notation instead of c:/rest/of/path. But the former is 
> not supported by programs that use the standard C runtime (instead of 
> MSYS's runtime). Hence, we must make sure that only drive letter 
> notations are generated by using pwd's -W option.

I guess that this patch will be obsolete when we have builtin clone.  So I 
have no objection to this pretty local workaround until then.

Ciao,
Dscho

[PATCH 24/40] Windows: Work around incompatible sort and find.

[Johannes Sixt]

If the PATH lists the Windows system directories before the MSYS
directories, Windows's own incompatible sort and find commands would be
picked up. We implement these commands as functions and call the real
tools by absolute path.

Also add a dummy implementation of sync to avoid an error in git-repack.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-clone.sh    |    4 ++++
 git-sh-setup.sh |   17 +++++++++++++++++
 t/test-lib.sh   |   13 +++++++++++++
 3 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index 640e29d..1fc5c92 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -35,6 +35,10 @@ die() {
 # Fix some commands on Windows
 case $(uname -s) in
 *MINGW*)
+	# Windows has its own (incompatible) find
+	find () {
+		/usr/bin/find "$@"
+	}
 	# pwd must return a path with a drive letter
 	bin_pwd() {
 		# there are no symlinks to resolve: /bin/pwd is not needed
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index a44b1c7..822aa6f 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -142,3 +142,20 @@ then
 	}
 	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
 fi
+
+# Fix some commands on Windows
+case $(uname -s) in
+*MINGW*)
+	# Windows has its own (incompatible) sort and find
+	sort () {
+		/usr/bin/sort "$@"
+	}
+	find () {
+		/usr/bin/find "$@"
+	}
+	# sync is missing
+	sync () {
+		:	# no implementation
+	}
+	;;
+esac
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83889c4..e2010d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -366,3 +366,16 @@ do
 		test_done
 	esac
 done
+
+# Fix some commands on Windows
+case $(uname -s) in
+*MINGW*)
+	# Windows has its own (incompatible) sort and find
+	sort () {
+		/usr/bin/sort "$@"
+	}
+	find () {
+		/usr/bin/find "$@"
+	}
+	;;
+esac
-- 
1.5.4.1.126.ge5a7d

[PATCH 25/40] Windows: Implement a cpio emulation in git-clone.sh.

[Johannes Sixt]

We use 'xargs cp' to emulate 'cpio -pumd'.

cpio is fed from the output of 'find --depth' without filtering out
directories. We don't need to copy directories except the empty ones, so
we need to filter out non-empty directories. Then we can use 'cp -r' and
it will not actually recurse anything.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-clone.sh |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index 1fc5c92..87fc0fb 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -39,6 +39,33 @@ case $(uname -s) in
 	find () {
 		/usr/bin/find "$@"
 	}
+	# need an emulation of cpio
+	cpio() {
+		case "$1" in
+		-pumd)	cp_arg=-pr;;
+		-pumdl)	cp_arg=-lr;;
+		*)	die "cpio $1 unexpected";;
+		esac
+		# copy only files and empty directories
+		prev=
+		while read f; do
+			if test -d "$f"; then
+				# here we assume that directories are listed after
+				# its files (aka 'find -depth'), hence, a directory
+				# that is not empty will be a leading sub-string
+				# of the preceding entry
+				case "$prev" in
+				"$f"/* ) ;;
+				*)	echo "$f";;
+				esac
+			else
+				echo "$f"
+			fi
+			prev="$f"
+		done |
+		xargs --no-run-if-empty \
+			cp $cp_arg --target-directory="$2" --parents
+	}
 	# pwd must return a path with a drive letter
 	bin_pwd() {
 		# there are no symlinks to resolve: /bin/pwd is not needed
-- 
1.5.4.1.126.ge5a7d

[PATCH 26/40] Windows: Implement wrappers for gethostbyname(), socket(), and connect().

[Johannes Sixt]

gethostbyname() is the first function that calls into the Winsock library,
and it is wrapped only to initialize the library.

socket() is wrapped for two reasons:
- Windows's socket() creates things that are like low-level file handles,
  and they must be converted into file descriptors first.
- And these handles cannot be used with plain ReadFile()/WriteFile()
  because they are opened for "overlapped IO". We have to use WSASocket()
  to create non-overlapped IO sockets.

connect() must be wrapped because Windows's connect() expects the low-level
sockets, not file descriptors, and we must first unwrap the file descriptor
before we can pass it on to Windows's connect().

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |    9 +++++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 837d741..1260be8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -436,6 +436,52 @@ char **env_setenv(char **env, const char *name)
 	return env;
 }
 
+/* this is the first function to call into WS_32; initialize it */
+#undef gethostbyname
+struct hostent *mingw_gethostbyname(const char *host)
+{
+	WSADATA wsa;
+
+	if (WSAStartup(MAKEWORD(2,2), &wsa))
+		die("unable to initialize winsock subsystem, error %d",
+			WSAGetLastError());
+	atexit((void(*)(void)) WSACleanup);
+	return gethostbyname(host);
+}
+
+int mingw_socket(int domain, int type, int protocol)
+{
+	int sockfd;
+	SOCKET s = WSASocket(domain, type, protocol, NULL, 0, 0);
+	if (s == INVALID_SOCKET) {
+		/*
+		 * WSAGetLastError() values are regular BSD error codes
+		 * biased by WSABASEERR.
+		 * However, strerror() does not know about networking
+		 * specific errors, which are values beginning at 38 or so.
+		 * Therefore, we choose to leave the biased error code
+		 * in errno so that _if_ someone looks up the code somewhere,
+		 * then it is at least the number that are usually listed.
+		 */
+		errno = WSAGetLastError();
+		return -1;
+	}
+	/* convert into a file descriptor */
+	if ((sockfd = _open_osfhandle(s, O_RDWR|O_BINARY)) < 0) {
+		closesocket(s);
+		return error("unable to make a socket file descriptor: %s",
+			strerror(errno));
+	}
+	return sockfd;
+}
+
+#undef connect
+int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)
+{
+	SOCKET s = (SOCKET)_get_osfhandle(sockfd);
+	return connect(s, sa, sz);
+}
+
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 046891d..0324789 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -596,6 +596,15 @@ int mingw_open (const char *filename, int oflags, ...);
 char *mingw_getcwd(char *pointer, int len);
 #define getcwd mingw_getcwd
 
+struct hostent *mingw_gethostbyname(const char *host);
+#define gethostbyname mingw_gethostbyname
+
+int mingw_socket(int domain, int type, int protocol);
+#define socket mingw_socket
+
+int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz);
+#define connect mingw_connect
+
 int mingw_rename(const char*, const char*);
 #define rename mingw_rename
 
-- 
1.5.4.1.126.ge5a7d

[PATCH 27/40] Windows: Implement a custom spawnve().

[Johannes Sixt]

The problem with Windows's own implementation is that it tries to be
clever when a console program is invoked from a GUI application: In this
case it sometimes automatically allocates a new console window. As a
consequence, the IO channels of the spawned program are directed to the
console, but the invoking application listens on channels that are now
directed to nowhere.

In this implementation we use the lowlevel facilities of CreateProcess(),
which offers a flag to tell the system not to open a console. As a side
effect, only stdin, stdout, and stderr channels will be accessible from
C programs that are spawned. Other channels (file handles, pipe handles,
etc.) are still inherited by the spawned program, but it doesn't get
enough information to access them.

Johannes Schindelin integrated path quoting and unified the various
*execv* and *spawnv* helpers.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |  203 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 git-compat-util.h |    1 +
 run-command.c     |    2 +-
 3 files changed, 199 insertions(+), 7 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 1260be8..146c170 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1,4 +1,5 @@
 #include "../git-compat-util.h"
+#include "../strbuf.h"
 
 unsigned int _CRT_fmode = _O_BINARY;
 
@@ -190,6 +191,65 @@ char *mingw_getcwd(char *pointer, int len)
 	return ret;
 }
 
+/*
+ * See http://msdn2.microsoft.com/en-us/library/17w5ykft(vs.71).aspx
+ * (Parsing C++ Command-Line Arguments)
+ */
+static const char *quote_arg(const char *arg)
+{
+	/* count chars to quote */
+	int len = 0, n = 0;
+	int force_quotes = 0;
+	char *q, *d;
+	const char *p = arg;
+	if (!*p) force_quotes = 1;
+	while (*p) {
+		if (isspace(*p) || *p == '*' || *p == '?')
+			force_quotes = 1;
+		else if (*p == '"')
+			n++;
+		else if (*p == '\\') {
+			int count = 0;
+			while (*p == '\\') {
+				count++;
+				p++;
+				len++;
+			}
+			if (*p == '"')
+				n += count*2 + 1;
+			continue;
+		}
+		len++;
+		p++;
+	}
+	if (!force_quotes && n == 0)
+		return arg;
+
+	/* insert \ where necessary */
+	d = q = xmalloc(len+n+3);
+	*d++ = '"';
+	while (*arg) {
+		if (*arg == '"')
+			*d++ = '\\';
+		else if (*arg == '\\') {
+			int count = 0;
+			while (*arg == '\\') {
+				count++;
+				*d++ = *arg++;
+			}
+			if (*arg == '"') {
+				while (count-- > 0)
+					*d++ = '\\';
+				*d++ = '\\';
+			}
+		}
+		*d++ = *arg++;
+	}
+	*d++ = '"';
+	*d++ = 0;
+	return q;
+}
+
 static const char *parse_interpreter(const char *cmd)
 {
 	static char buf[100];
@@ -311,6 +371,138 @@ static char *path_lookup(const char *cmd, char **path, int exe_only)
 	return prog;
 }
 
+static int env_compare(const void *a, const void *b)
+{
+	char *const *ea = a;
+	char *const *eb = b;
+	return strcasecmp(*ea, *eb);
+}
+
+static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
+			   int prepend_cmd)
+{
+	STARTUPINFO si;
+	PROCESS_INFORMATION pi;
+	struct strbuf envblk, args;
+	unsigned flags;
+	BOOL ret;
+
+	/* Determine whether or not we are associated to a console */
+	HANDLE cons = CreateFile("CONOUT$", GENERIC_WRITE,
+			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
+			FILE_ATTRIBUTE_NORMAL, NULL);
+	if (cons == INVALID_HANDLE_VALUE) {
+		/* There is no console associated with this process.
+		 * Since the child is a console process, Windows
+		 * would normally create a console window. But
+		 * since we'll be redirecting std streams, we do
+		 * not need the console.
+		 */
+		flags = CREATE_NO_WINDOW;
+	} else {
+		/* There is already a console. If we specified
+		 * CREATE_NO_WINDOW here, too, Windows would
+		 * disassociate the child from the console.
+		 * Go figure!
+		 */
+		flags = 0;
+		CloseHandle(cons);
+	}
+	memset(&si, 0, sizeof(si));
+	si.cb = sizeof(si);
+	si.dwFlags = STARTF_USESTDHANDLES;
+	si.hStdInput = (HANDLE) _get_osfhandle(0);
+	si.hStdOutput = (HANDLE) _get_osfhandle(1);
+	si.hStdError = (HANDLE) _get_osfhandle(2);
+
+	/* concatenate argv, quoting args as we go */
+	strbuf_init(&args, 0);
+	if (prepend_cmd) {
+		char *quoted = (char *)quote_arg(cmd);
+		strbuf_addstr(&args, quoted);
+		if (quoted != cmd)
+			free(quoted);
+	}
+	for (; *argv; argv++) {
+		char *quoted = (char *)quote_arg(*argv);
+		if (*args.buf)
+			strbuf_addch(&args, ' ');
+		strbuf_addstr(&args, quoted);
+		if (quoted != *argv)
+			free(quoted);
+	}
+
+	if (env) {
+		int count = 0;
+		char **e, **sorted_env;
+
+		for (e = env; *e; e++)
+			count++;
+
+		/* environment must be sorted */
+		sorted_env = xmalloc(sizeof(*sorted_env) * (count + 1));
+		memcpy(sorted_env, env, sizeof(*sorted_env) * (count + 1));
+		qsort(sorted_env, count, sizeof(*sorted_env), env_compare);
+
+		strbuf_init(&envblk, 0);
+		for (e = sorted_env; *e; e++) {
+			strbuf_addstr(&envblk, *e);
+			strbuf_addch(&envblk, '\0');
+		}
+		free(sorted_env);
+	}
+
+	memset(&pi, 0, sizeof(pi));
+	ret = CreateProcess(cmd, args.buf, NULL, NULL, TRUE, flags,
+		env ? envblk.buf : NULL, NULL, &si, &pi);
+
+	if (env)
+		strbuf_release(&envblk);
+	strbuf_release(&args);
+
+	if (!ret) {
+		errno = ENOENT;
+		return -1;
+	}
+	CloseHandle(pi.hThread);
+	return (pid_t)pi.hProcess;
+}
+
+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
+{
+	pid_t pid;
+	char **path = get_path_split();
+	char *prog = path_lookup(cmd, path, 0);
+
+	if (!prog) {
+		errno = ENOENT;
+		pid = -1;
+	}
+	else {
+		const char *interpr = parse_interpreter(prog);
+
+		if (interpr) {
+			const char *argv0 = argv[0];
+			char *iprog = path_lookup(interpr, path, 1);
+			argv[0] = prog;
+			if (!iprog) {
+				errno = ENOENT;
+				pid = -1;
+			}
+			else {
+				pid = mingw_spawnve(iprog, argv, env, 1);
+				free(iprog);
+			}
+			argv[0] = argv0;
+		}
+		else
+			pid = mingw_spawnve(prog, argv, env, 0);
+		free(prog);
+	}
+	free_path_split(path);
+	return pid;
+}
+
 static int try_shell_exec(const char *cmd, char *const *argv, char **env)
 {
 	const char *interpr = parse_interpreter(cmd);
@@ -326,11 +518,10 @@ static int try_shell_exec(const char *cmd, char *const *argv, char **env)
 		int argc = 0;
 		const char **argv2;
 		while (argv[argc]) argc++;
-		argv2 = xmalloc(sizeof(*argv) * (argc+2));
-		argv2[0] = (char *)interpr;
-		argv2[1] = (char *)cmd;	/* full path to the script file */
-		memcpy(&argv2[2], &argv[1], sizeof(*argv) * argc);
-		pid = spawnve(_P_NOWAIT, prog, argv2, (const char **)env);
+		argv2 = xmalloc(sizeof(*argv) * (argc+1));
+		argv2[0] = (char *)cmd;	/* full path to the script file */
+		memcpy(&argv2[1], &argv[1], sizeof(*argv) * argc);
+		pid = mingw_spawnve(prog, argv2, env, 1);
 		if (pid >= 0) {
 			int status;
 			if (waitpid(pid, &status, 0) < 0)
@@ -351,7 +542,7 @@ static void mingw_execve(const char *cmd, char *const *argv, char *const *env)
 	if (!try_shell_exec(cmd, argv, (char **)env)) {
 		int pid, status;
 
-		pid = spawnve(_P_NOWAIT, cmd, (const char **)argv, (const char **)env);
+		pid = mingw_spawnve(cmd, (const char **)argv, (char **)env, 0);
 		if (pid < 0)
 			return;
 		if (waitpid(pid, &status, 0) < 0)
diff --git a/git-compat-util.h b/git-compat-util.h
index 0324789..570eb10 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -611,6 +611,7 @@ int mingw_rename(const char*, const char*);
 int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
 #define vsnprintf mingw_vsnprintf
 
+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
 void mingw_execvp(const char *cmd, char *const *argv);
 #define execvp mingw_execvp
 
diff --git a/run-command.c b/run-command.c
index 3834f86..5ed338c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -156,7 +156,7 @@ int start_command(struct child_process *cmd)
 		cmd->argv[0] = git_cmd.buf;
 	}
 
-	cmd->pid = spawnvpe(_P_NOWAIT, cmd->argv[0], cmd->argv, (const char **)env);
+	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
 
 	if (cmd->env)
 		free_environ(env);
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 27/40] Windows: Implement a custom spawnve().

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 0324789..570eb10 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -611,6 +611,7 @@ int mingw_rename(const char*, const char*);
>  int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
>  #define vsnprintf mingw_vsnprintf
>  
> +pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
>  void mingw_execvp(const char *cmd, char *const *argv);
>  #define execvp mingw_execvp
>  
> diff --git a/run-command.c b/run-command.c
> index 3834f86..5ed338c 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -156,7 +156,7 @@ int start_command(struct child_process *cmd)
>  		cmd->argv[0] = git_cmd.buf;
>  	}
>  
> -	cmd->pid = spawnvpe(_P_NOWAIT, cmd->argv[0], cmd->argv, (const char **)env);
> +	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);

Should this not be "#define spawnvpe mingw_spanvpe" in git-compat-util.h 
instead?

Ciao,
Dscho

Re: [PATCH 27/40] Windows: Implement a custom spawnve().

[Johannes Sixt]

On Thursday 28 February 2008 16:36, Johannes Schindelin wrote:
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > -	cmd->pid = spawnvpe(_P_NOWAIT, cmd->argv[0], cmd->argv, (const 
char**)env); 
> > +	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
>
> Should this not be "#define spawnvpe mingw_spanvpe" in git-compat-util.h
> instead?

No. We don't mimic the API of the original spawnvpe. Why obfuscate this fact?

-- Hannes

Re: [PATCH 27/40] Windows: Implement a custom spawnve().

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 16:36, Johannes Schindelin wrote:
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > > -	cmd->pid = spawnvpe(_P_NOWAIT, cmd->argv[0], cmd->argv, (const 
> char**)env); 
> > > +	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
> >
> > Should this not be "#define spawnvpe mingw_spanvpe" in git-compat-util.h
> > instead?
> 
> No. We don't mimic the API of the original spawnvpe. Why obfuscate this fact?

Oops.  I forgot that.  Maybe we do not want to call the function 
mingw_spawnvpe(), then?  (That fact is what made me believe that the 
#define was a good idea, after all.)

Ciao,
Dscho

[PATCH 28/40] Windows: Add a new lstat and fstat implementation based on Win32 API.

[Johannes Sixt]

From: Marius Storm-Olsen <mstormo_git@storm-olsen.com>

From: Marius Storm-Olsen <mstormo_git@storm-olsen.com>

This gives us a significant speedup when adding, committing and stat'ing files.
Also, since Windows doesn't really handle symlinks, we let stat just uses lstat.
We also need to replace fstat, since our implementation and the standard stat()
functions report slightly different timestamps, possibly due to timezones.

We simply report UTC in our implementation, and do our FILETIME to time_t
conversion based on the document at http://support.microsoft.com/kb/167296.

With Moe's repo structure (100K files in 100 dirs, containing 2-4 bytes)
    mkdir bummer && cd bummer; for ((i=0;i<100;i++)); do
      mkdir $i && pushd $i;
        for ((j=0;j<1000;j++)); do echo "$j" >$j; done;
      popd;
    done

We get the following performance boost:

    With normal lstat & stat  Custom lstat/fstat
    ------------------------  ------------------------
    Command: git init         Command: git init
    ------------------------  ------------------------
    real    0m 0.047s          real   0m 0.063s
    user    0m 0.031s          user   0m 0.015s
    sys     0m 0.000s          sys    0m 0.015s
    ------------------------  ------------------------
    Command: git add .        Command: git add .
    ------------------------  ------------------------
    real    0m19.390s         real    0m12.031s       1.6x
    user    0m 0.015s         user    0m 0.031s
    sys     0m 0.030s         sys     0m 0.000s
    ------------------------  ------------------------
    Command: git commit -a..  Command: git commit -a..
    ------------------------  ------------------------
    real    0m30.812s         real    0m16.875s       1.8x
    user    0m 0.015s         user    0m 0.015s
    sys     0m 0.000s         sys     0m 0.015s
    ------------------------  ------------------------
    3x Command: git-status    3x Command: git-status
    ------------------------  ------------------------
    real    0m11.860s         real    0m 5.266s       2.2x
    user    0m 0.015s         user    0m 0.015s
    sys     0m 0.015s         sys     0m 0.015s

    real    0m11.703s         real    0m 5.234s
    user    0m 0.015s         user    0m 0.015s
    sys     0m 0.000s         sys     0m 0.000s

    real    0m11.672s         real    0m 5.250s
    user    0m 0.031s         user    0m 0.015s
    sys     0m 0.000s         sys     0m 0.000s
    ------------------------  ------------------------
    Command: git commit...    Command: git commit...
    (single file)             (single file)
    ------------------------  ------------------------
    real    0m14.234s         real    0m 7.735s       1.8x
    user    0m 0.015s         user    0m 0.031s
    sys     0m 0.000s         sys     0m 0.000s

Signed-off-by: Marius Storm-Olsen <mstormo_git@storm-olsen.com>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |   10 ++++-
 2 files changed, 141 insertions(+), 1 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 146c170..d44fbb3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -23,6 +23,138 @@ int mingw_open (const char *filename, int oflags, ...)
 	return fd;
 }
 
+static inline time_t filetime_to_time_t(const FILETIME *ft)
+{
+	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
+	winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */
+	winTime /= 10000000;		 /* Nano to seconds resolution */
+	return (time_t)winTime;
+}
+
+extern int _getdrive( void );
+/* We keep the do_lstat code in a separate function to avoid recursion.
+ * When a path ends with a slash, the stat will fail with ENOENT. In
+ * this case, we strip the trailing slashes and stat again.
+ */
+static int do_lstat(const char *file_name, struct stat *buf)
+{
+	WIN32_FILE_ATTRIBUTE_DATA fdata;
+
+	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
+		int fMode = S_IREAD;
+		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+			fMode |= S_IFDIR;
+		else
+			fMode |= S_IFREG;
+		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+			fMode |= S_IWRITE;
+
+		buf->st_ino = 0;
+		buf->st_gid = 0;
+		buf->st_uid = 0;
+		buf->st_nlink = 1;
+		buf->st_mode = fMode;
+		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
+		buf->st_dev = buf->st_rdev = (_getdrive() - 1);
+		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
+		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
+		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+		errno = 0;
+		return 0;
+	}
+
+	switch (GetLastError()) {
+	case ERROR_ACCESS_DENIED:
+	case ERROR_SHARING_VIOLATION:
+	case ERROR_LOCK_VIOLATION:
+	case ERROR_SHARING_BUFFER_EXCEEDED:
+		errno = EACCES;
+		break;
+	case ERROR_BUFFER_OVERFLOW:
+		errno = ENAMETOOLONG;
+		break;
+	case ERROR_NOT_ENOUGH_MEMORY:
+		errno = ENOMEM;
+		break;
+	default:
+		errno = ENOENT;
+		break;
+	}
+	return -1;
+}
+
+/* We provide our own lstat/fstat functions, since the provided
+ * lstat/fstat functions are so slow. These stat functions are
+ * tailored for Git's usage (read: fast), and are not meant to be
+ * complete. Note that Git stat()s are redirected to mingw_lstat()
+ * too, since Windows doesn't really handle symlinks that well.
+ */
+int mingw_lstat(const char *file_name, struct stat *buf)
+{
+	int namelen;
+	static char alt_name[PATH_MAX];
+
+	if (!do_lstat(file_name, buf))
+		return 0;
+
+	/* if file_name ended in a '/', Windows returned ENOENT;
+	 * try again without trailing slashes
+	 */
+	if (errno != ENOENT)
+		return -1;
+
+	namelen = strlen(file_name);
+	if (namelen && file_name[namelen-1] != '/')
+		return -1;
+	while (namelen && file_name[namelen-1] == '/')
+		--namelen;
+	if (!namelen || namelen >= PATH_MAX)
+		return -1;
+
+	memcpy(alt_name, file_name, namelen);
+	alt_name[namelen] = 0;
+	return do_lstat(alt_name, buf);
+}
+
+#undef fstat
+int mingw_fstat(int fd, struct stat *buf)
+{
+	HANDLE fh = (HANDLE)_get_osfhandle(fd);
+	BY_HANDLE_FILE_INFORMATION fdata;
+
+	if (fh == INVALID_HANDLE_VALUE) {
+		errno = EBADF;
+		return -1;
+	}
+	/* direct non-file handles to MS's fstat() */
+	if (GetFileType(fh) != FILE_TYPE_DISK)
+		return fstat(fd, buf);
+
+	if (GetFileInformationByHandle(fh, &fdata)) {
+		int fMode = S_IREAD;
+		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+			fMode |= S_IFDIR;
+		else
+			fMode |= S_IFREG;
+		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+			fMode |= S_IWRITE;
+
+		buf->st_ino = 0;
+		buf->st_gid = 0;
+		buf->st_uid = 0;
+		buf->st_nlink = 1;
+		buf->st_mode = fMode;
+		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
+		buf->st_dev = buf->st_rdev = (_getdrive() - 1);
+		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
+		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
+		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+		return 0;
+	}
+	errno = EBADF;
+	return -1;
+}
+
 unsigned int sleep (unsigned int seconds)
 {
 	Sleep(seconds*1000);
diff --git a/git-compat-util.h b/git-compat-util.h
index 570eb10..0583a1f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -509,7 +509,6 @@ struct itimerval {
 #define ITIMER_REAL 0
 
 #define st_blocks st_size/512	/* will be cleaned up later */
-#define lstat stat
 
 /*
  * trivial stubs
@@ -608,6 +607,15 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz);
 int mingw_rename(const char*, const char*);
 #define rename mingw_rename
 
+/* Use mingw_lstat() instead of lstat()/stat() and
+ * mingw_fstat() instead of fstat() on Windows.
+ */
+int mingw_lstat(const char *file_name, struct stat *buf);
+int mingw_fstat(int fd, struct stat *buf);
+#define fstat mingw_fstat
+#define lstat mingw_lstat
+#define stat(x,y) mingw_lstat(x,y)
+
 int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
 #define vsnprintf mingw_vsnprintf
 
-- 
1.5.4.1.126.ge5a7d

[PATCH 29/40] Windows: Use a customized struct stat that also has the st_blocks member.

[Johannes Sixt]

Windows's struct stat does not have a st_blocks member. Since we already
have our own stat/lstat/fstat implementations, we can just as well use
a customized struct stat. This patch introduces just that, and also fills
in the st_blocks member. On the other hand, we don't provide members that
are never used.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c    |   36 ++++++++++++++++++++++++++++--------
 git-compat-util.h |   18 +++++++++++++-----
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index d44fbb3..0888288 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -31,6 +31,11 @@ static inline time_t filetime_to_time_t(const FILETIME *ft)
 	return (time_t)winTime;
 }
 
+static inline size_t size_to_blocks(size_t s)
+{
+	return (s+511)/512;
+}
+
 extern int _getdrive( void );
 /* We keep the do_lstat code in a separate function to avoid recursion.
  * When a path ends with a slash, the stat will fail with ENOENT. In
@@ -52,10 +57,10 @@ static int do_lstat(const char *file_name, struct stat *buf)
 		buf->st_ino = 0;
 		buf->st_gid = 0;
 		buf->st_uid = 0;
-		buf->st_nlink = 1;
 		buf->st_mode = fMode;
 		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
-		buf->st_dev = buf->st_rdev = (_getdrive() - 1);
+		buf->st_blocks = size_to_blocks(buf->st_size);
+		buf->st_dev = _getdrive() - 1;
 		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
 		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
 		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
@@ -89,7 +94,7 @@ static int do_lstat(const char *file_name, struct stat *buf)
  * complete. Note that Git stat()s are redirected to mingw_lstat()
  * too, since Windows doesn't really handle symlinks that well.
  */
-int mingw_lstat(const char *file_name, struct stat *buf)
+int mingw_lstat(const char *file_name, struct mingw_stat *buf)
 {
 	int namelen;
 	static char alt_name[PATH_MAX];
@@ -117,7 +122,8 @@ int mingw_lstat(const char *file_name, struct stat *buf)
 }
 
 #undef fstat
-int mingw_fstat(int fd, struct stat *buf)
+#undef stat
+int mingw_fstat(int fd, struct mingw_stat *buf)
 {
 	HANDLE fh = (HANDLE)_get_osfhandle(fd);
 	BY_HANDLE_FILE_INFORMATION fdata;
@@ -127,8 +133,22 @@ int mingw_fstat(int fd, struct stat *buf)
 		return -1;
 	}
 	/* direct non-file handles to MS's fstat() */
-	if (GetFileType(fh) != FILE_TYPE_DISK)
-		return fstat(fd, buf);
+	if (GetFileType(fh) != FILE_TYPE_DISK) {
+		struct stat st;
+		if (fstat(fd, &st))
+			return -1;
+		buf->st_ino = st.st_ino;
+		buf->st_gid = st.st_gid;
+		buf->st_uid = st.st_uid;
+		buf->st_mode = st.st_mode;
+		buf->st_size = st.st_size;
+		buf->st_blocks = size_to_blocks(buf->st_size);
+		buf->st_dev = st.st_dev;
+		buf->st_atime = st.st_atime;
+		buf->st_mtime = st.st_mtime;
+		buf->st_ctime = st.st_ctime;
+		return 0;
+	}
 
 	if (GetFileInformationByHandle(fh, &fdata)) {
 		int fMode = S_IREAD;
@@ -142,10 +162,10 @@ int mingw_fstat(int fd, struct stat *buf)
 		buf->st_ino = 0;
 		buf->st_gid = 0;
 		buf->st_uid = 0;
-		buf->st_nlink = 1;
 		buf->st_mode = fMode;
 		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
-		buf->st_dev = buf->st_rdev = (_getdrive() - 1);
+		buf->st_blocks = size_to_blocks(buf->st_size);
+		buf->st_dev = _getdrive() - 1;
 		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
 		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
 		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
diff --git a/git-compat-util.h b/git-compat-util.h
index 0583a1f..4a53b3b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -508,8 +508,6 @@ struct itimerval {
 };
 #define ITIMER_REAL 0
 
-#define st_blocks st_size/512	/* will be cleaned up later */
-
 /*
  * trivial stubs
  */
@@ -609,12 +607,22 @@ int mingw_rename(const char*, const char*);
 
 /* Use mingw_lstat() instead of lstat()/stat() and
  * mingw_fstat() instead of fstat() on Windows.
+ * struct stat is redefined because it lacks the st_blocks member.
  */
-int mingw_lstat(const char *file_name, struct stat *buf);
-int mingw_fstat(int fd, struct stat *buf);
+struct mingw_stat {
+	unsigned st_mode;
+	time_t st_mtime, st_atime, st_ctime;
+	unsigned st_dev, st_ino, st_uid, st_gid;
+	size_t st_size;
+	size_t st_blocks;
+};
+int mingw_lstat(const char *file_name, struct mingw_stat *buf);
+int mingw_fstat(int fd, struct mingw_stat *buf);
 #define fstat mingw_fstat
 #define lstat mingw_lstat
-#define stat(x,y) mingw_lstat(x,y)
+#define stat mingw_stat
+static inline int mingw_stat(const char *file_name, struct mingw_stat *buf)
+{ return mingw_lstat(file_name, buf); }
 
 int mingw_vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
 #define vsnprintf mingw_vsnprintf
-- 
1.5.4.1.126.ge5a7d

[PATCH 30/40] Turn builtin_exec_path into a function.

[Johannes Sixt]

builtin_exec_path returns the hard-coded installation path, which is used
as the ultimate fallback to look for git commands. Making it into a function
enables us in a follow-up patch to return a computed value instead of just
a constant string.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 exec_cmd.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 343545d..3f3fa79 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -4,9 +4,13 @@
 #define MAX_ARGS	32
 
 extern char **environ;
-static const char *builtin_exec_path = GIT_EXEC_PATH;
 static const char *argv_exec_path;
 
+static const char *builtin_exec_path(void)
+{
+	return GIT_EXEC_PATH;
+}
+
 void git_set_argv_exec_path(const char *exec_path)
 {
 	argv_exec_path = exec_path;
@@ -26,7 +30,7 @@ const char *git_exec_path(void)
 		return env;
 	}
 
-	return builtin_exec_path;
+	return builtin_exec_path();
 }
 
 static void add_path(struct strbuf *out, const char *path)
@@ -54,7 +58,7 @@ void setup_path(const char *cmd_path)
 
 	add_path(&new_path, argv_exec_path);
 	add_path(&new_path, getenv(EXEC_PATH_ENVIRONMENT));
-	add_path(&new_path, builtin_exec_path);
+	add_path(&new_path, builtin_exec_path());
 	add_path(&new_path, cmd_path);
 
 	if (old_path)
-- 
1.5.4.1.126.ge5a7d

[PATCH 31/40] Compute the ultimate fallback for exec_path from the program invocation.

[Johannes Sixt]

Since on Windows the user is fairly free where to install programs, we
cannot rely on a hard-coded path. We use the program name to derive the
installation directory and use that as exec_path.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 exec_cmd.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 3f3fa79..6d2f740 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -8,7 +8,36 @@ static const char *argv_exec_path;
 
 static const char *builtin_exec_path(void)
 {
+#ifndef __MINGW32__
 	return GIT_EXEC_PATH;
+#else
+	int len;
+	char *p, *q, *sl;
+	static char *ep;
+	if (ep)
+		return ep;
+
+	len = strlen(_pgmptr);
+	if (len < 2)
+		return ep = ".";
+
+	p = ep = xmalloc(len+1);
+	q = _pgmptr;
+	sl = NULL;
+	/* copy program name, turn '\\' into '/', skip last part */
+	while ((*p = *q)) {
+		if (*q == '\\' || *q == '/') {
+			*p = '/';
+			sl = p;
+		}
+		p++, q++;
+	}
+	if (sl)
+		*sl = '\0';
+	else
+		ep[0] = '.', ep[1] = '\0';
+	return ep;
+#endif
 }
 
 void git_set_argv_exec_path(const char *exec_path)
-- 
1.5.4.1.126.ge5a7d

[PATCH 32/40] Windows: Use a relative default template_dir and ETC_GITCONFIG

[Johannes Sixt]

With this definition the templates and system config file will be found
irrespective of the installation location.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 7d4a591..15b13c0 100644
--- a/Makefile
+++ b/Makefile
@@ -548,6 +548,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	EXTLIBS += -lws2_32
 	X = .exe
 	NOEXECTEMPL = .noexec
+	template_dir = ../share/git-core/templates/
+	ETC_GITCONFIG = ../etc/gitconfig
 endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
-- 
1.5.4.1.126.ge5a7d

[PATCH 33/40] When installing, be prepared that template_dir may be relative.

[Johannes Sixt]

Since the Makefile in the template/ subdirectory is only used to install
the templates, we do not simply pass down the setting of template_dir
when it is relative, but construct the intended destination in a new
variable: A relative template_dir is relative to gitexecdir.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile           |    9 ++++++++-
 templates/Makefile |    8 ++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 15b13c0..53a4e2a 100644
--- a/Makefile
+++ b/Makefile
@@ -197,7 +197,7 @@ GITWEB_FAVICON = git-favicon.png
 GITWEB_SITE_HEADER =
 GITWEB_SITE_FOOTER =
 
-export prefix bindir gitexecdir sharedir template_dir htmldir sysconfdir
+export prefix bindir gitexecdir sharedir htmldir sysconfdir
 
 CC = gcc
 AR = ar
@@ -1094,6 +1094,13 @@ remove-dashes:
 
 ### Installation rules
 
+ifeq ($(firstword $(subst /, ,$(template_dir))),..)
+template_instdir = $(gitexecdir)/$(template_dir)
+else
+template_instdir = $template_dir
+endif
+export template_instdir
+
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexecdir_SQ)'
diff --git a/templates/Makefile b/templates/Makefile
index b341105..eb08702 100644
--- a/templates/Makefile
+++ b/templates/Makefile
@@ -8,14 +8,14 @@ INSTALL ?= install
 TAR ?= tar
 RM ?= rm -f
 prefix ?= $(HOME)
-template_dir ?= $(prefix)/share/git-core/templates
+template_instdir ?= $(prefix)/share/git-core/templates
 # DESTDIR=
 # set NOEXECTEMPL to non-empty to change the names of hook scripts
 # so that the tools will not find them
 
 # Shell quote (do not use $(call) to accommodate ancient setups);
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
-template_dir_SQ = $(subst ','\'',$(template_dir))
+template_instdir_SQ = $(subst ','\'',$(template_instdir))
 
 all: boilerplates.made custom
 
@@ -52,6 +52,6 @@ clean:
 	$(RM) -r blt boilerplates.made
 
 install: all
-	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(template_dir_SQ)'
+	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(template_instdir_SQ)'
 	(cd blt && $(TAR) cf - .) | \
-	(cd '$(DESTDIR_SQ)$(template_dir_SQ)' && $(TAR) xf -)
+	(cd '$(DESTDIR_SQ)$(template_instdir_SQ)' && $(TAR) xf -)
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 33/40] When installing, be prepared that template_dir may be relative.

[Paolo Bonzini]

> +ifeq ($(firstword $(subst /, ,$(template_dir))),..)
> +template_instdir = $(gitexecdir)/$(template_dir)
> +else
> +template_instdir = $template_dir
> +endif
> +export template_instdir

This is just a little more versatile:

ifeq ($(firstword $(subst /,
,ALONE-IF-ABSOLUTE$(template_dir))),ALONE-IF-ABSOLUTE)
template_instdir = $template_dir
else
template_instdir = $(gitexecdir)/$(template_dir)
endif
export template_instdir

Paolo

Re: [PATCH 33/40] When installing, be prepared that template_dir may be relative.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> @@ -1094,6 +1094,13 @@ remove-dashes:
>  
>  ### Installation rules
>  
> +ifeq ($(firstword $(subst /, ,$(template_dir))),..)

How portable is "firstword"?  Would it not be better to use

ifneq ($(patsubst ../%,,$(template_dir)),)

Hmm?

Ciao,
Dscho

Re: [PATCH 33/40] When installing, be prepared that template_dir may be relative.

[Paolo Bonzini]

>> +ifeq ($(firstword $(subst /, ,$(template_dir))),..)
> 
> How portable is "firstword"?  Would it not be better to use

At least make 3.78.1 (1999), search func_firstword in 
http://snipurl.com/20lle (Google Code Search).

Paolo

Re: [PATCH 33/40] When installing, be prepared that template_dir may be relative.

[Johannes Sixt]

On Thursday 28 February 2008 16:45, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > @@ -1094,6 +1094,13 @@ remove-dashes:
> >
> >  ### Installation rules
> >
> > +ifeq ($(firstword $(subst /, ,$(template_dir))),..)
>
> How portable is "firstword"?  Would it not be better to use

How portable is "patsubst"? ;)

> ifneq ($(patsubst ../%,,$(template_dir)),)

This programming language is sort of write-only, so I don't know because I 
can't read it ;) If you have tested your version and it works and you have 
thought through all consequences (like how does it work if 
template_dir=/a/../b), why not?

-- Hannes

Re: [PATCH 33/40] When installing, be prepared that template_dir may be relative.

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Johannes Sixt wrote:

> On Thursday 28 February 2008 16:45, Johannes Schindelin wrote:
>
> > On Wed, 27 Feb 2008, Johannes Sixt wrote:
> > > @@ -1094,6 +1094,13 @@ remove-dashes:
> > >
> > >  ### Installation rules
> > >
> > > +ifeq ($(firstword $(subst /, ,$(template_dir))),..)
> >
> > How portable is "firstword"?  Would it not be better to use
> 
> How portable is "patsubst"? ;)
> 
> > ifneq ($(patsubst ../%,,$(template_dir)),)
> 
> This programming language is sort of write-only, so I don't know because 
> I can't read it ;) If you have tested your version and it works and you 
> have thought through all consequences (like how does it work if 
> template_dir=/a/../b), why not?

I did not think through all consequences, but hoped that some knowledgable 
person could tell me that/if I am wrong in my assumption that patsubst is 
older than firstword.

Since I encountered "firstword" for the very first time today, I thought 
it might be younger than "patsubst", which I met plenty a times.

Ciao,
Dscho

[PATCH 34/40] Windows: Make the pager work.

[Johannes Sixt]

Since we have neither fork() nor exec(), we have to spawn the pager and
feed it with the program's output.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 pager.c |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index ca002f9..3f7f8c1 100644
--- a/pager.c
+++ b/pager.c
@@ -1,12 +1,13 @@
 #include "cache.h"
 
 /*
- * This is split up from the rest of git so that we might do
- * something different on Windows, for example.
+ * This is split up from the rest of git so that we can do
+ * something different on Windows.
  */
 
 static int spawned_pager;
 
+#ifndef __MINGW32__
 static void run_pager(const char *pager)
 {
 	/*
@@ -22,11 +23,31 @@ static void run_pager(const char *pager)
 	execlp(pager, pager, NULL);
 	execl("/bin/sh", "sh", "-c", pager, NULL);
 }
+#else
+#include "run-command.h"
+
+static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
+static struct child_process pager_process = {
+	.argv = pager_argv,
+	.in = -1
+};
+static void wait_for_pager(void)
+{
+	fflush(stdout);
+	fflush(stderr);
+	/* signal EOF to pager */
+	close(1);
+	close(2);
+	finish_command(&pager_process);
+}
+#endif
 
 void setup_pager(void)
 {
+#ifndef __MINGW32__
 	pid_t pid;
 	int fd[2];
+#endif
 	const char *pager = getenv("GIT_PAGER");
 
 	if (!isatty(1))
@@ -45,6 +66,7 @@ void setup_pager(void)
 
 	spawned_pager = 1; /* means we are emitting to terminal */
 
+#ifndef __MINGW32__
 	if (pipe(fd) < 0)
 		return;
 	pid = fork();
@@ -72,6 +94,20 @@ void setup_pager(void)
 	run_pager(pager);
 	die("unable to execute pager '%s'", pager);
 	exit(255);
+#else
+	/* spawn the pager */
+	pager_argv[2] = pager;
+	if (start_command(&pager_process))
+		return;
+
+	/* original process continues, but writes to the pipe */
+	dup2(pager_process.in, 1);
+	dup2(pager_process.in, 2);
+	close(pager_process.in);
+
+	/* this makes sure that the parent terminates after the pager */
+	atexit(wait_for_pager);
+#endif
 }
 
 int pager_in_use(void)
-- 
1.5.4.1.126.ge5a7d

[PATCH 35/40] Windows: Work around an oddity when a pipe with no reader is written to.

[Johannes Sixt]

On Windows, write() is implemented using WriteFile(). After the reader
closed its end of the pipe, the first WriteFile() returns
ERROR_BROKEN_PIPE (which translates to EPIPE), subsequent WriteFile()s
return ERROR_NO_DATA, which is translated to EINVAL.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 write_or_die.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/write_or_die.c b/write_or_die.c
index e125e11..f3b8566 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -34,7 +34,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 			return;
 	}
 	if (fflush(f)) {
-		if (errno == EPIPE)
+		/*
+		 * On Windows, EPIPE is returned only by the first write()
+		 * after the reading end has closed its handle; subsequent
+		 * write()s return EINVAL.
+		 */
+		if (errno == EPIPE || errno == EINVAL)
 			exit(0);
 		die("write failure on %s: %s", desc, strerror(errno));
 	}
-- 
1.5.4.1.126.ge5a7d

[PATCH 36/40] Avoid the "dup dance" in wt_status_print_verbose() when possible.

[Johannes Sixt]

If the status output already goes to stdout, then it is not necessary to
redirect stdout for the diff machinery. (This case hangs on Windows for
unknown reasons.)

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 wt-status.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 32d780a..e4999ea 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -312,15 +312,17 @@ static void wt_status_print_untracked(struct wt_status *s)
 static void wt_status_print_verbose(struct wt_status *s)
 {
 	struct rev_info rev;
-	int saved_stdout;
+	int saved_stdout = -1;
 
 	fflush(s->fp);
 
 	/* Sigh, the entire diff machinery is hardcoded to output to
 	 * stdout.  Do the dup-dance...*/
-	saved_stdout = dup(STDOUT_FILENO);
-	if (saved_stdout < 0 ||dup2(fileno(s->fp), STDOUT_FILENO) < 0)
-		die("couldn't redirect stdout\n");
+	if (fileno(s->fp) != STDOUT_FILENO) {
+		saved_stdout = dup(STDOUT_FILENO);
+		if (saved_stdout < 0 ||dup2(fileno(s->fp), STDOUT_FILENO) < 0)
+			die("couldn't redirect stdout\n");
+	}
 
 	init_revisions(&rev, NULL);
 	setup_revisions(0, NULL, &rev, s->reference);
@@ -330,7 +332,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 
 	fflush(stdout);
 
-	if (dup2(saved_stdout, STDOUT_FILENO) < 0)
+	if (saved_stdout >= 0 && dup2(saved_stdout, STDOUT_FILENO) < 0)
 		die("couldn't restore stdout\n");
 	close(saved_stdout);
 }
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 36/40] Avoid the "dup dance" in wt_status_print_verbose() when possible.

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> If the status output already goes to stdout, then it is not necessary to 
> redirect stdout for the diff machinery. (This case hangs on Windows for 
> unknown reasons.)

Just in case people like a diff machinery that can output somewhere else 
than stdout, I have a patch to do that, and also a corresponding patch to 
avoid the dup dance.

Ciao,
Dscho

[PATCH 37/40] Windows: Make 'git help -a' work.

[Johannes Sixt]

git help -a scans the PATH for git commands. On Windows it failed for two
reasons:

- The PATH separator is ';', not ':' on Windows.

- stat() does not set the executabe bit.

We now open the file and guess whether it is executable.

The result of the guess is good enough for the list of git commands, but
it is of no use for a general stat() implementation because (1) it is a
guess, (2) the user has no way to influence the outcome (via chmod or
similar), and (3) it would reduce stat() performance by an unacceptable
amount. Therefore, this strategy is a special-case local to help.c.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 help.c |   38 +++++++++++++++++++++++++++++++++-----
 1 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/help.c b/help.c
index 8143dcd..0248c76 100644
--- a/help.c
+++ b/help.c
@@ -163,6 +163,32 @@ static void pretty_print_string_list(struct cmdnames *cmds, int longest)
 	}
 }
 
+static int is_executable(const char *name)
+{
+	struct stat st;
+
+	if (stat(name, &st) || /* stat, not lstat */
+	    !S_ISREG(st.st_mode))
+		return 0;
+
+#ifdef __MINGW32__
+	/* cannot trust the executable bit, peek into the file instead */
+	char buf[3] = { 0 };
+	int n;
+	int fd = open(name, O_RDONLY);
+	st.st_mode &= ~S_IXUSR;
+	if (fd >= 0) {
+		n = read(fd, buf, 2);
+		if (n == 2)
+			/* DOS executables start with "MZ" */
+			if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
+				st.st_mode |= S_IXUSR;
+		close(fd);
+	}
+#endif
+	return st.st_mode & S_IXUSR;
+}
+
 static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 					 const char *path)
 {
@@ -176,15 +202,12 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 		return 0;
 
 	while ((de = readdir(dir)) != NULL) {
-		struct stat st;
 		int entlen;
 
 		if (prefixcmp(de->d_name, prefix))
 			continue;
 
-		if (stat(de->d_name, &st) || /* stat, not lstat */
-		    !S_ISREG(st.st_mode) ||
-		    !(st.st_mode & S_IXUSR))
+		if (!is_executable(de->d_name))
 			continue;
 
 		entlen = strlen(de->d_name) - prefix_len;
@@ -208,6 +231,11 @@ static void list_commands(void)
 	const char *env_path = getenv("PATH");
 	char *paths, *path, *colon;
 	const char *exec_path = git_exec_path();
+#ifdef __MINGW32__
+	char sep = ';';
+#else
+	char sep = ':';
+#endif
 
 	if (exec_path)
 		longest = list_commands_in_dir(&main_cmds, exec_path);
@@ -219,7 +247,7 @@ static void list_commands(void)
 
 	path = paths = xstrdup(env_path);
 	while (1) {
-		if ((colon = strchr(path, ':')))
+		if ((colon = strchr(path, sep)))
 			*colon = 0;
 
 		len = list_commands_in_dir(&other_cmds, path);
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 37/40] Windows: Make 'git help -a' work.

[Paolo Bonzini]

> +#ifdef __MINGW32__
> +	char sep = ';';
> +#else
> +	char sep = ':';
> +#endif

Aha! When I replied to patch 4/40 I knew there would be one more. :-)

Paolo

[PATCH 38/40] Windows: TMP and TEMP environment variables specify a temporary directory.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 path.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/path.c b/path.c
index 4260952..7f18539 100644
--- a/path.c
+++ b/path.c
@@ -75,6 +75,13 @@ int git_mkstemp(char *path, size_t len, const char *template)
 	size_t n;
 
 	tmp = getenv("TMPDIR");
+#ifdef __MINGW32__
+	/* on Windows it is TMP and TEMP */
+	if (!tmp)
+	    tmp = getenv("TMP");
+	if (!tmp)
+	    tmp = getenv("TEMP");
+#endif
 	if (!tmp)
 		tmp = "/tmp";
 	n = snprintf(path, len, "%s/%s", tmp, template);
-- 
1.5.4.1.126.ge5a7d

[PATCH 39/40] Windows: Fix ntohl() related warnings about printf formatting

[Johannes Sixt]

From: Steffen Prohaska <prohaska@zib.de>

From: Steffen Prohaska <prohaska@zib.de>

On Windows, ntohl() returns unsigned long.  On Unix it returns
uint32_t.  This makes choosing a suitable printf format string
hard.

This commit introduces a mingw specific helper function
git_ntohl() that casts to unsigned int before returning.  This
makes gcc's printf format check happy.  It should be safe because
we expect ntohl to use 32-bit numbers.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-compat-util.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4a53b3b..5ecdd40 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -631,6 +631,10 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
 void mingw_execvp(const char *cmd, char *const *argv);
 #define execvp mingw_execvp
 
+static inline unsigned int git_ntohl(unsigned int x)
+{ return (unsigned int)ntohl(x); }
+#define ntohl git_ntohl
+
 sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 #define signal mingw_signal
 
-- 
1.5.4.1.126.ge5a7d

[PATCH 40/40] compat/pread.c: Add foward decl to fix warning

[Johannes Sixt]

From: Steffen Prohaska <prohaska@zib.de>

From: Steffen Prohaska <prohaska@zib.de>

read_in_full() is used in compat/pread.c.  read_in_full() is
declared in cache.h. But we can't include cache.h because too
many macros are defined there.  Using read_in_full() without
including cache.h is dangerous because we wouldn't recognize if
its prototyp changed.  gcc issues a warning about that.

This commit adds a forward decl to git-compat-util.h.
git-compat-util.h is included by compat/pread.c _and_ cache.h.
Hence, changes in cache.h would be detected.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 git-compat-util.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 5ecdd40..4a8df8e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -168,6 +168,10 @@ extern int git_munmap(void *start, size_t length);
 #define pread git_pread
 extern ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
 #endif
+/* Forward decl that will remind us if its twin in cache.h changes.
+   This function in used in compat/pread.c.  But we can't include
+   cache.h there. */
+extern int read_in_full(int fd, void *buf, size_t count);
 
 #ifdef NO_SETENV
 #define setenv gitsetenv
-- 
1.5.4.1.126.ge5a7d

Re: [PATCH 40/40] compat/pread.c: Add foward decl to fix warning

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> +   This function in used in compat/pread.c.  But we can't include

s/ in / is /

Thanks,
Dscho

Re: [PATCH 00/40] MinGW port

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

Johannes Sixt wrote:
> So here it is, the MinGW port of git.
...
> I've arranged the patches in a few subsets. They are different from
> what Dscho had proposed some weeks ago when I announced the series
> for the first time because I felt this topical partitioning makes
> more sense.
...
> * Diffstat excluding 01/40 (which adds 5989 lines)
...
> 25 files changed, 1713 insertions(+), 60 deletions(-)

I just have to say, MASSIVE work Hannes! Thank you so much for your 
efforts in making the MinGW port the great port that it is!

You too Dscho, and the rest of the MSys Git team! ;-)

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 00/40] MinGW port

[Martin Langhoff]

On Thu, Feb 28, 2008 at 11:01 AM, Marius Storm-Olsen
<marius@trolltech.com> wrote:
>  I just have to say, MASSIVE work Hannes! Thank you so much for your
>  efforts in making the MinGW port the great port that it is!
>
>  You too Dscho, and the rest of the MSys Git team! ;-)

Indeed. Amazing stuff. I do some occasional work on Windows, and it is
a sheer pleasure to use msys git. Simple install, it just works, and
it is fast fast FAST. Hats off!



m

Re: [PATCH 00/40] MinGW port

[Nguyen Thai Ngoc Duy]

On Thu, Feb 28, 2008 at 6:34 AM, Martin Langhoff
<martin.langhoff@gmail.com> wrote:
> On Thu, Feb 28, 2008 at 11:01 AM, Marius Storm-Olsen
>  <marius@trolltech.com> wrote:
>  >  I just have to say, MASSIVE work Hannes! Thank you so much for your
>  >  efforts in making the MinGW port the great port that it is!
>  >
>  >  You too Dscho, and the rest of the MSys Git team! ;-)
>
>  Indeed. Amazing stuff. I do some occasional work on Windows, and it is
>  a sheer pleasure to use msys git. Simple install, it just works, and
>  it is fast fast FAST. Hats off!

Yeah. I've been waiting this moment for too long. Thanks all.
-- 
Duy

Re: [PATCH 00/40] MinGW port

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Johannes Sixt wrote:

> - perl scripts don't work. We have a hack in the port tree, but I haven't
>   yet taken the time to clean it up enough.

AFAICT the perl scripts work in msysGit.

> - The test suite does not pass because of missing features like symbolic
>   links. Again we have adjusted the tests in the port tree so that the
>   test suite can be completed unattended.

We have patches in msysGit for that.  Maybe they should come _before_ the 
MinGW series?

> I've arranged the patches in a few subsets. They are different from what 
> Dscho had proposed some weeks ago when I announced the series for the 
> first time because I felt this topical partitioning makes more sense.

FWIW I like your partitioning very much!

> 27/40 Windows: Implement a custom spawnve().

AFAIR this is needed to call git from git-gui, because of that fscked-up 
semantics of Windows' CreateProcess().

I'll try to review the patches in the next days, but I have no doubt that 
there is not much to complain about.

Thanks, Hannes, for all your valuable and persistent work on mingw.git!

Ciao,
Dscho

Re: [PATCH 00/40] MinGW port

[Johannes Sixt]

On Wednesday 27 February 2008 19:54, Johannes Sixt wrote:
> So here it is, the MinGW port of git.
> This series is also available from
>
> git://repo.or.cz/git/mingw/j6t.git upstream
> http://repo.or.cz/w/git/mingw/j6t.git?a=shortlog;h=upstream

I've integrated the feedback that I received in this series. Thanks to 
has_dos_drive_prefix() as suggested by Dscho quite a number of #ifdef's could 
be removed. Below is the interdiff between the old and the new state. 
However, I have not yet moved stuff out of git-compat-util.h into 
compat/mingw.h to keep this message readable. I'll do that later after I have 
rebased the series on top of the latest git.git master.

BTW, the last hunk in the interdiff is a new bug-fix.

-- Hannes

diff --git a/Makefile b/Makefile
index 53a4e2a..2ea53c0 100644
--- a/Makefile
+++ b/Makefile
@@ -265,6 +265,7 @@ PROGRAMS = \
 	git-pack-redundant$X git-var$X \
 	git-merge-tree$X \
 	git-merge-recursive$X \
+	$(POSIX_ONLY_PROGRAMS) \
 	$(EXTRA_PROGRAMS)
 
 # Empty...
@@ -541,9 +542,11 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_MKDTEMP = YesPlease
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
-	NO_EXTRA_PROGRAMS = YesPlease
+	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
+	COMPAT_CFLAGS += -DPATH_SEP="';'"
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_CFLAGS += -DPRIuMAX=\"I64u\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o
 	EXTLIBS += -lws2_32
 	X = .exe
@@ -611,8 +614,8 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz
 
-ifndef NO_EXTRA_PROGRAMS
-	EXTRA_PROGRAMS += \
+ifndef NO_POSIX_ONLY_PROGRAMS
+	POSIX_ONLY_PROGRAMS = \
 		git-daemon$X \
 		git-imap-send$X
 endif
diff --git a/cache.h b/cache.h
index 3e4e10a..781fa40 100644
--- a/cache.h
+++ b/cache.h
@@ -441,11 +441,7 @@ int safe_create_leading_directories(char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
-#ifndef __MINGW32__
-	return path[0] == '/';
-#else
-	return path[0] == '/' || (path[0] && path[1] == ':');
-#endif
+	return path[0] == '/' || has_dos_drive_prefix(path);
 }
 const char *make_absolute_path(const char *path);
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 0888288..6733727 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -330,16 +330,13 @@ struct tm *localtime_r(const time_t *timep, struct tm 
*result)
 #undef getcwd
 char *mingw_getcwd(char *pointer, int len)
 {
+	int i;
 	char *ret = getcwd(pointer, len);
 	if (!ret)
 		return ret;
-	if (pointer[0] != 0 && pointer[1] == ':') {
-		int i;
-		for (i = 2; pointer[i]; i++)
-			/* Thanks, Bill. You'll burn in hell for that. */
-			if (pointer[i] == '\\')
-				pointer[i] = '/';
-	}
+	for (i = 0; pointer[i]; i++)
+		if (pointer[i] == '\\')
+			pointer[i] = '/';
 	return ret;
 }
 
diff --git a/connect.c b/connect.c
index 7e18ac8..cb81b8c 100644
--- a/connect.c
+++ b/connect.c
@@ -529,13 +529,7 @@ struct child_process *git_connect(int fd[2], const char 
*url_orig,
 		end = host;
 
 	path = strchr(end, c);
-#ifdef __MINGW32__
-	/* host must have at least 2 chars to catch DOS C:/path */
-	if (path && path - end > 1)
-#else
-	if (path)
-#endif
-	{
+	if (path && !has_dos_drive_prefix(end)) {
 		if (c == ':') {
 			protocol = PROTO_SSH;
 			*path++ = '\0';
diff --git a/exec_cmd.c b/exec_cmd.c
index 6d2f740..84db7ee 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -70,11 +70,7 @@ static void add_path(struct strbuf *out, const char *path)
 		else
 			strbuf_addstr(out, make_absolute_path(path));
 
-#ifdef __MINGW32__
-		strbuf_addch(out, ';');
-#else
-		strbuf_addch(out, ':');
-#endif
+		strbuf_addch(out, PATH_SEP);
 	}
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 4a8df8e..3ea0d91 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -103,11 +103,11 @@
 #endif
 
 #ifndef PRIuMAX
-#ifndef __MINGW32__
 #define PRIuMAX "llu"
-#else
-#define PRIuMAX "I64u"
 #endif
+
+#ifndef PATH_SEP
+#define PATH_SEP ':'
 #endif
 
 #ifdef __GNUC__
@@ -168,9 +168,11 @@ extern int git_munmap(void *start, size_t length);
 #define pread git_pread
 extern ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
 #endif
-/* Forward decl that will remind us if its twin in cache.h changes.
-   This function in used in compat/pread.c.  But we can't include
-   cache.h there. */
+/*
+ * Forward decl that will remind us if its twin in cache.h changes.
+ * This function is used in compat/pread.c.  But we can't include
+ * cache.h there.
+ */
 extern int read_in_full(int fd, void *buf, size_t count);
 
 #ifdef NO_SETENV
@@ -650,6 +652,18 @@ char **copy_environ(void);
 void free_environ(char **env);
 char **env_setenv(char **env, const char *name);
 
+static inline int has_dos_drive_prefix(const char *path)
+{
+	return isalpha(*path) && path[1] == ':';
+}
+
+#else /* __MINGW32__ */
+
+static inline int has_dos_drive_prefix(const char *path)
+{
+	return 0;
+}
+
 #endif /* __MINGW32__ */
 
 #endif
diff --git a/help.c b/help.c
index 0248c76..b53c6d7 100644
--- a/help.c
+++ b/help.c
@@ -231,11 +231,6 @@ static void list_commands(void)
 	const char *env_path = getenv("PATH");
 	char *paths, *path, *colon;
 	const char *exec_path = git_exec_path();
-#ifdef __MINGW32__
-	char sep = ';';
-#else
-	char sep = ':';
-#endif
 
 	if (exec_path)
 		longest = list_commands_in_dir(&main_cmds, exec_path);
@@ -247,7 +242,7 @@ static void list_commands(void)
 
 	path = paths = xstrdup(env_path);
 	while (1) {
-		if ((colon = strchr(path, sep)))
+		if ((colon = strchr(path, PATH_SEP)))
 			*colon = 0;
 
 		len = list_commands_in_dir(&other_cmds, path);
diff --git a/setup.c b/setup.c
index 77cc461..212763f 100644
--- a/setup.c
+++ b/setup.c
@@ -12,15 +12,14 @@ static inline int is_dir_sep(char c) { return c == '/'; }
 
 static int sanitary_path_copy(char *dst, const char *src)
 {
-	char *dst0 = dst;
+	char *dst0;
 
-#ifdef __MINGW32__
-	if (isalpha(*src) && src[1] == ':') {
+	if (has_dos_drive_prefix(src)) {
 		*dst++ = *src++;
 		*dst++ = *src++;
-		dst0 = dst;
 	}
-#endif
+	dst0 = dst;
+
 	if (is_dir_sep(*src)) {
 		*dst++ = '/';
 		while (is_dir_sep(*src))
@@ -39,30 +38,22 @@ static int sanitary_path_copy(char *dst, const char *src)
 		 * (4) "../"          -- strip one, eat slash and continue.
 		 */
 		if (c == '.') {
-			switch (src[1]) {
-			case '\0':
+			if (!src[1]) {
 				/* (1) */
 				src++;
 				break;
-			case '/':
-#ifdef __MINGW32__
-			case '\\':
-#endif
+			} else if (is_dir_sep(src[1])) {
 				/* (2) */
 				src += 2;
 				while (is_dir_sep(*src))
 					src++;
 				continue;
-			case '.':
-				switch (src[2]) {
-				case '\0':
+			} else if (src[1] == '.') {
+				if (!src[2]) {
 					/* (3) */
 					src += 2;
 					goto up_one;
-				case '/':
-#ifdef __MINGW32__
-				case '\\':
-#endif
+				} else if (is_dir_sep(src[2])) {
 					/* (4) */
 					src += 3;
 					while (is_dir_sep(*src))
@@ -397,10 +388,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	if (!getcwd(cwd, sizeof(cwd)-1))
 		die("Unable to read current working directory");
-#ifdef __MINGW32__
-	if (cwd[1] == ':')
+	if (has_dos_drive_prefix(cwd))
 		minoffset = 2;
-#endif
 
 	/*
 	 * Test in the following order (relative to the cwd):
diff --git a/sha1_file.c b/sha1_file.c
index 0c60849..fa34c75 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -85,10 +85,8 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)
 
 static inline int offset_1st_component(const char *path)
 {
-#ifdef __MINGW32__
-	if (isalpha(path[0]) && path[1] == ':')
+	if (has_dos_drive_prefix(path))
 		return 2 + (path[2] == '/');
-#endif
 	return *path == '/';
 }
 
@@ -395,11 +393,7 @@ void prepare_alt_odb(void)
 	if (!alt) alt = "";
 
 	alt_odb_tail = &alt_odb_list;
-#ifdef __MINGW32__
-	link_alt_odb_entries(alt, alt + strlen(alt), ';', NULL, 0);
-#else
-	link_alt_odb_entries(alt, alt + strlen(alt), ':', NULL, 0);
-#endif
+	link_alt_odb_entries(alt, alt + strlen(alt), PATH_SEP, NULL, 0);
 
 	read_info_alternates(get_object_directory(), 0);
 }
diff --git a/templates/Makefile b/templates/Makefile
index eb08702..6c0da7a 100644
--- a/templates/Makefile
+++ b/templates/Makefile
@@ -34,11 +34,8 @@ boilerplates.made : $(bpsrc)
 		mkdir -p blt/$$dir && \
 		case "$$boilerplate" in \
 		*--) ;; \
-		*) if test -n "$$(sed -ne '/^#!\//p' -e '1q' < "$$boilerplate")"; then \
-			cp "$$boilerplate" "blt/$${dst}$(NOEXECTEMPL)"; \
-		   else \
-			cp "$$boilerplate" "blt/$$dst"; \
-		   fi ;; \
+		hooks--*) cp $$boilerplate blt/$${dst}$(NOEXECTEMPL) ;; \
+		*) cp $$boilerplate blt/$$dst ;; \
 		esac || exit; \
 	done && \
 	date >$@
diff --git a/transport.c b/transport.c
index 397983d..266a6cc 100644
--- a/transport.c
+++ b/transport.c
@@ -692,7 +692,8 @@ static int is_local(const char *url)
 {
 	const char *colon = strchr(url, ':');
 	const char *slash = strchr(url, '/');
-	return !colon || (slash && slash < colon);
+	return !colon || (slash && slash < colon) ||
+		has_dos_drive_prefix(url);
 }
 
 static int is_file(const char *url)

Re: [PATCH 00/40] MinGW port

[Johannes Schindelin]

Hi,

On Sun, 2 Mar 2008, Johannes Sixt wrote:

> On Wednesday 27 February 2008 19:54, Johannes Sixt wrote:
> > So here it is, the MinGW port of git.
> > This series is also available from
> >
> > git://repo.or.cz/git/mingw/j6t.git upstream
> > http://repo.or.cz/w/git/mingw/j6t.git?a=shortlog;h=upstream
> 
> I've integrated the feedback that I received in this series. Thanks to 
> has_dos_drive_prefix() as suggested by Dscho quite a number of #ifdef's 
> could be removed. Below is the interdiff between the old and the new 
> state. However, I have not yet moved stuff out of git-compat-util.h into 
> compat/mingw.h to keep this message readable. I'll do that later after I 
> have rebased the series on top of the latest git.git master.

Thanks!

And especially thanks for the interdiff, it makes reviewing much easier 
;-)

> diff --git a/Makefile b/Makefile
> index 53a4e2a..2ea53c0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -265,6 +265,7 @@ PROGRAMS = \
>  	git-pack-redundant$X git-var$X \
>  	git-merge-tree$X \
>  	git-merge-recursive$X \
> +	$(POSIX_ONLY_PROGRAMS) \
>  	$(EXTRA_PROGRAMS)

Clever name!  But shouldn't EXTRA_PROGRAMS go?

> diff --git a/compat/mingw.c b/compat/mingw.c
> index 0888288..6733727 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -330,16 +330,13 @@ struct tm *localtime_r(const time_t *timep, struct tm 
> *result)
>  #undef getcwd
>  char *mingw_getcwd(char *pointer, int len)
>  {
> +	int i;
>  	char *ret = getcwd(pointer, len);
>  	if (!ret)
>  		return ret;
> -	if (pointer[0] != 0 && pointer[1] == ':') {
> -		int i;
> -		for (i = 2; pointer[i]; i++)
> -			/* Thanks, Bill. You'll burn in hell for that. */
> -			if (pointer[i] == '\\')
> -				pointer[i] = '/';
> -	}
> +	for (i = 0; pointer[i]; i++)
> +		if (pointer[i] == '\\')
> +			pointer[i] = '/';

Boo.  My nice comment.  You just deleted it.  ;-)

> diff --git a/templates/Makefile b/templates/Makefile
> index eb08702..6c0da7a 100644
> --- a/templates/Makefile
> +++ b/templates/Makefile
> @@ -34,11 +34,8 @@ boilerplates.made : $(bpsrc)
>  		mkdir -p blt/$$dir && \
>  		case "$$boilerplate" in \
>  		*--) ;; \
> -		*) if test -n "$$(sed -ne '/^#!\//p' -e '1q' < "$$boilerplate")"; then \
> -			cp "$$boilerplate" "blt/$${dst}$(NOEXECTEMPL)"; \
> -		   else \
> -			cp "$$boilerplate" "blt/$$dst"; \
> -		   fi ;; \
> +		hooks--*) cp $$boilerplate blt/$${dst}$(NOEXECTEMPL) ;; \
> +		*) cp $$boilerplate blt/$$dst ;; \

Nice!  (IOW much better than what I suggested.)

> diff --git a/transport.c b/transport.c
> index 397983d..266a6cc 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -692,7 +692,8 @@ static int is_local(const char *url)
>  {
>  	const char *colon = strchr(url, ':');
>  	const char *slash = strchr(url, '/');
> -	return !colon || (slash && slash < colon);
> +	return !colon || (slash && slash < colon) ||
> +		has_dos_drive_prefix(url);

This must be the bug fix ;-)

I like it!

Thanks,
Dscho

Re: [PATCH 00/40] MinGW port

[Johannes Sixt]

On Sunday 02 March 2008 23:07, Johannes Schindelin wrote:
> On Sun, 2 Mar 2008, Johannes Sixt wrote:
> > diff --git a/Makefile b/Makefile
> > index 53a4e2a..2ea53c0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -265,6 +265,7 @@ PROGRAMS = \
> >  	git-pack-redundant$X git-var$X \
> >  	git-merge-tree$X \
> >  	git-merge-recursive$X \
> > +	$(POSIX_ONLY_PROGRAMS) \
> >  	$(EXTRA_PROGRAMS)
>
> Clever name!  But shouldn't EXTRA_PROGRAMS go?

No. It was there before I messed around with it.

> > -			/* Thanks, Bill. You'll burn in hell for that. */
>
> Boo.  My nice comment.  You just deleted it.  ;-)

It's not my fault, I swear it. It went like this: I erased the line, but then 
changed my mind and hit Undo. But then I thought, this is a serious project, 
we don't need such language. And hit Redo. Which made me think again... So I 
said to my little rascal, who was playing Lego: "Say 'stop'", and I hit Undo 
and Redo as fast as I could. And when he said "Stop", oh well, I had just hit 
Redo. So here we have it - no longer.

;-)

-- Hannes