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

Johannes Sixt schrieb:

Try this in your favorite git repo:

    git clean -n /

Here, it responds with:

    fatal: oops in prep_exclude

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

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

-- Hannes

Hi,

On Wed, 23 Jan 2008, Johannes Sixt wrote:

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

Johannes Schindelin schrieb:

Hi,

On Wed, 23 Jan 2008, Johannes Sixt wrote:

quoted

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

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

Signed-off-by: Shawn Bohrer <redacted>
---

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,

On Sun, 27 Jan 2008, Shawn Bohrer wrote:

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

Signed-off-by: Shawn Bohrer <redacted>
---

This fixes the oops part of the issue Johannes found,

have I?

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

On Sun, Jan 27, 2008 at 08:44:57PM +0000, Johannes Schindelin wrote:

Hi,

On Sun, 27 Jan 2008, Shawn Bohrer wrote:

quoted

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

Signed-off-by: Shawn Bohrer <redacted>
---

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

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