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

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

[Junio C Hamano]

I mentioned an idea to enhance the pretty=format language with a string
function syntax that people can extend by adding new functions in one of
the "What's cooking" messages earlier.  The general syntax would be like

    %[function(args...)any string here%]

where "any string here" part would have the usual pretty=format strings.
E.g.  git show -s --format='%{w(72,8,4)%s%+b%]' should give you a line
wrapped commit log message if w(width,in1,in2) is such a function.

This series is a proof of concept, as I didn't actually plug the
"wrapping" code into it; it would be fairly straightforward to integrate
the logic Dscho made strbuf capable in js/log-wrap series (queued in 'pu')
to finish this.

Junio C Hamano (3):
  format_commit_message(): fix function signature
  strbuf_nested_expand(): allow expansion to interrupt in the middle
  Add proof-of-concept %[w(width,in1,in2)<<any-string>>%]
    implementation

 commit.h |    2 +-
 pretty.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 strbuf.c |   23 +++++++++++++---
 strbuf.h |    3 +-
 4 files changed, 107 insertions(+), 7 deletions(-)

[PATCH 1/3] format_commit_message(): fix function signature

[Junio C Hamano]

The format template string was declared as "const void *" for some unknown
reason, even though it obviously is meant to be passed a string.  Make it
"const char *".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.h |    2 +-
 pretty.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/commit.h b/commit.h
index f4fc5c5..95f981a 100644
--- a/commit.h
+++ b/commit.h
@@ -70,7 +70,7 @@ extern char *reencode_commit_message(const struct commit *commit,
 				     const char **encoding_p);
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern void format_commit_message(const struct commit *commit,
-				  const void *format, struct strbuf *sb,
+				  const char *format, struct strbuf *sb,
 				  enum date_mode dmode);
 extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit*,
                                 struct strbuf *,
diff --git a/pretty.c b/pretty.c
index f5983f8..587101f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -740,7 +740,7 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 }
 
 void format_commit_message(const struct commit *commit,
-			   const void *format, struct strbuf *sb,
+			   const char *format, struct strbuf *sb,
 			   enum date_mode dmode)
 {
 	struct format_commit_context context;
-- 
1.6.5.99.g9ed7e

Re: [PATCH 1/3] format_commit_message(): fix function signature

[René Scharfe]

Junio C Hamano schrieb:
> The format template string was declared as "const void *" for some unknown
> reason, even though it obviously is meant to be passed a string.  Make it
> "const char *".

Yes.  I seem to have introduced that type in commit 7b95089c, but I
can't see (even less remember) why.  Also note that commit message and
header file call the parameter "template", while it's called "format" in
commit.c (where the function used to live back then).  What was I thinking?

Thanks for cleaning up after me.

René

[PATCH 2/3] strbuf_nested_expand(): allow expansion to interrupt in the middle

[Junio C Hamano]

This itself does not do a "nested" expansion, but it paves a way for
supporting an extended syntax to express a function that works on an
expanded substring, e.g. %[function(param...)expanded-string%], by
allowing the callback function to tell where the argument to the function
ends.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 strbuf.c |   23 +++++++++++++++++++----
 strbuf.h |    3 ++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index a6153dc..2bbc49c 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -214,25 +214,40 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_setlen(sb, sb->len + len);
 }
 
-void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
-		   void *context)
+void strbuf_nested_expand(struct strbuf *sb, const char **format_p,
+			  expand_fn_t fn, void *context)
 {
+	const char *format = *format_p;
 	for (;;) {
 		const char *percent;
 		size_t consumed;
 
 		percent = strchrnul(format, '%');
 		strbuf_add(sb, format, percent - format);
+		format = percent;
 		if (!*percent)
 			break;
-		format = percent + 1;
+		format++;
 
 		consumed = fn(sb, format, context);
-		if (consumed)
+		if ((ssize_t) consumed < 0)
+			break;
+		else if (consumed)
 			format += consumed;
 		else
 			strbuf_addch(sb, '%');
 	}
+	*format_p = format;
+}
+
+void strbuf_expand(struct strbuf *sb, const char *o_format, expand_fn_t fn,
+		   void *context)
+{
+	const char *format = o_format;
+	strbuf_nested_expand(sb, &format, fn, context);
+	if (*format)
+		die("format error: negative return from expand function: %s",
+		    o_format);
 }
 
 size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
