Skip to contentSkip to conversation

git-clean buglet

PATCH In review Johannes Sixt 8 participants 47 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

Try this in your favorite git repo:

git clean -n /

Here, it responds with:

fatal: oops in prep_exclude

I don't know what the expected behavior should be, but certainly not this. Maybe just do nothing like 'git ls-files /'.

I'm not familiar with either builtin-clean.c nor dir.c; perhaps someone else can fix this?

-- Hannes

#1 lore raw
Quoted 10 lines · Johannes Sixt

Try this in your favorite git repo:

git clean -n /

Here, it responds with:

fatal: oops in prep_exclude

I don't know what the expected behavior should be, but certainly not this. Maybe just do nothing like 'git ls-files /'.

Well, 'git ls-files -o /' would be more similar, but that one lists the entire disk contents - not what I would have expected, either. Hmm...

-- Hannes

#2 lore raw
In reply to Johannes Sixt

Hi,

Quoted 3 lines · Johannes Sixt

Try this in your favorite git repo:

git clean -n /

That's an absolute path. Like almost _all_ git commands, clean only takes relative ones. You probably meant "git clean -n".

Hth,
Dscho

#3 lore raw
Quoted 10 lines · Johannes Schindelin

Hi,

Quoted 3 lines · Johannes Sixt

Try this in your favorite git repo:

git clean -n /

That's an absolute path. Like almost _all_ git commands, clean only takes relative ones. You probably meant "git clean -n".

I know it's an absolute path, but, no, I said

git clean -n \*.vcproj

on Windows, which the MinGW port internally transforms into

git clean -n /*.vcproj

(which was not exactly what I meant, but that's a different story). And this also reports, just like on Linux:

fatal: oops in prep_exclude

There remain the questions whether we want to do something about absolute paths in general or this oops in particular.

-- Hannes

#4 lore raw

PATCH 1/1 Fix off by one error in prep_exclude.

base + current already includes the trailing slash so adding one removes the first character of the next directory.

---

This fixes the oops part of the issue Johannes found, but doesn't address the fact that we probably should remove files that aren't a part of the repository at in the first place.

 dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
dir.c +1 -1
index 3e345c2..9e5879a 100644
@@ -237,7 +237,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 			current = 0;
 		}
 		else {
-			cp = strchr(base + current + 1, '/');
+			cp = strchr(base + current, '/');
 			if (!cp)
 				die("oops in prep_exclude");
 			cp++;
Signature · 1 line
1.5.4-rc2.GIT
#5 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Hi,

Quoted 7 lines · Shawn Bohrer

base + current already includes the trailing slash so adding one removes the first character of the next directory.

---

This fixes the oops part of the issue Johannes found,

have I?

Quoted 2 lines

but doesn't address the fact that we probably should remove files that aren't a part of the repository at in the first place.

I am sorry, but I cannot begin to see what this commit tries to accomplish. Yes, sure, there is an off-by-one error, and your commit message says how that was fixed. But I miss a description what usage it would affect, i.e. when this bug triggers.

I imagine that you would be as lost as me, reading that commit message 6 months from now, trying to understand why that change was made.

Ciao,
Dscho

#6 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Quoted 7 lines · Johannes Schindelin

Hi,

Quoted 3 lines · Shawn Bohrer

base + current already includes the trailing slash so adding one removes the first character of the next directory.

Quoted 6 lines
Quoted 4 lines

---

This fixes the oops part of the issue Johannes found,

have I?

Sorry I should have been more explicit Johannes Sixt reported the issue, you were included simply because you had been involved in the thread.

Quoted 7 lines
Quoted 2 lines

but doesn't address the fact that we probably should remove files that aren't a part of the repository at in the first place.

I am sorry, but I cannot begin to see what this commit tries to accomplish. Yes, sure, there is an off-by-one error, and your commit message says how that was fixed. But I miss a description what usage it would affect, i.e. when this bug triggers.

As far as I can see there are two protential cases that could trigger this bug, but there may be more. This first was the arguably invalid case the Johannes Sixt reported.

git clean -n /

The other case that could trigger this bug and potentially others is if someone makes their root dircetory a git repository and then uses "/" as an absolute path. For example:

cd /
git init
git clean -n /

You may argue that both of these cases are invalid and that is fine by me, but since I noticed this bug I thought I would send a patch. If you wouldlike I can add these two use cases to the commit message.

--
Shawn

#7 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

In reply to Johannes Schindelin
Quoted 10 lines · Johannes Schindelin
Quoted 2 lines

but doesn't address the fact that we probably should remove files that aren't a part of the repository at in the first place.

I am sorry, but I cannot begin to see what this commit tries to accomplish. Yes, sure, there is an off-by-one error, and your commit message says how that was fixed. But I miss a description what usage it would affect, i.e. when this bug triggers.

I imagine that you would be as lost as me, reading that commit message 6 months from now, trying to understand why that change was made.

Likewise. The message has somewhat to be desired...

In "struct exclude_stack", prep_exclude() and excluded(), the convention for a path is to express the length of directory part including the trailing slash (e.g. "foo" and "bar/baz" will get baselen=0 and baselen=4 respectively).

The variable current and parameter baselen follow that convention in the codepath the patch touches.

		else {
			cp = strchr(base + current + 1, '/');
			if (!cp)
				die("oops in prep_exclude");
			cp++;
		}
		stk->prev = dir->exclude_stack;
		stk->baselen = cp - base;

is about coming up with the next value for current (which is taken from stk->baselen) to dig one more level.

If base="foo/a/boo" and current=4 (i.e. we are looking at "foo/"), at the point, scanning from (base+current) as Shawn Bohrer's patch suggests means the scan begins at "a/boo" to find the next slash. The existing code skips one letter ('a') and starts scanning from "/boo".

The only case this microoptimization makes difference is when an input is malformed and has double-slash (i.e. path component whose length is zero), like "foo//boo".

Perhaps the "oops part of the issue Johannes found" had a caller that feeds such an incorrect input?

#8 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Johannes Schindelin
Quoted 2 lines

but doesn't address the fact that we probably should remove files that aren't a part of the repository at in the first place.

I am sorry, but I cannot begin to see what this commit tries to accomplish. Yes, sure, there is an off-by-one error, and your commit message says how that was fixed. But I miss a description what usage it would affect, i.e. when this bug triggers.

Quoted 4 lines
Quoted 2 lines

I imagine that you would be as lost as me, reading that commit message 6 months from now, trying to understand why that change was made.

Likewise. The message has somewhat to be desired...

Agreed I'll resend with a improved message.

Quoted 26 lines

In "struct exclude_stack", prep_exclude() and excluded(), the convention for a path is to express the length of directory part including the trailing slash (e.g. "foo" and "bar/baz" will get baselen=0 and baselen=4 respectively).

The variable current and parameter baselen follow that convention in the codepath the patch touches.

		else {
			cp = strchr(base + current + 1, '/');
			if (!cp)
				die("oops in prep_exclude");
			cp++;
		}
		stk->prev = dir->exclude_stack;
		stk->baselen = cp - base;

is about coming up with the next value for current (which is taken from stk->baselen) to dig one more level.

If base="foo/a/boo" and current=4 (i.e. we are looking at "foo/"), at the point, scanning from (base+current) as Shawn Bohrer's patch suggests means the scan begins at "a/boo" to find the next slash. The existing code skips one letter ('a') and starts scanning from "/boo".

Quoted 3 lines

The only case this microoptimization makes difference is when an input is malformed and has double-slash (i.e. path component whose length is zero), like "foo//boo".

Good catch, I didn't think of this case but this indeed will cause the same issue.

Quoted 2 lines

Perhaps the "oops part of the issue Johannes found" had a caller that feeds such an incorrect input?

Nope the problem Johannes Sixt was having was that he mistakenly ran

git clean -n /*foo

Now that isn't what he meant to do, but I figured it might be possible that someone has their whole filesystem in a git repository, or maybe is using some sort of chroot on their repository. Your malformed paths guess is probably much more likely to occur.

--
Shawn

#9 lore raw

PATCH 1/1 Fix off by one error in prep_exclude.

In "struct exclude_stack", prep_exclude() and excluded(), the convention for a path is to express the length of directory part including the trailing slash (e.g. "foo" and "bar/baz" will get baselen=0 and baselen=4 respectively).

The variable current and parameter baselen follow that convention in the codepath the following patch touches.

                else {
                        cp = strchr(base + current + 1, '/');
                        if (!cp)
                                die("oops in prep_exclude");
                        cp++;
                }
                stk->prev = dir->exclude_stack;
                stk->baselen = cp - base;

is about coming up with the next value for current (which is taken from stk->baselen) to dig one more level.

If base="foo/bar/boo" and current=4 (i.e. we are looking at "foo/"), the current code (base + current + 1) will begin scanning for the next slash at ar/boo skipping one letter ('b'). This patch starts the scanning at bar/boo/

This only causes a problem when a path component has a length of zero which can happen when the user provides an absolute path to a file or directory in the root directory (i.e. "/", or "/foo"), or if the input is malformed and contains a double-slash such as "foo//boo".

---
 dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
dir.c +1 -1
index 3e345c2..9e5879a 100644
@@ -237,7 +237,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 			current = 0;
 		}
 		else {
-			cp = strchr(base + current + 1, '/');
+			cp = strchr(base + current, '/');
 			if (!cp)
 				die("oops in prep_exclude");
 			cp++;
Signature · 1 line
1.5.4.rc5.1.g813e
#10 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Hi,

Quoted 25 lines · Shawn Bohrer

In "struct exclude_stack", prep_exclude() and excluded(), the convention for a path is to express the length of directory part including the trailing slash (e.g. "foo" and "bar/baz" will get baselen=0 and baselen=4 respectively).

The variable current and parameter baselen follow that convention in the codepath the following patch touches.

                else {
                        cp = strchr(base + current + 1, '/');
                        if (!cp)
                                die("oops in prep_exclude");
                        cp++;
                }
                stk->prev = dir->exclude_stack;
                stk->baselen = cp - base;

is about coming up with the next value for current (which is taken from stk->baselen) to dig one more level.

If base="foo/bar/boo" and current=4 (i.e. we are looking at "foo/"), the current code (base + current + 1) will begin scanning for the next slash at ar/boo skipping one letter ('b'). This patch starts the scanning at bar/boo/

Quoted 7 lines

This only causes a problem when a path component has a length of zero which can happen when the user provides an absolute path to a file or directory in the root directory (i.e. "/", or "/foo"), or if the input is malformed and contains a double-slash such as "foo//boo".

I'll try to remember even 6 months from now that this was the "git clean -n /" problem ;-)

Ciao,
Dscho

#11 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Quoted 11 lines · Johannes Schindelin
Quoted 8 lines

...
This only causes a problem when a path component has a length of zero which can happen when the user provides an absolute path to a file or directory in the root directory (i.e. "/", or "/foo"), or if the input is malformed and contains a double-slash such as "foo//boo".

I'll try to remember even 6 months from now that this was the "git clean -n /" problem ;-)

Actually the quoted part of the message clearly tells that the patch is touching the wrong code. It should not blame the user but the caller of the function that did not check such an input, which is where the fix should be in.

#12 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

In reply to Shawn Bohrer
Quoted 8 lines · Shawn Bohrer

Nope the problem Johannes Sixt was having was that he mistakenly ran

git clean -n /*foo

Now that isn't what he meant to do, but I figured it might be possible that someone has their whole filesystem in a git repository, or maybe is using some sort of chroot on their repository. Your malformed paths guess is probably much more likely to occur.

That is not a user error from the syntax point of view (although it might be from the semantics point of view). I think the caller of the excluded() function (that is probably somewhere in builtin-clean.c -- I did not check) is responsible for not supplying such a path to the called function.

#13 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Shawn Bohrer

Nope the problem Johannes Sixt was having was that he mistakenly ran

git clean -n /*foo

Now that isn't what he meant to do, but I figured it might be possible that someone has their whole filesystem in a git repository, or maybe is using some sort of chroot on their repository. Your malformed paths guess is probably much more likely to occur.

Quoted 5 lines

That is not a user error from the syntax point of view (although it might be from the semantics point of view). I think the caller of the excluded() function (that is probably somewhere in builtin-clean.c -- I did not check) is responsible for not supplying such a path to the called function.

The "problem" is not only with git-clean, but also in others, like git-ls-files. Try this in you favorite repository:

   $ git ls-files -o /*bin

The output does not make a lot of sense. (Here it lists the contents of /bin and /sbin.) Not that it hurts with ls-files, but

   $ git clean -f /

is basically a synonym for

   $ rm -rf /

-- Hannes

#14 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Quoted 5 lines · Johannes Sixt

The "problem" is not only with git-clean, but also in others, like git-ls-files. Try this in you favorite repository:

   $ git ls-files -o /*bin
Quoted 8 lines

The output does not make a lot of sense. (Here it lists the contents of /bin and /sbin.) Not that it hurts with ls-files, but

   $ git clean -f /

is basically a synonym for

   $ rm -rf /

Yeah, /*bin is not inside the repository so it should not even be reported as "others". Shouldn't the commands detect this and reject feeding such paths outside the work tree to the core, which always expect you to talk about paths inside?

#15 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Quoted 12 lines · Junio C Hamano
Quoted 10 lines · Johannes Sixt

The "problem" is not only with git-clean, but also in others, like git-ls-files. Try this in you favorite repository:

   $ git ls-files -o /*bin

The output does not make a lot of sense. (Here it lists the contents of /bin and /sbin.) Not that it hurts with ls-files, but

   $ git clean -f /
Quoted 8 lines
Quoted 3 lines

is basically a synonym for

   $ rm -rf /

Yeah, /*bin is not inside the repository so it should not even be reported as "others". Shouldn't the commands detect this and reject feeding such paths outside the work tree to the core, which always expect you to talk about paths inside?

That's what I had expected. But look:

   $ git ls-files -o /
   [... tons of file names ...]
   $ git ls-files -o ..
   fatal: '..' is outside repository
   $ git clean -n /    # with Shawn's patch
   Would remove /bin/
   [... etc ...]
   $ git clean -n ..
   fatal: '..' is outside repository

Some mechanism for this is already there; it's just not complete enough.

-- Hannes

#16 lore raw

Re: [PATCH] Fix off by one error in prep_exclude.

Quoted 10 lines · Johannes Sixt

Junio C Hamano schrieb:
...

Quoted 4 lines

Yeah, /*bin is not inside the repository so it should not even be reported as "others". Shouldn't the commands detect this and reject feeding such paths outside the work tree to the core, which always expect you to talk about paths inside?

That's what I had expected. But look:
...
Some mechanism for this is already there; it's just not complete enough.

Exactly. That was what I've been getting at from the beginning of this thread --- fix for that incompleteness is what's needed.

#17 lore raw

[RFH/PATCH] prefix_path(): disallow absolute paths

In reply to Johannes Sixt

Without this fix, "git ls-files --others /" would list _all_ files, except for those tracked in the current repository. Worse, "git clean /" would start removing them.

Noticed by Johannes Sixt.

Incidentally, it fixes some strange code in builtin-mv.c by yours truly, where a slash was added to "dst" but then ignored, and instead taken from the source path. This triggered the new check for absolute paths.

---
On Mon, 28 Jan 2008, Johannes Sixt wrote:

> Junio C Hamano schrieb:
> > Johannes Sixt <[email protected]> writes:
> >
> >> The "problem" is not only with git-clean, but also in others, > >> like git-ls-files. Try this in you favorite repository:
> >>
> >> $ git ls-files -o /*bin
> >>
> >> The output does not make a lot of sense. (Here it lists the > >> contents of /bin and /sbin.) Not that it hurts with ls-files, > >> but
> >>
> >> $ git clean -f /
> >>
> >> is basically a synonym for
> >>
> >> $ rm -rf /
> >
> > Yeah, /*bin is not inside the repository so it should not even > > be reported as "others". Shouldn't the commands detect this > > and reject feeding such paths outside the work tree to the > > core, which always expect you to talk about paths inside? >
> That's what I had expected. But look:
>
> $ git ls-files -o /
> [... tons of file names ...]
>
> $ git ls-files -o ..
> fatal: '..' is outside repository
>
> $ git clean -n / # with Shawn's patch
> Would remove /bin/
> [... etc ...]
>
> $ git clean -n ..
> fatal: '..' is outside repository
>
> Some mechanism for this is already there; it's just not complete > enough.

This patch cannot be applied as-is: t3101 is failing (t7001 is fixed by the builtin-mv.c part).

The failure of t3101 has something to do with ls-tree filtering out invalid paths; I maintain that this behaviour is wrong to begin with.

So the help I am requesting is this: so late in the game for 1.5.4 I would hate to introduce a change in prefix_path(), because it affects apparently too much. However, the "git clean /" bug is a real one, and should at least be prevented. What to do?

 builtin-mv.c |    4 ++--
 setup.c      |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)
builtin-mv.c +2 -2
index 990e213..94f6dd2 100644
@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 
 				dst = add_slash(dst);
-				dst_len = strlen(dst) - 1;
+				dst_len = strlen(dst);
 
 				for (j = 0; j < last - first; j++) {
 					const char *path =
@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 					source[argc + j] = path;
 					destination[argc + j] =
 						prefix_path(dst, dst_len,
-							path + length);
+							path + length + 1);
 					modes[argc + j] = INDEX;
 				}
 				argc += last - first;
setup.c +2 -0
index 2174e78..5a4aadc 100644
@@ -13,6 +13,8 @@ const char *get_current_prefix()
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
+	if (is_absolute_path(path))
+		die("no absolute paths allowed: '%s'", path);
 	for (;;) {
 		char c;
 		if (*path != '.')
Signature · 1 line
1.5.4.rc5.15.g8231f
#18 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Quoted 21 lines · Johannes Schindelin

Without this fix, "git ls-files --others /" would list _all_ files, except for those tracked in the current repository. Worse, "git clean /" would start removing them.
...
This patch cannot be applied as-is: t3101 is failing (t7001 is fixed by the builtin-mv.c part).

The failure of t3101 has something to do with ls-tree filtering out invalid paths; I maintain that this behaviour is wrong to begin with.

So the help I am requesting is this: so late in the game for 1.5.4 I would hate to introduce a change in prefix_path(), because it affects apparently too much. However, the "git clean /" bug is a real one, and should at least be prevented. What to do?
...

setup.c +0 -0
index 2174e78..5a4aadc 100644

setup.c const char *get_current_prefix() · line 20

Quoted 8 lines
setup.c +2 -0
@@ -13,6 +13,8 @@ const char *get_current_prefix()
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
+	if (is_absolute_path(path))
+		die("no absolute paths allowed: '%s'", path);
 	for (;;) {
 		char c;
 		if (*path != '.')

If we are touching the prefix_path(), I think we should try to make its "ambiguous path rejection" more complete.

Currently, we:

- Remove "." path component (i.e. the directory leading part specified) from the input;

- Remove ".." path component and strip one level of the prefix;

only from the beginning. So if you give nonsense pathspec from the command line, you can end up calling prefix_path() with things like "/README", "/absolute/path/to//repository/tracked/file", and "fo//o/../o".

And not passing such ambiguous path like "fo//o" to the core level but sanitizing matters. Then core level can always do memcmp() with "fo/o" to see they are talking about the same path.

I suspect that the right approach might be something like the attached patch. It introduces a version of prefix_path() that sanitizes path (but not prefix part, which comes from git itself and hopefully there should not be a need to sanitize it) while doing the prefixing. It also strips the leading absolute path to the repository by comparing it with the value of work_tree.

A few things to note.

* Your mv fix is rolled in.

* This allows you to name a in-repository file as pwd/file, or pwd//file (iow, double-slash is also sanitized). It may kill the bird in another thread nearby.

* get_pathspec() drops paths outside of repository, so the caller may end up getting a smaller number of paths than it originally gave it. If an existing caller expects the same number of paths to come back, it needs to be adjusted (I did not check). We could alternatively die() but I couldn't decide which one is a better behaviour.

This is not to be applied (especially before auditing the callers), but to be thought about. Although it passes all the tests...

 builtin-mv.c |    4 +-
 setup.c      |  152 ++++++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 112 insertions(+), 44 deletions(-)
builtin-mv.c +2 -2
index 990e213..94f6dd2 100644
@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 
 				dst = add_slash(dst);
-				dst_len = strlen(dst) - 1;
+				dst_len = strlen(dst);
 
 				for (j = 0; j < last - first; j++) {
 					const char *path =
@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 					source[argc + j] = path;
 					destination[argc + j] =
 						prefix_path(dst, dst_len,
-							path + length);
+							path + length + 1);
 					modes[argc + j] = INDEX;
 				}
 				argc += last - first;
setup.c +110 -42

180 lines not on this page · raw message

#19 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

setup.c line 15

Quoted 10 lines · Junio C Hamano
setup.c +8 -1
+static int sanitary_path_copy(char *dst, const char *src)
 {
-	const char *orig = path;
+	char *dst0 = dst;
+
+	if (*src == '/') {
+		*dst++ = '/';
+		while (*src == '/')
+			src++;
+	}

Advance notice: In this function, tests of the kind *src == '/' need to be turned into is_dir_sep(*src) when we port to Windows.

setup.c line 60

Quoted 9 lines
setup.c +4 -4
+		/* copy up to the next '/', and eat all '/' */
+		while ((c = *src++) != '\0' && c != '/')
+			*dst++ = c;
 		if (c == '/') {
-			path += 2;
-			continue;
-		}
-		if (c != '.')
+			*dst++ = c;
			*dst++ = '/';

