On Thu, Sep 05, 2019 at 10:56:12AM -0700, Junio C Hamano wrote:

> Stephan Beyer <s-be...@gmx.net> writes:
> 
> > diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
> > index 7e79b555de..ef0963e2f4 100644
> > --- a/t/helper/test-read-cache.c
> > +++ b/t/helper/test-read-cache.c
> > @@ -4,7 +4,7 @@
> >
> >  int cmd__read_cache(int argc, const char **argv)
> >  {
> > -   int i, cnt = 1, namelen;
> > +   int i, cnt = 1, namelen = 0;
> >     const char *name = NULL;
> >
> >     if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
>               namelen = strlen(name);
> 
> The above is the only assignment to namelen in this function, and
> namelen is used like so:
> 
>               if (name) {
>                       ...
>                       pos = index_name_pos(&the_index, name, namelen);
> 
> So somebody does not realize that skip_prefix() returns true only
> when it touches name.  But skip_prefix() is inline and visible to
> the compiler, and it is quite clear that name is only touched when 
> the function returns non-zero.

I said earlier that I wouldn't mind seeing "namelen = 0" here. But I
think there is a much more direct solution: keeping the assignment and
point of use closer together. That makes it more clear both to the
compiler and to a human when we expect the variable to be valid. In
fact, since it's only used once, we can drop the variable altogther. :)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index 7e79b555de..244977a29b 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -4,11 +4,10 @@
 
 int cmd__read_cache(int argc, const char **argv)
 {
-       int i, cnt = 1, namelen;
+       int i, cnt = 1;
        const char *name = NULL;
 
        if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
-               namelen = strlen(name);
                argc--;
                argv++;
        }
@@ -24,7 +23,7 @@ int cmd__read_cache(int argc, const char **argv)
 
                        refresh_index(&the_index, REFRESH_QUIET,
                                      NULL, NULL, NULL);
-                       pos = index_name_pos(&the_index, name, namelen);
+                       pos = index_name_pos(&the_index, name, strlen(name));
                        if (pos < 0)
                                die("%s not in index", name);
                        printf("%s is%s up to date\n", name,

Reply via email to