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

[Jeff King <peff@peff.net>]

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