"git-checkout-cache -f -a" failure

"git-checkout-cache -f -a" failure

[Morten Welinder]

git-checkout-cache is having problems when files change from directories to
plain files or vice versa.  cg-seek seems to be similarly affected.

Note also, that  git-checkout-cache's error messages haven't caught up with
the command renaming.

Morten


cd /tmp
mkdir xxx
cd xxx
rm -rf .git empty yyy 
touch empty
cg-init </dev/null
cg-tag initial
mkdir yyy
touch yyy/zzz
cg-add yyy/zzz 
cg-commit </dev/null
cg-tag dir
rm -rf empty yyy
rm -f .git/HEAD
cat .git/refs/tags/initial > .git/HEAD 
git-read-tree -m HEAD
git-checkout-cache -f -a
touch yyy
cg-add yyy
cg-commit </dev/null 
cg-tag file
# Got that?

# yyy is a file right now.
git-read-tree `cat .git/refs/tags/dir`
git-checkout-cache -f -a
# error: checkout-cache: unable to create file yyy/zzz (Not a directory)
# Not good!

rm -rf empty yyy
git-read-tree `cat .git/refs/tags/dir`
git-checkout-cache -f -a

# yyy is a directory right now
git-read-tree `cat .git/refs/tags/file`
git-checkout-cache -f -a
# error: checkout-cache: unable to create file yyy (Is a directory)
# Not good!

Re: "git-checkout-cache -f -a" failure

[Junio C Hamano]

>>>>> "MW" == Morten Welinder <mwelinder@gmail.com> writes:

MW> git-checkout-cache is having problems when files change from directories to
MW> plain files or vice versa.

Changing files vs directories _is_ a big change and happens
rarely in practice; I think the current behaviour is
justified---it makes the user take notice and the user _should_
take notice.

Porcelain layer, if it wanted to, should be able to hide this
from the user, but it depends on which Porcelain layer command
is involved.  My understanding of cg-seek is to switch the work
tree to a completely different state, so in that case it
probably should hide this (still it makes me feel a bit nervous
to realize that I am advocating for the tool to silently remove
a whole subtree to make room for a file, though).

Re: "git-checkout-cache -f -a" failure

[Morten Welinder]

> Changing files vs directories _is_ a big change and happens
> rarely in practice; I think the current behaviour is
> justified---it makes the user take notice and the user _should_
> take notice.

File vs. directory was just the easiest way to demonstrate.  In the presense
of symlinks I am not sure you will always get a warning.  It'll be more of a
silent file-corrupting failure kind of thing.  (Somewhat worse if yyy points
to /your/home/.ssh and zzz is "authorized_keys".)

Morten

Re: "git-checkout-cache -f -a" failure

[Junio C Hamano]

>>>>> "MW" == Morten Welinder <mwelinder@gmail.com> writes:

MW> git-checkout-cache is having problems when files change from directories to
MW> plain files or vice versa.  cg-seek seems to be similarly affected.

Could you give this patch a try?  It lets checkout-cache remove
a file where you want to have a directory (because you want to
create something underneath) or remove a whole subdirectory
where you want to have a non-directory, when '-f' parameter is
specified.

If things test well, I'll put this in the git-jc repository and
ask Linus to pull from it alongside with other accumulated
patches when he returns.

Thanks.


Signed-off-by: Junio C Hamano <junkio@cox.net>
------------
# - HEAD: Adjust quoting styles for some environment variables in the documentation.
# + 7: Let checkout-cache stomp on existing file/directory with -f.
--- a/checkout-cache.c
+++ b/checkout-cache.c
@@ -32,6 +32,8 @@
  * of "-a" causing problems (not possible in the above example,
  * but get used to it in scripting!).
  */
+#include <sys/types.h>
+#include <dirent.h>
 #include "cache.h"
 
 static int force = 0, quiet = 0, not_new = 0;
@@ -46,11 +48,51 @@ static void create_directories(const cha
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
-		mkdir(buf, 0755);
+		if (mkdir(buf, 0755)) {
+			if (errno == EEXIST) {
+				struct stat st;
+				if (!lstat(buf, &st) && S_ISDIR(st.st_mode))
+					continue; /* ok */
+				if (force && !unlink(buf) && !mkdir(buf, 0755))
+					continue;
+			}
+			die("cannot create directory at %s", buf);
+		}
 	}
 	free(buf);
 }
 
+static void remove_subtree(const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *de;
+	char pathbuf[PATH_MAX];
+	char *name;
+	
+	if (!dir)
+		die("cannot opendir %s", path);
+	strcpy(pathbuf, path);
+	name = pathbuf + strlen(path);
+	*name++ = '/';
+	while ((de = readdir(dir)) != NULL) {
+		struct stat st;
+		if (de->d_name[0] == '.')
+			continue; /* we mean . and .. but verify_path would
+				   * have rejected any dotfiles earlier.
+				   */
+		strcpy(name, de->d_name);
+		if (lstat(pathbuf, &st))
+			die("cannot lstat %s", pathbuf);
+		if (S_ISDIR(st.st_mode))
+			remove_subtree(pathbuf);
+		else if (unlink(pathbuf))
+			die("cannot unlink %s", pathbuf);
+	}
+	closedir(dir);
+	if (rmdir(path))
+		die("cannot rmdir %s", path);
+}
+
 static int create_file(const char *path, unsigned int mode)
 {
 	int fd;
@@ -62,6 +104,14 @@ static int create_file(const char *path,
 			create_directories(path);
 			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 		}
+		else if (errno == EISDIR && force) {
+			remove_subtree(path);
+			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
+		}
+		else if (errno == ENOTDIR && force) {
+			create_directories(path);
+			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
+		}
 	}
 	return fd;
 }

Re: "git-checkout-cache -f -a" failure

[Petr Baudis]

Dear diary, on Wed, May 11, 2005 at 12:57:34AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> >>>>> "MW" == Morten Welinder <mwelinder@gmail.com> writes:
> 
> MW> git-checkout-cache is having problems when files change from directories to
> MW> plain files or vice versa.  cg-seek seems to be similarly affected.
> 
> Could you give this patch a try?  It lets checkout-cache remove
> a file where you want to have a directory (because you want to
> create something underneath) or remove a whole subdirectory
> where you want to have a non-directory, when '-f' parameter is
> specified.
> 
> If things test well, I'll put this in the git-jc repository and
> ask Linus to pull from it alongside with other accumulated
> patches when he returns.

Chilling.

What if you have some files not tracked by git in the subdirectory?
Either you need to check for this and deal with it (Cogito's approach
would be to remove the git-tracked files and rename the subdirectory),
or not do it at all. Or at least please use -F or something if you
really want this and like to gamble.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: "git-checkout-cache -f -a" failure

