Re: [PATCH] format_sanitized_subject: Don't trim past initial length of strbuf

Re: [PATCH] format_sanitized_subject: Don't trim past initial length of strbuf

[Stephen Boyd]

Forgot to say this is based on next.

Re: [PATCHv3 1/6] pretty.c: add %f format specifier to format_commit_message()

[René Scharfe]

Stephen Boyd schrieb:
> This specifier represents the sanitized and filename friendly subject
> line of a commit. No checks are made against the length of the string,
> so users may need to trim the result to the desired length if using as a
> filename. This is commonly used by format-patch to massage commit
> subjects into filenames and output patches to files.
> 
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---
>  Documentation/pretty-formats.txt |    1 +
>  pretty.c                         |   38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 5c6e678..2a845b1 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -121,6 +121,7 @@ The placeholders are:
>  - '%d': ref names, like the --decorate option of linkgit:git-log[1]
>  - '%e': encoding
>  - '%s': subject
> +- '%f': sanitized subject line, suitable for a filename
>  - '%b': body
>  - '%Cred': switch color to red
>  - '%Cgreen': switch color to green
> diff --git a/pretty.c b/pretty.c
> index efa7024..97de415 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -493,6 +493,41 @@ static void parse_commit_header(struct format_commit_context *context)
>  	context->commit_header_parsed = 1;
>  }
>  
> +static int istitlechar(char c)
> +{
> +	return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') ||
> +		(c >= '0' && c <= '9') || c == '.' || c == '_';

How about this?

	return isalnum(c) || c == '.' || c == '_';

> +}
> +
> +static void format_sanitized_subject(struct strbuf *sb, const char *msg)
> +{
> +	size_t trimlen;
> +	int space = 0;
> +
> +	for (; *msg && *msg != '\n'; msg++) {
> +		if (istitlechar(*msg))
> +		{
> +		    if (space) {
> +			strbuf_addch(sb, '-');
> +			space = 0;
> +		    }
> +		    strbuf_addch(sb, *msg);
> +		    if (*msg == '.')
> +			while (*(msg+1) == '.')
> +				msg++;
> +		}
> +		else
> +			space = 1;
> +	}
> +

> +	// trim any trailing '.' or '-' characters
> +	trimlen = 0;
> +	while (sb->buf[sb->len - 1 - trimlen] == '.'
> +		|| sb->buf[sb->len - 1 - trimlen] == '-')
> +		trimlen++;
> +	strbuf_remove(sb, sb->len - trimlen, trimlen);

You need to make sure that trimming stops as soon as the strbuf has been
shortened to its original length.  E.g. for a subject line of "..."
you'd access the char before the first dot currently, or sb->buf[-1] if
the strbuf was empty initially.

(One could also check for sb->len > 0 to just prevent the buffer
underrun, but %f sometimes eating preceding dots and dashes is
counter-intuitive to me.)

> +}
> +
>  const char *format_subject(struct strbuf *sb, const char *msg,
>  			   const char *line_separator)
>  {
> @@ -683,6 +718,9 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
>  	case 's':	/* subject */
>  		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;

[PATCH] format_sanitized_subject: Don't trim past initial length of strbuf

[Stephen Boyd]

If the subject line is '...' the strbuf will be accessed before the
first dot is added; potentially changing the strbuf passed into the
function or accessing sb->buf[-1] if it was originally empty.

Reported-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
I was thinking about this today actually. Thanks.

With regards to the isalnum(), I kept the original code because I wasn't sure
if the functionality would be different.

 pretty.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/pretty.c b/pretty.c
index c57cef4..a0ef356 100644
--- a/pretty.c
+++ b/pretty.c
@@ -502,6 +502,7 @@ static int istitlechar(char c)
 static void format_sanitized_subject(struct strbuf *sb, const char *msg)
 {
 	size_t trimlen;
+	size_t start_len = sb->len;
 	int space = 2;
 
 	for (; *msg && *msg != '\n'; msg++) {
@@ -519,8 +520,9 @@ static void format_sanitized_subject(struct strbuf *sb, const char *msg)
 
 	/* trim any trailing '.' or '-' characters */
 	trimlen = 0;
-	while (sb->buf[sb->len - 1 - trimlen] == '.'
-		|| sb->buf[sb->len - 1 - trimlen] == '-')
+	while (sb->len - trimlen > start_len &&
+		(sb->buf[sb->len - 1 - trimlen] == '.'
+		|| sb->buf[sb->len - 1 - trimlen] == '-'))
 		trimlen++;
 	strbuf_remove(sb, sb->len - trimlen, trimlen);
 }
-- 
1.6.2