{"thread":{"id":"17840","subject":"[PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","startedAt":"2009-02-17T03:26:49Z","lastAt":"2009-02-19T01:02:46Z","messageCount":8,"participants":["Keith Cascio","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"105069","messageId":"1234841209-3960-1-git-send-email-keith@cs.ucla.edu","threadId":"17840","inReplyTo":null,"subject":"[PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-17T03:26:49Z","receivedAt":"2009-02-17T03:26:49Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"\nSigned-off-by: Keith Cascio <keith@cs.ucla.edu>\n---\n diff.h |    3 +++\n diff.c |   17 ++++++++++-------\n 2 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex 6703a4f..6616877 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -69,6 +69,9 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)\n #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)\n+#define DIFF_XDL_TST(opts, flag)    ((opts)->xdl_opts & XDF_##flag)\n+#define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |= XDF_##flag)\n+#define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag)\n \n struct diff_options {\n \tconst char *filter;\ndiff --git a/diff.c b/diff.c\nindex 006aa01..ff3624e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2567,13 +2567,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \n \t/* xdiff options */\n \telse if (!strcmp(arg, \"-w\") || !strcmp(arg, \"--ignore-all-space\"))\n-\t\toptions->xdl_opts |= XDF_IGNORE_WHITESPACE;\n+\t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE);\n \telse if (!strcmp(arg, \"-b\") || !strcmp(arg, \"--ignore-space-change\"))\n-\t\toptions->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;\n+\t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);\n \telse if (!strcmp(arg, \"--ignore-space-at-eol\"))\n-\t\toptions->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;\n+\t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n \telse if (!strcmp(arg, \"--patience\"))\n-\t\toptions->xdl_opts |= XDF_PATIENCE_DIFF;\n+\t\tDIFF_XDL_SET(options, PATIENCE_DIFF);\n \n \t/* flags options */\n \telse if (!strcmp(arg, \"--binary\")) {\n@@ -2594,10 +2594,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_SET(options, COLOR_DIFF);\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n-\telse if (!strcmp(arg, \"--color-words\"))\n-\t\toptions->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;\n+\telse if (!strcmp(arg, \"--color-words\")) {\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF_WORDS);\n+\t}\n \telse if (!prefixcmp(arg, \"--color-words=\")) {\n-\t\toptions->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF_WORDS);\n \t\toptions->word_regex = arg + 14;\n \t}\n \telse if (!strcmp(arg, \"--exit-code\"))\n-- \n1.6.1\n"},{"id":"105130","messageId":"alpine.DEB.1.00.0902171304130.6185@intel-tinevez-2-302","threadId":"17840","inReplyTo":"1234841209-3960-1-git-send-email-keith@cs.ucla.edu","subject":"Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-17T12:05:26Z","receivedAt":"2009-02-17T12:05:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 16 Feb 2009, Keith Cascio wrote:\n\n> \n> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>\n> ---\n\nRationale?\n\nIf you are going to add something on top of that, I can understand that, \nbut this patch is not labeled [1/n].  And...\n\n>  diff.h |    3 +++\n>  diff.c |   17 ++++++++++-------\n>  2 files changed, 13 insertions(+), 7 deletions(-)\n\n... this does not look good to me, without a compelling reason why we want \nto have the patch nevertheless.\n\nCiao,\nDscho\n"},{"id":"105185","messageId":"alpine.GSO.2.00.0902170918180.27811@kiwi.cs.ucla.edu","threadId":"17840","inReplyTo":"alpine.DEB.1.00.0902171304130.6185@intel-tinevez-2-302","subject":"Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-17T17:33:42Z","receivedAt":"2009-02-17T17:33:42Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Dscho,\n\nOn Tue, 17 Feb 2009, Johannes Schindelin wrote:\n\n> Rationale?\n> If you are going to add something on top of that, I can understand that, but \n> this patch is not labeled [1/n].  And...\n\nThis patch is about consistency.  Yes I'd like to add something on top of it.  \nBut these improvements stand well on their own.  My work on the \ndiff.defaultOptions patch highlighted the desirability of handling these bit \nmanipulations consistently, via macros.\n\n> ... this does not look good to me, without a compelling reason why we want to \n> have the patch nevertheless.\n\nIs there something you dislike about the code style?  As always I'm happy to \nadjust it.\n                                    -- Keith\n"},{"id":"105230","messageId":"alpine.DEB.1.00.0902172354570.10279@pacific.mpi-cbg.de","threadId":"17840","inReplyTo":"alpine.GSO.2.00.0902170918180.27811@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-17T22:57:10Z","receivedAt":"2009-02-17T22:57:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 17 Feb 2009, Keith Cascio wrote:\n\n> On Tue, 17 Feb 2009, Johannes Schindelin wrote:\n> \n> But these improvements stand well on their own.\n\nThat was my point; without anything on top I would not like to risk \nregressions that easily.\n\n> > ... this does not look good to me, without a compelling reason why we \n> > want to have the patch nevertheless.\n> \n> Is there something you dislike about the code style?  As always I'm \n> happy to adjust it.\n\nIf you had not so conveniently clipped what I quoted just before the three \ndots, I could point out that it adds roughly double the number of lines \nas it removes.\n\nCiao,\nDscho\n"},{"id":"105338","messageId":"alpine.GSO.2.00.0902181238320.29723@kiwi.cs.ucla.edu","threadId":"17840","inReplyTo":"alpine.DEB.1.00.0902172354570.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-18T20:52:04Z","receivedAt":"2009-02-18T20:52:04Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Tue, 17 Feb 2009, Johannes Schindelin wrote:\n\n> > >  diff.h |    3 +++\n> > >  diff.c |   17 ++++++++++-------\n> > >  2 files changed, 13 insertions(+), 7 deletions(-)\n> \n> If you had not so conveniently clipped what I quoted just before the three \n> dots, I could point out that it adds roughly double the number of lines as it \n> removes.\n\nDidn't they say on Mount Sinai?\n\n\"Thou shalt not judge a patch on the diffstat alone.\"\n"},{"id":"105344","messageId":"alpine.DEB.1.00.0902182219370.10279@pacific.mpi-cbg.de","threadId":"17840","inReplyTo":"alpine.GSO.2.00.0902181238320.29723@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-18T21:22:11Z","receivedAt":"2009-02-18T21:22:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 18 Feb 2009, Keith Cascio wrote:\n\n> On Tue, 17 Feb 2009, Johannes Schindelin wrote:\n> \n> > > >  diff.h |    3 +++\n> > > >  diff.c |   17 ++++++++++-------\n> > > >  2 files changed, 13 insertions(+), 7 deletions(-)\n> > \n> > If you had not so conveniently clipped what I quoted just before the \n> > three dots, I could point out that it adds roughly double the number \n> > of lines as it removes.\n> \n> Didn't they say on Mount Sinai?\n> \n> \"Thou shalt not judge a patch on the diffstat alone.\"\n\nOkay, you asked for it.  I tried to be gentle.\n\nI see _no_ value in your changes, and the diffstat as a _very_ real \ndownside.\n\nIf the code would become clearer with your patch, I would not mind.  But I \nfind that the result is not more readable than the original.\n\nAs part of a parse-optification, I would not mind.  But before that, no.\n\nCiao,\nDscho\n"},{"id":"105356","messageId":"alpine.GSO.2.00.0902181437260.6181@kiwi.cs.ucla.edu","threadId":"17840","inReplyTo":"alpine.DEB.1.00.0902182219370.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-18T22:59:29Z","receivedAt":"2009-02-18T22:59:29Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Wed, 18 Feb 2009, Johannes Schindelin wrote:\n\n> Okay, you asked for it.  I tried to be gentle.\n> \n> I see _no_ value in your changes, and the diffstat as a _very_ real downside.\n> \n> If the code would become clearer with your patch, I would not mind.  But I \n> find that the result is not more readable than the original.\n> \n> As part of a parse-optification, I would not mind.  But before that, no.\n\nActually I appreciate your feedback, and the more direct the better.  The chief \nvalue I see in revising the code to accomplish these bit settings uniformly \nusing well known macros is that later, if someone has good reason to extend the \nmacro so it results in some new side effect, e.g. to update a dirty bit mask, \nthe new behavior automatically cascades to every appropriate use out there in \nthe code.  A macro is essentially a \"code constant\".  As with other constants, \nthe benefit comes from defining it once and using it everywhere.  Is this effect \nnot worth as much as I think it is?  Is there a hidden gotcha in my ideal?  Or \ndoes anyone else see value here?  Please speak up!\n\n                                             -- Keith\n"},{"id":"105367","messageId":"20090219010246.GD25808@coredump.intra.peff.net","threadId":"17840","inReplyTo":"alpine.GSO.2.00.0902181437260.6181@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T01:02:46Z","receivedAt":"2009-02-19T01:02:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 18, 2009 at 02:59:29PM -0800, Keith Cascio wrote:\n\n> Actually I appreciate your feedback, and the more direct the better.\n> The chief value I see in revising the code to accomplish these bit\n> settings uniformly using well known macros is that later, if someone\n> has good reason to extend the macro so it results in some new side\n> effect, e.g. to update a dirty bit mask, the new behavior\n> automatically cascades to every appropriate use out there in the code.\n> A macro is essentially a \"code constant\".  As with other constants,\n> the benefit comes from defining it once and using it everywhere.  Is\n> this effect not worth as much as I think it is?  Is there a hidden\n> gotcha in my ideal?  Or does anyone else see value here?  Please speak\n> up!\n\nI think you are running into \"if it's not broken, don't fix it\".  That\nis, there is a transaction cost to making changes to the code. It can be\nas small a cost as \"now somebody working in a related area has a\nconflict (even if textual) and has to spend time resolving\". Or it can\nbe as big as \"you introduced new bugs\".  In this case, I think any costs\nwould tend towards the former and not the latter.\n\nThat cost has to be weighed against the benefit.\n\nThe usual attitude in git is that minor cleanups or enhancements like\nthis should be coupled with patches that build on them. Saying \"this\nmight help in the future if somebody does X\" means you are paying the\ncost now, but there may or may not ever _be_ any benefit.\n\nIn this case, I think there are actually two changes in your patch:\n\n  1. consistently using DIFF_OPT_* macros, which are used in 99% of\n     callsites already\n\n  2. adding and using DIFF_XDL macros\n\nI think (1) has immediate value. Anyone looking at the code and seeing\nthe existence of DIFF_OPT macros and how commonly they are used might\nthink they are used exclusively. And that means it is easy to miss\ncallsites when searching through the code. So to me, making things\nconsistent brings value.\n\nFor (2), that situation does not exist, since you are introducing new\nmacros.\n\nPersonally, I don't mind the abstraction layer of the XDL macros,\nespecially since they match the style of the existing DIFF_OPT macros\n(and the only reason they are not in the DIFF_OPT bitfield at all is\nthat they must be passed separately to xdiff).\n\n-Peff\n"}]}