[PATCH 0/11] Miscellaneous MinGW port fallout

[PATCH 0/11] Miscellaneous MinGW port fallout

[Johannes Sixt]

This is a series of smallish, unrelated changes that were necessary
for the MinGW port.

[PATCH 01/11] t5300-pack-object.sh: Split the big verify-pack test into
	smaller parts.
[PATCH 02/11] t7501-commit.sh: Not all seds understand option -i
[PATCH 03/11] t5302-pack-index: Skip tests of 64-bit offsets if necessary.
[PATCH 04/11] Skip t3902-quoted.sh if the file system does not support
	funny names.

Some changes to the test suite.

[PATCH 05/11] Use is_absolute_path() in sha1_file.c.
[PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to
	git-compat-util.h.

These two are certainly undisputed.

[PATCH 07/11] builtin run_command: do not exit with -1.

Replaces exit(-1) by exit(255). I don't know if this has any bad
consequences on *nix.

[PATCH 08/11] Close files opened by lock_file() before unlinking.

This one was authored by Dscho. It is a definite MUST on Windows.

[PATCH 09/11] Allow a relative builtin template directory.
[PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
	of ETC_GITCONFIG.
[PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.

These need probably some discussion. They avoid that $(prefix) is
hardcoded and so allows that an arbitrary installation directory.

 Makefile               |    5 ++++-
 builtin-config.c       |    4 ++--
 builtin-init-db.c      |   16 +++++++++++++---
 cache.h                |    2 ++
 config.c               |   20 ++++++++++++++++++--
 git-compat-util.h      |    2 ++
 git.c                  |    2 +-
 help.c                 |    1 -
 lockfile.c             |   17 ++++++++++-------
 pager.c                |    2 --
 sha1_file.c            |    8 ++++----
 t/t3902-quoted.sh      |    7 +++++++
 t/t5300-pack-object.sh |   34 ++++++++++++++++++----------------
 t/t5302-pack-index.sh  |   25 +++++++++++++++++++++++--
 t/t7501-commit.sh      |    6 ++++--
 15 files changed, 108 insertions(+), 43 deletions(-)

-- Hannes

[PATCH 01/11] t5300-pack-object.sh: Split the big verify-pack test into smaller parts.

[Johannes Sixt]

This makes it easier to spot which of the tests failed.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 t/t5300-pack-object.sh |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index ba7579c..f1106e6 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -187,49 +187,51 @@ test_expect_success \
 			test-3-${packname_3}.idx'
 
 test_expect_success \
-    'corrupt a pack and see if verify catches' \
+    'verify-pack catches mismatched .idx and .pack files' \
     'cat test-1-${packname_1}.idx >test-3.idx &&
      cat test-2-${packname_2}.pack >test-3.pack &&
      if git verify-pack test-3.idx
      then false
      else :;
-     fi &&
+     fi'
 
-     : PACK_SIGNATURE &&
-     cat test-1-${packname_1}.pack >test-3.pack &&
+test_expect_success \
+    'verify-pack catches a corrupted pack signature' \
+    'cat test-1-${packname_1}.pack >test-3.pack &&
      dd if=/dev/zero of=test-3.pack count=1 bs=1 conv=notrunc seek=2 &&
      if git verify-pack test-3.idx
      then false
      else :;
-     fi &&
+     fi'
 
-     : PACK_VERSION &&
-     cat test-1-${packname_1}.pack >test-3.pack &&
+test_expect_success \
+    'verify-pack catches a corrupted pack version' \
+    'cat test-1-${packname_1}.pack >test-3.pack &&
      dd if=/dev/zero of=test-3.pack count=1 bs=1 conv=notrunc seek=7 &&
      if git verify-pack test-3.idx
      then false
      else :;
-     fi &&
+     fi'
 
-     : TYPE/SIZE byte of the first packed object data &&
-     cat test-1-${packname_1}.pack >test-3.pack &&
+test_expect_success \
+    'verify-pack catches a corrupted type/size of the 1st packed object data' \
+    'cat test-1-${packname_1}.pack >test-3.pack &&
      dd if=/dev/zero of=test-3.pack count=1 bs=1 conv=notrunc seek=12 &&
      if git verify-pack test-3.idx
      then false
      else :;
-     fi &&
+     fi'
 
-     : sum of the index file itself &&
-     l=`wc -c <test-3.idx` &&
+test_expect_success \
+    'verify-pack catches a corrupted sum of the index file itself' \
+    'l=`wc -c <test-3.idx` &&
      l=`expr $l - 20` &&
      cat test-1-${packname_1}.pack >test-3.pack &&
      dd if=/dev/zero of=test-3.idx count=20 bs=1 conv=notrunc seek=$l &&
      if git verify-pack test-3.pack
      then false
      else :;
-     fi &&
-
-     :'
+     fi'
 
 test_expect_success \
     'build pack index for an existing pack' \
-- 
1.5.3.5.1592.g0d6db

[PATCH 02/11] t7501-commit.sh: Not all seds understand option -i

[Johannes Sixt]

Use mv instead.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 t/t7501-commit.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 4dc35bd..38db2f2 100644
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -69,7 +69,8 @@ test_expect_success \
 
 cat >editor <<\EOF
 #!/bin/sh
-sed -i -e "s/a file/an amend commit/g" $1
+sed -e "s/a file/an amend commit/g" < $1 > $1-
+mv $1- $1
 EOF
 chmod 755 editor
 
@@ -88,7 +89,8 @@ test_expect_success \
 
 cat >editor <<\EOF
 #!/bin/sh
-sed -i -e "s/amend/older/g" $1
+sed -e "s/amend/older/g"  < $1 > $1-
+mv $1- $1
 EOF
 chmod 755 editor
 
-- 
1.5.3.5.1592.g0d6db

[PATCH 03/11] t5302-pack-index: Skip tests of 64-bit offsets if necessary.

[Johannes Sixt]

There are platforms where off_t is not 64 bits wide. In this case many tests
are doomed to fail. Let's skip them.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 t/t5302-pack-index.sh |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 4f58c4c..d93abc4 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -61,17 +61,33 @@ test_expect_success \
 
 test_expect_success \
     'index v2: force some 64-bit offsets with pack-objects' \
-    'pack3=$(git pack-objects --index-version=2,0x40000 test-3 <obj-list) &&
-     git verify-pack -v "test-3-${pack3}.pack"'
+    'pack3=$(git pack-objects --index-version=2,0x40000 test-3 <obj-list)'
+
+have_64bits=
+if msg=$(git verify-pack -v "test-3-${pack3}.pack" 2>&1) ||
+	! echo "$msg" | grep "pack too large .* off_t"
+then
+	have_64bits=t
+else
+	say "skipping tests concerning 64-bit offsets"
+fi
+
+test "$have_64bits" &&
+test_expect_success \
+    'index v2: verify a pack with some 64-bit offsets' \
+    'git verify-pack -v "test-3-${pack3}.pack"'
 
+test "$have_64bits" &&
 test_expect_failure \
     '64-bit offsets: should be different from previous index v2 results' \
     'cmp "test-2-${pack2}.idx" "test-3-${pack3}.idx"'
 
+test "$have_64bits" &&
 test_expect_success \
     'index v2: force some 64-bit offsets with index-pack' \
     'git-index-pack --index-version=2,0x40000 -o 3.idx "test-1-${pack1}.pack"'
 
+test "$have_64bits" &&
 test_expect_success \
     '64-bit offsets: index-pack result should match pack-objects one' \
     'cmp "test-3-${pack3}.idx" "3.idx"'
@@ -113,6 +129,7 @@ test_expect_failure \
     '[index v1] 6) newly created pack is BAD !' \
     'git verify-pack -v "test-4-${pack1}.pack"'
 
+test "$have_64bits" &&
 test_expect_success \
     '[index v2] 1) stream pack to repository' \
     'rm -f .git/objects/pack/* &&
@@ -122,6 +139,7 @@ test_expect_success \
      cmp "test-1-${pack1}.pack" ".git/objects/pack/pack-${pack1}.pack" &&
      cmp "test-3-${pack1}.idx"  ".git/objects/pack/pack-${pack1}.idx"'
 
+test "$have_64bits" &&
 test_expect_success \
     '[index v2] 2) create a stealth corruption in a delta base reference' \
     '# this test assumes a delta smaller than 16 bytes at the end of the pack
@@ -134,14 +152,17 @@ test_expect_success \
 	  bs=1 count=20 conv=notrunc &&
        git cat-file blob "$delta_sha1" > blob_4 )'
 
+test "$have_64bits" &&
 test_expect_failure \
     '[index v2] 3) corrupted delta happily returned wrong data' \
     'cmp blob_3 blob_4'
 
+test "$have_64bits" &&
 test_expect_failure \
     '[index v2] 4) confirm that the pack is actually corrupted' \
     'git fsck --full $commit'
 
+test "$have_64bits" &&
 test_expect_failure \
     '[index v2] 5) pack-objects refuses to reuse corrupted data' \
     'git pack-objects test-5 <obj-list'
-- 
1.5.3.5.1592.g0d6db

[PATCH 04/11] Skip t3902-quoted.sh if the file system does not support funny names.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 t/t3902-quoted.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t3902-quoted.sh b/t/t3902-quoted.sh
index 245fb3b..73da45f 100755
--- a/t/t3902-quoted.sh
+++ b/t/t3902-quoted.sh
@@ -20,6 +20,13 @@ LF='
 '
 DQ='"'
 
+echo foo > "Name and an${HT}HT"
+test -f "Name and an${HT}HT" || {
+	# since FAT/NTFS does not allow tabs in filenames, skip this test
+	say 'Your filesystem does not allow tabs in filenames, test skipped.'
+	test_done
+}
+
 for_each_name () {
 	for name in \
 	    Name "Name and a${LF}LF" "Name and an${HT}HT" "Name${DQ}" \
-- 
1.5.3.5.1592.g0d6db

[PATCH 05/11] Use is_absolute_path() in sha1_file.c.

[Johannes Sixt]

There are some places that test for an absolute path. Use the helper
function to ease porting.

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

diff --git a/sha1_file.c b/sha1_file.c
index f007874..560c0e0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
 	char *pos = path;
 	struct stat st;
 
-	if (*pos == '/')
+	if (is_absolute_path(path))
 		pos++;
 
 	while (pos) {
@@ -253,7 +253,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 	int entlen = pfxlen + 43;
 	int base_len = -1;
 
-	if (*entry != '/' && relative_base) {
+	if (!is_absolute_path(entry) && relative_base) {
 		/* Relative alt-odb */
 		if (base_len < 0)
 			base_len = strlen(relative_base) + 1;
@@ -262,7 +262,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 	}
 	ent = xmalloc(sizeof(*ent) + entlen);
 
-	if (*entry != '/' && relative_base) {
+	if (!is_absolute_path(entry) && relative_base) {
 		memcpy(ent->base, relative_base, base_len - 1);
 		ent->base[base_len - 1] = '/';
 		memcpy(ent->base + base_len, entry, len);
@@ -333,7 +333,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 		while (cp < ep && *cp != sep)
 			cp++;
 		if (last != cp) {
-			if ((*last != '/') && depth) {
+			if (!is_absolute_path(last) && depth) {
 				error("%s: ignoring relative alternate object store %s",
 						relative_base, last);
 			} else {
-- 
1.5.3.5.1592.g0d6db

[PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to git-compat-util.h.

[Johannes Sixt]

... since all system headers are pulled in via git-compat-util.h

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

diff --git a/git-compat-util.h b/git-compat-util.h
index 7b29d1b..6a8b592 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -52,6 +52,8 @@
 #include <fnmatch.h>
 #include <sys/poll.h>
 #include <sys/socket.h>
+#include <sys/ioctl.h>
+#include <sys/select.h>
 #include <assert.h>
 #include <regex.h>
 #include <netinet/in.h>
diff --git a/help.c b/help.c
index 8217d97..d340b6a 100644
--- a/help.c
+++ b/help.c
@@ -7,7 +7,6 @@
 #include "builtin.h"
 #include "exec_cmd.h"
 #include "common-cmds.h"
-#include <sys/ioctl.h>
 
 /* most GUI terminals set COLUMNS (although some don't export it) */
 static int term_columns(void)
diff --git a/pager.c b/pager.c
index 8bac9d9..fb7a1a6 100644
--- a/pager.c
+++ b/pager.c
@@ -1,7 +1,5 @@
 #include "cache.h"
 
-#include <sys/select.h>
-
 /*
  * This is split up from the rest of git so that we might do
  * something different on Windows, for example.
-- 
1.5.3.5.1592.g0d6db

[PATCH 07/11] builtin run_command: do not exit with -1.

[Johannes Sixt]

There are shells that do not correctly detect an exit code of -1 as a
failure. We simply truncate the status code to the lower 8 bits.

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

diff --git a/git.c b/git.c
index 4a250f7..37d99d6 100644
--- a/git.c
+++ b/git.c
@@ -256,7 +256,7 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 
 	status = p->fn(argc, argv, prefix);
 	if (status)
-		return status;
+		return status & 0xff;
 
 	/* Somebody closed stdout? */
 	if (fstat(fileno(stdout), &st))
-- 
1.5.3.5.1592.g0d6db

[PATCH 08/11] Close files opened by lock_file() before unlinking.

[Johannes Sixt]

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

This is needed on Windows since open files cannot be unlinked.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---

	This was authored by Dscho, but carries only my sign-off.
	Is this ok?

 cache.h    |    1 +
 lockfile.c |   17 ++++++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index f0a25c7..28870c5 100644
--- a/cache.h
+++ b/cache.h
@@ -284,6 +284,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const char **
 
 struct lock_file {
 	struct lock_file *next;
+	int fd;
 	pid_t owner;
 	char on_list;
 	char filename[PATH_MAX];
diff --git a/lockfile.c b/lockfile.c
index 9a1f64d..258fb3f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -12,8 +12,10 @@ static void remove_lock_file(void)
 
 	while (lock_file_list) {
 		if (lock_file_list->owner == me &&
-		    lock_file_list->filename[0])
+		    lock_file_list->filename[0]) {
+			close(lock_file_list->fd);
 			unlink(lock_file_list->filename);
+		}
 		lock_file_list = lock_file_list->next;
 	}
 }
@@ -120,8 +122,6 @@ static char *resolve_symlink(char *p, size_t s)
 
 static int lock_file(struct lock_file *lk, const char *path)
 {
-	int fd;
-
 	if (strlen(path) >= sizeof(lk->filename)) return -1;
 	strcpy(lk->filename, path);
 	/*
@@ -130,8 +130,8 @@ static int lock_file(struct lock_file *lk, const char *path)
 	 */
 	resolve_symlink(lk->filename, sizeof(lk->filename)-5);
 	strcat(lk->filename, ".lock");
-	fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
-	if (0 <= fd) {
+	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
+	if (0 <= lk->fd) {
 		if (!lock_file_list) {
 			signal(SIGINT, remove_lock_file_on_signal);
 			atexit(remove_lock_file);
@@ -148,7 +148,7 @@ static int lock_file(struct lock_file *lk, const char *path)
 	}
 	else
 		lk->filename[0] = 0;
-	return fd;
+	return lk->fd;
 }
 
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on_error)
@@ -163,6 +163,7 @@ int commit_lock_file(struct lock_file *lk)
 {
 	char result_file[PATH_MAX];
 	int i;
+	close(lk->fd);
 	strcpy(result_file, lk->filename);
 	i = strlen(result_file) - 5; /* .lock */
 	result_file[i] = 0;
@@ -194,7 +195,9 @@ int commit_locked_index(struct lock_file *lk)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (lk->filename[0])
+	if (lk->filename[0]) {
+		close(lk->fd);
 		unlink(lk->filename);
+	}
 	lk->filename[0] = 0;
 }
-- 
1.5.3.5.1592.g0d6db

[PATCH 09/11] Allow a relative builtin template directory.

[Johannes Sixt]

In order to make git relocatable (i.e. not have the prefix compiled-in)
the template directory must depend on the location where this git instance
is found, which is GIT_EXEC_DIR.

The exec path is prepended only if the compiled-in default template
directory is to be used and that is relative. Any relative directories
that are specified via environment variable or the --exec-dir switch are
taken as is.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 builtin-init-db.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 763fa55..e1393b8 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -5,6 +5,7 @@
  */
 #include "cache.h"
 #include "builtin.h"
+#include "exec_cmd.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -131,10 +132,19 @@ static void copy_templates(const char *git_dir, int len, const char *template_di
 	int template_len;
 	DIR *dir;
 
-	if (!template_dir) {
+	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
-		if (!template_dir)
-			template_dir = DEFAULT_GIT_TEMPLATE_DIR;
+	if (!template_dir) {
+		/*
+		 * if the hard-coded template is relative, it is
+		 * interpreted relative to the exec_dir
+		 */
+		template_dir = DEFAULT_GIT_TEMPLATE_DIR;
+		if (!is_absolute_path(template_dir)) {
+			const char *exec_path = git_exec_path();
+			template_dir = prefix_path(exec_path, strlen(exec_path),
+						   template_dir);
+		}
 	}
 	strcpy(template_path, template_dir);
 	template_len = strlen(template_path);
-- 
1.5.3.5.1592.g0d6db

[PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access of ETC_GITCONFIG.

[Johannes Sixt]

In a subsequent patch the path to the system-wide config file will be
computed. This is a preparation for that change. It turns all accesses
of ETC_GITCONFIG into function calls. There is no change in behavior.

As a consequence, config.c is the only file that needs the definition of
ETC_GITCONFIG. Hence, -DETC_GITCONFIG is removed from the CFLAGS and a
special build rule for config.c is introduced. As a side-effect, changing
the defintion of ETC_GITCONFIG (e.g. in config.mak) does not trigger a
complete rebuild anymore.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile         |    5 ++++-
 builtin-config.c |    4 ++--
 cache.h          |    1 +
 config.c         |    9 +++++++--
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 18c61f3..e32f07e 100644
--- a/Makefile
+++ b/Makefile
@@ -752,7 +752,7 @@ TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
-	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"' $(COMPAT_CFLAGS)
+	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
@@ -901,6 +901,9 @@ exec_cmd.o: exec_cmd.c GIT-CFLAGS
 builtin-init-db.o: builtin-init-db.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' $<
 
+config.o: config.c GIT-CFLAGS
+	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"' $<
+
 http.o: http.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DGIT_USER_AGENT='"git/$(GIT_VERSION)"' $<
 
diff --git a/builtin-config.c b/builtin-config.c
index e5e243f..f672c9c 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -81,7 +81,7 @@ static int get_value(const char* key_, const char* regex_)
 			local = repo_config = xstrdup(git_path("config"));
 		if (home)
 			global = xstrdup(mkpath("%s/.gitconfig", home));
-		system_wide = ETC_GITCONFIG;
+		system_wide = git_etc_gitconfig();
 	}
 
 	key = xstrdup(key_);
@@ -191,7 +191,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			}
 		}
 		else if (!strcmp(argv[1], "--system"))
-			setenv(CONFIG_ENVIRONMENT, ETC_GITCONFIG, 1);
+			setenv(CONFIG_ENVIRONMENT, git_etc_gitconfig(), 1);
 		else if (!strcmp(argv[1], "--file") || !strcmp(argv[1], "-f")) {
 			if (argc < 3)
 				usage(git_config_set_usage);
diff --git a/cache.h b/cache.h
index 28870c5..0eb7b43 100644
--- a/cache.h
+++ b/cache.h
@@ -551,6 +551,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
+extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value);
 
 #define MAX_GITNAME (1000)
diff --git a/config.c b/config.c
index dc3148d..dd7e9ad 100644
--- a/config.c
+++ b/config.c
@@ -452,6 +452,11 @@ int git_config_from_file(config_fn_t fn, const char *filename)
 	return ret;
 }
 
+const char *git_etc_gitconfig(void)
+{
+	return ETC_GITCONFIG;
+}
+
 int git_config(config_fn_t fn)
 {
 	int ret = 0;
@@ -464,8 +469,8 @@ int git_config(config_fn_t fn)
 	 * config file otherwise. */
 	filename = getenv(CONFIG_ENVIRONMENT);
 	if (!filename) {
-		if (!access(ETC_GITCONFIG, R_OK))
-			ret += git_config_from_file(fn, ETC_GITCONFIG);
+		if (!access(git_etc_gitconfig(), R_OK))
+			ret += git_config_from_file(fn, git_etc_gitconfig());
 		home = getenv("HOME");
 		filename = getenv(CONFIG_LOCAL_ENVIRONMENT);
 		if (!filename)
-- 
1.5.3.5.1592.g0d6db

[PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.

[Johannes Sixt]

If ETC_GITCONFIG is not an absolute path, interpret it relative to
--exec-dir. This makes the installed binaries relocatable because the
prefix is not compiled-in.

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

diff --git a/config.c b/config.c
index dd7e9ad..9f014bb 100644
--- a/config.c
+++ b/config.c
@@ -6,6 +6,7 @@
  *
  */
 #include "cache.h"
+#include "exec_cmd.h"
 
 #define MAXNAME (256)
 
@@ -454,7 +455,17 @@ int git_config_from_file(config_fn_t fn, const char *filename)
 
 const char *git_etc_gitconfig(void)
 {
-	return ETC_GITCONFIG;
+	static const char *system_wide;
+	if (!system_wide) {
+		system_wide = ETC_GITCONFIG;
+		if (!is_absolute_path(system_wide)) {
+			/* interpret path relative to exec-dir */
+			const char *exec_path = git_exec_path();
+			system_wide = prefix_path(exec_path, strlen(exec_path),
+						system_wide);
+		}
+	}
+	return system_wide;
 }
 
 int git_config(config_fn_t fn)
-- 
1.5.3.5.1592.g0d6db

Re: [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.

[Steffen Prohaska]

On Nov 13, 2007, at 9:05 PM, Johannes Sixt wrote:

> If ETC_GITCONFIG is not an absolute path, interpret it relative to
> --exec-dir. This makes the installed binaries relocatable because the
> prefix is not compiled-in.
>
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
>  config.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/config.c b/config.c
> index dd7e9ad..9f014bb 100644
> --- a/config.c
> +++ b/config.c
> @@ -6,6 +6,7 @@
>   *
>   */
>  #include "cache.h"
> +#include "exec_cmd.h"
>
>  #define MAXNAME (256)
>
> @@ -454,7 +455,17 @@ int git_config_from_file(config_fn_t fn, const  
> char *filename)
>
>  const char *git_etc_gitconfig(void)
>  {
> -	return ETC_GITCONFIG;
> +	static const char *system_wide;
> +	if (!system_wide) {
> +		system_wide = ETC_GITCONFIG;
> +		if (!is_absolute_path(system_wide)) {
> +			/* interpret path relative to exec-dir */
> +			const char *exec_path = git_exec_path();
> +			system_wide = prefix_path(exec_path, strlen(exec_path),
> +						system_wide);
> +		}
> +	}
> +	return system_wide;
>  }

When I first stumbled over this code in msysgit I didn't
understand how to use it for making git relocatable. Maybe
I didn't appreciate the clue about the relative path, which
indicates to change the Makefile to provide ETC_GITCONFIG
starting with "..".

I think I stumbled over the idea to build a path by starting
with an absolute path to exec_path and than go up with
".." before going down again to etc/gitconfig. So, a paths
would for example be "C:/msysgit/bin/../etc/gitconfig".

Hence I wrote a function git_install_prefix(), which directly
points to "C:/msysgit". It is used in the following way:

----
			system_wide = prefix_path(exec_path, strlen(exec_path),
						system_wide);
		}
+#if __MINGW32__
+		/*
+		 * If it is an absolute Unix path it is prefixed with
+		 * the git_install_prefix().
+		 */
+		else if (system_wide[0] == '/') {
+			const char* prefix = git_install_prefix();
+			system_wide = prefix_path(prefix, strlen(prefix),
+			                           system_wide);
+		}
+#endif
	}
	return system_wide;
}

----

This is not very portable.  Prefixing _every_ absolute Unix path
makes sense for MINGW to map the fake unix root to its real
location in the Windows filesystem. But this probably doesn't
make sense on Unix at all.  And it's probably not needed at
all. I should have just read your code more carefully ;)

Your solution provides a sensible way to handle the issue.

Now I'm wondering if we could make path relocation a bit more
explicit.  How about doing something like.

	system_wide = relocate_path(ETC_GITCONFIG);

and relocate_path(const char *) would expand a format
string in path.  At this point I see only a single %s
that would be expanded with the install prefix.  If
ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
"C:/msysgit/bin/etc/gitconfig" for my above example.  It is
basically a printf with the install prefix path.

This would support three cases:
- absolute path
- relative path
- paths that will be relocated.  The path includes a '%s'
   to indicate relocation.

I'm not sure if we ever need to support relative paths
without relocating them.

What do you think?  If my comments make any sense to you,
I could code a patch next weekend.

BTW, if we changed PATCH 11/11 we'd obviously change PATCH
9/11, too.

	Steffen


>  int git_config(config_fn_t fn)
> -- 
> 1.5.3.5.1592.g0d6db

Re: [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.

[Johannes Sixt]

On Thursday 15 November 2007 07:53, Steffen Prohaska wrote:
> Now I'm wondering if we could make path relocation a bit more
> explicit.  How about doing something like.
>
> 	system_wide = relocate_path(ETC_GITCONFIG);
>
> and relocate_path(const char *) would expand a format
> string in path.  At this point I see only a single %s
> that would be expanded with the install prefix.  If
> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
> basically a printf with the install prefix path.

I don't see the problem that you are trying to solve.

-- Hannes

Re: [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.

[Steffen Prohaska]

On Nov 15, 2007, at 7:31 PM, Johannes Sixt wrote:

> On Thursday 15 November 2007 07:53, Steffen Prohaska wrote:
>> Now I'm wondering if we could make path relocation a bit more
>> explicit.  How about doing something like.
>>
>> 	system_wide = relocate_path(ETC_GITCONFIG);
>>
>> and relocate_path(const char *) would expand a format
>> string in path.  At this point I see only a single %s
>> that would be expanded with the install prefix.  If
>> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
>> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
>> basically a printf with the install prefix path.
>
> I don't see the problem that you are trying to solve.

Path relocation would be more explicit:
1) Paths that need to be relocated are filtered through
    relocate_path().  That's easy to understand.
2) All the code prefixing the path is in relocate_path().
    This avoids code duplication.
3) Path that should be relocated contain "%s" in the Makefile.
    This indicates that some string expansion may take place.

(1) and (2) would be useful even if you do not agree with (3).
The code in PATCH 9/11 and PATCH 11/11 looks very similar.
If the prefixing code went into a separate function, we'd
have less code.  Also, relocate_path() could be useful at
other places.  For example, I'd use it to locate the HTML
documentation relative to the installation directory.

	Steffen

Re: [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.

[Johannes Sixt]

On Thursday 15 November 2007 21:10, Steffen Prohaska wrote:
> On Nov 15, 2007, at 7:31 PM, Johannes Sixt wrote:
> > On Thursday 15 November 2007 07:53, Steffen Prohaska wrote:
> >> Now I'm wondering if we could make path relocation a bit more
> >> explicit.  How about doing something like.
> >>
> >> 	system_wide = relocate_path(ETC_GITCONFIG);
> >>
> >> and relocate_path(const char *) would expand a format
> >> string in path.  At this point I see only a single %s
> >> that would be expanded with the install prefix.  If
> >> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
> >> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
> >> basically a printf with the install prefix path.
> >
> > I don't see the problem that you are trying to solve.
>
> Path relocation would be more explicit:
> 1) Paths that need to be relocated are filtered through
>     relocate_path().  That's easy to understand.
> 2) All the code prefixing the path is in relocate_path().
>     This avoids code duplication.
> 3) Path that should be relocated contain "%s" in the Makefile.
>     This indicates that some string expansion may take place.
>
> (1) and (2) would be useful even if you do not agree with (3).
> The code in PATCH 9/11 and PATCH 11/11 looks very similar.
> If the prefixing code went into a separate function, we'd
> have less code.  Also, relocate_path() could be useful at
> other places.  For example, I'd use it to locate the HTML
> documentation relative to the installation directory.

How is %s/foo different from ../foo? There are only 2 paths that need to be 
relocatable. Your proposal is over-engineering, IMHO.

-- Hannes

Re: [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.

[Steffen Prohaska]

On Nov 15, 2007, at 10:43 PM, Johannes Sixt wrote:

> On Thursday 15 November 2007 21:10, Steffen Prohaska wrote:
>> On Nov 15, 2007, at 7:31 PM, Johannes Sixt wrote:
>>> On Thursday 15 November 2007 07:53, Steffen Prohaska wrote:
>>>> Now I'm wondering if we could make path relocation a bit more
>>>> explicit.  How about doing something like.
>>>>
>>>> 	system_wide = relocate_path(ETC_GITCONFIG);
>>>>
>>>> and relocate_path(const char *) would expand a format
>>>> string in path.  At this point I see only a single %s
>>>> that would be expanded with the install prefix.  If
>>>> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
>>>> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
>>>> basically a printf with the install prefix path.
>>>
>>> I don't see the problem that you are trying to solve.
>>
>> Path relocation would be more explicit:
>> 1) Paths that need to be relocated are filtered through
>>     relocate_path().  That's easy to understand.
>> 2) All the code prefixing the path is in relocate_path().
>>     This avoids code duplication.
>> 3) Path that should be relocated contain "%s" in the Makefile.
>>     This indicates that some string expansion may take place.
>>
>> (1) and (2) would be useful even if you do not agree with (3).
>> The code in PATCH 9/11 and PATCH 11/11 looks very similar.
>> If the prefixing code went into a separate function, we'd
>> have less code.  Also, relocate_path() could be useful at
>> other places.  For example, I'd use it to locate the HTML
>> documentation relative to the installation directory.
>
> How is %s/foo different from ../foo? There are only 2 paths that  
> need to be
> relocatable. Your proposal is over-engineering, IMHO.

fair enough. Let's take your patches.

	Steffen

Re: [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access of ETC_GITCONFIG.

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Johannes Sixt wrote:

> diff --git a/config.c b/config.c
> index dc3148d..dd7e9ad 100644
> --- a/config.c
> +++ b/config.c
> @@ -452,6 +452,11 @@ int git_config_from_file(config_fn_t fn, const char *filename)
>  	return ret;
>  }
>  
> +const char *git_etc_gitconfig(void)
> +{
> +	return ETC_GITCONFIG;
> +}
> +

Wouldn't this logically belong into environment.c?

Ciao,
Dscho

Re: [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access of ETC_GITCONFIG.

[Johannes Sixt]

On Tuesday 13 November 2007 22:22, Johannes Schindelin wrote:
> Hi,
>
> On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > diff --git a/config.c b/config.c
> > index dc3148d..dd7e9ad 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -452,6 +452,11 @@ int git_config_from_file(config_fn_t fn, const char
> > *filename) return ret;
> >  }
> >
> > +const char *git_etc_gitconfig(void)
> > +{
> > +	return ETC_GITCONFIG;
> > +}
> > +
>
> Wouldn't this logically belong into environment.c?

I don't think so. environment.c is about things that concern the repository. 
This is really only about configuration.

-- Hannes

Re: [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access of ETC_GITCONFIG.

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Johannes Sixt wrote:

> On Tuesday 13 November 2007 22:22, Johannes Schindelin wrote:
>
> > On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > > diff --git a/config.c b/config.c
> > > index dc3148d..dd7e9ad 100644
> > > --- a/config.c
> > > +++ b/config.c
> > > @@ -452,6 +452,11 @@ int git_config_from_file(config_fn_t fn, const char
> > > *filename) return ret;
> > >  }
> > >
> > > +const char *git_etc_gitconfig(void)
> > > +{
> > > +	return ETC_GITCONFIG;
> > > +}
> > > +
> >
> > Wouldn't this logically belong into environment.c?
> 
> I don't think so. environment.c is about things that concern the 
> repository. This is really only about configuration.

I thought that environment.c is about getting certain settings from the 
environment.  In that sense, git_etc_gitconfig() would live a happier life 
there.

But I do not care all that deeply.  I know _I_ would have looked for it in 
environment.c first, but then, there's always good ole' git-grep.

Ciao,
Dscho

Re: [PATCH 08/11] Close files opened by lock_file() before unlinking.

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Johannes Sixt wrote:

> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> This is needed on Windows since open files cannot be unlinked.
> 
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
> 
> 	This was authored by Dscho, but carries only my sign-off.
> 	Is this ok?

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

BTW: Hannes, many thanks for your efforts.  Much appreciated.

Ciao,
Dscho

Re: [PATCH 05/11] Use is_absolute_path() in sha1_file.c.

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Johannes Sixt wrote:

> diff --git a/sha1_file.c b/sha1_file.c
> index f007874..560c0e0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
>  	char *pos = path;
>  	struct stat st;
>  
> -	if (*pos == '/')
> +	if (is_absolute_path(path))
>  		pos++;

Is this enough?  On Windows, certain "absolute" paths would need "pos += 
3", no?

Ciao,
Dscho

Re: [PATCH 05/11] Use is_absolute_path() in sha1_file.c.

[Johannes Sixt]

On Tuesday 13 November 2007 22:39, Johannes Schindelin wrote:
> Hi,
>
> On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > diff --git a/sha1_file.c b/sha1_file.c
> > index f007874..560c0e0 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
> >  	char *pos = path;
> >  	struct stat st;
> >
> > -	if (*pos == '/')
> > +	if (is_absolute_path(path))
> >  		pos++;
>
> Is this enough?  On Windows, certain "absolute" paths would need "pos +=
> 3", no?

True, but this series is not yet about the MinGW port itself. It will be 
changed eventually, but not at this time.

-- Hannes

Re: [PATCH 05/11] Use is_absolute_path() in sha1_file.c.

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Johannes Sixt wrote:

> On Tuesday 13 November 2007 22:39, Johannes Schindelin wrote:
>
> > On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > > diff --git a/sha1_file.c b/sha1_file.c
> > > index f007874..560c0e0 100644
> > > --- a/sha1_file.c
> > > +++ b/sha1_file.c
> > > @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
> > >  	char *pos = path;
> > >  	struct stat st;
> > >
> > > -	if (*pos == '/')
> > > +	if (is_absolute_path(path))
> > >  		pos++;
> >
> > Is this enough?  On Windows, certain "absolute" paths would need "pos +=
> > 3", no?
> 
> True, but this series is not yet about the MinGW port itself. It will be 
> changed eventually, but not at this time.

Okay, fair enough.

Ciao,
Dscho

Re: [PATCH 05/11] Use is_absolute_path() in sha1_file.c.

[Robin Rosenberg]

tisdag 13 november 2007 skrev Johannes Sixt:
> On Tuesday 13 November 2007 22:39, Johannes Schindelin wrote:
> > Hi,
> >
> > On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > > diff --git a/sha1_file.c b/sha1_file.c
> > > index f007874..560c0e0 100644
> > > --- a/sha1_file.c
> > > +++ b/sha1_file.c
> > > @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
> > >  	char *pos = path;
> > >  	struct stat st;
> > >
> > > -	if (*pos == '/')
> > > +	if (is_absolute_path(path))
> > >  		pos++;
> >
> > Is this enough?  On Windows, certain "absolute" paths would need "pos +=
> > 3", no?
> 
> True, but this series is not yet about the MinGW port itself. It will be 
> changed eventually, but not at this time.

D:/FOO and //deathstar/core work fine in cygwin, which is supported. but git
do not work well with such paths in some places.

-- robin

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Johannes Sixt]

On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
> [PATCH 09/11] Allow a relative builtin template directory.
> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> 	of ETC_GITCONFIG.
> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
>
> These need probably some discussion. They avoid that $(prefix) is
> hardcoded and so allows that an arbitrary installation directory.

... and so allow that the compiled binaries are installed in any directory 
that the user chooses.

-- Hannes

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Junio C Hamano]

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

> On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
>> [PATCH 09/11] Allow a relative builtin template directory.
>> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
>> 	of ETC_GITCONFIG.
>> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
>>
>> These need probably some discussion. They avoid that $(prefix) is
>> hardcoded and so allows that an arbitrary installation directory.
>
> ... and so allow that the compiled binaries are installed in any directory 
> that the user chooses.

If you can do that without breaking the tests (specifically, the
test script should pick up the version of git you just built,
not from /usr/bin nor /usr/local/stow/git/bin) that would be
great.

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Johannes Sixt]

On Tuesday 13 November 2007 22:10, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
> >> [PATCH 09/11] Allow a relative builtin template directory.
> >> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> >> 	of ETC_GITCONFIG.
> >> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> >>
> >> These need probably some discussion. They avoid that $(prefix) is
> >> hardcoded and so allows that an arbitrary installation directory.
> >
> > ... and so allow that the compiled binaries are installed in any
> > directory that the user chooses.
>
> If you can do that without breaking the tests (specifically, the
> test script should pick up the version of git you just built,
> not from /usr/bin nor /usr/local/stow/git/bin) that would be
> great.

Sorry, I don't understand your statement. Do you see any tests breaking?

These changes are only about where git-init looks up the templates and where 
the system configuration is looked for. They change git's behavior only if 
config.mak contains relative paths like this:

sysconfdir = ../etc
template_dir = ../share/git-core

It has nothing to do with where the test suite looks for the executables.

-- Hannes

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Johannes Sixt wrote:

> On Tuesday 13 November 2007 22:10, Junio C Hamano wrote:
> > Johannes Sixt <johannes.sixt@telecom.at> writes:
> > > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
> > >> [PATCH 09/11] Allow a relative builtin template directory.
> > >> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> > >> 	of ETC_GITCONFIG.
> > >> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> > >>
> > >> These need probably some discussion. They avoid that $(prefix) is
> > >> hardcoded and so allows that an arbitrary installation directory.
> > >
> > > ... and so allow that the compiled binaries are installed in any
> > > directory that the user chooses.
> >
> > If you can do that without breaking the tests (specifically, the
> > test script should pick up the version of git you just built,
> > not from /usr/bin nor /usr/local/stow/git/bin) that would be
> > great.
> 
> Sorry, I don't understand your statement. Do you see any tests breaking?

I guess what Junio is getting at: if your changes could lead to our not 
needing to hard code defaults, that would be awesome.

For example, a very unhappy camper reported recently that installing git 
with a different prefix triggers a complete rebuild.

Ciao,
Dscho

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Johannes Sixt]

