From: Jeff King <peff@peff.net>
Date: Tue, 13 Apr 2010 11:07:24 GMT
Subject: Re: [PATCHv3] pretty: Initialize notes if %N is used
Message-ID: <20100413110723.GA2910@coredump.intra.peff.net>
In-Reply-To: <1271156465-7302-1-git-send-email-heipei@hackvalue.de>

On Tue, Apr 13, 2010 at 01:01:05PM +0200, Johannes Gilger wrote:

> Introduced space when calling userformat_find_requirements and dealt
> with %+N and %-N during the strbuf_expand phase. I hope strncmp is the
> right way to do it here. strbuf is NUL-terminated so there should not
> be a problem.

Ugh. I didn't even know we had such a thing. Those look like the only
ones that should be a problem, though. I'm glad to have factored out the
"want" code, now. At least it will all be in one spot.

> +static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
> +				   void *context)
> +{
> +	struct userformat_want *w = context;
> +
> +	switch (*placeholder) {
> +		case '-':
> +		case '+':
> +			if (!strncmp(placeholder+1, "N", 1))
> +				w->notes = 1;
> +		case 'N': w->notes = 1;
> +	}
> +	return 0;
> +}

Should this perhaps be:

  if (*placeholder == '+' || *placeholder == '-')
    placeholder++;

  switch (*placeholder) {
    case 'N': w->notes = 1; break;
  }

so that it will extend naturally if other placeholder lookups are needed
(since those ones also could have + or - markers).

Also, I just noticed that your case is missing a 'break'. Not a bug yet,
but it will be if somebody adds a new case. This is almost certainly my
fault from the original version I posted. :)

-Peff