{"thread":{"id":"27328","subject":"[PATCH] setup_revisions(): take pathspec from command line and --stdin correctly","startedAt":"2011-05-11T21:44:55Z","lastAt":"2011-05-11T22:00:36Z","messageCount":2,"participants":["Junio C Hamano","Thiago Farina"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"167692","messageId":"7vd3jogbe0.fsf@alter.siamese.dyndns.org","threadId":"27328","inReplyTo":null,"subject":"[PATCH] setup_revisions(): take pathspec from command line and --stdin correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-11T21:44:55Z","receivedAt":"2011-05-11T21:44:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When the command line has \"--\" disambiguator, we take the remainder of\nargv[] as \"prune_data\", but when --stdin is given at the same time, we\nneed to append to the existing prune_data and end up attempting to\nrealloc(3) it, which would not work and can lead to a segfault or worse.\n\nFix it by consistently using append_prune_data() throughout the input\nprocessing.  Also avoid counting the number of existing paths in the\nfunction over and over again.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is relative to 1.6.6; the bug originates back to 60da8b1 (Make\n   --stdin option to \"log\" family read also pathspecs, 2009-11-20).\n\n revision.c                |   80 ++++++++++++++++-----------------------------\n t/t6017-rev-list-stdin.sh |   17 +++++++++\n 2 files changed, 45 insertions(+), 52 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex c92ffc2..58b5651 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -956,35 +956,34 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \treturn 0;\n }\n \n-static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb, const char ***prune_data)\n-{\n-\tconst char **prune = *prune_data;\n-\tint prune_nr;\n-\tint prune_alloc;\n+struct cmdline_pathspec {\n+\tint alloc;\n+\tint nr;\n+\tconst char **path;\n+};\n \n-\t/* count existing ones */\n-\tif (!prune)\n-\t\tprune_nr = 0;\n-\telse\n-\t\tfor (prune_nr = 0; prune[prune_nr]; prune_nr++)\n-\t\t\t;\n-\tprune_alloc = prune_nr; /* not really, but we do not know */\n+static void append_prune_data(struct cmdline_pathspec *prune, const char **av)\n+{\n+\twhile (*av) {\n+\t\tALLOC_GROW(prune->path, prune->nr+1, prune->alloc);\n+\t\tprune->path[prune->nr++] = *(av++);\n+\t}\n+}\n \n+static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,\n+\t\t\t\t     struct cmdline_pathspec *prune)\n+{\n \twhile (strbuf_getwholeline(sb, stdin, '\\n') != EOF) {\n \t\tint len = sb->len;\n \t\tif (len && sb->buf[len - 1] == '\\n')\n \t\t\tsb->buf[--len] = '\\0';\n-\t\tALLOC_GROW(prune, prune_nr+1, prune_alloc);\n-\t\tprune[prune_nr++] = xstrdup(sb->buf);\n+\t\tALLOC_GROW(prune->path, prune->nr+1, prune->alloc);\n+\t\tprune->path[prune->nr++] = xstrdup(sb->buf);\n \t}\n-\tif (prune) {\n-\t\tALLOC_GROW(prune, prune_nr+1, prune_alloc);\n-\t\tprune[prune_nr] = NULL;\n-\t}\n-\t*prune_data = prune;\n }\n \n-static void read_revisions_from_stdin(struct rev_info *revs, const char ***prune)\n+static void read_revisions_from_stdin(struct rev_info *revs,\n+\t\t\t\t      struct cmdline_pathspec *prune)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n@@ -1279,34 +1278,6 @@ static int for_each_good_bisect_ref(each_ref_fn fn, void *cb_data)\n \treturn for_each_ref_in(\"refs/bisect/good\", fn, cb_data);\n }\n \n-static void append_prune_data(const char ***prune_data, const char **av)\n-{\n-\tconst char **prune = *prune_data;\n-\tint prune_nr;\n-\tint prune_alloc;\n-\n-\tif (!prune) {\n-\t\t*prune_data = av;\n-\t\treturn;\n-\t}\n-\n-\t/* count existing ones */\n-\tfor (prune_nr = 0; prune[prune_nr]; prune_nr++)\n-\t\t;\n-\tprune_alloc = prune_nr; /* not really, but we do not know */\n-\n-\twhile (*av) {\n-\t\tALLOC_GROW(prune, prune_nr+1, prune_alloc);\n-\t\tprune[prune_nr++] = *av;\n-\t\tav++;\n-\t}\n-\tif (prune) {\n-\t\tALLOC_GROW(prune, prune_nr+1, prune_alloc);\n-\t\tprune[prune_nr] = NULL;\n-\t}\n-\t*prune_data = prune;\n-}\n-\n /*\n  * Parse revision information, filling in the \"rev_info\" structure,\n  * and removing the used arguments from the argument list.\n@@ -1317,7 +1288,9 @@ static void append_prune_data(const char ***prune_data, const char **av)\n int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)\n {\n \tint i, flags, left, seen_dashdash, read_from_stdin;\n-\tconst char **prune_data = NULL;\n+\tstruct cmdline_pathspec prune_data;\n+\n+\tmemset(&prune_data, 0, sizeof(prune_data));\n \n \t/* First, search for \"--\" */\n \tseen_dashdash = 0;\n@@ -1328,7 +1301,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\targv[i] = NULL;\n \t\targc = i;\n \t\tif (argv[i + 1])\n-\t\t\tprune_data = argv + i + 1;\n+\t\t\tappend_prune_data(&prune_data, argv + i + 1);\n \t\tseen_dashdash = 1;\n \t\tbreak;\n \t}\n@@ -1420,8 +1393,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\t}\n \t}\n \n-\tif (prune_data)\n-\t\trevs->prune_data = get_pathspec(revs->prefix, prune_data);\n+\tif (prune_data.nr) {\n+\t\tALLOC_GROW(prune_data.path, prune_data.nr+1, prune_data.alloc);\n+\t\tprune_data.path[prune_data.nr++] = NULL;\n+\t\trevs->prune_data = get_pathspec(revs->prefix, prune_data.path);\n+\t}\n \n \tif (revs->def == NULL)\n \t\trevs->def = def;\ndiff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh\nindex f1c32db..667b375 100755\n--- a/t/t6017-rev-list-stdin.sh\n+++ b/t/t6017-rev-list-stdin.sh\n@@ -58,4 +58,21 @@ check side-3 ^side-4 -- file-3\n check side-3 ^side-2\n check side-3 ^side-2 -- file-1\n \n+test_expect_success 'not only --stdin' '\n+\tcat >expect <<-EOF &&\n+\t7\n+\n+\tfile-1\n+\tfile-2\n+\tEOF\n+\tcat >input <<-EOF &&\n+\t^master^\n+\t--\n+\tfile-2\n+\tEOF\n+\tgit log --pretty=tformat:%s --name-only --stdin master -- file-1 \\\n+\t\t<input >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.5.1.315.ge7efa\n"},{"id":"167694","messageId":"BANLkTin401CWZntHowyCw=Ua0+SNAZ7m7A@mail.gmail.com","threadId":"27328","inReplyTo":"7vd3jogbe0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup_revisions(): take pathspec from command line and --stdin correctly","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2011-05-11T22:00:36Z","receivedAt":"2011-05-11T22:00:36Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Wed, May 11, 2011 at 6:44 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> When the command line has \"--\" disambiguator, we take the remainder of\n> argv[] as \"prune_data\", but when --stdin is given at the same time, we\n> need to append to the existing prune_data and end up attempting to\n> realloc(3) it, which would not work and can lead to a segfault or worse.\n>\n> Fix it by consistently using append_prune_data() throughout the input\n> processing.  Also avoid counting the number of existing paths in the\n> function over and over again.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * This is relative to 1.6.6; the bug originates back to 60da8b1 (Make\n>   --stdin option to \"log\" family read also pathspecs, 2009-11-20).\n>\n>  revision.c                |   80 ++++++++++++++++-----------------------------\n>  t/t6017-rev-list-stdin.sh |   17 +++++++++\n>  2 files changed, 45 insertions(+), 52 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index c92ffc2..58b5651 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -956,35 +956,34 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n>        return 0;\n>  }\n>\n> -static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb, const char ***prune_data)\n> -{\n> -       const char **prune = *prune_data;\n> -       int prune_nr;\n> -       int prune_alloc;\n> +struct cmdline_pathspec {\nMaybe prune_data_pathspec?\n\n> +       int alloc;\n> +       int nr;\n> +       const char **path;\n> +};\n>\n> -       /* count existing ones */\n> -       if (!prune)\n> -               prune_nr = 0;\n> -       else\n> -               for (prune_nr = 0; prune[prune_nr]; prune_nr++)\n> -                       ;\n> -       prune_alloc = prune_nr; /* not really, but we do not know */\n> +static void append_prune_data(struct cmdline_pathspec *prune, const char **av)\nav is the same thing as argv? If so, is it worth naming it argv? I\nknow the old was av, so maybe not though.\n\n> +{\n> +       while (*av) {\n> +               ALLOC_GROW(prune->path, prune->nr+1, prune->alloc);\nI'd add spaces between prune->nr + 1. But I think is fine as is too.\n\n> +               prune->path[prune->nr++] = *(av++);\n> +       }\n> +}\n>\n> +static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,\n> +                                    struct cmdline_pathspec *prune)\n> +{\n>        while (strbuf_getwholeline(sb, stdin, '\\n') != EOF) {\n>                int len = sb->len;\n>                if (len && sb->buf[len - 1] == '\\n')\n>                        sb->buf[--len] = '\\0';\n> -               ALLOC_GROW(prune, prune_nr+1, prune_alloc);\n> -               prune[prune_nr++] = xstrdup(sb->buf);\n> +               ALLOC_GROW(prune->path, prune->nr+1, prune->alloc);\n> +               prune->path[prune->nr++] = xstrdup(sb->buf);\n>        }\n> -       if (prune) {\n> -               ALLOC_GROW(prune, prune_nr+1, prune_alloc);\n> -               prune[prune_nr] = NULL;\n> -       }\n> -       *prune_data = prune;\n>  }\n>\n> -static void read_revisions_from_stdin(struct rev_info *revs, const char ***prune)\n> +static void read_revisions_from_stdin(struct rev_info *revs,\n> +                                     struct cmdline_pathspec *prune)\n>  {\n>        struct strbuf sb;\n>        int seen_dashdash = 0;\n> @@ -1279,34 +1278,6 @@ static int for_each_good_bisect_ref(each_ref_fn fn, void *cb_data)\n>        return for_each_ref_in(\"refs/bisect/good\", fn, cb_data);\n>  }\n>\n> -static void append_prune_data(const char ***prune_data, const char **av)\n> -{\n> -       const char **prune = *prune_data;\n> -       int prune_nr;\n> -       int prune_alloc;\n> -\n> -       if (!prune) {\n> -               *prune_data = av;\n> -               return;\n> -       }\n> -\n> -       /* count existing ones */\n> -       for (prune_nr = 0; prune[prune_nr]; prune_nr++)\n> -               ;\n> -       prune_alloc = prune_nr; /* not really, but we do not know */\n> -\n> -       while (*av) {\n> -               ALLOC_GROW(prune, prune_nr+1, prune_alloc);\n> -               prune[prune_nr++] = *av;\n> -               av++;\n> -       }\n> -       if (prune) {\n> -               ALLOC_GROW(prune, prune_nr+1, prune_alloc);\n> -               prune[prune_nr] = NULL;\n> -       }\n> -       *prune_data = prune;\n> -}\n> -\n>  /*\n>  * Parse revision information, filling in the \"rev_info\" structure,\n>  * and removing the used arguments from the argument list.\n> @@ -1317,7 +1288,9 @@ static void append_prune_data(const char ***prune_data, const char **av)\n>  int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)\n>  {\n>        int i, flags, left, seen_dashdash, read_from_stdin;\n> -       const char **prune_data = NULL;\n> +       struct cmdline_pathspec prune_data;\n> +\n> +       memset(&prune_data, 0, sizeof(prune_data));\n>\n>        /* First, search for \"--\" */\n>        seen_dashdash = 0;\n> @@ -1328,7 +1301,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n>                argv[i] = NULL;\n>                argc = i;\n>                if (argv[i + 1])\n> -                       prune_data = argv + i + 1;\n> +                       append_prune_data(&prune_data, argv + i + 1);\n>                seen_dashdash = 1;\n>                break;\n>        }\n> @@ -1420,8 +1393,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n>                }\n>        }\n>\n> -       if (prune_data)\n> -               revs->prune_data = get_pathspec(revs->prefix, prune_data);\n> +       if (prune_data.nr) {\n> +               ALLOC_GROW(prune_data.path, prune_data.nr+1, prune_data.alloc);\n> +               prune_data.path[prune_data.nr++] = NULL;\n> +               revs->prune_data = get_pathspec(revs->prefix, prune_data.path);\n> +       }\n>\n>        if (revs->def == NULL)\n>                revs->def = def;\n> diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh\n> index f1c32db..667b375 100755\n> --- a/t/t6017-rev-list-stdin.sh\n> +++ b/t/t6017-rev-list-stdin.sh\n> @@ -58,4 +58,21 @@ check side-3 ^side-4 -- file-3\n>  check side-3 ^side-2\n>  check side-3 ^side-2 -- file-1\n>\n> +test_expect_success 'not only --stdin' '\n> +       cat >expect <<-EOF &&\n> +       7\n> +\n> +       file-1\n> +       file-2\n> +       EOF\n> +       cat >input <<-EOF &&\n> +       ^master^\n> +       --\n> +       file-2\n> +       EOF\n> +       git log --pretty=tformat:%s --name-only --stdin master -- file-1 \\\n> +               <input >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 1.7.5.1.315.ge7efa\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"}]}