Skip to contentSkip to conversation

git merge does not prune empty directories

BUG In review Anders Melchiorsen 4 participants 10 messages Jun 15, 2016

lore
Share & tools
lore

Anders Melchiorsen

author
#0 lore raw

I got an empty directory left over today, and have reduced the problem to this sequence. If I leave out the second add (so the merge is a fast forward), the directory is removed as I would expect.

This is with Git 1.6.0.2.

Anders

and@dylle:~$ mkdir repo ; cd repo
and@dylle:~/repo$ git init
Initialized empty Git repository in /home/and/repo/.git/
and@dylle:~/repo$ mkdir a ; date >a/b ; git add a/b ; git commit -m'Add 1'
Created initial commit 72194c7: Add 1
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 a/b
and@dylle:~/repo$ git checkout -b other
Switched to a new branch "other"
and@dylle:~/repo$ git rm a/b ; git commit -m'Remove 1'
rm 'a/b'
Created commit 9c0282c: Remove 1
 1 files changed, 0 insertions(+), 1 deletions(-)
 delete mode 100644 a/b
and@dylle:~/repo$ git checkout master
Switched to branch "master"
and@dylle:~/repo$ date >c ; git add c ; git commit -m'Add 2'
Created commit 39d60d4: Add 2
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 c
and@dylle:~/repo$ git merge other
Removed a/b
Merge made by recursive.
 a/b |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
 delete mode 100644 a/b
