[PATCH] Do _not_ call unlink on a directory

[PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Calling unlink on a directory on a Solaris UFS filesystem as root makes it
inconsistent. Thanks to Johannes Sixt for the obvious fix.
---
 entry.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/entry.c b/entry.c
index 82bf725..1f2e34d 100644
--- a/entry.c
+++ b/entry.c
@@ -14,10 +14,10 @@ static void create_directories(const char *path, const struct checkout *state)
 		if (mkdir(buf, 0777)) {
 			if (errno == EEXIST) {
 				struct stat st;
-				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
-					continue;
 				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
 					continue; /* ok */
+				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
+					continue;
 			}
 			die("cannot create directory at %s", buf);
 		}
-- 
1.5.2.1

Re: [PATCH] Do _not_ call unlink on a directory

[Matthieu Moy]

Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:

I believe you still have a race condition if ...

> -				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
> -					continue;

... buf exists here as a file ...

>  				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
>  					continue; /* ok */

... and became a directory here.

> +				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
> +					continue;

But that's quite unlikely to happen. And I have no fix to propose.

-- 
Matthieu

Re: [PATCH] Do _not_ call unlink on a directory

[Scott Lamb]

Matthieu Moy wrote:
> Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:
> 
> I believe you still have a race condition if ...
> 
>> -				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
>> -					continue;
> 
> ... buf exists here as a file ...
> 
>>  				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
>>  					continue; /* ok */
> 
> ... and became a directory here.
> 
>> +				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
>> +					continue;
> 
> But that's quite unlikely to happen. And I have no fix to propose.
> 

If arbitrary other tasks are running, the only way to be absolutely
certain you're not calling unlink() in a directory is to never call
unlink().

SUS describes a safe remove(), but Solaris's implementation contains the
same race:

http://src.opensolaris.org/source/xref/pef/phase_I/usr/src/lib/libc/port/gen/rename.c

so I think this patch is the best that can be done.

Best regards,
Scott

-- 
Scott Lamb <http://www.slamb.org/>

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hello,

> If arbitrary other tasks are running, the only way to be absolutely
> certain you're not calling unlink() in a directory is to never call
> unlink().

there is one way to do it safe but it is so ugly that it is
unacceptable: don't call unlink as a privileged user (eg. root). So I
hope that one of the patches make it into git soon. I like the second
patch better because it does less system calls. Not that it matters.
For my co-workers I already build a git version with the patch in so
that they can continue to work as root. Don't even think about asking.

        Thomas

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hello,

> so I think this patch is the best that can be done.

is there a reason why we call unlink and not remove?

	Thomas

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Mon, 16 Jul 2007, Thomas Glanzmann wrote:
> 
> > so I think this patch is the best that can be done.
> 
> is there a reason why we call unlink and not remove?

Exactly because we only want to remove _files_.

If it's already a directory, we don't need to do anything at all (we just 
want to go to the next path component).

So what git wants is the modern "unlink()" behaviour that will return 
EPERM (oe EISDIR) for a directory. 

Not doing that in this day and age is *insane*. That whole "unlink/link" 
on directories is original UNIX, but it's original UNIX from several 
decades ago. It got fixed long long ago, and mkdir/rmdir have existed as 
system calls since at least SVR3. Nobody does the insane "unlink()" any 
more.

Except in Solaris, it would appear.

		Linus

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hello,

> > is there a reason why we call unlink and not remove?

> Exactly because we only want to remove _files_.

of course. That is the whole point. Call unlink for files, rmdir for
directories.

	Thomas

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Mon, 16 Jul 2007, Thomas Glanzmann wrote:
>
> Hello,
> 
> > > is there a reason why we call unlink and not remove?
> 
> > Exactly because we only want to remove _files_.
> 
> of course. That is the whole point. Call unlink for files, rmdir for
> directories.

No, but we don't *want* the "rmdir for directories" part! 

That's the whole point.

Calling "remove()" would be *wrong*. We want the *sane* "unlink()" 
behaviour, where it only removes files, and returns an error for 
directories.

		Linus

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hello,

> No, but we don't *want* the "rmdir for directories" part! 

that is what I meant. We call unlink because we want unlink to _fail_ on
directories while it deletes file. I forgot about the original
discussion but Johannes refreshed my memory.  If a file in our history
becomes a directory we want to get it out of our way. And we want to do
that by call unlink.

	Thomas

Re: [PATCH] Do _not_ call unlink on a directory

[Scott Lamb]

Linus Torvalds wrote:
> No, but we don't *want* the "rmdir for directories" part! 
> 
> That's the whole point.
> 
> Calling "remove()" would be *wrong*. We want the *sane* "unlink()" 
> behaviour, where it only removes files, and returns an error for 
> directories.

Of course, but when used immediately after stat() says the path does not
refer to a directory, I would prefer SUS remove() (rmdir() for
directories) to Solaris unlink() (break_filesystem() on directories).

But Solaris remove() is broken, too, so it's a moot point. The
post-patch behavior is good enough - as you said, it won't happen during
reasonable usage and the problem's not unique to git.

Best regards,
Scott

-- 
Scott Lamb <http://www.slamb.org/>

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Mon, 16 Jul 2007, Scott Lamb wrote:
> 
> But Solaris remove() is broken, too, so it's a moot point.

In fact, with the Solaris behaviour for unlink(), you *cannot* have a 
non-broken "remove()".

So the right fix is always to fix "unlink()" instead.

There really aren't any downsides (since no program can rely on it 
_anyway_, unless we're talking about some magic "early bootup" time 
scripts that depend on only running as root, and only ever running on UFS 
- but those kinds of scripts could be trivially fixed and are obviously 
under Sun control anyway)

			Linus

