[PATCH 0/5 v2] Introduce the .gitfile

[PATCH 0/5 v2] Introduce the .gitfile

[Lars Hjemli]

The patch-series has been reordered and updated according to comments on
the earlier series.

Shortlog:
 Simplify setup of $GIT_DIR in git-sh-setup.sh
 Add platform-independent .git "symlink"
 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                       |    2 +
 git-sh-setup.sh                     |    8 +---
 git-submodule.sh                    |    4 +-
 refs.c                              |   17 +++++++-
 setup.c                             |   47 ++++++++++++++++++++++
 t/t0002-gitfile.sh                  |   74 +++++++++++++++++++++++++++++++++++
 9 files changed, 146 insertions(+), 14 deletions(-)

[PATCH 1/5] Simplify setup of $GIT_DIR in git-sh-setup.sh

[Lars Hjemli]

Using 'git rev-parse --git-dir' makes the code shorter and more future-
proof.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 git-sh-setup.sh |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index f388275..a44b1c7 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -127,20 +127,14 @@ 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
 	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.g3ea1f5

[PATCH 2/5] Add platform-independent .git "symlink"

[Lars Hjemli]

This patch allows .git to be a regular textfile containing the path of
the real git directory (prefixed with "gitdir: "), which can be useful on
platforms lacking support for real symlinks.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 Documentation/repository-layout.txt |    5 ++-
 cache.h                             |    1 +
 environment.c                       |    2 +
 setup.c                             |   47 ++++++++++++++++++++++
 t/t0002-gitfile.sh                  |   74 +++++++++++++++++++++++++++++++++++
 5 files changed, 128 insertions(+), 1 deletions(-)
 create mode 100755 t/t0002-gitfile.sh

