Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Jeff Hostetler]

On 6/2/2018 1:13 AM, Jeff King wrote:

On Sat, Jun 02, 2018 at 06:41:06AM +0200, Duy Nguyen wrote:


On Mon, Mar 26, 2018 at 4:31 PM,   wrote:

+static inline void assert_in_array(const struct json_writer *jw)
+{
+   if (!jw->open_stack.len)
+   die("json-writer: array: missing jw_array_begin()");


When you reroll, please consider marking all these string for
translation with _(), unless these are for machine consumption.


Actually, these are all basically asserts, I think. So ideally they'd be
BUG()s and not translated.


Yes, these are basically asserts.  I'll convert them to BUG's and send
up another version this week.

Thanks
Jeff

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Jeff King]

On Sat, Jun 02, 2018 at 06:41:06AM +0200, Duy Nguyen wrote:

> On Mon, Mar 26, 2018 at 4:31 PM,   wrote:
> > +static inline void assert_in_array(const struct json_writer *jw)
> > +{
> > +   if (!jw->open_stack.len)
> > +   die("json-writer: array: missing jw_array_begin()");
> 
> When you reroll, please consider marking all these string for
> translation with _(), unless these are for machine consumption.

Actually, these are all basically asserts, I think. So ideally they'd be
BUG()s and not translated.

-Peff

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Duy Nguyen]

On Mon, Mar 26, 2018 at 4:31 PM,   wrote:
> +static inline void assert_in_array(const struct json_writer *jw)
> +{
> +   if (!jw->open_stack.len)
> +   die("json-writer: array: missing jw_array_begin()");

When you reroll, please consider marking all these string for
translation with _(), unless these are for machine consumption.

> +   if (jw->open_stack.buf[jw->open_stack.len - 1] != '[')
> +   die("json-writer: array: not in array");
-- 
Duy

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Junio C Hamano]

Ramsay Jones  writes:

> BTW, I forgot to mention that you had some whitespace problems
> with this patch, viz:
>
>   $ git log --oneline -1 --check master-json
>   ab643d838 (master-json) json_writer: new routines to create data in JSON 
> format
>   t/helper/test-json-writer.c:280: trailing whitespace.
>   + */ 
>   t/t0019-json-writer.sh:179: indent with spaces.
>   +"g": 0,
>   t/t0019-json-writer.sh:180: indent with spaces.
>   +"h": 1
>   $ 

Yes, and the here-doc that shows expected output looked somewhat
old-fashioned without using <<- indent.

Writing it in a way like this might be a reasonable workaround for
"indent with spaces", which is only about the leading blank used to
indent the line:

<--HT-->sed -e "s/^\|//" <<-\EOF
<--HT-->|  ...
<--HT-->|"g": 0,
<--HT-->|"h": 0,
<--HT-->|  ...
<--HT-->EOF

That is, (1) Use the same number of HT as the line that begins the
here-doc to indent the expected output; (2) but explicitly mark the
left-most column with '|' or something; and then (3) write 8 or more
spaces liberally as needed to reproduce the expected output.

I called it a "workaround", as another possibility is to loosen the
whitespace rule in t/.gitattributes; we _could_ declare that indent
with spaces is OK in these scripts as they contain many expected
output examples in here-doc form.

The trailing whitespace after closing C-comment is simply a style
violation that is not excuable (I think I've dealt with it when I
queued the patch); it should just be fixed.

Thanks.

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Ramsay Jones]

On 27/03/18 18:14, Jeff Hostetler wrote:
> 
> 
> On 3/27/2018 11:45 AM, Ramsay Jones wrote:
>>
>>
>> On 27/03/18 04:18, Ramsay Jones wrote:
>>> On 26/03/18 15:31, g...@jeffhostetler.com wrote:
 From: Jeff Hostetler 

>>> [snip]
>>>
>>> Thanks, this version fixes all issues I had (with the compilation
>>> and sparse warnings).
>>>
>>> [Was using UINT64_C(0x) a problem on windows?]
>>
>> BTW, I forgot to mention that you had some whitespace problems
>> with this patch, viz:
> 
> I ran "make sparse" on this and it did not complain about any of this.

No, sparse doesn't check for whitespace issues.

> What command do you run to get these messages?

'git am' told me when I applied the patch from the ML, and ...

> I know they appear as red in diffs (and I usually double check that),
> but I had not seen a command to complain about them like this.

... since I applied the patch yesterday, it was easier to
demonstrate the issue today using this command:

