Intoducing the .git file (again)

These patches enables .git to be a textfile containing the path to the git
directory proper. It passes all the tests so hopefully there are no
regressions, but there may be bugs and omissions lurking when this feature
is actually used; I've exercised it in my git, cgit and dayjob repos but git
is big and has many codepaths so I wouldn't be suprised if there still are
some git commands left which fails to obey the .git file.

PS: These patches could certainly be squashed into a single patch, but I've
left them as is to make each one easier to review.

PPS: If included, the .git file should probably be used by git-submodule to
clone submodule repositories into something like $GIT_DIR/submodules/<name>,
as that would make local submodule changes more resistant to dataloss due to
checkout/reset in the containing repository.

Shortlog:
 Add platform-independent .git "symlink"
 Fix setup of $GIT_DIR in git-sh-setup.sh
 Teach resolve_gitlink_ref() about the .git file
 git-submodule: prepare for the .git-file
 Teach GIT-VERSION-GEN about the .git file

Diffstat:
 Documentation/repository-layout.txt |    5 ++-
 GIT-VERSION-GEN                     |    2 +-
 cache.h                             |    1 +
 environment.c                       |   38 ++++++++++++++++++
 git-sh-setup.sh                     |   12 ++---
 git-submodule.sh                    |    4 +-
 refs.c                              |   17 +++++++-
 setup.c                             |    9 ++++
 t/t0002-gitfile.sh                  |   74 +++++++++++++++++++++++++++++++++++
 9 files changed, 148 insertions(+), 14 deletions(-)

When .git in a submodule is a file, resolve_gitlink_ref() needs to pick up
the real GIT_DIR of the submodule from that file.

Signed-off-by: Lars Hjemli <redacted>
---
 refs.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 67d2a50..56de5cf 100644
--- a/refs.c
+++ b/refs.c

@@ -351,6 +351,7 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *re
 {
 	int len = strlen(path), retval;
 	char *gitdir;
+	const char *tmp;
 
 	while (len && path[len-1] == '/')
 		len--;

@@ -358,9 +359,19 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *re
 		return -1;
 	gitdir = xmalloc(len + MAXREFLEN + 8);
 	memcpy(gitdir, path, len);
-	memcpy(gitdir + len, "/.git/", 7);
-
-	retval = resolve_gitlink_ref_recursive(gitdir, len+6, refname, result, 0);
+	memcpy(gitdir + len, "/.git", 6);
+	len += 5;
+
+	tmp = read_gitfile_gently(gitdir);
+	if (tmp) {
+		free(gitdir);
+		len = strlen(tmp);
+		gitdir = xmalloc(len + MAXREFLEN + 3);
+		memcpy(gitdir, tmp, len);
+	}
+	gitdir[len] = '/';
+	gitdir[++len] = '\0';
+	retval = resolve_gitlink_ref_recursive(gitdir, len, refname, result, 0);
 	free(gitdir);
 	return retval;
 }

-- 
1.5.4.1.188.gdfa6c

Signed-off-by: Lars Hjemli <redacted>
---
 GIT-VERSION-GEN |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 38a3273..2432a4f 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN

@@ -11,7 +11,7 @@ LF='
 if test -f version
 then
 	VN=$(cat version) || VN="$DEF_VER"
-elif test -d .git &&
+elif test -d .git -o -f .git &&
 	VN=$(git describe --abbrev=4 HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;

-- 
1.5.4.1.188.gdfa6c

This patch allows .git to be a regular textfile containing the path of
the real git directory (formatted like "gitdir: <path>\n"), which is
useful on platforms lacking support for real symlinks.

Signed-off-by: Lars Hjemli <redacted>
---
 Documentation/repository-layout.txt |    5 ++-
 cache.h                             |    1 +
 environment.c                       |   38 ++++++++++++++++++
 setup.c                             |    9 ++++
 t/t0002-gitfile.sh                  |   74 +++++++++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+), 1 deletions(-)
 create mode 100755 t/t0002-gitfile.sh

diff --git a/Documentation/repository-layout.txt b/Documentation/repository-layout.txt
index 6939130..e9db3a1 100644
--- a/Documentation/repository-layout.txt
+++ b/Documentation/repository-layout.txt

