Re: [PATCH] git-log --format: Add %B tag with %B(x) option

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 17, 2009, 23:27 UTC
Message-ID: <7vk4zxgnim.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1253227671-20493-1-git-send-email-heipei@hackvalue.de>

Johannes Gilger <heipei@hackvalue.de> writes:

Show 10 quoted lines

> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 2a845b1..c04f118 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -123,6 +123,8 @@ The placeholders are:
>  - '%s': subject
>  - '%f': sanitized subject line, suitable for a filename
>  - '%b': body
> +- '%B': body without trailing newline
> +- '%B(x)': body indented with x spaces

First the design issues.

Because this will set a precedent for possible future formatting features that take optional parameters, we need to pick the syntax carefully not only for this feature but for the later ones that we haven't invented yet.

Let's say that pair of parentheses is a good choice and if later features want to take more than one, it would be reasonable for them to use a comma-separated list, e.g. %Q(1,2,3).

I wonder if it is reasonable to invoke print_wrapped_text(), not just limit this feature to indenting.

Now, let's look at the implementation.

Show 14 quoted lines

> diff --git a/pretty.c b/pretty.c
> index f5983f8..6d530e1 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -735,6 +735,19 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
>  	case 'b':	/* body */
>  		strbuf_addstr(sb, msg + c->body_off);
>  		return 1;
> +	case 'B':
> +		if (placeholder[1] == '(') {
> +			const char *body = msg + c->body_off;
> +			const char *end = strchr(placeholder + 2, ')');
> +			if(!end)
> +				return 0;

Style: "if (!end)"

I'd sleep better if the syntax checking is done as a separate phase way before this in the codepath.

> +			pp_remainder(CMIT_FMT_MEDIUM, &body, sb, atoi(placeholder + 2));

What happens when atoi() fails, or %B(12Q) was given?

We tend to use strto[u]l when parsing integers and check for errors.