On Thu, Jan 30, 2020 at 7:33 AM Derrick Stolee [off-list ref] wrote:

On 1/29/2020 5:03 PM, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

Signed-off-by: Elijah Newren <redacted>
---
 dir.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 225f0bc082..ef3307718a 100644
--- a/dir.c
+++ b/dir.c

@@ -1659,7 +1659,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
      const char *dirname, int len, int baselen, int excluded,
      const struct pathspec *pathspec)
 {
-     int nested_repo = 0;
+     int nested_repo;

      /* The "len-1" is to strip the final '/' */
      switch (directory_exists_in_index(istate, dirname, len-1)) {

@@ -1670,6 +1670,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
              return path_none;

      case index_nonexistent:
+             nested_repo = 0;

I had to look at this code in-full from en/fill-directory-fixes-more to
be sure that this case was the only use of nested_repo. However, I found
that this switch statement is unnecessarily complicated. By converting
the switch to multiple "if" statements, I noticed that the third case
actually has a "break" statement that can lead to the final "fourth case"
outside the switch statement.

Hopefully the patch below is a worthy replacement for this one:

-->8--

From b5c04e6e028cb6c7f9e78fbdd2182383d928fe6d Mon Sep 17 00:00:00 2001
From: Derrick Stolee <redacted>
Date: Thu, 30 Jan 2020 15:28:39 +0000
Subject: [PATCH] dir: refactor treat_directory to clarify variable scope

The nested_repo variable in treat_directory() is created and
initialized before a multi-case switch statement, but is only
used by one case. In fact, this switch is very asymmetrical,
as the first two cases are simple but the third is more
complicated than the rest of the method.

Extract the switch statement into a series of "if" statements.
This simplifies the trivial cases, while highlighting the fact
that a "break" statement in a condition of the third case
actually leads to jumping to the fourth case (after the switch).
This assists a reader who provides an initial scan to notice
there is a second way to approach the "show_other_directories"
case than simply the response from directory_exists_in_index().

Wait, I'm lost.  Wasn't that break statement the only way to get to
the "show_other_directories" block of code after the switch statement?
 I can't see where the second way is; am I missing something?

That is, unless directory_exists_in_index() suddenly starts returning
some value other than the three current possibilities.  Perhaps we
should throw a BUG() if we get anything other than index_directory,
index_gitdir, or index_nonexistent.

I'd say we'd want to add a BUG("Unhandled value for
directory_exists_in_index: %d\n", status); right here.

        /* This is the "show_other_directories" case */
-
+show_other_directories:
        if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
                return exclude ? path_excluded : path_untracked;

--
2.25.0.vfs.1.1

Otherwise, the patch looks good to me and I'll be happy to replace my
patch with this one.