From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mon, 26 Apr 2010 03:38:13 GMT
Subject: Re: [PATCH v2 2/3] pretty: make %H/%h dependent on --abbrev[-commit]
Message-ID: <20100426033813.GA30544@progeny.tock>
In-Reply-To: <20100426033147.GB14421@coredump.intra.peff.net>

Jeff King wrote:
> On Sun, Apr 25, 2010 at 10:11:37PM -0500, Jonathan Nieder wrote:
>
>> Here’s an alternative implementation of the more controversial half of
>> your patch, for your amusement.  The big downside is that it requires
>> one to specify --abbrev-commit before the --format option.
>
> That is not insurmountable, as we could just check after the parsing
> stage. But there is a worse problem:
>
>> +static void abbreviate_commit_hashes(char *fmt)
>> +{
>> +	char *p;
>> +	for (p = fmt; p != NULL; p = strchr(p + 1, '%')) {
>> +		p++;
>> +		switch (*p) {
>> +		case 'H':
>> +			*p = 'h';
>> +			break;
>> +		case 'P':
>> +			*p = 'p';
>> +			break;
>> +		case 'T':
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +}
>
> You parse '%%H' incorrectly.

I’m pretty sure I don’t.

> I would really rather not see ad-hoc
> parsers for the format like this, but rather use or extend strbuf_expand
> as appropriate. That would make things less painful if and when we
> decide to tweak the syntax.

However, this point still applies.

Jonathan