diff --git a/Documentation/repository-layout.txt b/Documentation/repository-layout.txt
index 6939130..bbaed2e 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>`, 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..8058e7b 100644
--- a/environment.c
+++ b/environment.c
@@ -49,6 +49,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..20502be 100644
--- a/setup.c
+++ b/setup.c
@@ -239,6 +239,44 @@ static int check_repository_format_gently(int *nongit_ok)
 }
 
 /*
+ * Try to read the location of the git directory from the .git file,
+ * return path to git directory if found.
+ */
+const char *read_gitfile_gently(const char *path)
+{
+	char *buf;
+	struct stat st;
+	int fd;
+	size_t len;
+
+	if (stat(path, &st))
+		return NULL;
+	if (!S_ISREG(st.st_mode))
+		return NULL;
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		die("Error opening %s: %s", path, strerror(errno));
+	buf = xmalloc(st.st_size + 1);
+	len = read_in_full(fd, buf, st.st_size);
+	close(fd);
+	if (len != st.st_size)
+		die("Error reading %s", path);
+	buf[len] = '\0';
+	if (prefixcmp(buf, "gitdir: "))
+		die("Invalid gitfile format: %s", path);
+	while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
+		len--;
+	if (len < 9)
+		die("No path in gitfile: %s", path);
+	buf[len] = '\0';
+	if (!is_git_directory(buf + 8))
+		die("Not a git repository: %s", buf + 8);
+	path = make_absolute_path(buf + 8);
+	free(buf);
+	return path;
+}
+
+/*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
  */
@@ -247,6 +285,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 +332,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	/*
 	 * Test in the following order (relative to the cwd):
+	 * - .git (file containing "gitdir: <path>")
 	 * - .git/
 	 * - ./ (bare)
+	 * - ../.git
 	 * - ../.git/
 	 * - ../ (bare)
 	 * - ../../.git/
@@ -302,6 +343,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) {
+			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.g3ea1f5

[PATCH 3/5] Teach resolve_gitlink_ref() about the .git file

[Lars Hjemli]

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 <hjemli@gmail.com>
---
 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.g3ea1f5

[PATCH 4/5] git-submodule: prepare for the .git-file

[Lars Hjemli]

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 <hjemli@gmail.com>
---
 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.g3ea1f5

[PATCH 5/5] Teach GIT-VERSION-GEN about the .git file

[Lars Hjemli]

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 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.g3ea1f5

Re: [PATCH 0/5 v2] Introduce the .gitfile

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

> The patch-series has been reordered and updated according to comments on 
> the earlier series.

It would be easier on a stupid reviewer like me, if you summarised what 
you changed.  In the case of patch "1/5 => 2/5", I would even have 
appreciated an interdiff...

Oh, and I was kind of surprised to learn that it is the ".gitfile", not 
the ".git file"... I ran to your patch straight away, and was relieved 
when I found out that you still call it ".git".

Ciao,
Dscho

Re: [PATCH 2/5] Add platform-independent .git "symlink"

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

> diff --git a/environment.c b/environment.c
> index 3527f16..8058e7b 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -49,6 +49,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;

I still maintain that the code (maybe not the diff) is easier to read like 
this:

  	if (!git_dir) {
		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
		if (!git_dir)
  			git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
	}

> diff --git a/setup.c b/setup.c
> index 4509598..20502be 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -239,6 +239,44 @@ static int check_repository_format_gently(int *nongit_ok)
>  }
>  
>  /*
> + * Try to read the location of the git directory from the .git file,
> + * return path to git directory if found.
> + */
> +const char *read_gitfile_gently(const char *path)
> +{
> +	char *buf;
> +	struct stat st;
> +	int fd;
> +	size_t len;
> +
> +	if (stat(path, &st))
> +		return NULL;
> +	if (!S_ISREG(st.st_mode))
> +		return NULL;
> +	fd = open(path, O_RDONLY);
> +	if (fd < 0)
> +		die("Error opening %s: %s", path, strerror(errno));

Hmm.  Like I said, in the "gently" case, we might want to just print a 
warning and return NULL.

However, since you have 5 die()s in this function that would clutter the 
code tremendously.  I briefly considered (shut your eyes now if you do 
not like ugly code):

	int (*show_error)(const char *format, ...) = nongit_ok ?
		error : (int (*)(const char *format, ...))die;

but now I think a better method would be

static int show_error(int die_on_error, const char *format, ...)
{
        va_list params;

        va_start(params, err);
	if (die_on_error)
		die_routine(err, params);
	else
	        error_routine(err, params);
        va_end(params);
        return -1;
}

This would even be a candidate for a global function die_or_error().

Then you could use it like this:

	if (fd < 0 && die("Error opening %s: %s", path, strerror(errno))
		return NULL;

Hmm.  Seeing what I wrote, it does not really feel elegant.

So maybe we can just scratch all that, and I agree that an invalid .git 
file means "no repository" (as opposed to "no valid repository").

In that case, you might want to test for that, too...

Speaking about tests:

> +test_expect_success 'setup' '
> +       REAL="$(pwd)/.real" &&
> +       mv .git "$REAL" &&
> +       echo "gitdir: $REAL" >.git
> +'

Let's not do this.  It would clutter the t/ directory unnecessarily.  
Instead, do something like this:

test_expect_success setup '

	REAL="$(pwd)/.real" &&
	mkdir test &&
	cd test &&
	echo "gitdir: $REAL" > .git

'

Hmm?

Ciao,
Dscho "who likes to write 'Hmm' three times in a mail"

Re: [PATCH 0/5 v2] Introduce the .gitfile

[Lars Hjemli]

On Feb 18, 2008 1:17 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Mon, 18 Feb 2008, Lars Hjemli wrote:
>
> > The patch-series has been reordered and updated according to comments on
> > the earlier series.
>
> It would be easier on a stupid reviewer like me, if you summarised what
> you changed.

Ok, I'll keep that in mind next time, and here's a quick summary (from
the top of my head) of this series vs. the previous:
* patch 1 and 2 are swapped, since the changes to git-sh-setup.sh can
be applied onto master regardless of the rest of the patch series
(while the other patches still rely on 1/5).
* git-sh-setup.sh is simplified even further, and relies on
git-rev-parse to produce the error message when no gitdir can be
found.
* read_gitfile_gently() is moved from environment.c into setup.c since
it now invokes is_git_directory().
* read_gitfile_gently() will gently ignore a 'missing' .git or a .git
which isn't a regular file, but die()s on all other errors.


> In the case of patch "1/5 => 2/5", I would even have
> appreciated an interdiff...

Sorry, but I don't think I understand what you mean by interdiff.


>
> Oh, and I was kind of surprised to learn that it is the ".gitfile", not
> the ".git file"... I ran to your patch straight away, and was relieved
> when I found out that you still call it ".git".

Yeah, I suck really bad on consistency ;-)


--
lh

Re: [PATCH 2/5] Add platform-independent .git "symlink"

[Lars Hjemli]

On Feb 18, 2008 1:34 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 18 Feb 2008, Lars Hjemli wrote:
>
> > diff --git a/environment.c b/environment.c
> > index 3527f16..8058e7b 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -49,6 +49,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;
>
> I still maintain that the code (maybe not the diff) is easier to read like
> this:
>
>         if (!git_dir) {
>                 git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
>                 if (!git_dir)
>                         git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
>         }

IMHO such constructs are butt ugly, but if there's consensus for your
way, I'll abide...


> [* snip *]
>
> So maybe we can just scratch all that, and I agree that an invalid .git
> file means "no repository" (as opposed to "no valid repository").
>
> In that case, you might want to test for that, too...

I think I do, did you find a loophole in the testing?


>
> Speaking about tests:
>
> > +test_expect_success 'setup' '
> > +       REAL="$(pwd)/.real" &&
> > +       mv .git "$REAL" &&
> > +       echo "gitdir: $REAL" >.git
> > +'
>
> Let's not do this.  It would clutter the t/ directory unnecessarily.

What do you mean? The test just moves t/trash/.git to t/trash/.real...

--
larsh

Re: [PATCH 0/5 v2] Introduce the .gitfile

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

> On Feb 18, 2008 1:17 PM, Johannes Schindelin 
> <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 18 Feb 2008, Lars Hjemli wrote:
> >
> > > The patch-series has been reordered and updated according to 
> > > comments on the earlier series.
> >
> > It would be easier on a stupid reviewer like me, if you summarised 
> > what you changed.
> 
> Ok, I'll keep that in mind next time, and here's a quick summary (from
> the top of my head) of this series vs. the previous:
> * patch 1 and 2 are swapped, since the changes to git-sh-setup.sh can
> be applied onto master regardless of the rest of the patch series
> (while the other patches still rely on 1/5).
> * git-sh-setup.sh is simplified even further, and relies on
> git-rev-parse to produce the error message when no gitdir can be
> found.
> * read_gitfile_gently() is moved from environment.c into setup.c since
> it now invokes is_git_directory().
> * read_gitfile_gently() will gently ignore a 'missing' .git or a .git
> which isn't a regular file, but die()s on all other errors.

Thanks.

> > In the case of patch "1/5 => 2/5", I would even have appreciated an 
> > interdiff...
> 
> Sorry, but I don't think I understand what you mean by interdiff.

The tool interdiff of patchutils is really nice: you can visualise what 
would be the diff between the state after applying the first patch, and 
the state after applying the second patch, without applying anything at 
all:

	$ interdiff <patch1> <patch2>

Ciao,
Dscho

Re: [PATCH 2/5] Add platform-independent .git "symlink"

[Jakub Narebski]

Lars Hjemli wrote:
> On Feb 18, 2008 1:34 PM, Johannes Schindelin wrote:
>> On Mon, 18 Feb 2008, Lars Hjemli wrote:
>>
>> > diff --git a/environment.c b/environment.c
>> > index 3527f16..8058e7b 100644
>> > --- a/environment.c
>> > +++ b/environment.c
>> > @@ -49,6 +49,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;
>>
>> I still maintain that the code (maybe not the diff) is easier to read like
>> this:
>>
>>         if (!git_dir) {
>>                 git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
>>                 if (!git_dir)
>>                         git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
>>         }
> 
> IMHO such constructs are butt ugly, but if there's consensus for your
> way, I'll abide...

I prefer the original variant. It is IMHO no less, and perhaps even
more readable, and it is not soo deeply nested.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 2/5] Add platform-independent .git "symlink"

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

> On Feb 18, 2008 1:34 PM, Johannes Schindelin 
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > [* snip *]
> >
> > So maybe we can just scratch all that, and I agree that an invalid 
> > .git file means "no repository" (as opposed to "no valid repository").
> >
> > In that case, you might want to test for that, too...
> 
> I think I do, did you find a loophole in the testing?

I meant writing a readable, but invalid .git file.  Then testing that Git 
die()s with the correct message.

> > Speaking about tests:
> >
> > > +test_expect_success 'setup' '
> > > +       REAL="$(pwd)/.real" &&
> > > +       mv .git "$REAL" &&
> > > +       echo "gitdir: $REAL" >.git
> > > +'
> >
> > Let's not do this.  It would clutter the t/ directory unnecessarily.
> 
> What do you mean? The test just moves t/trash/.git to t/trash/.real...

Oh, well.  I'll go and buy me some new glasses.

Thanks,
Dscho

Re: [PATCH 0/5 v2] Introduce the .gitfile

[Lars Hjemli]

On Feb 18, 2008 2:31 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 18 Feb 2008, Lars Hjemli wrote:
>
> > On Feb 18, 2008 1:17 PM, Johannes Schindelin wrote:
> > > In the case of patch "1/5 => 2/5", I would even have appreciated an
> > > interdiff...
> >
> > Sorry, but I don't think I understand what you mean by interdiff.
>
> The tool interdiff of patchutils is really nice: you can visualise what
> would be the diff between the state after applying the first patch, and
> the state after applying the second patch, without applying anything at
> all:
>
>         $ interdiff <patch1> <patch2>

Ok, that sounds useful (I was kind of confused since 'man interdiff'
gave me nothing: being on slackware I'm so spoiled with preinstalled
dev-tools that I see no point in consulting google ;).

Something like this (possibly mangled by gmail)?

$ interdiff prev-patch-2 curr-patch-1
diff -u b/git-sh-setup.sh b/git-sh-setup.sh
--- b/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -127,11 +127,7 @@
 # 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
-       }
+       GIT_DIR=$(git rev-parse --git-dir) || exit
        if [ -z "$SUBDIRECTORY_OK" ]
        then
                test -z "$(git rev-parse --show-cdup)" || {

$ interdiff prev-patch-1 curr-patch-2
diff -u b/Documentation/repository-layout.txt
b/Documentation/repository-layout.txt
--- b/Documentation/repository-layout.txt
+++ b/Documentation/repository-layout.txt
@@ -5,7 +5,7 @@
 directory for a repository associated with your working tree, or
 `'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
