Git changes permissions on directories when deleting files.

Git changes permissions on directories when deleting files.

[Chad Joan]

Hello,

What I'm experiencing is this:

$ cd ~/project
$ ls -dl somedir
drwxrwx--- 1 cjoan cjoan 0 Feb 28 19:57 somedir
$ echo "some text" > somedir/somefile.txt
$ git add somedir/somefile.txt
$ git rm -f somedir/somefile.txt
rm 'somedir/somefile.txt'
$ ls -dl somedir
drw------- 1 cjoan cjoan 0 Feb 28 19:57 somedir
$ echo "some text" > somedir/somefile.txt
bash: somedir/somefile.txt: Permission denied

~/project is actually a CIFS mount, with the host being an OpenVMS machine.

If I use the normal rm command without using git then the permissions
will remain the same on 'somedir'.  This is why I suspect (and hope)
this isn't OpenVMS related.

It seems that execute bit is important for CIFS mounted files, because
after this happens I am no longer able to do /anything/ within the
'somedir' directory.  This also affects branches via "git checkout
branchname": if the checkout happens to delete files then this will
happen, and it will salt the wound by failing to sync a bunch of files
in 'somedir' (because I can't access them anymore) while still moving
HEAD to the new branch.

The share on the CIFS host looks like this:

[homes]
        comment = Home Directories
        read only = No
        create mask = 0770
        browseable = No
        vfs objects = varvfc
        vms path names = No
        case sensitive = Yes

The fstab entry for the mount looks like this:

//vms/homes  /home/cjoan/project  cifs
credentials=/home/cjoan/.cifs_credentials,_netdev,uid=cjoan,gid=cjoan
0 0

I'd really like my directories to keep their permissions.
Any idea what might cause this?

- Chad

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

I should probably also mention that my git version is 1.7.3.4-r1

Re: Git changes permissions on directories when deleting files.

[Computer Druid]

On Mon, Feb 28, 2011 at 8:42 PM, Chad Joan <chadjoan@gmail.com> wrote:
> Hello,
>
> What I'm experiencing is this:
>
> $ cd ~/project
> $ ls -dl somedir
> drwxrwx--- 1 cjoan cjoan 0 Feb 28 19:57 somedir
> $ echo "some text" > somedir/somefile.txt
> $ git add somedir/somefile.txt
> $ git rm -f somedir/somefile.txt
> rm 'somedir/somefile.txt'
> $ ls -dl somedir
> drw------- 1 cjoan cjoan 0 Feb 28 19:57 somedir
> $ echo "some text" > somedir/somefile.txt
> bash: somedir/somefile.txt: Permission denied

After you remove the file, is "somedir" empty?

Git doesn't track empty directories, and therefore git rm on the last
file in a directory deletes it:

% git init
Initialized empty Git repository in /home/cdruid/testrepo/.git/
% mkdir dir
% ls -l
total 4
drwxr-xr-x 2 cdruid cdruid 4096 Feb 28 21:14 dir
% touch dir/test.txt
% git add dir/test.txt
% git rm -f dir/test.txt
rm 'dir/test.txt'
% ls -l
total 0

My guess is git is somehow failing to delete the directory, thus
causing your changed permissions issue.

-Dan Johnson

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

On Mon, Feb 28, 2011 at 9:19 PM, Computer Druid <computerdruid@gmail.com> wrote:
> On Mon, Feb 28, 2011 at 8:42 PM, Chad Joan <chadjoan@gmail.com> wrote:
>> Hello,
>>
>> What I'm experiencing is this:
>>
>> $ cd ~/project
>> $ ls -dl somedir
>> drwxrwx--- 1 cjoan cjoan 0 Feb 28 19:57 somedir
>> $ echo "some text" > somedir/somefile.txt
>> $ git add somedir/somefile.txt
>> $ git rm -f somedir/somefile.txt
>> rm 'somedir/somefile.txt'
>> $ ls -dl somedir
>> drw------- 1 cjoan cjoan 0 Feb 28 19:57 somedir
>> $ echo "some text" > somedir/somefile.txt
>> bash: somedir/somefile.txt: Permission denied
>
> After you remove the file, is "somedir" empty?
>

Nope.

> Git doesn't track empty directories, and therefore git rm on the last
> file in a directory deletes it:
>
> % git init
> Initialized empty Git repository in /home/cdruid/testrepo/.git/
> % mkdir dir
> % ls -l
> total 4
> drwxr-xr-x 2 cdruid cdruid 4096 Feb 28 21:14 dir
> % touch dir/test.txt
> % git add dir/test.txt
> % git rm -f dir/test.txt
> rm 'dir/test.txt'
> % ls -l
> total 0
>
> My guess is git is somehow failing to delete the directory, thus
> causing your changed permissions issue.
>
> -Dan Johnson
>

'somedir' still has plenty of files in it after the deletion, so I'm
afraid this isn't the case.

- Chad

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

More info:

$ mkdir foo
$ mkdir foo/bar
$ echo "test" > foo/bar/baz.txt
$ echo "somestuff" > foo/bar/somefile.txt
$ git add foo/bar/*
$ ls -dl foo
drwxr-x--x 1 cjoan cjoan 0 Mar  1 10:46 foo
$ ls -dl foo/bar
drwxr-x--x 1 cjoan cjoan 0 Mar  1 10:46 foo/bar
$ git rm -f foo/bar/somefile.txt
rm 'foo/bar/somefile.txt'
$ ls -dl foo
drwxr-x--x 1 cjoan cjoan 0 Mar  1 10:46 foo
$ ls -dl foo/bar
drw------- 1 cjoan cjoan 0 Mar  1 10:47 foo/bar
$ git rm -f foo/bar/baz.txt
rm 'foo/bar/baz.txt'
fatal: git rm: 'foo/bar/baz.txt': Permission denied



This time I tried it with git 1.7.4.

Re: Git changes permissions on directories when deleting files.

[Computer Druid]

On Tue, Mar 1, 2011 at 10:51 AM, Chad Joan <chadjoan@gmail.com> wrote:
> More info:
>
> $ mkdir foo
> $ mkdir foo/bar
> $ echo "test" > foo/bar/baz.txt
> $ echo "somestuff" > foo/bar/somefile.txt
What happens if you "rmdir foo/bar" here? (while there are files still in it)

-Dan Johnson

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

On Tue, Mar 1, 2011 at 12:11 PM, Computer Druid <computerdruid@gmail.com> wrote:
> On Tue, Mar 1, 2011 at 10:51 AM, Chad Joan <chadjoan@gmail.com> wrote:
>> More info:
>>
>> $ mkdir foo
>> $ mkdir foo/bar
>> $ echo "test" > foo/bar/baz.txt
>> $ echo "somestuff" > foo/bar/somefile.txt
> What happens if you "rmdir foo/bar" here? (while there are files still in it)
>
> -Dan Johnson
>

Something fairly interesting:

$ mkdir foo
$ mkdir foo/bar
$ ls -dl foo/bar
drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo/bar
$ ls -dl foo
drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
$ echo "test" > foo/bar/baz.txt
$ echo "somestuff" > foo/bar/somefile.txt
$ ls -dl foo/bar
drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo/bar
$ ls -dl foo
drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
$ rmdir foo/bar
rmdir: failed to remove `foo/bar': Directory not empty
$ ls -dl foo/bar
drw------- 1 cjoan cjoan 0 Mar  1 14:32 foo/bar
$ ls -dl foo
drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo


The rmdir fails of course, but it also changes the permissions.
So I take it that git always runs an rmdir on the parent directory
when it removes a file?  Seems like it would be a sensible way to do
it on a system without this behavior.

- Chad

Re: Git changes permissions on directories when deleting files.

[Jeff King]

On Tue, Mar 01, 2011 at 02:35:41PM -0500, Chad Joan wrote:

> Something fairly interesting:
> 
> $ mkdir foo
> $ mkdir foo/bar
> $ ls -dl foo/bar
> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo/bar
> $ ls -dl foo
> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
> $ echo "test" > foo/bar/baz.txt
> $ echo "somestuff" > foo/bar/somefile.txt
> $ ls -dl foo/bar
> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo/bar
> $ ls -dl foo
> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
> $ rmdir foo/bar
> rmdir: failed to remove `foo/bar': Directory not empty
> $ ls -dl foo/bar
> drw------- 1 cjoan cjoan 0 Mar  1 14:32 foo/bar
> $ ls -dl foo
> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
> 
> 
> The rmdir fails of course, but it also changes the permissions.
> So I take it that git always runs an rmdir on the parent directory
> when it removes a file?  Seems like it would be a sensible way to do
> it on a system without this behavior.

Exactly. Rather than spend time figuring out if the directory is
removable (which would not be atomic, anyway), we just rmdir and ignore
the error condition.

I would argue that your filesystem is broken. Even if we implemented a
workaround to opendir() and check for files, it would still have a race
condition that could cause this situation to occur.

-Peff

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

On Tue, Mar 1, 2011 at 2:44 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Mar 01, 2011 at 02:35:41PM -0500, Chad Joan wrote:
>
>> Something fairly interesting:
>>
>> $ mkdir foo
>> $ mkdir foo/bar
>> $ ls -dl foo/bar
>> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo/bar
>> $ ls -dl foo
>> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
>> $ echo "test" > foo/bar/baz.txt
>> $ echo "somestuff" > foo/bar/somefile.txt
>> $ ls -dl foo/bar
>> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo/bar
>> $ ls -dl foo
>> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
>> $ rmdir foo/bar
>> rmdir: failed to remove `foo/bar': Directory not empty
>> $ ls -dl foo/bar
>> drw------- 1 cjoan cjoan 0 Mar  1 14:32 foo/bar
>> $ ls -dl foo
>> drwxr-x--x 1 cjoan cjoan 0 Mar  1 14:31 foo
>>
>>
>> The rmdir fails of course, but it also changes the permissions.
>> So I take it that git always runs an rmdir on the parent directory
>> when it removes a file?  Seems like it would be a sensible way to do
>> it on a system without this behavior.
>
> Exactly. Rather than spend time figuring out if the directory is
> removable (which would not be atomic, anyway), we just rmdir and ignore
> the error condition.
>
> I would argue that your filesystem is broken. Even if we implemented a
> workaround to opendir() and check for files, it would still have a race
> condition that could cause this situation to occur.
>
> -Peff
>

Ouch.

Would it work to do something like alias rmdir to a script or program
that would call /bin/rmdir and then fix up the permissions?

Re: Git changes permissions on directories when deleting files.

[Jeff King]

On Tue, Mar 01, 2011 at 02:57:19PM -0500, Chad Joan wrote:

> > Exactly. Rather than spend time figuring out if the directory is
> > removable (which would not be atomic, anyway), we just rmdir and ignore
> > the error condition.
> >
> > I would argue that your filesystem is broken. Even if we implemented a
> > workaround to opendir() and check for files, it would still have a race
> > condition that could cause this situation to occur.
> 
> Ouch.
> 
> Would it work to do something like alias rmdir to a script or program
> that would call /bin/rmdir and then fix up the permissions?

Well, we're using the rmdir system call, so you would need a patch to
git either way. If that was something we wanted to support (with a
config option, of course), we could do the permissions check-and-restore
ourselves.

But it just seems horribly broken to me. This is CIFS to an OpenVMS
machine you said? Do the broken permissions appear to other clients or
across a remount (i.e., is it broken state in your CIFS client, or has
the server actually munged permissions)? If so, have you tried reporting
the issue to whoever writes CIFS server on OpenVMS (is it just samba)?

-Peff

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

On Tue, Mar 1, 2011 at 3:08 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Mar 01, 2011 at 02:57:19PM -0500, Chad Joan wrote:
>
>> > Exactly. Rather than spend time figuring out if the directory is
>> > removable (which would not be atomic, anyway), we just rmdir and ignore
>> > the error condition.
>> >
>> > I would argue that your filesystem is broken. Even if we implemented a
>> > workaround to opendir() and check for files, it would still have a race
>> > condition that could cause this situation to occur.
>>
>> Ouch.
>>
>> Would it work to do something like alias rmdir to a script or program
>> that would call /bin/rmdir and then fix up the permissions?
>
> Well, we're using the rmdir system call, so you would need a patch to
> git either way. If that was something we wanted to support (with a
> config option, of course), we could do the permissions check-and-restore
> ourselves.
>
> But it just seems horribly broken to me. This is CIFS to an OpenVMS
> machine you said? Do the broken permissions appear to other clients or
> across a remount (i.e., is it broken state in your CIFS client, or has
> the server actually munged permissions)? If so, have you tried reporting
> the issue to whoever writes CIFS server on OpenVMS (is it just samba)?
>
> -Peff
>

Yep, CIFS to OpenVMS.

I don't know about other clients because there are none (yet).  The
permission change does survive remounting.

I haven't reported it.  I didn't know it existed until now ;)

I'll do that, but it will probably take a long long time for me to see
the patch.  I'm hoping there's some cheap hack I can use to work
around it in the meantime.

- Chad

Re: Git changes permissions on directories when deleting files.

[Computer Druid]

On Tue, Mar 1, 2011 at 3:30 PM, Chad Joan <chadjoan@gmail.com> wrote:
> On Tue, Mar 1, 2011 at 3:08 PM, Jeff King <peff@peff.net> wrote:
>> But it just seems horribly broken to me. This is CIFS to an OpenVMS
>> machine you said? Do the broken permissions appear to other clients or
>> across a remount (i.e., is it broken state in your CIFS client, or has
>> the server actually munged permissions)? If so, have you tried reporting
>> the issue to whoever writes CIFS server on OpenVMS (is it just samba)?
>>
>> -Peff
>>
>
> Yep, CIFS to OpenVMS.
>
> I don't know about other clients because there are none (yet).  The
> permission change does survive remounting.
>
> I haven't reported it.  I didn't know it existed until now ;)
>
> I'll do that, but it will probably take a long long time for me to see
> the patch.  I'm hoping there's some cheap hack I can use to work
> around it in the meantime.

A simple answer to preventing git from calling rmdir would be to run
rm and git rm separately:
$ rm file
$ git rm --cached -f file

But that doesn't solve the misbehavior of git under the previous
scenario. I'm not sure if this is something we should fix in git or if
it should be fixed in cifs.

Re: Git changes permissions on directories when deleting files.

[Matthieu Moy]

Chad Joan <chadjoan@gmail.com> writes:

> I'll do that, but it will probably take a long long time for me to see
> the patch.  I'm hoping there's some cheap hack I can use to work
> around it in the meantime.

I'd say grep for "rmdir" is Git's source code, and replace the calls
with a wrapper that does roughly

rmdir_wrapper(dir) {
	rmdir(dir);
	if (stat(dir, &buf))
		chmod(dir, buf.st_mode | 0777);
}

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

On Tue, Mar 1, 2011 at 3:43 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Chad Joan <chadjoan@gmail.com> writes:
>
>> I'll do that, but it will probably take a long long time for me to see
>> the patch.  I'm hoping there's some cheap hack I can use to work
>> around it in the meantime.
>
> I'd say grep for "rmdir" is Git's source code, and replace the calls
> with a wrapper that does roughly
>
> rmdir_wrapper(dir) {
>        rmdir(dir);
>        if (stat(dir, &buf))
>                chmod(dir, buf.st_mode | 0777);
> }
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>

OK, I'll try that when I get a chance.

- Chad

Re: Git changes permissions on directories when deleting files.

[Jeff King]

On Tue, Mar 01, 2011 at 03:39:10PM -0500, Computer Druid wrote:

> > I'll do that, but it will probably take a long long time for me to see
> > the patch.  I'm hoping there's some cheap hack I can use to work
> > around it in the meantime.
> 
> A simple answer to preventing git from calling rmdir would be to run
> rm and git rm separately:
> $ rm file
> $ git rm --cached -f file
> 
> But that doesn't solve the misbehavior of git under the previous
> scenario. I'm not sure if this is something we should fix in git or if
> it should be fixed in cifs.

That will fix some instances. But git will rmdir to clean up anytime it
removes content. That includes during a merge or patch application. So
you can't really get around those cases.

-Peff

Re: Git changes permissions on directories when deleting files.

[Jeff King]

On Tue, Mar 01, 2011 at 03:46:46PM -0500, Chad Joan wrote:

> >> I'll do that, but it will probably take a long long time for me to see
> >> the patch.  I'm hoping there's some cheap hack I can use to work
> >> around it in the meantime.
> >
> > I'd say grep for "rmdir" is Git's source code, and replace the calls
> > with a wrapper that does roughly
> >
> > rmdir_wrapper(dir) {
> >        rmdir(dir);
> >        if (stat(dir, &buf))
> >                chmod(dir, buf.st_mode | 0777);
> > }
> >
> OK, I'll try that when I get a chance.

I think this is the cheap hack that you want:

diff --git a/dir.c b/dir.c
index 168dad6..fb6d306 100644
--- a/dir.c
+++ b/dir.c
@@ -1236,6 +1236,29 @@ void setup_standard_excludes(struct dir_struct *dir)
 		add_excludes_from_file(dir, excludes_file);
 }
 
+static int rmdir_on_broken_cifs(const char *path)
+{
+	struct stat sb;
+	if (stat(path, &sb) < 0) {
+		/* Oh well, hopefully if we can't stat it
+		 * it is already gone or we don't have
+		 * permissions to screw it up anyway. */
+		return rmdir(path);
+	}
+	if (rmdir(path) == 0) {
+		/* it worked, nothing to restore */
+		return 0;
+	}
+	/* maybe remove this conditional if you can trigger
+	 * the problem with other types of errors */
+	if (errno != ENOTEMPTY)
+		return -1;
+	if (chmod(path, sb.st_mode) < 0)
+		warning("we probably just screwed up the permissions of %s",
+			path);
+	return -1;
+}
+
 int remove_path(const char *name)
 {
 	char *slash;
@@ -1249,7 +1272,7 @@ int remove_path(const char *name)
 		slash = dirs + (slash - name);
 		do {
 			*slash = '\0';
-		} while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
+		} while (rmdir_on_broken_cifs(dirs) == 0 && (slash = strrchr(dirs, '/')));
 		free(dirs);
 	}
 	return 0;

