{"thread":{"id":"34875","subject":"[PATCH] dir: remove dead code","startedAt":"2013-09-08T06:09:23Z","lastAt":"2013-09-08T16:03:36Z","messageCount":3,"participants":["Ramkumar Ramachandra","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"227075","messageId":"1378620563-32709-1-git-send-email-artagnon@gmail.com","threadId":"34875","inReplyTo":null,"subject":"[PATCH] dir: remove dead code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-09-08T06:09:23Z","receivedAt":"2013-09-08T06:09:23Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Remove dead code around remove_dir_recursively().\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n dir.c | 21 ++++-----------------\n 1 file changed, 4 insertions(+), 17 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 910bfcd..2b31241 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1464,11 +1464,11 @@ int is_empty_dir(const char *path)\n \treturn ret;\n }\n \n-static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n+int remove_dir_recursively(struct strbuf *path, int flag)\n {\n \tDIR *dir;\n \tstruct dirent *e;\n-\tint ret = 0, original_len = path->len, len, kept_down = 0;\n+\tint ret = 0, original_len = path->len, len;\n \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n \tint keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);\n \tunsigned char submodule_head[20];\n@@ -1476,8 +1476,6 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\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\tif (kept_up)\n-\t\t\t*kept_up = 1;\n \t\treturn 0;\n \t}\n \n@@ -1504,7 +1502,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\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_recurse(path, flag, &kept_down))\n+\t\t\tif (!remove_dir_recursively(path, flag))\n \t\t\t\tcontinue; /* happy */\n \t\t} else if (!only_empty && !unlink(path->buf))\n \t\t\tcontinue; /* happy, too */\n@@ -1516,22 +1514,11 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \tclosedir(dir);\n \n \tstrbuf_setlen(path, original_len);\n-\tif (!ret && !keep_toplevel && !kept_down)\n+\tif (!ret && !keep_toplevel)\n \t\tret = rmdir(path->buf);\n-\telse if (kept_up)\n-\t\t/*\n-\t\t * report the uplevel that it is not an error that we\n-\t\t * did not rmdir() our directory.\n-\t\t */\n-\t\t*kept_up = !ret;\n \treturn ret;\n }\n \n-int remove_dir_recursively(struct strbuf *path, int flag)\n-{\n-\treturn remove_dir_recurse(path, flag, NULL);\n-}\n-\n void setup_standard_excludes(struct dir_struct *dir)\n {\n \tconst char *path;\n-- \n1.8.4.100.gde18f6d.dirty\n"},{"id":"227125","messageId":"522C6DFA.10204@web.de","threadId":"34875","inReplyTo":"1378620563-32709-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] dir: remove dead code","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-09-08T12:30:50Z","receivedAt":"2013-09-08T12:30:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.09.2013 08:09, schrieb Ramkumar Ramachandra:\n> Remove dead code around remove_dir_recursively().\n\nThis basically reverts ae2f203e (clean: preserve nested git worktree in \nsubdirectories).  t7300 still seems to pass, though.  I wonder why.\n\n>\n> Signed-off-by: Ramkumar Ramachandra<artagnon@gmail.com>\n> ---\n>   dir.c | 21 ++++-----------------\n>   1 file changed, 4 insertions(+), 17 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 910bfcd..2b31241 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1464,11 +1464,11 @@ int is_empty_dir(const char *path)\n>   \treturn ret;\n>   }\n>\n> -static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n> +int remove_dir_recursively(struct strbuf *path, int flag)\n>   {\n>   \tDIR *dir;\n>   \tstruct dirent *e;\n> -\tint ret = 0, original_len = path->len, len, kept_down = 0;\n> +\tint ret = 0, original_len = path->len, len;\n>   \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n>   \tint keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);\n>   \tunsigned char submodule_head[20];\n> @@ -1476,8 +1476,6 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\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\tif (kept_up)\n> -\t\t\t*kept_up = 1;\n>   \t\treturn 0;\n>   \t}\n>\n> @@ -1504,7 +1502,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\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_recurse(path, flag, &kept_down))\n> +\t\t\tif (!remove_dir_recursively(path, flag))\n\nkept_down could have been set to 1 here...\n\n>   \t\t\t\tcontinue; /* happy */\n>   \t\t} else if (!only_empty && !unlink(path->buf))\n>   \t\t\tcontinue; /* happy, too */\n> @@ -1516,22 +1514,11 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>   \tclosedir(dir);\n>\n>   \tstrbuf_setlen(path, original_len);\n> -\tif (!ret && !keep_toplevel && !kept_down)\n> +\tif (!ret && !keep_toplevel)\n>   \t\tret = rmdir(path->buf);\n\n... and would have prevented the rmdir() call here.\n\nIs the removed code really dead?  And if not, why does t7300 still pass?\n\n> -\telse if (kept_up)\n> -\t\t/*\n> -\t\t * report the uplevel that it is not an error that we\n> -\t\t * did not rmdir() our directory.\n> -\t\t */\n> -\t\t*kept_up = !ret;\n>   \treturn ret;\n>   }\n>\n> -int remove_dir_recursively(struct strbuf *path, int flag)\n> -{\n> -\treturn remove_dir_recurse(path, flag, NULL);\n> -}\n> -\n>   void setup_standard_excludes(struct dir_struct *dir)\n>   {\n>   \tconst char *path;\n> -- 1.8.4.100.gde18f6d.dirty\n>\n"},{"id":"227140","messageId":"522C9FD8.9000804@web.de","threadId":"34875","inReplyTo":"CALkWK0kYq8nxUVg7bOr-93+WFRSVp4YcpVsJ+3wKcveRV8As2A@mail.gmail.com","subject":"Re: [PATCH] dir: remove dead code","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-09-08T16:03:36Z","receivedAt":"2013-09-08T16:03:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.09.2013 16:42, schrieb Ramkumar Ramachandra:\n> On Sun, Sep 8, 2013 at 6:00 PM, René Scharfe <l.s.r@web.de\n> <mailto:l.s.r@web.de>> wrote:\n>\n>     Am 08.09.2013 08:09, schrieb Ramkumar Ramachandra:\n>\n>         Remove dead code around remove_dir_recursively().\n>\n>\n>     This basically reverts ae2f203e (clean: preserve nested git worktree\n>     in subdirectories).  t7300 still seems to pass, though.  I wonder why.\n>\n>\n> t7300 has nothing to do with ae2f203e.\n\nae2f203e modified t/t7300-clean.sh.\n\n>\n>            dir.c | 21 ++++-----------------\n>            1 file changed, 4 insertions(+), 17 deletions(-)\n>\n>         diff --git a/dir.c b/dir.c\n>         index 910bfcd..2b31241 100644\n>         --- a/dir.c\n>         +++ b/dir.c\n>         @@ -1464,11 +1464,11 @@ int is_empty_dir(const char *path)\n>                  return ret;\n>            }\n>\n>         -static int remove_dir_recurse(struct strbuf *path, int flag,\n>         int *kept_up)\n>         +int remove_dir_recursively(struct strbuf *path, int flag)\n>            {\n>                  DIR *dir;\n>                  struct dirent *e;\n>         -       int ret = 0, original_len = path->len, len, kept_down = 0;\n>         +       int ret = 0, original_len = path->len, len;\n>                  int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n>                  int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);\n>                  unsigned char submodule_head[20];\n>         @@ -1476,8 +1476,6 @@ static int remove_dir_recurse(struct\n>         strbuf *path, int flag, int *kept_up)\n>                  if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n>                      !resolve_gitlink_ref(path->__buf, \"HEAD\",\n>         submodule_head)) {\n>                          /* Do not descend and nuke a nested git work\n>         tree. */\n>         -               if (kept_up)\n>         -                       *kept_up = 1;\n>                          return 0;\n>                  }\n>\n>         @@ -1504,7 +1502,7 @@ static int remove_dir_recurse(struct\n>         strbuf *path, int flag, int *kept_up)\n>                          if (lstat(path->buf, &st))\n>                                  ; /* fall thru */\n>                          else if (S_ISDIR(st.st_mode)) {\n>         -                       if (!remove_dir_recurse(path, flag,\n>         &kept_down))\n>         +                       if (!remove_dir_recursively(path, flag))\n>\n>\n>     kept_down could have been set to 1 here...\n>\n>\n> Not possible.\n\nWhy?\n\nI guess the answer is that kept_down could have only been set if the \nflag REMOVE_DIR_KEEP_NESTED_GIT is given, which only git clean uses, \nwhich in turn has its own implementation of remove_dir_recursively() \nnamed remove_dirs() since f538a91e (git-clean: Display more accurate \ndelete messages).\n\nThat probably means you can remove even more code from \nremove_dir_recursively().\n\n>\n>                                          continue; /* happy */\n>                          } else if (!only_empty && !unlink(path->buf))\n>                                  continue; /* happy, too */\n>         @@ -1516,22 +1514,11 @@ static int remove_dir_recurse(struct\n>         strbuf *path, int flag, int *kept_up)\n>                  closedir(dir);\n>\n>                  strbuf_setlen(path, original_len);\n>         -       if (!ret && !keep_toplevel && !kept_down)\n>         +       if (!ret && !keep_toplevel)\n>                          ret = rmdir(path->buf);\n>\n>\n>     ... and would have prevented the rmdir() call here.\n>\n>     Is the removed code really dead?  And if not, why does t7300 still pass?\n>\n>\n> Yes, it clearly is. Shared secret.\n\nWhat is the secret and who shares it?\n\nThanks,\nRené\n"}]}