+ascii file containing `gitdir: <path>`, i.e. the path to the
 real git repository).

 objects::
diff -u b/environment.c b/environment.c
--- b/environment.c
+++ b/environment.c
@@ -45,42 +45,6 @@
 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);
diff -u b/setup.c b/setup.c
--- b/setup.c
+++ b/setup.c
@@ -239,6 +239,44 @@
 }

 /*
+ * Try to read the location of the git directory from the .git file,
+ * return path to git directory if found.
+ */
+const char *read_gitfile_gently(const char *path)
+{
+       char *buf;
+       struct stat st;
+       int fd;
+       size_t len;
+
+       if (stat(path, &st))
+               return NULL;
+       if (!S_ISREG(st.st_mode))
+               return NULL;
+       fd = open(path, O_RDONLY);
+       if (fd < 0)
+               die("Error opening %s: %s", path, strerror(errno));
+       buf = xmalloc(st.st_size + 1);
+       len = read_in_full(fd, buf, st.st_size);
+       close(fd);
+       if (len != st.st_size)
+               die("Error reading %s", path);
+       buf[len] = '\0';
+       if (prefixcmp(buf, "gitdir: "))
+               die("Invalid gitfile format: %s", path);
+       while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
+               len--;
+       if (len < 9)
+               die("No path in gitfile: %s", path);
+       buf[len] = '\0';
+       if (!is_git_directory(buf + 8))
+               die("Not a git repository: %s", buf + 8);
+       path = make_absolute_path(buf + 8);
+       free(buf);
+       return path;
+}
+
+/*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
  */