[Junio C Hamano]

Please forget about the previous one which was buggy when you
had ".something" file under the directory being replaced.

Could you give this patch a try?

---

Running git-checkout-cache -f -a to check out a file in a
directory fails when the work tree has a non-directory with the
same name as the directory being checked out.  Similarly it
fails to check out a file when the work tree has a directory
with the same name as the file being checked out.

When '-f' is specified, the user is telling us that "I really
want to match the work tree with what is in the cache."  So
removing the offending file or directory from the work tree to
make room when necessary and possible in order to make the
checkout succeed is the right thing to do.

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

--- a/checkout-cache.c
+++ b/checkout-cache.c
@@ -32,6 +32,8 @@
  * of "-a" causing problems (not possible in the above example,
  * but get used to it in scripting!).
  */
+#include <sys/types.h>
+#include <dirent.h>
 #include "cache.h"
 
 static int force = 0, quiet = 0, not_new = 0;
@@ -46,11 +48,51 @@ static void create_directories(const cha
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
-		mkdir(buf, 0755);
+		if (mkdir(buf, 0755)) {
+			if (errno == EEXIST) {
+				struct stat st;
+				if (!lstat(buf, &st) && S_ISDIR(st.st_mode))
+					continue; /* ok */
+				if (force && !unlink(buf) && !mkdir(buf, 0755))
+					continue;
+			}
+			die("cannot create directory at %s", buf);
+		}
 	}
 	free(buf);
 }
 
