make gitfakemmap standalone to fix linking error in git.c

make gitfakemmap standalone to fix linking error in git.c

[Alex Riesen]

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

The patch is to fix linking error because of gitfakemmap referencing
die living in usage.c, and git.c not linking in the file. Instead of
hardcoding usage.o in git.c prerequisites, I separated mmap from the
main sources.

[-- Attachment #2: 0001-make-fakemmap-standalone-to-fix-linking-error-because-of-gitfakemmap.txt --]
[-- Type: text/plain, Size: 2720 bytes --]

Subject: [PATCH] make fakemmap standalone to fix linking error because of gitfakemmap
referencing die living in usage.c, and git.c not linking in the file.
Instead of hardcoding usage.o in git.c prerequisites, I separated mmap.

Signed-off-by: Alex Riesen <fork0@gmail.com>


---

 cache.h       |   18 +++---------------
 compat/mmap.c |   11 +++++++----
 compat/mmap.h |   15 +++++++++++++++
 3 files changed, 25 insertions(+), 19 deletions(-)
 create mode 100644 compat/mmap.h

e1e24d29b684d468a7be5d097ba3e1679170e7db
diff --git a/cache.h b/cache.h
index f9b367f..c70a467 100644
--- a/cache.h
+++ b/cache.h
@@ -11,7 +11,9 @@
 #include <string.h>
 #include <errno.h>
 #include <limits.h>
-#ifndef NO_MMAP
+#ifdef NO_MMAP
+#include "compat/mmap.h"
+#else
 #include <sys/mman.h>
 #endif
 #include <sys/param.h>
@@ -373,20 +375,6 @@ extern void packed_object_info_detail(st
 /* Dumb servers support */
 extern int update_server_info(int);
 
-#ifdef NO_MMAP
-
-#ifndef PROT_READ
-#define PROT_READ 1
-#define PROT_WRITE 2
-#define MAP_PRIVATE 1
-#define MAP_FAILED ((void*)-1)
-#endif
-
-extern void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset);
-extern int gitfakemunmap(void *start, size_t length);
-
-#endif
-
 typedef int (*config_fn_t)(const char *, const char *);
 extern int git_default_config(const char *, const char *);
 extern int git_config_from_file(config_fn_t fn, const char *);
diff --git a/compat/mmap.c b/compat/mmap.c
index a051c47..98cf3cb 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -2,21 +2,24 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <errno.h>
-#include "../cache.h"
+#include <string.h>
+#include "mmap.h"
 
 void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset)
 {
 	int n = 0;
 
-	if (start != NULL || !(flags & MAP_PRIVATE))
-		die("Invalid usage of gitfakemmap.");
+	if (!start || !(flags & MAP_PRIVATE)) {
+		fprintf(stderr, "Invalid usage of gitfakemmap.\n");
+		exit(128); /* see die() in ../usage.c */
+	}
 
 	if (lseek(fd, offset, SEEK_SET) < 0) {
 		errno = EINVAL;
 		return MAP_FAILED;
 	}
 
-	start = xmalloc(length);
+	start = malloc(length);
 	if (start == NULL) {
 		errno = ENOMEM;
 		return MAP_FAILED;
diff --git a/compat/mmap.h b/compat/mmap.h
new file mode 100644
index 0000000..08d7f99
--- /dev/null
+++ b/compat/mmap.h
@@ -0,0 +1,15 @@
+#ifndef MMAP_H
+#define MMAP_H
+
+#ifndef PROT_READ
+#define PROT_READ 1
+#define PROT_WRITE 2
+#define MAP_PRIVATE 1
+#define MAP_FAILED ((void*)-1)
+#endif
+
+extern void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset);
+extern int gitfakemunmap(void *start, size_t length);
+
+#endif
+
-- 
0.99.9.GIT

Re: make gitfakemmap standalone to fix linking error in git.c

[Alex Riesen]

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

The patch is to fix the error introduced by previous patch, which
inverted condition of checking for the supported case (start must be
NULL).

On 12/5/05, Alex Riesen <raa.lkml@gmail.com> wrote:
> The patch is to fix linking error because of gitfakemmap referencing
> die living in usage.c, and git.c not linking in the file. Instead of
> hardcoding usage.o in git.c prerequisites, I separated mmap from the
> main sources.

Why does it always happen...

[-- Attachment #2: 0002-fix-bug-introduced-by-previous-change.txt --]
[-- Type: text/plain, Size: 623 bytes --]

Subject: [PATCH] fix bug introduced by previous change

Signed-off-by: Alex Riesen <ariesen@harmanbecker.com>


---

 compat/mmap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

58d38f03086fe30f3f758f2f27fde0ad05107b4a
diff --git a/compat/mmap.c b/compat/mmap.c
index 98cf3cb..cfa1f25 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -9,7 +9,7 @@ void *gitfakemmap(void *start, size_t le
 {
 	int n = 0;
 
-	if (!start || !(flags & MAP_PRIVATE)) {
+	if (start || !(flags & MAP_PRIVATE)) {
 		fprintf(stderr, "Invalid usage of gitfakemmap.\n");
 		exit(128); /* see die() in ../usage.c */
 	}
-- 
0.99.9.GIT

Re: make gitfakemmap standalone to fix linking error in git.c

[Petr Baudis]

Dear diary, on Mon, Dec 05, 2005 at 02:24:47PM CET, I got a letter
where Alex Riesen <raa.lkml@gmail.com> said that...
> The patch is to fix the error introduced by previous patch, which
> inverted condition of checking for the supported case (start must be
> NULL).
> 
> On 12/5/05, Alex Riesen <raa.lkml@gmail.com> wrote:
> > The patch is to fix linking error because of gitfakemmap referencing
> > die living in usage.c, and git.c not linking in the file. Instead of
> > hardcoding usage.o in git.c prerequisites, I separated mmap from the
> > main sources.
> 
> Why does it always happen...

Possibly not worth yet another patch submission now, but in general I
believe it's better to send new revision of the original patch rather
than another patch on top of the previous one.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: make gitfakemmap standalone to fix linking error in git.c

[Alex Riesen]

On 12/5/05, Petr Baudis <pasky@suse.cz> wrote:
> > The patch is to fix the error introduced by previous patch, which
> > inverted condition of checking for the supported case (start must be
> > NULL).
>
> Possibly not worth yet another patch submission now, but in general I
> believe it's better to send new revision of the original patch rather
> than another patch on top of the previous one.

Yes, of course. Sorry... I will not resend, it's trivial, but I
promise to get better.

Re: make gitfakemmap standalone to fix linking error in git.c

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Why does it always happen...

Because you touched you did not absolutely have to ;-).

When I took setenv patch for git.c, I was thinking about
something similar.  It may be cleaner to split out the pieces so
that the functions like usage(), die(), xmalloc() and friends
could be linkable in git.c and others that want to link in only
the minimum set without including "cache.h" which has more
intimate knowledge of other git internals.

[PATCH] Clean up compatibility definitions.

[Junio C Hamano]

This attempts to clean up the way various compatibility
functions are defined and used.

 - A new header file, git-compat-util.h, is introduced.  This
   looks at various NO_XXX and does necessary function name
   replacements, equivalent of -Dstrcasestr=gitstrcasestr in the
   Makefile.

 - Those function name replacements are removed from the Makefile.

 - Common features such as usage(), die(), xmalloc() are moved
   from cache.h to git-compat-util.h; cache.h includes
   git-compat-util.h itself.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

  Junio C Hamano <junkio@cox.net> writes:

  > When I took setenv patch for git.c, I was thinking about
  > something similar.  It may be cleaner to split out the pieces so
  > that the functions like usage(), die(), xmalloc() and friends
  > could be linkable in git.c and others that want to link in only
  > the minimum set without including "cache.h" which has more
  > intimate knowledge of other git internals.

  Maybe something like this...

 Makefile            |   16 ++++---
 cache.h             |   96 -------------------------------------------
 compat/mmap.c       |    2 -
 compat/strcasestr.c |    3 -
 git-compat-util.h   |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++
 git.c               |   15 +++----
 mailinfo.c          |    4 --
 usage.c             |    2 -
 8 files changed, 132 insertions(+), 119 deletions(-)
 create mode 100644 git-compat-util.h

2823be66bb4a1f6a6b4240e24dfd402e4146c43c
diff --git a/Makefile b/Makefile
index df3c6eb..1f1088e 100644
--- a/Makefile
+++ b/Makefile
@@ -162,7 +162,7 @@ LIB_FILE=libgit.a
 LIB_H = \
 	blob.h cache.h commit.h count-delta.h csum-file.h delta.h \
 	diff.h epoch.h object.h pack.h pkt-line.h quote.h refs.h \
-	run-command.h strbuf.h tag.h tree.h
+	run-command.h strbuf.h tag.h tree.h git-compat-util.h
 
 DIFF_OBJS = \
 	diff.o diffcore-break.o diffcore-order.o diffcore-pathspec.o \
@@ -174,9 +174,11 @@ LIB_OBJS = \
 	object.o pack-check.o patch-delta.o path.o pkt-line.o \
 	quote.o read-cache.o refs.o run-command.o \
 	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \
-	tag.o tree.o usage.o config.o environment.o ctype.o copy.o \
+	tag.o tree.o config.o environment.o copy.o \
 	$(DIFF_OBJS)
 
+COMPAT_OBJS = usage.o ctype.o
+
 LIBS = $(LIB_FILE)
 LIBS += -lz
 
@@ -320,15 +322,15 @@ ifdef NEEDS_NSL
 	SIMPLE_LIB += -lnsl
 endif
 ifdef NO_STRCASESTR
-	COMPAT_CFLAGS += -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1
+	COMPAT_CFLAGS += -DNO_STRCASESTR
 	COMPAT_OBJS += compat/strcasestr.o
 endif
 ifdef NO_SETENV
-	COMPAT_CFLAGS += -Dsetenv=gitsetenv -DNO_SETENV=1
+	COMPAT_CFLAGS += -DNO_SETENV
 	COMPAT_OBJS += compat/setenv.o
 endif
 ifdef NO_MMAP
-	COMPAT_CFLAGS += -Dmmap=gitfakemmap -Dmunmap=gitfakemunmap -DNO_MMAP
+	COMPAT_CFLAGS += -DNO_MMAP
 	COMPAT_OBJS += compat/mmap.o
 endif
 ifdef NO_IPV6
@@ -363,9 +365,9 @@ all: $(ALL_PROGRAMS)
 all:
 	$(MAKE) -C templates
 
-git$(X): git.c $(COMPAT_OBJS) Makefile
+git$X: git.c $(LIB_FILE) Makefile
 	$(CC) -DGIT_EXEC_PATH='"$(bindir)"' -DGIT_VERSION='"$(GIT_VERSION)"' \
-		$(CFLAGS) $(COMPAT_CFLAGS) -o $@ $(filter %.c,$^) $(filter %.o,$^)
+		$(CFLAGS) $(COMPAT_CFLAGS) -o $@ $(filter %.c,$^) $(LIB_FILE)
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 	rm -f $@
diff --git a/cache.h b/cache.h
index f9b367f..86fc250 100644
--- a/cache.h
+++ b/cache.h
@@ -1,23 +1,7 @@
 #ifndef CACHE_H
 #define CACHE_H
 
-#include <unistd.h>
-#include <stdio.h>
-#include <sys/stat.h>
-#include <fcntl.h>
-#include <stddef.h>
-#include <stdlib.h>
-#include <stdarg.h>
-#include <string.h>
-#include <errno.h>
-#include <limits.h>
-#ifndef NO_MMAP
-#include <sys/mman.h>
-#endif
-#include <sys/param.h>
-#include <netinet/in.h>
-#include <sys/types.h>
-#include <dirent.h>
+#include "git-compat-util.h"
 
 #include SHA1_HEADER
 #include <zlib.h>
@@ -36,15 +20,6 @@
 #define DTYPE(de)	DT_UNKNOWN
 #endif
 
-#ifdef __GNUC__
-#define NORETURN __attribute__((__noreturn__))
-#else
-#define NORETURN
-#ifndef __attribute__
-#define __attribute__(x)
-#endif
-#endif
-
 /*
  * Intensive research over the course of many years has shown that
  * port 9418 is totally unused by anything else. Or
@@ -250,11 +225,6 @@ extern const char *resolve_ref(const cha
 extern int create_symref(const char *git_HEAD, const char *refs_heads_master);
 extern int validate_symref(const char *git_HEAD);
 
-/* General helper functions */
-extern void usage(const char *err) NORETURN;
-extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
-
 extern int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2);
 extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);
 
@@ -272,30 +242,6 @@ extern int setup_ident(void);
 extern const char *git_author_info(void);
 extern const char *git_committer_info(void);
 
-static inline void *xmalloc(size_t size)
-{
-	void *ret = malloc(size);
-	if (!ret)
-		die("Out of memory, malloc failed");
-	return ret;
-}
-
-static inline void *xrealloc(void *ptr, size_t size)
-{
-	void *ret = realloc(ptr, size);
-	if (!ret)
-		die("Out of memory, realloc failed");
-	return ret;
-}
-
-static inline void *xcalloc(size_t nmemb, size_t size)
-{
-	void *ret = calloc(nmemb, size);
-	if (!ret)
-		die("Out of memory, calloc failed");
-	return ret;
-}
-
 struct checkout {
 	const char *base_dir;
 	int base_dir_len;
@@ -373,20 +319,6 @@ extern void packed_object_info_detail(st
 /* Dumb servers support */
 extern int update_server_info(int);
 
-#ifdef NO_MMAP
-
-#ifndef PROT_READ
-#define PROT_READ 1
-#define PROT_WRITE 2
-#define MAP_PRIVATE 1
-#define MAP_FAILED ((void*)-1)
-#endif
-
-extern void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset);
-extern int gitfakemunmap(void *start, size_t length);
-
-#endif
-
 typedef int (*config_fn_t)(const char *, const char *);
 extern int git_default_config(const char *, const char *);
 extern int git_config_from_file(config_fn_t fn, const char *);
@@ -404,31 +336,5 @@ extern char git_default_name[MAX_GITNAME
 #define MAX_ENCODING_LENGTH 64
 extern char git_commit_encoding[MAX_ENCODING_LENGTH];
 
-/* Sane ctype - no locale, and works with signed chars */
-#undef isspace
-#undef isdigit
-#undef isalpha
-#undef isalnum
-#undef tolower
-#undef toupper
-extern unsigned char sane_ctype[256];
-#define GIT_SPACE 0x01
-#define GIT_DIGIT 0x02
-#define GIT_ALPHA 0x04
-#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
-#define isspace(x) sane_istest(x,GIT_SPACE)
-#define isdigit(x) sane_istest(x,GIT_DIGIT)
-#define isalpha(x) sane_istest(x,GIT_ALPHA)
-#define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
-#define tolower(x) sane_case((unsigned char)(x), 0x20)
-#define toupper(x) sane_case((unsigned char)(x), 0)
-
-static inline int sane_case(int x, int high)
-{
-	if (sane_istest(x, GIT_ALPHA))
-		x = (x & ~0x20) | high;
-	return x;
-}
-
 extern int copy_fd(int ifd, int ofd);
 #endif /* CACHE_H */
diff --git a/compat/mmap.c b/compat/mmap.c
index a051c47..55cb120 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -2,7 +2,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <errno.h>
-#include "../cache.h"
+#include "../git-compat-util.h"
 
 void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset)
 {
diff --git a/compat/strcasestr.c b/compat/strcasestr.c
index b96414d..26896de 100644
--- a/compat/strcasestr.c
+++ b/compat/strcasestr.c
@@ -1,5 +1,4 @@
-#include <string.h>
-#include <ctype.h>
+#include "../git-compat-util.h"
 
 char *gitstrcasestr(const char *haystack, const char *needle)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
new file mode 100644
index 0000000..4185b12
--- /dev/null
+++ b/git-compat-util.h
@@ -0,0 +1,113 @@
+#ifndef GIT_COMPAT_UTIL_H
+#define GIT_COMPAT_UTIL_H
+
+#include <unistd.h>
+#include <stdio.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stddef.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <string.h>
+#include <errno.h>
+#include <limits.h>
+#include <sys/param.h>
+#include <netinet/in.h>
+#include <sys/types.h>
+#include <dirent.h>
+
+#ifdef __GNUC__
+#define NORETURN __attribute__((__noreturn__))
+#else
+#define NORETURN
+#ifndef __attribute__
+#define __attribute__(x)
+#endif
+#endif
+
+/* General helper functions */
+extern void usage(const char *err) NORETURN;
+extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
+extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+
+#ifdef NO_MMAP
+
+#ifndef PROT_READ
+#define PROT_READ 1
+#define PROT_WRITE 2
+#define MAP_PRIVATE 1
+#define MAP_FAILED ((void*)-1)
+#endif
+
+#define mmap gitfakemmap
+#define munmap gitfakemunmap
+extern void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset);
+extern int gitfakemunmap(void *start, size_t length);
+
+#else /* NO_MMAP */
+
+#include <sys/mman.h>
+
+#endif /* NO_MMAP */
+
+#ifdef NO_SETENV
+#define setenv gitsetenv
+extern int gitsetenv(const char *, const char *, int);
+#endif
+
+#ifdef NO_STRCASESTR
+#define strcasestr gitstrcasestr
+extern char *gitstrcasestr(const char *haystack, const char *needle);
+#endif
+
+static inline void *xmalloc(size_t size)
+{
+	void *ret = malloc(size);
+	if (!ret)
+		die("Out of memory, malloc failed");
+	return ret;
+}
+
+static inline void *xrealloc(void *ptr, size_t size)
+{
+	void *ret = realloc(ptr, size);
+	if (!ret)
+		die("Out of memory, realloc failed");
+	return ret;
+}
+
+static inline void *xcalloc(size_t nmemb, size_t size)
+{
+	void *ret = calloc(nmemb, size);
+	if (!ret)
+		die("Out of memory, calloc failed");
+	return ret;
+}
+
+/* Sane ctype - no locale, and works with signed chars */
+#undef isspace
+#undef isdigit
+#undef isalpha
+#undef isalnum
+#undef tolower
+#undef toupper
+extern unsigned char sane_ctype[256];
+#define GIT_SPACE 0x01
+#define GIT_DIGIT 0x02
+#define GIT_ALPHA 0x04
+#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
+#define isspace(x) sane_istest(x,GIT_SPACE)
+#define isdigit(x) sane_istest(x,GIT_DIGIT)
+#define isalpha(x) sane_istest(x,GIT_ALPHA)
+#define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
+#define tolower(x) sane_case((unsigned char)(x), 0x20)
+#define toupper(x) sane_case((unsigned char)(x), 0)
+
+static inline int sane_case(int x, int high)
+{
+	if (sane_istest(x, GIT_ALPHA))
+		x = (x & ~0x20) | high;
+	return x;
+}
+
+#endif
diff --git a/git.c b/git.c
index ee5048a..c26cac6 100644
--- a/git.c
+++ b/git.c
@@ -8,15 +8,12 @@
 #include <errno.h>
 #include <limits.h>
 #include <stdarg.h>
+#include "git-compat-util.h"
 
 #ifndef PATH_MAX
 # define PATH_MAX 4096
 #endif
 
-#ifdef NO_SETENV
-extern int gitsetenv(const char *, const char *, int);
-#endif
-
 static const char git_usage[] =
 	"Usage: git [--version] [--exec-path[=GIT_EXEC_PATH]] [--help] COMMAND [ ARGS ]";
 
@@ -156,10 +153,10 @@ static void list_commands(const char *ex
 }
 
 #ifdef __GNUC__
-static void usage(const char *exec_path, const char *fmt, ...)
+static void cmd_usage(const char *exec_path, const char *fmt, ...)
 	__attribute__((__format__(__printf__, 2, 3), __noreturn__));
 #endif
-static void usage(const char *exec_path, const char *fmt, ...)
+static void cmd_usage(const char *exec_path, const char *fmt, ...)
 {
 	if (fmt) {
 		va_list ap;
@@ -254,12 +251,12 @@ int main(int argc, char **argv, char **e
 		else if (!strcmp(arg, "help"))
 			show_help = 1;
 		else if (!show_help)
-			usage(NULL, NULL);
+			cmd_usage(NULL, NULL);
 	}
 
 	if (i >= argc || show_help) {
 		if (i >= argc)
-			usage(exec_path, NULL);
+			cmd_usage(exec_path, NULL);
 
 		show_man_page(argv[i]);
 	}
@@ -297,7 +294,7 @@ int main(int argc, char **argv, char **e
 	execve(git_command, &argv[i], envp);
 
 	if (errno == ENOENT)
-		usage(exec_path, "'%s' is not a git-command", argv[i]);
+		cmd_usage(exec_path, "'%s' is not a git-command", argv[i]);
 
 	fprintf(stderr, "Failed to run command '%s': %s\n",
 		git_command, strerror(errno));
diff --git a/mailinfo.c b/mailinfo.c
index 890e348..3b97a89 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -10,10 +10,6 @@
 #include <iconv.h>
 #include "cache.h"
 
-#ifdef NO_STRCASESTR
-extern char *gitstrcasestr(const char *haystack, const char *needle);
-#endif
-
 static FILE *cmitmsg, *patchfile;
 
 static int keep_subject = 0;
diff --git a/usage.c b/usage.c
index dfa87fe..1fa924c 100644
--- a/usage.c
+++ b/usage.c
@@ -3,7 +3,7 @@
  *
  * Copyright (C) Linus Torvalds, 2005
  */
-#include "cache.h"
+#include "git-compat-util.h"
 
 static void report(const char *prefix, const char *err, va_list params)
 {
-- 
0.99.9.GIT

Re: [PATCH] Clean up compatibility definitions.

[Johannes Schindelin]

Hi,

On Mon, 5 Dec 2005, Junio C Hamano wrote:

> This attempts to clean up the way various compatibility
> functions are defined and used.

You sure you want that before 1.0?

Ciao,
Dscho

Re: [PATCH] Clean up compatibility definitions.

[Junio C Hamano]

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

>> This attempts to clean up the way various compatibility
>> functions are defined and used.
>
> You sure you want that before 1.0?

I think this is mostly an obvious clean-up, but I do not have
enough test environments, so ...

Re: make gitfakemmap standalone to fix linking error in git.c

[Alex Riesen]

Junio C Hamano, Mon, Dec 05, 2005 18:40:21 +0100:
> > Why does it always happen...
> Because you touched you did not absolutely have to ;-).

well, git$(X) didn't link...

Re: [PATCH] Clean up compatibility definitions.

[Alex Riesen]

Junio C Hamano, Mon, Dec 05, 2005 21:51:56 +0100:
> >> This attempts to clean up the way various compatibility
> >> functions are defined and used.
> >
> > You sure you want that before 1.0?
> 
> I think this is mostly an obvious clean-up, but I do not have
> enough test environments, so ...

I will do cygwin. Tomorrow, that is.

Re: make gitfakemmap standalone to fix linking error in git.c

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Mon, Dec 05, 2005 18:40:21 +0100:
>> > Why does it always happen...
>> Because you touched you did not absolutely have to ;-).
>
> well, git$(X) didn't link...

I meant your change to the "if ()" expression in gitfakemmap().
Does the change have anything to do with git$X linkage?  I think
not.

Re: [PATCH] Clean up compatibility definitions.

[Alex Riesen]

Junio C Hamano, Mon, Dec 05, 2005 21:22:42 +0100:
> This attempts to clean up the way various compatibility
> functions are defined and used.
...
> --- a/compat/mmap.c
> +++ b/compat/mmap.c
> @@ -2,7 +2,7 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <errno.h>
> -#include "../cache.h"
> +#include "../git-compat-util.h"

I still think that compat functions should stand alone.
Especially if it does not costs us much (or even less than that).

Re: [PATCH] Clean up compatibility definitions.

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Mon, Dec 05, 2005 21:22:42 +0100:
>> This attempts to clean up the way various compatibility
>> functions are defined and used.
> ...
>> --- a/compat/mmap.c
>> +++ b/compat/mmap.c
>> @@ -2,7 +2,7 @@
>>  #include <stdlib.h>
>>  #include <unistd.h>
>>  #include <errno.h>
>> -#include "../cache.h"
>> +#include "../git-compat-util.h"
>
> I still think that compat functions should stand alone.
> Especially if it does not costs us much (or even less than that).

Sorry, you lost me.  What do you mean by "standing alone"?

Re: [PATCH] Clean up compatibility definitions.

[Petr Baudis]

Dear diary, on Mon, Dec 05, 2005 at 09:22:42PM CET, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> diff --git a/git-compat-util.h b/git-compat-util.h
> new file mode 100644

What about compat/util.h or something? Nicer, shorter, and takes
advantage of this fancy hierarchical namespace "directories" invention.
;-)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] Clean up compatibility definitions.

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Dear diary, on Mon, Dec 05, 2005 at 09:22:42PM CET, I got a letter
> where Junio C Hamano <junkio@cox.net> said that...
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> new file mode 100644
>
> What about compat/util.h or something? Nicer, shorter, and takes
> advantage of this fancy hierarchical namespace "directories" invention.
> ;-)

While I would appreciate a better name, I am afraid that is not a
particularly good one.  It is not "compatibility utilities", but
compat things and util things mixed together, so it does not
belong to compat/ directory to begin with.  die() and friends
are not about compatibility at all.

Re: [PATCH] Clean up compatibility definitions.

[Alex Riesen]

On 12/5/05, Junio C Hamano <junkio@cox.net> wrote:
> >> This attempts to clean up the way various compatibility
> >> functions are defined and used.
> >> --- a/compat/mmap.c
> >> +++ b/compat/mmap.c
> >
> > I still think that compat functions should stand alone.
> > Especially if it does not costs us much (or even less than that).
> Sorry, you lost me.  What do you mean by "standing alone"?

Do not depend nor use anything from the calling code (do not take
anything from git). If it has to serve as compatibility wrapper to
mmap, it better should be mmap(2): return MAP_FAILED if error, do not
exit process, do no output. I wouldn't expect open(2) to print "File
not found", would you?

Besides, MAP_FAILED is perfectly handled in calling layers. If they
even print out errno, the case is completely covered and simplier
handled.

Re: [PATCH] Clean up compatibility definitions.

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> ... return MAP_FAILED if error, do not
> exit process, do no output.

Ah, that is what you meant.  I agree.

Re: [PATCH] Clean up compatibility definitions.

[Alex Riesen]

On 12/5/05, Alex Riesen <raa.lkml@gmail.com> wrote:
> Junio C Hamano, Mon, Dec 05, 2005 21:51:56 +0100:
> > >> This attempts to clean up the way various compatibility
> > >> functions are defined and used.
> > >
> > > You sure you want that before 1.0?
> >
> > I think this is mostly an obvious clean-up, but I do not have
> > enough test environments, so ...
> I will do cygwin. Tomorrow, that is.

The patch works on cygwin. I let the tests run, too.

Re: make gitfakemmap standalone to fix linking error in git.c

[Alex Riesen]

On 12/5/05, Junio C Hamano <junkio@cox.net> wrote:
> >> > Why does it always happen...
> >> Because you touched you did not absolutely have to ;-).
> >
> > well, git$(X) didn't link...
>
> I meant your change to the "if ()" expression in gitfakemmap().
> Does the change have anything to do with git$X linkage?  I think
> not.

Oh, yes. Sorry. Too much sorries recently :(

Re: [PATCH] Clean up compatibility definitions.

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> On 12/5/05, Alex Riesen <raa.lkml@gmail.com> wrote:
>> Junio C Hamano, Mon, Dec 05, 2005 21:51:56 +0100:
>> > >> This attempts to clean up the way various compatibility
>> > >> functions are defined and used.
>> > >
>> > > You sure you want that before 1.0?
>> >
>> > I think this is mostly an obvious clean-up, but I do not have
>> > enough test environments, so ...
>> I will do cygwin. Tomorrow, that is.
>
> The patch works on cygwin. I let the tests run, too.

Thanks.  I've had a chance to try the one in the "master" branch
on cygwin myself today and it seemed to work OK.  I did a
partial build on Solaris 5.9 as well.

Re: [PATCH] Clean up compatibility definitions.

[Alex Riesen]

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

On 12/6/05, Junio C Hamano <junkio@cox.net> wrote:
> > ... return MAP_FAILED if error, do not
> > exit process, do no output.
> Ah, that is what you meant.  I agree.

now, while we agree, how about the patch removing "user interface"
from gitfakemmap altogether? The patch is not really tested (running
tests is not useful in this case), but I looked over the call sites,
and they seem either to handle mmap's or the previous open's return
values.
One exception is in config.c where neither is handled, the code will
race between stat and open.

Signed-off-by: Alex Riesen <fork0@gmail.com>

[-- Attachment #2: 0004-cleanup-mmap-fake.-No-need-to-output-anything-callers-handle-the-return-value.txt --]
[-- Type: text/plain, Size: 849 bytes --]

cleanup mmap fake. No need to output anything, callers handle the return value

Signed-off-by: Alex Riesen <fork0@gmail.com>

---

 compat/mmap.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

311da12b7aaca9b7329a07807e1ce4afba4bff77
diff --git a/compat/mmap.c b/compat/mmap.c
index 55cb120..32d7a30 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -8,15 +8,17 @@ void *gitfakemmap(void *start, size_t le
 {
 	int n = 0;
 
-	if (start != NULL || !(flags & MAP_PRIVATE))
-		die("Invalid usage of gitfakemmap.");
+	if (start != NULL || !(flags & MAP_PRIVATE)) {
+		errno = ENOTSUP;
+		return MAP_FAILED;
+	}
 
 	if (lseek(fd, offset, SEEK_SET) < 0) {
 		errno = EINVAL;
 		return MAP_FAILED;
 	}
 
-	start = xmalloc(length);
+	start = malloc(length);
 	if (start == NULL) {
 		errno = ENOMEM;
 		return MAP_FAILED;
-- 
0.99.9.GIT

Re: [PATCH] Clean up compatibility definitions.

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> On 12/6/05, Junio C Hamano <junkio@cox.net> wrote:
>> > ... return MAP_FAILED if error, do not
>> > exit process, do no output.
>> Ah, that is what you meant.  I agree.
>
> now, while we agree, how about the patch removing "user interface"
> from gitfakemmap altogether?

> 311da12b7aaca9b7329a07807e1ce4afba4bff77
> diff --git a/compat/mmap.c b/compat/mmap.c
> index 55cb120..32d7a30 100644
> --- a/compat/mmap.c
> +++ b/compat/mmap.c
> @@ -8,15 +8,17 @@ void *gitfakemmap(void *start, size_t le
>  {
>  	int n = 0;
>  
> -	if (start != NULL || !(flags & MAP_PRIVATE))
> -		die("Invalid usage of gitfakemmap.");
> +	if (start != NULL || !(flags & MAP_PRIVATE)) {
> +		errno = ENOTSUP;
> +		return MAP_FAILED;
> +	}

I've considered this one after hearing what you meant by your
previous comment, but I am slightly in favor of keeping this
die(); what would trigger this is a programmer error, and the
extra error message from die() makes it obvious, although I
agree that it is unclean from purist point of view.

On the other hand,

> -	start = xmalloc(length);
> +	start = malloc(length);

This one I'd agree with 100%.

Re: [PATCH] Clean up compatibility definitions.

[Petr Baudis]

Dear diary, on Tue, Dec 06, 2005 at 04:17:58AM CET, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> Petr Baudis <pasky@suse.cz> writes:
> 
> > Dear diary, on Mon, Dec 05, 2005 at 09:22:42PM CET, I got a letter
> > where Junio C Hamano <junkio@cox.net> said that...
> >> diff --git a/git-compat-util.h b/git-compat-util.h
> >> new file mode 100644
> >
> > What about compat/util.h or something? Nicer, shorter, and takes
> > advantage of this fancy hierarchical namespace "directories" invention.
> > ;-)
> 
> While I would appreciate a better name, I am afraid that is not a
> particularly good one.  It is not "compatibility utilities", but
> compat things and util things mixed together, so it does not
> belong to compat/ directory to begin with.  die() and friends
> are not about compatibility at all.

Then perhaps the .h file should be split? At least the "compat-util"
name did not suggest combination to me, but specialization (not
"compatibility and utils" but "compatibility utils").

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.