Re: [PATCH] Do _not_ call unlink on a directory

[Scott Lamb]

Linus Torvalds wrote:
> 
> On Mon, 16 Jul 2007, Scott Lamb wrote:
>> But Solaris remove() is broken, too, so it's a moot point.
> 
> In fact, with the Solaris behaviour for unlink(), you *cannot* have a 
> non-broken "remove()".

I'd hoped to see that they made a new syscall to properly implement the
new behavior. But they didn't. It reminds me of glibc's pselect().

Best regards,
Scott

-- 
Scott Lamb <http://www.slamb.org/>

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Mon, 16 Jul 2007, Scott Lamb wrote:
> Linus Torvalds wrote:
> > 
> > In fact, with the Solaris behaviour for unlink(), you *cannot* have a 
> > non-broken "remove()".
> 
> I'd hoped to see that they made a new syscall to properly implement the
> new behavior.

Ahh, yes, with a new system call you could do it.

> But they didn't. It reminds me of glibc's pselect().

Yeah, that was a bit pointless, although it does make it easier to port 
binaries and then have them to work in practice most of the time.

		Linus

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Mon, 16 Jul 2007, Linus Torvalds wrote:
>
> [..] mkdir/rmdir have existed as 
> system calls since at least SVR3.

Correction. Apparently since 4.2BSD (1983).

		Linus

[PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Calling unlink on a directory on a Solaris UFS filesystem as root makes it
inconsistent. Thanks to Johannes Sixt for the obvious fix.

Signed-off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
---
 entry.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/entry.c b/entry.c
index 82bf725..907293f 100644
--- a/entry.c
+++ b/entry.c
@@ -6,18 +6,18 @@ static void create_directories(const char *path, const struct checkout *state)
 	int len = strlen(path);
 	char *buf = xmalloc(len + 1);
 	const char *slash = path;
+        struct stat st;
 
 	while ((slash = strchr(slash+1, '/')) != NULL) {
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
+                if (!stat(buf, &st) && S_ISDIR(st.st_mode))
+                        continue; /* ok */
 		if (mkdir(buf, 0777)) {
 			if (errno == EEXIST) {
-				struct stat st;
 				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
 					continue;
-				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
-					continue; /* ok */
 			}
 			die("cannot create directory at %s", buf);
 		}
-- 
1.5.2.1

Re: [PATCH] Do _not_ call unlink on a directory

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]

On Mon, 2007-07-16 19:38:41 +0200, Thomas Glanzmann <sithglan@stud.uni-erlangen.de> wrote:
> Calling unlink on a directory on a Solaris UFS filesystem as root makes it
> inconsistent. Thanks to Johannes Sixt for the obvious fix.
> 
> Signed-off-by: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
> ---
>  entry.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/entry.c b/entry.c
> index 82bf725..907293f 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -6,18 +6,18 @@ static void create_directories(const char *path, const struct checkout *state)
>  	int len = strlen(path);
>  	char *buf = xmalloc(len + 1);
>  	const char *slash = path;
> +        struct stat st;

Whitespace damage.

>  	while ((slash = strchr(slash+1, '/')) != NULL) {
>  		len = slash - path;
>  		memcpy(buf, path, len);
>  		buf[len] = 0;
> +                if (!stat(buf, &st) && S_ISDIR(st.st_mode))
> +                        continue; /* ok */

Dito.

>  		if (mkdir(buf, 0777)) {
>  			if (errno == EEXIST) {
> -				struct stat st;
>  				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
>  					continue;
> -				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
> -					continue; /* ok */
>  			}
>  			die("cannot create directory at %s", buf);
>  		}

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:         "really soon now":      an unspecified period of time, likly to
the second  :                                 be greater than any reasonable definition
                                              of "soon".

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Do _not_ call unlink on a directory

[Brian Downing]

On Mon, Jul 16, 2007 at 07:38:41PM +0200, Thomas Glanzmann wrote:
>  	const char *slash = path;
> +        struct stat st;

>  		memcpy(buf, path, len);
>  		buf[len] = 0;
> +                if (!stat(buf, &st) && S_ISDIR(st.st_mode))
> +                        continue; /* ok */

You've got some whitespace damage here.  Git's style is to use tabs.

-bcd

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hallo Brian,

> You've got some whitespace damage here.  Git's style is to use tabs.

I have expandtab in my vim config which writes 8 spaces instead of tabs.
I should change that for git.

        Thomas

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Mon, 16 Jul 2007, Thomas Glanzmann wrote:
>
> Calling unlink on a directory on a Solaris UFS filesystem as root makes it
> inconsistent. Thanks to Johannes Sixt for the obvious fix.

Ack, I think this is the right thing to do.

As pointed out, it doesn't _guarantee_ that git won't call "unlink()" on a 
directory (race conditions etc), but that's fundamentally true (there is 
no "funlink()" like there is "fstat()"), and besides, that is in no way 
git-specific (ie it's true of *any* application that gets run as root).

The theoretical race would only happen if somebody on purpose tries to 
screw things over, it would never happen under any reasonable usage. 

The old ordering of those tests was designed for sane operating systems, 
so that you could basically do the unlink() without bothering, but 
switching the order around is certainly not a disaster either, and if it 
avoids the nasty bug in Solaris it's worth doing.

I have to say that I'm still a bit shocked that Solaris would have that 
kind of behaviour. And they call that pile of sh*t "enterprise class"..

		Linus

Re: [PATCH] Do _not_ call unlink on a directory

[Brian Downing]

On Mon, Jul 16, 2007 at 12:58:14PM -0700, Linus Torvalds wrote:
> I have to say that I'm still a bit shocked that Solaris would have that 
> kind of behaviour. And they call that pile of sh*t "enterprise class"..

Apparently "enterprise class" in this case really means "fully
compatibility with all those wonderful userland implementations of
rmdir."  :-)

-bcd

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Mon, 16 Jul 2007, Brian Downing wrote:
> 
> Apparently "enterprise class" in this case really means "fully
> compatibility with all those wonderful userland implementations of
> rmdir."  :-)

I see the smiley, but it's actually not possible even for that.

The broken "unlink()" behaviour doesn't work on any other filesystem (eg 
NFS) at all anyway, and even on UFS would only work for root (or 
setuid-root) binaries. So any user-land that depended on it literally 
wouldn't work _anyway_, even on Solaris itself.

And we're talking about the same company that was *famous* for screwing 
people over when they converted from SunOS to Solaris and broke binaries 
_and_ source code in the process.

So no, "compatibility" can't realistically be the reason.

			Linus

Re: [PATCH] Do _not_ call unlink on a directory

[David Kastrup]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, 16 Jul 2007, Thomas Glanzmann wrote:
>>
>> Calling unlink on a directory on a Solaris UFS filesystem as root makes it
>> inconsistent. Thanks to Johannes Sixt for the obvious fix.
>
> Ack, I think this is the right thing to do.
>
> As pointed out, it doesn't _guarantee_ that git won't call
> "unlink()" on a directory (race conditions etc), but that's
> fundamentally true (there is no "funlink()" like there is
> "fstat()"), and besides, that is in no way git-specific (ie it's
> true of *any* application that gets run as root).

Please note that doing "remove" before "mkdir" without checking for
directoriness still offers a race window where one can slip in a new
non-directory file.

-- 
David Kastrup

Re: [PATCH] Do _not_ call unlink on a directory

[Junio C Hamano]

Thanks.

Re: [PATCH] Do _not_ call unlink on a directory

[Junio C Hamano]

Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:

> Calling unlink on a directory on a Solaris UFS filesystem as root makes it
> inconsistent. Thanks to Johannes Sixt for the obvious fix.
> ---
>  entry.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 82bf725..1f2e34d 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -14,10 +14,10 @@ static void create_directories(const char *path, const struct checkout *state)
>  		if (mkdir(buf, 0777)) {
>  			if (errno == EEXIST) {
>  				struct stat st;
> -				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
> -					continue;
>  				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
>  					continue; /* ok */
> +				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
> +					continue;
>  			}
>  			die("cannot create directory at %s", buf);
>  		}
> -- 
> 1.5.2.1

This is wrong.  If the filesystem has a symlink and we would
want a directory there, we should unlink().  So at least the
stat there needs to be lstat().

I wonder if anybody involved in the discussion has actually
tested this patch (or the other one, that has the same problem)?

Does the following replacement work for you?  It adds far more
lines than your version, but they are mostly comments to make it
clear why we do things this way.

---
 entry.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/entry.c b/entry.c
index f9e7dc5..42fda36 100644
--- a/entry.c
+++ b/entry.c
@@ -8,17 +8,40 @@ static void create_directories(const char *path, const struct checkout *state)
 	const char *slash = path;
 
 	while ((slash = strchr(slash+1, '/')) != NULL) {
+		struct stat st;
+		int stat_status;
+
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
+
+		if (len <= state->base_dir_len)
+			/*
+			 * checkout-index --prefix=<dir>; <dir> is
+			 * allowed to be a symlink to an existing
+			 * directory.
+			 */
+			stat_status = stat(buf, &st);
+		else
+			/*
+			 * if there currently is a symlink, we would
+			 * want to replace it with a real directory.
+			 */
+			stat_status = lstat(buf, &st);
+
+		if (!stat_status && S_ISDIR(st.st_mode))
+			continue; /* ok, it is already a directory. */
+		
+		/*
+		 * We know stat_status == 0 means something exists
+		 * there and this mkdir would fail, but that is an
+		 * error codepath; we do not care, as we unlink and
+		 * mkdir again in such a case.
+		 */
 		if (mkdir(buf, 0777)) {
-			if (errno == EEXIST) {
-				struct stat st;
-				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
-					continue; /* ok */
-				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
-					continue;
-			}
+			if (errno == EEXIST && state->force &&
+			    !unlink(buf) && !mkdir(buf, 0777))
+				continue;
 			die("cannot create directory at %s", buf);
 		}
 	}

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hello Junio,

> This is wrong.  If the filesystem has a symlink and we would want a
> directory there, we should unlink().  So at least the stat there needs
> to be lstat().

I see.

> I wonder if anybody involved in the discussion has actually
> tested this patch (or the other one, that has the same problem)?

I tested it. But I did not test it with symlinks.

> Does the following replacement work for you?  It adds far more lines
> than your version, but they are mostly comments to make it clear why
> we do things this way.

Yes, it does. Excuse the delay but my build machine is not the fastest.

	(faui04a) [/var/tmp] git clone ~/work/repositories/public/easix.git test-10
	Initialized empty Git repository in /var/tmp/test-10/.git/
	remote: Generating pack...
	remote: Done counting 317 objects.
	remote: Deltifying 317 objects...
	remote: te: % (317/317) done: ) done
	Indexing 317 objects...
	remote: Total 317 (delta 182), reused 278 (delta 157)
	100% (317/317) done
	Resolving 182 deltas...
	100% (182/182) done
	(faui04a) [/var/tmp] cd test-10
	./test-10
	(faui04a) [/var/tmp/test-10] git status
	# On branch master
	nothing to commit (working directory clean)

