[RFC/PATCH 0/3] JSON/XML output for scripting interface

[RFC/PATCH 0/3] JSON/XML output for scripting interface

[Julian Phillips]

Here is an attempt at making a format agnostic structured output library.  The
idea being that the command doing the output doesn't have to care what the
actual output format is, it just uses the abstract notion of objects and arrays.

The current backend/frontend interface probably needs expanding so that a less
noddy XML output can be used, but it's a start.

Rather than building an in-memory structure and then writing it out I've gone
for the approach of writing out immediately.  The thought behind this was that I
didn't really want to force commands like log to have to wait 'til the end to
start outputting information.

The JSON output is formatted differently from my previous JSON-only status
modification, but the actual information output is the same (i.e. the output of
a json parser should be identical ignoring item ordering in objects).

Julian Phillips (3):
  strbuf: Add strbuf_vaddf function
  add a library of code for producing structured output
  status: add support for structured output

 Makefile         |    3 +
 builtin/commit.c |   12 +++
 output-json.c    |  128 ++++++++++++++++++++++++++++++++
 output-xml.c     |   68 +++++++++++++++++
 output.c         |  212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 output.h         |   71 ++++++++++++++++++
 strbuf.c         |   13 +++-
 strbuf.h         |    1 +
 wt-status.c      |   73 +++++++++++++++++++
 wt-status.h      |    2 +
 10 files changed, 581 insertions(+), 2 deletions(-)
 create mode 100644 output-json.c
 create mode 100644 output-xml.c
 create mode 100644 output.c
 create mode 100644 output.h

[RFC/PATCH 1/3] strbuf: Add strbuf_vaddf function

[Julian Phillips]

