[PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

Signed-off-by: Keith Cascio <redacted>
---
 diff.h |    3 +++
 diff.c |   17 ++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/diff.h b/diff.h
index 6703a4f..6616877 100644
--- a/diff.h
+++ b/diff.h

@@ -69,6 +69,9 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
 #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
+#define DIFF_XDL_TST(opts, flag)    ((opts)->xdl_opts & XDF_##flag)
+#define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |= XDF_##flag)
+#define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag)
 
 struct diff_options {
 	const char *filter;

diff --git a/diff.c b/diff.c
index 006aa01..ff3624e 100644
--- a/diff.c
+++ b/diff.c

@@ -2567,13 +2567,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 
 	/* xdiff options */
 	else if (!strcmp(arg, "-w") || !strcmp(arg, "--ignore-all-space"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE);
 	else if (!strcmp(arg, "-b") || !strcmp(arg, "--ignore-space-change"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);
 	else if (!strcmp(arg, "--ignore-space-at-eol"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
 	else if (!strcmp(arg, "--patience"))
-		options->xdl_opts |= XDF_PATIENCE_DIFF;
+		DIFF_XDL_SET(options, PATIENCE_DIFF);
 
 	/* flags options */
 	else if (!strcmp(arg, "--binary")) {

@@ -2594,10 +2594,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_SET(options, COLOR_DIFF);
 	else if (!strcmp(arg, "--no-color"))
 		DIFF_OPT_CLR(options, COLOR_DIFF);
-	else if (!strcmp(arg, "--color-words"))
-		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
+	else if (!strcmp(arg, "--color-words")) {
+		DIFF_OPT_SET(options, COLOR_DIFF);
+		DIFF_OPT_SET(options, COLOR_DIFF_WORDS);
+	}
 	else if (!prefixcmp(arg, "--color-words=")) {
-		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
+		DIFF_OPT_SET(options, COLOR_DIFF);
+		DIFF_OPT_SET(options, COLOR_DIFF_WORDS);
 		options->word_regex = arg + 14;
 	}
 	else if (!strcmp(arg, "--exit-code"))

-- 
1.6.1

Hi,

On Mon, 16 Feb 2009, Keith Cascio wrote:

Signed-off-by: Keith Cascio <redacted>
---

Rationale?

If you are going to add something on top of that, I can understand that, 
but this patch is not labeled [1/n].  And...

 diff.h |    3 +++
 diff.c |   17 ++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

... this does not look good to me, without a compelling reason why we want 
to have the patch nevertheless.

Ciao,
Dscho

Dscho,

On Tue, 17 Feb 2009, Johannes Schindelin wrote:

Rationale?
If you are going to add something on top of that, I can understand that, but 
this patch is not labeled [1/n].  And...

This patch is about consistency.  Yes I'd like to add something on top of it.  
But these improvements stand well on their own.  My work on the 
diff.defaultOptions patch highlighted the desirability of handling these bit 
manipulations consistently, via macros.

... this does not look good to me, without a compelling reason why we want to 
have the patch nevertheless.

Is there something you dislike about the code style?  As always I'm happy to 
adjust it.
                                    -- Keith

Hi,

On Tue, 17 Feb 2009, Keith Cascio wrote:

On Tue, 17 Feb 2009, Johannes Schindelin wrote:

But these improvements stand well on their own.

That was my point; without anything on top I would not like to risk 
regressions that easily.

quoted

... this does not look good to me, without a compelling reason why we 
want to have the patch nevertheless.

Is there something you dislike about the code style?  As always I'm 
happy to adjust it.

If you had not so conveniently clipped what I quoted just before the three 
dots, I could point out that it adds roughly double the number of lines 
as it removes.

Ciao,
Dscho

On Tue, 17 Feb 2009, Johannes Schindelin wrote:

quoted

quoted

 diff.h |    3 +++
 diff.c |   17 ++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

If you had not so conveniently clipped what I quoted just before the three 
dots, I could point out that it adds roughly double the number of lines as it 
removes.

Didn't they say on Mount Sinai?

"Thou shalt not judge a patch on the diffstat alone."

Hi,

On Wed, 18 Feb 2009, Keith Cascio wrote:

On Tue, 17 Feb 2009, Johannes Schindelin wrote:

quoted

quoted

quoted

 diff.h |    3 +++
 diff.c |   17 ++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

If you had not so conveniently clipped what I quoted just before the 
three dots, I could point out that it adds roughly double the number 
of lines as it removes.

Didn't they say on Mount Sinai?

"Thou shalt not judge a patch on the diffstat alone."

Okay, you asked for it.  I tried to be gentle.

I see _no_ value in your changes, and the diffstat as a _very_ real 
downside.

If the code would become clearer with your patch, I would not mind.  But I 
find that the result is not more readable than the original.

As part of a parse-optification, I would not mind.  But before that, no.

Ciao,
Dscho

On Wed, 18 Feb 2009, Johannes Schindelin wrote:

Okay, you asked for it.  I tried to be gentle.

I see _no_ value in your changes, and the diffstat as a _very_ real downside.

If the code would become clearer with your patch, I would not mind.  But I 
find that the result is not more readable than the original.

As part of a parse-optification, I would not mind.  But before that, no.

Actually I appreciate your feedback, and the more direct the better.  The chief 
value I see in revising the code to accomplish these bit settings uniformly 
using well known macros is that later, if someone has good reason to extend the 
macro so it results in some new side effect, e.g. to update a dirty bit mask, 
the new behavior automatically cascades to every appropriate use out there in 
the code.  A macro is essentially a "code constant".  As with other constants, 
the benefit comes from defining it once and using it everywhere.  Is this effect 
not worth as much as I think it is?  Is there a hidden gotcha in my ideal?  Or 
does anyone else see value here?  Please speak up!

                                             -- Keith

On Wed, Feb 18, 2009 at 02:59:29PM -0800, Keith Cascio wrote:

Actually I appreciate your feedback, and the more direct the better.
The chief value I see in revising the code to accomplish these bit
settings uniformly using well known macros is that later, if someone
has good reason to extend the macro so it results in some new side
effect, e.g. to update a dirty bit mask, the new behavior
automatically cascades to every appropriate use out there in the code.
A macro is essentially a "code constant".  As with other constants,
the benefit comes from defining it once and using it everywhere.  Is
this effect not worth as much as I think it is?  Is there a hidden
gotcha in my ideal?  Or does anyone else see value here?  Please speak
up!

I think you are running into "if it's not broken, don't fix it".  That
is, there is a transaction cost to making changes to the code. It can be
as small a cost as "now somebody working in a related area has a
conflict (even if textual) and has to spend time resolving". Or it can
be as big as "you introduced new bugs".  In this case, I think any costs
would tend towards the former and not the latter.

That cost has to be weighed against the benefit.

The usual attitude in git is that minor cleanups or enhancements like
this should be coupled with patches that build on them. Saying "this
might help in the future if somebody does X" means you are paying the
cost now, but there may or may not ever _be_ any benefit.

In this case, I think there are actually two changes in your patch:

  1. consistently using DIFF_OPT_* macros, which are used in 99% of
     callsites already

  2. adding and using DIFF_XDL macros

I think (1) has immediate value. Anyone looking at the code and seeing
the existence of DIFF_OPT macros and how commonly they are used might
think they are used exclusively. And that means it is easy to miss
callsites when searching through the code. So to me, making things
consistent brings value.

For (2), that situation does not exist, since you are introducing new
macros.

Personally, I don't mind the abstraction layer of the XDL macros,
especially since they match the style of the existing DIFF_OPT macros
(and the only reason they are not in the DIFF_OPT bitfield at all is
that they must be passed separately to xdiff).

-Peff