* [BUG] git merge does not prune empty directories
@ 2008-09-24 16:32 Anders Melchiorsen
2008-09-25 20:12 ` [PATCH] Remove empty directories in recursive merge Alex Riesen
0 siblings, 1 reply; 10+ messages in thread
From: Anders Melchiorsen @ 2008-09-24 16:32 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH] Remove empty directories in recursive merge 2008-09-24 16:32 [BUG] git merge does not prune empty directories Anders Melchiorsen @ 2008-09-25 20:12 ` Alex Riesen 2008-09-25 20:17 ` [PATCH] Cleanup remove_path Alex Riesen ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Alex Riesen @ 2008-09-25 20:12 UTC (permalink / raw) To: git; +Cc: Anders Melchiorsen, Shawn O. Pearce, Junio C Hamano The code was actually supposed to do that, but was accidentally broken. Noticed by Anders Melchiorsen. Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- Anders Melchiorsen, Wed, Sep 24, 2008 18:32:22 +0200: > 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(-) diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index dfb363e..a29b47f 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -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; } diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh index de0cdb1..0de613d 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -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 -- 1.6.0.2.328.g14651 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] Cleanup remove_path 2008-09-25 20:12 ` [PATCH] Remove empty directories in recursive merge Alex Riesen @ 2008-09-25 20:17 ` Alex Riesen 2008-09-25 20:22 ` [PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c Alex Riesen 2008-09-25 20:33 ` [PATCH] Remove empty directories in recursive merge Alex Riesen 2008-09-26 5:58 ` Johannes Sixt 2 siblings, 1 reply; 10+ messages in thread From: Alex Riesen @ 2008-09-25 20:17 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce, Junio C Hamano Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- While at it, could we cleanup the remove_file routines a little? builtin-merge-recursive.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-) diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index a29b47f..dc9652d 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -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) -- 1.6.0.2.328.g14651 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c 2008-09-25 20:17 ` [PATCH] Cleanup remove_path Alex Riesen @ 2008-09-25 20:22 ` Alex Riesen 2008-09-26 15:28 ` Shawn O. Pearce 0 siblings, 1 reply; 10+ messages in thread From: Alex Riesen @ 2008-09-25 20:22 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce, Junio C Hamano Actually, just replace it with the one from builtin-merge-recursive.c, except for ignoring ENOENT error. Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- 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(-) diff --git a/builtin-rm.c b/builtin-rm.c index fdac34f..910a34d 100644 --- a/builtin-rm.c +++ b/builtin-rm.c @@ -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) -- 1.6.0.2.328.g14651 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c 2008-09-25 20:22 ` [PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c Alex Riesen @ 2008-09-26 15:28 ` Shawn O. Pearce 2008-09-26 22:56 ` [PATCH] Add remove_path: a function to remove as much as possible of a path Alex Riesen 0 siblings, 1 reply; 10+ messages in thread From: Shawn O. Pearce @ 2008-09-26 15:28 UTC (permalink / raw) To: Alex Riesen; +Cc: git, Junio C Hamano Alex Riesen <raa.lkml@gmail.com> wrote: > 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... ;-) > diff --git a/builtin-rm.c b/builtin-rm.c > index fdac34f..910a34d 100644 > --- a/builtin-rm.c > +++ b/builtin-rm.c > @@ -31,22 +31,18 @@ static void add_list(const char *name) > > static int remove_file(const char *name) > { -- Shawn. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Add remove_path: a function to remove as much as possible of a path 2008-09-26 15:28 ` Shawn O. Pearce @ 2008-09-26 22:56 ` Alex Riesen 2008-09-26 22:59 ` [PATCH] Use remove_path from dir.c instead of own implementation Alex Riesen 0 siblings, 1 reply; 10+ messages in thread From: Alex Riesen @ 2008-09-26 22:56 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git, Junio C Hamano 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 <raa.lkml@gmail.com> --- Shawn O. Pearce, Fri, Sep 26, 2008 17:28:23 +0200: > Alex Riesen <raa.lkml@gmail.com> wrote: > > 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... ;-) > 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(-) diff --git a/dir.c b/dir.c index acf1001..1c3c501 100644 --- a/dir.c +++ b/dir.c @@ -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; +} + diff --git a/dir.h b/dir.h index 2df15de..278ee42 100644 --- a/dir.h +++ b/dir.h @@ -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 -- 1.6.0.2.328.g14651 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] Use remove_path from dir.c instead of own implementation 2008-09-26 22:56 ` [PATCH] Add remove_path: a function to remove as much as possible of a path Alex Riesen @ 2008-09-26 22:59 ` Alex Riesen 0 siblings, 0 replies; 10+ messages in thread From: Alex Riesen @ 2008-09-26 22:59 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git, Junio C Hamano 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 <raa.lkml@gmail.com> --- Alex Riesen, Sat, Sep 27, 2008 00:56:45 +0200: > > 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(-) diff --git a/builtin-apply.c b/builtin-apply.c index 20bef1f..70c9f93 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -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); } } } diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index a29b47f..36aa798 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -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; diff --git a/builtin-rm.c b/builtin-rm.c index fdac34f..50ae6d5 100644 --- a/builtin-rm.c +++ b/builtin-rm.c @@ -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; } -- 1.6.0.2.328.g14651 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Remove empty directories in recursive merge 2008-09-25 20:12 ` [PATCH] Remove empty directories in recursive merge Alex Riesen 2008-09-25 20:17 ` [PATCH] Cleanup remove_path Alex Riesen @ 2008-09-25 20:33 ` Alex Riesen 2008-09-26 15:06 ` Shawn O. Pearce 2008-09-26 5:58 ` Johannes Sixt 2 siblings, 1 reply; 10+ messages in thread From: Alex Riesen @ 2008-09-25 20:33 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce, Junio C Hamano Alex Riesen, Thu, Sep 25, 2008 22:12:45 +0200: > The code was actually supposed to do that, but was accidentally broken. > Noticed by Anders Melchiorsen. > > Signed-off-by: Alex Riesen <raa.lkml@gmail.com> 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Remove empty directories in recursive merge 2008-09-25 20:33 ` [PATCH] Remove empty directories in recursive merge Alex Riesen @ 2008-09-26 15:06 ` Shawn O. Pearce 0 siblings, 0 replies; 10+ messages in thread From: Shawn O. Pearce @ 2008-09-26 15:06 UTC (permalink / raw) To: Alex Riesen; +Cc: git, Junio C Hamano Alex Riesen <raa.lkml@gmail.com> wrote: > Alex Riesen, Thu, Sep 25, 2008 22:12:45 +0200: > > The code was actually supposed to do that, but was accidentally broken. > > Noticed by Anders Melchiorsen. > > > > Signed-off-by: Alex Riesen <raa.lkml@gmail.com> > > 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. > 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. -- Shawn. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Remove empty directories in recursive merge 2008-09-25 20:12 ` [PATCH] Remove empty directories in recursive merge Alex Riesen 2008-09-25 20:17 ` [PATCH] Cleanup remove_path Alex Riesen 2008-09-25 20:33 ` [PATCH] Remove empty directories in recursive merge Alex Riesen @ 2008-09-26 5:58 ` Johannes Sixt 2 siblings, 0 replies; 10+ messages in thread From: Johannes Sixt @ 2008-09-26 5:58 UTC (permalink / raw) To: Alex Riesen; +Cc: git, Anders Melchiorsen, Shawn O. Pearce, Junio C Hamano Alex Riesen schrieb: > +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. > +' > + > test_done -- Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-09-26 23:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-09-24 16:32 [BUG] git merge does not prune empty directories Anders Melchiorsen 2008-09-25 20:12 ` [PATCH] Remove empty directories in recursive merge Alex Riesen 2008-09-25 20:17 ` [PATCH] Cleanup remove_path Alex Riesen 2008-09-25 20:22 ` [PATCH] Fix memleak and the implementation of remove_file in builtin-rm.c Alex Riesen 2008-09-26 15:28 ` Shawn O. Pearce 2008-09-26 22:56 ` [PATCH] Add remove_path: a function to remove as much as possible of a path Alex Riesen 2008-09-26 22:59 ` [PATCH] Use remove_path from dir.c instead of own implementation Alex Riesen 2008-09-25 20:33 ` [PATCH] Remove empty directories in recursive merge Alex Riesen 2008-09-26 15:06 ` Shawn O. Pearce 2008-09-26 5:58 ` Johannes Sixt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).