Re: [PATCH] use skip_prefix() to avoid more magic numbers

> On Sun, Oct 05, 2014 at 03:49:19PM -0700, Jonathan Nieder wrote:
>
>> > --- a/builtin/branch.c
>> > +++ b/builtin/branch.c
>> > @@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int ofs)
>> >  
>> >  static int git_branch_config(const char *var, const char *value, void *cb)
>> >  {
>> > +	const char *slot_name;
>> > +
>> >  	if (starts_with(var, "column."))
>> >  		return git_column_config(var, value, "branch", &colopts);
>> >  	if (!strcmp(var, "color.branch")) {
>> >  		branch_use_color = git_config_colorbool(var, value);
>> >  		return 0;
>> >  	}
>> > -	if (starts_with(var, "color.branch.")) {
>> > -		int slot = parse_branch_color_slot(var, 13);
>> > +	if (skip_prefix(var, "color.branch.", &slot_name)) {
>> > +		int slot = parse_branch_color_slot(var, slot_name - var);
>> 
>> I wonder why the separate var and offset parameters exist in the first
>> place.  Wouldn't taking a single const char * so the old code could use
>> 'var + 13' instead of 'var, 13' have worked?
>
> I think this is in the same boat as parse_diff_color_slot, which I fixed
> in 9e1a5eb (parse_diff_color_slot: drop ofs parameter, 2014-06-18). The
> short of it is that the function used to want both the full name and the
> slot name, but now needs only the latter.
>
> The fix you proposed below is along the same line, and looks good to me
> (and grepping for 'var *+ *ofs' shows only the two sites you found, so
> hopefully that is the last of it).

>
>> > @@ -809,18 +808,19 @@ static void parse_commit_header(struct
>> > format_commit_context *context)
>> >  	int i;
>> >  
>> >  	for (i = 0; msg[i]; i++) {
>> > +		const char *name;
>> 
>> ident instead of name, maybe? (since it's a 'name <email> timestamp'
>> field)
>
> Yeah, agreed.
>
> -Peff