@@ -294,10 +332,10 @@

        /*
         * Test in the following order (relative to the cwd):
-        * - .git (file containing "gitdir: <path>\n")
+        * - .git (file containing "gitdir: <path>")
         * - .git/
         * - ./ (bare)
-        * - ../.git (file containing "gitdir: <path>\n")
+        * - ../.git
         * - ../.git/
         * - ../ (bare)
         * - ../../.git/
@@ -306,9 +344,9 @@
        offset = len = strlen(cwd);
        for (;;) {
                gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
-               if (gitfile_dir && is_git_directory(gitfile_dir)) {
+               if (gitfile_dir) {
                        if (set_git_dir(gitfile_dir))
-                               return NULL;
+                               die("Repository setup failed");
                        break;
                }
                if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))


--
larsh

Re: [PATCH 2/5] Add platform-independent .git "symlink"

[Lars Hjemli]

On Feb 18, 2008 2:35 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 18 Feb 2008, Lars Hjemli wrote:
>
> > On Feb 18, 2008 1:34 PM, Johannes Schindelin wrote:
> > >
> > > [* snip *]
> > >
> > > So maybe we can just scratch all that, and I agree that an invalid
> > > .git file means "no repository" (as opposed to "no valid repository").
> > >
> > > In that case, you might want to test for that, too...
> >
> > I think I do, did you find a loophole in the testing?
>
> I meant writing a readable, but invalid .git file.  Then testing that Git
> die()s with the correct message.

Ah, sorry for being slow, that's an excellent suggestion. I'll update
the test-script and resend 2/5.

--
larsh

Re: [PATCH 0/5 v2] Introduce the .gitfile

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

> On Feb 18, 2008 2:31 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > On Mon, 18 Feb 2008, Lars Hjemli wrote:
> >
> > > On Feb 18, 2008 1:17 PM, Johannes Schindelin wrote:
> > > > In the case of patch "1/5 => 2/5", I would even have appreciated 
> > > > an interdiff...
> > >
> > > Sorry, but I don't think I understand what you mean by interdiff.
> >
> > The tool interdiff of patchutils is really nice: you can visualise 
> > what would be the diff between the state after applying the first 
> > patch, and the state after applying the second patch, without applying 
> > anything at all:
> >
> >         $ interdiff <patch1> <patch2>
> 
> Ok, that sounds useful (I was kind of confused since 'man interdiff' 
> gave me nothing: being on slackware I'm so spoiled with preinstalled 
> dev-tools that I see no point in consulting google ;).
> 
> Something like this (possibly mangled by gmail)?

Heh, thank you!  The mangling does not matter, since this is purely for 
reviewing pleasure.

Thanks,
Dscho

[PATCH 2/5] Add platform-independent .git "symlink"

[Lars Hjemli]

This patch allows .git to be a regular textfile containing the path of
the real git directory (prefixed with "gitdir: "), which can be useful on
platforms lacking support for real symlinks.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---

On Feb 18, 2008 2:35 PM, Johannes Schindelin wrote:
> I meant writing a readable, but invalid .git file.  Then testing that Git
> die()s with the correct message.

Something like this?


 Documentation/repository-layout.txt |    5 ++-
 cache.h                             |    1 +
 environment.c                       |    2 +
 setup.c                             |   47 ++++++++++++++++
 t/t0002-gitfile.sh                  |  103 +++++++++++++++++++++++++++++++++++
 5 files changed, 157 insertions(+), 1 deletions(-)
 create mode 100755 t/t0002-gitfile.sh

diff --git a/Documentation/repository-layout.txt b/Documentation/repository-layout.txt
index 6939130..bbaed2e 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>`, 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..8058e7b 100644
--- a/environment.c
+++ b/environment.c
@@ -49,6 +49,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..20502be 100644
--- a/setup.c
+++ b/setup.c
@@ -239,6 +239,44 @@ static int check_repository_format_gently(int *nongit_ok)
 }
 
 /*
+ * Try to read the location of the git directory from the .git file,
+ * return path to git directory if found.
+ */
+const char *read_gitfile_gently(const char *path)
+{
+	char *buf;
+	struct stat st;
+	int fd;
+	size_t len;
+
+	if (stat(path, &st))
+		return NULL;
+	if (!S_ISREG(st.st_mode))
+		return NULL;
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		die("Error opening %s: %s", path, strerror(errno));
+	buf = xmalloc(st.st_size + 1);
+	len = read_in_full(fd, buf, st.st_size);
+	close(fd);
+	if (len != st.st_size)
+		die("Error reading %s", path);
+	buf[len] = '\0';
+	if (prefixcmp(buf, "gitdir: "))
+		die("Invalid gitfile format: %s", path);
+	while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
+		len--;
+	if (len < 9)
+		die("No path in gitfile: %s", path);
+	buf[len] = '\0';
+	if (!is_git_directory(buf + 8))
+		die("Not a git repository: %s", buf + 8);
+	path = make_absolute_path(buf + 8);
+	free(buf);
+	return path;
+}
+
+/*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
  */