I rebased your patch on top of current HEAD (as I can access it on
git.kernel.org) and removed trailing whitspace from one line (git-apply
complained)

	Thomas

>From 3b60b807007507ce5e1f8490f1469dac5bb95917 Mon Sep 17 00:00:00 2001
From: Thomas Glanzmann <sithglan@stud.uni-erlangen.de>
Date: Tue, 17 Jul 2007 11:31:07 +0200
Subject: [PATCH] Do _not_ call unlink on a directory

Calling unlink on a directory on a Solaris UFS filesystem as root makes it
inconsistent. Thanks to Junio for the not so obvious fix.
---
 entry.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/entry.c b/entry.c
index c540ae1..0625112 100644
--- a/entry.c
+++ b/entry.c
@@ -8,17 +8,40 @@ static void create_directories(const char *path, const struct checkout *state)
 	const char *slash = path;
 
 	while ((slash = strchr(slash+1, '/')) != NULL) {
+		struct stat st;
+		int stat_status;
+
 		len = slash - path;
 		memcpy(buf, path, len);
 		buf[len] = 0;
+
+		if (len <= state->base_dir_len)
+			/*
+			 * checkout-index --prefix=<dir>; <dir> is
+			 * allowed to be a symlink to an existing
+			 * directory.
+			 */
+			stat_status = stat(buf, &st);
+		else
+			/*
+			 * if there currently is a symlink, we would
+			 * want to replace it with a real directory.
+			 */
+			stat_status = lstat(buf, &st);
+
+		if (!stat_status && S_ISDIR(st.st_mode))
+			continue; /* ok, it is already a directory. */
+
+		/*
+		 * We know stat_status == 0 means something exists
+		 * there and this mkdir would fail, but that is an
+		 * error codepath; we do not care, as we unlink and
+		 * mkdir again in such a case.
+		 */
 		if (mkdir(buf, 0777)) {
-			if (errno == EEXIST) {
-				struct stat st;
-				if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777))
-					continue;
-				if (!stat(buf, &st) && S_ISDIR(st.st_mode))
-					continue; /* ok */
-			}
+			if (errno == EEXIST && state->force &&
+			    !unlink(buf) && !mkdir(buf, 0777))
+				continue;
 			die("cannot create directory at %s", buf);
 		}
 	}