Add strbuf_vaddf which is to strbuf_addf as vprintf is to printf.

Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
---
 strbuf.c |   13 +++++++++++--
 strbuf.h |    1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index bc3a080..8f312f8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -194,19 +194,28 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)
 
 void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 {
+	va_list ap;
+
+	va_start(ap, fmt);
+        strbuf_vaddf(sb, fmt, ap);
+	va_end(ap);
+}
+
+void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list args)
+{
 	int len;
 	va_list ap;
 
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 64);
-	va_start(ap, fmt);
+	va_copy(ap, args);
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 	va_end(ap);
 	if (len < 0)
 		die("your vsnprintf is broken");
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
-		va_start(ap, fmt);
+		va_copy(ap, args);
 		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 		va_end(ap);
 		if (len > strbuf_avail(sb)) {
diff --git a/strbuf.h b/strbuf.h
index fac2dbc..ac52834 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -120,6 +120,7 @@ extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *
 
 __attribute__((format (printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list args);
 
 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
-- 
1.7.0.4

[RFC/PATCH 2/3] add a library of code for producing structured output

[Julian Phillips]

Add a library that allows commands to produce structured output in any
of a range of formats using a single API.

The API includes an OPT_OUTPUT and handle_output_arg so that the
option handling for different commands will be as similar as possible.

At the moment JSON and XML output options are available - though the
XML output is _very_ rudimentary.

Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
---
 Makefile      |    3 +
 output-json.c |  128 ++++++++++++++++++++++++++++++++++
 output-xml.c  |   68 ++++++++++++++++++
 output.c      |  212 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 output.h      |   71 +++++++++++++++++++
 5 files changed, 482 insertions(+), 0 deletions(-)
 create mode 100644 output-json.c
 create mode 100644 output-xml.c
 create mode 100644 output.c
 create mode 100644 output.h

diff --git a/Makefile b/Makefile
index 910f471..4ba2a4f 100644
--- a/Makefile
+++ b/Makefile
@@ -576,6 +576,9 @@ LIB_OBJS += merge-recursive.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += notes.o
 LIB_OBJS += object.o
+LIB_OBJS += output.o
+LIB_OBJS += output-json.o
+LIB_OBJS += output-xml.o
 LIB_OBJS += pack-check.o
 LIB_OBJS += pack-refs.o
 LIB_OBJS += pack-revindex.o
diff --git a/output-json.c b/output-json.c
new file mode 100644
index 0000000..0eb66b2
--- /dev/null
+++ b/output-json.c
@@ -0,0 +1,128 @@
+#include "git-compat-util.h"
+#include "output.h"
+#include "strbuf.h"
+
+static char *json_quote(char *s)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	while (*s) {
+		switch (*s) {
+		case '"':
+			strbuf_addstr(&buf, "\\\"");
+			break;
+		case '\\':
+			strbuf_addstr(&buf, "\\\\");
+			break;
+		case '\b':
+			strbuf_addstr(&buf, "\\b");
+			break;
+		case '\f':
+			strbuf_addstr(&buf, "\\f");
+			break;
+		case '\n':
+			strbuf_addstr(&buf, "\\n");
+			break;
+		case '\r':
+			strbuf_addstr(&buf, "\\r");
+			break;
+		case '\t':
+			strbuf_addstr(&buf, "\\t");
+			break;
+		default:
+			/* All control characters must be encode, even if they
+			 * don't have a specific escape character of their own */
+			if (*s < 0x20)
+				strbuf_addf(&buf, "\\u%04x", *s);
+			else
+				strbuf_addch(&buf, *s);
+			break;
+		}
+		s++;
+	}
+
+	return strbuf_detach(&buf, NULL);
+}
+
+static void json_obj_start(FILE *file, char *name)
+{
+	fprintf(file, "{\n");
+}
+
+static void json_obj_end(FILE *file, char *name)
+{
+	fprintf(file, "\n}");
+}
+
+static void json_obj_item_start(FILE *file, char *name, int first)
+{
+	char *quoted = json_quote(name);
+	if (!first)
+		fprintf(file, ",\n");
+	fprintf(file, "\"%s\" : ", quoted);
+	free(quoted);
+}
+
+static void json_array_start(FILE *file, char *name)
+{
+	fprintf(file, "[\n");
+}
+
+static void json_array_end(FILE *file, char *name)
+{
+	fprintf(file, "\n]");
+}
+
+static void json_array_item_start(FILE *file, char *name, int first)
+{
+	if (!first)
+		fprintf(file, ",\n");
+}
+
+static void json_bool(FILE *file, int value)
+{
+	if (value)
+		fprintf(file, "true");
+	else
+		fprintf(file, "false");
+}
+
+static void json_str(FILE *file, char *value)
+{
+	char *quoted = json_quote(value);
+	fprintf(file, "\"%s\"", quoted);
+	free(quoted);
+}
+
+static void json_int(FILE *file, int64_t value)
+{
+	fprintf(file, "%lld", value);
+}
+
+static void json_uint(FILE *file, uint64_t value)
+{
+	fprintf(file, "%llu", value);
+}
+
+static void json_double(FILE *file, double value, int precision)
+{
+	fprintf(file, "%.*f", precision, value);
+}
+
+struct output_ops output_json_ops = {
+	json_obj_start,
+	json_obj_end,
+	json_obj_item_start,
+	NULL,
+
+	json_array_start,
+	json_array_end,
+	json_array_item_start,
+	NULL,
+
+	json_bool,
+	json_str,
+	json_int,
+	json_uint,
+	json_double,
+};
diff --git a/output-xml.c b/output-xml.c
new file mode 100644
index 0000000..50dd7d6
--- /dev/null
+++ b/output-xml.c
@@ -0,0 +1,68 @@
+#include "git-compat-util.h"
+#include "output.h"
+
+static void xml_obj_start(FILE *file, char *name)
+{
+	fprintf(file, "<object name=\"%s\">\n", name);
+}
+
+static void xml_obj_end(FILE *file, char *name)
+{
+	fprintf(file, "</object>\n");
+}
+
+static void xml_obj_item_start(FILE *file, char *name, int first)
+{
+	fprintf(file, "<%s>", name);
+}
+
+static void xml_obj_item_end(FILE *file, char *name, int first)
+{
+	fprintf(file, "</%s>\n", name);
+}
+
+static void xml_bool(FILE *file, int value)
+{
+	if (value)
+		fprintf(file, "true");
+	else
+		fprintf(file, "false");
+}
+
+static void xml_str(FILE *file, char *value)
+{
+	fprintf(file, "\"%s\"", value);
+}
+
+static void xml_int(FILE *file, int64_t value)
+{
+	fprintf(file, "%lld", value);
+}
+
+static void xml_uint(FILE *file, uint64_t value)
+{
+	fprintf(file, "%llu", value);
+}
+
+static void xml_double(FILE *file, double value, int precision)
+{
+	fprintf(file, "%.*f", precision, value);
+}
+
+struct output_ops output_xml_ops = {
+	xml_obj_start,
+	xml_obj_end,
+	xml_obj_item_start,
+	xml_obj_item_end,
+
+	NULL,
+	NULL,
+	NULL,
+	NULL,
+
+	xml_bool,
+	xml_str,
+	xml_int,
+	xml_uint,
+	xml_double,
+};
diff --git a/output.c b/output.c
new file mode 100644
index 0000000..305428c
--- /dev/null
+++ b/output.c
@@ -0,0 +1,212 @@
+#include "git-compat-util.h"
+#include "output.h"
+#include "strbuf.h"
+
+#define DEFAULT_OUTPUT_FORMAT OUTPUT_JSON
+
+extern struct output_ops output_json_ops;
+extern struct output_ops output_xml_ops;
+
+struct output_ops *output_ops[] = {
+	NULL, /* OUTPUT_NORMAL */
+	&output_json_ops,
+	&output_xml_ops,
+};
+
+enum output_style handle_output_arg(char *s)
+{
+	if (!s)
+		return OUTPUT_NORMAL;
+	else if (!strcmp(s, "no"))
+		return OUTPUT_NORMAL;
+	else if (!strcmp(s, "json"))
+		return OUTPUT_JSON;
+	else if (!strcmp(s, "xml"))
+		return OUTPUT_XML;
+	else
+		die("Invalid output style '%s'", s);
+}
+
+char *output_supported_styles()
+{
+	return "";
+}
+
+struct output_context *output_start(enum output_style style)
+{
+	struct output_context *context = xmallocz(sizeof(*context));
+
+	context->style = style;
+	context->file = stdout;
+	context->ops = output_ops[style];
+
+	output_start_object(context, "git");
+
+	return context;
+}
+
+void output_end(struct output_context *context)
+{
+	while(context->current)
+		output_end_current(context);
+
+	fprintf(context->file, "\n");
+
+	free(context);
+}
+
+
+static void item_start(struct output_context *context, char *name)
+{
+	if (!context->current)
+		return;
+
+	switch (context->current->type) {
+	case OUTPUT_ITEM_OBJECT:
+		if (context->ops->obj_item_start)
+			context->ops->obj_item_start(context->file, name,
+						     context->current->first);
+		break;
+	case OUTPUT_ITEM_ARRAY:
+		if (context->ops->array_item_start)
+			context->ops->array_item_start(context->file, name,
+						       context->current->first);
+		break;
+	}
+}
+
+static void item_end(struct output_context *context, char *name)
+{
+	if (!context->current)
+		return;
+
+	switch (context->current->type) {
+	case OUTPUT_ITEM_OBJECT:
+		if (context->ops->obj_item_end)
+			context->ops->obj_item_end(context->file, name,
+						   context->current->first);
+		break;
+	case OUTPUT_ITEM_ARRAY:
+		if (context->ops->array_item_end)
+			context->ops->array_item_end(context->file, name,
+						     context->current->first);
+		break;
+	}
+
+	context->current->first = 0;
+}
+
+
+void output_new_current(struct output_context *context, char *name,
+			enum output_item_type type)
+{
+	struct output_item *item = xmallocz(sizeof(*item));
+
+	item->name = xstrdup(name);
+	item->type = type;
+	item->first = 1;
+
+	item->prev = context->current;
+	context->current = item;
+}
+
+void output_start_object(struct output_context *context, char *name)
+{
+	item_start(context, name);
+
+	output_new_current(context, name, OUTPUT_ITEM_OBJECT);
+
+	if (context->ops->obj_start)
+		context->ops->obj_start(context->file, name);
+}
+
+void output_start_array(struct output_context *context, char *name)
+{
+	item_start(context, name);
+
+	output_new_current(context, name, OUTPUT_ITEM_ARRAY);
+
+	if (context->ops->array_start)
+		context->ops->array_start(context->file, name);
+}
+
+void output_end_current(struct output_context *context)
+{
+	struct output_item *item = context->current;
+
+	switch (item->type) {
+	case OUTPUT_ITEM_OBJECT:
+		if (context->ops->obj_end)
+			context->ops->obj_end(context->file, item->name);
+		break;
+	case OUTPUT_ITEM_ARRAY:
+		if (context->ops->array_end)
+			context->ops->array_end(context->file, item->name);
+		break;
+	}
+
+	item = context->current;
+	context->current = context->current->prev;
+
+	item_end(context, item->name);
+
+	free(item->name);
+	free(item);
+}
+
+
+void output_bool(struct output_context *context, char *name, int value)
+{
+	item_start(context, name);
+	if (context->ops->bool)
+		context->ops->bool(context->file, value);
+	item_end(context, name);
+}
+
+void output_str(struct output_context *context, char *name, char *value)
+{
+	item_start(context, name);
+	if (context->ops->str)
+		context->ops->str(context->file, value);
+	item_end(context, name);
+}
+
+void output_strf(struct output_context *context, char *name, char *fmt, ...)
+{
+	struct strbuf buf = STRBUF_INIT;
+	va_list ap;
+
+	va_start(ap, fmt);
+	strbuf_vaddf(&buf, fmt, ap);
+	va_end(ap);
+
+	output_str(context, name, buf.buf);
+
+	strbuf_release(&buf);
+}
+
+void output_int(struct output_context *context, char *name, int64_t value)
+{
+	item_start(context, name);
+	if (context->ops->int_)
+		context->ops->int_(context->file, value);
+	item_end(context, name);
+}
+
+void output_uint(struct output_context *context, char *name, uint64_t value)
+{
+	item_start(context, name);
+	if (context->ops->uint)
+		context->ops->uint(context->file, value);
+	item_end(context, name);
+}
+
+void output_double(struct output_context *context, char *name, double value,
+		   int precision)
+{
+	item_start(context, name);
+	if (context->ops->double_)
+		context->ops->double_(context->file, value, precision);
+	item_end(context, name);
+}
+
diff --git a/output.h b/output.h
new file mode 100644
index 0000000..9472ae4
--- /dev/null
+++ b/output.h
@@ -0,0 +1,71 @@
+#ifndef OUTPUT_H
+#define OUTPUT_H
+
+enum output_style {
+	OUTPUT_NORMAL,
+	OUTPUT_JSON,
+	OUTPUT_XML,
+};
+
+struct output_ops {
+	void (*obj_start)(FILE *file, char *name);
+	void (*obj_end)(FILE *file, char *name);
+	void (*obj_item_start)(FILE *file, char *name, int first);
+	void (*obj_item_end)(FILE *file, char *name, int first);
+
+	void (*array_start)(FILE *file, char *name);
+	void (*array_end)(FILE *file, char *name);
+	void (*array_item_start)(FILE *file, char *name, int first);
+	void (*array_item_end)(FILE *file, char *name, int first);
+
+	void (*bool)(FILE *file, int value);
+	void (*str)(FILE *file, char *value);
+	void (*int_)(FILE *file, int64_t value);
+	void (*uint)(FILE *file, uint64_t value);
+	void (*double_)(FILE *file, double value, int precision);
+};
+
+enum output_item_type {
+	OUTPUT_ITEM_OBJECT,
+	OUTPUT_ITEM_ARRAY,
+};
+
+struct output_item {
+	struct output_item *prev;
+	char *name;
+	enum output_item_type type;
+	int first;
+};
+
+struct output_context {
+	enum output_style style;
+	FILE *file;
+	struct output_ops *ops;
+	struct output_item *current;
+};
+
+extern struct option OUTPUT_OPTION;
+
+#define OPT_OUTPUT(s, l, v) { OPTION_STRING, (s), (l), (v), "style",     \
+			      "Use a structured output style, options: " \
+			      "no, json, xml (Default: no)",             \
+			      PARSE_OPT_OPTARG, NULL, (intptr_t)"no" }
+
+enum output_style handle_output_arg(char *s);
+
+struct output_context *output_start(enum output_style style);
+void output_end(struct output_context *context);
+
+void output_start_object(struct output_context *context, char *name);
+void output_start_array(struct output_context *context, char *name);
+void output_end_current(struct output_context *context);
+
+void output_bool(struct output_context *context, char *name, int value);
+void output_str(struct output_context *context, char *name, char *value);
+void output_strf(struct output_context *context, char *name, char *fmt, ...);
+void output_int(struct output_context *context, char *name, int64_t value);
+void output_uint(struct output_context *context, char *name, uint64_t value);
+void output_double(struct output_context *context, char *name, double value,
+		   int precision);
+
+#endif /* OUTPUT_H */
-- 
1.7.0.4

[RFC/PATCH 3/3] status: add support for structured output

[Julian Phillips]

Add support for using the new structured output modes for outputting
status information.

Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
---
 builtin/commit.c |   12 +++++++++
 wt-status.c      |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h      |    2 +
 3 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index c5ab683..77464d3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -25,6 +25,7 @@
 #include "rerere.h"
 #include "unpack-trees.h"
 #include "quote.h"
+#include "output.h"
 
 static const char * const builtin_commit_usage[] = {
 	"git commit [options] [--] <filepattern>...",
@@ -68,6 +69,7 @@ static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite;
 static char *untracked_files_arg, *force_date;
+static char *structured_output_arg;
 /*
  * The default commit message cleanup mode will remove the lines
  * beginning with # (shell comments) and leading and trailing
@@ -91,6 +93,7 @@ static enum {
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN,
+	STATUS_FORMAT_STRUCTURED,
 } status_format = STATUS_FORMAT_LONG;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
@@ -1018,6 +1021,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 {
 	struct wt_status s;
 	unsigned char sha1[20];
+	enum output_style output_style;
 	static struct option builtin_status_options[] = {
 		OPT__VERBOSE(&verbose),
 		OPT_SET_INT('s', "short", &status_format,
@@ -1031,6 +1035,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		  "mode",
 		  "show untracked files, optional modes: all, normal, no. (Default: all)",
 		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+		OPT_OUTPUT(0, "structured-output", &structured_output_arg),
 		OPT_END(),
 	};
 
@@ -1045,6 +1050,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			     builtin_status_usage, 0);
 	handle_untracked_files_arg(&s);
 
+	output_style = handle_output_arg(structured_output_arg);
+	if (output_style != OUTPUT_NORMAL)
+		status_format = STATUS_FORMAT_STRUCTURED;
+
 	if (*argv)
 		s.pathspec = get_pathspec(prefix, argv);
 
@@ -1068,6 +1077,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	case STATUS_FORMAT_PORCELAIN:
 		wt_porcelain_print(&s, null_termination);
 		break;
+	case STATUS_FORMAT_STRUCTURED:
+		wt_structured_print(&s, output_style);
+		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
 		wt_status_print(&s);
diff --git a/wt-status.c b/wt-status.c
index 8ca59a2..162f719 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -750,3 +750,76 @@ void wt_porcelain_print(struct wt_status *s, int null_termination)
 	s->prefix = NULL;
 	wt_shortstatus_print(s, null_termination);
 }
+
+static void wt_structured_unmerged(struct string_list_item *it,
+				   struct output_context *oc)
+{
+	struct wt_status_change_data *d = it->util;
+	char *ours = "?", *theirs = "?";
+
+	switch (d->stagemask) {
+	case 1: ours = "D"; theirs = "D"; break; /* both deleted */
+	case 2: ours = "A"; theirs = "U"; break; /* added by us */
+	case 3: ours = "U"; theirs = "D"; break; /* deleted by them */
+	case 4: ours = "U"; theirs = "A"; break; /* added by them */
+	case 5: ours = "D"; theirs = "U"; break; /* deleted by us */
+	case 6: ours = "A"; theirs = "A"; break; /* both added */
+	case 7: ours = "U"; theirs = "U"; break; /* both modified */
+	}
+
+	output_start_object(oc, "entry");
+	output_str(oc, "name", it->string);
+	output_str(oc, "ours", ours);
+	output_str(oc, "theirs", theirs);
+	output_end_current(oc);
+}
+
+static void wt_structured_status(struct string_list_item *it,
+				 struct output_context *oc)
+{
+	struct wt_status_change_data *d = it->util;
+	char index = '-', worktree = '-';
+
+	if (d->index_status)
+		index = d->index_status;
+	if (d->worktree_status)
+		worktree = d->worktree_status;
+
+	output_start_object(oc, "entry");
+	output_str(oc, "name", it->string);
+	if (d->head_path)
+		output_str(oc, "orig_name", d->head_path);
+	output_strf(oc, "index", "%c", index);
+	output_strf(oc, "worktree", "%c", worktree);
+	output_end_current(oc);
+}
+
+void wt_structured_print(struct wt_status *s, enum output_style style)
+{
+	int i;
+	struct output_context *oc = output_start(style);
+
+	output_start_array(oc, "entries");
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d;
+		struct string_list_item *it;
+
+		it = &(s->change.items[i]);
+		d = it->util;
+		if (d->stagemask)
+			wt_structured_unmerged(it, oc);
+		else
+			wt_structured_status(it, oc);
+	}
+
+	for (i = 0; i < s->untracked.nr; i++) {
+		output_start_object(oc, "entry");
+		output_str(oc, "name", s->untracked.items[i].string);
+		output_str(oc, "index", "?");
+		output_str(oc, "worktree", "?");
+		output_end_current(oc);
+	}
+
+	output_end(oc);
+}
diff --git a/wt-status.h b/wt-status.h
index 9120673..d5b1342 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include "string-list.h"
 #include "color.h"
