git-clean buglet
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
Quoted 10 lines · Johannes Sixt
Try this in your favorite git repo:
git clean -n /
Here, it responds with:
fatal: oops in
prep_excludeI 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
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
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
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
@@ -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
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.
Signed-off-by Shawn Bohrer <[email protected]>---
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
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
Signed-off-by Shawn Bohrer <[email protected]>---
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
Re: [PATCH] Fix off by one error in prep_exclude.
In reply to Johannes SchindelinQuoted 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?
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()andexcluded(), 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
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
@@ -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
Re: [PATCH] Fix off by one error in prep_exclude.
Hi,
Quoted 25 lines · Shawn Bohrer
In "struct
exclude_stack",prep_exclude()andexcluded(), 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".
Signed-off-by Shawn Bohrer <[email protected]>
I'll try to remember even 6 months from now that this was the "git clean -n /" problem ;-)
Ciao,
Dscho
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".Signed-off-by Shawn Bohrer <[email protected]>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.
Re: [PATCH] Fix off by one error in prep_exclude.
In reply to Shawn BohrerQuoted 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.
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 inbuiltin-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
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?
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 /*binThe 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
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.
[RFH/PATCH] prefix_path(): disallow absolute paths
In reply to Johannes SixtWithout 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
@@ -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
@@ -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
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 thebuiltin-mv.cpart).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
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
@@ -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
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
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?
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
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.
Re: [RFH/PATCH] prefix_path(): disallow absolute paths
In reply to Junio C HamanoQuoted 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 ofwork_tree.A few things to note.
* Your mv fix is rolled in.
* This allows you to name a in-repository file as
pwd/file, orpwd//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 alternativelydie()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
@@ -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) {
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
@@ -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
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.txtbut not as:
howto/./maintain-git.txt
$(pwd)/howto/maintain-git.txtThis 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
@@ -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
@@ -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
Re: [RFH/PATCH] prefix_path(): disallow absolute paths
In reply to Junio C HamanoThis 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
@@ -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; }
Re: [RFH/PATCH] prefix_path(): disallow absolute paths
In reply to Junio C HamanoHi,
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
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.
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
PATCH 1/1 prefix_path(): disallow absolute paths
In reply to Johannes SchindelinWithout 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
@@ -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
@@ -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
@@ -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
PATCH 1/1 Make blame accept absolute paths
In reply to Junio C HamanoBlame 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
@@ -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
PATCH 1/1 More test cases for sanitized path names
In reply to Junio C HamanoVerify 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
@@ -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
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 ;-).
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 usetest_expect_failureto 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
Re: [PATCH] More test cases for sanitized path names
In reply to Junio C Hamanofredagen 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 usetest_expect_failureto mark tests that ought to pass but don't because of bugs in the commands. That way, people who are absolutely bored can grep fortest_expect_failureto 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
11a52821ca81096987f53c29bf1b9ce373fe7fd4Mon 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
@@ -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
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".
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_failureto 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 toget_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
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".
Re: [PATCH] More test cases for sanitized path names
In reply to Junio C HamanoQuoted 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.
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
@@ -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
@@ -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" )'
PATCH 1/1 Sane use of test_expect_failure
In reply to Junio C HamanoOriginally, 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 usetest_expect_failureto mark tests that ought to pass but don't because of bugs in the commands. That way, people who are absolutely bored can grep fortest_expect_failureto 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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:
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?
Re: [RFH/PATCH] prefix_path(): disallow absolute paths
In reply to Junio C HamanoQuoting 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
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.
Re: [PATCH] More test cases for sanitized path names
In reply to Robin RosenbergQuoted 11 lines · Robin Rosenberg
Verify a few more commands and pathname variants.
Signed-off-by Robin Rosenberg <[email protected]>--- 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
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.
Signed-off-by Robin Rosenberg <[email protected]>--- 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