will be needed on Windows to sanitize all is_dir_sep(c) to '/'.

Quoted 5 lines
setup.c +4 -0
+			while (c == '/')
+				c = *src++;
+			src--;
+		} else if (!c)
 			break;

...

setup.c line 94

Quoted 5 lines
setup.c +5 -0
+const char *prefix_path(const char *prefix, int len, const char *path)
+{
+	const char *orig = path;
+	char *sanitized = xmalloc(len + strlen(path) + 1);
+	if (*orig == '/')

if (is_absolute_path(*orig))

setup.c line 103

Quoted 10 lines
setup.c +8 -1
+		strcpy(sanitized, path);
+	else {
+		if (len)
+			memcpy(sanitized, prefix, len);
+		strcpy(sanitized + len, path);		
 	}
-	return path;
+	if (sanitary_path_copy(sanitized, sanitized))
+		goto error_out;
+	if (*orig == '/') {

Ditto.

setup.c line 115

Quoted 12 lines
setup.c +11 -0
+		const char *work_tree = get_git_work_tree();
+		size_t len = strlen(work_tree);
+		if (strncmp(sanitized, work_tree, len) ||
+		    (sanitized[len] != '\0' && sanitized[len] != '/')) {
+		error_out:
+			error("'%s' is outside repository", orig);
+			free(sanitized);
+			return NULL;
+		}
+	}
+	return sanitized;
 }

I appreciate this new sanitary_copy_path() because I expect that we will need at least one less #ifdef __MINGW32__/#endif compared to our current Windows port.

-- Hannes

#20 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Quoted 3 lines · Johannes Sixt

I appreciate this new sanitary_copy_path() because I expect that we will need at least one less #ifdef __MINGW32__/#endif compared to our current Windows port.

I would have expected that the whole function would have platform specific implementation to deal with Windows, not just ifdef sprinkled everywhere that says "Oh, on this platform directory separator is a backslash".

Especially I have no idea how would that drive lettter stuff would/should work. When you are in C:\Documents\Panda\, how would you express D:\Movie\Porn\My Favorite.mpg as a relative path?

#21 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Johannes Sixt

I appreciate this new sanitary_copy_path() because I expect that we will need at least one less #ifdef __MINGW32__/#endif compared to our current Windows port.

I would have expected that the whole function would have platform specific implementation to deal with Windows, not just ifdef sprinkled everywhere that says "Oh, on this platform directory separator is a backslash".

I agree. Therefore, we would have *one* conditionalized definition of is_dir_sep() upfront, and use that in place of c == '/'.

The #ifdef I'm addressing above is one that we had to introduce because in the old implementation of prefix_path() on Windows we had to rewrite the path more often than on *nix due to '\\' => '/' conversion. In your new implementation this rewriting now always takes place, but on *nix it more often turns out to be an identity operation.

Quoted 4 lines

Especially I have no idea how would that drive lettter stuff would/should work. When you are in C:\Documents\Panda\, how would you express D:\Movie\Porn\My Favorite.mpg as a relative path?

You can't. But when would this be necessary?

-- Hannes

#22 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Quoted 5 lines · Johannes Sixt

The #ifdef I'm addressing above is one that we had to introduce because in the old implementation of prefix_path() on Windows we had to rewrite the path more often than on *nix due to '\\' => '/' conversion. In your new implementation this rewriting now always takes place, but on *nix it more often turns out to be an identity operation.

I am not sure what you mean by "\\" => '/', but you are right. We should be able to optimize the common case of prefix=none and no special path components found in path, which should be an identity function.

Quoted 6 lines
Quoted 4 lines

Especially I have no idea how would that drive lettter stuff would/should work. When you are in C:\Documents\Panda\, how would you express D:\Movie\Porn\My Favorite.mpg as a relative path?

You can't. But when would this be necessary?

I don't know. That's why I asked.

#23 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

In reply to Junio C Hamano
Quoted 15 lines · Junio C Hamano

I suspect that the right approach might be something like the attached patch. It introduces a version of prefix_path() that sanitizes path (but not prefix part, which comes from git itself and hopefully there should not be a need to sanitize it) while doing the prefixing. It also strips the leading absolute path to the repository by comparing it with the value of work_tree.

A few things to note.

* Your mv fix is rolled in.

* This allows you to name a in-repository file as pwd/file, or pwd//file (iow, double-slash is also sanitized). It may kill the bird in another thread nearby.

Quoted 6 lines

* get_pathspec() drops paths outside of repository, so the caller may end up getting a smaller number of paths than it originally gave it. If an existing caller expects the same number of paths to come back, it needs to be adjusted (I did not check). We could alternatively die() but I couldn't decide which one is a better behaviour.

This is the kind of "fixing existing callers" that would be needed if we take this approach.

 builtin-ls-files.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)
builtin-ls-files.c +10 -1
index 0f0ab2d..3801cf4 100644
@@ -572,8 +572,17 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	pathspec = get_pathspec(prefix, argv + i);
 
 	/* Verify that the pathspec matches the prefix */
-	if (pathspec)
+	if (pathspec) {
+		if (argc != i) {
+			int cnt;
+			for (cnt = 0; pathspec[cnt]; cnt++)
+				;
+			if (cnt != (argc - i))
+				exit(1); /* error message already given */
+		}
 		prefix = verify_pathspec(prefix);
+	} else if (argc != i)
+		exit(1); /* error message already given */
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec && error_unmatch) {
#24 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Ok, the three patches from this afternoon, squashed together into one, seems to give us quite a nice property.

Here is an demonstration.

 t/t7010-setup.sh |  117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)
 create mode 100755 t/t7010-setup.sh
t/t7010-setup.sh new file +117 -0
new file mode 100755
index 0000000..da20ba5
@@ -0,0 +1,117 @@
+#!/bin/sh
+
+test_description='setup taking and sanitizing funny paths'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	mkdir -p a/b/c a/e &&
+	D=$(pwd) &&
+	>a/b/c/d &&
+	>a/e/f
+
+'
+
+test_expect_success 'git add (absolute)' '
+
+	git add "$D/a/b/c/d" &&
+	git ls-files >current &&
+	echo a/b/c/d >expect &&
+	diff -u expect current
+
+'
+
+
+test_expect_success 'git add (funny relative)' '
+
+	rm -f .git/index &&
+	(
+		cd a/b &&
+		git add "../e/./f"
+	) &&
+	git ls-files >current &&
+	echo a/e/f >expect &&
+	diff -u expect current
+
+'
+
+test_expect_success 'git rm (absolute)' '
+
+	rm -f .git/index &&
+	git add a &&
+	git rm -f --cached "$D/a/b/c/d" &&
+	git ls-files >current &&
+	echo a/e/f >expect &&
+	diff -u expect current
+
+'
+
+test_expect_success 'git rm (funny relative)' '
+
+	rm -f .git/index &&
+	git add a &&
+	(
+		cd a/b &&
+		git rm -f --cached "../e/./f"
+	) &&
+	git ls-files >current &&
+	echo a/b/c/d >expect &&
+	diff -u expect current
+
+'
+
+test_expect_success 'git ls-files (absolute)' '
+
+	rm -f .git/index &&
+	git add a &&
+	git ls-files "$D/a/e/../b" >current &&
+	echo a/b/c/d >expect &&
+	diff -u expect current
+
+'
+
+test_expect_success 'git ls-files (relative #1)' '
+
+	rm -f .git/index &&
+	git add a &&
+	(
+		cd a/b &&
+		git ls-files "../b/c"
+	)  >current &&
+	echo c/d >expect &&
+	diff -u expect current
+
+'
+
+test_expect_success 'git ls-files (relative #2)' '
+
+	rm -f .git/index &&
+	git add a &&
+	(
+		cd a/b &&
+		git ls-files --full-name "../e/f"
+	)  >current &&
+	echo a/e/f >expect &&
+	diff -u expect current
+
+'
+
+test_expect_success 'git ls-files (relative #3)' '
+
+	rm -f .git/index &&
+	git add a &&
+	(
+		cd a/b &&
+		if git ls-files "../e/f"
+		then
+			echo Gaah, should have failed
+			exit 1
+		else
+			: happy
+		fi
+	)
+
+'
+
+test_done
#25 lore raw

PATCH 1/1 setup: sanitize absolute and funny paths in get_pathspec()

The prefix_path() function called from get_pathspec() is responsible for translating list of user-supplied pathspecs to list of pathspecs that is relative to the root of the work tree. When working inside a subdirectory, the user-supplied pathspecs are taken to be relative to the current subdirectory.

Among special path components in pathspecs, we used to accept and interpret only "." ("the directory", meaning a no-op) and ".." ("up one level") at the beginning. Everything else was passed through as-is.

For example, if you are in Documentation/ directory of the project, you can name Documentation/howto/maintain-git.txt as:

    howto/maintain-git.txt
    ../Documentation/howto/maitain-git.txt
    ../././Documentation/howto/maitain-git.txt

but not as:

    howto/./maintain-git.txt
    $(pwd)/howto/maintain-git.txt

This patch updates prefix_path() in several ways:

- If the pathspec is not absolute, prefix (i.e. the current subdirectory relative to the root of the work tree, with terminating slash, if not empty) and the pathspec is concatenated first and used in the next step. Otherwise, that absolute pathspec is used in the next step.

- Then special path components "." (no-op) and ".." (up one level) are interpreted to simplify the path. It is an error to have too many ".." to cause the intermediate result to step outside of the input to this step.

- If the original pathspec was not absolute, the result from the previous step is the resulting "sanitized" pathspec. Otherwise, the result from the previous step is still absolute, and it is an error if it does not begin with the directory that corresponds to the root of the work tree. The directory is stripped away from the result and is returned.

- In any case, the resulting pathspec in the array get_pathspec() returns omit the ones that caused errors.

With this patch, the last two examples also behave as expected.

---

* This is a squash of the handful patches I sent out with some clean-ups, to conclude the day for me. Perhaps this will appear in 'pu' before 1.5.4 if I really am bored but not very likely. I am pretty much bogged down with day-job these days during the week...

 builtin-ls-files.c |   11 +++-
 builtin-mv.c       |    4 +-
 setup.c            |  158 ++++++++++++++++++++++++++++++++++++++--------------
 t/t7010-setup.sh   |  117 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 245 insertions(+), 45 deletions(-)
 create mode 100755 t/t7010-setup.sh
builtin-ls-files.c +10 -1
index 0f0ab2d..3801cf4 100644
@@ -572,8 +572,17 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	pathspec = get_pathspec(prefix, argv + i);
 
 	/* Verify that the pathspec matches the prefix */