Totally untested, of course. But hey, it compiles, so it must be good.

-Peff

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

On Tue, Mar 1, 2011 at 4:08 PM, Jeff King <peff@peff.net> wrote:
>
> I think this is the cheap hack that you want:
>
> diff --git a/dir.c b/dir.c
> index 168dad6..fb6d306 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1236,6 +1236,29 @@ void setup_standard_excludes(struct dir_struct *dir)
>                add_excludes_from_file(dir, excludes_file);
>  }
>
> +static int rmdir_on_broken_cifs(const char *path)
> +{
> +       struct stat sb;
> +       if (stat(path, &sb) < 0) {
> +               /* Oh well, hopefully if we can't stat it
> +                * it is already gone or we don't have
> +                * permissions to screw it up anyway. */
> +               return rmdir(path);
> +       }
> +       if (rmdir(path) == 0) {
> +               /* it worked, nothing to restore */
> +               return 0;
> +       }
> +       /* maybe remove this conditional if you can trigger
> +        * the problem with other types of errors */
> +       if (errno != ENOTEMPTY)
> +               return -1;
> +       if (chmod(path, sb.st_mode) < 0)
> +               warning("we probably just screwed up the permissions of %s",
> +                       path);
> +       return -1;
> +}
> +
>  int remove_path(const char *name)
>  {
>        char *slash;
> @@ -1249,7 +1272,7 @@ int remove_path(const char *name)
>                slash = dirs + (slash - name);
>                do {
>                        *slash = '\0';
> -               } while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
> +               } while (rmdir_on_broken_cifs(dirs) == 0 && (slash = strrchr(dirs, '/')));
>                free(dirs);
>        }
>        return 0;
>
> Totally untested, of course. But hey, it compiles, so it must be good.
>
> -Peff
>

