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

Johannes Sixt [off-list ref] 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?

Junio C Hamano schrieb:

Johannes Sixt [off-list ref] writes:

quoted

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.

-- 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 <redacted>
---
	On Mon, 28 Jan 2008, Johannes Sixt wrote:

	> Junio C Hamano schrieb:
	> > Johannes Sixt [off-list ref] 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(-)

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 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 != '.')

-- 
1.5.4.rc5.15.g8231f

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

	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 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 != '.')

diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
index 39fe267..cc5b982 100755
--- a/t/t3101-ls-tree-dirname.sh
+++ b/t/t3101-ls-tree-dirname.sh

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

-- 
1.5.4.rc5.15.g8231f