and@dylle:~/repo$ rmdir a
and@dylle:~/repo$ rmdir a
rmdir: failed to remove `a': No such file or directory
#1 lore raw

PATCH 1/1 Remove empty directories in recursive merge

The code was actually supposed to do that, but was accidentally broken. Noticed by Anders Melchiorsen.

Signed-off-by Alex Riesen <[email protected]>

---
Anders Melchiorsen, Wed, Sep 24, 2008 18:32:22 +0200:

Quoted 3 lines

I got an empty directory left over today, and have reduced the problem to this sequence. If I leave out the second add (so the merge is a fast forward), the directory is removed as I would expect.

Ach, an old bug. Thanks for reminding!

 builtin-merge-recursive.c  |    4 +---
 t/t3030-merge-recursive.sh |   11 +++++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)
builtin-merge-recursive.c +1 -3
index dfb363e..a29b47f 100644
@@ -444,10 +444,8 @@ static int remove_file(int clean, const char *path, int no_wd)
 			return -1;
 	}
 	if (update_working_directory) {
-		unlink(path);
-		if (errno != ENOENT || errno != EISDIR)
+		if (remove_path(path) && errno != ENOENT)
 			return -1;
-		remove_path(path);
 	}
 	return 0;
 }
t/t3030-merge-recursive.sh +11 -0
index de0cdb1..0de613d 100755
@@ -535,4 +535,15 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_success 'merge removes empty directories' '
+
+	git reset --hard master &&
+	git checkout -b rm &&
+	git rm d/e &&
+	git commit -mremoved-d/e &&
+	git checkout master &&
+	git merge -s recursive rm &&
+	test_must_fail test -d d
+'
+
 test_done
Signature · 1 line
1.6.0.2.328.g14651
#2 lore raw

PATCH 1/1 Cleanup remove_path

Signed-off-by Alex Riesen <[email protected]>

---

While at it, could we cleanup the remove_file routines a little?

 builtin-merge-recursive.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)
builtin-merge-recursive.c +3 -5
index a29b47f..dc9652d 100644
@@ -418,12 +418,10 @@ static int update_stages(const char *path, struct diff_filespec *o,
 
 static int remove_path(const char *name)
 {
-	int ret;
 	char *slash, *dirs;
 
-	ret = unlink(name);
-	if (ret)
-		return ret;
+	if (unlink(name))
+		return -1;
 	dirs = xstrdup(name);
 	while ((slash = strrchr(name, '/'))) {
 		*slash = '\0';

@@ -431,7 +429,7 @@ static int remove_path(const char *name)
 			break;
 	}
 	free(dirs);
-	return ret;
+	return 0;
 }
 
 static int remove_file(int clean, const char *path, int no_wd)
Signature · 1 line
1.6.0.2.328.g14651
#3 lore raw

PATCH 1/1 Fix memleak and the implementation of remove_file in builtin-rm.c

Actually, just replace it with the one from builtin-merge-recursive.c, except for ignoring ENOENT error.

Signed-off-by Alex Riesen <[email protected]>

---

It is the same as in merge-recursive, but they're so small so unless we get a special file with such random routines there is no much point exporting it. Actually, we do seem to have such a file: dir.c. It is already plagued by file_exists kind of things, why not remove_path...

 builtin-rm.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)
builtin-rm.c +11 -15
index fdac34f..910a34d 100644
@@ -31,22 +31,18 @@ static void add_list(const char *name)
 
 static int remove_file(const char *name)
 {
-	int ret;
-	char *slash;
-
-	ret = unlink(name);
-	if (ret && errno == ENOENT)
-		/* The user has removed it from the filesystem by hand */
-		ret = errno = 0;
-
-	if (!ret && (slash = strrchr(name, '/'))) {
-		char *n = xstrdup(name);
-		do {
-			n[slash - name] = 0;
-			name = n;
-		} while (!rmdir(name) && (slash = strrchr(name, '/')));
+	char *slash, *dirs;
+
+	if (unlink(name) && errno != ENOENT)
+		return -1;
+	dirs = xstrdup(name);
+	while ((slash = strrchr(name, '/'))) {
+		*slash = '\0';
+		if (rmdir(name) != 0)
+			break;
 	}
-	return ret;
+	free(dirs);
+	return 0;
 }
 
 static int check_local_mod(unsigned char *head, int index_only)
Signature · 1 line
1.6.0.2.328.g14651
#4 lore raw

Re: [PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c

Quoted 4 lines · Alex Riesen

It is the same as in merge-recursive, but they're so small so unless we get a special file with such random routines there is no much point exporting it. Actually, we do seem to have such a file: dir.c. It is already plagued by file_exists kind of things, why not remove_path...

Yea. I'm thinking remove_path should migrate to dir.c. Hell, we already have rm -rf as remove_dir_recursively() in dir.c. remove_path is its long-lost soul mate. I'm not applying this builtin-rm fix, and am hoping you'll rewrite it around a move of remove_path to dir.c... ;-)

Quoted 8 lines
builtin-rm.c +0 -0
index fdac34f..910a34d 100644
@@ -31,22 +31,18 @@ static void add_list(const char *name)
 
 static int remove_file(const char *name)
 {
Signature · 1 line
Shawn.
#5 lore raw

PATCH 1/1 Add remove_path: a function to remove as much as possible of a path

The function has two potential users which both managed to get wrong their implementations (the one in builtin-rm.c one has a memleak, and builtin-merge-recursive.c scribles over its const argument).

Signed-off-by Alex Riesen <[email protected]>

---
Shawn O. Pearce, Fri, Sep 26, 2008 17:28:23 +0200:

Quoted 6 lines
Quoted 4 lines · Alex Riesen

It is the same as in merge-recursive, but they're so small so unless we get a special file with such random routines there is no much point exporting it. Actually, we do seem to have such a file: dir.c. It is already plagued by file_exists kind of things, why not remove_path...

Quoted 6 lines

Yea. I'm thinking remove_path should migrate to dir.c. Hell, we already have rm -rf as remove_dir_recursively() in dir.c. remove_path is its long-lost soul mate. I'm not applying this builtin-rm fix, and am hoping you'll rewrite it around a move of remove_path to dir.c... ;-)

Okay :) The next one is on top of the previous fix in merge-recursive (removes ENOENT conditional)

 dir.c |   20 ++++++++++++++++++++
 dir.h |    3 +++
 2 files changed, 23 insertions(+), 0 deletions(-)
dir.c +20 -0
index acf1001..1c3c501 100644
@@ -831,3 +831,23 @@ void setup_standard_excludes(struct dir_struct *dir)
 	if (excludes_file && !access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
+
+int remove_path(const char *name)
+{
+	char *slash;
+
+	if (unlink(name) && errno != ENOENT)
+		return -1;
+
+	slash = strrchr(name, '/');
+	if (slash) {
+		char *dirs = xstrdup(name);
+		slash = dirs + (slash - name);
+		do {
+			*slash = '\0';
+		} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));
+		free(dirs);
+	}
+	return 0;
+}
+
dir.h +3 -0
index 2df15de..278ee42 100644
@@ -81,4 +81,7 @@ extern int is_inside_dir(const char *dir);
 extern void setup_standard_excludes(struct dir_struct *dir);
 extern int remove_dir_recursively(struct strbuf *path, int only_empty);
 
+/* tries to remove the path with empty directories along it, ignores ENOENT */
+extern int remove_path(const char *path);
+
 #endif
Signature · 1 line
1.6.0.2.328.g14651
#6 lore raw

PATCH 1/1 Use remove_path from dir.c instead of own implementation

Besides, it fixes a memleak (builtin-rm.c) and accidental change of the input const argument (builtin-merge-recursive.c).

Signed-off-by Alex Riesen <[email protected]>

---

Alex Riesen, Sat, Sep 27, 2008 00:56:45 +0200:

Quoted 8 lines
Quoted 4 lines

remove_path is its long-lost soul mate. I'm not applying this builtin-rm fix, and am hoping you'll rewrite it around a move of remove_path to dir.c... ;-)

Okay :) The next one is on top of the previous fix in merge-recursive (removes ENOENT conditional)

This one.

 builtin-apply.c           |   11 ++---------
 builtin-merge-recursive.c |   21 ++-------------------
 builtin-rm.c              |   22 +---------------------
 3 files changed, 5 insertions(+), 49 deletions(-)
builtin-apply.c +2 -9
index 20bef1f..70c9f93 100644
@@ -13,6 +13,7 @@
 #include "delta.h"
 #include "builtin.h"
 #include "string-list.h"
+#include "dir.h"
 
 /*
  *  --check turns on checking that the working tree matches the

@@ -2735,15 +2736,7 @@ static void remove_file(struct patch *patch, int rmdir_empty)
 				warning("unable to remove submodule %s",
 					patch->old_name);
 		} else if (!unlink(patch->old_name) && rmdir_empty) {
-			char *name = xstrdup(patch->old_name);
-			char *end = strrchr(name, '/');
-			while (end) {
-				*end = 0;
-				if (rmdir(name))
-					break;
-				end = strrchr(name, '/');
-			}
-			free(name);
+			remove_path(patch->old_name);
 		}
 	}
 }
builtin-merge-recursive.c +2 -19
index a29b47f..36aa798 100644
@@ -18,6 +18,7 @@
 #include "ll-merge.h"
 #include "interpolate.h"
 #include "attr.h"
+#include "dir.h"
 #include "merge-recursive.h"
 
 static int subtree_merge;

@@ -416,24 +417,6 @@ static int update_stages(const char *path, struct diff_filespec *o,
 	return 0;
 }
 
-static int remove_path(const char *name)
-{
-	int ret;
-	char *slash, *dirs;
-
-	ret = unlink(name);
-	if (ret)
-		return ret;
-	dirs = xstrdup(name);
-	while ((slash = strrchr(name, '/'))) {
-		*slash = '\0';
-		if (rmdir(name) != 0)
-			break;
-	}
-	free(dirs);
-	return ret;
-}
-
 static int remove_file(int clean, const char *path, int no_wd)
 {
 	int update_cache = index_only || clean;

@@ -444,7 +427,7 @@ static int remove_file(int clean, const char *path, int no_wd)
 			return -1;
 	}
 	if (update_working_directory) {
-		if (remove_path(path) && errno != ENOENT)
+		if (remove_path(path))
 			return -1;
 	}
 	return 0;
builtin-rm.c +1 -21
index fdac34f..50ae6d5 100644
@@ -29,26 +29,6 @@ static void add_list(const char *name)
 	list.name[list.nr++] = name;
 }
 
-static int remove_file(const char *name)
-{
-	int ret;
-	char *slash;
-
-	ret = unlink(name);
-	if (ret && errno == ENOENT)
-		/* The user has removed it from the filesystem by hand */
-		ret = errno = 0;
-
-	if (!ret && (slash = strrchr(name, '/'))) {
-		char *n = xstrdup(name);
-		do {
-			n[slash - name] = 0;
-			name = n;
-		} while (!rmdir(name) && (slash = strrchr(name, '/')));
-	}
-	return ret;
-}
-
 static int check_local_mod(unsigned char *head, int index_only)
 {
 	/* items in list are already sorted in the cache order,

@@ -239,7 +219,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		int removed = 0;
 		for (i = 0; i < list.nr; i++) {
 			const char *path = list.name[i];
-			if (!remove_file(path)) {
+			if (!remove_path(path)) {
 				removed = 1;
 				continue;
 			}
Signature · 1 line
1.6.0.2.328.g14651
#7 lore raw

Re: [PATCH] Remove empty directories in recursive merge

In reply to Alex Riesen

Alex Riesen, Thu, Sep 25, 2008 22:12:45 +0200:

Quoted 4 lines

The code was actually supposed to do that, but was accidentally broken. Noticed by Anders Melchiorsen.

Signed-off-by Alex Riesen <[email protected]>

Shawn, the change is intentionally based on Junios master, so people can just apply it. But if you wish, I can rebase it on top of Miklos' patches.

#8 lore raw

Re: [PATCH] Remove empty directories in recursive merge

Quoted 7 lines · Alex Riesen

Alex Riesen, Thu, Sep 25, 2008 22:12:45 +0200:

Quoted 4 lines

The code was actually supposed to do that, but was accidentally broken. Noticed by Anders Melchiorsen.

Signed-off-by Alex Riesen <[email protected]>

Shawn, the change is intentionally based on Junios master,

Actually I think this is 'maint' worthy. It applies clean there. I'm trying to slate it for maint.

Quoted 2 lines

so people can just apply it. But if you wish, I can rebase it on top of Miklos' patches.

Its small. I can manage to merge it over myself. Thanks.

Signature · 1 line
Shawn.
#9 lore raw

Re: [PATCH] Remove empty directories in recursive merge

In reply to Alex Riesen

t/t3030-merge-recursive.sh test_expect_success 'reset and bind merge' ' · line 546

Quoted 9 lines · Alex Riesen
t/t3030-merge-recursive.sh +9 -0
+test_expect_success 'merge removes empty directories' '
+
+	git reset --hard master &&
+	git checkout -b rm &&
+	git rm d/e &&
+	git commit -mremoved-d/e &&
+	git checkout master &&
+	git merge -s recursive rm &&
+	test_must_fail test -d d

FWIW, 'test_must_fail' is intended for git commands only. Here,

! test -d d

would be just fine.

Quoted 3 lines

+'
+
test_done

-- Hannes