Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
On 6/8/2018 4:07 PM, René Scharfe wrote: Am 07.06.2018 um 16:12 schrieb g...@jeffhostetler.com: From: Jeff Hostetler [...] + if (jw->first_stack.buf[jw->first_stack.len - 1] == '1') + jw->first_stack.buf[jw->first_stack.len - 1] = '0'; + else + strbuf_addch(>json, ','); You only need a binary flag to indicate if a comma is needed, not a stack. We need a comma at the current level if and only if we already wrote a child item. If we finish a level then we do need a comma at the previous level because we just wrote a sub-object or sub-array there. And that should cover all cases. Am I missing something? I get a sense of déjà vu, by the way. :) [...] Yes, your way is simpler. I'll update and re-roll. (And yes, we did discuss this earlier. I found a problem with my first version where it wouldn't handle empty sub-items, but you found the missing piece.) Thanks Jeff
Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
On 6/8/2018 2:05 AM, René Scharfe wrote: Am 07.06.2018 um 16:12 schrieb g...@jeffhostetler.com: From: Jeff Hostetler Add a series of jw_ routines and "struct json_writer" structure to compose [...] TEST_PROGRAMS_NEED_X += test-index-version +TEST_PROGRAMS_NEED_X += test-json-writer> TEST_PROGRAMS_NEED_X += test-lazy-init-name-hash TEST_PROGRAMS_NEED_X += test-line-buffer TEST_PROGRAMS_NEED_X += test-match-trees This doesn't apply cleanly on master, because most test helpers have been combined into a single binary to reduce their disk footprint and link times. See efd71f8913 (t/helper: add an empty test-tool program) for the overall rationale. test-json-writer could become a built-in as well, I think. You can see e.g. in c932a5ff28 (t/helper: merge test-string-list into test-tool) what needs to be done to convert a helper. René You're right, the test helper framework changed since I started this patch series. I was trying to keep the same parent commit as my V1 to make it easier to compare, but that's not working out so well. I'll move it forward to the current master and fix it up. Thanks, Jeff
Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
On 6/8/2018 4:32 PM, René Scharfe wrote: Am 07.06.2018 um 16:12 schrieb g...@jeffhostetler.com: Makefile| 2 + json-writer.c | 419 json-writer.h | 113 + t/helper/test-json-writer.c | 572 t/t0019-json-writer.sh | 236 ++ 5 files changed, 1342 insertions(+) create mode 100644 json-writer.c create mode 100644 json-writer.h create mode 100644 t/helper/test-json-writer.c create mode 100755 t/t0019-json-writer.sh $ git grep 'static inline' '*json*' json-writer.c:static inline void indent_pretty(struct json_writer *jw) json-writer.c:static inline void begin(struct json_writer *jw, char ch_open, int pretty) json-writer.c:static inline void assert_in_object(const struct json_writer *jw, const char *key) json-writer.c:static inline void assert_in_array(const struct json_writer *jw) json-writer.c:static inline void maybe_add_comma(struct json_writer *jw) json-writer.c:static inline void fmt_double(struct json_writer *jw, int precision, json-writer.c:static inline void object_common(struct json_writer *jw, const char *key) json-writer.c:static inline void array_common(struct json_writer *jw) json-writer.c:static inline void assert_is_terminated(const struct json_writer *jw) t/helper/test-json-writer.c:static inline int scripted(int argc, const char **argv) t/helper/test-json-writer.c:static inline int my_usage(void) Do you need all those inline keywords? I'd rather leave the decision about inlining to the compiler and (via optimization flags) the user as much as possible. Not a biggie, but the high frequency of that word made me blink.. René I was just trying to match the patterns I found elsewhere in the code. $ grep "static inline" *.c builtin/*.c | wc -l 111 But yeah, it's no big deal. I can remove them. Jeff
Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
Am 07.06.2018 um 16:12 schrieb g...@jeffhostetler.com: > Makefile| 2 + > json-writer.c | 419 > json-writer.h | 113 + > t/helper/test-json-writer.c | 572 > > t/t0019-json-writer.sh | 236 ++ > 5 files changed, 1342 insertions(+) > create mode 100644 json-writer.c > create mode 100644 json-writer.h > create mode 100644 t/helper/test-json-writer.c > create mode 100755 t/t0019-json-writer.sh $ git grep 'static inline' '*json*' json-writer.c:static inline void indent_pretty(struct json_writer *jw) json-writer.c:static inline void begin(struct json_writer *jw, char ch_open, int pretty) json-writer.c:static inline void assert_in_object(const struct json_writer *jw, const char *key) json-writer.c:static inline void assert_in_array(const struct json_writer *jw) json-writer.c:static inline void maybe_add_comma(struct json_writer *jw) json-writer.c:static inline void fmt_double(struct json_writer *jw, int precision, json-writer.c:static inline void object_common(struct json_writer *jw, const char *key) json-writer.c:static inline void array_common(struct json_writer *jw) json-writer.c:static inline void assert_is_terminated(const struct json_writer *jw) t/helper/test-json-writer.c:static inline int scripted(int argc, const char **argv) t/helper/test-json-writer.c:static inline int my_usage(void) Do you need all those inline keywords? I'd rather leave the decision about inlining to the compiler and (via optimization flags) the user as much as possible. Not a biggie, but the high frequency of that word made me blink.. René
Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
On 6/7/2018 1:24 PM, Eric Sunshine wrote:
On Thu, Jun 7, 2018 at 10:12 AM, wrote:
Add a series of jw_ routines and "struct json_writer" structure to compose
JSON data. The resulting string data can then be output by commands wanting
to support a JSON output format.
[...]
Signed-off-by: Jeff Hostetler
---
diff --git a/t/t0019-json-writer.sh b/t/t0019-json-writer.sh
@@ -0,0 +1,236 @@
+test_expect_success 'simple object' '
+ cat >expect <<-\EOF &&
+ {"a":"abc","b":42,"c":3.14,"d":true,"e":false,"f":null}
+ EOF
+ test-json-writer >actual \
+ @object \
+ @object-string a abc \
+ @object-int b 42 \
+ @object-double c 2 3.140 \
+ @object-true d \
+ @object-false e \
+ @object-null f \
+ @end &&
+ test_cmp expect actual
+'
To make it easier on people writing these tests, it might be nice for
this to be less noisy by getting rid of "@" and "\". To get rid of
"\", the test program could grab its script commands from stdin (one
instruction per line) rather than from argv[]. For instance:
test-json-writer >actual <<-\EOF &&
object
object-string a abc
...
end
EOF
Not a big deal, and certainly not worth a re-roll.
I hadn't thought about doing it that way. Might be a little easier
to use. Let me take a look and see if it would be much work to switch.
Thanks
Jeff
Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
Am 07.06.2018 um 16:12 schrieb g...@jeffhostetler.com:
> From: Jeff Hostetler
> +/*
> + * Add comma if we have already seen a member at this level.
> + */
> +static inline void maybe_add_comma(struct json_writer *jw)
> +{
> + if (!jw->open_stack.len)
> + return;
This is impossible. maybe_add_comma() is only called directly after
assert_in_object() and assert_in_object(), which abort if open_stack
is empty.
> + if (jw->first_stack.buf[jw->first_stack.len - 1] == '1')
> + jw->first_stack.buf[jw->first_stack.len - 1] = '0';
> + else
> + strbuf_addch(>json, ',');
You only need a binary flag to indicate if a comma is needed, not a
stack. We need a comma at the current level if and only if we already
wrote a child item. If we finish a level then we do need a comma at the
previous level because we just wrote a sub-object or sub-array there.
And that should cover all cases. Am I missing something?
I get a sense of déjà vu, by the way. :)
Here's what I mean:
---
json-writer.c | 17 ++---
json-writer.h | 13 +
2 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/json-writer.c b/json-writer.c
index f35ce1912a..14a4e89188 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -5,7 +5,7 @@ void jw_init(struct json_writer *jw)
{
strbuf_reset(>json);
strbuf_reset(>open_stack);
- strbuf_reset(>first_stack);
+ jw->need_comma = 0;
jw->pretty = 0;
}
@@ -13,7 +13,6 @@ void jw_release(struct json_writer *jw)
{
strbuf_release(>json);
strbuf_release(>open_stack);
- strbuf_release(>first_stack);
}
/*
@@ -69,7 +68,7 @@ static inline void begin(struct json_writer *jw, char
ch_open, int pretty)
strbuf_addch(>json, ch_open);
strbuf_addch(>open_stack, ch_open);
- strbuf_addch(>first_stack, '1');
+ jw->need_comma = 0;
}
/*
@@ -99,12 +98,10 @@ static inline void assert_in_array(const struct json_writer
*jw)
*/
static inline void maybe_add_comma(struct json_writer *jw)
{
- if (!jw->open_stack.len)
- return;
- if (jw->first_stack.buf[jw->first_stack.len - 1] == '1')
- jw->first_stack.buf[jw->first_stack.len - 1] = '0';
- else
+ if (jw->need_comma)
strbuf_addch(>json, ',');
+ else
+ jw->need_comma = 1;
}
static inline void fmt_double(struct json_writer *jw, int precision,
@@ -397,8 +394,6 @@ void jw_end(struct json_writer *jw)
char ch_open;
int len;
- if (jw->open_stack.len != jw->first_stack.len)
- BUG("jwon-writer: open_ and first_ stacks out of sync");
if (!jw->open_stack.len)
BUG("json-writer: too many jw_end(): '%s'", jw->json.buf);
@@ -406,7 +401,7 @@ void jw_end(struct json_writer *jw)
ch_open = jw->open_stack.buf[len];
strbuf_setlen(>open_stack, len);
- strbuf_setlen(>first_stack, len);
+ jw->need_comma = 1;
if (jw->pretty)
strbuf_addch(>json, '\n');
diff --git a/json-writer.h b/json-writer.h
index af9c2612f8..c437462ba8 100644
--- a/json-writer.h
+++ b/json-writer.h
@@ -59,18 +59,15 @@ struct json_writer
struct strbuf open_stack;
/*
-* Another simple stack of the currently open array and object
-* forms. This is used in parallel to "open_stack" (same length).
-* It contains a string of '1' and '0' where '1' indicates that
-* the next child-element seen will be the first child (and does
-* not need a leading comma).
+* Indicates if the next child item needs a leading comma.
+* Only the first item of arrays and objects doesn't need one.
*/
- struct strbuf first_stack;
+ unsigned int need_comma:1;
- int pretty;
+ unsigned int pretty:1;
};
-#define JSON_WRITER_INIT { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, 0 }
+#define JSON_WRITER_INIT { STRBUF_INIT, STRBUF_INIT, 0, 0 }
void jw_init(struct json_writer *jw);
void jw_release(struct json_writer *jw);
--
2.17.1
Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
Am 07.06.2018 um 16:12 schrieb g...@jeffhostetler.com: > From: Jeff Hostetler > > Add a series of jw_ routines and "struct json_writer" structure to compose > JSON data. The resulting string data can then be output by commands wanting > to support a JSON output format. > > The json-writer routines can be used to generate structured data in a > JSON-like format. We say "JSON-like" because we do not enforce the Unicode > (usually UTF-8) requirement on string fields. Internally, Git does not > necessarily have Unicode/UTF-8 data for most fields, so it is currently > unclear the best way to enforce that requirement. For example, on Linx > pathnames can contain arbitrary 8-bit character data, so a command like > "status" would not know how to encode the reported pathnames. We may want > to revisit this (or double encode such strings) in the future. > > The initial use for the json-writer routines is for generating telemetry > data for executed Git commands. Later, we may want to use them in other > commands, such as status. > > Helped-by: René Scharfe > Helped-by: Wink Saville > Helped-by: Ramsay Jones > Signed-off-by: Jeff Hostetler > --- > Makefile| 2 + > json-writer.c | 419 > json-writer.h | 113 + > t/helper/test-json-writer.c | 572 > > t/t0019-json-writer.sh | 236 ++ > 5 files changed, 1342 insertions(+) > create mode 100644 json-writer.c > create mode 100644 json-writer.h > create mode 100644 t/helper/test-json-writer.c > create mode 100755 t/t0019-json-writer.sh > > diff --git a/Makefile b/Makefile > index a1d8775..4ae6946 100644 > --- a/Makefile > +++ b/Makefile > @@ -666,6 +666,7 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh > TEST_PROGRAMS_NEED_X += test-genrandom > TEST_PROGRAMS_NEED_X += test-hashmap > TEST_PROGRAMS_NEED_X += test-index-version > +TEST_PROGRAMS_NEED_X += test-json-writer> TEST_PROGRAMS_NEED_X += > test-lazy-init-name-hash > TEST_PROGRAMS_NEED_X += test-line-buffer > TEST_PROGRAMS_NEED_X += test-match-trees This doesn't apply cleanly on master, because most test helpers have been combined into a single binary to reduce their disk footprint and link times. See efd71f8913 (t/helper: add an empty test-tool program) for the overall rationale. test-json-writer could become a built-in as well, I think. You can see e.g. in c932a5ff28 (t/helper: merge test-string-list into test-tool) what needs to be done to convert a helper. René
Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format
On Thu, Jun 7, 2018 at 10:12 AM, wrote:
> Add a series of jw_ routines and "struct json_writer" structure to compose
> JSON data. The resulting string data can then be output by commands wanting
> to support a JSON output format.
> [...]
> Signed-off-by: Jeff Hostetler
> ---
> diff --git a/t/t0019-json-writer.sh b/t/t0019-json-writer.sh
> @@ -0,0 +1,236 @@
> +test_expect_success 'simple object' '
> + cat >expect <<-\EOF &&
> + {"a":"abc","b":42,"c":3.14,"d":true,"e":false,"f":null}
> + EOF
> + test-json-writer >actual \
> + @object \
> + @object-string a abc \
> + @object-int b 42 \
> + @object-double c 2 3.140 \
> + @object-true d \
> + @object-false e \
> + @object-null f \
> + @end &&
> + test_cmp expect actual
> +'
To make it easier on people writing these tests, it might be nice for
this to be less noisy by getting rid of "@" and "\". To get rid of
"\", the test program could grab its script commands from stdin (one
instruction per line) rather than from argv[]. For instance:
test-json-writer >actual <<-\EOF &&
object
object-string a abc
...
end
EOF
Not a big deal, and certainly not worth a re-roll.