-	if (pathspec)
+	if (pathspec) {
+		if (argc != i) {
+			int cnt;
+			for (cnt = 0; pathspec[cnt]; cnt++)
+				;
+			if (cnt != (argc - i))
+				exit(1); /* error message already given */
+		}
 		prefix = verify_pathspec(prefix);
+	} else if (argc != i)
+		exit(1); /* error message already given */
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec && error_unmatch) {
builtin-mv.c +2 -2
index 990e213..94f6dd2 100644
@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 
 				dst = add_slash(dst);
-				dst_len = strlen(dst) - 1;
+				dst_len = strlen(dst);
 
 				for (j = 0; j < last - first; j++) {
 					const char *path =
@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 					source[argc + j] = path;
 					destination[argc + j] =
 						prefix_path(dst, dst_len,
-							path + length);
+							path + length + 1);
 					modes[argc + j] = INDEX;
 				}
 				argc += last - first;
setup.c +116 -42

186 lines not on this page · raw message

t/t7010-setup.sh new file +117 -0

117 lines not on this page · raw message

Signature · 1 line
1.5.4.rc5
#40 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

In reply to Junio C Hamano

This is on top of the original "sanitary_path_copy()" patch to fix the case where the user gives pwd/foobar (or just pwd) to us. After making sure the leading part matches with the work tree, we need to strip that to make the result relative to the work tree.

 setup.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
setup.c +5 -0
index 156d417..0688e7b 100644
@@ -110,6 +110,9 @@ const char *prefix_path(const char *prefix, int len, const char *path)
 			free(sanitized);
 			return NULL;
 		}
+		sanitized += len;
+		if (*sanitized == '/')
+			sanitized++;
 	}
 	return sanitized;
 }
@@ -201,6 +204,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 		src++;
 	}
 	*dst = NULL;
+	if (!*pathspec)
+		return NULL;
 	return pathspec;
 }
 
#41 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

In reply to Junio C Hamano

Hi,

Quoted 2 lines · Junio C Hamano

If we are touching the prefix_path(), I think we should try to make its "ambiguous path rejection" more complete.

I should have made more clear that I tried to avoid exactly that before 1.5.4, I guess.

Quoted 2 lines

This is not to be applied (especially before auditing the callers), but to be thought about. Although it passes all the tests...

It certainly is tempting.

setup.c line 63

Quoted 3 lines
setup.c +3 -0
+			while (c == '/')
+				c = *src++;
+			src--;

This is ugly. I would like this better:

			while (src[1] == '/')
				src++;

setup.c line 93

Quoted 4 lines
setup.c +4 -0
+const char *prefix_path(const char *prefix, int len, const char *path)
+{
+	const char *orig = path;
+	char *sanitized = xmalloc(len + strlen(path) + 1);

There _has_ to be a way to avoid malloc()ing things that will _never_ be free()d again with every second patch ;-)

Ciao,
Dscho

#42 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Quoted 6 lines · Johannes Schindelin
Quoted 2 lines

This is not to be applied (especially before auditing the callers), but to be thought about. Although it passes all the tests...

It certainly is tempting.

Quoted 8 lines

setup.c line 63

Quoted 3 lines
setup.c +3 -0
+			while (c == '/')
+				c = *src++;
+			src--;

This is ugly. I would like this better:

			while (src[1] == '/')
				src++;

Whatever. That was just for discussion.

Quoted 7 lines

setup.c line 93

Quoted 4 lines
setup.c +4 -0
+const char *prefix_path(const char *prefix, int len, const char *path)
+{
+	const char *orig = path;
+	char *sanitized = xmalloc(len + strlen(path) + 1);

There _has_ to be a way to avoid malloc()ing things that will _never_ be free()d again with every second patch ;-)

Huh? prefix_path() already allocates for rewritten pathspec entries; this is nothing new.

#43 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Hi,

Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Johannes Schindelin

setup.c line 93

Quoted 4 lines
setup.c +4 -0
+const char *prefix_path(const char *prefix, int len, const char *path)
+{
+	const char *orig = path;
+	char *sanitized = xmalloc(len + strlen(path) + 1);

There _has_ to be a way to avoid malloc()ing things that will _never_ be free()d again with every second patch ;-)

Quoted 2 lines

Huh? prefix_path() already allocates for rewritten pathspec entries; this is nothing new.

Right. It is nothing new. Except that we now allocate for more paths than before.

At the same time as introducing a new feature (path normalisation), we could introduce another change, which would introduce a function cleanup_prefixed_pathspecs(), which would free all path that were malloc()ed.

Ciao,
Dscho

#46 lore raw

PATCH 1/1 prefix_path(): disallow absolute paths

In reply to Johannes Schindelin

Without this fix, "git ls-files --others /" would list _all_ files, except for those tracked in the current repository. Worse, "git clean /" would start removing them.

Noticed by Johannes Sixt.

Incidentally, it fixes some strange code in builtin-mv.c by yours truly, where a slash was added to "dst" but then ignored, and instead taken from the source path. This triggered the new check for absolute paths.

A test in t3101 started failing, too, because it tested ls-tree with not-really-absolute paths (expecting the leading "/" to be ignored). Those paths were changed to relative paths.

---

On Mon, 28 Jan 2008, Johannes Schindelin wrote:

> The failure of t3101 has something to do with ls-tree filtering > out invalid paths; I maintain that this behaviour is wrong to > begin with.

This patch fixes the test.

But as this fix illustrates, it is a change in semantics: where earlier

git ls-tree /README

was allowed, it is no longer.

Comments?

 builtin-mv.c               |    4 ++--
 setup.c                    |    2 ++
 t/t3101-ls-tree-dirname.sh |    2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)
builtin-mv.c +2 -2
index 990e213..94f6dd2 100644
@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 
 				dst = add_slash(dst);
-				dst_len = strlen(dst) - 1;
+				dst_len = strlen(dst);
 
 				for (j = 0; j < last - first; j++) {
 					const char *path =
@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 					source[argc + j] = path;
 					destination[argc + j] =
 						prefix_path(dst, dst_len,
-							path + length);
+							path + length + 1);
 					modes[argc + j] = INDEX;
 				}
 				argc += last - first;
setup.c +2 -0
index 2174e78..5a4aadc 100644
@@ -13,6 +13,8 @@ const char *get_current_prefix()
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
+	if (is_absolute_path(path))
+		die("no absolute paths allowed: '%s'", path);
 	for (;;) {
 		char c;
 		if (*path != '.')
t/t3101-ls-tree-dirname.sh +1 -1
index 39fe267..cc5b982 100755
@@ -120,7 +120,7 @@ EOF
 # having 1.txt and path3
 test_expect_success \
     'ls-tree filter odd names' \
-    'git ls-tree $tree 1.txt /1.txt //1.txt path3/1.txt /path3/1.txt //path3//1.txt path3 /path3/ path3// >current &&
+    'git ls-tree $tree 1.txt ./1.txt .//1.txt path3/1.txt ./path3/1.txt .//path3//1.txt path3 ./path3/ path3// >current &&
      cat >expected <<\EOF &&
 100644 blob X	1.txt
 100644 blob X	path3/1.txt
Signature · 1 line
1.5.4.rc5.15.g8231f
#26 lore raw

PATCH 1/1 Make blame accept absolute paths

In reply to Junio C Hamano

Blame did not always use prefix_path.

---
 builtin-blame.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

This is a followup to "setup: sanitize absolute and funny paths in get_pathspec()" Thanks Junio for fixing this wish of mine.

builtin-blame.c +1 -3
index c7e6887..f88c32a 100644
@@ -1894,9 +1894,7 @@ static unsigned parse_score(const char *arg)
 
 static const char *add_prefix(const char *prefix, const char *path)
 {
-	if (!prefix || !prefix[0])
-		return path;
-	return prefix_path(prefix, strlen(prefix), path);
+	return prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
 }
 
 /*
Signature · 1 line
1.5.4.rc4.25.g81cc
#27 lore raw

PATCH 1/1 More test cases for sanitized path names

In reply to Junio C Hamano

Verify a few more commands and pathname variants.

---
 t/t7010-setup.sh |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

These are a few testcases from my earlier attempt at this. The log and commit cases succeeded with Junios version, but not blame and some of the nastier versions for git add (same principle for all commands, just that I use add as an example)

-- robin

t/t7010-setup.sh +39 -0
index da20ba5..60c4a46 100755
@@ -114,4 +114,43 @@ test_expect_success 'git ls-files (relative #3)' '
 
 '
 
+test_expect_success 'commit using absolute path names' '
+	git commit -m "foo" &&
+	echo aa >>a/b/c/d &&
+	git commit -m "aa" "$(pwd)/a/b/c/d"
+'
+
+test_expect_success 'log using absolute path names' '
+	echo bb >>a/b/c/d &&
+	git commit -m "bb" $(pwd)/a/b/c/d &&
+
+	git log a/b/c/d >f1.txt &&
+	git log "$(pwd)/a/b/c/d" >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_success 'blame using absolute path names' '
+	git blame a/b/c/d >f1.txt &&
+	git blame "$(pwd)/a/b/c/d" >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'
+
+test_expect_failure 'add a file outside the work tree, nasty case 1' '(
+	f="$(pwd)x" &&
+	touch "$f" &&
+	git add "$f"
+)'
+
+test_expect_failure 'add a file outside the work tree, nasty case 2' '(
+	f="$(pwd|sed "s/.$//")x" &&
+	touch "$f" &&
+	git add "$f"
+)'
+
 test_done
Signature · 1 line
1.5.4.rc4.25.g81cc
#28 lore raw

Re: [PATCH] More test cases for sanitized path names

t/t7010-setup.sh test_expect_success 'git ls-files (relative #3)' ' · line 142

Quoted 5 lines · Robin Rosenberg
t/t7010-setup.sh +5 -0
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'

This test will always fail as the final exit status is that of "echo", which will exit with success and you are expecting a failure.

t/t7010-setup.sh test_expect_success 'git ls-files (relative #3)' ' · line 148

Quoted 5 lines
t/t7010-setup.sh +5 -0
+test_expect_failure 'add a file outside the work tree, nasty case 1' '(
+	f="$(pwd)x" &&
+	touch "$f" &&
+	git add "$f"
+)'

You are in the directory "t/trash", and try to add t/trashx, so this should fail and you would want to make sure it fails.

But this has a few problems:

* First, the obvious one. You are creating a garbage file outside of t/trash directory. Don't. If you need to, dig a test directory one level lower inside t/trash and play around there.

* In general you should stay away from test_expect_failure. If any of the command in && chain fails, it fails the whole thing, but you cannot tell if the sequence failed at the command you expected to fail or something else that is much earlier. For example, it may be that somebody created t/trashx file in the source tree that is read-only, and the comand that failed in the sequence could be 'touch' before the command you are testing.

Instead, write it like (after fixing it not to go outside t/trash):

	test_expect_success 'add a path outside repo (1)' '

		file=path_to_outside_repo &&
                touch "$file" &&
		! git add "$f"

	'

I'd like to make the _first_ patch after 1.5.4 to be a fix-up for tests that misuse test_expect_failure. After that, we can use test_expect_failure to mark tests that ought to pass but don't because of bugs in the commands. That way, people who are absolutely bored can grep for test_expect_failure to see what existing issues to tackle ;-).

#29 lore raw

Re: [PATCH] More test cases for sanitized path names

Quoted 4 lines · Junio C Hamano

I'd like to make the _first_ patch after 1.5.4 to be a fix-up for tests that misuse test_expect_failure. After that, we can use test_expect_failure to mark tests that ought to pass but don't because of bugs in the commands.

This is precisely what StGit's test suite does, and it has a very nice property (beyond looking tidy): it's possible to first commit a new (failing) test, and then commit a fix for the bug that made the test fail, and have the test suite pass at every step.

Signature · 2 lines
Karl Hasselström, [email protected]
      www.treskal.com/kalle
#30 lore raw

Re: [PATCH] More test cases for sanitized path names

In reply to Junio C Hamano

fredagen den 1 februari 2008 skrev Junio C Hamano:

Quoted 7 lines

Review

Quoted 5 lines · Robin Rosenberg
(quoted diff) +5 -0
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'

Oops. Remove the echo $?. It still fails, i.e. git add succeeds when it shouldn't. I was double checking it just before sending the patch.

Quoted 9 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+test_expect_failure 'add a file outside the work tree, nasty case 1' '(
+	f="$(pwd)x" &&
+	touch "$f" &&
+	git add "$f"
+)'

You are in the directory "t/trash", and try to add t/trashx, so this should fail and you would want to make sure it fails.

Quoted 6 lines

But this has a few problems:

* First, the obvious one. You are creating a garbage file outside of t/trash directory. Don't. If you need to, dig a test directory one level lower inside t/trash and play around there.

Can we move the default trash one level down for all tests? That would give us one free level to play with.

Quoted 1 line

* In general you should stay away from test_expect_failure. If

I respect that.
[...]

Quoted 6 lines

I'd like to make the _first_ patch after 1.5.4 to be a fix-up for tests that misuse test_expect_failure. After that, we can use test_expect_failure to mark tests that ought to pass but don't because of bugs in the commands. That way, people who are absolutely bored can grep for test_expect_failure to see what existing issues to tackle ;-).

If I recall things properly there are lots of test that test for success rather than checking that the command does what it should.

Update follows.

-- robin

Quoted 1 line

From 11a52821ca81096987f53c29bf1b9ce373fe7fd4 Mon Sep 17 00:00:00 2001

From: Robin Rosenberg <[email protected]>
Date: Fri, 1 Feb 2008 05:29:38 +0100
Subject: [PATCH] More test cases for sanitized path names

Verify a few more commands and pathname variants.

---
 t/t7010-setup.sh |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
t/t7010-setup.sh +42 -0
index da20ba5..ef30099 100755
@@ -4,6 +4,10 @@ test_description='setup taking and sanitizing funny paths'
 
 . ./test-lib.sh
 
+rm -rf .git
+test_create_repo repo
+cd repo
+
 test_expect_success setup '
 
 	mkdir -p a/b/c a/e &&
@@ -114,4 +118,42 @@ test_expect_success 'git ls-files (relative #3)' '
 
 '
 
+test_expect_success 'commit using absolute path names' '
+	git commit -m "foo" &&
+	echo aa >>a/b/c/d &&
+	git commit -m "aa" "$(pwd)/a/b/c/d"
+'
+
+test_expect_success 'log using absolute path names' '
+	echo bb >>a/b/c/d &&
+	git commit -m "bb" $(pwd)/a/b/c/d &&
+
+	git log a/b/c/d >f1.txt &&
+	git log "$(pwd)/a/b/c/d" >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_success 'blame using absolute path names' '
+	git blame a/b/c/d >f1.txt &&
+	git blame "$(pwd)/a/b/c/d" >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_success 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	! git add "$d1"
+'
+
+test_expect_success 'add a file outside the work tree, nasty case 1' '(
+	f="$(pwd)x" &&
+	touch "$f" &&
+	! git add "$f"
+)'
+
+test_expect_success 'add a file outside the work tree, nasty case 2' '(
+	f="$(pwd|sed "s/.$//")x" &&
+	touch "$f" &&
+	! git add "$f"
+)'
+
 test_done
Signature · 1 line
1.5.4.rc4.25.g81cc
#31 lore raw

Re: [PATCH] More test cases for sanitized path names

Quoted 8 lines · Robin Rosenberg
Quoted 5 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'

Oops. Remove the echo $?. It still fails, i.e. git add succeeds when it shouldn't. I was double checking it just before sending the patch.

Ah, you found breakages.

I could not quite tell what you meant by these tests with test_expect_failure, either they were misuse or "currently fails but they shouldn't". Coming up with a small reproducible failure case is 50% of solving the problem. That's very appreciated.

In any case, I think the large-ish test framework update patch I sent tonight should go in very early post 1.5.4 cycle, so plesae use the new-and-improved test_expect_failure to mark these reproducible failure cases. Also, there is no need for hurry for you to just send test cases without fixes. When I say I do not take patches early, I do mean it --- I do not even take _my_ own patches like the one you are following up on. I've sent quite a many of them, and I think some are on 'pu' or 'offcuts', but most are only in the list archive. If nobody cares deeply enough about them to test and resend with Tested-by: , I am not planning to go back to pick them up.

Quoted 7 lines
Quoted 4 lines

* First, the obvious one. You are creating a garbage file outside of t/trash directory. Don't. If you need to, dig a test directory one level lower inside t/trash and play around there.

Can we move the default trash one level down for all tests? That would give us one free level to play with.

I'd rather not. Most tests do not have to step outside and it is very handy to debug any breakage they find by always being able to go there with "cd t/trash".

#32 lore raw

Re: [PATCH] More test cases for sanitized path names

fredagen den 1 februari 2008 skrev Junio C Hamano:

Quoted 19 lines
Quoted 8 lines · Robin Rosenberg
Quoted 5 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'

Oops. Remove the echo $?. It still fails, i.e. git add succeeds when it shouldn't. I was double checking it just before sending the patch.

Ah, you found breakages.

I could not quite tell what you meant by these tests with test_expect_failure, either they were misuse or "currently fails but they shouldn't". Coming up with a small reproducible failure case is 50% of solving the problem. That's very appreciated.

Quoted 11 lines

In any case, I think the large-ish test framework update patch I sent tonight should go in very early post 1.5.4 cycle, so plesae use the new-and-improved test_expect_failure to mark these reproducible failure cases. Also, there is no need for hurry for you to just send test cases without fixes. When I say I do not take patches early, I do mean it --- I do not even take _my_ own patches like the one you are following up on. I've sent quite a many of them, and I think some are on 'pu' or 'offcuts', but most are only in the list archive. If nobody cares deeply enough about them to test and resend with Tested-by: , I am not planning to go back to pick them up.

I had those test cases on my machine from my earlier work on absolute path names so I just ran them with your code and extracted those that failed and put them into your testsuite instead. That's why I sent them so early. Read them as comments on your patches, "oh you need to cover this too". It was simply very convenient for me, and hopefull for you, to supply them in patch form.

The reasone my code for absolute path names wasn't re-submitted was because I had some test cases that didn't pass. I don't really care how the problem is solved.

Quoted 6 lines
Quoted 2 lines

Can we move the default trash one level down for all tests? That would give us one free level to play with.

I'd rather not. Most tests do not have to step outside and it is very handy to debug any breakage they find by always being able to go there with "cd t/trash".

The change would be to "cd t/trash/repo" instead. Not much different.

Quoted 8 lines

The updated get_pathspec() issues a warning message and returns the result that omits paths outside of the work tree. It does not die (and it is intentional, by the way). The callers that expect to always receive the same number of paths in the return value as argv+i they pass to get_pathspec() should be updated to notice that they got less than they passed in, if they care about this error condition, and --error-unmatch codepath is one of them. I did not touch that in the weatherbaloon patch.

Git add in the current version on an absolute path outside the repo fails, so I think the updated one should. It really doesn't make sense.

ls-files outside the repo doesn't make sense either, but then the current version exits with 0 so I can't make the same reference to existing behaviour there. It just might break someone's script.

-- robin

#33 lore raw

Re: [PATCH] More test cases for sanitized path names

Quoted 10 lines · Robin Rosenberg

fredagen den 1 februari 2008 skrev Junio C Hamano:

Quoted 8 lines

Ah, you found breakages.

I could not quite tell what you meant by these tests with test_expect_failure, either they were misuse or "currently fails but they shouldn't". Coming up with a small reproducible failure case is 50% of solving the problem. That's very appreciated.
...

Quoted 4 lines

I had those test cases on my machine from my earlier work on absolute path names so I just ran them with your code and extracted those that failed and put them into your testsuite instead. That's why I sent them so early...

Yeah, I initially did not realize that was what you were doing, hence the above "I could not quite tell, ... but thanks now I got it".

#34 lore raw

Re: [PATCH] More test cases for sanitized path names

In reply to Junio C Hamano
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Robin Rosenberg
Quoted 5 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'

Oops. Remove the echo $?. It still fails, i.e. git add succeeds when it shouldn't. I was double checking it just before sending the patch.

Quoted 1 line

Ah, you found breakages.

I haven't looked at the code, but I suspect that "git add" and anything that uses the same logic as "ls-files --error-unmatch" would still not work with the setup patch.

The updated get_pathspec() issues a warning message and returns the result that omits paths outside of the work tree. It does not die (and it is intentional, by the way). The callers that expect to always receive the same number of paths in the return value as argv+i they pass to get_pathspec() should be updated to notice that they got less than they passed in, if they care about this error condition, and --error-unmatch codepath is one of them. I did not touch that in the weatherbaloon patch.

#35 lore raw

Re: [PATCH] More test cases for sanitized path names

Quoted 3 lines · Junio C Hamano

I haven't looked at the code, but I suspect that "git add" and anything that uses the same logic as "ls-files --error-unmatch" would still not work with the setup patch.

Ok, I looked at the code.

I already had a fix for "ls-files --error-unmatch" in the weatherbaloon patch. The logic to complain and fail nonsense paths was needed for "add".

The attached is on top of the previous one and your test case updates. We may want to also add tests for --error-unmatch.

It is very tempting to enhance pathspec API in such a way that it is not just a NULL terminated array of (char*) pointers, but a pointer to a richer structure that knows the number of elements and which strings need to be freed (and we would invent a "free_pathspec()" function to free them) and such, but that is an independent surgery that is outside of the scope of flying weatherbaloons.

---

 builtin-add.c    |   12 ++++++++++++
 t/t7010-setup.sh |   20 ++++++++++++++------
 2 files changed, 26 insertions(+), 6 deletions(-)
builtin-add.c +12 -0
index 4a91e3e..820110e 100644
@@ -228,6 +228,18 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		goto finish;
 	}
 
+	if (*argv) {
+		/* Was there an invalid path? */
+		if (pathspec) {
+			int num;
+			for (num = 0; pathspec[num]; num++)
+				; /* just counting */
+			if (argc != num)
+				exit(1); /* error message already given */
+		} else
+			exit(1); /* error message already given */
+	}
+
 	fill_directory(&dir, pathspec, ignored_too);
 
 	if (show_only) {
t/t7010-setup.sh +14 -6
index 60c4a46..e809e0e 100755
@@ -135,20 +135,28 @@ test_expect_success 'blame using absolute path names' '
 	diff -u f1.txt f2.txt
 '
 
-test_expect_failure 'add a directory outside the work tree' '
+test_expect_success 'setup deeper work tree' '
+	test_create_repo tester
+'
+
+test_expect_success 'add a directory outside the work tree' '(
+	cd tester &&
 	d1="$(cd .. ; pwd)" &&
 	git add "$d1"
-	echo $?
-'
+)'
 
