Re: [PATCH v2 2/3] pretty: make %H/%h dependent on --abbrev[-commit]
Show 29 quoted lines
> 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 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
Jonathan Nieder <jrnieder@gmail.com>