It seems to be working!  I've tried it with 'git rm' and when pulling
deletions.

I imagine that race condition can happen if files in the directory are
being modified while git does an rmdir?  If that's the case then I'm
not too worried.  There is only one other programmer that might be
working with me at the same time on an infrequently used directory.

Thank you everyone for the excellent help!

I modified the patch with some extra paranoia and replaced the other
rmdir instance in that file:

diff -crB git-1.7.3.4/dir.c git-1.7.3.4-new/dir.c
*** git-1.7.3.4/dir.c	Wed Mar  2 13:00:54 2011
--- git-1.7.3.4-new/dir.c	Wed Mar  2 14:25:10 2011
***************
*** 994,999 ****
--- 994,1022 ----
  	return ret;
  }

+ static int rmdir_on_broken_cifs(const char *path)
+ {
+        struct stat sb;
+        if (stat(path, &sb) < 0) {
+                /* Oh well, hopefully if we can't stat it
+                 * it is already gone or we don't have
+                 * permissions to screw it up anyway. */
+                return rmdir(path);
+        }
+        if (rmdir(path) == 0) {
+                /* it worked, nothing to restore */
+                return 0;
+        }
+        /* maybe remove this conditional if you can trigger
+         * the problem with other types of errors */
+        if (errno != ENOTEMPTY)
+                return -1;
+        if (chmod(path, sb.st_mode) < 0)
+                warning("we probably just screwed up the permissions of %s",
+                        path);
+        return -1;
+ }
+
  int remove_dir_recursively(struct strbuf *path, int flag)
  {
  	DIR *dir;
***************
*** 1037,1043 ****

  	strbuf_setlen(path, original_len);
  	if (!ret)
! 		ret = rmdir(path->buf);
  	return ret;
  }