-test_expect_failure 'add a file outside the work tree, nasty case 1' '(
+test_expect_success 'add a file outside the work tree, nasty case 1' '(
+	cd tester &&
 	f="$(pwd)x" &&
+	echo "$f" &&
 	touch "$f" &&
 	git add "$f"
 )'
 
-test_expect_failure 'add a file outside the work tree, nasty case 2' '(
-	f="$(pwd|sed "s/.$//")x" &&
+test_expect_success 'add a file outside the work tree, nasty case 2' '(
+	cd tester &&
+	f="$(pwd | sed "s/.$//")x" &&
+	echo "$f" &&
 	touch "$f" &&
 	git add "$f"
 )'
#36 lore raw

PATCH 1/1 Sane use of test_expect_failure

In reply to Junio C Hamano

Originally, test_expect_failure was designed to be the opposite of test_expect_success, but this was a bad decision. Most tests run a series of commands that leads to the single command that needs to be tested, like this:

    test_expect_{success,failure} 'test title' '
	setup1 &&
        setup2 &&
        setup3 &&
        what is to be tested
    '

And expecting a failure exit from the whole sequence misses the point of writing tests. Your setup$N that are supposed to succeed may have failed without even reaching what you are trying to test. The only valid use of test_expect_failure is to check a trivial single command that is expected to fail, which is a minority in tests of Porcelain-ish commands.

This large-ish patch rewrites all uses of test_expect_failure to use test_expect_success and rewrites the condition of what is tested, like this:

    test_expect_success 'test title' '
	setup1 &&
        setup2 &&
        setup3 &&
        ! this command should fail
    '

test_expect_failure is redefined to serve as a reminder that that test *should* succeed but due to a known breakage in git it currently does not pass. So if git-foo command should create a file 'bar' but you discovered a bug that it doesn't, you can write a test like this:

    test_expect_failure 'git-foo should create bar' '
        rm -f bar &&
        git foo &&
        test -f bar
    '

This construct acts similar to test_expect_success, but instead of reporting "ok/FAIL" like test_expect_success does, the outcome is reported as "FIXED/still broken".

---

Quoted 6 lines · Junio C Hamano

I'd like to make the _first_ patch after 1.5.4 to be a fix-up for tests that misuse test_expect_failure. After that, we can use test_expect_failure to mark tests that ought to pass but don't because of bugs in the commands. That way, people who are absolutely bored can grep for test_expect_failure to see what existing issues to tackle ;-).

This turned out to be a huge patch. I tried to be careful by keeping the conversion mostly mechanical, but I am not sure about some of the git-svn and git-cvsserver tests, some of which may already have been using test_expect_failure to mark a known breakage.

Eyeballing by area experts are very much appreciated.

 t/README                                  |   14 +--
 t/t0000-basic.sh                          |   30 ++++--
 t/t0030-stripspace.sh                     |   40 ++++----
 t/t0040-parse-options.sh                  |    4 +-
 t/t1000-read-tree-m-3way.sh               |  161 ++++++++++++++++-------------
 t/t1200-tutorial.sh                       |    8 +-
 t/t1300-repo-config.sh                    |   39 ++++---
 t/t1302-repo-version.sh                   |    5 +-
 t/t1400-update-ref.sh                     |   24 ++--
 t/t2000-checkout-cache-clash.sh           |    4 +-
 t/t2002-checkout-cache-u.sh               |    4 +-
 t/t2008-checkout-subdir.sh                |   16 ++--
 t/t2100-update-cache-badpath.sh           |    4 +-
 t/t3020-ls-files-error-unmatch.sh         |    4 +-
 t/t3200-branch.sh                         |   36 ++++---
 t/t3210-pack-refs.sh                      |   26 +++---
 t/t3400-rebase.sh                         |    4 +-
 t/t3403-rebase-skip.sh                    |    6 +-
 t/t3600-rm.sh                             |   17 ++--
 t/t4103-apply-binary.sh                   |   68 +++++++------
 t/t4113-apply-ending.sh                   |    8 +-
 t/t5300-pack-object.sh                    |    4 +-
 t/t5302-pack-index.sh                     |   32 +++---
 t/t5401-update-hooks.sh                   |    8 +-
 t/t5402-post-merge-hook.sh                |    4 +-
 t/t5500-fetch-pack.sh                     |    4 +-
 t/t5510-fetch.sh                          |   12 +-
 t/t5530-upload-pack-error.sh              |   14 +--
 t/t5600-clone-fail-cleanup.sh             |   12 +-
 t/t5710-info-alternate.sh                 |   14 ++-
 t/t6023-merge-file.sh                     |   12 +-
 t/t6024-recursive-merge.sh                |    2 +-
 t/t6025-merge-symlinks.sh                 |   21 ++--
 t/t6101-rev-parse-parents.sh              |    2 +-
 t/t6300-for-each-ref.sh                   |    8 +-
 t/t7001-mv.sh                             |    4 +-
 t/t7002-grep.sh                           |    4 +-
 t/t7004-tag.sh                            |   36 +++---
 t/t7101-reset.sh                          |   24 ++--
 t/t7501-commit.sh                         |   40 ++++----
 t/t7503-pre-commit-hook.sh                |    4 +-
 t/t7504-commit-msg-hook.sh                |    8 +-
 t/t9100-git-svn-basic.sh                  |   28 +++---
 t/t9106-git-svn-commit-diff-clobber.sh    |   18 ++--
 t/t9106-git-svn-dcommit-clobber-series.sh |    4 +-
 t/t9300-fast-import.sh                    |   24 ++--
 t/t9400-git-cvsserver-server.sh           |   30 +++--
 t/test-lib.sh                             |   30 +++++-
 48 files changed, 498 insertions(+), 427 deletions(-)
t/README +6 -8
index 36f2517..73ed11b 100644
@@ -160,14 +160,12 @@ library for your script to use.
 
  - test_expect_failure <message> <script>
 
-   This is the opposite of test_expect_success.  If <script>
-   yields success, test is considered a failure.
-
-   Example:
-
-	test_expect_failure \
-	    'git-update-index without --add should fail adding.' \
-	    'git-update-index should-be-empty'
+   This is NOT the opposite of test_expect_success, but is used
+   to mark a test that demonstrates a known breakage.  Unlike
+   the usual test_expect_success tests, which say "ok" on
+   success and "FAIL" on failure, this will say "FIXED" on
+   success and "still broken" on failure.  Failures from these
+   tests won't cause -i (immediate) to stop.
 
  - test_debug <script>
 
t/t0000-basic.sh +21 -9
index 4e49d59..cd0de50 100755
@@ -47,12 +47,24 @@ test_expect_success \
     'test $(wc -l < full-of-directories) = 3'
 
 ################################################################
+# Test harness
+test_expect_success 'success is reported like this' '
+    :
+'
+test_expect_failure 'pretend we have a known breakage' '
+    false
+'
+test_expect_failure 'pretend we have fixed a known breakage' '
+    :
+'
+
+################################################################
 # Basics of the basics
 
 # updating a new file without --add should fail.
-test_expect_failure \
-    'git update-index without --add should fail adding.' \
-    'git update-index should-be-empty'
+test_expect_success 'git update-index without --add should fail adding.' '
+    ! git update-index should-be-empty
+'
 
 # and with --add it should succeed, even if it is empty (it used to fail).
 test_expect_success \
@@ -70,9 +82,9 @@ test_expect_success \
 
 # Removing paths.
 rm -f should-be-empty full-of-directories
-test_expect_failure \
-    'git update-index without --remove should fail removing.' \
-    'git update-index should-be-empty'
+test_expect_success 'git update-index without --remove should fail removing.' '
+    ! git update-index should-be-empty
+'
 
 test_expect_success \
     'git update-index with --remove should be able to remove.' \
@@ -204,9 +216,9 @@ test_expect_success \
     'put invalid objects into the index.' \
     'git update-index --index-info < badobjects'
 
-test_expect_failure \
-    'writing this tree without --missing-ok.' \
-    'git write-tree'
+test_expect_success 'writing this tree without --missing-ok.' '
+    ! git write-tree
+'
 
 test_expect_success \
     'writing this tree with --missing-ok.' \
t/t0030-stripspace.sh +20 -20
index cad95f3..818c862 100755
@@ -243,14 +243,14 @@ test_expect_success \
     test `printf "$ttt$sss$sss$sss" | git stripspace | wc -l` -gt 0
 '
 
-test_expect_failure \
+test_expect_success \
     'text plus spaces without newline at end should not show spaces' '
-    printf "$ttt$sss" | git stripspace | grep -q "  " ||
-    printf "$ttt$ttt$sss" | git stripspace | grep -q "  " ||
-    printf "$ttt$ttt$ttt$sss" | git stripspace | grep -q "  " ||
-    printf "$ttt$sss$sss" | git stripspace | grep -q "  " ||
-    printf "$ttt$ttt$sss$sss" | git stripspace | grep -q "  " ||
-    printf "$ttt$sss$sss$sss" | git stripspace | grep -q "  "
+    ! (printf "$ttt$sss" | git stripspace | grep -q "  ") &&
+    ! (printf "$ttt$ttt$sss" | git stripspace | grep -q "  ") &&
+    ! (printf "$ttt$ttt$ttt$sss" | git stripspace | grep -q "  ") &&
+    ! (printf "$ttt$sss$sss" | git stripspace | grep -q "  ") &&
+    ! (printf "$ttt$ttt$sss$sss" | git stripspace | grep -q "  ") &&
+    ! (printf "$ttt$sss$sss$sss" | git stripspace | grep -q "  ")
 '
 
 test_expect_success \
