On Wed, Feb 18, 2015 at 11:08:34AM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html