+#include "output.h"
 
 enum color_wt_status {
 	WT_STATUS_HEADER = 0,
@@ -61,5 +62,6 @@ void wt_status_collect(struct wt_status *s);
 
 void wt_shortstatus_print(struct wt_status *s, int null_termination);
 void wt_porcelain_print(struct wt_status *s, int null_termination);
+void wt_structured_print(struct wt_status *s, enum output_style style);
 
 #endif /* STATUS_H */
-- 
1.7.0.4

Re: [RFC/PATCH 1/3] strbuf: Add strbuf_vaddf function

[Erik Faye-Lund]

On Sun, Apr 11, 2010 at 1:37 PM, Julian Phillips
<julian@quantumfyre.co.uk> wrote:
> Add strbuf_vaddf which is to strbuf_addf as vprintf is to printf.
>
> Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
> ---
>  strbuf.c |   13 +++++++++++--
>  strbuf.h |    1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index bc3a080..8f312f8 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -194,19 +194,28 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)
>
>  void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
>  {
> +       va_list ap;
> +
> +       va_start(ap, fmt);
> +        strbuf_vaddf(sb, fmt, ap);
> +       va_end(ap);
> +}
> +
> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list args)
> +{
>        int len;
>        va_list ap;
>
>        if (!strbuf_avail(sb))
>                strbuf_grow(sb, 64);
> -       va_start(ap, fmt);
> +       va_copy(ap, args);

Isn't va_copy C99? The only other place we use this is in
compat/winansi.c, but that file is only compiled on Windows. Both
compilers we support on Windows supports va_copy (or some way of
emulating it).

IIRC, strbuf_vaddf() has been attempted added multiple times before
(by me, for one), and the efforts have always ended up being scrapped
due to the lack of a portable va_copy.

-- 
Erik "kusma" Faye-Lund

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Erik Faye-Lund]