@@ -280,14 +280,14 @@ test_expect_success \
     git diff expect actual
 '
 
-test_expect_failure \
+test_expect_success \
     'text plus spaces at end should not show spaces' '
-    echo "$ttt$sss" | git stripspace | grep -q "  " ||
-    echo "$ttt$ttt$sss" | git stripspace | grep -q "  " ||
-    echo "$ttt$ttt$ttt$sss" | git stripspace | grep -q "  " ||
-    echo "$ttt$sss$sss" | git stripspace | grep -q "  " ||
-    echo "$ttt$ttt$sss$sss" | git stripspace | grep -q "  " ||
-    echo "$ttt$sss$sss$sss" | git stripspace | grep -q "  "
+    ! (echo "$ttt$sss" | git stripspace | grep -q "  ") &&
+    ! (echo "$ttt$ttt$sss" | git stripspace | grep -q "  ") &&
+    ! (echo "$ttt$ttt$ttt$sss" | git stripspace | grep -q "  ") &&
+    ! (echo "$ttt$sss$sss" | git stripspace | grep -q "  ") &&
+    ! (echo "$ttt$ttt$sss$sss" | git stripspace | grep -q "  ") &&
+    ! (echo "$ttt$sss$sss$sss" | git stripspace | grep -q "  ")
 '
 
 test_expect_success \
@@ -339,13 +339,13 @@ test_expect_success \
     git diff expect actual
 '
 
-test_expect_failure \
+test_expect_success \
     'spaces without newline at end should not show spaces' '
-    printf "" | git stripspace | grep -q " " ||
-    printf "$sss" | git stripspace | grep -q " " ||
-    printf "$sss$sss" | git stripspace | grep -q " " ||
-    printf "$sss$sss$sss" | git stripspace | grep -q " " ||
-    printf "$sss$sss$sss$sss" | git stripspace | grep -q " "
+    ! (printf "" | git stripspace | grep -q " ") &&
+    ! (printf "$sss" | git stripspace | grep -q " ") &&
+    ! (printf "$sss$sss" | git stripspace | grep -q " ") &&
+    ! (printf "$sss$sss$sss" | git stripspace | grep -q " ") &&
+    ! (printf "$sss$sss$sss$sss" | git stripspace | grep -q " ")
 '
 
 test_expect_success \
t/t0040-parse-options.sh +2 -2
index 0a3b55d..2ecc283 100755
@@ -87,9 +87,9 @@ test_expect_success 'unambiguously abbreviated option with "="' '
 	git diff expect output
 '
 
-test_expect_failure 'ambiguously abbreviated option' '
+test_expect_success 'ambiguously abbreviated option' '
 	test-parse-options --strin 123;
-        test $? != 129
+        test $? = 129
 '
 
 cat > expect << EOF
t/t1000-read-tree-m-3way.sh +89 -72

279 lines not on this page · raw message

t/t1200-tutorial.sh +5 -3
index 991d3c5..dcb3108 100755
@@ -101,8 +101,8 @@ echo "Play, play, play" >>hello
 echo "Lots of fun" >>example
 git commit -m 'Some fun.' -i hello example
 
-test_expect_failure 'git resolve now fails' '
-	git merge -m "Merge work in mybranch" mybranch
+test_expect_success 'git resolve now fails' '
+	! git merge -m "Merge work in mybranch" mybranch
 '
 
 cat > hello << EOF
@@ -156,6 +156,8 @@ test_expect_success 'git show-branch' 'cmp show-branch2.expect show-branch2.outp
 
 test_expect_success 'git repack' 'git repack'
 test_expect_success 'git prune-packed' 'git prune-packed'
-test_expect_failure '-> only packed objects' 'find -type f .git/objects/[0-9a-f][0-9a-f]'
+test_expect_success '-> only packed objects' '
+	! find -type f .git/objects/[0-9a-f][0-9a-f]
+'
 
 test_done
t/t1300-repo-config.sh +23 -16
index 42eac2a..a786c5c 100755
@@ -181,8 +181,9 @@ test_expect_success 'non-match' \
 test_expect_success 'non-match value' \
 	'test wow = $(git config --get nextsection.nonewline !for)'
 
-test_expect_failure 'ambiguous get' \
-	'git config --get nextsection.nonewline'
+test_expect_success 'ambiguous get' '
+	! git config --get nextsection.nonewline
+'
 
 test_expect_success 'get multivar' \
 	'git config --get-all nextsection.nonewline'
@@ -202,13 +203,17 @@ EOF
 
 test_expect_success 'multivar replace' 'cmp .git/config expect'
 
-test_expect_failure 'ambiguous value' 'git config nextsection.nonewline'
+test_expect_success 'ambiguous value' '
+	! git config nextsection.nonewline
+'
 
-test_expect_failure 'ambiguous unset' \
-	'git config --unset nextsection.nonewline'
+test_expect_success 'ambiguous unset' '
+	! git config --unset nextsection.nonewline
+'
 
-test_expect_failure 'invalid unset' \
-	'git config --unset somesection.nonewline'
+test_expect_success 'invalid unset' '
+	! git config --unset somesection.nonewline
+'
 
 git config --unset nextsection.nonewline "wow3$"
 
@@ -224,7 +229,7 @@ EOF
 
 test_expect_success 'multivar unset' 'cmp .git/config expect'
 
-test_expect_failure 'invalid key' 'git config inval.2key blabla'
+test_expect_success 'invalid key' '! git config inval.2key blabla'
 
 test_expect_success 'correct key' 'git config 123456.a123 987'
 
@@ -382,8 +387,9 @@ EOF
 
 test_expect_success "rename succeeded" "git diff expect .git/config"
 
-test_expect_failure "rename non-existing section" \
-	'git config --rename-section branch."world domination" branch.drei'
+test_expect_success "rename non-existing section" '
+	! git config --rename-section branch."world domination" branch.drei
+'
 
 test_expect_success "rename succeeded" "git diff expect .git/config"
 
@@ -494,14 +500,14 @@ test_expect_success bool '
         done &&
 	cmp expect result'
 
-test_expect_failure 'invalid bool (--get)' '
+test_expect_success 'invalid bool (--get)' '
 
 	git config bool.nobool foobar &&
-	git config --bool --get bool.nobool'
+	! git config --bool --get bool.nobool'
 
-test_expect_failure 'invalid bool (set)' '
+test_expect_success 'invalid bool (set)' '
 
-	git config --bool bool.nobool foobar'
+	! git config --bool bool.nobool foobar'
 
 rm .git/config
 
@@ -562,8 +568,9 @@ EOF
 
 test_expect_success 'quoting' 'cmp .git/config expect'
 
-test_expect_failure 'key with newline' 'git config key.with\\\
-newline 123'
+test_expect_success 'key with newline' '
+	! git config "key.with
+newline" 123'
 
 test_expect_success 'value with newline' 'git config key.sub value.with\\\
 newline'
t/t1302-repo-version.sh +3 -2
index 37fc1c8..9be0770 100755
@@ -40,7 +40,8 @@ test_expect_success 'gitdir required mode on normal repos' '
 	(git apply --check --index test.patch &&
 	cd test && git apply --check --index ../test.patch)'
 
-test_expect_failure 'gitdir required mode on unsupported repo' '
-	(cd test2 && git apply --check --index ../test.patch)'
+test_expect_success 'gitdir required mode on unsupported repo' '
+	(cd test2 && ! git apply --check --index ../test.patch)
+'
 
 test_done
t/t1400-update-ref.sh +12 -12
index 71ab2dd..78cd412 100755
@@ -51,23 +51,23 @@ test_expect_success \
 	 test $B"' = $(cat .git/'"$m"')'
 rm -f .git/$m
 
-test_expect_failure \
-	'(not) create HEAD with old sha1' \
-	"git update-ref HEAD $A $B"
-test_expect_failure \
-	"(not) prior created .git/$m" \
-	"test -f .git/$m"
+test_expect_success '(not) create HEAD with old sha1' "
+	! git update-ref HEAD $A $B
+"
+test_expect_success "(not) prior created .git/$m" "
+	! test -f .git/$m
+"
 rm -f .git/$m
 
 test_expect_success \
 	"create HEAD" \
 	"git update-ref HEAD $A"
-test_expect_failure \
-	'(not) change HEAD with wrong SHA1' \
-	"git update-ref HEAD $B $Z"
-test_expect_failure \
-	"(not) changed .git/$m" \
-	"test $B"' = $(cat .git/'"$m"')'
+test_expect_success '(not) change HEAD with wrong SHA1' "
+	! git update-ref HEAD $B $Z
+"
+test_expect_success "(not) changed .git/$m" "
+	! test $B"' = $(cat .git/'"$m"')
+'
 rm -f .git/$m
 
 : a repository with working tree always has reflog these days...
t/t2000-checkout-cache-clash.sh +2 -2
index ac84335..5141fab 100755
@@ -36,9 +36,9 @@ mkdir path0
 date >path0/file0
 date >path1
 
-test_expect_failure \
+test_expect_success \
     'git checkout-index without -f should fail on conflicting work tree.' \
-    'git checkout-index -a'
+    '! git checkout-index -a'
 
 test_expect_success \
     'git checkout-index with -f should succeed.' \
t/t2002-checkout-cache-u.sh +2 -2
index f7a0055..0f441bc 100755
@@ -16,12 +16,12 @@ echo frotz >path0 &&
 git update-index --add path0 &&
 t=$(git write-tree)'
 
-test_expect_failure \
+test_expect_success \
 'without -u, git checkout-index smudges stat information.' '
 rm -f path0 &&
 git read-tree $t &&
 git checkout-index -f -a &&
-git diff-files | diff - /dev/null'
+! git diff-files | diff - /dev/null'
 
 test_expect_success \
 'with -u, git checkout-index picks up stat information from new files.' '
t/t2008-checkout-subdir.sh +8 -8
index f78945e..4a723dc 100755
@@ -67,16 +67,16 @@ test_expect_success 'checkout with simple prefix' '
 
 '
 
-test_expect_failure 'relative path outside tree should fail' \
-	'git checkout HEAD -- ../../Makefile'
+test_expect_success 'relative path outside tree should fail' \
+	'! git checkout HEAD -- ../../Makefile'
 
-test_expect_failure 'incorrect relative path to file should fail (1)' \
-	'git checkout HEAD -- ../file0'
+test_expect_success 'incorrect relative path to file should fail (1)' \
+	'! git checkout HEAD -- ../file0'
 
-test_expect_failure 'incorrect relative path should fail (2)' \
-	'( cd dir1 && git checkout HEAD -- ./file0 )'
+test_expect_success 'incorrect relative path should fail (2)' \
+	'( cd dir1 && ! git checkout HEAD -- ./file0 )'
 
-test_expect_failure 'incorrect relative path should fail (3)' \
-	'( cd dir1 && git checkout HEAD -- ../../file0 )'
+test_expect_success 'incorrect relative path should fail (3)' \
+	'( cd dir1 && ! git checkout HEAD -- ../../file0 )'
 
 test_done
t/t2100-update-cache-badpath.sh +2 -2
index 04a1ed1..9beaecd 100755
@@ -44,8 +44,8 @@ date >path1/file1
 
 for p in path0/file0 path1/file1 path2 path3
 do
-	test_expect_failure \
+	test_expect_success \
 	    "git update-index to add conflicting path $p should fail." \
-	    "git update-index --add -- $p"
+	    "! git update-index --add -- $p"
 done
 test_done
t/t3020-ls-files-error-unmatch.sh +2 -2
index c83f820..f4da869 100755
@@ -15,9 +15,9 @@ touch foo bar
 git update-index --add foo bar
 git-commit -m "add foo bar"
 
-test_expect_failure \
+test_expect_success \
     'git ls-files --error-unmatch should fail with unmatched path.' \
-    'git ls-files --error-unmatch foo bar-does-not-match'
+    '! git ls-files --error-unmatch foo bar-does-not-match'
 
 test_expect_success \
     'git ls-files --error-unmatch should succeed eith matched paths.' \
t/t3200-branch.sh +19 -17
index ef1eeb7..fe353ff 100755
@@ -17,10 +17,11 @@ test_expect_success \
      git-commit -m "Initial commit." &&
      HEAD=$(git rev-parse --verify HEAD)'
 
-test_expect_failure \
-    'git branch --help should not have created a bogus branch' \
-    'git branch --help </dev/null >/dev/null 2>/dev/null || :
-     test -f .git/refs/heads/--help'
+test_expect_success \
+    'git branch --help should not have created a bogus branch' '
+     git branch --help </dev/null >/dev/null 2>/dev/null;
+     ! test -f .git/refs/heads/--help
+'
 
 test_expect_success \
     'git branch abc should create a branch' \
@@ -71,17 +72,17 @@ test_expect_success \
         git branch -m n/n n
         test -f .git/logs/refs/heads/n'
 
-test_expect_failure \
-    'git branch -m o/o o should fail when o/p exists' \
-       'git branch o/o &&
+test_expect_success 'git branch -m o/o o should fail when o/p exists' '
+        git branch o/o &&
         git branch o/p &&
-        git branch -m o/o o'
+        ! git branch -m o/o o
+'
 
-test_expect_failure \
-    'git branch -m q r/q should fail when r exists' \
-       'git branch q &&
-         git branch r &&
-         git branch -m q r/q'
+test_expect_success 'git branch -m q r/q should fail when r exists' '
+        git branch q &&
+        git branch r &&
+        ! git branch -m q r/q
+'
 
 mv .git/config .git/config-saved
 
@@ -108,12 +109,13 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 ! git config branch.s/s/dummy"
 
-test_expect_failure \
-    'git branch -m u v should fail when the reflog for u is a symlink' \
-    'git branch -l u &&
+test_expect_success \
+    'git branch -m u v should fail when the reflog for u is a symlink' '
+     git branch -l u &&
      mv .git/logs/refs/heads/u real-u &&
      ln -s real-u .git/logs/refs/heads/u &&
-     git branch -m u v'
+     ! git branch -m u v
+'
 
 test_expect_success 'test tracking setup via --track' \
     'git config remote.local.url . &&
t/t3210-pack-refs.sh +13 -13
index 4ddc634..b64ccfb 100755
@@ -39,12 +39,12 @@ test_expect_success \
      git show-ref b >result &&
      diff expect result'
 
-test_expect_failure \
-    'git branch c/d should barf if branch c exists' \
-    'git branch c &&
+test_expect_success 'git branch c/d should barf if branch c exists' '
+     git branch c &&
      git pack-refs --all &&
-     rm .git/refs/heads/c &&
-     git branch c/d'
+     rm -f .git/refs/heads/c &&
+     ! git branch c/d
+'
 
 test_expect_success \
     'see if a branch still exists after git pack-refs --prune' \
@@ -54,11 +54,11 @@ test_expect_success \
      git show-ref e >result &&
      diff expect result'
 
-test_expect_failure \
-    'see if git pack-refs --prune remove ref files' \
-    'git branch f &&
+test_expect_success 'see if git pack-refs --prune remove ref files' '
+     git branch f &&
      git pack-refs --all --prune &&
-     ls .git/refs/heads/f'
+     ! test -f .git/refs/heads/f
+'
 
 test_expect_success \
     'git branch g should work when git branch g/h has been deleted' \
@@ -69,11 +69,11 @@ test_expect_success \
      git pack-refs --all &&
      git branch -d g'
 
-test_expect_failure \
-    'git branch i/j/k should barf if branch i exists' \
-    'git branch i &&
+test_expect_success 'git branch i/j/k should barf if branch i exists' '
+     git branch i &&
      git pack-refs --all --prune &&
-     git branch i/j/k'
+     ! git branch i/j/k
+'
 
 test_expect_success \
     'test git branch k after branch k/l/m and k/lm have been deleted' \
t/t3400-rebase.sh +2 -2
index 95e33b5..496f4ec 100755
@@ -42,9 +42,9 @@ test_expect_success \
 test_expect_success 'rebase against master' '
      git rebase master'
 
-test_expect_failure \
+test_expect_success \
     'the rebase operation should not have destroyed author information' \
