2009/2/15 Johannes Schindelin <Johannes.Schindelin@xxxxxx>:
> Hi,
>
> On Sun, 15 Feb 2009, demerphq wrote:
>
>> t/t4015-diff-whitespace.sh | 79 ++++++++++++++++++++++++++++++++
>
> Phew, you certainly want to make sure that it works...
Yeah, Exhaustive testing is good. (When it doesn't take hours and
hours to run :-)
>
>> @@ -33,7 +33,14 @@ extern "C" {
>> #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>> #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
>> #define XDF_PATIENCE_DIFF (1 << 5)
>> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
>> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
>> +#define XDF_IGNORE_WHITESPACE_AT_EOF (1 << 6)
>> +/*
>> + * note this is deliberately a different define from XDF_WHITESPACE_FLAGS as
>> + * there could be a new whitespace related flag which would not be part of
>> + * the XDF_IGNORE_WHITESPACE_AT_EOF_ANY flags.
>> + */
>> +#define XDF_IGNORE_WHITESPACE_AT_EOF_ANY
>> (XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_WHITESPACE_CHANGE |
>> XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_AT_EOF)
>> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
>> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL |
>> XDF_IGNORE_WHITESPACE_AT_EOF)
>
> As I told you on IRC, I do not follow that reasoning. Rather, I would add
> the exceptions to xemit.c, when -- and if(!) -- they are needed.
Yeah I know you said that, and I *think* I followed all your advice
(much appreciated by the way) except for that point as I've been
nailed by inappropriate addition of flags to masks before, and well,
you know, once bitten twice shy, and patchers perogative and all that
eh? :-)
For instance what happens if someone adds XDF_IGNORE_WHITESPACE_AT_SOL
(start of line) or XDF_IGNORE_SPACES_WHERE_TABS_EXPECTED in the
future, and then adds it to XDF_IGNORE_WHITESPACE_FLAGS? And
personally such options seem quite reasonable to me. It just happens
to be coincidence that all of the currently existing flags also impact
this particular behaviour, IMO it wouldnt have been so strange to find
one that didn't.
And thanks again for your handholding on this patch. I hope the
pasting of it inline was correct. I'm not sure where I should have
said that it was a patch against the master branch without it also
appearing in the commit body. Should I haved attached the format-patch
file as well?
Also, on a related note I personally would have reorganized the flags
so that the ones relating to whitespace control are in a different bit
range than the ones that have to do with other things. The precedent
in the file appeared to not follow this approach as the patience flag
was "at the end", so i didnt modify this, and just stuck the new flag
also at the end. I think it would be better to do a lowbit moving up
and highbit moving down approach or something like that as otherwise
when new flags get added over time the different types of flags are
interspersed and it becomes a real mess to maintain and understand. In
fact, I dont really "get" the whitespace flags as currently
implemented. Why does "ignore all whitespace" get its own bit?
Shouldn't it just be a mask of all the other whitespace bits?
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
--
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