On Sun, Apr 11, 2010 at 1:37 PM, Julian Phillips
<julian@quantumfyre.co.uk> wrote:
> Add a library that allows commands to produce structured output in any
> of a range of formats using a single API.
>
> The API includes an OPT_OUTPUT and handle_output_arg so that the
> option handling for different commands will be as similar as possible.
>
> At the moment JSON and XML output options are available - though the
> XML output is _very_ rudimentary.
>
> Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
> ---
>  Makefile      |    3 +
>  output-json.c |  128 ++++++++++++++++++++++++++++++++++
>  output-xml.c  |   68 ++++++++++++++++++
>  output.c      |  212 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  output.h      |   71 +++++++++++++++++++
>  5 files changed, 482 insertions(+), 0 deletions(-)
>  create mode 100644 output-json.c
>  create mode 100644 output-xml.c
>  create mode 100644 output.c
>  create mode 100644 output.h
>
> diff --git a/Makefile b/Makefile
> index 910f471..4ba2a4f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -576,6 +576,9 @@ LIB_OBJS += merge-recursive.o
>  LIB_OBJS += name-hash.o
>  LIB_OBJS += notes.o
>  LIB_OBJS += object.o
> +LIB_OBJS += output.o
> +LIB_OBJS += output-json.o
> +LIB_OBJS += output-xml.o
>  LIB_OBJS += pack-check.o
>  LIB_OBJS += pack-refs.o
>  LIB_OBJS += pack-revindex.o
> diff --git a/output-json.c b/output-json.c
> new file mode 100644
> index 0000000..0eb66b2
> --- /dev/null
> +++ b/output-json.c
> @@ -0,0 +1,128 @@
> <snip>
> +
> +static void json_str(FILE *file, char *value)
> +{
> +       char *quoted = json_quote(value);
> +       fprintf(file, "\"%s\"", quoted);
> +       free(quoted);
> +}
> +
> <snip>
> diff --git a/output-xml.c b/output-xml.c
> new file mode 100644
> index 0000000..50dd7d6
> --- /dev/null
> +++ b/output-xml.c
> @@ -0,0 +1,68 @@
> <snip>
> +
> +static void xml_str(FILE *file, char *value)
> +{
> +       fprintf(file, "\"%s\"", value);
> +}
> +

Don't you need to quote this one, like you did in json_str()?

-- 
Erik "kusma" Faye-Lund

Re: [RFC/PATCH 1/3] strbuf: Add strbuf_vaddf function

[Julian Phillips]

On Sun, 11 Apr 2010 14:42:29 +0200, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> On Sun, Apr 11, 2010 at 1:37 PM, Julian Phillips
> <julian@quantumfyre.co.uk> wrote:
>> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list args)
>> +{
>>        int len;
>>        va_list ap;
>>
>>        if (!strbuf_avail(sb))
>>                strbuf_grow(sb, 64);
>> -       va_start(ap, fmt);
>> +       va_copy(ap, args);
> 
> Isn't va_copy C99?

Indeed.

Hi ho, hi ho, It's off to a custom implementation I go ...

Ho hum.  

-- 
Julian

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Julian Phillips]