-    'git log | grep "Author:" | grep "<>"'
+    '! git log | grep "Author:" | grep "<>"'
 
 test_expect_success 'rebase after merge master' '
      git reset --hard topic &&
t/t3403-rebase-skip.sh +3 -3
index 657f681..0a26099 100755
@@ -31,8 +31,8 @@ test_expect_success setup '
 	git branch skip-merge skip-reference
 	'
 
-test_expect_failure 'rebase with git am -3 (default)' '
-	git rebase master
+test_expect_success 'rebase with git am -3 (default)' '
+	! git rebase master
 '
 
 test_expect_success 'rebase --skip with am -3' '
@@ -53,7 +53,7 @@ test_expect_success 'rebase moves back to skip-reference' '
 
 test_expect_success 'checkout skip-merge' 'git checkout -f skip-merge'
 
-test_expect_failure 'rebase with --merge' 'git rebase --merge master'
+test_expect_success 'rebase with --merge' '! git rebase --merge master'
 
 test_expect_success 'rebase --skip with --merge' '
 	git rebase --skip
t/t3600-rm.sh +9 -8
index b1ee622..f542f0a 100755
@@ -59,15 +59,16 @@ test_expect_success \
      echo "other content" > foo
      git rm --cached foo'
 
-test_expect_failure \
-    'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' \
-    'echo content > foo
+test_expect_success \
+    'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
+     echo content > foo
      git add foo
      git commit -m foo
      echo "other content" > foo
      git add foo
      echo "yet another content" > foo
-     git rm --cached foo'
+     ! git rm --cached foo
+'
 
 test_expect_success \
     'Test that git rm --cached -f foo works in case where --cached only did not' \
@@ -106,9 +107,9 @@ embedded'"
 
 if test "$test_failed_remove" = y; then
 chmod a-w .
-test_expect_failure \
+test_expect_success \
     'Test that "git rm -f" fails if its rm fails' \
-    'git rm -f baz'
+    '! git rm -f baz'
 chmod 775 .
 else
     test_expect_success 'skipping removal failure (perhaps running as root?)' :
@@ -212,8 +213,8 @@ test_expect_success 'Recursive with -r -f' '
 	! test -d frotz
 '
 
-test_expect_failure 'Remove nonexistent file returns nonzero exit status' '
-	git rm nonexistent
+test_expect_success 'Remove nonexistent file returns nonzero exit status' '
+	! git rm nonexistent
 '
 
 test_done
t/t4103-apply-binary.sh +36 -32

99 lines not on this page · raw message

t/t4113-apply-ending.sh +4 -4
index 1c6bec0..d741039 100755
@@ -29,8 +29,8 @@ test_expect_success setup \
 
 # test
 
-test_expect_failure 'apply at the end' \
-    'git apply --index test-patch'
+test_expect_success 'apply at the end' \
+    '! git apply --index test-patch'
 
 cat >test-patch <<\EOF
 diff a/file b/file
@@ -47,7 +47,7 @@ b
 c'
 git update-index file
 
-test_expect_failure 'apply at the beginning' \
-	'git apply --index test-patch'
+test_expect_success 'apply at the beginning' \
+	'! git apply --index test-patch'
 
 test_done
t/t5300-pack-object.sh +2 -2
index 6e594bf..4f350dd 100755
@@ -264,8 +264,8 @@ test_expect_success \
      cp -f	.git/objects/9d/235ed07cd19811a6ceb342de82f190e49c9f68 \
 		.git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67'
 
-test_expect_failure \
+test_expect_success \
     'make sure index-pack detects the SHA1 collision' \
-    'git-index-pack -o bad.idx test-3.pack'
+    '! git-index-pack -o bad.idx test-3.pack'
 
 test_done
t/t5302-pack-index.sh +16 -16
index 2a2878b..67b9a7b 100755
@@ -42,9 +42,9 @@ test_expect_success \
     'both packs should be identical' \
     'cmp "test-1-${pack1}.pack" "test-2-${pack2}.pack"'
 
-test_expect_failure \
+test_expect_success \
     'index v1 and index v2 should be different' \
-    'cmp "test-1-${pack1}.idx" "test-2-${pack2}.idx"'
+    '! cmp "test-1-${pack1}.idx" "test-2-${pack2}.idx"'
 
 test_expect_success \
     'index-pack with index version 1' \
@@ -78,9 +78,9 @@ test_expect_success \
     'git verify-pack -v "test-3-${pack3}.pack"'
 
 test "$have_64bits" &&
-test_expect_failure \
+test_expect_success \
     '64-bit offsets: should be different from previous index v2 results' \
-    'cmp "test-2-${pack2}.idx" "test-3-${pack3}.idx"'
+    '! cmp "test-2-${pack2}.idx" "test-3-${pack3}.idx"'
 
 test "$have_64bits" &&
 test_expect_success \
@@ -112,22 +112,22 @@ test_expect_success \
 	  bs=1 count=20 conv=notrunc &&
        git cat-file blob "$delta_sha1" > blob_2 )'
 
-test_expect_failure \
+test_expect_success \
     '[index v1] 3) corrupted delta happily returned wrong data' \
-    'cmp blob_1 blob_2'
+    '! cmp blob_1 blob_2'
 
-test_expect_failure \
+test_expect_success \
     '[index v1] 4) confirm that the pack is actually corrupted' \
-    'git fsck --full $commit'
+    '! git fsck --full $commit'
 
 test_expect_success \
     '[index v1] 5) pack-objects happily reuses corrupted data' \
     'pack4=$(git pack-objects test-4 <obj-list) &&
      test -f "test-4-${pack1}.pack"'
 
-test_expect_failure \
+test_expect_success \
     '[index v1] 6) newly created pack is BAD !' \
-    'git verify-pack -v "test-4-${pack1}.pack"'
+    '! git verify-pack -v "test-4-${pack1}.pack"'
 
 test_expect_success \
     '[index v2] 1) stream pack to repository' \
@@ -150,16 +150,16 @@ test_expect_success \
 	  bs=1 count=20 conv=notrunc &&
        git cat-file blob "$delta_sha1" > blob_4 )'
 
-test_expect_failure \
+test_expect_success \
     '[index v2] 3) corrupted delta happily returned wrong data' \
-    'cmp blob_3 blob_4'
+    '! cmp blob_3 blob_4'
 
-test_expect_failure \
+test_expect_success \
     '[index v2] 4) confirm that the pack is actually corrupted' \
-    'git fsck --full $commit'
+    '! git fsck --full $commit'
 
-test_expect_failure \
+test_expect_success \
     '[index v2] 5) pack-objects refuses to reuse corrupted data' \
-    'git pack-objects test-5 <obj-list'
+    '! git pack-objects test-5 <obj-list'
 
 test_done
t/t5401-update-hooks.sh +4 -4
index 9734fc5..9a12024 100755
@@ -60,8 +60,8 @@ echo STDERR post-update >&2
 EOF
 chmod u+x victim/.git/hooks/post-update
 
-test_expect_failure push '
-	git-send-pack --force ./victim/.git master tofail >send.out 2>send.err
+test_expect_success push '
+    ! git-send-pack --force ./victim/.git master tofail >send.out 2>send.err
 '
 
 test_expect_success 'updated as expected' '
@@ -112,8 +112,8 @@ test_expect_success 'all *-receive hook args are empty' '
 	! test -s victim/.git/post-receive.args
 '
 
-test_expect_failure 'send-pack produced no output' '
-	test -s send.out
+test_expect_success 'send-pack produced no output' '
+	! test -s send.out
 '
 
 cat <<EOF >expect
t/t5402-post-merge-hook.sh +2 -2
index 1c4b0b3..1394047 100755
@@ -30,9 +30,9 @@ EOF
     chmod u+x clone${clone}/.git/hooks/post-merge
 done
 
-test_expect_failure 'post-merge does not run for up-to-date ' '
+test_expect_success 'post-merge does not run for up-to-date ' '
         GIT_DIR=clone1/.git git merge $commit0 &&
-	test -e clone1/.git/post-merge.args
+	! test -f clone1/.git/post-merge.args
 '
 
 test_expect_success 'post-merge runs as expected ' '
t/t5500-fetch-pack.sh +2 -2
index 7b6798d..788b4a5 100755
@@ -176,7 +176,7 @@ test_expect_success "deepening fetch in shallow repo" \
 test_expect_success "clone shallow object count" \
 	"test \"count: 18\" = \"$(grep count count.shallow)\""
 
-test_expect_failure "pull in shallow repo with missing merge base" \
-	"(cd shallow; git pull --depth 4 .. A)"
+test_expect_success "pull in shallow repo with missing merge base" \
+	"(cd shallow && ! git pull --depth 4 .. A)"
 
 test_done
t/t5510-fetch.sh +6 -6
index 02882c1..9b948c1 100755
@@ -95,7 +95,7 @@ test_expect_success 'fetch following tags' '
 
 '
 
-test_expect_failure 'fetch must not resolve short tag name' '
+test_expect_success 'fetch must not resolve short tag name' '
 
 	cd "$D" &&
 
@@ -103,11 +103,11 @@ test_expect_failure 'fetch must not resolve short tag name' '
 	cd five &&
 	git init &&
 
-	git fetch .. anno:five
+	! git fetch .. anno:five
 
 '
 
-test_expect_failure 'fetch must not resolve short remote name' '
+test_expect_success 'fetch must not resolve short remote name' '
 
 	cd "$D" &&
 	git-update-ref refs/remotes/six/HEAD HEAD
@@ -116,7 +116,7 @@ test_expect_failure 'fetch must not resolve short remote name' '
 	cd six &&
 	git init &&
 
-	git fetch .. six:six
+	! git fetch .. six:six
 
 '
 
@@ -139,10 +139,10 @@ test_expect_success 'create bundle 2' '
 	git bundle create bundle2 master~2..master
 '
 
-test_expect_failure 'unbundle 1' '
+test_expect_success 'unbundle 1' '
 	cd "$D/bundle" &&
 	git checkout -b some-branch &&
-	git fetch "$D/bundle1" master:master
+	! git fetch "$D/bundle1" master:master
 '
 
 test_expect_success 'bundle 1 has only 3 files ' '
t/t5530-upload-pack-error.sh +6 -8
index cc8949e..8b05091 100755
@@ -26,9 +26,8 @@ test_expect_success 'setup and corrupt repository' '
 
 '
 
-test_expect_failure 'fsck fails' '
-
-	git fsck
+test_expect_success 'fsck fails' '
+	! git fsck
 '
 
 test_expect_success 'upload-pack fails due to error in pack-objects' '
@@ -46,9 +45,8 @@ test_expect_success 'corrupt repo differently' '
 
 '
 
-test_expect_failure 'fsck fails' '
-
-	git fsck
+test_expect_success 'fsck fails' '
+	! git fsck
 '
 test_expect_success 'upload-pack fails due to error in rev-list' '
 
@@ -66,9 +64,9 @@ test_expect_success 'create empty repository' '
 
 '
 
-test_expect_failure 'fetch fails' '
+test_expect_success 'fetch fails' '
 
-	git fetch .. master
+	! git fetch .. master
 
 '
 
t/t5600-clone-fail-cleanup.sh +6 -6
index 1776b37..acf34ce 100755
@@ -11,13 +11,13 @@ remove the directory before attempting a clone again.'
 
 . ./test-lib.sh
 
-test_expect_failure \
+test_expect_success \
     'clone of non-existent source should fail' \
-    'git-clone foo bar'
+    '! git-clone foo bar'
 
-test_expect_failure \
+test_expect_success \
     'failed clone should not leave a directory' \
-    'cd bar'
+    '! test -d bar'
 
 # Need a repo to clone
 test_create_repo foo
@@ -27,9 +27,9 @@ test_create_repo foo
 
 # source repository given to git-clone should be relative to the
 # current path not to the target dir
-test_expect_failure \
+test_expect_success \
     'clone of non-existent (relative to $PWD) source should fail' \
-    'git-clone ../foo baz'
+    '! git-clone ../foo baz'
 
 test_expect_success \
     'clone should work now that source exists' \
t/t5710-info-alternate.sh +8 -6
index 1908dc8..910ccb4 100755
@@ -87,10 +87,10 @@ test_valid_repo"
 
 cd "$base_dir"
 
-test_expect_failure 'that info/alternates is necessary' \
+test_expect_success 'that info/alternates is necessary' \
 'cd C &&
-rm .git/objects/info/alternates &&
-test_valid_repo'
+rm -f .git/objects/info/alternates &&
+! (test_valid_repo)'
 
 cd "$base_dir"
 
@@ -101,9 +101,11 @@ test_valid_repo'
 
 cd "$base_dir"
 
-test_expect_failure 'that relative alternate is only possible for current dir' \
-'cd D &&
-test_valid_repo'
+test_expect_success \
+    'that relative alternate is only possible for current dir' '
+    cd D &&
+    ! (test_valid_repo)
+'
 
 cd "$base_dir"
 
t/t6023-merge-file.sh +6 -6
index ae3b6f2..8641996 100755
@@ -66,8 +66,8 @@ test_expect_success "merge result added missing LF" \
 	"git diff test.txt test2.txt"
 
 cp test.txt backup.txt
-test_expect_failure "merge with conflicts" \
-	"git merge-file test.txt orig.txt new3.txt"
+test_expect_success "merge with conflicts" \
+	"! git merge-file test.txt orig.txt new3.txt"
 
 cat > expect.txt << EOF
 <<<<<<< test.txt
@@ -89,8 +89,8 @@ EOF
 test_expect_success "expected conflict markers" "git diff test.txt expect.txt"
 
 cp backup.txt test.txt
-test_expect_failure "merge with conflicts, using -L" \
-	"git merge-file -L 1 -L 2 test.txt orig.txt new3.txt"
+test_expect_success "merge with conflicts, using -L" \
+	"! git merge-file -L 1 -L 2 test.txt orig.txt new3.txt"
 
 cat > expect.txt << EOF
 <<<<<<< 1
@@ -113,8 +113,8 @@ test_expect_success "expected conflict markers, with -L" \
 	"git diff test.txt expect.txt"
 
 sed "s/ tu / TU /" < new1.txt > new5.txt
-test_expect_failure "conflict in removed tail" \
-	"git merge-file -p orig.txt new1.txt new5.txt > out"
+test_expect_success "conflict in removed tail" \
+	"! git merge-file -p orig.txt new1.txt new5.txt > out"
 
 cat > expect << EOF
 Dominus regit me,
t/t6024-recursive-merge.sh +1 -1
index c154f03..149ea85 100755
@@ -60,7 +60,7 @@ git update-index a1 &&
 GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
 '
 
-test_expect_failure "combined merge conflicts" "git merge -m final G"
+test_expect_success "combined merge conflicts" "! git merge -m final G"
 
 cat > expect << EOF
 <<<<<<< HEAD:a1
t/t6025-merge-symlinks.sh +10 -11
index 950c2e9..6004deb 100755
@@ -30,30 +30,29 @@ echo plain-file > symlink &&
 git add symlink &&
 git-commit -m b-file'
 
-test_expect_failure \
+test_expect_success \
 'merge master into b-symlink, which has a different symbolic link' '
-! git-checkout b-symlink ||
-git-merge master'
+git-checkout b-symlink &&
+! git-merge master'
 
 test_expect_success \
 'the merge result must be a file' '
 test -f symlink'
 
-test_expect_failure \
+test_expect_success \
 'merge master into b-file, which has a file instead of a symbolic link' '
-! (git-reset --hard &&
-git-checkout b-file) ||
-git-merge master'
+git-reset --hard && git-checkout b-file &&
+! git-merge master'
 
 test_expect_success \
 'the merge result must be a file' '
 test -f symlink'
 
-test_expect_failure \
+test_expect_success \
 'merge b-file, which has a file instead of a symbolic link, into master' '
-! (git-reset --hard &&
-git-checkout master) ||
-git-merge b-file'
+git-reset --hard &&
+git-checkout master &&
+! git-merge b-file'
 
 test_expect_success \
 'the merge result must be a file' '