On Tuesday 13 November 2007 22:46, Johannes Schindelin wrote:
> Hi,
>
> On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > On Tuesday 13 November 2007 22:10, Junio C Hamano wrote:
> > > Johannes Sixt <johannes.sixt@telecom.at> writes:
> > > > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
> > > >> [PATCH 09/11] Allow a relative builtin template directory.
> > > >> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> > > >> 	of ETC_GITCONFIG.
> > > >> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> > > >>
> > > >> These need probably some discussion. They avoid that $(prefix) is
> > > >> hardcoded and so allows that an arbitrary installation directory.
> > > >
> > > > ... and so allow that the compiled binaries are installed in any
> > > > directory that the user chooses.
> > >
> > > If you can do that without breaking the tests (specifically, the
> > > test script should pick up the version of git you just built,
> > > not from /usr/bin nor /usr/local/stow/git/bin) that would be
> > > great.
> >
> > Sorry, I don't understand your statement. Do you see any tests breaking?
>
> I guess what Junio is getting at: if your changes could lead to our not
> needing to hard code defaults, that would be awesome.
>
> For example, a very unhappy camper reported recently that installing git
> with a different prefix triggers a complete rebuild.

[PATCH 10/11] is one step into this direction.

-- Hannes

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Junio C Hamano]

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