On Sun, 11 Apr 2010 14:51:37 +0200, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> On Sun, Apr 11, 2010 at 1:37 PM, Julian Phillips
> <julian@quantumfyre.co.uk> wrote:
>> Add a library that allows commands to produce structured output in any
>> of a range of formats using a single API.
>>
>> The API includes an OPT_OUTPUT and handle_output_arg so that the
>> option handling for different commands will be as similar as possible.
>>
>> At the moment JSON and XML output options are available - though the
>> XML output is _very_ rudimentary.
>>
>> Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
>> ---
>>  Makefile      |    3 +
>>  output-json.c |  128 ++++++++++++++++++++++++++++++++++
>>  output-xml.c  |   68 ++++++++++++++++++
>>  output.c      |  212
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  output.h      |   71 +++++++++++++++++++
>>  5 files changed, 482 insertions(+), 0 deletions(-)
>>  create mode 100644 output-json.c
>>  create mode 100644 output-xml.c
>>  create mode 100644 output.c
>>  create mode 100644 output.h
>>
>> diff --git a/Makefile b/Makefile
>> index 910f471..4ba2a4f 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -576,6 +576,9 @@ LIB_OBJS += merge-recursive.o
>>  LIB_OBJS += name-hash.o
>>  LIB_OBJS += notes.o
>>  LIB_OBJS += object.o
>> +LIB_OBJS += output.o
>> +LIB_OBJS += output-json.o
>> +LIB_OBJS += output-xml.o
>>  LIB_OBJS += pack-check.o
>>  LIB_OBJS += pack-refs.o
>>  LIB_OBJS += pack-revindex.o
>> diff --git a/output-json.c b/output-json.c
>> new file mode 100644
>> index 0000000..0eb66b2
>> --- /dev/null
>> +++ b/output-json.c
>> @@ -0,0 +1,128 @@
>> <snip>
>> +
>> +static void json_str(FILE *file, char *value)
>> +{
>> +       char *quoted = json_quote(value);
>> +       fprintf(file, "\"%s\"", quoted);
>> +       free(quoted);
>> +}
>> +
>> <snip>
>> diff --git a/output-xml.c b/output-xml.c
>> new file mode 100644
>> index 0000000..50dd7d6
>> --- /dev/null
>> +++ b/output-xml.c
>> @@ -0,0 +1,68 @@
>> <snip>
>> +
>> +static void xml_str(FILE *file, char *value)
>> +{
>> +       fprintf(file, "\"%s\"", value);
>> +}
>> +
> 
> Don't you need to quote this one, like you did in json_str()?

Yes.  That would be part of the reason for the "_very_" in the comment ...

As it stands the XML code is more of an example of a second output format
than actually usable.  However, since I envision that the frontend/backend
API probably needs tweaking to accommodate backends other than JSON I
wanted to get at least one other backend going ...

-- 
Julian

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Jakub Narebski]

Julian Phillips <julian@quantumfyre.co.uk> writes:

> Add a library that allows commands to produce structured output in any
> of a range of formats using a single API.
> 
> The API includes an OPT_OUTPUT and handle_output_arg so that the
> option handling for different commands will be as similar as possible.
> 
> At the moment JSON and XML output options are available - though the
> XML output is _very_ rudimentary.
> 
> Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
> ---
>  Makefile      |    3 +
>  output-json.c |  128 ++++++++++++++++++++++++++++++++++
>  output-xml.c  |   68 ++++++++++++++++++
>  output.c      |  212 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  output.h      |   71 +++++++++++++++++++
>  5 files changed, 482 insertions(+), 0 deletions(-)
>  create mode 100644 output-json.c
>  create mode 100644 output-xml.c
>  create mode 100644 output.c
>  create mode 100644 output.h

How about some technical documentation, in the form of
Documentation/technical/api-structured-output.txt (or something like
that), describing how this API should be used.

How about some tests?  Note that you need to take care of commits that
are encoded in encoding other than utf-8 (but with 'encoding' header,
so you know what encoding it is), and of filenames that are invalid
UTF-8 (and their encoding is unknown in general: they are raw binary
data).  You need to take care of non-ASCII characters, and of special
characters (like '"', SPC, TAB, LF, '\') in commits and in filenames.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Sverre Rabbelier]

Heya,

On Sun, Apr 11, 2010 at 13:37, Julian Phillips <julian@quantumfyre.co.uk> wrote:
> Here is an attempt at making a format agnostic structured output library.  The
> idea being that the command doing the output doesn't have to care what the
> actual output format is, it just uses the abstract notion of objects and arrays.

How easy is it to add support for this to other commands using the
infrastructure this command adds? I assume that we'd want to do this
for all/most plumbing commands, so I think it's important that we make
sure it's easy to add for other commands other than 'git status', no?

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Julian Phillips]

On Sun, 11 Apr 2010 17:48:28 +0200, Sverre Rabbelier
<srabbelier@gmail.com>
wrote:
> Heya,
> 
> On Sun, Apr 11, 2010 at 13:37, Julian Phillips
<julian@quantumfyre.co.uk>
> wrote:
>> Here is an attempt at making a format agnostic structured output
>> library.  The
>> idea being that the command doing the output doesn't have to care what
>> the
>> actual output format is, it just uses the abstract notion of objects
and
>> arrays.
> 
> How easy is it to add support for this to other commands using the
> infrastructure this command adds? I assume that we'd want to do this
> for all/most plumbing commands, so I think it's important that we make
> sure it's easy to add for other commands other than 'git status', no?

It's intended to be easy, as the intention was to make the structured
output available from all plumbing commands (and maybe even some porcelain
commands, if they are often scripted?).  Easy is in the eye of the beholder
though.  I've done ls-tree as an example below (I expect my MUA will
probably mangle the patch - sorry).  I didn't think it was too hard, but
that's not really surprising since I wrote the API ... you'll have to tell
me what you think.

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index dc86b0d..7b5a5e8 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -10,6 +10,7 @@
 #include "quote.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "output.h"
 
 static int line_termination = '\n';
 #define LS_RECURSIVE 1
@@ -22,6 +23,7 @@ static int ls_options;
 static const char **pathspec;
 static int chomp_prefix;
 static const char *ls_tree_prefix;