t/t6101-rev-parse-parents.sh +1 -1
index 0724864..2328b69 100755
@@ -26,7 +26,7 @@ test_expect_success 'final^1^1^1 = final^^^' "test $(git rev-parse final^1^1^1)
 test_expect_success 'final^1^2' "test $(git rev-parse start2) = $(git rev-parse final^1^2)"
 test_expect_success 'final^1^2 != final^1^1' "test $(git rev-parse final^1^2) != $(git rev-parse final^1^1)"
 test_expect_success 'final^1^3 not valid' "if git rev-parse --verify final^1^3; then false; else :; fi"
-test_expect_failure '--verify start2^1' 'git rev-parse --verify start2^1'
+test_expect_success '--verify start2^1' '! git rev-parse --verify start2^1'
 test_expect_success '--verify start2^0' 'git rev-parse --verify start2^0'
 
 test_expect_success 'repack for next test' 'git repack -a -d'
t/t6300-for-each-ref.sh +4 -4
index 8a23aaf..f46ec93 100755
@@ -43,8 +43,8 @@ test_expect_success 'Check atom names are valid' '
 	test -z "$bad"
 '
 
-test_expect_failure 'Check invalid atoms names are errors' '
-	git-for-each-ref --format="%(INVALID)" refs/heads
+test_expect_success 'Check invalid atoms names are errors' '
+	! git-for-each-ref --format="%(INVALID)" refs/heads
 '
 
 test_expect_success 'Check format specifiers are ignored in naming date atoms' '
@@ -63,8 +63,8 @@ test_expect_success 'Check valid format specifiers for date fields' '
 	git-for-each-ref --format="%(authordate:rfc2822)" refs/heads
 '
 
-test_expect_failure 'Check invalid format specifiers are errors' '
-	git-for-each-ref --format="%(authordate:INVALID)" refs/heads
+test_expect_success 'Check invalid format specifiers are errors' '
+	! git-for-each-ref --format="%(authordate:INVALID)" refs/heads
 '
 
 cat >expected <<\EOF
t/t7001-mv.sh +2 -2
index b730c90..b1243b4 100755
@@ -78,9 +78,9 @@ test_expect_success \
      git diff-tree -r -M --name-status  HEAD^ HEAD | \
      grep "^R100..*path2/README..*path1/path2/README"'
 
-test_expect_failure \
+test_expect_success \
     'do not move directory over existing directory' \
-    'mkdir path0 && mkdir path0/path2 && git mv path2 path0'
+    'mkdir path0 && mkdir path0/path2 && ! git mv path2 path0'
 
 test_expect_success \
     'move into "."' \
t/t7002-grep.sh +2 -2
index 68b2b92..8d3a8eb 100755
@@ -107,8 +107,8 @@ do
 		diff expected actual
 	'
 
-        test_expect_failure "grep -c $L (no /dev/null)" '
-		git grep -c test $H | grep -q "/dev/null"
+        test_expect_success "grep -c $L (no /dev/null)" '
+		! git grep -c test $H | grep -q /dev/null
         '
 
 done
t/t7004-tag.sh +18 -18
index df496a9..75cd33b 100755
@@ -26,8 +26,8 @@ test_expect_success 'listing all tags in an empty tree should output nothing' '
 	test `git-tag | wc -l` -eq 0
 '
 
-test_expect_failure 'looking for a tag in an empty tree should fail' \
-	'tag_exists mytag'
+test_expect_success 'looking for a tag in an empty tree should fail' \
+	'! (tag_exists mytag)'
 
 test_expect_success 'creating a tag in an empty tree should fail' '
 	! git-tag mynotag &&
@@ -83,9 +83,9 @@ test_expect_success \
 
 # special cases for creating tags:
 
-test_expect_failure \
+test_expect_success \
 	'trying to create a tag with the name of one existing should fail' \
-	'git tag mytag'
+	'! git tag mytag'
 
 test_expect_success \
 	'trying to create a tag with a non-valid name should fail' '
@@ -146,8 +146,8 @@ test_expect_success \
 	! tag_exists myhead
 '
 
-test_expect_failure 'trying to delete an already deleted tag should fail' \
-	'git-tag -d mytag'
+test_expect_success 'trying to delete an already deleted tag should fail' \
+	'! git-tag -d mytag'
 
 # listing various tags with pattern matching:
 
@@ -265,16 +265,16 @@ test_expect_success \
 	test $(git rev-parse non-annotated-tag) = $(git rev-parse HEAD)
 '
 
-test_expect_failure 'trying to verify an unknown tag should fail' \
-	'git-tag -v unknown-tag'
+test_expect_success 'trying to verify an unknown tag should fail' \
+	'! git-tag -v unknown-tag'
 
-test_expect_failure \
+test_expect_success \
 	'trying to verify a non-annotated and non-signed tag should fail' \
-	'git-tag -v non-annotated-tag'
+	'! git-tag -v non-annotated-tag'
 
-test_expect_failure \
+test_expect_success \
 	'trying to verify many non-annotated or unknown tags, should fail' \
-	'git-tag -v unknown-tag1 non-annotated-tag unknown-tag2'
+	'! git-tag -v unknown-tag1 non-annotated-tag unknown-tag2'
 
 # creating annotated tags:
 
@@ -1027,21 +1027,21 @@ test_expect_success \
 
 # try to sign with bad user.signingkey
 git config user.signingkey BobTheMouse
-test_expect_failure \
+test_expect_success \
 	'git-tag -s fails if gpg is misconfigured' \
-	'git tag -s -m tail tag-gpg-failure'
+	'! git tag -s -m tail tag-gpg-failure'
 git config --unset user.signingkey
 
 # try to verify without gpg:
 
 rm -rf gpghome
-test_expect_failure \
+test_expect_success \
 	'verify signed tag fails when public key is not present' \
-	'git-tag -v signed-tag'
+	'! git-tag -v signed-tag'
 
-test_expect_failure \
+test_expect_success \
 	'git-tag -a fails if tag annotation is empty' '
-	GIT_EDITOR=cat git tag -a initial-comment
+	! (GIT_EDITOR=cat git tag -a initial-comment)
 '
 
 test_expect_success \
t/t7101-reset.sh +12 -12
index 66d4043..0d9874b 100755
@@ -36,28 +36,28 @@ test_expect_success \
     'test -d path0 &&
      test -f path0/COPYING'
 
-test_expect_failure \
+test_expect_success \
     'checking lack of path1/path2/COPYING' \
-    'test -f path1/path2/COPYING'
+    '! test -f path1/path2/COPYING'
 
-test_expect_failure \
+test_expect_success \
     'checking lack of path1/COPYING' \
-    'test -f path1/COPYING'
+    '! test -f path1/COPYING'
 
-test_expect_failure \
+test_expect_success \
     'checking lack of COPYING' \
-    'test -f COPYING'
+    '! test -f COPYING'
 
-test_expect_failure \
+test_expect_success \
     'checking checking lack of path1/COPYING-TOO' \
-    'test -f path0/COPYING-TOO'
+    '! test -f path0/COPYING-TOO'
 
-test_expect_failure \
+test_expect_success \
     'checking lack of path1/path2' \
-    'test -d path1/path2'
+    '! test -d path1/path2'
 
-test_expect_failure \
+test_expect_success \
     'checking lack of path1' \
-    'test -d path1'
+    '! test -d path1'
 
 test_done
t/t7501-commit.sh +20 -20
index d1a415a..21dcf55 100755
@@ -17,49 +17,49 @@ test_expect_success \
 	 git-add file && \
 	 git-status | grep 'Initial commit'"
 
-test_expect_failure \
+test_expect_success \
 	"fail initial amend" \
-	"git-commit --amend"
+	"! git-commit --amend"
 
 test_expect_success \
 	"initial commit" \
 	"git-commit -m initial"
 
-test_expect_failure \
+test_expect_success \
 	"invalid options 1" \
-	"git-commit -m foo -m bar -F file"
+	"! git-commit -m foo -m bar -F file"
 
-test_expect_failure \
+test_expect_success \
 	"invalid options 2" \
-	"git-commit -C HEAD -m illegal"
+	"! git-commit -C HEAD -m illegal"
 
-test_expect_failure \
+test_expect_success \
 	"using paths with -a" \
 	"echo King of the bongo >file &&
-	git-commit -m foo -a file"
+	! git-commit -m foo -a file"
 
-test_expect_failure \
+test_expect_success \
 	"using paths with --interactive" \
 	"echo bong-o-bong >file &&
-	echo 7 | git-commit -m foo --interactive file"
+	! echo 7 | git-commit -m foo --interactive file"
 
-test_expect_failure \
+test_expect_success \
 	"using invalid commit with -C" \
-	"git-commit -C bogus"
+	"! git-commit -C bogus"
 
-test_expect_failure \
+test_expect_success \
 	"testing nothing to commit" \
-	"git-commit -m initial"
+	"! git-commit -m initial"
 
 test_expect_success \
 	"next commit" \
 	"echo 'bongo bongo bongo' >file \
 	 git-commit -m next -a"
 
-test_expect_failure \
+test_expect_success \
 	"commit message from non-existing file" \
 	"echo 'more bongo: bongo bongo bongo bongo' >file && \
-	 git-commit -F gah -a"
+	 ! git-commit -F gah -a"
 
 # Empty except stray tabs and spaces on a few lines.
 sed -e 's/@$//' >msg <<EOF
@@ -68,9 +68,9 @@ sed -e 's/@$//' >msg <<EOF
   @
 Signed-off-by: hula
 EOF
-test_expect_failure \
+test_expect_success \
 	"empty commit message" \
-	"git-commit -F msg -a"
+	"! git-commit -F msg -a"
 
 test_expect_success \
 	"commit message from file" \
@@ -88,10 +88,10 @@ test_expect_success \
 	"amend commit" \
 	"VISUAL=./editor git-commit --amend"
 
-test_expect_failure \
+test_expect_success \
 	"passing -m and -F" \
 	"echo 'enough with the bongos' >file && \
-	 git-commit -F msg -m amending ."
+	 ! git-commit -F msg -m amending ."
 
 test_expect_success \
 	"using message from other commit" \
t/t7503-pre-commit-hook.sh +2 -2
index d787cac..2dd5a5e 100755
@@ -52,11 +52,11 @@ cat > "$HOOK" <<EOF
 exit 1
 EOF
 
-test_expect_failure 'with failing hook' '
+test_expect_success 'with failing hook' '
 
 	echo "another" >> file &&
 	git add file &&
-	git commit -m "another"
+	! git commit -m "another"
 
 '
 
t/t7504-commit-msg-hook.sh +4 -4
index 751b113..eff36aa 100755
@@ -98,20 +98,20 @@ cat > "$HOOK" <<EOF
 exit 1
 EOF
 
-test_expect_failure 'with failing hook' '
+test_expect_success 'with failing hook' '
 
 	echo "another" >> file &&
 	git add file &&
-	git commit -m "another"
+	! git commit -m "another"
 
 '
 
-test_expect_failure 'with failing hook (editor)' '
+test_expect_success 'with failing hook (editor)' '
 
 	echo "more another" >> file &&
 	git add file &&
 	echo "more another" > FAKE_MSG &&
-	GIT_EDITOR="$FAKE_EDITOR" git commit
+	! (GIT_EDITOR="$FAKE_EDITOR" git commit)
 
 '
 
t/t9100-git-svn-basic.sh +14 -14
index 614cf50..1078f87 100755
@@ -56,19 +56,19 @@ test_expect_success "$name" "
 
 
 name='detect node change from file to directory #1'
-test_expect_failure "$name" "
+test_expect_success "$name" "
 	mkdir dir/new_file &&
 	mv dir/file dir/new_file/file &&
 	mv dir/new_file dir/file &&
 	git update-index --remove dir/file &&
 	git update-index --add dir/file/file &&
-	git commit -m '$name'  &&
-	git-svn set-tree --find-copies-harder --rmdir \
+	git commit -m '$name' &&
+	! git-svn set-tree --find-copies-harder --rmdir \
 		remotes/git-svn..mybranch" || true
 
 
 name='detect node change from directory to file #1'
-test_expect_failure "$name" "
+test_expect_success "$name" "
 	rm -rf dir '$GIT_DIR'/index &&
 	git checkout -f -b mybranch2 remotes/git-svn &&
 	mv bar/zzz zzz &&
@@ -77,12 +77,12 @@ test_expect_failure "$name" "
 	git update-index --remove -- bar/zzz &&
 	git update-index --add -- bar &&
 	git commit -m '$name' &&
-	git-svn set-tree --find-copies-harder --rmdir \
+	! git-svn set-tree --find-copies-harder --rmdir \
 		remotes/git-svn..mybranch2" || true
 
 
 name='detect node change from file to directory #2'
-test_expect_failure "$name" "
+test_expect_success "$name" "
 	rm -f '$GIT_DIR'/index &&
 	git checkout -f -b mybranch3 remotes/git-svn &&
 	rm bar/zzz &&
@@ -91,12 +91,12 @@ test_expect_failure "$name" "
 	echo yyy > bar/zzz/yyy &&
 	git update-index --add bar/zzz/yyy &&
 	git commit -m '$name' &&
-	git-svn set-tree --find-copies-harder --rmdir \
+	! git-svn set-tree --find-copies-harder --rmdir \
 		remotes/git-svn..mybranch3" || true
 
 
 name='detect node change from directory to file #2'
-test_expect_failure "$name" "
+test_expect_success "$name" "
 	rm -f '$GIT_DIR'/index &&
 	git checkout -f -b mybranch4 remotes/git-svn &&
 	rm -rf dir &&
@@ -105,7 +105,7 @@ test_expect_failure "$name" "
 	echo asdf > dir &&
 	git update-index --add -- dir &&
 	git commit -m '$name' &&
-	git-svn set-tree --find-copies-harder --rmdir \
+	! git-svn set-tree --find-copies-harder --rmdir \
 		remotes/git-svn..mybranch4" || true
 
 
@@ -213,18 +213,18 @@ EOF
 
 test_expect_success "$name" "git diff a expected"
 
-test_expect_failure 'exit if remote refs are ambigious' "
+test_expect_success 'exit if remote refs are ambigious' "
         git config --add svn-remote.svn.fetch \
                               bar:refs/remotes/git-svn &&
-        git-svn migrate
-        "
+        ! git-svn migrate
+"
 
-test_expect_failure 'exit if init-ing a would clobber a URL' "
+test_expect_success 'exit if init-ing a would clobber a URL' "
         svnadmin create ${PWD}/svnrepo2 &&
         svn mkdir -m 'mkdir bar' ${svnrepo}2/bar &&
         git config --unset svn-remote.svn.fetch \
                                 '^bar:refs/remotes/git-svn$' &&
-        git-svn init ${svnrepo}2/bar
+        ! git-svn init ${svnrepo}2/bar
         "
 
 test_expect_success \
t/t9106-git-svn-commit-diff-clobber.sh +9 -9
index 79b7968..f74ab12 100755
@@ -24,11 +24,11 @@ test_expect_success 'commit change from svn side' "
 	rm -rf t.svn
 	"
 
-test_expect_failure 'commit conflicting change from git' "
+test_expect_success 'commit conflicting change from git' "
 	echo second line from git >> file &&
 	git commit -a -m 'second line from git' &&
-	git-svn commit-diff -r1 HEAD~1 HEAD $svnrepo
-	" || true
+	! git-svn commit-diff -r1 HEAD~1 HEAD $svnrepo
+"
 
 test_expect_success 'commit complementing change from git' "
 	git reset --hard HEAD~1 &&
@@ -39,7 +39,7 @@ test_expect_success 'commit complementing change from git' "
 	git-svn commit-diff -r2 HEAD~1 HEAD $svnrepo
 	"
 
-test_expect_failure 'dcommit fails to commit because of conflict' "
+test_expect_success 'dcommit fails to commit because of conflict' "
 	git-svn init $svnrepo &&
 	git-svn fetch &&
 	git reset --hard refs/remotes/git-svn &&
@@ -52,8 +52,8 @@ test_expect_failure 'dcommit fails to commit because of conflict' "
 	rm -rf t.svn &&
 	echo 'fourth line from git' >> file &&
 	git commit -a -m 'fourth line from git' &&
