On Tue, Mar 1, 2011 at 4:08 PM, Jeff King [off-list ref] wrote:
quoted hunk
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;
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
On Thu, Mar 3, 2011 at 10:16 AM, Jeff King [off-list ref] wrote:
...
quoted
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)