+static struct output_context *oc;
 
 static const  char * const ls_tree_usage[] = {
 	"git ls-tree [<options>] <tree-ish> [path...]",
@@ -90,6 +92,25 @@ static int show_tree(const unsigned char *sha1, const
char *base, int baselen,
 	    (baselen < chomp_prefix || memcmp(ls_tree_prefix, base,
chomp_prefix)))
 		return 0;
 
+	if (oc != NULL) {
+		output_start_object(oc, "entry");
+		output_strf(oc, "path", "%s%s", base + chomp_prefix, pathname);
+		if (!(ls_options & LS_NAME_ONLY)) {
+			output_strf(oc, "mode", "%06o", mode);
+			output_str(oc, "type", type);
+			output_str(oc, "hash", sha1_to_hex(sha1));
+			if ((ls_options & LS_SHOW_SIZE) && !strcmp(type, blob_type)) {
+				unsigned long size;
+				if (sha1_object_info(sha1, &size) == OBJ_BAD)
+					output_str(oc, "size", "bad");
+				else
+					output_uint(oc, "size", size);
+			}
+		}
+		output_end_current(oc);
+		return retval;
+	}
+
 	if (!(ls_options & LS_NAME_ONLY)) {
 		if (ls_options & LS_SHOW_SIZE) {
 			char size_text[24];
@@ -119,6 +140,8 @@ int cmd_ls_tree(int argc, const char **argv, const
char *prefix)
 	unsigned char sha1[20];
 	struct tree *tree;
 	int full_tree = 0;
+	char *structured_output_arg = NULL;
+	enum output_style output_style;
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &ls_options, "only show trees",
 			LS_TREE_ONLY),
@@ -140,6 +163,7 @@ int cmd_ls_tree(int argc, const char **argv, const
char *prefix)
 			    "list entire tree; not just current directory "
 			    "(implies --full-name)"),
 		OPT__ABBREV(&abbrev),
+		OPT_OUTPUT(0, "structured-output", &structured_output_arg),
 		OPT_END()
 	};
 
@@ -159,6 +183,8 @@ int cmd_ls_tree(int argc, const char **argv, const
char *prefix)
 	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
 		ls_options |= LS_SHOW_TREES;
 
+	output_style = handle_output_arg(structured_output_arg);
+
 	if (argc < 1)
 		usage_with_options(ls_tree_usage, ls_tree_options);
 	if (get_sha1(argv[0], sha1))
@@ -168,7 +194,16 @@ int cmd_ls_tree(int argc, const char **argv, const
char *prefix)
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
+
+	if (output_style != OUTPUT_NORMAL) {
+		oc = output_start(output_style);
+		output_start_array(oc, "entries");
+	}
+
 	read_tree_recursive(tree, "", 0, 0, pathspec, show_tree, NULL);
 
+	if (output_style != OUTPUT_NORMAL)
+		output_end(oc);
+
 	return 0;
 }


-- 
Julian

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Sverre Rabbelier]

Heya,

On Sun, Apr 11, 2010 at 19:30, Julian Phillips <julian@quantumfyre.co.uk> wrote:
> you'll have to tell me what you think.

The patch does look fairly simple, but I worry that not all commands
will be so simple, that is, they might not all have such an easy point
where you can hook in a different output method? Or am I seeing bears
where there are none?

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Julian Phillips]

On Sun, 11 Apr 2010 19:34:13 +0200, Sverre Rabbelier
<srabbelier@gmail.com>
wrote:
> Heya,
> 
> On Sun, Apr 11, 2010 at 19:30, Julian Phillips
<julian@quantumfyre.co.uk>
> wrote:
>> you'll have to tell me what you think.
> 
> The patch does look fairly simple, but I worry that not all commands
> will be so simple, that is, they might not all have such an easy point
> where you can hook in a different output method? Or am I seeing bears
> where there are none?

I think that there probably are commands where it will be more work to
integrate the output - but I think that is probably more to do with the
structure of the current code than the API of the new.  Does it make a
difference what the API of the new output code is if there isn't currently
a sensible hook-in point?

If code has been written without the expectation that the output format
could be changed then the effort to add a new output format could be
considerably more than for status or ls-tree.  However, with the
frontend/backend design hopefully we only have to endure the effort once to
get multiple output formats.

-- 
Julian

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Sverre Rabbelier]

Heya,

On Sun, Apr 11, 2010 at 19:45, Julian Phillips <julian@quantumfyre.co.uk> wrote:
> I think that there probably are commands where it will be more work to
> integrate the output - but I think that is probably more to do with the
> structure of the current code than the API of the new.  Does it make a
> difference what the API of the new output code is if there isn't currently
> a sensible hook-in point?

No you are right, the existance of such hard-to-change commands does
not really affect the API design in this case, although I think it
might be a good idea to try out at least one such command before
committing to using this API. For example, it might turn out that
there's an elegant way to hook in, or that adding all those if
(output_style != OUTPUT_NORMAL)  statements gets cluttery and there
should be a different way to do things instead.

> If code has been written without the expectation that the output format
> could be changed then the effort to add a new output format could be
> considerably more than for status or ls-tree.  However, with the
> frontend/backend design hopefully we only have to endure the effort once to
> get multiple output formats.

I'm curious to see where this will lead us :).

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Junio C Hamano]

Julian Phillips <julian@quantumfyre.co.uk> writes:

> Add a library that allows commands to produce structured output in any
> of a range of formats using a single API.
>
> The API includes an OPT_OUTPUT and handle_output_arg so that the
> option handling for different commands will be as similar as possible.

I was hoping that the existing low-level -z routines (e.g. "diff-* -z")
follow similar enough patterns to have a corresponding output-z.c and be
handled inside output.c library.  But that is not a requirement, just
"would have been nicer if the original were written that way".

