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

From: Jeff King <peff@peff.net>
Date: Sep 9, 2009, 20:39 UTC
Message-ID: <20090909203939.GA10438@sigill.intra.peff.net>
In-Reply-To: <7vr5ugszte.fsf@alter.siamese.dyndns.org>

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

Show 21 quoted lines

> 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):

Show 6 quoted lines

> 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