-	git-svn dcommit
-	" || true
+	! git-svn dcommit
+	"
 
 test_expect_success 'dcommit does the svn equivalent of an index merge' "
 	git reset --hard refs/remotes/git-svn &&
@@ -76,15 +76,15 @@ test_expect_success 'commit another change from svn side' "
 	rm -rf t.svn
 	"
 
-test_expect_failure 'multiple dcommit from git-svn will not clobber svn' "
+test_expect_success 'multiple dcommit from git-svn will not clobber svn' "
 	git reset --hard refs/remotes/git-svn &&
 	echo new file >> new-file &&
 	git update-index --add new-file &&
 	git commit -a -m 'new file' &&
 	echo clobber > file &&
 	git commit -a -m 'clobber' &&
-	git svn dcommit
-	" || true
+	! git svn dcommit
+	"
 
 
 test_expect_success 'check that rebase really failed' 'test -d .dotest'
t/t9106-git-svn-dcommit-clobber-series.sh +2 -2
index 7452546..ca8a00e 100755
@@ -54,10 +54,10 @@ test_expect_success 'change file but in unrelated area' "
 		test x\"\`sed -n -e 61p < file\`\" = x6611
 	"
 
-test_expect_failure 'attempt to dcommit with a dirty index' '
+test_expect_success 'attempt to dcommit with a dirty index' '
 	echo foo >>file &&
 	git add file &&
-	git svn dcommit
+	! git svn dcommit
 '
 
 test_done
t/t9300-fast-import.sh +12 -12
index 0595041..cceedbb 100755
@@ -165,9 +165,9 @@ from refs/heads/master
 M 755 0000000000000000000000000000000000000001 zero1
 
 INPUT_END
-test_expect_failure \
-    'B: fail on invalid blob sha1' \
-    'git-fast-import <input'
+test_expect_success 'B: fail on invalid blob sha1' '
+    ! git-fast-import <input
+'
 rm -f .git/objects/pack_* .git/objects/index_*
 
 cat >input <<INPUT_END
@@ -180,9 +180,9 @@ COMMIT
 from refs/heads/master
 
 INPUT_END
-test_expect_failure \
-    'B: fail on invalid branch name ".badbranchname"' \
-    'git-fast-import <input'
+test_expect_success 'B: fail on invalid branch name ".badbranchname"' '
+    ! git-fast-import <input
+'
 rm -f .git/objects/pack_* .git/objects/index_*
 
 cat >input <<INPUT_END
@@ -195,9 +195,9 @@ COMMIT
 from refs/heads/master
 
 INPUT_END
-test_expect_failure \
-    'B: fail on invalid branch name "bad[branch]name"' \
-    'git-fast-import <input'
+test_expect_success 'B: fail on invalid branch name "bad[branch]name"' '
+    ! git-fast-import <input
+'
 rm -f .git/objects/pack_* .git/objects/index_*
 
 cat >input <<INPUT_END
@@ -339,9 +339,9 @@ COMMIT
 from refs/heads/branch^0
 
 INPUT_END
-test_expect_failure \
-    'E: rfc2822 date, --date-format=raw' \
-    'git-fast-import --date-format=raw <input'
+test_expect_success 'E: rfc2822 date, --date-format=raw' '
+    ! git-fast-import --date-format=raw <input
+'
 test_expect_success \
     'E: rfc2822 date, --date-format=rfc2822' \
     'git-fast-import --date-format=rfc2822 <input'
t/t9400-git-cvsserver-server.sh +18 -12
index 75d1ce4..0a20971 100755
@@ -156,15 +156,19 @@ test_expect_success 'req_Root (strict paths)' \
   'cat request-anonymous | git-cvsserver --strict-paths pserver $SERVERDIR >log 2>&1 &&
    tail -n1 log | grep -q "^I LOVE YOU$"'
 
-test_expect_failure 'req_Root failure (strict-paths)' \
-  'cat request-anonymous | git-cvsserver --strict-paths pserver $WORKDIR >log 2>&1'
+test_expect_success 'req_Root failure (strict-paths)' '
+    ! cat request-anonymous |
+    git-cvsserver --strict-paths pserver $WORKDIR >log 2>&1
+'
 
 test_expect_success 'req_Root (w/o strict-paths)' \
   'cat request-anonymous | git-cvsserver pserver $WORKDIR/ >log 2>&1 &&
    tail -n1 log | grep -q "^I LOVE YOU$"'
 
-test_expect_failure 'req_Root failure (w/o strict-paths)' \
-  'cat request-anonymous | git-cvsserver pserver $WORKDIR/gitcvs >log 2>&1'
+test_expect_success 'req_Root failure (w/o strict-paths)' '
+    ! cat request-anonymous |
+    git-cvsserver pserver $WORKDIR/gitcvs >log 2>&1
+'
 
 cat >request-base  <<EOF
 BEGIN AUTH REQUEST
@@ -179,8 +183,10 @@ test_expect_success 'req_Root (base-path)' \
   'cat request-base | git-cvsserver --strict-paths --base-path $WORKDIR/ pserver $SERVERDIR >log 2>&1 &&
    tail -n1 log | grep -q "^I LOVE YOU$"'
 
-test_expect_failure 'req_Root failure (base-path)' \
-  'cat request-anonymous | git-cvsserver --strict-paths --base-path $WORKDIR pserver $SERVERDIR >log 2>&1'
+test_expect_success 'req_Root failure (base-path)' '
+    ! cat request-anonymous |
+    git-cvsserver --strict-paths --base-path $WORKDIR pserver $SERVERDIR >log 2>&1
+'
 
 GIT_DIR="$SERVERDIR" git config --bool gitcvs.enabled false || exit 1
 
@@ -188,9 +194,8 @@ test_expect_success 'req_Root (export-all)' \
   'cat request-anonymous | git-cvsserver --export-all pserver $WORKDIR >log 2>&1 &&
    tail -n1 log | grep -q "^I LOVE YOU$"'
 
-test_expect_failure 'req_Root failure (export-all w/o whitelist)' \
-  'cat request-anonymous | git-cvsserver --export-all pserver >log 2>&1 ||
-   false'
+test_expect_success 'req_Root failure (export-all w/o whitelist)' \
+  '! (cat request-anonymous | git-cvsserver --export-all pserver >log 2>&1 || false)'
 
 test_expect_success 'req_Root (everything together)' \
   'cat request-base | git-cvsserver --export-all --strict-paths --base-path $WORKDIR/ pserver $SERVERDIR >log 2>&1 &&
@@ -290,15 +295,16 @@ test_expect_success 'cvs update (update existing file)' \
 
 cd "$WORKDIR"
 #TODO: cvsserver doesn't support update w/o -d
-test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" \
-  'mkdir test &&
+test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" '
+   mkdir test &&
    echo >test/empty &&
    git add test &&
    git commit -q -m "Single Subdirectory" &&
    git push gitcvs.git >/dev/null &&
    cd cvswork &&
    GIT_CONFIG="$git_config" cvs -Q update &&
-   test ! -d test'
+   test ! -d test
+'
 
 cd "$WORKDIR"
 test_expect_success 'cvs update (subdirectories)' \
t/test-lib.sh +26 -4
index 142540e..9a3c0b4 100644
@@ -139,6 +139,8 @@ fi
 
 test_failure=0
 test_count=0
+test_fixed=0
+test_broken=0
 
 trap 'echo >&5 "FATAL: Unexpected exit with code $?"; exit 1' exit
 
@@ -171,6 +173,17 @@ test_failure_ () {
 	test "$immediate" = "" || { trap - exit; exit 1; }
 }
 
+test_known_broken_ok_ () {
+	test_count=$(expr "$test_count" + 1)
+	test_fixed=$(($test_fixed+1))
+	say_color "" "  FIXED $test_count: $@"
+}
+
+test_known_broken_failure_ () {
+	test_count=$(expr "$test_count" + 1)
+	test_broken=$(($test_broken+1))
+	say_color skip "  still broken $test_count: $@"
+}
 
 test_debug () {
 	test "$debug" = "" || eval "$1"
@@ -211,13 +224,13 @@ test_expect_failure () {
 	error "bug in the test script: not 2 parameters to test-expect-failure"
 	if ! test_skip "$@"
 	then
-		say >&3 "expecting failure: $2"
+		say >&3 "checking known breakage: $2"
 		test_run_ "$2"
-		if [ "$?" = 0 -a "$eval_ret" != 0 -a "$eval_ret" -lt 129 ]
+		if [ "$?" = 0 -a "$eval_ret" = 0 ]
 		then
-			test_ok_ "$1"
+			test_known_broken_ok_ "$1"
 		else
-			test_failure_ "$@"
+		    test_known_broken_failure_ "$1"
 		fi
 	fi
 	echo >&3 ""
@@ -274,6 +287,15 @@ test_create_repo () {
 
 test_done () {
 	trap - exit
+
+	if test "$test_fixed" != 0
+	then
+		say_color pass "fixed $test_fixed known breakage(s)"
+	fi
+	if test "$test_broken" != 0
+	then
+		say_color error "still have $test_broken known breakage(s)"
+	fi
 	case "$test_failure" in
 	0)
 		# We could:
#37 lore raw

Re: [PATCH] Sane use of test_expect_failure

As I promised, a patch to revamp test_expect_failure semantics has been applied to 'master' and pushed out.

The rule used to be that test_expect_failure is to see if the command sequence exits with non-zero status. It was tempting to incorrectly use it like this:

    test_expect_failure 'this should fail' '
	setup1 &&
        setup2 &&
        setup3 &&
        what you expect to fail
    '

but was very error prone, because the failure can come from the earlier "setup" stages.

The new world order is that test_expect_failure is used to mark a known breakage, so that people can run "git grep t/" to see if there are things to work on.

We have an example in cvsserver test:

#TODO: cvsserver doesn't support update w/o -d test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" '
...
test ! -d test
'

If git-cvsserver did not have this bug, this should succeed, but there is a known breakage that is waiting to be fixed.

I may have missed tests that were using test_expect_failure to mark known bug that need to be fixed and converted that to test_expect_success to check an error exit status from the last command in the sequence. IOW, a mistranslation of the above might have done:

test_expect_success "cvs update w/o -d doesn't create subdir (TODO)" '
...
test -d test
'

which would be wrong. Fixing the bug would then "break" this test.

Could people who added test_expect_failure in the past that this patch updated, look them over to catch such a misconversion please?

#44 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

In reply to Junio C Hamano

Quoting Junio C Hamano <[email protected]>:

Quoted 12 lines

Currently, we:

- Remove "." path component (i.e. the directory leading part specified) from the input;

- Remove ".." path component and strip one level of the prefix;

only from the beginning. So if you give nonsense pathspec from the command line, you can end up calling prefix_path() with things like "/README", "/absolute/path/to//repository/tracked/file", and "fo//o/../o".

Quoted 4 lines

And not passing such ambiguous path like "fo//o" to the core level but sanitizing matters. Then core level can always do memcmp() with "fo/o" to see they are talking about the same path.

I may be mistaken but I think "fo//o" and "fo//o/" are returned as two different strings "fo/o" and "fo/o/" from your patch. Shouldn't you clean-up the second one to "fo/o", too?

Signature · 6 lines
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

----------------------------------------------------------------------
Free pop3 email with a spam filter.
http://www.bluebottle.com/tag/5
#45 lore raw

Re: [RFH/PATCH] prefix_path(): disallow absolute paths

Quoted 10 lines · しらいしななこ

Quoting Junio C Hamano <[email protected]>:
...

Quoted 4 lines

And not passing such ambiguous path like "fo//o" to the core level but sanitizing matters. Then core level can always do memcmp() with "fo/o" to see they are talking about the same path.

I may be mistaken but I think "fo//o" and "fo//o/" are returned as two different strings "fo/o" and "fo/o/" from your patch. Shouldn't you clean-up the second one to "fo/o", too?

That certainly is a potentially possible sanitization, and I actually thought about it when I did these patches.

However, I chose not to because I was not sure if some core functions want to differentiate pathspecs "foo/" and "foo". The former says "I want to make sure that 'foo' is a directory, and want to affect everything under it", the latter says "I do not care if 'foo' is a blob or a directory, but I want to affect it (if a blob) or everything under it (if a directory)".

In fact, stripping trailing slashes off would break this pair:

	git ls-files --error-unmatch Makefile/
	git ls-files --error-unmatch Makefile

Things like "git add Makefile/" relies on the former to fail loudly. So the answer is no, we do not want to clean it up.

Incidentally, I notice that in addition to the squashed patch from yesterday, we would need to teach "error-unmatch" code that it should trigger when get_pathspec() returns a pathspec that has fewer number of paths than its input.

It should be a pretty straightforward patch, but I haven't looked into fixing it. I'm lazy and I'd rather have other people to do the fixing for me. Hint, hint... ;-)

By the way, please keep your lines to reasonable length by wrapping in your e-mailed messages.

#38 lore raw

Re: [PATCH] More test cases for sanitized path names

In reply to Robin Rosenberg
Quoted 11 lines · Robin Rosenberg

Verify a few more commands and pathname variants.

---
 t/t7010-setup.sh |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

These are a few testcases from my earlier attempt at this. The log and commit cases succeeded with Junios version, but not blame and some of the nastier versions for git add (same principle for all commands, just that I use add as an example)

I am very sorry about replying to an ancient topic, but I think I misread your patch.

t/t7010-setup.sh test_expect_success 'git ls-files (relative #3)' ' · line 142

Quoted 5 lines
t/t7010-setup.sh +5 -0
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'

What I think I misunderstood was that you _wanted_ this (after removing the "echo", which was a mistake, which we already talked about) to fail. Somehow I ended up committing test_expect_success, which I think was a mistake, and I am asking for a sanity-check.

Likewise for the other two tests. These "add outside" should fail, right?

Quoted 13 lines
t/t7010-setup.sh +12 -0
+test_expect_failure 'add a file outside the work tree, nasty case 1' '(
+	f="$(pwd)x" &&
+	touch "$f" &&
+	git add "$f"
+)'
+
+test_expect_failure 'add a file outside the work tree, nasty case 2' '(
+	f="$(pwd|sed "s/.$//")x" &&
+	touch "$f" &&
+	git add "$f"
+)'
+
 test_done
#39 lore raw

Re: [PATCH] More test cases for sanitized path names

Den Friday 07 March 2008 09.23.54 skrev Junio C Hamano:

Quoted 16 lines
Quoted 11 lines · Robin Rosenberg

Verify a few more commands and pathname variants.

---
 t/t7010-setup.sh |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

These are a few testcases from my earlier attempt at this. The log and commit cases succeeded with Junios version, but not blame and some of the nastier versions for git add (same principle for all commands, just that I use add as an example)

I am very sorry about replying to an ancient topic, but I think I misread your patch.

Quoted 10 lines

t/t7010-setup.sh test_expect_success 'git ls-files (relative #3)' ' · line 142

Quoted 5 lines
t/t7010-setup.sh +5 -0
+test_expect_failure 'add a directory outside the work tree' '
+	d1="$(cd .. ; pwd)" &&
+	git add "$d1"
+	echo $?
+'

What I think I misunderstood was that you _wanted_ this (after removing the "echo", which was a mistake, which we already talked about) to fail. Somehow I ended up committing test_expect_success, which I think was a mistake, and I am asking for a sanity-check.

Yes, it should fail, so according to your filosophy, the test should be reverted, i.e. ! git add "$d1 and that negated test should pass.

Quoted 15 lines

Likewise for the other two tests. These "add outside" should fail, right?

t/t7010-setup.sh test_expect_success 'git ls-files (relative #3)' ' · line 156

Quoted 13 lines
t/t7010-setup.sh +12 -0
+test_expect_failure 'add a file outside the work tree, nasty case 1' '(
+	f="$(pwd)x" &&
+	touch "$f" &&
+	git add "$f"
+)'
+
+test_expect_failure 'add a file outside the work tree, nasty case 2' '(
+	f="$(pwd|sed "s/.$//")x" &&
+	touch "$f" &&
+	git add "$f"
+)'
+
 test_done

Yes.

-- robin