+static void remove_subtree(const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *de;
+	char pathbuf[PATH_MAX];
+	char *name;
+	
+	if (!dir)
+		die("cannot opendir %s", path);
+	strcpy(pathbuf, path);
+	name = pathbuf + strlen(path);
+	*name++ = '/';
+	while ((de = readdir(dir)) != NULL) {
+		struct stat st;
+		if ((de->d_name[0] == '.') &&
+		    ((de->d_name[1] == 0) ||
+		     (de->d_name[1] == '.') && de->d_name[2] == 0))
+			continue;
+		strcpy(name, de->d_name);
+		if (lstat(pathbuf, &st))
+			die("cannot lstat %s", pathbuf);
+		if (S_ISDIR(st.st_mode))
+			remove_subtree(pathbuf);
+		else if (unlink(pathbuf))
+			die("cannot unlink %s", pathbuf);
+	}
+	closedir(dir);
+	if (rmdir(path))
+		die("cannot rmdir %s", path);
+}
+
 static int create_file(const char *path, unsigned int mode)
 {
 	int fd;
@@ -58,10 +100,14 @@ static int create_file(const char *path,
 	mode = (mode & 0100) ? 0777 : 0666;
 	fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 	if (fd < 0) {
-		if (errno == ENOENT) {
+		if ((errno == ENOENT) || (errno == ENOTDIR && force)) {
 			create_directories(path);
 			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 		}
+		else if (errno == EISDIR && force) {
+			remove_subtree(path);
+			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
+		}
 	}
 	return fd;
 }
------------------------------------------------

Re: "git-checkout-cache -f -a" failure

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> What if you have some files not tracked by git in the subdirectory?
PB> Either you need to check for this and deal with it (Cogito's approach
PB> would be to remove the git-tracked files and rename the subdirectory),
PB> or not do it at all.

Let me understand you correctly.  You "rename the subdirectory"
because the cache you want to check out records a non-directory
there.  Is this correct?  When do you "remove the git-tracked
files"?  Do you mean "if a file that is not tracked (in the
user's mind) is there in the filesystem, and the cache to be
checked out has a file there, the file on the filesystem is
removed"?  What happens if an existing non-directory interferes
with a directory to be checked out from the cache?

I think the right thing for the core GIT layer to do, when the
end user is using the Plumbing tools directly and says "really
please" by specifying an '-f', is to make the checkout succeed
by removing conflicting stuff, as my fixed patch does.

If the Porcelain runs checkout-cache on the user's behalf, the
story is a bit different.  Porcelain would have some idea of
what GIT tree the files in the working directory originated from
("the last commit", or "my head before the read-tree -m merge")
before the user started working in it.  When the user wants to
update the work tree to match a cache (which may be quite
different from "the last commit", if it is coming from cg-seek
or result of a merge), the Porcelain knows:

 - the set of non git-tracked (from the point of view from "the
   last commit") files that are in the work tree;

 - among those, the set of files that would be overwritten by
   this checkout.  This includes, but not limited to, the files
   that would otherwise interfere because of file-directory
   conflicts.

And then you can set aside those "to-be-overwritten" files,
remove them from the filesystem, and run your checkout.

The determination of which files to set aside should not be done
based on whether they cause file-directory conflicts.  If the
user has a non-tracked file which happens to be registered as a
tracked file in the cache to be checked out (e.g. after a
three-way merge), that file is as valuable as a non-tracked file
that is in a subdirectory that the new cache happens to have as
a non-directory.  Losing the former is as harmful as losing the
latter.  From what you say above, it sounds like you are saving
the latter but not the former, but my readins of what you wrote
above may be mistaken.

Re: "git-checkout-cache -f -a" failure

[Morten Welinder]

Here's the symlink version.  Note, that git does not complain but
simply creates (or
overwrites) the wrong file.

Morten


cd /tmp
mkdir xxx
cd xxx
rm -rf .git dir yyy
mkdir dir
touch dir/empty
cg-init </dev/null
cg-tag initial
mkdir yyy
touch yyy/zzz
cg-add yyy/zzz
cg-commit </dev/null
cg-tag dir
rm -rf yyy
rm -f .git/HEAD
cat .git/refs/tags/initial > .git/HEAD
git-read-tree -m HEAD
git-checkout-cache -f -a
ln -s dir yyy
git-update-cache --add -- yyy
cg-commit </dev/null
cg-tag symlink
# Got that?

# yyy is a symlink right now.
git-read-tree `cat .git/refs/tags/dir`
git-checkout-cache -f -a
ls -l dir
total 0
-rw-rw-r--    1 welinder research        0 May 11 14:20 empty
-rw-rw-r--    1 welinder research        0 May 11 14:26 zzz
# dir/zzz should not exist at ths point!

Re: "git-checkout-cache -f -a" failure

[Junio C Hamano]

>>>>> "MW" == Morten Welinder <mwelinder@gmail.com> writes:

MW> Here's the symlink version.  Note, that git does not complain but
MW> simply creates (or
MW> overwrites) the wrong file.

MW> Morten

Here is a proposed fix.

------------
Commit    685c391c9a755936175193f8b58973b74eaef930
Author    Junio C Hamano <junkio@cox.net>, Wed May 11 14:36:05 2005 -0700
Committer Junio C Hamano <junkio@cox.net>, Wed May 11 14:36:05 2005 -0700

Fix checkout-cache when existing work tree interferes with the checkout.

The checkout-cache command gets confused when checking out a
file in a subdirectory and the work tree has a symlink to the
subdirectory.  Also it fails to check things out when there is a
non-directory in the work tree when cache expects a directory
there, and vice versa.  This patch fixes the first problem by
making sure all the leading paths in the file being checked out
are indeed directories, and also fixes directory vs
non-directory conflicts when '-f' is specified by removing the
offending paths.

I've added test subdirectory and two tests to check the above
problems are fixed.

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

--- a/checkout-cache.c
+++ b/checkout-cache.c
@@ -32,6 +32,8 @@
  * of "-a" causing problems (not possible in the above example,
  * but get used to it in scripting!).
  */
+#include <sys/types.h>
+#include <dirent.h>
 #include "cache.h"
 
 static int force = 0, quiet = 0, not_new = 0;
@@ -46,22 +48,67 @@ static void create_directories(const cha
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
-		mkdir(buf, 0755);
+		if (mkdir(buf, 0755)) {
+			if (errno == EEXIST) {
+				struct stat st;
+				if (!lstat(buf, &st) && S_ISDIR(st.st_mode))
+					continue; /* ok */
+				if (force && !unlink(buf) && !mkdir(buf, 0755))
+					continue;
+			}
+			die("cannot create directory at %s", buf);
+		}
 	}
 	free(buf);
 }
 
+static void remove_subtree(const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *de;
+	char pathbuf[PATH_MAX];
+	char *name;
+	
+	if (!dir)
+		die("cannot opendir %s", path);
+	strcpy(pathbuf, path);
+	name = pathbuf + strlen(path);
+	*name++ = '/';
+	while ((de = readdir(dir)) != NULL) {
+		struct stat st;
+		if ((de->d_name[0] == '.') &&
+		    ((de->d_name[1] == 0) ||
+		     ((de->d_name[1] == '.') && de->d_name[2] == 0)))
+			continue;
+		strcpy(name, de->d_name);
+		if (lstat(pathbuf, &st))
+			die("cannot lstat %s", pathbuf);
+		if (S_ISDIR(st.st_mode))
+			remove_subtree(pathbuf);
+		else if (unlink(pathbuf))
+			die("cannot unlink %s", pathbuf);
+	}
+	closedir(dir);
+	if (rmdir(path))
+		die("cannot rmdir %s", path);
+}
+
 static int create_file(const char *path, unsigned int mode)
 {
 	int fd;
 
 	mode = (mode & 0100) ? 0777 : 0666;
+	create_directories(path);
 	fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 	if (fd < 0) {
-		if (errno == ENOENT) {
+		if ((errno == ENOENT) || (errno == ENOTDIR && force)) {
 			create_directories(path);
 			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 		}
+		else if (errno == EISDIR && force) {
+			remove_subtree(path);
+			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
+		}
 	}
 	return fd;
 }
Created: t/t0000.sh (mode:100755)
--- /dev/null
+++ b/t/t0000.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+case "${verbose+set}" in
+set)	say= ;;
+*)	say=: ;;
+esac
+
+export LANG C
+unset AUTHOR_DATE
+unset AUTHOR_EMAIL
+unset AUTHOR_NAME
+unset COMMIT_AUTHOR_EMAIL
+unset COMMIT_AUTHOR_NAME
+unset GIT_ALTERNATE_OBJECT_DIRECTORIES
+unset GIT_AUTHOR_DATE
+unset GIT_AUTHOR_EMAIL
+unset GIT_AUTHOR_NAME
+unset GIT_COMMITTER_EMAIL
+unset GIT_COMMITTER_NAME
+unset GIT_DIFF_OPTS
+unset GIT_DIR
+unset GIT_EXTERNAL_DIFF
+unset GIT_INDEX_FILE
+unset GIT_OBJECT_DIRECTORY
+unset SHA1_FILE_DIRECTORIES
+unset SHA1_FILE_DIRECTORY
+
+# Test the binaries we have just built.
+PATH=$(pwd)/..:$PATH
+
+# Test repository
+test=test-repo
+rm -fr "$test"
+mkdir "$test"
+cd "$test"
Created: t/t1000-checkout-cache.sh (mode:100755)
--- /dev/null
+++ b/t/t1000-checkout-cache.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+. ./t0000.sh
+git-init-db 2>/dev/null || exit
+date >path0
+mkdir path1
+date >path1/file1
+git-update-cache --add path0 path1/file1
+$say git-ls-files --stage
+
+rm -fr path0 path1
+mkdir path0
+date >path0/file0
+date >path1
+$say git-ls-files --stage
+$say find path*
+
+echo >&2 "* checkout-cache sans -f"
+git-checkout-cache -a
+case "$?" in
+0)	echo >&2 "*** bug: should not have succeeded." ;;
+*)	echo >&2 "*** ok: failed as expected." ;;
+esac
+$say find path*
+
+echo >&2 "* checkout-cache with -f"
+git-checkout-cache -f -a
+case "$?" in
+0)	echo >&2 "*** ok: succeeded as expected." ;;
+*)	echo >&2 "*** bug: should have succeeded." ;;
+esac
+$say find path*
+if test -f path0 && test -d path1 && test -f path1/file1
+then
+	echo >&2 "*** ok: checked out correctly."
+else
+	echo >&2 "*** bug: checkout failed."
+	exit 1
+fi
Created: t/t1001-checkout-cache.sh (mode:100755)
--- /dev/null
+++ b/t/t1001-checkout-cache.sh
@@ -0,0 +1,61 @@
+#!/bin/sh
+
+. ./t0000.sh
+git-init-db 2>/dev/null || exit
+
+show_files() {
+	find path? -ls |
+	sed -e 's/^[0-9]* * [0-9]* * \([-bcdl]\)[^ ]* *[0-9]* *[^ ]* *[^ ]* *[0-9]* [A-Z][a-z][a-z] [0-9][0-9] [^ ]* /fs: \1 /'
+	git-ls-files --stage |
+	sed -e 's/^\([0-9]*\) [0-9a-f]* [0-3] /ca: \1 /'
+	git-ls-tree -r "$1" |
+	sed -e 's/^\([0-9]*\)	[^ ]*	[0-9a-f]*	/tr: \1 /'
+}
+
+mkdir path0
+date >path0/file0
+git-update-cache --add path0/file0
+echo >&2 "* initial state: one file under one directory"
+tree1=$(git-write-tree)
+$say show_files $tree1
+
+mkdir path1
+date >path1/file1
+git-update-cache --add path1/file1
+echo >&2 "* two directories with one file each"
+tree2=$(git-write-tree)
+$say show_files $tree2
+
+rm -fr path1
+git-read-tree -m $tree1
+git-checkout-cache -f -a
+echo >&2 "* go back to initial state"
+$say show_files $tree1
+
+ln -s path0 path1
+git-update-cache --add path1
+echo >&2 "* a symlink where the other side would create a directory."
+tree3=$(git-write-tree)
+$say show_files $tree3
+
+# Morten says "Got that?" here.
+
+git-read-tree $tree2
+git-checkout-cache -f -a
+case "$?" in
+0)	echo >&2 "*** ok: succeeded as expected." ;;
+*)	echo >&2 "*** bug: should have succeeded." ;;
+esac
+echo >&2 "* read tree2 and checkout"
+$say show_files $tree2
+
+if test ! -h path0 && test -d path0 &&
+   test ! -h path1 && test -d path1 &&
+   test ! -h path0/file0 && test -f path0/file0 &&
+   test ! -h path1/file1 && test -f path1/file1
+then
+    echo >&2 "*** ok: checked out correctly."
+else
+    echo >&2 "*** bug: did not check out correctly."
+    exit 1
+fi
------------------------------------------------