@@ -247,6 +285,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 +332,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	/*
 	 * Test in the following order (relative to the cwd):
+	 * - .git (file containing "gitdir: <path>")
 	 * - .git/
 	 * - ./ (bare)
+	 * - ../.git
 	 * - ../.git/
 	 * - ../ (bare)
 	 * - ../../.git/
@@ -302,6 +343,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) {
+			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..c5dbc72
--- /dev/null
+++ b/t/t0002-gitfile.sh
@@ -0,0 +1,103 @@
+#!/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 'initial setup' '
+	REAL="$(pwd)/.real" &&
+	mv .git "$REAL"
+'
+
+test_expect_success 'bad setup: invalid .git file format' '
+	echo "gitdir $REAL" >.git &&
+	if git rev-parse 2>.err
+	then
+		echo "git rev-parse accepted an invalid .git file"
+		false
+	fi &&
+	if ! grep -qe "Invalid gitfile format" .err
+	then
+		echo "git rev-parse returned wrong error"
+		false
+	fi
+'
+
+test_expect_success 'bad setup: invalid .git file path' '
+	echo "gitdir: $REAL.not" >.git &&
+	if git rev-parse 2>.err
+	then
+		echo "git rev-parse accepted an invalid .git file path"
+		false
+	fi &&
+	if ! grep -qe "Not a git repository" .err
+	then
+		echo "git rev-parse returned wrong error"
+		false
+	fi
+'
+
+test_expect_success 'final setup + check rev-parse --git-dir' '
+	echo "gitdir: $REAL" >.git &&
+	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.g3ea1f5

Re: [PATCH 2/5] Add platform-independent .git "symlink"

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

> On Feb 18, 2008 2:35 PM, Johannes Schindelin wrote:
> > I meant writing a readable, but invalid .git file.  Then testing that 
> > Git die()s with the correct message.
> 
> Something like this?

Sure.

Ciao,
Dscho