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

-

Hi,


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

Hth,
Dscho
-

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

-

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

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---

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(-)

diff --git a/dir.c b/dir.c
index 3e345c2..9e5879a 100644
--- a/dir.c
+++ b/dir.c
@@ -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++;
-- 
1.5.4-rc2.GIT

-

Hi,



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

-

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?

-

Good catch, I didn't think of this case but this indeed will cause

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
-

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

-

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

   $ git ls-files -o /*bin

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

   $ git clean -f /

is basically a synonym for

   $ rm -rf /

-- Hannes

-

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
-

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

Noticed by Johannes Sixt.

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Mon, 28 Jan 2008, Johannes Sixt wrote:

	> Junio C Hamano schrieb:
	> > Johannes Sixt <j.sixt@viscovery.net> 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 applie...
[ message continues ]

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(-)

diff --git a/builtin-mv.c b/builtin-mv.c
index 990e2...
[ message continues ]

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?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

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

-

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.
-

Advance notice: In this function, tests of the kind *src == '/' need to be

			*dst++ = '/';




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

-

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?
-

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

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

-- Hannes

-

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

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

Hi,


I should have made more clear that I tried to avoid exactly that before 


This is ugly.  I would like this better:

			while (src[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

-

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


-

Hi,


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

-

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(-)

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 0f0ab2d..3801cf4 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -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) {
-

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

diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh
new file mode 100755
index 0000000..da20ba5
--- /dev/null
+++ b/t/t7010-setup.sh
@@ -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 &&...
[ message continues ]

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

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

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

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

but not as:

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

This patch updates prefix_path() in several ways:

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

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

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

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

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

Signed-off-by: Junio C Hamano <gitster@pobox....
[ message continues ]

Verify a few more commands and pathname variants.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 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

diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh
index da20ba5..60c4a46 100755
--- a/t/t7010-setup.sh
+++ b/t/t7010-setup.sh
@@ -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
-- 
1.5.4.rc4.25.g81cc

-

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

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 ;-).

-

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

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

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

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

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

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

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

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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Junio C Hamano <gitster@pobox.com> writes:

 > 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, peo...
[ message continues ]

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?
-

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.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
-

Oops. Remove the echo $?. It still fails, i.e. git add succeeds when
Can we move the default trash one level down for all tests? That
I respect that.

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

From 11a52821ca81096987f53c29bf1b9ce373fe7fd4 Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosenberg@dewire.com>
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.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 t/t7010-setup.sh |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh
index da20ba5..ef30099 100755
--- a/t/t7010-setup.sh
+++ b/t/t7010-setup.sh
@@ -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 cas...
[ message continues ]

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

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".
-

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


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
-

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".
-

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.

-

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(-)

diff --git a/builtin-add.c b/builtin-add.c
index 4a91e3e..820110e 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -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) {
diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh
index 60c4a46..e809e0e 100755
--- a/t/t7010-setup.sh
+++ b/t/t7010-setup.sh
@@ -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 ...
[ message continues ]

Blame did not always use prefix_path.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 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.

diff --git a/builtin-blame.c b/builtin-blame.c
index c7e6887..f88c32a 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -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);
 }
 
 /*
-- 
1.5.4.rc4.25.g81cc

-

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

 setup.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/setup.c b/setup.c
index 156d417..0688e7b 100644
--- a/setup.c
+++ b/setup.c
@@ -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;
 }
 
-

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

Noticed by Johannes Sixt.

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

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	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(-)

diff --git a/builtin-mv.c b/builtin-mv.c
index 990e213..94f6dd2 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -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;
diff --git a/setup.c b/setup.c
index 2174e78..5a4aadc 100644
--- a/setup.c
+++ b/setup.c
@@ -13,6 +13,8 @@ const ch...
[ message continues ]

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


-

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".

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/dir.c b/dir.c
index 3e345c2..9e5879a 100644
--- a/dir.c
+++ b/dir.c
@@ -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++;
-- 
1.5.4.rc5.1.g813e

-

Hi,


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

Ciao,
Dscho
-

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.

-

Sorry I should have been more explicit Johannes Sixt reported the issue,

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
-

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

-