Re: "git-checkout-cache -f -a" failure

[Junio C Hamano]

Here is a re-diff against the current tip of the git-pb tree.

------------
Fix checkout-cache when existing work tree interferes with the checkout.

This is essentially the same one as the last one I sent to the
GIT list, except that the patch is rebased to the current tip of
the git-pb tree, and an unnecessary call to create_directories()
removed.

The checkout-cache command gets confused when checking out a
file in a subdirectory and the work tree has a symlink to the
subdirectory.  Also it fails to check things out when there is a
non-directory in the work tree when cache expects a directory
there, and vice versa.  This patch fixes the first problem by
making sure all the leading paths in the file being checked out
are indeed directories, and also fixes directory vs
non-directory conflicts when '-f' is specified by removing the
offending paths.

I've added test subdirectory and two tests to check the above
problems are fixed.

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

checkout-cache.c          |   49 +++++++++++++++++++++++++++++++++--
t/t0000.sh                |   38 +++++++++++++++++++++++++++
t/t1000-checkout-cache.sh |   42 ++++++++++++++++++++++++++++++
t/t1001-checkout-cache.sh |   64 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 190 insertions(+), 3 deletions(-)

--- a/checkout-cache.c
+++ b/checkout-cache.c
@@ -32,6 +32,8 @@
  * of "-a" causing problems (not possible in the above example,
  * but get used to it in scripting!).
  */
+#include <sys/types.h>
+#include <dirent.h>
 #include "cache.h"
 
 static int force = 0, quiet = 0, not_new = 0;
@@ -46,20 +48,61 @@ static void create_directories(const cha
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
-		mkdir(buf, 0755);
+		if (mkdir(buf, 0755)) {
+			if (errno == EEXIST) {
+				struct stat st;
+				if (!lstat(buf, &st) && S_ISDIR(st.st_mode))
+					continue; /* ok */
+				if (force && !unlink(buf) && !mkdir(buf, 0755))
+					continue;
+			}
+			die("cannot create directory at %s", buf);
+		}
 	}
 	free(buf);
 }
 
+static void remove_subtree(const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *de;
+	char pathbuf[PATH_MAX];
+	char *name;
+	
+	if (!dir)
+		die("cannot opendir %s", path);
+	strcpy(pathbuf, path);
+	name = pathbuf + strlen(path);
+	*name++ = '/';
+	while ((de = readdir(dir)) != NULL) {
+		struct stat st;
+		if ((de->d_name[0] == '.') &&
+		    ((de->d_name[1] == 0) ||
+		     ((de->d_name[1] == '.') && de->d_name[2] == 0)))
+			continue;
+		strcpy(name, de->d_name);
+		if (lstat(pathbuf, &st))
+			die("cannot lstat %s", pathbuf);
+		if (S_ISDIR(st.st_mode))
+			remove_subtree(pathbuf);
+		else if (unlink(pathbuf))
+			die("cannot unlink %s", pathbuf);
+	}
+	closedir(dir);
+	if (rmdir(path))
+		die("cannot rmdir %s", path);
+}
+
 static int create_file(const char *path, unsigned int mode)
 {
 	int fd;
 
 	mode = (mode & 0100) ? 0777 : 0666;
+	create_directories(path);
 	fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 	if (fd < 0) {
-		if (errno == ENOENT) {
-			create_directories(path);
+		if (errno == EISDIR && force) {
+			remove_subtree(path);
 			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 		}
 	}
Created: t/t0000.sh (mode:100755)
--- /dev/null
+++ b/t/t0000.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+case "${verbose+set}" in
+set)	say= ;;
+*)	say=: ;;
+esac
+
+export LANG C
+unset AUTHOR_DATE
+unset AUTHOR_EMAIL
+unset AUTHOR_NAME
+unset COMMIT_AUTHOR_EMAIL
+unset COMMIT_AUTHOR_NAME
+unset GIT_ALTERNATE_OBJECT_DIRECTORIES
+unset GIT_AUTHOR_DATE
+unset GIT_AUTHOR_EMAIL
+unset GIT_AUTHOR_NAME
+unset GIT_COMMITTER_EMAIL
+unset GIT_COMMITTER_NAME
+unset GIT_DIFF_OPTS
+unset GIT_DIR
+unset GIT_EXTERNAL_DIFF
+unset GIT_INDEX_FILE
+unset GIT_OBJECT_DIRECTORY
+unset SHA1_FILE_DIRECTORIES
+unset SHA1_FILE_DIRECTORY
+
+# Test the binaries we have just built.
+PATH=$(pwd)/..:$PATH
+
+# Test repository
+test=test-repo
+rm -fr "$test"
+mkdir "$test"
+cd "$test"
Created: t/t1000-checkout-cache.sh (mode:100755)
--- /dev/null
+++ b/t/t1000-checkout-cache.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+. ./t0000.sh
+git-init-db 2>/dev/null || exit
+date >path0
+mkdir path1
+date >path1/file1
+git-update-cache --add path0 path1/file1
+$say git-ls-files --stage
+
+rm -fr path0 path1
+mkdir path0
+date >path0/file0
+date >path1
+$say git-ls-files --stage
+$say find path*
+
+echo >&2 "* checkout-cache sans -f"
+git-checkout-cache -a
+case "$?" in
+0)	echo >&2 "*** bug: should not have succeeded." ;;
+*)	echo >&2 "*** ok: failed as expected." ;;
+esac
+$say find path*
+
+echo >&2 "* checkout-cache with -f"
+git-checkout-cache -f -a
+case "$?" in
+0)	echo >&2 "*** ok: succeeded as expected." ;;
+*)	echo >&2 "*** bug: should have succeeded." ;;
+esac
+$say find path*
+if test -f path0 && test -d path1 && test -f path1/file1
+then
+	echo >&2 "*** ok: checked out correctly."
+else
+	echo >&2 "*** bug: checkout failed."
+	exit 1
+fi
Created: t/t1001-checkout-cache.sh (mode:100755)
--- /dev/null
+++ b/t/t1001-checkout-cache.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+. ./t0000.sh
+git-init-db 2>/dev/null || exit
+
+show_files() {
+	find path? -ls |
+	sed -e 's/^[0-9]* * [0-9]* * \([-bcdl]\)[^ ]* *[0-9]* *[^ ]* *[^ ]* *[0-9]* [A-Z][a-z][a-z] [0-9][0-9] [^ ]* /fs: \1 /'
+	git-ls-files --stage |
+	sed -e 's/^\([0-9]*\) [0-9a-f]* [0-3] /ca: \1 /'
+	git-ls-tree -r "$1" |
+	sed -e 's/^\([0-9]*\)	[^ ]*	[0-9a-f]*	/tr: \1 /'
+}
+
+mkdir path0
+date >path0/file0
+git-update-cache --add path0/file0
+echo >&2 "* initial state: one file under one directory"
+tree1=$(git-write-tree)
+$say show_files $tree1
+
+mkdir path1
+date >path1/file1
+git-update-cache --add path1/file1
+echo >&2 "* two directories with one file each"
+tree2=$(git-write-tree)
+$say show_files $tree2
+
+rm -fr path1
+git-read-tree -m $tree1
+git-checkout-cache -f -a
+echo >&2 "* go back to initial state"
+$say show_files $tree1
+
+ln -s path0 path1
+git-update-cache --add path1
+echo >&2 "* a symlink where the other side would create a directory."
+tree3=$(git-write-tree)
+$say show_files $tree3
+
+# Morten says "Got that?" here.
+
+git-read-tree $tree2
+git-checkout-cache -f -a
+case "$?" in
+0)	echo >&2 "*** ok: succeeded as expected." ;;
+*)	echo >&2 "*** bug: should have succeeded." ;;
+esac
+echo >&2 "* read tree2 and checkout"
+$say show_files $tree2
+
+if test ! -h path0 && test -d path0 &&
+   test ! -h path1 && test -d path1 &&
+   test ! -h path0/file0 && test -f path0/file0 &&
+   test ! -h path1/file1 && test -f path1/file1
+then
+    echo >&2 "*** ok: checked out correctly."
+else
+    echo >&2 "*** bug: did not check out correctly."
+    exit 1
+fi
------------------------------------------------