--- 1060,1066 ----

  	strbuf_setlen(path, original_len);
  	if (!ret)
! 		ret = rmdir_on_broken_cifs(path->buf);
  	return ret;
  }

***************
*** 1066,1072 ****
  		slash = dirs + (slash - name);
  		do {
  			*slash = '\0';
! 		} while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
  		free(dirs);
  	}
  	return 0;
--- 1090,1096 ----
  		slash = dirs + (slash - name);
  		do {
  			*slash = '\0';
! 		} while (rmdir_on_broken_cifs(dirs) == 0 && (slash = strrchr(dirs, '/')));
  		free(dirs);
  	}
  	return 0;

Re: Git changes permissions on directories when deleting files.

[Jeff King]

On Wed, Mar 02, 2011 at 10:48:00PM -0500, Chad Joan wrote:

> It seems to be working!  I've tried it with 'git rm' and when pulling
> deletions.

Great.

> I imagine that race condition can happen if files in the directory are
> being modified while git does an rmdir?  If that's the case then I'm
> not too worried.  There is only one other programmer that might be
> working with me at the same time on an infrequently used directory.

The race condition I mentioned earlier was for a different workaround.
Basically there are two strategies, each with a difference race:

  1. Don't rmdir on non-empty directories. This means we have to opendir
     the directory and look for entries before rmdir(). If there is file
     activity in the directory while we are looking we may think it is
     empty when it's not and rmdir(), screwing up the permissions.

  2. Before any rmdir, check permissions. Do the rmdir, and then restore
     the permissions if rmdir fails. The race here is if somebody is
     modifying the permissions on a non-empty directory, we may
     overwrite their changes.