> diff --git a/output-json.c b/output-json.c
> new file mode 100644
> index 0000000..0eb66b2
> --- /dev/null
> +++ b/output-json.c
> @@ -0,0 +1,128 @@
> +#include "git-compat-util.h"
> +#include "output.h"
> +#include "strbuf.h"
> +
> +static char *json_quote(char *s)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	while (*s) {
> +		switch (*s) {
> +...
> +		default:
> +			/* All control characters must be encode, even if they
> +			 * don't have a specific escape character of their own */
> +			if (*s < 0x20)
> +				strbuf_addf(&buf, "\\u%04x", *s);

As you didn't say your "char" is either signed or unsigned upfront, this
will behave differently when you are fed a UTF-8 string.  If it is signed,
you will end up showing bytes in a single letter separately at wrong
codepoint, and if it is unsigned, you will give UTF-8 string unquoted,
which probably is what you meant to do.

What is your design intention regarding legacy encoding?  This code does
not yet declare "dear user, if you plan to use json/xml output, your
repository metadata (notably the pathnames) has to be in UTF-8", as the
caller _could_ transliterate legacy data before feeding it to output.c
layer.  An alternative would be for the output.c layer to know about the
encoding of incoming data and transliterate when the output format
requires a particular encoding.

> +static void json_obj_item_start(FILE *file, char *name, int first)
> +{
> +	char *quoted = json_quote(name);
> +	if (!first)
> +		fprintf(file, ",\n");
> +	fprintf(file, "\"%s\" : ", quoted);
> +	free(quoted);
> +}
> + ...
> +static void json_str(FILE *file, char *value)
> +{
> +	char *quoted = json_quote(value);
> +	fprintf(file, "\"%s\"", quoted);
> +	free(quoted);
> +}

An obvious improvement would be to make json_quote() to take FILE * to
avoid wasteful allocation and copy, as it doesn't do anything but addstr
and addch, and all of its callers don't do anything but spitting the
result out to FILE *.

> diff --git a/output-xml.c b/output-xml.c
> new file mode 100644
> index 0000000..50dd7d6
> --- /dev/null
> +++ b/output-xml.c
> @@ -0,0 +1,68 @@
> +#include "git-compat-util.h"
> +#include "output.h"

This seems to totally lack quoting of any metacharacters for "name" and
string "value".

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Sverre Rabbelier]

Heya,

On Sun, Apr 11, 2010 at 20:16, Junio C Hamano <gitster@pobox.com> wrote:
> I was hoping that the existing low-level -z routines (e.g. "diff-* -z")
> follow similar enough patterns to have a corresponding output-z.c and be
> handled inside output.c library.  But that is not a requirement, just
> "would have been nicer if the original were written that way".

I like that idea, I think it would make our plumbing interface more
consistent, and further validate the API design. Any plumbing command
(once converted) can then be used with -z (or --format=zero or
whatever it is) and give a similar output format, very nice.

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Julian Phillips]

On Sun, 11 Apr 2010 11:16:18 -0700, Junio C Hamano <gitster@pobox.com>
wrote:
> Julian Phillips <julian@quantumfyre.co.uk> writes:
> 
>> Add a library that allows commands to produce structured output in any
>> of a range of formats using a single API.
>>
>> The API includes an OPT_OUTPUT and handle_output_arg so that the
>> option handling for different commands will be as similar as possible.
> 
> I was hoping that the existing low-level -z routines (e.g. "diff-* -z")
> follow similar enough patterns to have a corresponding output-z.c and be
> handled inside output.c library.  But that is not a requirement, just
> "would have been nicer if the original were written that way".

As the API currently stands, I don't think it would be possible to
recreate the existing output of -z, as the separator between values is not
constant.  I haven't really looked into whether the output is completely
incompatible with structured output though (i.e. could -z be supported by
adding one or two functions to the API?).