Test suite

[Petr Baudis]

Dear diary, on Thu, May 12, 2005 at 12:12:10AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> I've added test subdirectory and two tests to check the above
> problems are fixed.

I'd prefer introduction of the infrastructure for this in a separate
commit. I'll focus on it here; I will have a look at the checkout-cache
stuff tomorrow.

> Created: t/t0000.sh (mode:100755)
> --- /dev/null
> +++ b/t/t0000.sh

I'd prefer some better name for this. What about testlib.sh?

> @@ -0,0 +1,38 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2005 Junio C Hamano
> +#

A short description here would be nice.

> +case "${verbose+set}" in
> +set)	say= ;;
> +*)	say=: ;;
> +esac

Perhaps set $verbose if -v was passed or something.

Looking at how you use it, I wouldn't run the command at all if not
verbose.

> Created: t/t1000-checkout-cache.sh (mode:100755)
> --- /dev/null
> +++ b/t/t1000-checkout-cache.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2005 Junio C Hamano
> +#

I think each test in the suite should have a short summary of its focus
in the top comment.

> +. ./t0000.sh
> +git-init-db 2>/dev/null || exit

I'd do this in testlib.sh (you might well blast it away and redo it when
testing init-db).

> +echo >&2 "* checkout-cache sans -f"
> +git-checkout-cache -a
> +case "$?" in
> +0)	echo >&2 "*** bug: should not have succeeded." ;;
> +*)	echo >&2 "*** ok: failed as expected." ;;
> +esac

And this essentially repeats all over and over.

What about something like

cmdtest "sans -f" git-checkout-cache -a

> +if test -f path0 && test -d path1 && test -f path1/file1
> +then
> +	echo >&2 "*** ok: checked out correctly."
> +else
> +	echo >&2 "*** bug: checkout failed."
> +	exit 1
> +fi

We might get a good function for this later too, for now I'd propose

if test -f path0 && test -d path1 && test -f path1/file1; then
	testok "checkout"
else
	testfail "checkout" # non-fatal test failure
	testfatal "checkout" # fatal test failure, die right away
fi


And at the end the script, you should do

exit $teststatus

which is set to non-zero anytime testfail was invoked.

> Created: t/t1001-checkout-cache.sh (mode:100755)
> --- /dev/null
> +++ b/t/t1001-checkout-cache.sh
> @@ -0,0 +1,64 @@
> +show_files() {
> +	find path? -ls |
> +	sed -e 's/^[0-9]* * [0-9]* * \([-bcdl]\)[^ ]* *[0-9]* *[^ ]* *[^ ]* *[0-9]* [A-Z][a-z][a-z] [0-9][0-9] [^ ]* /fs: \1 /'
> +	git-ls-files --stage |
> +	sed -e 's/^\([0-9]*\) [0-9a-f]* [0-3] /ca: \1 /'
> +	git-ls-tree -r "$1" |
> +	sed -e 's/^\([0-9]*\)	[^ ]*	[0-9a-f]*	/tr: \1 /'
> +}

Ugh. I think a comment would be suitable here. ;-)

> +mkdir path0
> +date >path0/file0
> +git-update-cache --add path0/file0
> +echo >&2 "* initial state: one file under one directory"
> +tree1=$(git-write-tree)

cmdtest --capture-stdout "one file under one directory" git-write-tree
tree1=$(cat $teststdout)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

[PATCH] Test suite

[Junio C Hamano]

Commit    1da683e1247046796a094c4917bc0c4591530272
Author    Junio C Hamano <junkio@cox.net>, Wed May 11 16:59:35 2005 -0700
Committer Junio C Hamano <junkio@cox.net>, Wed May 11 16:59:35 2005 -0700

Test suite: infrastructure and examples.