Obviously the patch does (2), so there is still that race.

> diff -crB git-1.7.3.4/dir.c git-1.7.3.4-new/dir.c

Context diff? Eww. There is this awesome tool called "git" that can help
you with managing versions of software. :)

-Peff

Re: Git changes permissions on directories when deleting files.

[Chad Joan]

On Thu, Mar 3, 2011 at 10:16 AM, Jeff King <peff@peff.net> wrote:
>
> ...
>
> > diff -crB git-1.7.3.4/dir.c git-1.7.3.4-new/dir.c
>
> Context diff? Eww. There is this awesome tool called "git" that can help
> you with managing versions of software. :)
>
> -Peff

Yes... though I'm just letting Gentoo compile it for me and I'm not
doing anything serious enough to justify downloading/installing GIT by
hand and next to the one that's already there.  So I just worked from
the tarball that Gentoo uses and added stuff.  Any other day and I'd
be all over that git usage ;)


 I did run into more permissions being messed up, so I grepped for
rmdir and replaced /all/ instances of it in git's C code.  I haven't
had anymore trouble so far.  Here's the newer patch:

diff -crB git-1.7.3.4/dir.c git-1.7.3.4-new/dir.c
*** git-1.7.3.4/dir.c	Wed Mar  2 13:00:54 2011
--- git-1.7.3.4-new/dir.c	Thu Mar 10 11:09:32 2011
***************
*** 994,999 ****
--- 994,1022 ----
  	return ret;
  }