@@ -3,7 +3,10 @@ git repository layout
 
 You may find these things in your git repository (`.git`
 directory for a repository associated with your working tree, or
-`'project'.git` directory for a public 'bare' repository).
+`'project'.git` directory for a public 'bare' repository. It is
+also possible to have a working tree where `.git` is a plain
+ascii file containing `gitdir: <path>\n`, i.e. the path to the
+real git repository).
 
 objects::
 	Object store associated with this repository.  Usually

diff --git a/cache.h b/cache.h
index e1000bc..1ad822a 100644
--- a/cache.h
+++ b/cache.h

@@ -277,6 +277,7 @@ extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
 extern const char *get_git_work_tree(void);
+extern const char *read_gitfile_gently(const char *path);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 

diff --git a/environment.c b/environment.c
index 3527f16..d120e8f 100644
--- a/environment.c
+++ b/environment.c

@@ -45,10 +45,48 @@ static const char *work_tree;
 static const char *git_dir;
 static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
 
+/*
+ * Try to read the location of the git directory from the .git file,
+ * return path to git directory if found.
+ * Format of the .git file is
+ *    gitdir: <path>\n
+ */
+const char *read_gitfile_gently(const char *path)
+{
+	static char buf[PATH_MAX + 9];  /* "gitdir: " + "\n" */
+	struct stat st;
+	int fd;
+	size_t len;
+
+	if (stat(path, &st))
+		return NULL;
+	if (!S_ISREG(st.st_mode) || st.st_size >= sizeof(buf))
+		return NULL;
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+	len = read_in_full(fd, buf, sizeof(buf));
+	close(fd);
+	if (len != st.st_size)
+		return NULL;
+	if (!len || buf[len - 1] != '\n')
+		return NULL;
+	buf[len - 1] = '\0';
+	if (prefixcmp(buf, "gitdir: "))
+		return NULL;
+/*
+	if (!is_git_directory(buf + 8))
+		return NULL;
+*/
+	return make_absolute_path(buf + 8);
+}
+
 static void setup_git_env(void)
 {
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
 	if (!git_dir)
+		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	git_object_dir = getenv(DB_ENVIRONMENT);
 	if (!git_object_dir) {

diff --git a/setup.c b/setup.c
index 4509598..ebdf64b 100644
--- a/setup.c
+++ b/setup.c

@@ -247,6 +247,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
+	const char *gitfile_dir;
 	int len, offset;
 
 	/*

@@ -293,8 +294,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	/*
 	 * Test in the following order (relative to the cwd):
+	 * - .git (file containing "gitdir: <path>\n")
 	 * - .git/
 	 * - ./ (bare)
+	 * - ../.git (file containing "gitdir: <path>\n")
 	 * - ../.git/
 	 * - ../ (bare)
 	 * - ../../.git/

@@ -302,6 +305,12 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 */
 	offset = len = strlen(cwd);
 	for (;;) {
+		gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+		if (gitfile_dir && is_git_directory(gitfile_dir)) {
+			if (set_git_dir(gitfile_dir))
+				return NULL;
+			break;
+		}
 		if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
 			break;
 		if (is_git_directory(".")) {

diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
new file mode 100755
index 0000000..d280663
--- /dev/null
+++ b/t/t0002-gitfile.sh

@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='.git file
+
+Verify that plumbing commands work when .git is a file
+'
+. ./test-lib.sh
+
+objpath() {
+    echo "$1" | sed -e 's|\(..\)|\1/|'
+}
+
+objck() {
+	p=$(objpath "$1")
+	if test ! -f "$REAL/objects/$p"
+	then
+		echo "Object not found: $REAL/objects/$p"
+		false
+	fi
+}
+
+test_expect_success 'setup' '
+	REAL="$(pwd)/.real" &&
+	mv .git "$REAL" &&
+	echo "gitdir: $REAL" >.git
+'
+
+test_expect_success 'check rev-parse --git-dir' '
+	test "$REAL" = "$(git rev-parse --git-dir)"
+'
+
+test_expect_success 'check hash-object' '
+	echo "foo" >bar &&
+	SHA=$(cat bar | git hash-object -w --stdin) &&
+	objck $SHA
+'
+
+test_expect_success 'check cat-file' '
+	git cat-file blob $SHA >actual &&
+	diff -u bar actual
+'
+
+test_expect_success 'check update-index' '
+	if test -f "$REAL/index"
+	then
+		echo "Hmm, $REAL/index exists?"
+		false
+	fi &&
+	rm -f "$REAL/objects/$(objpath $SHA)" &&
+	git update-index --add bar &&
+	if ! test -f "$REAL/index"
+	then
+		echo "$REAL/index not found"
+		false
+	fi &&
+	objck $SHA
+'
+
+test_expect_success 'check write-tree' '
+	SHA=$(git write-tree) &&
+	objck $SHA
+'
+
+test_expect_success 'check commit-tree' '
+	SHA=$(echo "commit bar" | git commit-tree $SHA) &&
+	objck $SHA
+'
+
+test_expect_success 'check rev-list' '
+	echo $SHA >"$REAL/HEAD" &&
+	test "$SHA" = "$(git rev-list HEAD)"
+'
+
+test_done

-- 
1.5.4.1.188.gdfa6c

When git-submodule tries to detect 'active' submodules, it checks for the
existence of a directory named '.git'. This isn't good enough now that .git
can be a file pointing to the real $GIT_DIR so the tests are changed to
reflect this.

Signed-off-by: Lars Hjemli <redacted>
---
 git-submodule.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index a6aaf40..e7c08b5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -288,7 +288,7 @@ cmd_update()
 			continue
 		fi
 
-		if ! test -d "$path"/.git
+		if ! test -d "$path"/.git -o -f "$path"/.git
 		then
 			module_clone "$path" "$url" || exit
 			subsha1=

@@ -362,7 +362,7 @@ cmd_status()
 	do
 		name=$(module_name "$path") || exit
 		url=$(git config submodule."$name".url)
-		if test -z "url" || ! test -d "$path"/.git
+		if test -z "url" || ! test -d "$path"/.git -o -f "$path"/.git
 		then
 			say "-$sha1 $path"
 			continue;

-- 
1.5.4.1.188.gdfa6c

Since .git can be a file refering to the real GIT_DIR, git-sh-setup needs
to use 'git rev-parse --git-dir' to obtain the location of the git
repository.

Signed-off-by: Lars Hjemli <redacted>
---
 git-sh-setup.sh |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index f388275..a7dbce2 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh

@@ -127,20 +127,18 @@ get_author_ident_from_commit () {
 # if we require to be in a git repository.
 if test -z "$NONGIT_OK"
 then
+	GIT_DIR=$(git rev-parse --git-dir) || {
+		exit=$?
+		echo >&2 "Failed to find a valid git directory."
+		exit $exit
+	}
 	if [ -z "$SUBDIRECTORY_OK" ]
 	then
-		: ${GIT_DIR=.git}
 		test -z "$(git rev-parse --show-cdup)" || {
 			exit=$?
 			echo >&2 "You need to run this command from the toplevel of the working tree."
 			exit $exit
 		}
-	else
-		GIT_DIR=$(git rev-parse --git-dir) || {
-		    exit=$?
-		    echo >&2 "Failed to find a valid git directory."
-		    exit $exit
-		}
 	fi
 	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
 		echo >&2 "Unable to determine absolute path of git directory"

-- 
1.5.4.1.188.gdfa6c

Hi,

On Sun, 17 Feb 2008, Lars Hjemli wrote:

PPS: If included, the .git file should probably be used by git-submodule 
to clone submodule repositories into something like 
$GIT_DIR/submodules/<name>, as that would make local submodule changes 
more resistant to dataloss due to checkout/reset in the containing 
repository.

I don't buy that argument.  For the moment, the submodules are 
self-contained repositories.  The superproject does not even have to have 
a single object contained in a submodule.  I'd try to keep that 
separation.

As for data loss, again, as a submodule is self-contained, the same rules 
apply to it as for any repository.

Ciao,
Dscho

Hi,

On Sun, 17 Feb 2008, Lars Hjemli wrote:

Should this not complain loudly?  Really loudly, as in die()?

Ciao,
Dscho

On Feb 17, 2008 11:20 PM, Johannes Schindelin
[off-list ref] wrote:

Hi,

On Sun, 17 Feb 2008, Lars Hjemli wrote:

quoted

PPS: If included, the .git file should probably be used by git-submodule
to clone submodule repositories into something like
$GIT_DIR/submodules/<name>, as that would make local submodule changes
more resistant to dataloss due to checkout/reset in the containing
repository.

I don't buy that argument.  For the moment, the submodules are
self-contained repositories.  The superproject does not even have to have
a single object contained in a submodule.  I'd try to keep that
separation.

They would still be separated and self-contained repositories, but
with my suggestion the submodule repository would be moved out of the
working tree of the containing repository. Which I believe is a good
thing, especially when you switch from one branch in the containing
repository which had the submodule in './foo' to another branch where
the submodule is located in './lib/foo'.

--
larsh

On Feb 17, 2008 11:25 PM, Johannes Schindelin
[off-list ref] wrote:

On Sun, 17 Feb 2008, Lars Hjemli wrote:

quoted

@@ -302,6 +305,12 @@ const char *setup_git_directory_gently(int *nongit_ok)
       */
      offset = len = strlen(cwd);
      for (;;) {
+             gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+             if (gitfile_dir && is_git_directory(gitfile_dir)) {
+                     if (set_git_dir(gitfile_dir))
+                             return NULL;

Should this not complain loudly?  Really loudly, as in die()?

Yes, I'll resend. Thanks.

--
larsh

This patch allows .git to be a regular textfile containing the path of
the real git directory (formatted like "gitdir: <path>\n"), which is
useful on platforms lacking support for real symlinks.

Signed-off-by: Lars Hjemli <redacted>
---
 Documentation/repository-layout.txt |    5 ++-
 cache.h                             |    1 +
 environment.c                       |   38 ++++++++++++++++++
 setup.c                             |    9 ++++
 t/t0002-gitfile.sh                  |   74 +++++++++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+), 1 deletions(-)
 create mode 100755 t/t0002-gitfile.sh

diff --git a/Documentation/repository-layout.txt b/Documentation/repository-layout.txt
index 6939130..e9db3a1 100644
--- a/Documentation/repository-layout.txt
+++ b/Documentation/repository-layout.txt

@@ -3,7 +3,10 @@ git repository layout
 
 You may find these things in your git repository (`.git`
 directory for a repository associated with your working tree, or
-`'project'.git` directory for a public 'bare' repository).
+`'project'.git` directory for a public 'bare' repository. It is
+also possible to have a working tree where `.git` is a plain
+ascii file containing `gitdir: <path>\n`, i.e. the path to the
+real git repository).
 
 objects::
 	Object store associated with this repository.  Usually

diff --git a/cache.h b/cache.h
index e1000bc..1ad822a 100644
--- a/cache.h
+++ b/cache.h

@@ -277,6 +277,7 @@ extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
 extern const char *get_git_work_tree(void);
+extern const char *read_gitfile_gently(const char *path);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 

diff --git a/environment.c b/environment.c
index 3527f16..d120e8f 100644
--- a/environment.c
+++ b/environment.c

@@ -45,10 +45,48 @@ static const char *work_tree;
 static const char *git_dir;
 static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
 
+/*
+ * Try to read the location of the git directory from the .git file,
+ * return path to git directory if found.
+ * Format of the .git file is
+ *    gitdir: <path>\n
+ */
+const char *read_gitfile_gently(const char *path)
+{
+	static char buf[PATH_MAX + 9];  /* "gitdir: " + "\n" */
+	struct stat st;
+	int fd;
+	size_t len;
+
+	if (stat(path, &st))
+		return NULL;
+	if (!S_ISREG(st.st_mode) || st.st_size >= sizeof(buf))
+		return NULL;
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+	len = read_in_full(fd, buf, sizeof(buf));
+	close(fd);
+	if (len != st.st_size)
+		return NULL;
+	if (!len || buf[len - 1] != '\n')
+		return NULL;
+	buf[len - 1] = '\0';
+	if (prefixcmp(buf, "gitdir: "))
+		return NULL;
+/*
+	if (!is_git_directory(buf + 8))
+		return NULL;
+*/
+	return make_absolute_path(buf + 8);
+}
+
 static void setup_git_env(void)
 {
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
 	if (!git_dir)
+		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	git_object_dir = getenv(DB_ENVIRONMENT);
 	if (!git_object_dir) {

diff --git a/setup.c b/setup.c
index 4509598..f8ebaf5 100644
--- a/setup.c
+++ b/setup.c

@@ -247,6 +247,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
+	const char *gitfile_dir;
 	int len, offset;
 
 	/*

@@ -293,8 +294,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	/*
 	 * Test in the following order (relative to the cwd):
+	 * - .git (file containing "gitdir: <path>\n")
 	 * - .git/
 	 * - ./ (bare)
+	 * - ../.git (file containing "gitdir: <path>\n")
 	 * - ../.git/
 	 * - ../ (bare)
 	 * - ../../.git/

@@ -302,6 +305,12 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 */
 	offset = len = strlen(cwd);
 	for (;;) {
+		gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+		if (gitfile_dir && is_git_directory(gitfile_dir)) {
+			if (set_git_dir(gitfile_dir))
+				die("Repository setup failed");
+			break;
+		}
 		if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
 			break;
 		if (is_git_directory(".")) {

diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
new file mode 100755
index 0000000..d280663
--- /dev/null
+++ b/t/t0002-gitfile.sh

@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='.git file
+
+Verify that plumbing commands work when .git is a file
+'
+. ./test-lib.sh
+
+objpath() {
+    echo "$1" | sed -e 's|\(..\)|\1/|'
+}
+
+objck() {
+	p=$(objpath "$1")
+	if test ! -f "$REAL/objects/$p"
+	then
+		echo "Object not found: $REAL/objects/$p"
+		false
+	fi
+}
+
+test_expect_success 'setup' '
+	REAL="$(pwd)/.real" &&
+	mv .git "$REAL" &&
+	echo "gitdir: $REAL" >.git
+'
+
+test_expect_success 'check rev-parse --git-dir' '
+	test "$REAL" = "$(git rev-parse --git-dir)"
+'
+
+test_expect_success 'check hash-object' '
+	echo "foo" >bar &&
+	SHA=$(cat bar | git hash-object -w --stdin) &&
+	objck $SHA
+'
+
+test_expect_success 'check cat-file' '
+	git cat-file blob $SHA >actual &&
+	diff -u bar actual
+'
+
+test_expect_success 'check update-index' '
+	if test -f "$REAL/index"
+	then
+		echo "Hmm, $REAL/index exists?"
+		false
+	fi &&
+	rm -f "$REAL/objects/$(objpath $SHA)" &&
+	git update-index --add bar &&
+	if ! test -f "$REAL/index"
+	then
+		echo "$REAL/index not found"
+		false
+	fi &&
+	objck $SHA
+'
+
+test_expect_success 'check write-tree' '
+	SHA=$(git write-tree) &&
+	objck $SHA
+'
+
+test_expect_success 'check commit-tree' '
+	SHA=$(echo "commit bar" | git commit-tree $SHA) &&
+	objck $SHA
+'
+
+test_expect_success 'check rev-list' '
+	echo $SHA >"$REAL/HEAD" &&
+	test "$SHA" = "$(git rev-list HEAD)"
+'
+
+test_done

-- 
1.5.4.1.188.gfde78

Hi,

On Sun, 17 Feb 2008, Lars Hjemli wrote:

On Feb 17, 2008 11:20 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

On Sun, 17 Feb 2008, Lars Hjemli wrote:

quoted

PPS: If included, the .git file should probably be used by 
git-submodule to clone submodule repositories into something like 
$GIT_DIR/submodules/<name>, as that would make local submodule 
changes more resistant to dataloss due to checkout/reset in the 
containing repository.

I don't buy that argument.  For the moment, the submodules are 
self-contained repositories.  The superproject does not even have to 
have a single object contained in a submodule.  I'd try to keep that 
separation.

They would still be separated and self-contained repositories, but with 
my suggestion the submodule repository would be moved out of the working 
tree of the containing repository. Which I believe is a good thing, 
especially when you switch from one branch in the containing repository 
which had the submodule in './foo' to another branch where the submodule 
is located in './lib/foo'.

That is a good argument.

But what about the argument when you stop being interested in the 
superproject, and move the submodule out of it (then deleting the 
superproject)?

However, when a submodule is deleted, wasn't the plan all along to put the 
submodule's .git/ into the superproject's .git/submodules-deleted/ or some 
such?

Ciao,
Dscho

Lars Hjemli [off-list ref] writes:

This patch allows .git to be a regular textfile containing the path of
the real git directory (formatted like "gitdir: <path>\n"), which is
useful on platforms lacking support for real symlinks.

Hmmmm.  I have suspected all along that these "platforms lacking
support for real symlinks" are the ones whose native line
termination convention is CRLF.  How do you envision this file
is initialized?  By users editing the file by hand?  Or some git
specific tool?  If the former, it might make sense to define the
format as "a single line, terminated with platform line
terminator" and open and read it in text mode.  If the latter,
we do not have to care, and "terminated with LF" is good enough.

I think a sane simplification is to allow the file to have
any number of optional \r or \n at the end.  We may care about
allowing arbitrary and possibly crazy filenames in the tracked
contents, but we can say "Sorry, you cannot create a directory
'ab\nc\r' and use it as the .git directory substitute."

+const char *read_gitfile_gently(const char *path)
+{
+	static char buf[PATH_MAX + 9];  /* "gitdir: " + "\n" */
+	struct stat st;
+	int fd;
+	size_t len;
+
+	if (stat(path, &st))
+		return NULL;
+	if (!S_ISREG(st.st_mode) || st.st_size >= sizeof(buf))
+		return NULL;
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return NULL;

Up to this point it is fine "gently" semantics.

+	len = read_in_full(fd, buf, sizeof(buf));
+	close(fd);
+	if (len != st.st_size)
+		return NULL;
+	if (!len || buf[len - 1] != '\n')
+		return NULL;
+	buf[len - 1] = '\0';
+	if (prefixcmp(buf, "gitdir: "))
+		return NULL;

But I am not sure about this part.  We found what claims to be
the ".git" fake symlink but it is ill-formed.  Don't we want to
diagnose the possible breakage for the user?

+/*
+	if (!is_git_directory(buf + 8))
+		return NULL;
+*/

Likewise.

Lars Hjemli [off-list ref] writes:

Since .git can be a file refering to the real GIT_DIR, git-sh-setup needs
to use 'git rev-parse --git-dir' to obtain the location of the git
repository.

I wonder if this depend on your [1/5].  Isn't this actually a
simplification (removing 7 adding 5 lines) that applies to the
mainline already?  IOW, is there a downside of doing this
without any of the rest of the series?

rev-parse --git-dir would have said "fatal: Not a git
repository" already.  Do we still need to say "Failed to
find..."?

On Feb 18, 2008 6:43 AM, Junio C Hamano [off-list ref] wrote:

Lars Hjemli [off-list ref] writes:

quoted

This patch allows .git to be a regular textfile containing the path of
the real git directory (formatted like "gitdir: <path>\n"), which is
useful on platforms lacking support for real symlinks.

I think a sane simplification is to allow the file to have
any number of optional \r or \n at the end.

Agreed.

quoted

+     len = read_in_full(fd, buf, sizeof(buf));
+     close(fd);
+     if (len != st.st_size)
+             return NULL;
+     if (!len || buf[len - 1] != '\n')
+             return NULL;
+     buf[len - 1] = '\0';
+     if (prefixcmp(buf, "gitdir: "))
+             return NULL;

But I am not sure about this part.  We found what claims to be
the ".git" fake symlink but it is ill-formed.  Don't we want to
diagnose the possible breakage for the user?

Yes, I think I got to eager in my 'gentleness'. It's probably better
to die() with an appropriate errormessage.

quoted

+/*
+     if (!is_git_directory(buf + 8))
+             return NULL;
+*/

Likewise.

True, I'll uncomment and die().

Thanks for the review.

--
larsh

On Feb 18, 2008 6:44 AM, Junio C Hamano [off-list ref] wrote:

Lars Hjemli [off-list ref] writes:

quoted

Since .git can be a file refering to the real GIT_DIR, git-sh-setup needs
to use 'git rev-parse --git-dir' to obtain the location of the git
repository.

I wonder if this depend on your [1/5].  Isn't this actually a
simplification (removing 7 adding 5 lines) that applies to the
mainline already?

True. I'll resend as [1/5] with a fixed up commit message.

quoted

@@ -127,20 +127,18 @@ get_author_ident_from_commit () {
 # if we require to be in a git repository.
 if test -z "$NONGIT_OK"
 then
+     GIT_DIR=$(git rev-parse --git-dir) || {
+             exit=$?
+             echo >&2 "Failed to find a valid git directory."
+             exit $exit

rev-parse --git-dir would have said "fatal: Not a git
repository" already.  Do we still need to say "Failed to
find..."?

Absolutely not. Thanks.

-- 
larsh

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

On Feb 18, 2008 6:43 AM, Junio C Hamano [off-list ref] wrote:

quoted

Lars Hjemli [off-list ref] writes:

quoted

+/*
+     if (!is_git_directory(buf + 8))
+             return NULL;
+*/

Likewise.

True, I'll uncomment and die().

Hmm.  From check_repository_format_gently():

		if (!nongit_ok)
                        die ("...")
		warning("Expected git repo version <= %d...");

I think we want that, too.  (die() when !nongit_ok, but warn otherwise)

Ciao,
Dscho