>> diff --git a/output-json.c b/output-json.c
>> new file mode 100644
>> index 0000000..0eb66b2
>> --- /dev/null
>> +++ b/output-json.c
>> @@ -0,0 +1,128 @@
>> +#include "git-compat-util.h"
>> +#include "output.h"
>> +#include "strbuf.h"
>> +
>> +static char *json_quote(char *s)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +
>> +	while (*s) {
>> +		switch (*s) {
>> +...
>> +		default:
>> +			/* All control characters must be encode, even if they
>> +			 * don't have a specific escape character of their own */
>> +			if (*s < 0x20)
>> +				strbuf_addf(&buf, "\\u%04x", *s);
> 
> As you didn't say your "char" is either signed or unsigned upfront, this
> will behave differently when you are fed a UTF-8 string.  If it is
signed,
> you will end up showing bytes in a single letter separately at wrong
> codepoint, and if it is unsigned, you will give UTF-8 string unquoted,
> which probably is what you meant to do.

Oops. :(
Yep, unsigned it should have been.

> What is your design intention regarding legacy encoding?  This code does
> not yet declare "dear user, if you plan to use json/xml output, your
> repository metadata (notably the pathnames) has to be in UTF-8", as the
> caller _could_ transliterate legacy data before feeding it to output.c
> layer.  An alternative would be for the output.c layer to know about the
> encoding of incoming data and transliterate when the output format
> requires a particular encoding.

To be perfectly honest I had forgotten about encodings.  The code was
written with the thought that strings would be UTF-8 (except that even that
didn't work as you pointed out above).  Having English as your first
language doesn't help with this sort of thing.  I'm not even sure how to
create a file that has accented letters ... :$

Probably having the output_str function take UTF-8, and then having a
separate output_encoded_str that also takes an encoding might make sense? 
Unfortunately I have no idea how to convert an encoded string in git - a
quick grep suggests reenocde_string from utf8.h?

>> +static void json_obj_item_start(FILE *file, char *name, int first)
>> +{
>> +	char *quoted = json_quote(name);
>> +	if (!first)
>> +		fprintf(file, ",\n");
>> +	fprintf(file, "\"%s\" : ", quoted);
>> +	free(quoted);
>> +}
>> + ...
>> +static void json_str(FILE *file, char *value)
>> +{
>> +	char *quoted = json_quote(value);
>> +	fprintf(file, "\"%s\"", quoted);
>> +	free(quoted);
>> +}
> 
> An obvious improvement would be to make json_quote() to take FILE * to
> avoid wasteful allocation and copy, as it doesn't do anything but addstr
> and addch, and all of its callers don't do anything but spitting the
> result out to FILE *.

Yep, makes sense, thanks.

>> diff --git a/output-xml.c b/output-xml.c
>> new file mode 100644
>> index 0000000..50dd7d6
>> --- /dev/null
>> +++ b/output-xml.c
>> @@ -0,0 +1,68 @@
>> +#include "git-compat-util.h"
>> +#include "output.h"
> 
> This seems to totally lack quoting of any metacharacters for "name" and
> string "value".

Yep.  The XML output is still a long way from usable.  As I said in
another email, I mainly added it to see what different demands it placed on
the frontend/backend interface.  In future versions, I'll split out the XML
backend and try to make it clear that it's incomplete and only included in
the hope that someone who wants XML output takes it over ;).

-- 
Julian

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Jakub Narebski]

Julian Phillips <julian@quantumfyre.co.uk> writes:
> On Sun, 11 Apr 2010 11:16:18 -0700, Junio C Hamano <gitster@pobox.com>
> wrote:
>> Julian Phillips <julian@quantumfyre.co.uk> writes:
>> 
>>> Add a library that allows commands to produce structured output in any
>>> of a range of formats using a single API.
>>>
>>> The API includes an OPT_OUTPUT and handle_output_arg so that the
>>> option handling for different commands will be as similar as possible.
>> 
>> I was hoping that the existing low-level -z routines (e.g. "diff-* -z")
>> follow similar enough patterns to have a corresponding output-z.c and be
>> handled inside output.c library.  But that is not a requirement, just
>> "would have been nicer if the original were written that way".
> 
> As the API currently stands, I don't think it would be possible to
> recreate the existing output of -z, as the separator between values is not
> constant.  I haven't really looked into whether the output is completely
> incompatible with structured output though (i.e. could -z be supported by
> adding one or two functions to the API?).

What about the new(ly) proposed -Z output in one of its variants,
namely with single NUL ("\0") as field separator, and double NUL ("\0\0")
as a record terminator?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Julian Phillips]

On Sun, 11 Apr 2010 13:34:59 -0700 (PDT), Jakub Narebski
<jnareb@gmail.com>
wrote:
> Julian Phillips <julian@quantumfyre.co.uk> writes:
>> On Sun, 11 Apr 2010 11:16:18 -0700, Junio C Hamano <gitster@pobox.com>
>> wrote:
>>> Julian Phillips <julian@quantumfyre.co.uk> writes:
>>> 
>>>> Add a library that allows commands to produce structured output in
any
>>>> of a range of formats using a single API.
>>>>
>>>> The API includes an OPT_OUTPUT and handle_output_arg so that the
>>>> option handling for different commands will be as similar as
possible.
>>> 
>>> I was hoping that the existing low-level -z routines (e.g. "diff-*
-z")
>>> follow similar enough patterns to have a corresponding output-z.c and
be
>>> handled inside output.c library.  But that is not a requirement, just
>>> "would have been nicer if the original were written that way".
>> 
>> As the API currently stands, I don't think it would be possible to
>> recreate the existing output of -z, as the separator between values is
>> not
>> constant.  I haven't really looked into whether the output is
completely
>> incompatible with structured output though (i.e. could -z be supported
by
>> adding one or two functions to the API?).
> 
> What about the new(ly) proposed -Z output in one of its variants,
> namely with single NUL ("\0") as field separator, and double NUL
("\0\0")
> as a record terminator?

That should be much easier.  Though actually I am fairly close to getting
_all_ output from ls-tree going through the output library ... (i.e. even
the normal no-option output).  I don't know what people's opinion on this
approach is, but I thought it was worth a try anyway.

-- 
Julian

Re: [RFC/PATCH 2/3] add a library of code for producing structured output

[Eric Raymond]

Julian Phillips <julian@quantumfyre.co.uk>:
> That should be much easier.  Though actually I am fairly close to getting
> _all_ output from ls-tree going through the output library ... (i.e. even
> the normal no-option output).  I don't know what people's opinion on this
> approach is, but I thought it was worth a try anyway.

I make encouraging noises in your direction.  This approach sounds
like an orthogonality and consistency win.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Jon Seymour]

I can see that retrofitting this more widely would add quite a lot of
conditional logic to a lot of places.

If one was designing for both line-oriented and structured outputs
from the start, I imagine one would build a map for each record, and
then hand that to the output context when it is complete, allowing the
considerations of both line orientation and structured output to be
encapsulated within the backend. Self-describing output formats can
use a simple map without needing to know the record type, but line
oriented outputs, of course, would need to know the type of record in
order to select the correct line formatter.

So, would it be worth providing a hint as to record type in the
output_start_object call so that if it was later desired to subsume
line-oriented formats under the same framework, there is enough
information available to the backend to do that?

[ And, yes, I understand that to making line-oriented formats a
backend would be a reasonably invasive change to existing code that
would involve a level of indirection and abstraction that may not be
to everyone's taste. ]

jon.

On Mon, Apr 12, 2010 at 3:50 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Sun, Apr 11, 2010 at 19:45, Julian Phillips <julian@quantumfyre.co.uk> wrote:
>> I think that there probably are commands where it will be more work to
>> integrate the output - but I think that is probably more to do with the
>> structure of the current code than the API of the new.  Does it make a
>> difference what the API of the new output code is if there isn't currently
>> a sensible hook-in point?
>
> No you are right, the existance of such hard-to-change commands does
> not really affect the API design in this case, although I think it
> might be a good idea to try out at least one such command before
> committing to using this API. For example, it might turn out that
> there's an elegant way to hook in, or that adding all those if
> (output_style != OUTPUT_NORMAL)  statements gets cluttery and there
> should be a different way to do things instead.
>
>> If code has been written without the expectation that the output format
>> could be changed then the effort to add a new output format could be
>> considerably more than for status or ls-tree.  However, with the
>> frontend/backend design hopefully we only have to endure the effort once to
>> get multiple output formats.
>
> I'm curious to see where this will lead us :).
>
> --
> Cheers,
>
> Sverre Rabbelier
> --
> 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
>

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Eric Raymond]

Jon Seymour <jon.seymour@gmail.com>:
> [ And, yes, I understand that to making line-oriented formats a
> backend would be a reasonably invasive change to existing code that
> would involve a level of indirection and abstraction that may not be
> to everyone's taste. ]

For whatever my opinion is worth I think this is a good direction to
go in.  I think it fits the well-established git design philosophy of
separating content manipulation (plumbing) from presentation
(porcelain).
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Jon Seymour]

On 12/04/2010, at 8:22, Jon Seymour <jon.seymour@gmail.com> wrote:
>
> So, would it be worth providing a hint as to record type in the
> output_start_object call so that if it was later desired to subsume
> line-oriented formats under the same framework, there is enough
> information available to the backend to do that?

Of course, one way to do this would be to use a more descriptive  
record name than "entry". This would make the record itself (as  
opposed to just it's fields) self-describing.

The point is, you would want to start using descriptive record names  
now so that you don't end up locked into a partially context sensitive  
base of consumers who are expecting their JSON records to be called  
"entry" and using context hints to infer the actual record type.

jon.

Re: [RFC/PATCH 0/3] JSON/XML output for scripting interface

[Julian Phillips]

On Mon, 12 Apr 2010 09:25:04 +1000, Jon Seymour <jon.seymour@gmail.com>
wrote:
> On 12/04/2010, at 8:22, Jon Seymour <jon.seymour@gmail.com> wrote:
>>
>> So, would it be worth providing a hint as to record type in the
>> output_start_object call so that if it was later desired to subsume
>> line-oriented formats under the same framework, there is enough
>> information available to the backend to do that?
> 
> Of course, one way to do this would be to use a more descriptive  
> record name than "entry". This would make the record itself (as  
> opposed to just it's fields) self-describing.
> 
> The point is, you would want to start using descriptive record names  
> now so that you don't end up locked into a partially context sensitive  
> base of consumers who are expecting their JSON records to be called  
> "entry" and using context hints to infer the actual record type.

I have to admit that most of the names were just "first idea out of the
hat" - not really something I was paying too much attention to.  It's
fairly easy to tweak them later, provided it's done before they get
published.

Having said that, I've just mailed out v2 patches, which already include
line-based output (using a different approach). ;)

More descriptive names are probably something that should be done anyway
though.

-- 
Julian