On Wed, Feb 18, 2015 at 11:50:20AM -0800, Junio C Hamano wrote: > Jeff King <p...@peff.net> writes: > > > On Wed, Feb 18, 2015 at 11:08:34AM -0800, Junio C Hamano wrote: > > ... > >> 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); > > Maybe. I do not feel very strongly either way.
So I started to prepare a patch for this, because I wanted to see how it would look. It's below in case you are curious, but note that it doesn't compile, and that is does something a bit dangerous. So I think I am giving up on this line of thought. Read on if you're curious. The sticking point is the type of the bit-field. If it is: void flip_bits(int set_or_clear, int *field, int bits); then we cannot pass a pointer to "unsigned field". We can pass unsigned bits, but note that we might lose the 32nd (or 64th) bit. We can do this instead: void flip_bits(int set_or_clear, void *field, unsigned bits); But of course we will end up dereferencing "void *field" as "unsigned *". I suspect if field is originally an "int" it works fine on most platforms, but isn't legal according to the standard. Much worse, though: if your original field is a different size, it's even less likely to work. Passing a "char *" may set bits in random adjacent memory (depending on your endianness), and an "unsigned long *" may set bits in the wrong part of the variable. And because of the "void *", we get no compiler warnings. :) Add on top that we may want to use this for C bit-fields, whose address cannot legally be taken (and this is why it does not compile). So besides adding type-specific bit-flippers (yuck), I think the only way to do it universally would be with a macro. Which I think tips it over the "too gross, just write it out" line. --- diff --git a/archive-tar.c b/archive-tar.c index 0d1e6bd..3c794e2 100644 --- a/archive-tar.c +++ b/archive-tar.c @@ -352,10 +352,7 @@ static int tar_filter_config(const char *var, const char *value, void *data) return 0; } if (!strcmp(type, "remote")) { - if (git_config_bool(var, value)) - ar->flags |= ARCHIVER_REMOTE; - else - ar->flags &= ~ARCHIVER_REMOTE; + git_config_bits(var, value, &ar->flags, ARCHIVER_REMOTE); return 0; } diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index d816587..073445e 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2216,10 +2216,8 @@ static int git_pack_config(const char *k, const char *v, void *cb) return 0; } if (!strcmp(k, "pack.writebitmaphashcache")) { - if (git_config_bool(k, v)) - write_bitmap_options |= BITMAP_OPT_HASH_CACHE; - else - write_bitmap_options &= ~BITMAP_OPT_HASH_CACHE; + git_config_bits(k, v, &write_bitmap_options, + BITMAP_OPT_HASH_CACHE); } if (!strcmp(k, "pack.usebitmaps")) { use_bitmap_index = git_config_bool(k, v); diff --git a/builtin/update-index.c b/builtin/update-index.c index 5878986..9d4eb24 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -53,10 +53,7 @@ static int mark_ce_flags(const char *path, int flag, int mark) int namelen = strlen(path); int pos = cache_name_pos(path, namelen); if (0 <= pos) { - if (mark) - active_cache[pos]->ce_flags |= flag; - else - active_cache[pos]->ce_flags &= ~flag; + flip_bits(mark, &active_cache[pos]->ce_flags, flag); active_cache[pos]->ce_flags |= CE_UPDATE_IN_BASE; cache_tree_invalidate_path(&the_index, path); active_cache_changed |= CE_ENTRY_CHANGED; diff --git a/cache.h b/cache.h index f704af5..957f150 100644 --- a/cache.h +++ b/cache.h @@ -1316,6 +1316,18 @@ extern int sha1_object_info_extended(const unsigned char *, struct object_info * /* Dumb servers support */ extern int update_server_info(int); +/* + * Either set or clear "bits" from "field", based on + * whether or not "set_or_clear" is set. + */ +static inline void flip_bits(int set_or_clear, void *field, unsigned bits) +{ + if (set_or_clear) + *(unsigned *)field |= bits; + else + *(unsigned *)field &= ~bits; +} + /* git_config_parse_key() returns these negated: */ #define CONFIG_INVALID_KEY 1 #define CONFIG_NO_SECTION_OR_NAME 2 @@ -1385,6 +1397,12 @@ struct config_include_data { #define CONFIG_INCLUDE_INIT { 0 } extern int git_config_include(const char *name, const char *value, void *data); +static inline int git_config_bits(const char *name, const char *value, void *field, unsigned bits) +{ + flip_bits(git_config_bool(name, value), field, bits); + return 0; +} + /* * Match and parse a config key of the form: * diff --git a/column.c b/column.c index 786abe6..21c9765 100644 --- a/column.c +++ b/column.c @@ -281,12 +281,8 @@ static int parse_option(const char *arg, int len, unsigned int *colopts, if (opts[i].mask) *colopts = (*colopts & ~opts[i].mask) | opts[i].value; - else { - if (set) - *colopts |= opts[i].value; - else - *colopts &= ~opts[i].value; - } + else + flip_bits(set, colopts, opts[i].value); return 0; } diff --git a/combine-diff.c b/combine-diff.c index 91edce5..ad52b77 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -581,12 +581,9 @@ static int make_hunks(struct sline *sline, unsigned long cnt, unsigned long i; int has_interesting = 0; - for (i = 0; i <= cnt; i++) { - if (interesting(&sline[i], all_mask)) - sline[i].flag |= mark; - else - sline[i].flag &= ~mark; - } + for (i = 0; i <= cnt; i++) + flip_bits(interesting(&sline[i], all_mask), + &sline[i].flag, mark); if (!dense) return give_context(sline, cnt, num_parent); diff --git a/diff.c b/diff.c index d1bd534..53b9481 100644 --- a/diff.c +++ b/diff.c @@ -3585,22 +3585,17 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt) for (i = 0; (optch = optarg[i]) != '\0'; i++) { unsigned int bit; - int negate; + int positive = 1; if ('a' <= optch && optch <= 'z') { - negate = 1; + positive = 0; optch = toupper(optch); - } else { - negate = 0; } bit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0; if (!bit) return optarg[i]; - if (negate) - opt->filter &= ~bit; - else - opt->filter |= bit; + flip_bits(positive, &opt->filter, bit); } return 0; } diff --git a/parse-options.c b/parse-options.c index 80106c0..47c169c 100644 --- a/parse-options.c +++ b/parse-options.c @@ -100,17 +100,11 @@ static int get_value(struct parse_opt_ctx_t *p, return (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset); case OPTION_BIT: - if (unset) - *(int *)opt->value &= ~opt->defval; - else - *(int *)opt->value |= opt->defval; + flip_bits(!unset, opt->value, opt->defval); return 0; case OPTION_NEGBIT: - if (unset) - *(int *)opt->value |= opt->defval; - else - *(int *)opt->value &= ~opt->defval; + flip_bits(unset, opt->value, opt->defval); return 0; case OPTION_COUNTUP: diff --git a/revision.c b/revision.c index 66520c6..85cb245 100644 --- a/revision.c +++ b/revision.c @@ -554,10 +554,8 @@ static int compact_treesame(struct rev_info *revs, struct commit *commit, unsign if (nth_parent != 0) die("compact_treesame %u", nth_parent); old_same = !!(commit->object.flags & TREESAME); - if (rev_same_tree_as_empty(revs, commit)) - commit->object.flags |= TREESAME; - else - commit->object.flags &= ~TREESAME; + flip_bits(rev_same_tree_as_empty(revs, commit), + &commit->object.flags, TREESAME); return old_same; } @@ -578,10 +576,8 @@ static int compact_treesame(struct rev_info *revs, struct commit *commit, unsign if (--st->nparents == 1) { if (commit->parents->next) die("compact_treesame parents mismatch"); - if (st->treesame[0] && revs->dense) - commit->object.flags |= TREESAME; - else - commit->object.flags &= ~TREESAME; + flip_bits(st->treesame[0] && revs->dense, + &commit->object.flags, TREESAME); free(add_decoration(&revs->treesame, &commit->object, NULL)); } @@ -609,10 +605,8 @@ static unsigned update_treesame(struct rev_info *revs, struct commit *commit) } else irrelevant_change |= !st->treesame[n]; } - if (relevant_parents ? relevant_change : irrelevant_change) - commit->object.flags &= ~TREESAME; - else - commit->object.flags |= TREESAME; + flip_bits(!(relevant_parents ? relevant_change : irrelevant_change), + &commit->object.flags, TREESAME); } return commit->object.flags & TREESAME; diff --git a/unpack-trees.c b/unpack-trees.c index be84ba2..9089c5b 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -248,10 +248,8 @@ static int apply_sparse_checkout(struct index_state *istate, { int was_skip_worktree = ce_skip_worktree(ce); - if (ce->ce_flags & CE_NEW_SKIP_WORKTREE) - ce->ce_flags |= CE_SKIP_WORKTREE; - else - ce->ce_flags &= ~CE_SKIP_WORKTREE; + flip_bits(ce->ce_flags & CE_NEW_SKIP_WORKTREE, + &ce->ce_flags, CE_SKIP_WORKTREE); if (was_skip_worktree != ce_skip_worktree(ce)) { ce->ce_flags |= CE_UPDATE_IN_BASE; istate->cache_changed |= CE_ENTRY_CHANGED; @@ -982,10 +980,7 @@ static void mark_new_skip_worktree(struct exclude_list *el, if (select_flag && !(ce->ce_flags & select_flag)) continue; - if (!ce_stage(ce)) - ce->ce_flags |= skip_wt_flag; - else - ce->ce_flags &= ~skip_wt_flag; + flip_bits(!ce_stage(ce), &ce->ce_flags, skip_wt_flag); } /* diff --git a/ws.c b/ws.c index ea4b2b1..6c5f4bd 100644 --- a/ws.c +++ b/ws.c @@ -30,14 +30,14 @@ unsigned parse_whitespace_rule(const char *string) int i; size_t len; const char *ep; - int negated = 0; + int positive = 1; string = string + strspn(string, ", \t\n\r"); ep = strchrnul(string, ','); len = ep - string; if (*string == '-') { - negated = 1; + positive = 0; string++; len--; } @@ -47,10 +47,8 @@ unsigned parse_whitespace_rule(const char *string) if (strncmp(whitespace_rule_names[i].rule_name, string, len)) continue; - if (negated) - rule &= ~whitespace_rule_names[i].rule_bits; - else - rule |= whitespace_rule_names[i].rule_bits; + flip_bits(positive, &rule, + whitespace_rule_names[i].rule_bits); break; } if (strncmp(string, "tabwidth=", 9) == 0) { -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html