Re: [PATCH 4/6] dir: move setting of nested_repo next to its actual usage
From: Elijah Newren <hidden>
Date: 2020-01-30 16:20:47
On Thu, Jan 30, 2020 at 8:10 AM Derrick Stolee [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On 1/30/2020 11:00 AM, Derrick Stolee wrote:quoted
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 <redacted> 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 [off-list ref] Signed-off-by: Derrick Stolee <redacted> --- 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.1Otherwise, I'm quite happy with these changes.