This adds the test suite infrastructure with two example tests.
The current git-checkout-cache the example tests would fail this
test and will be corrected in a separate patch.

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

Created: t/t1000-checkout-cache.sh (mode:100755)
--- /dev/null
+++ b/t/t1000-checkout-cache.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+. ./test-lib.sh
+test_description "$@" 'git-checkout-cache test.
+
+This test registers the following filesystem structure in the
+cache:
+
+    path0       - a file
+    path1/file1 - a file in a directory
+
+And then tries to checkout in a work tree that has the following:
+
+    path0/file0 - a file in a directory
+    path1       - a file
+
+The git-checkout-cache command should fail when attempting to checkout
+path0, finding it is occupied by a directory, and path1/file1, finding
+path1 is occupied by a non-directory.  With "-f" flag, it should remove
+the conflicting paths and succeed.
+'
+
+date >path0
+mkdir path1
+date >path1/file1
+git-update-cache --add path0 path1/file1
+test_debug 'git-ls-files --stage'
+
+rm -fr path0 path1
+mkdir path0
+date >path0/file0
+date >path1
+test_debug 'git-ls-files --stage'
+test_debug 'find path*'
+
+test_expect_failure 'git-checkout-cache -a'
+test_debug 'find path*'
+
+test_expect_success 'git-checkout-cache -f -a'
+test_debug 'find path*'
+
+if test -f path0 && test -d path1 && test -f path1/file1
+then
+	test_ok "checkout successful"
+else
+	test_failure "checkout failed"
+fi
+
+test_done
+
+
Created: t/t1001-checkout-cache.sh (mode:100755)
--- /dev/null
+++ b/t/t1001-checkout-cache.sh
@@ -0,0 +1,76 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+. ./test-lib.sh
+test_description "$@" 'git-checkout-cache test.
+
+This test registers the following filesystem structure in the cache:
+
+    path0/file0	- a file in a directory
+    path1/file1 - a file in a directory
+
+and attempts to check it out when the work tree has:
+
+    path0/file0 - a file in a directory
+    path1       - a symlink pointing at "path0"
+
+Checkout cache should fail to extract path1/file1 because the leading
+path path1 is occupied by a non-directory.  With "-f" it should remove
+the symlink path1 and create directory path1 and file path1/file1.
+'
+
+show_files() {
+	# show filesystem files, just [-dl] for type and name
+	find path? -ls |
+	sed -e 's/^[0-9]* * [0-9]* * \([-bcdl]\)[^ ]* *[0-9]* *[^ ]* *[^ ]* *[0-9]* [A-Z][a-z][a-z] [0-9][0-9] [^ ]* /fs: \1 /'
+	# what's in the cache, just mode and name
+	git-ls-files --stage |
+	sed -e 's/^\([0-9]*\) [0-9a-f]* [0-3] /ca: \1 /'
+	# what's in the tree, just mode and name.
+	git-ls-tree -r "$1" |
+	sed -e 's/^\([0-9]*\)	[^ ]*	[0-9a-f]*	/tr: \1 /'
+}
+
+mkdir path0
+date >path0/file0
+git-update-cache --add path0/file0
+tree1=$(git-write-tree)
+test_debug 'show_files $tree1'
+
+mkdir path1
+date >path1/file1
+git-update-cache --add path1/file1
+tree2=$(git-write-tree)
+test_debug 'show_files $tree2'
+
+rm -fr path1
+git-read-tree -m $tree1
+git-checkout-cache -f -a
+test_debug 'show_files $tree1'
+
+ln -s path0 path1
+git-update-cache --add path1
+tree3=$(git-write-tree)
+test_debug 'show_files $tree3'
+
+# Morten says "Got that?" here.
+# Test begins.
+
+git-read-tree $tree2
+test_expect_success 'git-checkout-cache -f -a'
+test_debug show_files $tree2
+
+if test ! -h path0 && test -d path0 &&
+   test ! -h path1 && test -d path1 &&
+   test ! -h path0/file0 && test -f path0/file0 &&
+   test ! -h path1/file1 && test -f path1/file1
+then
+    test_ok "checked out correctly."
+else
+    test_failure "did not check out correctly."
+fi
+
+test_done
+
Created: t/test-lib.sh (mode:100755)
--- /dev/null
+++ b/t/test-lib.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+# For repeatability, reset the environment to known value.
+export LANG C
+export TZ UTC
+unset AUTHOR_DATE
+unset AUTHOR_EMAIL
+unset AUTHOR_NAME
+unset COMMIT_AUTHOR_EMAIL
+unset COMMIT_AUTHOR_NAME
+unset GIT_ALTERNATE_OBJECT_DIRECTORIES
+unset GIT_AUTHOR_DATE
+unset GIT_AUTHOR_EMAIL
+unset GIT_AUTHOR_NAME
+unset GIT_COMMITTER_EMAIL
+unset GIT_COMMITTER_NAME
+unset GIT_DIFF_OPTS
+unset GIT_DIR
+unset GIT_EXTERNAL_DIFF
+unset GIT_INDEX_FILE
+unset GIT_OBJECT_DIRECTORY
+unset SHA1_FILE_DIRECTORIES
+unset SHA1_FILE_DIRECTORY
+
+# Each test should start with something like this, after copyright notices:
+#
+# . ./testlib.sh
+# test_description "$@" 'Description of this test...
+# This test checks if command xyzzy does the right thing...
+# '
+#
+
+test_description () {
+	while case "$#" in 0) break;; esac
+	do
+		case "$1" in
+		-d|--d|--de|--deb|--debu|--debug)
+			debug=t; shift ;;
+		-h|--h|--he|--hel|--help)
+			eval echo '"$'$#'"'
+			exit 0
+			;;
+		*)
+			break ;;
+		esac
+	done
+	test_failure=0
+}
+
+say () {
+	echo "* $*"
+}
+
+test_debug () {
+	case "$debug" in '') ;; ?*) eval "$*" ;; esac
+}
+
+test_ok () {
+	echo "* $*";
+}
+
+test_failure () {
+	echo "* $*";
+	test_failure=1;
+}
+
+test_expect_failure () {
+	say "expecting failure: $1"
+	eval "$1"
+	case $? in
+	0)	test_failure "did not fail as expected" ;;
+	*) 	test_ok "failed as expected" ;;
+	esac
+}
+
+test_expect_success () {
+	say "expecting success: $1"
+	eval "$1"
+	case $? in
+	0) 	test_ok "succeeded as expected" ;;
+	*)	test_failure "did not succeed as expected" ;;
+	esac
+}
+
+test_done () {
+	case "$test_failure" in
+	0)	exit 0 ;;
+	'')	echo "*** test script did not start with test_description";
+		exit 2 ;;
+	*)	exit 1 ;;
+	esac
+}
+
+# Test the binaries we have just built.  The tests are kept in
+# t/ subdirectory and are run in test-repo subdirectory.
+PATH=$(pwd)/..:$PATH
+
+# Test repository
+test=test-repo
+rm -fr "$test"
+mkdir "$test"
+cd "$test"
+git-init-db 2>/dev/null || error "cannot run git-init-db"
------------------------------------------------