+ int rmdir_on_broken_cifs(const char *path)
+ {
+        struct stat sb;
+        if (stat(path, &sb) < 0) {
+                /* Oh well, hopefully if we can't stat it
+                 * it is already gone or we don't have
+                 * permissions to screw it up anyway. */
+                return rmdir(path);
+        }
+        if (rmdir(path) == 0) {
+                /* it worked, nothing to restore */
+                return 0;
+        }
+        /* maybe remove this conditional if you can trigger
+         * the problem with other types of errors */
+        if (errno != ENOTEMPTY)
+                return -1;
+        if (chmod(path, sb.st_mode) < 0)
+                warning("we probably just screwed up the permissions of %s",
+                        path);
+        return -1;
+ }
+
  int remove_dir_recursively(struct strbuf *path, int flag)
  {
  	DIR *dir;
***************
*** 1037,1043 ****

  	strbuf_setlen(path, original_len);
  	if (!ret)
! 		ret = rmdir(path->buf);
  	return ret;
  }

--- 1060,1066 ----

  	strbuf_setlen(path, original_len);
  	if (!ret)
! 		ret = rmdir_on_broken_cifs(path->buf);
  	return ret;
  }

***************
*** 1066,1072 ****
  		slash = dirs + (slash - name);
  		do {
  			*slash = '\0';
! 		} while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
  		free(dirs);
  	}
  	return 0;
