Git changes permissions on directories when deleting files.

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

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

On Mon, Feb 28, 2011 at 8:42 PM, Chad Joan <chadjoan@gmail.com> wrote:

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

On Mon, Feb 28, 2011 at 9:19 PM, Computer Druid <computerdruid@gmail.com> wrote:

Nope.

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

- Chad

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.

On Tue, Mar 1, 2011 at 10:51 AM, Chad Joan <chadjoan@gmail.com> wrote:

What happens if you "rmdir foo/bar" here? (while there are files still in it)

-Dan Johnson

On Tue, Mar 1, 2011 at 12:11 PM, Computer Druid <computerdruid@gmail.com> 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.

- Chad

On Tue, Mar 01, 2011 at 02:35:41PM -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.

-Peff

On Tue, Mar 1, 2011 at 2:44 PM, Jeff King <peff@peff.net> wrote:

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?

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

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

On Tue, Mar 1, 2011 at 3:08 PM, Jeff King <peff@peff.net> wrote:

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

On Tue, Mar 1, 2011 at 3:30 PM, Chad Joan <chadjoan@gmail.com> wrote:

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.

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

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

Chad Joan <chadjoan@gmail.com> writes:

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

On Tue, Mar 1, 2011 at 3:43 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

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

- Chad

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

I think this is the cheap hack that you want:

On Tue, Mar 1, 2011 at 4:08 PM, Jeff King <peff@peff.net> wrote:

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;

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

Great.

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.

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

-Peff

On Thu, Mar 3, 2011 at 10:16 AM, Jeff King <peff@peff.net> wrote:

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)