Re: [PATCH 0/3] Generalized "string function" syntax

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>> I'm more in favour of adding ways to customize the shape of the elements
>> rather than adding string functions.  %S(width=76,indent=4) over
>> %[wrap(76,4)%s%].
> 
> Yeah, %X(some modifier) that can apply to any 'X' looks much simpler and
> easier to look at.  The way the code is structured currently it might be
> more work and risk to break things, though.

Here's something along this line.  It's an experiment that tries to
explore named parameters and extending individual place holders instead of
adding a generic modifier (%w).

The patch implements a way to pass named parameters to %s and %b.  Those
parameters are width, indent1 and indent2, which are then passed to
strbuf_add_wrapped_text() to indent and wrap the subject or body text,
respectively.

Handling wrapping for the individual place holders avoids having to deal
with terminal colour codes.  The patch that implements %w() currently in
next ignores this issue.

It also allows to avoid copying the results around -- no temporary strbuf
is needed for %b().  However, quick tests showed no measurable performance
improvement.

While indent1 and indent2 are numerical parameters in this patch, they
really should be strings, in order to allow indenting using tabs etc..
For that to work, strbuf_add_wrapped_text() would need to be changed
first, though.

The parser parse_params() supports strings and numerical values, but only
the latter are used in this patch.  String parameters are intended to be
fed to unquote_c_style().  It never complains about syntax errors.  It
aborts if the string ends prematurely, but otherwise ignores invalid input
and just keeps going.  That's how the % place holder parser has always
worked, but perhaps the introduction of named parameters justifies a more
strict approach?


My main motivation for this experiment was to avoid extra copies in the
hope to speed things up.  However, my basic timing tests show that it's
not that much of an improvement.

The remaining reason is the handling of colour codes.  I think we can keep
ignoring this issue -- the only impact is that lines with colour codes and
wrapping combined (e.g. "%w(72)%Cred%s") shorter than they should be,
because the colour code is considered (incorrectly) to have a non-zero
width.  I think we can get away with mentioning that fact in the docs.
After all, one can simple use "%Cred%w(72)%s".

Anyway, here's the patch.


 pretty.c |  154 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 150 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index da15cf2..09272ec 100644
--- a/pretty.c
+++ b/pretty.c
@@ -596,6 +596,117 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
 		strbuf_addch(sb, ')');
 }
 
+struct param {
+	const char *name;
+	long *numeric_value;
+	const char *value;
+	size_t value_len;
+};
+
+static size_t prefixlen(const char *s, const char *prefix)
+{
+	size_t len = 0;
+
+	while (*prefix) {
+		if (*s != *prefix)
+			return 0;
+		s++;
+		prefix++;
+		len++;
+	}
+	return len;
+}
+
+static size_t parse_params(const char *s, struct param *params, int count,
+			   int end)
+{
+	const char *start = s;
+	int i;
+
+	for (;;) {
+		while (isspace(*s))
+			s++;
+again:
+		if (*s == end)
+			return s - start + 1;
+		if (*s == '\0')
+			return 0;
+
+		for (i = 0; i < count; i++) {
+			size_t len = prefixlen(s, params[i].name);
+			if (len) {
+				if (s[len] == end)
+					return s - start + 1;
+				if (s[len] == '\0')
+					return 0;
+				if (isspace(s[len])) {
+					s += len + 1;
+					while (isspace(*s))
+						s++;
+					if (*s != '=')
+						goto again;
+					s++;
+					break;
+				} else if (s[len] == '=') {
+					s += len + 1;
+					break;
+				}
+			}
+		}
+
+		if (i < count) {
+			while (isspace(*s))
+				s++;
+			if (*s == end)
+				return s - start + 1;
+			if (*s == '\0')
+				return 0;
+
+			params[i].value = s;
+			if (*s == '"') {
+				for (;;) {
+					int quoted = 0;
+					const char *q;
+
+					s = strchr(s + 1, '"');
+					if (!s)
+						return 0;
+
+					for (q = s - 1; *q == '\\'; q--)
+						quoted = !quoted;
+					if (!quoted)
+						break;
+				}
+				s++;
+			} else {
+				char *endp;
+				long num = strtol(s, &endp, 10);
+
+				s = endp;
+				if (isspace(*s) || *s == end) {
+					if (params[i].numeric_value)
+						*params[i].numeric_value = num;
+				} else {
+					while (!isspace(*s) && *s != end) {
+						if (*s == '\0')
+							return 0;
+						s++;
+					}
+				}
+			}
+			params[i].value_len = s - params[i].value;
+		} else {
+			while (!isspace(*s)) {
+				if (*s == end)
+					return s - start + 1;
+				if (*s == '\0')
+					return 0;
+				s++;
+			}
+		}
+	}
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -744,14 +855,49 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 
 	switch (placeholder[0]) {
 	case 's':	/* subject */
-		format_subject(sb, msg + c->subject_off, " ");
-		return 1;
+		if (placeholder[1] == '(') {
+			struct strbuf subject = STRBUF_INIT;
+			long indent1 = 0, indent2 = 0, width = 0;
+			struct param params[] = {
+				{ "indent1", &indent1 },
+				{ "indent2", &indent2 },
+				{ "width", &width },
+			};
+			size_t consumed = parse_params(placeholder + 2, params,
+						       ARRAY_SIZE(params), ')');
+			if (!consumed)
+				return 0;
+			format_subject(&subject, msg + c->subject_off, " ");
+			strbuf_add_wrapped_text(sb, subject.buf, indent1,
+						indent2, width);
+			strbuf_release(&subject);
+			return consumed + 2;
+		} else {
+			format_subject(sb, msg + c->subject_off, " ");
+			return 1;
+		}
 	case 'f':	/* sanitized subject */
 		format_sanitized_subject(sb, msg + c->subject_off);
 		return 1;
 	case 'b':	/* body */
-		strbuf_addstr(sb, msg + c->body_off);
-		return 1;
+		if (placeholder[1] == '(') {
+			long indent1 = 0, indent2 = 0, width = 0;
+			struct param params[] = {
+				{ "indent1", &indent1 },
+				{ "indent2", &indent2 },
+				{ "width", &width },
+			};
+			size_t consumed = parse_params(placeholder + 2, params,
+						       ARRAY_SIZE(params), ')');
+			if (!consumed)
+				return 0;
+			strbuf_add_wrapped_text(sb, msg + c->body_off, indent1,
+						indent2, width);
+			return consumed + 2;
+		} else {
+			strbuf_addstr(sb, msg + c->body_off);
+			return 1;
+		}
 	}
 	return 0;	/* unknown placeholder */
 }