On 1/30/2020 10:45 AM, Elijah Newren wrote:
> On Thu, Jan 30, 2020 at 7:33 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>>
>> On 1/29/2020 5:03 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@xxxxxxxxx>
>>>
>>> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
>>> ---
>>> 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 <dstolee@xxxxxxxxxxxxx>
>> 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?
Ah, I guess I didn't realize that exist_status didn't have a fourth
mode. I was assuming that normally the switch would not hit any of
the case statements was the way you would _assume_ to hit the block
after the switch.
So yes, my statement is incorrect, but the intention is correct:
the flow of this method is very confusing.
> 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.
>
>>
>> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>> ---
>> dir.c | 17 ++++++++---------
>> 1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index b460211e61..e48812efe6 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1659,17 +1659,16 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>> const char *dirname, int len, int baselen, int exclude,
>> 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;
>>
>> - case index_nonexistent:
>> + if (status == index_nonexistent) {
Since exist_status only has three options, this "if" is redundant.
>> + int nested_repo = 0;
>> if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
>> !(dir->flags & DIR_NO_GITLINKS)) {
>> struct strbuf sb = STRBUF_INIT;
>> @@ -1682,7 +1681,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>> (exclude ? path_excluded : path_untracked));
>>
>> if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
>> - break;
>> + goto show_other_directories;
It would be better to nest the rest of this block in an
if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
>> if (exclude &&
>> (dir->flags & DIR_SHOW_IGNORED_TOO) &&
>> (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING)) {
>> @@ -1711,7 +1710,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>> }
>
> 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:
...allowing us to drop this.
>> 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.
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.
Thanks,
-Stolee