diff --git a/strbuf.h b/strbuf.h
index d05e056..e602899 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -109,8 +109,9 @@ static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) {
 }
 extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
 
-typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
+typedef size_t (*expand_fn_t)(struct strbuf *sb, const char *placeholder, void *context);
 extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);
+extern void strbuf_nested_expand(struct strbuf *sb, const char **format_p, expand_fn_t fn, void *context);
 struct strbuf_expand_dict_entry {
 	const char *placeholder;
 	const char *value;
-- 
1.6.5.99.g9ed7e

Re: [PATCH 2/3] strbuf_nested_expand(): allow expansion to interrupt in the middle

[Johannes Schindelin]

Hi,

On Fri, 16 Oct 2009, Junio C Hamano wrote:

>  		consumed = fn(sb, format, context);
> -		if (consumed)
> +		if ((ssize_t) consumed < 0)
> +			break;

Would it not be much better to fix the signature of fn in a separate 
commit before this one?

Ciao,
Dscho

Re: [PATCH 2/3] strbuf_nested_expand(): allow expansion to interrupt in the middle

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 16 Oct 2009, Junio C Hamano wrote:
>
>>  		consumed = fn(sb, format, context);
>> -		if (consumed)
>> +		if ((ssize_t) consumed < 0)
>> +			break;
>
> Would it not be much better to fix the signature of fn in a separate 
> commit before this one?

Yes, I considered it and it is a reasonable thing to do if this were to
become a real series for includion, but I thought it would add unnecessary
noise to the patch when the main purpose of posting the series is still to
be a proof-of-concept for discussing the design and future directions
(including "it should not have any future---it is useless code churn for
supporting only one example user 'rewrap'").

Please remind and yell at me if (1) this turns out to be going in the
right direction and (2) I forget to fix it when I redo the series after
discussion to apply them for real.

Thanks.

[PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Junio C Hamano]

This uses the strbuf_nested_expand() mechanism introduced earlier
to demonstrate how to implement a nested string function.  It does
not "wrap" using the line-wrap code, but lifting the change by Dscho
and plugging it in should be a trivial exercise.

The overall idea is to parse something like "%[w(72,4,8)%an %ae %s%]" in
these steps:

  #1 "%[" introduces the nested string function.

  #2 After that, a name identifies what function to call.

  #3 The function parses its parameters ("(72,4,8)" in the above example),
     and makes a nested expansion on the remainder of the format string.

  #4 The nested expansion is terminated at "%]" and returned to the
     function.

  #5 The function massages the string returned from #4, and the result
     becomes the expansion of the whole thing.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pretty.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/pretty.c b/pretty.c
index 587101f..a8a38c3 100644
--- a/pretty.c
+++ b/pretty.c
@@ -595,6 +595,80 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
 		strbuf_addch(sb, ')');
 }
 
+typedef int (*string_fmt_fn)(struct strbuf *sb, const char *placeholder, void *context);
+static size_t format_commit_item(struct strbuf *, const char *, void *);
+
+static int wrap_fn(struct strbuf *sb, const char *placeholder, void *context)
+{
+	const char *template = placeholder;
+	char *endptr;
+	long width = 0, indent1 = 0, indent2 = 0;
+
+	width = strtol(template, &endptr, 10);
+	if (*endptr == ',') {
+		template = endptr + 1;
+		indent1 = strtol(template, &endptr, 10);
+		if (*endptr == ',') {
+			template = endptr + 1;
+			indent2 = strtol(template, &endptr, 10);
+		}
+	}
+	if (*endptr++ != ')')
+		return 0;
+
+	template = endptr;
+	strbuf_nested_expand(sb, &template, format_commit_item, context);
+	if (*template++ != ']')
+		return 0;
+
+	/*
+	 * NEEDSWORK: here you wrap the contents of substr with
+	 * strbuf_wrap(&substr, width, indent1, indent2);
+	 *
+	 * ... but I am too lazy to do that here, and I just demonstrate
+	 * how it should work by just upcasing the result ;-)
+	 */
+	{
+		int i;
+		for (i = 0; i < sb->len; i++)
+			sb->buf[i] = toupper(sb->buf[i]);
+	}
+	return template - placeholder;
+}
+
+static struct {
+	const char *name;
+	string_fmt_fn fn;
+} format_fn_list[] = {
+	{ "w(", wrap_fn }
+};
+
+static size_t format_fn(struct strbuf *sb, const char *placeholder,
+			void *context)
+{
+	const char *template = placeholder;
+	size_t consumed;
+	struct strbuf substr = STRBUF_INIT;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(format_fn_list); i++)
+		if (!prefixcmp(template, format_fn_list[i].name))
+			break;
+	if (ARRAY_SIZE(format_fn_list) <= i)
+		return 0;
+	template += strlen(format_fn_list[i].name);
+	consumed = format_fn_list[i].fn(&substr, template, context);
+	if (!consumed) {
+		strbuf_release(&substr);
+		return 0;
+	}
+
+	strbuf_add(sb, substr.buf, substr.len);
+	template += consumed;
+	strbuf_release(&substr);
+	return template - placeholder;
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -603,9 +677,19 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	const char *msg = commit->buffer;
 	struct commit_list *p;
 	int h1, h2;
+	size_t nested;
 
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
+	case ']':
+		return -1;
+	case '[':
+		/*
+		 * %[func(arg...) string %]: we consumed the opening '['
+		 * and the callee consumed up to the closing '%]'.
+		 */
+		nested = format_fn(sb, placeholder + 1, context);
+		return nested ? 1 + nested : 0;
 	case 'C':
 		if (placeholder[1] == '(') {
 			const char *end = strchr(placeholder + 2, ')');
-- 
1.6.5.99.g9ed7e

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Johannes Schindelin]

Hi,

maybe "rewrap" would be a better name than "w"?

On Fri, 16 Oct 2009, Junio C Hamano wrote:

>   #1 "%[" introduces the nested string function.
> 
>   #2 After that, a name identifies what function to call.
> 
>   #3 The function parses its parameters ("(72,4,8)" in the above example),
>      and makes a nested expansion on the remainder of the format string.

Can't we parse it once, i.e. the first time?

Ciao,
Dscho

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> maybe "rewrap" would be a better name than "w"?

Perhaps, but I do not know if wrap() is even better.  The only reason I
said w() here is because I saw you used w() and this is meant to be a
superset replacement for it, as this can re-wrap anything, not just one
particular field from the commit object.

> On Fri, 16 Oct 2009, Junio C Hamano wrote:
>
>>   #1 "%[" introduces the nested string function.
>> 
>>   #2 After that, a name identifies what function to call.
>> 
>>   #3 The function parses its parameters ("(72,4,8)" in the above example),
>>      and makes a nested expansion on the remainder of the format string.
>
> Can't we parse it once, i.e. the first time?

I may be missing the issue you are raising here, but it parses the string
only once; it stuffs the expansion of the enclosed string into a separate
buffer (while noting where it ends), applies the function to the result
obtained in the separate buffer and appends the result of the function
application to the main buffer, and the main expansion resumes where the
nested one finished.

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> maybe "rewrap" would be a better name than "w"?
> 
> Perhaps, but I do not know if wrap() is even better.  The only reason I
> said w() here is because I saw you used w() and this is meant to be a
> superset replacement for it, as this can re-wrap anything, not just one
> particular field from the commit object.

wrap(initial_prefix, subsequent_prefix, columns) would for example
follow somewhat Text::Wrap syntax, together with 'fill'.  With columns
being 0, not set, or negative it could simply indent result; this way
we finally would be able to get default git-log / git-rev-list format
using --pretty format.

I don't remember what were original parameters to w(72,4,8) means...

> 
>> On Fri, 16 Oct 2009, Junio C Hamano wrote:
>>
>>>   #1 "%[" introduces the nested string function.
>>> 
>>>   #2 After that, a name identifies what function to call.
>>> 
>>>   #3 The function parses its parameters ("(72,4,8)" in the above example),
>>>      and makes a nested expansion on the remainder of the format string.
>>
>> Can't we parse it once, i.e. the first time?
> 
> I may be missing the issue you are raising here, but it parses the string
> only once; it stuffs the expansion of the enclosed string into a separate
> buffer (while noting where it ends), applies the function to the result
> obtained in the separate buffer and appends the result of the function
> application to the main buffer, and the main expansion resumes where the
> nested one finished.

Do I understand it correctly that generic syntax is the following:

  %[function(params) format specifiers]

which would run given function, with given extra parameters, on the
result of expansion of the rest of the group?  That is a very
powerfull syntax... I wonder how other tools solved such problem...

BTW. can we have this also for git-for-each-ref format parameter?


Note that for single parameter we have different syntax (for
git-for-each-ref), namely

  %(field:modifier)

which could be expanded to allow for parametrized modifiers with one
of the following:

  %(field:modifier=param)
  %(field:modifier[param])
  %(field:modifier(param))

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> I don't remember what were original parameters to w(72,4,8) means...

"man git-shortlog" look for -w.

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Jakub Narebski]

