{"thread":{"id":"15654","subject":"[BUG] git merge does not prune empty directories","startedAt":"2008-09-24T16:32:22Z","lastAt":"2008-09-26T22:59:14Z","messageCount":10,"participants":["Anders Melchiorsen","Alex Riesen","Johannes Sixt","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"91487","messageId":"87k5d1v71l.fsf@cup.kalibalik.dk","threadId":"15654","inReplyTo":null,"subject":"[BUG] git merge does not prune empty directories","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-09-24T16:32:22Z","receivedAt":"2008-09-24T16:32:22Z","isPatch":false,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"I got an empty directory left over today, and have reduced the problem\nto this sequence. If I leave out the second add (so the merge is a\nfast forward), the directory is removed as I would expect.\n\nThis is with Git 1.6.0.2.\n\n\n    Anders\n\n\n\nand@dylle:~$ mkdir repo ; cd repo\nand@dylle:~/repo$ git init\nInitialized empty Git repository in /home/and/repo/.git/\nand@dylle:~/repo$ mkdir a ; date >a/b ; git add a/b ; git commit -m'Add 1'\nCreated initial commit 72194c7: Add 1\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 a/b\nand@dylle:~/repo$ git checkout -b other\nSwitched to a new branch \"other\"\nand@dylle:~/repo$ git rm a/b ; git commit -m'Remove 1'\nrm 'a/b'\nCreated commit 9c0282c: Remove 1\n 1 files changed, 0 insertions(+), 1 deletions(-)\n delete mode 100644 a/b\nand@dylle:~/repo$ git checkout master\nSwitched to branch \"master\"\nand@dylle:~/repo$ date >c ; git add c ; git commit -m'Add 2'\nCreated commit 39d60d4: Add 2\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 c\nand@dylle:~/repo$ git merge other\nRemoved a/b\nMerge made by recursive.\n a/b |    1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n delete mode 100644 a/b\nand@dylle:~/repo$ rmdir a\nand@dylle:~/repo$ rmdir a\nrmdir: failed to remove `a': No such file or directory\n"},{"id":"91621","messageId":"20080925201245.GB3959@blimp.localhost","threadId":"15654","inReplyTo":"87k5d1v71l.fsf@cup.kalibalik.dk","subject":"[PATCH] Remove empty directories in recursive merge","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-09-25T20:12:45Z","receivedAt":"2008-09-25T20:12:45Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The code was actually supposed to do that, but was accidentally broken.\nNoticed by Anders Melchiorsen.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\nAnders Melchiorsen, Wed, Sep 24, 2008 18:32:22 +0200:\n> I got an empty directory left over today, and have reduced the problem\n> to this sequence. If I leave out the second add (so the merge is a\n> fast forward), the directory is removed as I would expect.\n\nAch, an old bug. Thanks for reminding!\n\n builtin-merge-recursive.c  |    4 +---\n t/t3030-merge-recursive.sh |   11 +++++++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c\nindex dfb363e..a29b47f 100644\n--- a/builtin-merge-recursive.c\n+++ b/builtin-merge-recursive.c\n@@ -444,10 +444,8 @@ static int remove_file(int clean, const char *path, int no_wd)\n \t\t\treturn -1;\n \t}\n \tif (update_working_directory) {\n-\t\tunlink(path);\n-\t\tif (errno != ENOENT || errno != EISDIR)\n+\t\tif (remove_path(path) && errno != ENOENT)\n \t\t\treturn -1;\n-\t\tremove_path(path);\n \t}\n \treturn 0;\n }\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex de0cdb1..0de613d 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -535,4 +535,15 @@ test_expect_success 'reset and bind merge' '\n \n '\n \n+test_expect_success 'merge removes empty directories' '\n+\n+\tgit reset --hard master &&\n+\tgit checkout -b rm &&\n+\tgit rm d/e &&\n+\tgit commit -mremoved-d/e &&\n+\tgit checkout master &&\n+\tgit merge -s recursive rm &&\n+\ttest_must_fail test -d d\n+'\n+\n test_done\n-- \n1.6.0.2.328.g14651\n"},{"id":"91622","messageId":"20080925201734.GA18315@blimp.localhost","threadId":"15654","inReplyTo":"20080925201245.GB3959@blimp.localhost","subject":"[PATCH] Cleanup remove_path","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-09-25T20:17:34Z","receivedAt":"2008-09-25T20:17:34Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nWhile at it, could we cleanup the remove_file routines a little?\n\n builtin-merge-recursive.c |    8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c\nindex a29b47f..dc9652d 100644\n--- a/builtin-merge-recursive.c\n+++ b/builtin-merge-recursive.c\n@@ -418,12 +418,10 @@ static int update_stages(const char *path, struct diff_filespec *o,\n \n static int remove_path(const char *name)\n {\n-\tint ret;\n \tchar *slash, *dirs;\n \n-\tret = unlink(name);\n-\tif (ret)\n-\t\treturn ret;\n+\tif (unlink(name))\n+\t\treturn -1;\n \tdirs = xstrdup(name);\n \twhile ((slash = strrchr(name, '/'))) {\n \t\t*slash = '\\0';\n@@ -431,7 +429,7 @@ static int remove_path(const char *name)\n \t\t\tbreak;\n \t}\n \tfree(dirs);\n-\treturn ret;\n+\treturn 0;\n }\n \n static int remove_file(int clean, const char *path, int no_wd)\n-- \n1.6.0.2.328.g14651\n"},{"id":"91623","messageId":"20080925202237.GB18315@blimp.localhost","threadId":"15654","inReplyTo":"20080925201734.GA18315@blimp.localhost","subject":"[PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-09-25T20:22:37Z","receivedAt":"2008-09-25T20:22:37Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Actually, just replace it with the one from builtin-merge-recursive.c,\nexcept for ignoring ENOENT error.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nIt is the same as in merge-recursive, but they're so small so unless\nwe get a special file with such random routines there is no much point\nexporting it. Actually, we do seem to have such a file: dir.c. It is\nalready plagued by file_exists kind of things, why not remove_path...\n\n builtin-rm.c |   24 ++++++++++--------------\n 1 files changed, 10 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex fdac34f..910a34d 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -31,22 +31,18 @@ static void add_list(const char *name)\n \n static int remove_file(const char *name)\n {\n-\tint ret;\n-\tchar *slash;\n-\n-\tret = unlink(name);\n-\tif (ret && errno == ENOENT)\n-\t\t/* The user has removed it from the filesystem by hand */\n-\t\tret = errno = 0;\n-\n-\tif (!ret && (slash = strrchr(name, '/'))) {\n-\t\tchar *n = xstrdup(name);\n-\t\tdo {\n-\t\t\tn[slash - name] = 0;\n-\t\t\tname = n;\n-\t\t} while (!rmdir(name) && (slash = strrchr(name, '/')));\n+\tchar *slash, *dirs;\n+\n+\tif (unlink(name) && errno != ENOENT)\n+\t\treturn -1;\n+\tdirs = xstrdup(name);\n+\twhile ((slash = strrchr(name, '/'))) {\n+\t\t*slash = '\\0';\n+\t\tif (rmdir(name) != 0)\n+\t\t\tbreak;\n \t}\n-\treturn ret;\n+\tfree(dirs);\n+\treturn 0;\n }\n \n static int check_local_mod(unsigned char *head, int index_only)\n-- \n1.6.0.2.328.g14651\n"},{"id":"91624","messageId":"20080925203329.GC3959@blimp.localhost","threadId":"15654","inReplyTo":"20080925201245.GB3959@blimp.localhost","subject":"Re: [PATCH] Remove empty directories in recursive merge","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-09-25T20:33:29Z","receivedAt":"2008-09-25T20:33:29Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Alex Riesen, Thu, Sep 25, 2008 22:12:45 +0200:\n> The code was actually supposed to do that, but was accidentally broken.\n> Noticed by Anders Melchiorsen.\n> \n> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n\nShawn, the change is intentionally based on Junios master,\nso people can just apply it. But if you wish, I can rebase it on top\nof Miklos' patches.\n"},{"id":"91658","messageId":"48DC7A13.7000907@viscovery.net","threadId":"15654","inReplyTo":"20080925201245.GB3959@blimp.localhost","subject":"Re: [PATCH] Remove empty directories in recursive merge","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-09-26T05:58:43Z","receivedAt":"2008-09-26T05:58:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> +test_expect_success 'merge removes empty directories' '\n> +\n> +\tgit reset --hard master &&\n> +\tgit checkout -b rm &&\n> +\tgit rm d/e &&\n> +\tgit commit -mremoved-d/e &&\n> +\tgit checkout master &&\n> +\tgit merge -s recursive rm &&\n> +\ttest_must_fail test -d d\n\nFWIW, 'test_must_fail' is intended for git commands only. Here,\n\n\t! test -d d\n\nwould be just fine.\n\n> +'\n> +\n>  test_done\n\n-- Hannes\n"},{"id":"91694","messageId":"20080926150636.GA17584@spearce.org","threadId":"15654","inReplyTo":"20080925203329.GC3959@blimp.localhost","subject":"Re: [PATCH] Remove empty directories in recursive merge","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-26T15:06:36Z","receivedAt":"2008-09-26T15:06:36Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> wrote:\n> Alex Riesen, Thu, Sep 25, 2008 22:12:45 +0200:\n> > The code was actually supposed to do that, but was accidentally broken.\n> > Noticed by Anders Melchiorsen.\n> > \n> > Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n> \n> Shawn, the change is intentionally based on Junios master,\n\nActually I think this is 'maint' worthy.  It applies clean there.\nI'm trying to slate it for maint.\n\n> so people can just apply it. But if you wish, I can rebase it on top\n> of Miklos' patches.\n\nIts small.  I can manage to merge it over myself.  Thanks.\n\n-- \nShawn.\n"},{"id":"91698","messageId":"20080926152823.GA17470@spearce.org","threadId":"15654","inReplyTo":"20080925202237.GB18315@blimp.localhost","subject":"Re: [PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-26T15:28:23Z","receivedAt":"2008-09-26T15:28:23Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> wrote:\n> It is the same as in merge-recursive, but they're so small so unless\n> we get a special file with such random routines there is no much point\n> exporting it. Actually, we do seem to have such a file: dir.c. It is\n> already plagued by file_exists kind of things, why not remove_path...\n\nYea.  I'm thinking remove_path should migrate to dir.c.  Hell,\nwe already have rm -rf as remove_dir_recursively() in dir.c.\nremove_path is its long-lost soul mate.  I'm not applying this\nbuiltin-rm fix, and am hoping you'll rewrite it around a move\nof remove_path to dir.c...  ;-)\n \n> diff --git a/builtin-rm.c b/builtin-rm.c\n> index fdac34f..910a34d 100644\n> --- a/builtin-rm.c\n> +++ b/builtin-rm.c\n> @@ -31,22 +31,18 @@ static void add_list(const char *name)\n>  \n>  static int remove_file(const char *name)\n>  {\n\n\n-- \nShawn.\n"},{"id":"91720","messageId":"20080926225645.GA13412@blimp.localhost","threadId":"15654","inReplyTo":"20080926152823.GA17470@spearce.org","subject":"[PATCH] Add remove_path: a function to remove as much as possible of a path","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-09-26T22:56:46Z","receivedAt":"2008-09-26T22:56:46Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The function has two potential users which both managed to get wrong\ntheir implementations (the one in builtin-rm.c one has a memleak, and\nbuiltin-merge-recursive.c scribles over its const argument).\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\nShawn O. Pearce, Fri, Sep 26, 2008 17:28:23 +0200:\n> Alex Riesen <raa.lkml@gmail.com> wrote:\n> > It is the same as in merge-recursive, but they're so small so unless\n> > we get a special file with such random routines there is no much point\n> > exporting it. Actually, we do seem to have such a file: dir.c. It is\n> > already plagued by file_exists kind of things, why not remove_path...\n> \n> Yea.  I'm thinking remove_path should migrate to dir.c.  Hell,\n> we already have rm -rf as remove_dir_recursively() in dir.c.\n> remove_path is its long-lost soul mate.  I'm not applying this\n> builtin-rm fix, and am hoping you'll rewrite it around a move\n> of remove_path to dir.c...  ;-)\n>  \n\nOkay :) The next one is on top of the previous fix in merge-recursive\n(removes ENOENT conditional)\n\n dir.c |   20 ++++++++++++++++++++\n dir.h |    3 +++\n 2 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex acf1001..1c3c501 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -831,3 +831,23 @@ void setup_standard_excludes(struct dir_struct *dir)\n \tif (excludes_file && !access(excludes_file, R_OK))\n \t\tadd_excludes_from_file(dir, excludes_file);\n }\n+\n+int remove_path(const char *name)\n+{\n+\tchar *slash;\n+\n+\tif (unlink(name) && errno != ENOENT)\n+\t\treturn -1;\n+\n+\tslash = strrchr(name, '/');\n+\tif (slash) {\n+\t\tchar *dirs = xstrdup(name);\n+\t\tslash = dirs + (slash - name);\n+\t\tdo {\n+\t\t\t*slash = '\\0';\n+\t\t} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));\n+\t\tfree(dirs);\n+\t}\n+\treturn 0;\n+}\n+\ndiff --git a/dir.h b/dir.h\nindex 2df15de..278ee42 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -81,4 +81,7 @@ extern int is_inside_dir(const char *dir);\n extern void setup_standard_excludes(struct dir_struct *dir);\n extern int remove_dir_recursively(struct strbuf *path, int only_empty);\n \n+/* tries to remove the path with empty directories along it, ignores ENOENT */\n+extern int remove_path(const char *path);\n+\n #endif\n-- \n1.6.0.2.328.g14651\n"},{"id":"91721","messageId":"20080926225914.GA13879@blimp.localhost","threadId":"15654","inReplyTo":"20080926225645.GA13412@blimp.localhost","subject":"[PATCH] Use remove_path from dir.c instead of own implementation","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-09-26T22:59:14Z","receivedAt":"2008-09-26T22:59:14Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Besides, it fixes a memleak (builtin-rm.c) and accidental change of\nthe input const argument (builtin-merge-recursive.c).\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nAlex Riesen, Sat, Sep 27, 2008 00:56:45 +0200:\n> > remove_path is its long-lost soul mate.  I'm not applying this\n> > builtin-rm fix, and am hoping you'll rewrite it around a move\n> > of remove_path to dir.c...  ;-)\n> >  \n> \n> Okay :) The next one is on top of the previous fix in merge-recursive\n> (removes ENOENT conditional)\n> \n\nThis one.\n\n builtin-apply.c           |   11 ++---------\n builtin-merge-recursive.c |   21 ++-------------------\n builtin-rm.c              |   22 +---------------------\n 3 files changed, 5 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 20bef1f..70c9f93 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -13,6 +13,7 @@\n #include \"delta.h\"\n #include \"builtin.h\"\n #include \"string-list.h\"\n+#include \"dir.h\"\n \n /*\n  *  --check turns on checking that the working tree matches the\n@@ -2735,15 +2736,7 @@ static void remove_file(struct patch *patch, int rmdir_empty)\n \t\t\t\twarning(\"unable to remove submodule %s\",\n \t\t\t\t\tpatch->old_name);\n \t\t} else if (!unlink(patch->old_name) && rmdir_empty) {\n-\t\t\tchar *name = xstrdup(patch->old_name);\n-\t\t\tchar *end = strrchr(name, '/');\n-\t\t\twhile (end) {\n-\t\t\t\t*end = 0;\n-\t\t\t\tif (rmdir(name))\n-\t\t\t\t\tbreak;\n-\t\t\t\tend = strrchr(name, '/');\n-\t\t\t}\n-\t\t\tfree(name);\n+\t\t\tremove_path(patch->old_name);\n \t\t}\n \t}\n }\ndiff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c\nindex a29b47f..36aa798 100644\n--- a/builtin-merge-recursive.c\n+++ b/builtin-merge-recursive.c\n@@ -18,6 +18,7 @@\n #include \"ll-merge.h\"\n #include \"interpolate.h\"\n #include \"attr.h\"\n+#include \"dir.h\"\n #include \"merge-recursive.h\"\n \n static int subtree_merge;\n@@ -416,24 +417,6 @@ static int update_stages(const char *path, struct diff_filespec *o,\n \treturn 0;\n }\n \n-static int remove_path(const char *name)\n-{\n-\tint ret;\n-\tchar *slash, *dirs;\n-\n-\tret = unlink(name);\n-\tif (ret)\n-\t\treturn ret;\n-\tdirs = xstrdup(name);\n-\twhile ((slash = strrchr(name, '/'))) {\n-\t\t*slash = '\\0';\n-\t\tif (rmdir(name) != 0)\n-\t\t\tbreak;\n-\t}\n-\tfree(dirs);\n-\treturn ret;\n-}\n-\n static int remove_file(int clean, const char *path, int no_wd)\n {\n \tint update_cache = index_only || clean;\n@@ -444,7 +427,7 @@ static int remove_file(int clean, const char *path, int no_wd)\n \t\t\treturn -1;\n \t}\n \tif (update_working_directory) {\n-\t\tif (remove_path(path) && errno != ENOENT)\n+\t\tif (remove_path(path))\n \t\t\treturn -1;\n \t}\n \treturn 0;\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex fdac34f..50ae6d5 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -29,26 +29,6 @@ static void add_list(const char *name)\n \tlist.name[list.nr++] = name;\n }\n \n-static int remove_file(const char *name)\n-{\n-\tint ret;\n-\tchar *slash;\n-\n-\tret = unlink(name);\n-\tif (ret && errno == ENOENT)\n-\t\t/* The user has removed it from the filesystem by hand */\n-\t\tret = errno = 0;\n-\n-\tif (!ret && (slash = strrchr(name, '/'))) {\n-\t\tchar *n = xstrdup(name);\n-\t\tdo {\n-\t\t\tn[slash - name] = 0;\n-\t\t\tname = n;\n-\t\t} while (!rmdir(name) && (slash = strrchr(name, '/')));\n-\t}\n-\treturn ret;\n-}\n-\n static int check_local_mod(unsigned char *head, int index_only)\n {\n \t/* items in list are already sorted in the cache order,\n@@ -239,7 +219,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\tint removed = 0;\n \t\tfor (i = 0; i < list.nr; i++) {\n \t\t\tconst char *path = list.name[i];\n-\t\t\tif (!remove_file(path)) {\n+\t\t\tif (!remove_path(path)) {\n \t\t\t\tremoved = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n1.6.0.2.328.g14651\n"}]}