>>    $ git log --oneline -1 --check master-json
>>    ab643d838 (master-json) json_writer: new routines to create data in JSON 
>> format
>>    t/helper/test-json-writer.c:280: trailing whitespace.
>>    + */
>>    t/t0019-json-writer.sh:179: indent with spaces.
>>    +    "g": 0,
>>    t/t0019-json-writer.sh:180: indent with spaces.
>>    +    "h": 1
>>    $
> 
> the leading spaces are required in this case.
> the pretty json output contains 8 spaces for that sub-structure not a tab.
> is there a preferred way to denote this in the test script?

OK, I should have looked at the output more closely! :(

Only the trailing whitespace in test-json-writer.c needs
to be addressed then.

ATB,
Ramsay Jones

RE: [PATCH v4] json_writer: new routines to create data in JSON format

[Randall S. Becker]

On March 27, 2018 1:43 PM, Wink Saville wrote:
> > the leading spaces are required in this case.
> > the pretty json output contains 8 spaces for that sub-structure not a tab.
> > is there a preferred way to denote this in the test script?
> >
> > Jeff
> 
> I've used "git diff --check" which I got from
> Documentation/SubmittingPatches.

While I have not done this in the git suite, my own suites use something along 
the lines of the following when I need (and have to validate) exact spacing at 
the beginning of lines in expected output:

cat <<-EOF | sed "1,\$s/_/ /g" >expect
{ level1 ...
{ level2
Etc.
EOF

Providing you don't use _ elsewhere in the output. It's a bit hacky but works.

Cheers,
Randall

-- Brief whoami:
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Wink Saville]

> the leading spaces are required in this case.
> the pretty json output contains 8 spaces for that sub-structure not a tab.
> is there a preferred way to denote this in the test script?
>
> Jeff

I've used "git diff --check" which I got from Documentation/SubmittingPatches.

-- Wink

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Jeff Hostetler]

On 3/27/2018 11:45 AM, Ramsay Jones wrote:



On 27/03/18 04:18, Ramsay Jones wrote:

On 26/03/18 15:31, g...@jeffhostetler.com wrote:

From: Jeff Hostetler 


[snip]

Thanks, this version fixes all issues I had (with the compilation
and sparse warnings).

[Was using UINT64_C(0x) a problem on windows?]


BTW, I forgot to mention that you had some whitespace problems
with this patch, viz:


I ran "make sparse" on this and it did not complain about any of this.

What command do you run to get these messages?
I know they appear as red in diffs (and I usually double check that),
but I had not seen a command to complain about them like this.



   $ git log --oneline -1 --check master-json
   ab643d838 (master-json) json_writer: new routines to create data in JSON 
format
   t/helper/test-json-writer.c:280: trailing whitespace.
   + */
   t/t0019-json-writer.sh:179: indent with spaces.
   +"g": 0,
   t/t0019-json-writer.sh:180: indent with spaces.
   +"h": 1
   $


the leading spaces are required in this case.
the pretty json output contains 8 spaces for that sub-structure not a tab.
is there a preferred way to denote this in the test script?

Jeff

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Ramsay Jones]

On 27/03/18 04:18, Ramsay Jones wrote:
> On 26/03/18 15:31, g...@jeffhostetler.com wrote:
>> From: Jeff Hostetler 
>>
> [snip]
> 
> Thanks, this version fixes all issues I had (with the compilation
> and sparse warnings).
> 
> [Was using UINT64_C(0x) a problem on windows?]

BTW, I forgot to mention that you had some whitespace problems
with this patch, viz:

  $ git log --oneline -1 --check master-json
  ab643d838 (master-json) json_writer: new routines to create data in JSON 
format
  t/helper/test-json-writer.c:280: trailing whitespace.
  + */ 
  t/t0019-json-writer.sh:179: indent with spaces.
  +"g": 0,
  t/t0019-json-writer.sh:180: indent with spaces.
  +"h": 1
  $ 

ATB,
Ramsay Jones

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Jeff Hostetler]

On 3/26/2018 11:18 PM, Ramsay Jones wrote:

On 26/03/18 15:31, g...@jeffhostetler.com wrote:

From: Jeff Hostetler 

[snip]

Thanks, this version fixes all issues I had (with the compilation
and sparse warnings).

[Was using UINT64_C(0x) a problem on windows?]


Thanks for the confirmation.

I was building on Linux.  I haven't tried using UINT64_C()
for anything, but I'll keep that in mind next time.

Thanks,
Jeff

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Ramsay Jones]

On 26/03/18 15:31, g...@jeffhostetler.com wrote:
> 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   | 395 +
>  json-writer.h   |  92 +++
>  t/helper/test-json-writer.c | 590 
> 
>  t/t0019-json-writer.sh  | 253 +++
>  5 files changed, 1332 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
> 
[snip]

Thanks, this version fixes all issues I had (with the compilation
and sparse warnings).

[Was using UINT64_C(0x) a problem on windows?]

ATB,
Ramsay Jones