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

Reply via email to