git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] clean: remove unnecessary variable
@ 2021-05-06 19:33 Matheus Tavares
  2021-05-06 20:03 ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Matheus Tavares @ 2021-05-06 19:33 UTC (permalink / raw)
  To: git

The variable `matches` used to hold the return of a `dir_path_match()`
call that was removed in 95c11ecc73 ("Fix error-prone fill_directory()
API; make it only return matches", 2020-04-01). Now `matches` will
always hold 0, which is the value it's initialized with; and the
condition `matches != MATCHED_EXACTLY` will always evaluate to true. So
let's remove this unnecessary variable.

Interestingly, it seems that `matches != MATCHED_EXACTLY` was already
unnecessary before 95c11ecc73. That's because `remove_directories` is
always set to 1 when we have pathspecs; So, in the condition
`!remove_directories && matches != MATCHED_EXACTLY`, we would either:

- have pathspecs (or have been given `-d`) and ignore `matches` because
  `remove_directories` is 1; or

- not have pathspecs (nor `-d`) and end up just checking that
  `0 != MATCHED_EXACTLY`, as `matches` would never get reassigned
  after its zero initialization (because there is no pathspec to match).

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/clean.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 995053b791..f6d7e8119c 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -1003,7 +1003,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
-		int matches = 0;
 		struct stat st;
 		const char *rel;
 
@@ -1013,8 +1012,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		if (lstat(ent->name, &st))
 			die_errno("Cannot lstat '%s'", ent->name);
 
-		if (S_ISDIR(st.st_mode) && !remove_directories &&
-		    matches != MATCHED_EXACTLY)
+		if (S_ISDIR(st.st_mode) && !remove_directories)
 			continue;
 
 		rel = relative_path(ent->name, prefix, &buf);
-- 
2.30.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clean: remove unnecessary variable
  2021-05-06 19:33 [PATCH] clean: remove unnecessary variable Matheus Tavares
@ 2021-05-06 20:03 ` Jeff King
  2021-05-06 21:14   ` Matheus Tavares Bernardino
  2021-05-06 21:35   ` Elijah Newren
  0 siblings, 2 replies; 4+ messages in thread
From: Jeff King @ 2021-05-06 20:03 UTC (permalink / raw)
  To: Matheus Tavares; +Cc: Elijah Newren, git

On Thu, May 06, 2021 at 04:33:15PM -0300, Matheus Tavares wrote:

> The variable `matches` used to hold the return of a `dir_path_match()`
> call that was removed in 95c11ecc73 ("Fix error-prone fill_directory()
> API; make it only return matches", 2020-04-01). Now `matches` will
> always hold 0, which is the value it's initialized with; and the
> condition `matches != MATCHED_EXACTLY` will always evaluate to true. So
> let's remove this unnecessary variable.
>
> Interestingly, it seems that `matches != MATCHED_EXACTLY` was already
> unnecessary before 95c11ecc73. That's because `remove_directories` is
> always set to 1 when we have pathspecs; So, in the condition
> `!remove_directories && matches != MATCHED_EXACTLY`, we would either:
> 
> - have pathspecs (or have been given `-d`) and ignore `matches` because
>   `remove_directories` is 1; or
> 
> - not have pathspecs (nor `-d`) and end up just checking that
>   `0 != MATCHED_EXACTLY`, as `matches` would never get reassigned
>   after its zero initialization (because there is no pathspec to match).

Thanks for this digging and the extra analysis. We can see from the
patch that this variable can't possibly be doing anything, but it is
always a comfort to see authors researching the source of the oddity and
explaining what they found.

I'm adding Elijah to the cc as an area expert, just in case he has any
other insight.

> diff --git a/builtin/clean.c b/builtin/clean.c
> index 995053b791..f6d7e8119c 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -1003,7 +1003,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  
>  	for (i = 0; i < dir.nr; i++) {
>  		struct dir_entry *ent = dir.entries[i];
> -		int matches = 0;
>  		struct stat st;
>  		const char *rel;
>  
> @@ -1013,8 +1012,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  		if (lstat(ent->name, &st))
>  			die_errno("Cannot lstat '%s'", ent->name);
>  
> -		if (S_ISDIR(st.st_mode) && !remove_directories &&
> -		    matches != MATCHED_EXACTLY)
> +		if (S_ISDIR(st.st_mode) && !remove_directories)
>  			continue;
>  
>  		rel = relative_path(ent->name, prefix, &buf);

Definitely not necessary, but on a patch like this I'll sometimes
manually specify "-U4" (and I always have diff.interhunkcontext set to
"1") to show the complete code between the declaration and use. It makes
it even more obvious that the result is correct (though obviously
applying and compiling shows it, too). #gitlifehacks

-Peff

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clean: remove unnecessary variable
  2021-05-06 20:03 ` Jeff King
@ 2021-05-06 21:14   ` Matheus Tavares Bernardino
  2021-05-06 21:35   ` Elijah Newren
  1 sibling, 0 replies; 4+ messages in thread
From: Matheus Tavares Bernardino @ 2021-05-06 21:14 UTC (permalink / raw)
  To: Jeff King; +Cc: Elijah Newren, git

On Thu, May 6, 2021 at 5:03 PM Jeff King <peff@peff.net> wrote:
>
> Definitely not necessary, but on a patch like this I'll sometimes
> manually specify "-U4" (and I always have diff.interhunkcontext set to
> "1") to show the complete code between the declaration and use. It makes
> it even more obvious that the result is correct (though obviously
> applying and compiling shows it, too). #gitlifehacks

That's a good idea, thanks for the advice :)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clean: remove unnecessary variable
  2021-05-06 20:03 ` Jeff King
  2021-05-06 21:14   ` Matheus Tavares Bernardino
@ 2021-05-06 21:35   ` Elijah Newren
  1 sibling, 0 replies; 4+ messages in thread
From: Elijah Newren @ 2021-05-06 21:35 UTC (permalink / raw)
  To: Jeff King; +Cc: Matheus Tavares, Git Mailing List

On Thu, May 6, 2021 at 1:03 PM Jeff King <peff@peff.net> wrote:
>
> On Thu, May 06, 2021 at 04:33:15PM -0300, Matheus Tavares wrote:
>
> > The variable `matches` used to hold the return of a `dir_path_match()`
> > call that was removed in 95c11ecc73 ("Fix error-prone fill_directory()
> > API; make it only return matches", 2020-04-01). Now `matches` will
> > always hold 0, which is the value it's initialized with; and the
> > condition `matches != MATCHED_EXACTLY` will always evaluate to true. So
> > let's remove this unnecessary variable.
> >
> > Interestingly, it seems that `matches != MATCHED_EXACTLY` was already
> > unnecessary before 95c11ecc73. That's because `remove_directories` is
> > always set to 1 when we have pathspecs; So, in the condition
> > `!remove_directories && matches != MATCHED_EXACTLY`, we would either:
> >
> > - have pathspecs (or have been given `-d`) and ignore `matches` because
> >   `remove_directories` is 1; or
> >
> > - not have pathspecs (nor `-d`) and end up just checking that
> >   `0 != MATCHED_EXACTLY`, as `matches` would never get reassigned
> >   after its zero initialization (because there is no pathspec to match).
>
> Thanks for this digging and the extra analysis. We can see from the
> patch that this variable can't possibly be doing anything, but it is
> always a comfort to see authors researching the source of the oddity and
> explaining what they found.
>
> I'm adding Elijah to the cc as an area expert, just in case he has any
> other insight.

Thanks for catching this Matheus, and digging in a bit on the
analysis.  This change looks good to me:

Reviewed-by: Elijah Newren <newren@gmail.com>


>
> > diff --git a/builtin/clean.c b/builtin/clean.c
> > index 995053b791..f6d7e8119c 100644
> > --- a/builtin/clean.c
> > +++ b/builtin/clean.c
> > @@ -1003,7 +1003,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> >
> >       for (i = 0; i < dir.nr; i++) {
> >               struct dir_entry *ent = dir.entries[i];
> > -             int matches = 0;
> >               struct stat st;
> >               const char *rel;
> >
> > @@ -1013,8 +1012,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> >               if (lstat(ent->name, &st))
> >                       die_errno("Cannot lstat '%s'", ent->name);
> >
> > -             if (S_ISDIR(st.st_mode) && !remove_directories &&
> > -                 matches != MATCHED_EXACTLY)
> > +             if (S_ISDIR(st.st_mode) && !remove_directories)
> >                       continue;
> >
> >               rel = relative_path(ent->name, prefix, &buf);
>
> Definitely not necessary, but on a patch like this I'll sometimes
> manually specify "-U4" (and I always have diff.interhunkcontext set to
> "1") to show the complete code between the declaration and use. It makes
> it even more obvious that the result is correct (though obviously
> applying and compiling shows it, too). #gitlifehacks
>
> -Peff

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-06 21:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 19:33 [PATCH] clean: remove unnecessary variable Matheus Tavares
2021-05-06 20:03 ` Jeff King
2021-05-06 21:14   ` Matheus Tavares Bernardino
2021-05-06 21:35   ` Elijah Newren

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).