[4/6] dir: move setting of nested_repo next to its actual usage
diff mbox series

Message ID 3b2ec5eaf65c9fe44c4337a4cc2fc3dae6203d54.1580335424.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Avoid multiple recursive calls for same path in read_directory_recursive()
Related show

Commit Message

starlord via GitGitGadget Jan. 29, 2020, 10:03 p.m. UTC
From: Elijah Newren <newren@gmail.com>

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

Comments

Derrick Stolee Jan. 30, 2020, 3:33 p.m. UTC | #1
On 1/29/2020 5:03 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  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@microsoft.com>
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().

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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) {
+		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;
 		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,
 	}
 
 	/* This is the "show_other_directories" case */
-
+show_other_directories:
 	if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
 		return exclude ? path_excluded : path_untracked;
Elijah Newren Jan. 30, 2020, 3:45 p.m. UTC | #2
On Thu, Jan 30, 2020 at 7:33 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 1/29/2020 5:03 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  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@microsoft.com>
> 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.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  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) {
> +               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;
>                 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:
>         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.
Derrick Stolee Jan. 30, 2020, 4 p.m. UTC | #3
On 1/30/2020 10:45 AM, Elijah Newren wrote:
> On Thu, Jan 30, 2020 at 7:33 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 1/29/2020 5:03 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>>  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@microsoft.com>
>> 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@microsoft.com>
>> ---
>>  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
Derrick Stolee Jan. 30, 2020, 4:10 p.m. UTC | #4
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;
 
-	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)) {
Elijah Newren Jan. 30, 2020, 4:20 p.m. UTC | #5
On Thu, Jan 30, 2020 at 8:10 AM Derrick Stolee <stolee@gmail.com> 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@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.

> -       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.
Derrick Stolee Jan. 30, 2020, 6:17 p.m. UTC | #6
On 1/30/2020 11:20 AM, Elijah Newren wrote:
> On Thu, Jan 30, 2020 at 8:10 AM Derrick Stolee <stolee@gmail.com> wrote:
>> 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.

I'm happy if you squash this into the commit. Thanks!

Patch
diff mbox series

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;
 		if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
 		    !(dir->flags & DIR_NO_GITLINKS)) {
 			struct strbuf sb = STRBUF_INIT;