-- 
1.5.2.1

Re: [PATCH] Do _not_ call unlink on a directory

[Junio C Hamano]

Thomas Glanzmann <thomas@glanzmann.de> writes:

>> I wonder if anybody involved in the discussion has actually
>> tested this patch (or the other one, that has the same problem)?
>
> I tested it. But I did not test it with symlinks.
>
>> Does the following replacement work for you?  It adds far more lines
>> than your version, but they are mostly comments to make it clear why
>> we do things this way.
>
> Yes, it does. Excuse the delay but my build machine is not the fastest.
>
> 	(faui04a) [/var/tmp] git clone ~/work/repositories/public/easix.git test-10
> 	Initialized empty Git repository in /var/tmp/test-10/.git/
> 	remote: Generating pack...
> 	remote: Done counting 317 objects.
> 	remote: Deltifying 317 objects...
> 	remote: te: % (317/317) done: ) done
> 	Indexing 317 objects...
> 	remote: Total 317 (delta 182), reused 278 (delta 157)
> 	100% (317/317) done
> 	Resolving 182 deltas...
> 	100% (182/182) done
> 	(faui04a) [/var/tmp] cd test-10
> 	./test-10
> 	(faui04a) [/var/tmp/test-10] git status
> 	# On branch master
> 	nothing to commit (working directory clean)