> On Tue, 13 Nov 2007, Johannes Sixt wrote:
>> On Tuesday 13 November 2007 22:10, Junio C Hamano wrote:
>> > Johannes Sixt <johannes.sixt@telecom.at> writes:
>> > > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
>> > > ... and so allow that the compiled binaries are installed in any
>> > > directory that the user chooses.
>> >
>> > If you can do that without breaking the tests (specifically, the
>> > test script should pick up the version of git you just built,
>> > not from /usr/bin nor /usr/local/stow/git/bin) that would be
>> > great.
>> 
>> Sorry, I don't understand your statement. Do you see any tests breaking?
>
> I guess what Junio is getting at: if your changes could lead to our not 
> needing to hard code defaults, that would be awesome.

Yes.  Another thing I said was that it would be grave regression
for testability if the change leads git to look at somewhere
else other than it was told to look via GIT_EXEC_PATH
environment,

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Junio C Hamano]

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

> This is a series of smallish, unrelated changes that were necessary
> for the MinGW port.

I was _VERY_ afraid of reviewing this series.

> [PATCH 05/11] Use is_absolute_path() in sha1_file.c.
> [PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to
> 	git-compat-util.h.
>
> These two are certainly undisputed.

Except on esoteric/broken systems there might be some dependency
on the order the system include files are included, so 06/11
needs some testing.  But it is a change in the right direction.

> [PATCH 07/11] builtin run_command: do not exit with -1.
>
> Replaces exit(-1) by exit(255). I don't know if this has any bad
> consequences on *nix.

Linux manual page says "the value of status & 0377 is returned
to the parent", which agrees with POSIX's "only the least
significant 8 bits(that is, status & 0377) shall be available to
a waiting parent process".  So I think we are safe on conforming
platforms.

> [PATCH 08/11] Close files opened by lock_file() before unlinking.
>
> This one was authored by Dscho. It is a definite MUST on Windows.

This was something we've talked about doing a few times on the
list but did not.  It is good that this saw some testing in the
field, as it is easy to get wrong while moving the call site of
close(2) around.

> [PATCH 09/11] Allow a relative builtin template directory.
> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> 	of ETC_GITCONFIG.
> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
>
> These need probably some discussion. They avoid that $(prefix) is
> hardcoded and so allows that an arbitrary installation directory.

I had to worry a bit about bootstrapping issues in 11/11.  We
need to ensure that anybody who wants to read the configuration
data first does setup_path() because git_exec_path() reads from
argv_exec_path and setup_path() is what assigns to that
variable.

But other than that and 08/11, I found everything is trivially
correct and it was a pleasant read.

Thanks.

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Johannes Schindelin]