--- 1090,1096 ----
  		slash = dirs + (slash - name);
  		do {
  			*slash = '\0';
! 		} while (rmdir_on_broken_cifs(dirs) == 0 && (slash = strrchr(dirs, '/')));
  		free(dirs);
  	}
  	return 0;
Only in git-1.7.3.4: dir.c~
diff -crB git-1.7.3.4/dir.h git-1.7.3.4-new/dir.h
*** git-1.7.3.4/dir.h	Wed Dec 15 21:52:11 2010
--- git-1.7.3.4-new/dir.h	Thu Mar 10 11:09:13 2011
***************
*** 101,104 ****
--- 101,106 ----
  /* tries to remove the path with empty directories along it, ignores ENOENT */
  extern int remove_path(const char *path);

+ extern int rmdir_on_broken_cifs(const char *path);
+
  #endif
diff -crB git-1.7.3.4/entry.c git-1.7.3.4-new/entry.c
*** git-1.7.3.4/entry.c	Wed Dec 15 21:52:11 2010
--- git-1.7.3.4-new/entry.c	Thu Mar 10 11:12:25 2011
***************
*** 68,74 ****
  			die_errno("cannot unlink '%s'", pathbuf);
  	}
  	closedir(dir);
! 	if (rmdir(path))
  		die_errno("cannot rmdir '%s'", path);
  }