[PATCH] checkout-cache fix

[Junio C Hamano]

Commit    cc01b05f0a3dfdf5ed114e429a7bec1ad549ab1c
Author    Junio C Hamano <junkio@cox.net>, Wed May 11 17:00:16 2005 -0700
Committer Junio C Hamano <junkio@cox.net>, Wed May 11 17:00:16 2005 -0700

Fix checkout-cache when existing work tree interferes with the checkout.

This is essentially the same one as the last one I sent to the
GIT list, except that the patch is rebased to the current tip of
the git-pb tree, and an unnecessary call to create_directories()
removed.

The checkout-cache command gets confused when checking out a
file in a subdirectory and the work tree has a symlink to the
subdirectory.  Also it fails to check things out when there is a
non-directory in the work tree when cache expects a directory
there, and vice versa.  This patch fixes the first problem by
making sure all the leading paths in the file being checked out
are indeed directories, and also fixes directory vs
non-directory conflicts when '-f' is specified by removing the
offending paths.

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

--- a/checkout-cache.c
+++ b/checkout-cache.c
@@ -32,6 +32,8 @@
  * of "-a" causing problems (not possible in the above example,
  * but get used to it in scripting!).
  */
+#include <sys/types.h>
+#include <dirent.h>
 #include "cache.h"
 
 static int force = 0, quiet = 0, not_new = 0;
@@ -46,20 +48,61 @@ static void create_directories(const cha
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
-		mkdir(buf, 0755);
+		if (mkdir(buf, 0755)) {
+			if (errno == EEXIST) {
+				struct stat st;
+				if (!lstat(buf, &st) && S_ISDIR(st.st_mode))
+					continue; /* ok */
+				if (force && !unlink(buf) && !mkdir(buf, 0755))
+					continue;
+			}
+			die("cannot create directory at %s", buf);
+		}
 	}
 	free(buf);
 }
 
+static void remove_subtree(const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *de;
+	char pathbuf[PATH_MAX];
+	char *name;
+	
+	if (!dir)
+		die("cannot opendir %s", path);
+	strcpy(pathbuf, path);
+	name = pathbuf + strlen(path);
+	*name++ = '/';
+	while ((de = readdir(dir)) != NULL) {
+		struct stat st;
+		if ((de->d_name[0] == '.') &&
+		    ((de->d_name[1] == 0) ||
+		     ((de->d_name[1] == '.') && de->d_name[2] == 0)))
+			continue;
+		strcpy(name, de->d_name);
+		if (lstat(pathbuf, &st))
+			die("cannot lstat %s", pathbuf);
+		if (S_ISDIR(st.st_mode))
+			remove_subtree(pathbuf);
+		else if (unlink(pathbuf))
+			die("cannot unlink %s", pathbuf);
+	}
+	closedir(dir);
+	if (rmdir(path))
+		die("cannot rmdir %s", path);
+}
+
 static int create_file(const char *path, unsigned int mode)
 {
 	int fd;
 
 	mode = (mode & 0100) ? 0777 : 0666;
+	create_directories(path);
 	fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 	if (fd < 0) {
-		if (errno == ENOENT) {
-			create_directories(path);
+		if (errno == EISDIR && force) {
+			remove_subtree(path);
 			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
 		}
 	}
------------------------------------------------

Re: [PATCH] Test suite

[Petr Baudis]

Dear diary, on Thu, May 12, 2005 at 02:01:34AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> Commit    1da683e1247046796a094c4917bc0c4591530272
> Author    Junio C Hamano <junkio@cox.net>, Wed May 11 16:59:35 2005 -0700
> Committer Junio C Hamano <junkio@cox.net>, Wed May 11 16:59:35 2005 -0700
> 
> Test suite: infrastructure and examples.
> 
> This adds the test suite infrastructure with two example tests.
> The current git-checkout-cache the example tests would fail this
> test and will be corrected in a separate patch.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

Admittely, I'm not happy with this. From the design standpoint it looks
mostly fine now, but the code is rather crude. I wanted to go over it at
first and fix the obvious stuff, but it appeared to be overall quite
broken, so I decided to return it to you for another iteration. I don't
mind if you just fix the broken code for now, we can fix the semantics
and design stuff later - what is in the patch is already good enough for
the start.

I'll drop the testcases from your other patches for now so that we don't
get long stalls.

> --- /dev/null
> +++ b/t/test-lib.sh
> +# For repeatability, reset the environment to known value.
> +export LANG C
> +export TZ UTC

Dunno about *your* shell but this just exports variables $LANG, $C, $TZ
and $UTC here. You probably wanted assignments there?

> +# Each test should start with something like this, after copyright notices:
> +#
> +# . ./testlib.sh

test-lib.sh

> +# test_description "$@" 'Description of this test...
> +# This test checks if command xyzzy does the right thing...
> +# '

I think this usage is pretty weird. Why not just

test_description='Description, blah blah.'
. ./testlib.sh

I think it would be quite less confusing than test_description, which
actually does effectively something different anyway.

> +
> +test_description () {
> +	while case "$#" in 0) break;; esac

Duh. This looks mysterious - why not a simple test?

> +	do
> +		case "$1" in
> +		-d|--d|--de|--deb|--debu|--debug)
> +			debug=t; shift ;;
> +		-h|--h|--he|--hel|--help)
> +			eval echo '"$'$#'"'
> +			exit 0
> +			;;
> +		*)
> +			break ;;

This branch makes no sense, I think.

> +		esac
> +	done
> +	test_failure=0
> +}
> +
> +say () {
> +	echo "* $*"
> +}
> +
> +test_debug () {
> +	case "$debug" in '') ;; ?*) eval "$*" ;; esac

Again, why not a simple test?

[ "$debug" ] && eval "$*"

(Actually, eval will do the wrong thing here - it just concatenates the
arguments. Just "$@" would do, I guess.)