Hi,

On Wed, 14 Nov 2007, Junio C Hamano wrote:

> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
> > This is a series of smallish, unrelated changes that were necessary
> > for the MinGW port.
> 
> I was _VERY_ afraid of reviewing this series.

Why?  Because we get closer to MinGW integration into git.git for real? 
;-)

> > [PATCH 05/11] Use is_absolute_path() in sha1_file.c.
> > [PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to
> > 	git-compat-util.h.
> >
> > These two are certainly undisputed.
> 
> Except on esoteric/broken systems there might be some dependency
> on the order the system include files are included, so 06/11
> needs some testing.  But it is a change in the right direction.

The safe thing, of course, would be to move them at the end of the include 
list in git-compat-util.h, since they are now included after cache.h, 
(which includes git-compat-util.h and only strbuf.h, the sha1 header and 
zlib.h).

This way it should be really certainly undisputed.

> > [PATCH 08/11] Close files opened by lock_file() before unlinking.
> >
> > This one was authored by Dscho. It is a definite MUST on Windows.
> 
> This was something we've talked about doing a few times on the
> list but did not.  It is good that this saw some testing in the
> field, as it is easy to get wrong while moving the call site of
> close(2) around.

Note that we are not strictly _moving_ it around. In fact, we are _adding_ 
more close() calls...  And even ignoring the errors when close() was 
already called, so it feels a tad hacky.  But it does the job.

> > [PATCH 09/11] Allow a relative builtin template directory.
> > [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> > 	of ETC_GITCONFIG.
> > [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> >
> > These need probably some discussion. They avoid that $(prefix) is
> > hardcoded and so allows that an arbitrary installation directory.
> 
> I had to worry a bit about bootstrapping issues in 11/11.  We
> need to ensure that anybody who wants to read the configuration
> data first does setup_path() because git_exec_path() reads from
> argv_exec_path and setup_path() is what assigns to that
> variable.

Just to be safe in the future, we could check for that condition (by 
introducing a static variable setup_path_called) and die() should anybody 
introduce a code path where the order of calls is not maintained.

> But other than that and 08/11, I found everything is trivially correct 
> and it was a pleasant read.

Me, too.

Ciao,
Dscho

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Junio C Hamano]

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

> On Wed, 14 Nov 2007, Junio C Hamano wrote:
>
>> Johannes Sixt <johannes.sixt@telecom.at> writes:
>> 
>> > This is a series of smallish, unrelated changes that were necessary
>> > for the MinGW port.
>> 
>> I was _VERY_ afraid of reviewing this series.
>
> Why?  Because we get closer to MinGW integration into git.git for real? 
> ;-)

I know you know me better than that.

Conversion from "Too UNIXy" stuff into another form that "claims
to" run on a different platform that I do not have a good way of
testing myself?  I should feel scared, even if I can always punt
and say "I'll only make sure UNIX side does not regress,
Windose, who cares" ;-).

Re: [PATCH 0/11] Miscellaneous MinGW port fallout

[Johannes Schindelin]

Hi,

On Wed, 14 Nov 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 14 Nov 2007, Junio C Hamano wrote:
> >
> >> Johannes Sixt <johannes.sixt@telecom.at> writes:
> >> 
> >> > This is a series of smallish, unrelated changes that were necessary
> >> > for the MinGW port.
> >> 
> >> I was _VERY_ afraid of reviewing this series.
> >
> > Why?  Because we get closer to MinGW integration into git.git for real? 
> > ;-)
> 
> I know you know me better than that.
> 
> Conversion from "Too UNIXy" stuff into another form that "claims to" run 
> on a different platform that I do not have a good way of testing myself?  
> I should feel scared, even if I can always punt and say "I'll only make 
> sure UNIX side does not regress, Windose, who cares" ;-).

Yes, I do know you better than that. But I guess git grows up, and there 
is no way that you can guarantee it to perform well everywhere.

But I know some people who will make sure that it works on Windows.  For 
them, at least. ;-)

Ciao,
Dscho