{"thread":{"id":"19974","subject":"[PATCH 0/2] Don't delete untracked submodule's .git dirs by default","startedAt":"2009-06-30T02:10:43Z","lastAt":"2009-07-01T02:13:22Z","messageCount":10,"participants":["Jason Holden","Paolo Bonzini","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"117200","messageId":"1246327845-22718-1-git-send-email-jason.k.holden@gmail.com","threadId":"19974","inReplyTo":null,"subject":"[PATCH 0/2] Don't delete untracked submodule's .git dirs by default","fromName":"Jason Holden","fromEmail":"jason.k.holden@gmail.com","sentAt":"2009-06-30T02:10:43Z","receivedAt":"2009-06-30T02:10:43Z","isPatch":true,"sender":{"key":"jason.k.holden@gmail.com","avatar":null},"body":"Git-clean is not safe when there is a submodule tracked in a local branch that\nis not tracked in the mainline branch. Running git-clean from the mainline\nbranch when we have unpushed changes in a submodule tracked only in a local\nbranch can lose local changes to that submodule permanentely.\n\nI have accidentally lost changes in this way when working with very \nlarge projects where a git-clean is more reliable than a makefile's \n\"make clean\". \n\nBy changing the default behavior of git-clean to not delete the .git\ndirectories allows the history of the submodules to be recovered.\n\n# Example of issue:\n#\n# Clone mainline project\ngit clone git://github.com/thoughtbot/paperclip.git           \ncd paperclip/\n\n# Add a submodule not tracked by mainline\ngit checkout -b test_submodule_clean\ngit submodule add git://github.com/technoweenie/attachment_fu.git attachement_fu\ngit commit -m \"add submodule\"\n\n# Make a modification to the submodule.  Note that we haven't pushed the change\ncd attachement_fu/\ngit checkout -b mod_readme_in_submodule\nvi README \ngit add README\ngit commit -m \"Small change in submodule\"\n\n# Go back to mainline's master branch and do a clean\ncd ..\ngit checkout master\ngit clean -f -d\n\n# Our change to the submodule, that was never pushed, is now gone forever \n# because all the history stored in the submodule's .git direct is deleted.\n# There is no recovering from this.\n# This breaks the \"git must be safe\" rule, as we've lost potentially a lot of\n# changes to any submodule projects that didn't get pushed yet. Solve\n# this issue by not deleting any .git directories we come across during a\n# git-clean, unless the \"-m\" option is passed to git-clean.\n\nThis is my first email submittal using git, so apologies in advance for any\nformatting issues\n\nJason Holden (2):\n  Add option to not delete a .git directory in remove_dir_recursively()\n  Don't clean any untracked submodule's .git dir by default in\n    git-clean\n\n Documentation/git-clean.txt |    6 +++++-\n builtin-clean.c             |   15 +++++++++++++--\n builtin-clone.c             |    4 ++--\n dir.c                       |   17 +++++++++++++++--\n dir.h                       |    2 +-\n refs.c                      |    2 +-\n transport.c                 |    4 ++--\n 7 files changed, 39 insertions(+), 11 deletions(-)\n"},{"id":"117201","messageId":"1246327845-22718-2-git-send-email-jason.k.holden@gmail.com","threadId":"19974","inReplyTo":"1246327845-22718-1-git-send-email-jason.k.holden@gmail.com","subject":"[PATCH 1/2] Add option to not delete a .git directory in remove_dir_recursively()","fromName":"Jason Holden","fromEmail":"jason.k.holden@gmail.com","sentAt":"2009-06-30T02:10:44Z","receivedAt":"2009-06-30T02:10:44Z","isPatch":true,"sender":{"key":"jason.k.holden@gmail.com","avatar":null},"body":"Because all existing calls to remove_dir_recursively() do not\ncurrently have this protection, default all existing calls\nto 0 (to not keep .git directories)\n\nSigned-off-by: Jason Holden <jason.k.holden@gmail.com>\n---\n builtin-clean.c |    2 +-\n builtin-clone.c |    4 ++--\n dir.c           |   17 +++++++++++++++--\n dir.h           |    2 +-\n refs.c          |    2 +-\n transport.c     |    4 ++--\n 6 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex 1c1b6d2..cd82407 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -141,7 +141,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\t   (matches == MATCHED_EXACTLY)) {\n \t\t\t\tif (!quiet)\n \t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n-\t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\n+\t\t\t\tif (remove_dir_recursively(&directory, 0, 0) != 0) {\n \t\t\t\t\twarning(\"failed to remove '%s'\", qname);\n \t\t\t\t\terrors++;\n \t\t\t\t}\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 2ceacb7..0c00c87 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -304,12 +304,12 @@ static void remove_junk(void)\n \t\treturn;\n \tif (junk_git_dir) {\n \t\tstrbuf_addstr(&sb, junk_git_dir);\n-\t\tremove_dir_recursively(&sb, 0);\n+\t\tremove_dir_recursively(&sb, 0, 0);\n \t\tstrbuf_reset(&sb);\n \t}\n \tif (junk_work_tree) {\n \t\tstrbuf_addstr(&sb, junk_work_tree);\n-\t\tremove_dir_recursively(&sb, 0);\n+\t\tremove_dir_recursively(&sb, 0, 0);\n \t\tstrbuf_reset(&sb);\n \t}\n }\ndiff --git a/dir.c b/dir.c\nindex bbfcb56..eadcddd 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -800,7 +800,7 @@ int is_empty_dir(const char *path)\n \treturn ret;\n }\n \n-int remove_dir_recursively(struct strbuf *path, int only_empty)\n+int remove_dir_recursively(struct strbuf *path, int only_empty, int keep_dot_git)\n {\n \tDIR *dir = opendir(path->buf);\n \tstruct dirent *e;\n@@ -812,6 +812,19 @@ int remove_dir_recursively(struct strbuf *path, int only_empty)\n \t\tstrbuf_addch(path, '/');\n \n \tlen = path->len;\n+\n+\tif (keep_dot_git) {\n+\t\tchar end_of_path[6]; /* enough space for \".git/\"*/\n+\t\tmemset(end_of_path, '\\0', 6);\n+\t\tif (len >= 5) {\n+\t\t\tstrncpy(end_of_path, path->buf + len - 5, 5);\n+\t\t\tif (strcmp(end_of_path, \".git/\") == 0) {\n+\t\t\t\tprintf(\"********Found .git!!!!  Skipping delete\\n\");\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \twhile ((e = readdir(dir)) != NULL) {\n \t\tstruct stat st;\n \t\tif (is_dot_or_dotdot(e->d_name))\n@@ -822,7 +835,7 @@ int remove_dir_recursively(struct strbuf *path, int only_empty)\n \t\tif (lstat(path->buf, &st))\n \t\t\t; /* fall thru */\n \t\telse if (S_ISDIR(st.st_mode)) {\n-\t\t\tif (!remove_dir_recursively(path, only_empty))\n+\t\t\tif (!remove_dir_recursively(path, only_empty, keep_dot_git))\n \t\t\t\tcontinue; /* happy */\n \t\t} else if (!only_empty && !unlink(path->buf))\n \t\t\tcontinue; /* happy, too */\ndiff --git a/dir.h b/dir.h\nindex 541286a..8273bb9 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -89,7 +89,7 @@ static inline int is_dot_or_dotdot(const char *name)\n extern int is_empty_dir(const char *dir);\n \n extern void setup_standard_excludes(struct dir_struct *dir);\n-extern int remove_dir_recursively(struct strbuf *path, int only_empty);\n+extern int remove_dir_recursively(struct strbuf *path, int only_empty, int keep_dot_git);\n \n /* tries to remove the path with empty directories along it, ignores ENOENT */\n extern int remove_path(const char *path);\ndiff --git a/refs.c b/refs.c\nindex 24438c6..4ddb361 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -820,7 +820,7 @@ static int remove_empty_directories(const char *file)\n \tstrbuf_init(&path, 20);\n \tstrbuf_addstr(&path, file);\n \n-\tresult = remove_dir_recursively(&path, 1);\n+\tresult = remove_dir_recursively(&path, 1, 0);\n \n \tstrbuf_release(&path);\n \ndiff --git a/transport.c b/transport.c\nindex 501a77b..067d6b1 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -196,7 +196,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)\n \tinsert_packed_refs(temp_dir.buf, &tail);\n \tstrbuf_setlen(&temp_dir, temp_dir_len);\n \n-\tif (remove_dir_recursively(&temp_dir, 0))\n+\tif (remove_dir_recursively(&temp_dir, 0, 0))\n \t\twarning (\"Error removing temporary directory %s.\",\n \t\t\t\ttemp_dir.buf);\n \n@@ -342,7 +342,7 @@ static int rsync_transport_push(struct transport *transport,\n \t\tresult = error(\"Could not push to %s\",\n \t\t\t\trsync_url(transport->url));\n \n-\tif (remove_dir_recursively(&temp_dir, 0))\n+\tif (remove_dir_recursively(&temp_dir, 0, 0))\n \t\twarning (\"Could not remove temporary directory %s.\",\n \t\t\t\ttemp_dir.buf);\n \n-- \n1.6.3.2.207.ga8208\n"},{"id":"117202","messageId":"1246327845-22718-3-git-send-email-jason.k.holden@gmail.com","threadId":"19974","inReplyTo":"1246327845-22718-2-git-send-email-jason.k.holden@gmail.com","subject":"[PATCH 2/2] Don't clean any untracked submodule's .git dir by default in git-clean","fromName":"Jason Holden","fromEmail":"jason.k.holden@gmail.com","sentAt":"2009-06-30T02:10:45Z","receivedAt":"2009-06-30T02:10:45Z","isPatch":true,"sender":{"key":"jason.k.holden@gmail.com","avatar":null},"body":"Git-clean is not safe when the submodules are not tracked in mainline.  If\nwe run git-clean on the mainline branch, when we have a submodule that only\nexists on a local branch, the entire .git directory of the untracked\nsubmodule will get deleted, possibly losing any un-pushed local changes to\nthe submodule.\n\nThis change doesn't delete any untracked submodule's .git directories during\nthe recursive-delete (unless forced with the -m option to git-clean), so that\nthe submodule history can be restored w/ the proper git commands.\n\n# Example illustrating problem:\n# Clone mainline project\ngit clone git://github.com/thoughtbot/paperclip.git\ncd paperclip/\n\n# Add a submodule not tracked by mainline\ngit checkout -b test_submodule_clean\ngit submodule add git://github.com/technoweenie/attachment_fu.git attachement_fu\ngit commit -m \"add submodule\"\n\n# Make a modification to the submodule.  Note that we haven't pushed the change\ncd attachement_fu/\ngit checkout -b mod_readme_in_submodule\nvi README\ngit add README\ngit commit -m \"Small change in submodule\"\n\n# Go back to mainline's master branch and do a clean\ncd ..\ngit checkout master\ngit clean -f -d\n\n# Our change to the submodule, that was never pushed, is now gone forever\n# because all the history stored in the submodule's .git direct is deleted.\n# There is no recovering from this.\n# This breaks the \"git must be safe\" rule, as we've lost potentially a lot of\n# changes to any submodule projects that didn't get pushed yet. Solve\n# this issue by not deleting any .git directories we come across during a\n# git-clean, unless the \"-m\" option is passed to git-clean.\n\nSigned-off-by: Jason Holden <jason.k.holden@gmail.com>\n---\n Documentation/git-clean.txt |    6 +++++-\n builtin-clean.c             |   15 +++++++++++++--\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex be894af..04a5a65 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -8,7 +8,7 @@ git-clean - Remove untracked files from the working tree\n SYNOPSIS\n --------\n [verse]\n-'git clean' [-d] [-f] [-n] [-q] [-x | -X] [--] <path>...\n+'git clean' [-d] [-f] [-n] [-q] [-m] [-x | -X] [--] <path>...\n \n DESCRIPTION\n -----------\n@@ -41,6 +41,10 @@ OPTIONS\n \tBe quiet, only report errors, but not the files that are\n \tsuccessfully removed.\n \n+-m::\n+\tClean any .git directories that may be left-over, untracked\n+\tsubmodules.\n+\n -x::\n \tDon't use the ignore rules.  This allows removing all untracked\n \tfiles, including build products.  This can be used (possibly in\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex cd82407..60d78dc 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -15,7 +15,7 @@\n static int force = -1; /* unset */\n \n static const char *const builtin_clean_usage[] = {\n-\t\"git clean [-d] [-f] [-n] [-q] [-x | -X] [--] <paths>...\",\n+\t\"git clean [-d] [-f] [-n] [-q] [-m] [-x | -X] [--] <paths>...\",\n \tNULL\n };\n \n@@ -31,6 +31,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;\n \tint ignored_only = 0, baselen = 0, config_set = 0, errors = 0;\n+\tint rm_untracked_submodules = 0;\n \tstruct strbuf directory = STRBUF_INIT;\n \tstruct dir_struct dir;\n \tconst char *path, *base;\n@@ -44,6 +45,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOLEAN('f', NULL, &force, \"force\"),\n \t\tOPT_BOOLEAN('d', NULL, &remove_directories,\n \t\t\t\t\"remove whole directories\"),\n+\t\tOPT_BOOLEAN('m', NULL, &rm_untracked_submodules,\n+\t\t\t\t\"remove untracked submodules\"),\n \t\tOPT_BOOLEAN('x', NULL, &ignored, \"remove ignored files, too\"),\n \t\tOPT_BOOLEAN('X', NULL, &ignored_only,\n \t\t\t\t\"remove only ignored files\"),\n@@ -59,6 +62,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t     0);\n \n+\n+\tint keep_dot_git = 0;\n+\tif (rm_untracked_submodules == 0)\n+\t\tkeep_dot_git = 1;\n+\telse\n+\t\tprintf(\"Any untracked .git directories will be deleted (abandoned submodules)\\n\");\n+\n+\n \tmemset(&dir, 0, sizeof(dir));\n \tif (ignored_only)\n \t\tdir.flags |= DIR_SHOW_IGNORED;\n@@ -141,7 +152,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\t   (matches == MATCHED_EXACTLY)) {\n \t\t\t\tif (!quiet)\n \t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n-\t\t\t\tif (remove_dir_recursively(&directory, 0, 0) != 0) {\n+\t\t\t\tif (remove_dir_recursively(&directory, 0, keep_dot_git) != 0) {\n \t\t\t\t\twarning(\"failed to remove '%s'\", qname);\n \t\t\t\t\terrors++;\n \t\t\t\t}\n-- \n1.6.3.2.207.ga8208\n"},{"id":"117210","messageId":"4A49AB85.40303@gnu.org","threadId":"19974","inReplyTo":"1246327845-22718-3-git-send-email-jason.k.holden@gmail.com","subject":"Re: [PATCH 2/2] Don't clean any untracked submodule's .git dir by default in git-clean","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2009-06-30T06:07:01Z","receivedAt":"2009-06-30T06:07:01Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"Useful indeed.\n\nNote that 'git clean -n -d ' however will still report the directory as \nbeing removed.  Also, I'm not sure what happens (and what should happen) \nif an untracked directory foo.git is found.\n\nProbably the best way to fix this is to add an is_dot_git_path function \nto dir.c like this\n\nint\nis_dot_git_path (const char *s, int len);\n{\n   while (len && s[len - 1] == '/')\n     len--;\n   return len >= 4 && !memcmp (s + len - 4, \".git\", 5) &&\n          (len == 4 || s[len - 5] == '/');\n}\n\nThis function is safer if the directory does not have a trailing slash, \nas it might be for the paths in builtin-clean.c for the -n case.  You \ncan have some adjustments if you decide do keep foo.git (just removing \nthe last && of course).\n\nThanks!\n\nPaolo\n"},{"id":"117212","messageId":"4A49B36D.2080103@viscovery.net","threadId":"19974","inReplyTo":"1246327845-22718-3-git-send-email-jason.k.holden@gmail.com","subject":"Re: [PATCH 2/2] Don't clean any untracked submodule's .git dir by default in git-clean","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-06-30T06:40:45Z","receivedAt":"2009-06-30T06:40:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jason Holden schrieb:\n> Git-clean is not safe when the submodules are not tracked in mainline.\n\nGenerally, I think you are addressing a real issue. On the other hand, it\nalso changes the behavior substantially. Nevertheless IMO it is better to\nbe safe than sorry, even if existing 'git clean' users may now observe\nthat directories are left over that previously weren't.\n\n>  If\n> we run git-clean on the mainline branch, when we have a submodule that only\n> exists on a local branch, the entire .git directory of the untracked\n> submodule will get deleted, possibly losing any un-pushed local changes to\n> the submodule.\n\nThis is not about \"mainline\" and \"local branch\"; it is about switching\nfrom one branch that tracks the submodule to another one that doesn't\ntrack it.\n\n> This change doesn't delete any untracked submodule's .git directories during\n> the recursive-delete (unless forced with the -m option to git-clean), so that\n> the submodule history can be restored w/ the proper git commands.\n> \n> # Example illustrating problem:\n> # Clone mainline project\n> git clone git://github.com/thoughtbot/paperclip.git\n> cd paperclip/\n> \n> # Add a submodule not tracked by mainline\n> git checkout -b test_submodule_clean\n\n# Add a submodule to a different branch\n# git checkout -b has-submodule\n\n> git submodule add git://github.com/technoweenie/attachment_fu.git attachement_fu\n> git commit -m \"add submodule\"\n> \n> # Make a modification to the submodule.  Note that we haven't pushed the change\n> cd attachement_fu/\n> git checkout -b mod_readme_in_submodule\n> vi README\n> git add README\n> git commit -m \"Small change in submodule\"\n> \n> # Go back to mainline's master branch and do a clean\n> cd ..\n> git checkout master\n> git clean -f -d\n> \n> # Our change to the submodule, that was never pushed, is now gone forever\n> # because all the history stored in the submodule's .git direct is deleted.\n> # There is no recovering from this.\n> # This breaks the \"git must be safe\" rule, as we've lost potentially a lot of\n> # changes to any submodule projects that didn't get pushed yet. Solve\n> # this issue by not deleting any .git directories we come across during a\n> # git-clean, unless the \"-m\" option is passed to git-clean.\n\nIf you indent the example script by some spaces, you won't have to mark\nthe surrounding text like shell script comments (surrounding text is the\nline '# Example...' and the paragraph '# Our change...'. But the\ninterspersed comments are very helpful.\n\n> +-m::\n> +\tClean any .git directories that may be left-over, untracked\n> +\tsubmodules.\n\n\tRemove .git directories from subdirectories (i.e.\n\tuntracked submodules).\n\nPlease address (here and in the code later) that -m makes sense only in\ncombination with -d.\n\nThere is one in-tree user of git-clean (git-filter-branch). Did you check\nwhether it needs this new flag?\n\n> @@ -44,6 +45,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOLEAN('f', NULL, &force, \"force\"),\n>  \t\tOPT_BOOLEAN('d', NULL, &remove_directories,\n>  \t\t\t\t\"remove whole directories\"),\n> +\t\tOPT_BOOLEAN('m', NULL, &rm_untracked_submodules,\n> +\t\t\t\t\"remove untracked submodules\"),\n>  \t\tOPT_BOOLEAN('x', NULL, &ignored, \"remove ignored files, too\"),\n>  \t\tOPT_BOOLEAN('X', NULL, &ignored_only,\n>  \t\t\t\t\"remove only ignored files\"),\n> @@ -59,6 +62,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>  \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n>  \t\t\t     0);\n>  \n> +\n> +\tint keep_dot_git = 0;\n> +\tif (rm_untracked_submodules == 0)\n> +\t\tkeep_dot_git = 1;\n> +\telse\n> +\t\tprintf(\"Any untracked .git directories will be deleted (abandoned submodules)\\n\");\n> +\n> +\n\nI can share your feelings about lost work, and that you want to be extra\nverbose about .git directories.\n\nBut step back a bit. This warning is absolutely useless: It just repeats\nthe user's instruction: After passing -m, we *expect* 'git clean' to\nremove .git directories.\n\nBTW, for what reason are you using a new variable keep_dot_git if there is\nalready rm_untracked_submodules?\n\nPlease add a test to the test suite.\n\n-- Hannes\n"},{"id":"117213","messageId":"4A49B529.7030900@viscovery.net","threadId":"19974","inReplyTo":"1246327845-22718-2-git-send-email-jason.k.holden@gmail.com","subject":"Re: [PATCH 1/2] Add option to not delete a .git directory in remove_dir_recursively()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-06-30T06:48:09Z","receivedAt":"2009-06-30T06:48:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jason Holden schrieb:\n> @@ -812,6 +812,19 @@ int remove_dir_recursively(struct strbuf *path, int only_empty)\n>  \t\tstrbuf_addch(path, '/');\n>  \n>  \tlen = path->len;\n> +\n> +\tif (keep_dot_git) {\n> +\t\tchar end_of_path[6]; /* enough space for \".git/\"*/\n> +\t\tmemset(end_of_path, '\\0', 6);\n> +\t\tif (len >= 5) {\n> +\t\t\tstrncpy(end_of_path, path->buf + len - 5, 5);\n> +\t\t\tif (strcmp(end_of_path, \".git/\") == 0) {\n> +\t\t\t\tprintf(\"********Found .git!!!!  Skipping delete\\n\");\n\nI see no reason to ***shout!!!*** here. IOW:\n\n\t\t\t\twarning(\"not removing %s\", dir);\n\nis enough. This also sends the text to stderr.\n\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n>  \twhile ((e = readdir(dir)) != NULL) {\n>  \t\tstruct stat st;\n>  \t\tif (is_dot_or_dotdot(e->d_name))\n\nI think it is even better to move the check for \".git\" below this 'if'. It\nshould not make a difference in practice.\n\n-- Hannes\n"},{"id":"117215","messageId":"7vljna9nuz.fsf@alter.siamese.dyndns.org","threadId":"19974","inReplyTo":"4A49B36D.2080103@viscovery.net","subject":"Re: [PATCH 2/2] Don't clean any untracked submodule's .git dir by default in git-clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-30T07:34:12Z","receivedAt":"2009-06-30T07:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Jason Holden schrieb:\n> ...\n>>  If\n>> we run git-clean on the mainline branch, when we have a submodule that only\n>> exists on a local branch, the entire .git directory of the untracked\n>> submodule will get deleted, possibly losing any un-pushed local changes to\n>> the submodule.\n>\n> This is not about \"mainline\" and \"local branch\"; it is about switching\n> from one branch that tracks the submodule to another one that doesn't\n> track it.\n\nI do not think it is even about that.\n\nIf you have an old-style nested git work tree, i.e. you have an\nindependent git repository in some subdirectory of a work tree of a git\nwork tree, you will have exactly the same issue.  There is no need for any\nsubmodule to get involved.\n\nFor example, I have a clone of git.git repository at Meta/ and have the\n'todo' branch checked out, so that I can say \"Meta/Make\", \"Meta/Dothem\",\netc.  In such a set-up, if you do not have Meta/ in .gitignore (or even if\nyou do, if you said \"git clean -f -x -d\"), you will lose that directory\n(and arguably that is by design).\n\nI think protecting users from mistakes is a very good idea, but I see at\nleast two small problems with the patch.  For brevity I'll name the \"not a\nsubmodule in the HEAD commit of the superproject\" directory \"Meta/\" in the\nfollowing.\n\n (1) Protecting Meta/.git is not goot enough. If it were, and if this is\n     only about submodules, then you can use the \"gitdir:\" facility to\n     relocate Meta/.git directory to somewhere under superproject's .git\n     and be done with it.\n\n     You _must_ protect the checked out files, their uncommitted contents\n     and untracked but unignored files.  After all, Meta/ is a valid git\n     repository in its own right.  Noticing that \"rm -r\" is about to\n     remove Meta/.git after it has already touched many other files in\n     Meta/ is one recursion level too late.\n\n (2) Naming the option to force removal \"-m\" is wrong; this is not about\n     submodule at all.  Can we use double-force \"-f -f\", perhaps?\n"},{"id":"117270","messageId":"7vws6t490z.fsf@alter.siamese.dyndns.org","threadId":"19974","inReplyTo":"7vljna9nuz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Don't clean any untracked submodule's .git dir by default in git-clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-30T23:05:48Z","receivedAt":"2009-06-30T23:05:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think protecting users from mistakes is a very good idea, but I see at\n> least two small problems with the patch.  For brevity I'll name the \"not a\n> submodule in the HEAD commit of the superproject\" directory \"Meta/\" in the\n> following.\n>\n>  (1) Protecting Meta/.git is not goot enough. If it were, and if this is\n>      only about submodules, then you can use the \"gitdir:\" facility to\n>      relocate Meta/.git directory to somewhere under superproject's .git\n>      and be done with it.\n>\n>      You _must_ protect the checked out files, their uncommitted contents\n>      and untracked but unignored files.  After all, Meta/ is a valid git\n>      repository in its own right.  Noticing that \"rm -r\" is about to\n>      remove Meta/.git after it has already touched many other files in\n>      Meta/ is one recursion level too late.\n>\n>  (2) Naming the option to force removal \"-m\" is wrong; this is not about\n>      submodule at all.  Can we use double-force \"-f -f\", perhaps?\n\nPerhaps like this.\n\nUntested, as I never use \"git clean\" myself.\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Tue, 30 Jun 2009 15:33:45 -0700\nSubject: [PATCH] clean: require double -f options to nuke nested git repository and work tree\n\nWhen you have an embedded git work tree in your work tree (be it\nan orphaned submodule, or an independent checkout of an unrelated\nproject), \"git clean -d -f\" blindly descended into it and removed\neverything.  This is rarely what the user wants.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-clean.c  |    7 ++++++-\n dir.c            |   12 ++++++++++--\n dir.h            |    5 ++++-\n refs.c           |    2 +-\n t/t7300-clean.sh |   39 +++++++++++++++++++++++++++++++++++++++\n 5 files changed, 60 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex 1c1b6d2..04ea181 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -31,6 +31,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;\n \tint ignored_only = 0, baselen = 0, config_set = 0, errors = 0;\n+\tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n \tstruct strbuf directory = STRBUF_INIT;\n \tstruct dir_struct dir;\n \tconst char *path, *base;\n@@ -70,6 +71,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tdie(\"clean.requireForce%s set and -n or -f not given; \"\n \t\t    \"refusing to clean\", config_set ? \"\" : \" not\");\n \n+\tif (force > 1)\n+\t\trm_flags = 0;\n+\n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \n \tif (!ignored)\n@@ -141,7 +145,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\t   (matches == MATCHED_EXACTLY)) {\n \t\t\t\tif (!quiet)\n \t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n-\t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\n+\t\t\t\tif (remove_dir_recursively(&directory,\n+\t\t\t\t\t\t\t   rm_flags) != 0) {\n \t\t\t\t\twarning(\"failed to remove '%s'\", qname);\n \t\t\t\t\terrors++;\n \t\t\t\t}\ndiff --git a/dir.c b/dir.c\nindex bbfcb56..d0cfe74 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -800,12 +800,20 @@ int is_empty_dir(const char *path)\n \treturn ret;\n }\n \n-int remove_dir_recursively(struct strbuf *path, int only_empty)\n+int remove_dir_recursively(struct strbuf *path, int flag)\n {\n-\tDIR *dir = opendir(path->buf);\n+\tDIR *dir;\n \tstruct dirent *e;\n \tint ret = 0, original_len = path->len, len;\n+\tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n+\tunsigned char submodule_head[20];\n \n+\tif ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n+\t    !resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head))\n+\t\t/* Do not descend and nuke a nested git work tree. */\n+\t\treturn 0;\n+\n+\tdir = opendir(path->buf);\n \tif (!dir)\n \t\treturn -1;\n \tif (path->buf[original_len - 1] != '/')\ndiff --git a/dir.h b/dir.h\nindex 541286a..8c69bdd 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -89,7 +89,10 @@ static inline int is_dot_or_dotdot(const char *name)\n extern int is_empty_dir(const char *dir);\n \n extern void setup_standard_excludes(struct dir_struct *dir);\n-extern int remove_dir_recursively(struct strbuf *path, int only_empty);\n+\n+#define REMOVE_DIR_EMPTY_ONLY 01\n+#define REMOVE_DIR_KEEP_NESTED_GIT 02\n+extern int remove_dir_recursively(struct strbuf *path, int flag);\n \n /* tries to remove the path with empty directories along it, ignores ENOENT */\n extern int remove_path(const char *path);\ndiff --git a/refs.c b/refs.c\nindex 24438c6..a7dd5ae 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -820,7 +820,7 @@ static int remove_empty_directories(const char *file)\n \tstrbuf_init(&path, 20);\n \tstrbuf_addstr(&path, file);\n \n-\tresult = remove_dir_recursively(&path, 1);\n+\tresult = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);\n \n \tstrbuf_release(&path);\n \ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 929d5d4..118c6eb 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -380,4 +380,43 @@ test_expect_success 'removal failure' '\n '\n chmod 755 foo\n \n+test_expect_success 'nested git work tree' '\n+\trm -fr foo bar &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\t>hello.world\n+\t\tgit add . &&\n+\t\tgit commit -a -m nested\n+\t) &&\n+\t(\n+\t\tcd bar &&\n+\t\t>goodbye.people\n+\t) &&\n+\tgit clean -f -d &&\n+\ttest -f foo/.git/index &&\n+\ttest -f foo/hello.world &&\n+\t! test -d bar\n+'\n+\n+test_expect_success 'force removal of nested git work tree' '\n+\trm -fr foo bar &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\t>hello.world\n+\t\tgit add . &&\n+\t\tgit commit -a -m nested\n+\t) &&\n+\t(\n+\t\tcd bar &&\n+\t\t>goodbye.people\n+\t) &&\n+\tgit clean -f -f -d &&\n+\t! test -d foo &&\n+\t! test -d bar\n+'\n+\n test_done\n-- \n1.6.3.3.362.g3c77e\n"},{"id":"117280","messageId":"4A4ABF61.7040009@gmail.com","threadId":"19974","inReplyTo":"7vws6t490z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Don't clean any untracked submodule's .git dir by default in git-clean","fromName":"Jason Holden","fromEmail":"jason.k.holden@gmail.com","sentAt":"2009-07-01T01:44:01Z","receivedAt":"2009-07-01T01:44:01Z","isPatch":true,"sender":{"key":"jason.k.holden@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> diff --git a/dir.c b/dir.c\n> index bbfcb56..d0cfe74 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -800,12 +800,20 @@ int is_empty_dir(const char *path)\n>  \treturn ret;\n>  }\n>  \n> -int remove_dir_recursively(struct strbuf *path, int only_empty)\n> +int remove_dir_recursively(struct strbuf *path, int flag)\n>  {\n> -\tDIR *dir = opendir(path->buf);\n> +\tDIR *dir;\n>  \tstruct dirent *e;\n>  \tint ret = 0, original_len = path->len, len;\n> +\tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n> +\tunsigned char submodule_head[20];\n>  \n> +\tif ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n> +\t    !resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head))\n> +\t\t/* Do not descend and nuke a nested git work tree. */\n\nAdd a printout here to indicate that we didn't end up removing the\ndirectory.  Otherwise when you run git clean -f -d you just end up\nwith something like:\n\"Removing attachement_fu/\"\neven when we didn't actually end up removing it.  Paolo noticed this\nsame issue in my original patch.\n\n> +\t\treturn 0;\n> +\n> +\tdir = opendir(path->buf);\n>  \tif (!dir)\n\nI was able to test this patch and everything seems to behave as\nexpected.\n\nIf this becomes the final fix, don't forget to update\nDocumentation/git-clean.txt\n\nI'm leaving for vacation tomorrow, so you won't hear any more from\nme until ~July 8th.  Thanks for the quick responses.\n\n-- \nRegards,\nJason Holden\nGet my PGP Key at\nhttp://pgp.mit.edu:11371/pks/lookup?op=get&search=0x6B7FBC8D\n"},{"id":"117282","messageId":"7vskhhxi9p.fsf@alter.siamese.dyndns.org","threadId":"19974","inReplyTo":"4A4ABF61.7040009@gmail.com","subject":"Re: [PATCH 2/2] Don't clean any untracked submodule's .git dir by default in git-clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-01T02:13:22Z","receivedAt":"2009-07-01T02:13:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jason Holden <jason.k.holden@gmail.com> writes:\n\n> If this becomes the final fix, don't forget to update\n> Documentation/git-clean.txt\n\nThat's a note to yourself and other people who are intereseted ;-).\n\nMy patch was, as with many other patches I send to this list, no more than\n\"if you wanted to do that, you would do it like this.\".  It definitely\nwasn't meant to be the final shape of the resolution of this issue.\n\nThis is not my itch with a particularly high priority, and I do not have\ninfinite amount of time right now to scratch it.\n\nThere shouldn't be any output from dir.[ch] recursive removal function\n(unless it is reporting an error).  Instead, the caller should say \"removed\"\nonly after it actually removed it, and it needs some reorganizing of the\ncall sequence.\n\nI think the loop in builtin_clean.c should first be refactored into\nsmaller helper functions before any of these changes happen.  It has got\nunmanageably large and ugly over time (or perhaps it was large and ugly\nfrom the beginning. I do not even remember who did it initially).  \n\nAnyway, enjoy your vacation.\n"}]}