> +}
> +
> +test_ok () {
> +	echo "* $*";
> +}
> +
> +test_failure () {
> +	echo "* $*";
> +	test_failure=1;
> +}
> +
> +test_expect_failure () {
> +	say "expecting failure: $1"
> +	eval "$1"
> +	case $? in
> +	0)	test_failure "did not fail as expected" ;;
> +	*) 	test_ok "failed as expected" ;;
> +	esac
> +}
> +
> +test_expect_success () {
> +	say "expecting success: $1"
> +	eval "$1"
> +	case $? in
> +	0) 	test_ok "succeeded as expected" ;;
> +	*)	test_failure "did not succeed as expected" ;;
> +	esac
> +}
> +
> +test_done () {
> +	case "$test_failure" in
> +	0)	exit 0 ;;

Please clean up after yourself in this case.

> +	'')	echo "*** test script did not start with test_description";
> +		exit 2 ;;
> +	*)	exit 1 ;;
> +	esac
> +}
> +
> +# Test the binaries we have just built.  The tests are kept in
> +# t/ subdirectory and are run in test-repo subdirectory.
> +PATH=$(pwd)/..:$PATH
> +
> +# Test repository
> +test=test-repo
> +rm -fr "$test"
> +mkdir "$test"
> +cd "$test"
> +git-init-db 2>/dev/null || error "cannot run git-init-db"

But there's no 'error' thing.

> --- /dev/null
> +++ b/t/t1000-checkout-cache.sh
> +git-update-cache --add path0 path1/file1

You should make sure even those preparations calls actually succeed.

> --- /dev/null
> +++ b/t/t1001-checkout-cache.sh
> +git-update-cache --add path0/file0
> +tree1=$(git-write-tree)

Here too.


The testcases currently utterly fail, which is not good sign - either
they are ahead of the current code, or they are broken. This is the main
hurdle making me not accept it yet - it does not work for me. If you fix
this and the nits above, it can go in, I think.


* expecting failure: git-checkout-cache -a
checkout-cache: path0 already exists
error: checkout-cache: unable to create file path1/file1 (Not a directory)
* did not fail as expected
* expecting success: git-checkout-cache -f -a
error: checkout-cache: unable to create file path0 (Is a directory)
* succeeded as expected
* checkout failed

I consider this output... well, totally confusing. checkout-cache fails
but testcase thinks it does not, then it fails again but testcase thinks
it succeeded as expected but dies anyway.

I think it's too messy. I would much more appreciate output like this:

* checkout-cache test [1/3]: git-checkout-cache -a... passed
* checkout-cache test [2/3]: git-checkout-cache -f -a... NOT PASSED
Expected success, but the command failed. Output:
error: checkout-cache: unable to create file path0 (Is a directory)
* checkout-cache test [3/3]: git-checkout-cache foobar... NOT PASSED
Expected failure, but the command succeeded.

Much less cluttered, it is clear what went wrong etc.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] checkout-cache fix

[Petr Baudis]

Dear diary, on Thu, May 12, 2005 at 02:02:45AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> Commit    cc01b05f0a3dfdf5ed114e429a7bec1ad549ab1c
> Author    Junio C Hamano <junkio@cox.net>, Wed May 11 17:00:16 2005 -0700
> Committer Junio C Hamano <junkio@cox.net>, Wed May 11 17:00:16 2005 -0700
> 
> Fix checkout-cache when existing work tree interferes with the checkout.

Thanks, applied. A nit about the commit message, though - I'd prefer you
to put this metadata stuff belong the --- separator, since they really
do not belong to the log message. I've already seen something like this
in one commit merged from git-jc (IIRC some of the Ingo Molnar's leak
fixes), and it was a little PITA there since the first line was some
 Date: header but we tend to use the first line as the commit's caption
at some places.

Thanks,

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: [PATCH] Test suite

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

>> --- /dev/null
>> +++ b/t/test-lib.sh
>> +# For repeatability, reset the environment to known value.
>> +export LANG C
>> +export TZ UTC

These are just "Duh".  Sorry.

>> +
>> +test_description () {
>> +	while case "$#" in 0) break;; esac

PB> Duh. This looks mysterious - why not a simple test?

Because I am old fashioned and am used to this kind of Bourne
shell idioms.

PB> This branch makes no sense, I think.

Because...?

PB> Again, why not a simple test?

Again using case when test is not needed is a good old Bourne
shell idiom.

PB> [ "$debug" ] && eval "$*"

PB> (Actually, eval will do the wrong thing here - it just concatenates the
PB> arguments. Just "$@" would do, I guess.)

No, I wrote "$*" because that was what I wanted and not "$@".  I
wanted to give eval a _single_ string.

PB> Please clean up after yourself in this case.

I would not mind, but I'd mildly disagree.  The next test will
start by cleaning up test-repo/ anyway.  What we are lacking is
t/Makefile that drives all the tests and clean-up can be done
there.

>> +git-init-db 2>/dev/null || error "cannot run git-init-db"

PB> But there's no 'error' thing.

Yes, and that way you can get an error message ;-).  Simple
oversight.

PB> The testcases currently utterly fail,

That is something I told you in the cover letter (or commit log).

Re: [PATCH] checkout-cache fix

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> ... I've already seen something like this
PB> in one commit merged from git-jc (IIRC some of the Ingo Molnar's leak
PB> fixes),...

Yes, I am very unhappy about that commit, too.  I was mucking
with jit-commit command at the time, and apparently used a buggy
one to make that commit---I realized what happened much later
when I reviewed the commit log and it was already too late (I
told everybody that the tree is published and pullable by then).

Sorry about that.

[PATCH] Fix git-diff-files for symlinks.

[Junio C Hamano]

Again I am not sure why this was missed during the last round,
but git-diff-files mishandles symlinks on the filesystem.  This
patch fixes it.

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

*** Petr, this one falls into "emergency obvious fix" category.
*** I found it during adding more "basic" test to the test suite,
*** which turns out to be quite useful.

diff-files.c |    3 ++-
1 files changed, 2 insertions(+), 1 deletion(-)

--- a/diff-files.c
+++ b/diff-files.c
@@ -126,7 +126,8 @@
 			continue;
 
 		oldmode = ntohl(ce->ce_mode);
-		mode = S_IFREG | ce_permissions(st.st_mode);
+		mode = (S_ISLNK(st.st_mode) ? S_IFLNK :
+			S_IFREG | ce_permissions(st.st_mode));
 
 		show_modified(oldmode, mode, ce->sha1, null_sha1,
 			      ce->name);
------------------------------------------------

[PATCH 0/3] Core GIT fixes and additions.

[Junio C Hamano]

Pasky,
        I am sending you three patches rediffed against the tip
of git-pb tree for inclusion.

  * [PATCH 1/3] Introduce "rev-list --stop-at=<commit>".
  * [PATCH 2/3] Support symlinks in git-ls-files --others.
  * [PATCH 3/3] Add git-ls-files -k.

The first one is independent from other two.  3/3 touches the
same file as 2/3.