* [PATCH] setup_revisions(): take pathspec from command line and --stdin correctly
@ 2011-05-11 21:44 Junio C Hamano
2011-05-11 22:00 ` Thiago Farina
0 siblings, 1 reply; 2+ messages in thread
From: Junio C Hamano @ 2011-05-11 21:44 UTC (permalink / raw)
To: git
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 <gitster@pobox.com>
---
* 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 {
+ 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)
+{
+ while (*av) {
+ ALLOC_GROW(prune->path, prune->nr+1, prune->alloc);
+ 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 \
+ <input >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.5.1.315.ge7efa
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] setup_revisions(): take pathspec from command line and --stdin correctly
2011-05-11 21:44 [PATCH] setup_revisions(): take pathspec from command line and --stdin correctly Junio C Hamano
@ 2011-05-11 22:00 ` Thiago Farina
0 siblings, 0 replies; 2+ messages in thread
From: Thiago Farina @ 2011-05-11 22:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, May 11, 2011 at 6:44 PM, Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com>
> ---
>
> * 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 \
> + <input >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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2011-05-11 22:00 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-05-11 21:44 [PATCH] setup_revisions(): take pathspec from command line and --stdin correctly Junio C Hamano
2011-05-11 22:00 ` Thiago Farina
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 NNTP newsgroup(s).