On Fri, 16 Oct 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > I don't remember what were original parameters to w(72,4,8) means...
> 
> "man git-shortlog" look for -w.

Thanks.  So those would be:

-w[<width>[,<indent1>[,<indent2>]]]::
        Linewrap the output by wrapping each line at `width`.  The first
        line of each entry is indented by `indent1` spaces, and the second
        and subsequent lines are indented by `indent2` spaces. `width`,
        `indent1`, and `indent2` default to 76, 6 and 9 respectively.

I think better solution might be to give _string_ to use for initial and
subsequent indent rather than number of spaces... well, at least more
generic one, allowing one to use e.g. "\t" (TAB) character to indent,
or indent in the following way:

 * Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod
   tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
   veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea
   commodo consequat. [...]

But even with original w(<width>,<indent1>,<indent2>) we can get output
of bare "git log" using pretty format... well, almost; it would be the
same if there was ability to put infinite width, and there doesn't seem
to be specifier for the whole, unchanged commit message (subject,
unwrapped + separating lines + body).

-- 
Jakub Narebski
Poland

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> I think better solution might be to give _string_ to use for initial and
> subsequent indent rather than number of spaces... well, at least more
> generic one, allowing one to use e.g. "\t" (TAB) character to indent,
> or indent in the following way:

