Re: [PATCH 1/2] push: make non-fast-forward help message configurable

[Jeff King <peff@peff.net>]

On Wed, Sep 09, 2009 at 12:06:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/advice.c b/advice.c
> > new file mode 100644
> > index 0000000..b5216a2
> > --- /dev/null
> > +++ b/advice.c
> > @@ -0,0 +1,25 @@
> > +#include "cache.h"
> > +
> > +int advice_push_nonfastforward = 1;
> > +
> > +static struct {
> > +	const char *name;
> > +	int *preference;
> > +} advice_config[] = {
> > +	{ "pushnonfastforward", &advice_push_nonfastforward },
> > +};
> 
> Can we have the value inside this struct, instead of having a pointer
> to another variable, and get rid of that variable altogether?

We could, but then callers need some way of indexing into the array.
Which means either:

  - a constant offset (like "#define ADVICE_PUSH_NONFASTFORWARD 0"). The
    problem with that is you get no compile-time support for making sure
    that your index matches the variable you want. I.e., you have:

      { "pushnonfastforward", 1 } /* ADVICE_PUSH_NONFASTFORWARD */

    with the position in the array implicitly matching the manually
    assigned numbers.  We do have precedent in things like diff_colors.
    I don't remember if we have ever screwed it up.

  - a dynamic offset, like (as you noted):

> If we did so, this part needs to become
> 
> 	if (nonfastforward && check_advice("pushnonfastforward")) {
> 
> which would be less efficient, but by definition advices are on the slow
> path, right?

which, as you note, is less efficient. It also turns typo-checking into
a run-time error rather than a compile-time error, which is IMHO a bad
idea. And if you care about such things, it is worse for using something
like ctags to find variable uses.

I went the way I did because it provides compile-time checking, and
because the variable is referred to by name in the table, the matching
is explicit and thus harder to screw up.

One final option would be to get rid of the table altogether. Its
function is to allow you to iterate over all of the variables. Now that
the "advice.all" option has been dropped, the only use is during config
parsing. We could simply unroll that loop to:

  if (!strcmp(k, "pushnonfastforward")) {
          advice_push_nonfastforward = git_config_bool(var, value);
          return 0;
  }
  if (!strcmp(k, "statushints")) {
          advice_status_hints = git_config_bool(var, value);
          return 0;
  }

as we do in other config parsing.

-Peff