Ahhhh, by "testing", I meant "runnnig the testsuite shipped with
the source".  Both of your patches were failing in somewhere in
t2000 series of tests.

> I rebased your patch on top of current HEAD (as I can access it on
> git.kernel.org) and removed trailing whitspace from one line (git-apply
> complained)

I am thinking that this fix should go to 'maint' and merged to
'master', as it is a grave problem in at least one setup.

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hello Junio,

> Ahhhh, by "testing", I meant "runnnig the testsuite shipped with
> the source".  Both of your patches were failing in somewhere in
> t2000 series of tests.

That was the last time, I am going to submit a patch _without_ running
the whole testsuite before. I hate it myself when other people don't do
the obvious tests and break something that worked before.

> I am thinking that this fix should go to 'maint' and merged to
> 'master', as it is a grave problem in at least one setup.

Thanks. For packages that I distribute, I fixed it of course by myself.
And to be precise I use git on Solaris a lot by myself but I don't work
as root so the bug never showed up before and as you can see by the
pastes that I provided to track down the bug I have

	if [ $UID -eq 0 ]; then
		export PS1="(${PROMPT_RED}\h${PROMPT_END}) [${PROMPT_BLUE}\w${PROMPT_END}] ";
		alias bk='echo DO *NOT* RUN BK AS ROOT'
		alias git='echo DO *NOT* RUN GIT AS ROOT'
		alias links='echo DO *NOT* RUN LINKS AS ROOT'
		alias elinks='echo DO *NOT* RUN ELINKS AS ROOT'
	...

in my distributed environment. But my coworker who I "show" git to work
a lot as root. A very bad habbit that is hard to get rid of. Btw. I
prepare to setup a automatic build script which I am going to let run
automatic on a daily basis so that I catch Solaris compile problems
early and report them to you.

	Thomas

Re: [PATCH] Do _not_ call unlink on a directory

[Johannes Sixt]

Thomas Glanzmann wrote:
> and as you can see by the
> pastes that I provided to track down the bug I have
> 
>         if [ $UID -eq 0 ]; then
>                 export PS1="(${PROMPT_RED}\h${PROMPT_END}) [${PROMPT_BLUE}\w${PROMPT_END}] ";
>                 alias bk='echo DO *NOT* RUN BK AS ROOT'
>                 alias git='echo DO *NOT* RUN GIT AS ROOT'
>                 alias links='echo DO *NOT* RUN LINKS AS ROOT'
>                 alias elinks='echo DO *NOT* RUN ELINKS AS ROOT'

And if you have a file NOTES in $pwd, it will tell you:

DO NOTES RUN GIT AS ROOT

;-P

-- Hannes

Re: [PATCH] Do _not_ call unlink on a directory

[Thomas Glanzmann]

Hello,

> >                 alias elinks='echo DO *NOT* RUN ELINKS AS ROOT'

> And if you have a file NOTES in $pwd, it will tell you:
> DO NOTES RUN GIT AS ROOT

fixed.

	Thomas

Re: [PATCH] Do _not_ call unlink on a directory

[Linus Torvalds]

On Tue, 17 Jul 2007, Junio C Hamano wrote:
> 
> This is wrong.  If the filesystem has a symlink and we would
> want a directory there, we should unlink().  So at least the
> stat there needs to be lstat().

Good catch. Ack.

		Linus