I do not see there is anything _better_ or _worse_.  It may simply be a
useful _addition_.  I'd usually say "care to send in a patch?" here, but
given that the general framework and the "wrap like the shortlog" hasn't
been plugged-in yet, there is no need to rush.

> But even with original w(<width>,<indent1>,<indent2>) we can get output
> of bare "git log" using pretty format... well, almost; it would be the
> same if there was ability to put infinite width, and there doesn't seem
> to be specifier for the whole, unchanged commit message (subject,
> unwrapped + separating lines + body).

I think I already discussed this when I sent out %s%+b patch.  You would
need to adjust and apply both series, but essentially it would become
something like:

    %s%+[w(-1,4,4)%b]

I.e. a single subject line, potentially followed by a LF and body indented
by 4-place, but the LF will be there only when the body is not empty.

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Jakub Narebski]

On Sat, 17 Oct 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > But even with original w(<width>,<indent1>,<indent2>) we can get output
> > of bare "git log" using pretty format... well, almost; it would be the
> > same if there was ability to put infinite width, and there doesn't seem
> > to be specifier for the whole, unchanged commit message (subject,
> > unwrapped + separating lines + body).
> 
> I think I already discussed this when I sent out %s%+b patch.  You would
> need to adjust and apply both series, but essentially it would become
> something like:
> 
>     %s%+[w(-1,4,4)%b]
> 
> I.e. a single subject line, potentially followed by a LF and body indented
> by 4-place, but the LF will be there only when the body is not empty.

Why not

    %[w(-1,4,4)%s%+b]

Or is it

    %[w(-1,4,4)%s%+%b]

(i.e. %+ is this empty line between subject and body, if it exists).

The %+x seems a bit strange... but I guess implementing conditional
expansion a la shell or rpn spec/queryformat would be out of question
(i.e. %?s:+ )...
-- 
Jakub Narebski
Poland

Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<<any-string>>%] implementation

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Why not
>
>     %[w(-1,4,4)%s%+b]

Yeah.  As you said "subject unwrapped and body indented" or something like
that, I excluded that part outside the indentation, but log output indents
everything, so placing both inside %[w] would be correct.

> (i.e. %+ is this empty line between subject and body, if it exists).

As %+ is to add LF iff the next expansion is non-empty, it must always be
followed by an expansion, i.e. %something.  That means "%+%something" is
unnecessary verbose and "%+something" is enough.  "%+anything" syntax,
just like "%[func()anything]" syntax, is not limited to any particular
expansion.

> The %+x seems a bit strange... but I guess implementing conditional
> expansion a la shell or rpn spec/queryformat would be out of question
> (i.e. %?s:+ )...

Why not?

I can see us doing something like %[conditional%|iftrue%|iffalse%] quite
easily, now we have the "nested" variant of the strbuf_expand()
infrastructure.

