Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Jeff King <peff@peff.net>]

On Wed, Feb 18, 2015 at 11:08:34AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> +	if (!strcmp(k, "push.followtags")) {
> >> +		if (git_config_bool(k, v))
> >> +			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
> >> +		else
> >> +			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
> >> +		return 0;
> >> +	}
> >
> > Did you have an opinion on sticking this behind a helper function?
> 
> Not very strongly either way.  Seeing the above does not bother me
> too much, but I do not know how I would feel when I start seeing
> 
> 	val = git_config_book(k, v);
> 	flip_bool(val, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);
> 
> often.  Not having to make sure that the bit constant whose name
> tends to get long is not misspelled is certainly a plus.

I think it would be even nicer as:

  git_config_bits(k, v, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);

There is a similar spot in the tar.*.remote config. And that could of
course build on a "flip_bool" or similar, which itself has many other
uses. But after taking a quick peek, I noticed that one call around
diff.c:3600 would look like:

  flip_bool(!negate, &opt->filter, bit);

IOW, it is the same pattern of conditional, but it flips the AND and OR,
because its flag is flipped. Reading that line makes me head hurt,
because we've really introduced an extra double-negative into the flow.

That "negate" flag is local to the loop we are in, and we could flip it
for clarity. But it makes me second-guess the technique.

-Peff