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

From: Jeff King <peff@peff.net>
Date: Feb 18, 2015, 19:25 UTC
Message-ID: <20150218192518.GA7891@peff.net>
In-Reply-To: <xmqqh9uj2g25.fsf@gitster.dls.corp.google.com>

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

Show 20 quoted lines

> 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