But that is not the primary focus of the two patch series I've
demonstrated so far.

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

[René Scharfe]

Junio C Hamano schrieb:
> I mentioned an idea to enhance the pretty=format language with a
> string function syntax that people can extend by adding new functions
> in one of the "What's cooking" messages earlier.  The general syntax
> would be like
> 
> %[function(args...)any string here%]
> 
> where "any string here" part would have the usual pretty=format
> strings. E.g.  git show -s --format='%{w(72,8,4)%s%+b%]' should give
> you a line wrapped commit log message if w(width,in1,in2) is such a
> function.

I pondered line wrapping with format strings briefly a long time ago, and
I always considered it to be more similar to a colour, i.e. a state that
one can change and that is applied to all following text until the next
state change.  (Except that it's always reset at the end of the format
string.)  The example above would then turn into '%w(72,8,4)%s%+b'.

Here's a patch to implement this behaviour.  It leaves the implementation
of the actual wrap function as an exercise to the reader, too. ;-)


 pretty.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/pretty.c b/pretty.c
index f5983f8..33464f1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -445,6 +445,7 @@ struct format_commit_context {
 	enum date_mode dmode;
 	unsigned commit_header_parsed:1;
 	unsigned commit_message_parsed:1;
+	size_t width, indent1, indent2;
 
 	/* These offsets are relative to the start of the commit message. */
 	struct chunk author;
@@ -458,6 +459,7 @@ struct format_commit_context {
 	struct chunk abbrev_commit_hash;
 	struct chunk abbrev_tree_hash;
 	struct chunk abbrev_parent_hashes;
+	size_t wrap_start;
 };
 
 static int add_again(struct strbuf *sb, struct chunk *chunk)
@@ -595,6 +597,44 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
 		strbuf_addch(sb, ')');
 }
 
+static void strbuf_wrap(struct strbuf *sb, size_t pos, size_t len,
+			size_t width, size_t indent1, size_t indent2)
+{
+	struct strbuf tmp = STRBUF_INIT;
+
+	if (!len)
+		return;
+	if (pos)
+		strbuf_add(&tmp, sb->buf, pos);
+
+	/* XXX: all that's missing is the wrapping code itself.. */
+	strbuf_addf(&tmp, "w(%u,%u,%u)[\n",
+		    (unsigned)width, (unsigned)indent1, (unsigned)indent2);
+	strbuf_add(&tmp, sb->buf + pos, len);
+	strbuf_addstr(&tmp, "\n]");
+
+	if (pos + len < sb->len)
+		strbuf_add(&tmp, sb->buf + pos + len, sb->len - pos - len);
+	strbuf_swap(&tmp, sb);
+	strbuf_release(&tmp);
+}
+
+static void rewrap_message_tail(struct strbuf *sb,
+				struct format_commit_context *c,
+				size_t new_width, size_t new_indent1,
+				size_t new_indent2)
+{
+	if (c->width == new_width && c->indent1 == new_indent1 &&
+	    c->indent2 == new_indent2)
+		return;
+	strbuf_wrap(sb, c->wrap_start, sb->len - c->wrap_start, c->width,
+		    c->indent1, c->indent2);
+	c->wrap_start = sb->len;
+	c->width = new_width;
+	c->indent1 = new_indent1;
+	c->indent2 = new_indent2;
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -645,6 +685,30 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 			return 3;
 		} else
 			return 0;
+	case 'w':
+		if (placeholder[1] == '(') {
+			unsigned long width = 0, indent1 = 0, indent2 = 0;
+			char *next;
+			const char *start = placeholder + 2;
+			const char *end = strchr(start, ')');
+			if (!end)
+				return 0;
+			if (end > start) {
+				width = strtoul(start, &next, 10);
+				if (*next == ',') {
+					indent1 = strtoul(next + 1, &next, 10);
+					if (*next == ',') {
+						indent2 = strtoul(next + 1,
+								 &next, 10);
+					}
+				}
+				if (*next != ')')
+					return 0;
+			}
+			rewrap_message_tail(sb, c, width, indent1, indent2);
+			return end - placeholder + 1;
+		} else
+			return 0;
 	}
 
 	/* these depend on the commit */
