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
