On Thu, Jan 30, 2020 at 8:10 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> 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.
> - 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.
