Re: [PATCH 0/3] Generalized "string function" syntax
Show 8 quoted lines
> 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(-)
Show changes to pretty.c +150 −4
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 */
}
Junio C Hamano
