Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Tue, 13 Jun 2017, Johannes Schindelin wrote:

> On Sat, 10 Jun 2017, Jeff King wrote:
> 
> > On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:
> > 
> > > -char *alias_lookup(const char *alias)
> > > [...]
> > >  {
> > > -	char *v = NULL;
> > > -	struct strbuf key = STRBUF_INIT;
> > > -	strbuf_addf(&key, "alias.%s", alias);
> > > -	if (git_config_key_is_valid(key.buf))
> > > -		git_config_get_string(key.buf, &v);
> > > -	strbuf_release(&key);
> > > -	return v;
> > > +	struct strbuf key;
> > > +	char *v;
> > > +};
> > > [...]
> > > +char *alias_lookup(const char *alias, struct strbuf *cdup_dir)
> > > +{
> > > +	struct config_alias_data data = { STRBUF_INIT, NULL };
> > > +
> > > +	strbuf_addf(&data.key, "alias.%s", alias);
> > > +	if (git_config_key_is_valid(data.key.buf))
> > > +		read_early_config(config_alias_cb, &data, cdup_dir);
> > > +	strbuf_release(&data.key);
> > > +
> > > +	return data.v;
> > >  }
> > 
> > Two optional cleanups here:
> > 
> >   1. We don't need to call config_key_is_valid when using a callback. We
> >      only needed that to prevent the configset machinery from issuing a
> >      warning. It does save us reading the config entirely when the
> >      program name is syntactically invalid as an alias, but that's a
> >      pretty rare case.
> 
> It may be a pretty rare case, or it may not be. I do not want to think
> hard about this, so I just wanted to keep that test.
> 
> But since you suggested it, I will simply blame all the fallout (if any)
> on you.
> 
> ;-)
> 
> >   2. Now that we're not using the configset machinery, we don't need to
> >      have the alias name as a full string. Instead of using the strbuf,
> >      we could just pass the "alias" string itself and do:
> > 
> >        if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
> > 
> >      in the callback.
> 
> As you probably guessed, I had tried that first and then figured that if
> I needed to keep the config_key_is_valid() test anyway, I could just as
> well keep the strbuf around for later use.
> 
> Will change the code,

Alas, I won't change the code after all.

It is really tempting to avoid the extra strbuf, but then the error
message would change from

	error: missing value for 'alias.br'

to

	error: missing value for 'br'

which is of course no good at all.

And since I already have to keep that strbuf, I'll simply keep the
config_key_is_valid() guard, too (because why not).

Ciao,
Dscho