@@ -748,7 +812,9 @@ void format_commit_message(const struct commit *commit,
 	memset(&context, 0, sizeof(context));
 	context.commit = commit;
 	context.dmode = dmode;
+	context.wrap_start = sb->len;
 	strbuf_expand(sb, format, format_commit_item, &context);
+	rewrap_message_tail(sb, &context, 0, 0, 0);
 }
 
 static void pp_header(enum cmit_fmt fmt,

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

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Junio C Hamano schrieb:
>> I mentioned an idea to enhance the pretty=format language with a
>> string function syntax that people can extend by adding new functions
>> in one of the "What's cooking" messages earlier.  The general syntax
>> would be like
>> 
>> %[function(args...)any string here%]
>> 
>> where "any string here" part would have the usual pretty=format
>> strings. E.g.  git show -s --format='%{w(72,8,4)%s%+b%]' should give
>> you a line wrapped commit log message if w(width,in1,in2) is such a
>> function.
>
> I pondered line wrapping with format strings briefly a long time ago, and
> I always considered it to be more similar to a colour, i.e. a state that
> one can change and that is applied to all following text until the next
> state change.  (Except that it's always reset at the end of the format
> string.)  The example above would then turn into '%w(72,8,4)%s%+b'.

As a syntax to express "wrapping" behaviour alone, I think this is much
simpler and more superiour.  I guess with this if you want to wrap
something to 72 columns and then wrap something else to 66 columns, you
would write '%w(72,8,4)something%w(66,8,4)something else', right?

I used %] only for two reasons.

 - Without an explicit "here it ends", I couldn't come up with a good way
   to express '%[w(72,8,4)something%]something else'.  IOW, how I can say
   "wrap something to 72 columns and then place something else without any
   wrapping"?
   
 - When we need to support more than one string function like this, it is
   unclear what '%f()one string%g()another one' in your syntax means.
   Does it mean '%[f()one string%]%[g()another one%]' (i.e. concatenate
   the result of applying string function f to 'one string' and the result
   of applying string function g to 'another one')?  Or does it mean
   '%[f()one string%[g()another one%]%]' (apply 'f' to concatenation of
   'one string' and the result of applying 'g' to 'another one')?

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

[René Scharfe]

Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Junio C Hamano schrieb:
>>> I mentioned an idea to enhance the pretty=format language with a
>>> string function syntax that people can extend by adding new functions
>>> in one of the "What's cooking" messages earlier.  The general syntax
>>> would be like
>>>
>>> %[function(args...)any string here%]
>>>
>>> where "any string here" part would have the usual pretty=format
>>> strings. E.g.  git show -s --format='%{w(72,8,4)%s%+b%]' should give
>>> you a line wrapped commit log message if w(width,in1,in2) is such a
>>> function.
>> I pondered line wrapping with format strings briefly a long time ago, and
>> I always considered it to be more similar to a colour, i.e. a state that
>> one can change and that is applied to all following text until the next
>> state change.  (Except that it's always reset at the end of the format
>> string.)  The example above would then turn into '%w(72,8,4)%s%+b'.
> 
> As a syntax to express "wrapping" behaviour alone, I think this is much
> simpler and more superiour.  I guess with this if you want to wrap
> something to 72 columns and then wrap something else to 66 columns, you
> would write '%w(72,8,4)something%w(66,8,4)something else', right?

That's right.

> I used %] only for two reasons.
> 
>  - Without an explicit "here it ends", I couldn't come up with a good way
>    to express '%[w(72,8,4)something%]something else'.  IOW, how I can say
>    "wrap something to 72 columns and then place something else without any
>    wrapping"?

My patch makes '%w()' reset the wrapping parameters to their defaults.

>  - When we need to support more than one string function like this, it is
>    unclear what '%f()one string%g()another one' in your syntax means.
>    Does it mean '%[f()one string%]%[g()another one%]' (i.e. concatenate
>    the result of applying string function f to 'one string' and the result
>    of applying string function g to 'another one')?  Or does it mean
>    '%[f()one string%[g()another one%]%]' (apply 'f' to concatenation of
>    'one string' and the result of applying 'g' to 'another one')?

I was going to say that we already have something like that with %C, and
that the natural way (to me) is to apply them both, independently.  Case
modification functions (upper, lower, capitalized) could be treated the
same way -- as state changes (like pressing caps lock when typing text).

Which other text functions are we going to add which would break this
model?  The only thing I can think of right now is nesting such
functions themselves, e.g. when indenting a list in an indented
sub-paragraph in an indented paragraph.  Useful?

