Re: [PATCH 4/6] dir: move setting of nested_repo next to its actual usage

From: Elijah Newren <newren@gmail.com>
Date: Jan 30, 2020, 16:20 UTC
Message-ID: <CABPp-BGe7ae+pf+HUdLE-xDoXEPDJ=5vWH4cnU9N6pZM2o1Q-g@mail.gmail.com>
In-Reply-To: <d67eb395-3b6d-9558-0288-cd3878290c9e@gmail.com>

On Thu, Jan 30, 2020 at 8:10 AM Derrick Stolee <stolee@gmail.com> wrote:

Show 57 quoted lines

>
> On 1/30/2020 11:00 AM, Derrick Stolee wrote:
> >
> > Let me send a v2 of this patch now that you've pointed out my error. It
> > is worth making this method clearer before you expand substantially on
> > this final case.
>
> Here we are:
>
> -->8--
>
> From 3fb4fdda25affe9fe6b3e91050e8ad105bcb6fe0 Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <dstolee@microsoft.com>
> Date: Thu, 30 Jan 2020 15:28:39 +0000
> Subject: [PATCH v2] dir: refactor treat_directory to clarify control flow
>
> The logic in treat_directory() is handled by a multi-case
> switch statement, but this switch is very asymmetrical, as
> the first two cases are simple but the third is more
> complicated than the rest of the method. In fact, the third
> case includes a "break" statement that leads to the block
> of code outside the switch statement. That is the only way
> to reach that block, as the switch handles all possible
> values from directory_exists_in_index();
>
> Extract the switch statement into a series of "if" statements.
> This simplifies the trivial cases, while clarifying how to
> reach the "show_other_directories" case. This is particularly
> important as the "show_other_directories" case will expand
> in a later change.
>
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  dir.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index b460211e61..0989558ae6 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1660,29 +1660,26 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>         const struct pathspec *pathspec)
>  {
>         int nested_repo = 0;
> -
>         /* The "len-1" is to strip the final '/' */
> -       switch (directory_exists_in_index(istate, dirname, len-1)) {
> -       case index_directory:
> -               return path_recurse;
> +       enum exist_status status = directory_exists_in_index(istate, dirname, len-1);
>
> -       case index_gitdir:
> +       if (status == index_directory)
> +               return path_recurse;
> +       if (status == index_gitdir)
>                 return path_none;

I think right here we should add:

        if (status != index_nonexistent):
                BUG("Unhandled value for directory_exists_in_index:
%d\n", status);

for future-proofing, since both you and I had to look up what possibilities existed as a return status from directory_exists_in_index(), and I'd rather a large warning was thrown if someone ever adds a fourth option to that function rather than assume treat_directory() is fine and only needs to special case two choices.

Or we could add an assert or a code comment, just so long as we document to future readers that the remainder of the code is assuming status==index_nonexistent.

Show 30 quoted lines

> -       case index_nonexistent:
> -               if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
> -                   !(dir->flags & DIR_NO_GITLINKS)) {
> -                       struct strbuf sb = STRBUF_INIT;
> -                       strbuf_addstr(&sb, dirname);
> -                       nested_repo = is_nonbare_repository_dir(&sb);
> -                       strbuf_release(&sb);
> -               }
> -               if (nested_repo)
> -                       return ((dir->flags & DIR_SKIP_NESTED_GIT) ? path_none :
> -                               (exclude ? path_excluded : path_untracked));
> +       if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
> +               !(dir->flags & DIR_NO_GITLINKS)) {
> +               struct strbuf sb = STRBUF_INIT;
> +               strbuf_addstr(&sb, dirname);
> +               nested_repo = is_nonbare_repository_dir(&sb);
> +               strbuf_release(&sb);
> +       }
> +       if (nested_repo)
> +               return ((dir->flags & DIR_SKIP_NESTED_GIT) ? path_none :
> +                       (exclude ? path_excluded : path_untracked));
>
> -               if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
> -                       break;
> +       if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {
>                 if (exclude &&
>                         (dir->flags & DIR_SHOW_IGNORED_TOO) &&
>                         (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING)) {
> --
> 2.25.0.vfs.1.1

Otherwise, I'm quite happy with these changes.