Package: git-core
Severity: important
Tags: patch
Hi!
When trying to build your package on hurd-i386, the following error is
raised:
> gcc -o checkout-index.o -c -g -Wall -O2 -DNO_OPENSSL
> -DSHA1_HEADER='"mozilla-sha1/sha1.h"' checkout-index.c
> In file included from checkout-index.c:39:
> cache.h:172: error: 'PATH_MAX' undeclared here (not in a function)
> make[1]: *** [checkout-index.o] Error 1
> make[1]: Leaving directory `/home/kibi/build/git-core-1.4.1'
> make: *** [build-arch-stamp] Error 2
As briefly explained on the hurd-devel-debian page[1], unconditional use
of PATH_MAX is a POSIX incompatibility. Due to the number of occurrences
of it in the source code (`grep PATH_MAX * | wc -l` reports 80), I guess
that a workaround will be sufficient, at least at the moment. Another
justification for using this workaround is that the same one is already
used for MAXPATHLEN (see the context in PATH_MAX.diff).
1. http://www.debian.org/ports/hurd/hurd-devel-debian
A second problem is that GNU(/Hurd) is not detected by the Makefile;
that's solved with the second patch I join, Makefile-GNU.diff.
The last but not least problem resides in read-cache.c. In IS_ERR, an
assumption is made: ENOENT, EINVAL and all error codes (errno) of
the function 'lstat' are lower (in absolute value) than 1000, which is
the case on many arch (e.g. on Linux) but that's not the case on Hurd.
You may have a look at the file 'errno-hurd.log' that I also join, which
provides a sample program and its result on both Hurd & Linux platform.
In the last patch I hereby submit, I try to solve the strange error
handling by using a switch on the error codes that can be encountered
(either direct affectations, or errno set by lstat), but that doesn't
guarantee that _correct_ affected items won't have an address matching
an error code... That patch (error_handling.diff) has been tested on
both hurd and linux arch, and allows git-core to pass the testsuite on
both (whereas without it, the testsuite fails on hurd).
I agree that this patch doesn't solve all the problems that can be
encountered, but at least a great part. I guess that a correct error
handling using a variable (e.g. acting as a flag or as another
generalized errno) should be developed to ensure portability and
robustness.
As a quick reminder, summary of attached files:
- PATH_MAX.diff: solves POSIX incompatibily
- Makefile-GNU.diff: enables GNU detection, turns on NO_STRLCPY
- errno-hurd.log: displays why IS_ERR doesn't play its role on hurd
- error_handling.diff: fixes error handling as it can
The three .diff files can be dropped directly in debian/diff (once
reviewed), the package construction has been checked on both linux-i386
and hurd-i386.
Cheers,
--
Cyril Brulebois
--- a/git-compat-util.h.orig 2006-07-22 14:44:14.000000000 +0000
+++ b/git-compat-util.h 2006-07-22 14:54:21.000000000 +0000
@@ -165,4 +165,8 @@
#ifndef MAXPATHLEN
#define MAXPATHLEN 256
#endif
+
+#ifndef PATH_MAX
+#define PATH_MAX 4096
+#endif
#endif
--- a/Makefile 2006-07-25 14:00:48.000000000 +0000
+++ b/Makefile 2006-07-25 14:02:07.000000000 +0000
@@ -327,6 +327,10 @@
# for now, build 32-bit version
ALL_LDFLAGS += -L/usr/lib32
endif
+ifeq ($(uname_S),GNU)
+ # GNU stands for GNU/Hurd
+ NO_STRLCPY=YesPlease
+endif
ifneq (,$(findstring arm,$(uname_M)))
ARM_SHA1 = YesPlease
endif
/* Sample program */
[EMAIL PROTECTED]:~/build/debug$ cat einval.c
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
int main(void) {
/* Taken from read-cache.c directly */
printf("EINVAL : %d\n", EINVAL);
printf("ENOENT : %d\n", ENOENT);
printf("EBUSY : %d\n", EBUSY );
/* Taken from lstat(2) */
printf("EACCES : %d\n", EACCES );
printf("EBADF : %d\n", EBADF );
printf("EFAULT : %d\n", EFAULT );
printf("ELOOP : %d\n", ELOOP );
printf("ENOENT : %d\n", ENOENT );
printf("ENOMEM : %d\n", ENOMEM );
printf("ENOTDIR: %d\n", ENOTDIR);
return 0;
}
/* HURD-box */
[EMAIL PROTECTED]:~/build/debug$ make test
gcc -o einval einval.c
./einval
EINVAL : 1073741846
ENOENT : 1073741826
EBUSY : 1073741840
EACCES : 1073741837
EBADF : 1073741833
EFAULT : 1073741838
ELOOP : 1073741886
ENOENT : 1073741826
ENOMEM : 1073741836
ENOTDIR: 1073741844
/* LINUX-box */
[EMAIL PROTECTED]:~$ make test
gcc -o einval einval.c
./einval
EINVAL : 22
ENOENT : 2
EBUSY : 16
EACCES : 13
EBADF : 9
EFAULT : 14
ELOOP : 40
ENOENT : 2
ENOMEM : 12
ENOTDIR: 20
--- a/read-cache.c 2006-07-25 03:09:56.000000000 +0000
+++ b/read-cache.c 2006-07-25 03:05:05.000000000 +0000
@@ -590,7 +593,28 @@
static inline long IS_ERR(const void *ptr)
{
- return (unsigned long)ptr > (unsigned long)-1000L;
+ switch ((unsigned long)ptr) {
+
+ /* Called directly */
+ case (unsigned long)-EINVAL :
+ case (unsigned long)-ENOENT :
+
+ /* One never knows... */
+ case (unsigned long)-EBUSY :
+
+ /* lstat() [ENOENT not repeated] */
+ /* We should also support other less common errors, see man 2 lstat */
+ case (unsigned long)-EACCES :
+ case (unsigned long)-EBADF :
+ case (unsigned long)-EFAULT :
+ case (unsigned long)-ELOOP :
+ case (unsigned long)-ENOMEM :
+ case (unsigned long)-ENOTDIR:
+ return 1; /* This is an error */
+
+ default:
+ return 0; /* This is not an error */
+ }
}
/*
@@ -610,6 +634,7 @@
struct cache_entry *updated;
int changed, size;
+ /* BE CAREFUL with this errno! */
if (lstat(ce->name, &st) < 0)
return ERR_PTR(-errno);
