Re: [PATCH] clean: remove unnecessary variable

[Elijah Newren <newren@gmail.com>]

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