From: Thiago Farina Date: Wed, 11 May 2011 22:00:36 GMT Subject: Re: [PATCH] setup_revisions(): take pathspec from command line and --stdin correctly Message-ID: In-Reply-To: <7vd3jogbe0.fsf@alter.siamese.dyndns.org> On Wed, May 11, 2011 at 6:44 PM, Junio C Hamano wrote: > When the command line has "--" disambiguator, we take the remainder of > argv[] as "prune_data", but when --stdin is given at the same time, we > need to append to the existing prune_data and end up attempting to > realloc(3) it, which would not work and can lead to a segfault or worse. > > Fix it by consistently using append_prune_data() throughout the input > processing.  Also avoid counting the number of existing paths in the > function over and over again. > > Signed-off-by: Junio C Hamano > --- > >  * This is relative to 1.6.6; the bug originates back to 60da8b1 (Make >   --stdin option to "log" family read also pathspecs, 2009-11-20). > >  revision.c                |   80 ++++++++++++++++----------------------------- >  t/t6017-rev-list-stdin.sh |   17 +++++++++ >  2 files changed, 45 insertions(+), 52 deletions(-) > > diff --git a/revision.c b/revision.c > index c92ffc2..58b5651 100644 > --- a/revision.c > +++ b/revision.c > @@ -956,35 +956,34 @@ int handle_revision_arg(const char *arg, struct rev_info *revs, >        return 0; >  } > > -static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb, const char ***prune_data) > -{ > -       const char **prune = *prune_data; > -       int prune_nr; > -       int prune_alloc; > +struct cmdline_pathspec { Maybe prune_data_pathspec? > +       int alloc; > +       int nr; > +       const char **path; > +}; > > -       /* count existing ones */ > -       if (!prune) > -               prune_nr = 0; > -       else > -               for (prune_nr = 0; prune[prune_nr]; prune_nr++) > -                       ; > -       prune_alloc = prune_nr; /* not really, but we do not know */ > +static void append_prune_data(struct cmdline_pathspec *prune, const char **av) av is the same thing as argv? If so, is it worth naming it argv? I know the old was av, so maybe not though. > +{ > +       while (*av) { > +               ALLOC_GROW(prune->path, prune->nr+1, prune->alloc); I'd add spaces between prune->nr + 1. But I think is fine as is too. > +               prune->path[prune->nr++] = *(av++); > +       } > +} > > +static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb, > +                                    struct cmdline_pathspec *prune) > +{ >        while (strbuf_getwholeline(sb, stdin, '\n') != EOF) { >                int len = sb->len; >                if (len && sb->buf[len - 1] == '\n') >                        sb->buf[--len] = '\0'; > -               ALLOC_GROW(prune, prune_nr+1, prune_alloc); > -               prune[prune_nr++] = xstrdup(sb->buf); > +               ALLOC_GROW(prune->path, prune->nr+1, prune->alloc); > +               prune->path[prune->nr++] = xstrdup(sb->buf); >        } > -       if (prune) { > -               ALLOC_GROW(prune, prune_nr+1, prune_alloc); > -               prune[prune_nr] = NULL; > -       } > -       *prune_data = prune; >  } > > -static void read_revisions_from_stdin(struct rev_info *revs, const char ***prune) > +static void read_revisions_from_stdin(struct rev_info *revs, > +                                     struct cmdline_pathspec *prune) >  { >        struct strbuf sb; >        int seen_dashdash = 0; > @@ -1279,34 +1278,6 @@ static int for_each_good_bisect_ref(each_ref_fn fn, void *cb_data) >        return for_each_ref_in("refs/bisect/good", fn, cb_data); >  } > > -static void append_prune_data(const char ***prune_data, const char **av) > -{ > -       const char **prune = *prune_data; > -       int prune_nr; > -       int prune_alloc; > - > -       if (!prune) { > -               *prune_data = av; > -               return; > -       } > - > -       /* count existing ones */ > -       for (prune_nr = 0; prune[prune_nr]; prune_nr++) > -               ; > -       prune_alloc = prune_nr; /* not really, but we do not know */ > - > -       while (*av) { > -               ALLOC_GROW(prune, prune_nr+1, prune_alloc); > -               prune[prune_nr++] = *av; > -               av++; > -       } > -       if (prune) { > -               ALLOC_GROW(prune, prune_nr+1, prune_alloc); > -               prune[prune_nr] = NULL; > -       } > -       *prune_data = prune; > -} > - >  /* >  * Parse revision information, filling in the "rev_info" structure, >  * and removing the used arguments from the argument list. > @@ -1317,7 +1288,9 @@ static void append_prune_data(const char ***prune_data, const char **av) >  int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def) >  { >        int i, flags, left, seen_dashdash, read_from_stdin; > -       const char **prune_data = NULL; > +       struct cmdline_pathspec prune_data; > + > +       memset(&prune_data, 0, sizeof(prune_data)); > >        /* First, search for "--" */ >        seen_dashdash = 0; > @@ -1328,7 +1301,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch >                argv[i] = NULL; >                argc = i; >                if (argv[i + 1]) > -                       prune_data = argv + i + 1; > +                       append_prune_data(&prune_data, argv + i + 1); >                seen_dashdash = 1; >                break; >        } > @@ -1420,8 +1393,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch >                } >        } > > -       if (prune_data) > -               revs->prune_data = get_pathspec(revs->prefix, prune_data); > +       if (prune_data.nr) { > +               ALLOC_GROW(prune_data.path, prune_data.nr+1, prune_data.alloc); > +               prune_data.path[prune_data.nr++] = NULL; > +               revs->prune_data = get_pathspec(revs->prefix, prune_data.path); > +       } > >        if (revs->def == NULL) >                revs->def = def; > diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh > index f1c32db..667b375 100755 > --- a/t/t6017-rev-list-stdin.sh > +++ b/t/t6017-rev-list-stdin.sh > @@ -58,4 +58,21 @@ check side-3 ^side-4 -- file-3 >  check side-3 ^side-2 >  check side-3 ^side-2 -- file-1 > > +test_expect_success 'not only --stdin' ' > +       cat >expect <<-EOF && > +       7 > + > +       file-1 > +       file-2 > +       EOF > +       cat >input <<-EOF && > +       ^master^ > +       -- > +       file-2 > +       EOF > +       git log --pretty=tformat:%s --name-only --stdin master -- file-1 \ > +               actual && > +       test_cmp expect actual > +' > + >  test_done > -- > 1.7.5.1.315.ge7efa > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at  http://vger.kernel.org/majordomo-info.html >