But then something else hit me: the line wrap function needs to consider
colour codes as having a length of zero.  Ugh.

René

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

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Which other text functions are we going to add which would break this
> model?  The only thing I can think of right now is nesting such
> functions themselves, e.g. when indenting a list in an indented
> sub-paragraph in an indented paragraph.  Useful?

I was more worried about painting ourselves now in a corner we cannot get
out of easily later.  Even if my answer to question "what are we going to
add" may be "nothing I can think of right now", it does not make me happy.

Something off the top of my head are combinations like these.

    %[toupper()%cD%] => 'SUN, 18 OCT 2009 12:34:56 -0700'
    %[substr(7,3)%[toupper()%cD%]] => 'OCT'

    %[sanitize()%s%] === %f (i.e. format-patch filename)
    %[sanitize()%[substr(0,7)%[toupper()%aN%]%]%s] (with upcased author name)

By the way, I think that date formatting can be helped by introducing a
strftime() function that takes %ct/%at as input, e.g. %aD would become

    %[strftime(%a, %d %b %Y %H:%M:%S %z)%at]

and we do not have to worry about keep adding random %[ac]X formats and
running out of X.  Right now we use d/D/r/i and there were talks of adding
a shortened 8601 format without time or something we did not implement.

Also, if we had this %[func() any string%] mechanism, we probably wouldn't
have had to add distinction between n/N and e/E after %a and %c.

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

[René Scharfe]

Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Which other text functions are we going to add which would break this
>> model?  The only thing I can think of right now is nesting such
>> functions themselves, e.g. when indenting a list in an indented
>> sub-paragraph in an indented paragraph.  Useful?
> 
> I was more worried about painting ourselves now in a corner we cannot get
> out of easily later.  Even if my answer to question "what are we going to
> add" may be "nothing I can think of right now", it does not make me happy.

If wrapping wasn't implemented as a nested function, nesting could still
be introduced independently and used for other things -- once these
other things arrive.

> Something off the top of my head are combinations like these.
> 
>     %[toupper()%cD%] => 'SUN, 18 OCT 2009 12:34:56 -0700'
>     %[substr(7,3)%[toupper()%cD%]] => 'OCT'
> 
>     %[sanitize()%s%] === %f (i.e. format-patch filename)
>     %[sanitize()%[substr(0,7)%[toupper()%aN%]%]%s] (with upcased author name)

Interesting examples, I particular like sanitize().

> By the way, I think that date formatting can be helped by introducing a
> strftime() function that takes %ct/%at as input, e.g. %aD would become
> 
>     %[strftime(%a, %d %b %Y %H:%M:%S %z)%at]
> 
> and we do not have to worry about keep adding random %[ac]X formats and
> running out of X.  Right now we use d/D/r/i and there were talks of adding
> a shortened 8601 format without time or something we did not implement.

The number of date formats is scary, but this could be solved e.g. by
introducing "%aT(<date format specifiers etc.>)", without nesting.

> Also, if we had this %[func() any string%] mechanism, we probably wouldn't
> have had to add distinction between n/N and e/E after %a and %c.

Yeah, the place holders multiplied, and some of that growth could have
been avoided by providing ways to change the change the output instead
of providing the processed results.

However, I think that nesting is such a big addition that it warrants
further planning.  It turns the simple "see place holder, fill in value"
interpolator into more of a programming language.  Is that really
needed?  And if yes, do we want to keep all these percent signs around
or is it better to invent a nicer syntax?  Or borrow it from somewhere
else?  Or perhaps I'm just afraid of change and complexity.

Anyway, all of the functions that accept strings need to be able to skip
over escape codes, which includes all of those mentioned above except
perhaps strftime.  This is ugly.  Or one could forbid colour codes in
function arguments.

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%].

I feel I need to think a bit more about this; currently I'm a bit scared
by %[...%].  But first to catch some sleep..

René

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

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Junio C Hamano schrieb:
> ...
>> I was more worried about painting ourselves now in a corner we cannot get
>> out of easily later.  Even if my answer to question "what are we going to
>> add" may be "nothing I can think of right now", it does not make me happy.
>
> If wrapping wasn't implemented as a nested function, nesting could still
> be introduced independently and used for other things -- once these
> other things arrive.

True.  I do not think we _need_ nested expansion; obviously we have lived
without it for a long time.

> 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.

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

[René Scharfe]

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 */
 }