--- 68,74 ----
  			die_errno("cannot unlink '%s'", pathbuf);
  	}
  	closedir(dir);
! 	if (rmdir_on_broken_cifs(path))
  		die_errno("cannot rmdir '%s'", path);
  }

diff -crB git-1.7.3.4/pack-refs.c git-1.7.3.4-new/pack-refs.c
*** git-1.7.3.4/pack-refs.c	Wed Dec 15 21:52:11 2010
--- git-1.7.3.4-new/pack-refs.c	Thu Mar 10 12:34:53 2011
***************
*** 2,7 ****
--- 2,8 ----
  #include "refs.h"
  #include "tag.h"
  #include "pack-refs.h"
+ #include "dir.h"

  struct ref_to_prune {
  	struct ref_to_prune *next;
***************
*** 86,92 ****
  		if (q == p)
  			break;
  		*q = '\0';
! 		if (rmdir(git_path("%s", name)))
  			break;
  	}
  }
--- 87,93 ----
  		if (q == p)
  			break;
  		*q = '\0';
! 		if (rmdir_on_broken_cifs(git_path("%s", name)))
  			break;
  	}
  }
diff -crB git-1.7.3.4/symlinks.c git-1.7.3.4-new/symlinks.c
*** git-1.7.3.4/symlinks.c	Wed Dec 15 21:52:11 2010
--- git-1.7.3.4-new/symlinks.c	Thu Mar 10 11:24:08 2011
***************
*** 1,4 ****
--- 1,5 ----
  #include "cache.h"
+ #include "dir.h"

  /*
   * Returns the length (on a path component basis) of the longest
***************
*** 255,261 ****
  {
  	while (removal.len > new_len) {
  		removal.path[removal.len] = '\0';
! 		if (rmdir(removal.path))
  			break;
  		do {
  			removal.len--;
--- 256,262 ----
  {
  	while (removal.len > new_len) {
  		removal.path[removal.len] = '\0';
! 		if (rmdir_on_broken_cifs(removal.path))
  			break;
  		do {
  			removal.len--;
diff -crB git-1.7.3.4/wrapper.c git-1.7.3.4-new/wrapper.c
*** git-1.7.3.4/wrapper.c	Wed Dec 15 21:52:11 2010
--- git-1.7.3.4-new/wrapper.c	Thu Mar 10 11:12:36 2011
***************
*** 2,7 ****
--- 2,8 ----
   * Various trivial helper wrappers around standard functions
   */
  #include "cache.h"
+ #include "dir.h"

  static void try_to_free_builtin(size_t size)
  {
***************
*** 346,352 ****

  int rmdir_or_warn(const char *file)
  {
! 	return warn_if_unremovable("rmdir", file, rmdir(file));
  }

  int remove_or_warn(unsigned int mode, const char *file)
--- 347,353 ----

  int rmdir_or_warn(const char *file)
  {
! 	return warn_if_unremovable("rmdir", file, rmdir_on_broken_cifs(file));
  }

  int remove_or_warn(unsigned int mode, const char *file)