Re: [PATCH] dir: remove dead code

From: René Scharfe <l.s.r@web.de>
Date: Sep 8, 2013, 16:03 UTC
Message-ID: <522C9FD8.9000804@web.de>
In-Reply-To: <CALkWK0kYq8nxUVg7bOr-93+WFRSVp4YcpVsJ+3wKcveRV8As2A@mail.gmail.com>

Am 08.09.2013 16:42, schrieb Ramkumar Ramachandra:

Show 13 quoted lines

> On Sun, Sep 8, 2013 at 6:00 PM, René Scharfe <l.s.r@web.de
> <mailto:l.s.r@web.de>> wrote:
>
>     Am 08.09.2013 08:09, schrieb Ramkumar Ramachandra:
>
>         Remove dead code around remove_dir_recursively().
>
>
>     This basically reverts ae2f203e (clean: preserve nested git worktree
>     in subdirectories).  t7300 still seems to pass, though.  I wonder why.
>
>
> t7300 has nothing to do with ae2f203e.

ae2f203e modified t/t7300-clean.sh.

Show 49 quoted lines

>
>            dir.c | 21 ++++-----------------
>            1 file changed, 4 insertions(+), 17 deletions(-)
>
>         diff --git a/dir.c b/dir.c
>         index 910bfcd..2b31241 100644
>         --- a/dir.c
>         +++ b/dir.c
>         @@ -1464,11 +1464,11 @@ int is_empty_dir(const char *path)
>                  return ret;
>            }
>
>         -static int remove_dir_recurse(struct strbuf *path, int flag,
>         int *kept_up)
>         +int remove_dir_recursively(struct strbuf *path, int flag)
>            {
>                  DIR *dir;
>                  struct dirent *e;
>         -       int ret = 0, original_len = path->len, len, kept_down = 0;
>         +       int ret = 0, original_len = path->len, len;
>                  int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
>                  int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
>                  unsigned char submodule_head[20];
>         @@ -1476,8 +1476,6 @@ static int remove_dir_recurse(struct
>         strbuf *path, int flag, int *kept_up)
>                  if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
>                      !resolve_gitlink_ref(path->__buf, "HEAD",
>         submodule_head)) {
>                          /* Do not descend and nuke a nested git work
>         tree. */
>         -               if (kept_up)
>         -                       *kept_up = 1;
>                          return 0;
>                  }
>
>         @@ -1504,7 +1502,7 @@ static int remove_dir_recurse(struct
>         strbuf *path, int flag, int *kept_up)
>                          if (lstat(path->buf, &st))
>                                  ; /* fall thru */
>                          else if (S_ISDIR(st.st_mode)) {
>         -                       if (!remove_dir_recurse(path, flag,
>         &kept_down))
>         +                       if (!remove_dir_recursively(path, flag))
>
>
>     kept_down could have been set to 1 here...
>
>
> Not possible.

Why?

I guess the answer is that kept_down could have only been set if the flag REMOVE_DIR_KEEP_NESTED_GIT is given, which only git clean uses, which in turn has its own implementation of remove_dir_recursively() named remove_dirs() since f538a91e (git-clean: Display more accurate delete messages).

That probably means you can remove even more code from remove_dir_recursively().

Show 20 quoted lines

>
>                                          continue; /* happy */
>                          } else if (!only_empty && !unlink(path->buf))
>                                  continue; /* happy, too */
>         @@ -1516,22 +1514,11 @@ static int remove_dir_recurse(struct
>         strbuf *path, int flag, int *kept_up)
>                  closedir(dir);
>
>                  strbuf_setlen(path, original_len);
>         -       if (!ret && !keep_toplevel && !kept_down)
>         +       if (!ret && !keep_toplevel)
>                          ret = rmdir(path->buf);
>
>
>     ... and would have prevented the rmdir() call here.
>
>     Is the removed code really dead?  And if not, why does t7300 still pass?
>
>
> Yes, it clearly is. Shared secret.

What is the secret and who shares it?

Thanks, René