Hi,
On Sun, 18 Jan 2009, Keith Cascio wrote:
> Fixed bug in diff whitespace ignore options.
> It is now OK to specify more than one whitespace ignore option
> on the command line. In unit test 4015, expect success rather
> than failure for 4 cases.
> Note: I do not fully understand why this fix works, but it passes
> all 68 t4???-* diff test scripts.
>
> The semantics of the three whitespace ignore flags
> { -w, -b, --ignore-space-at-eol }
> obey a relation of transitive implication, i.e. the stronger
> options imply the weaker options:
> -w implies the other two
> -b implies --ignore-space-at-eol
> --ignore-space-at-eol implies only itself
>
> Therefore it is never necessary to specify more than one of these
> on the command line. Yet we imagine scenarios where software
> wrappers (e.g. GUIs, etc) generate command lines that switch on
> more than one of these flags simultaneously. It is unreasonable
> to prohibit specifying more than one, since a new user might not
> immediately discern the implication relation. Now we call such
> a command line valid and legal.
>
> Signed-off-by: Keith Cascio <keith@xxxxxxxxxxx>
> ---
This does not really look all that similar to other commit messages.
For example, "Note: I do not fully understand why this fix works, but it
passes all 68 t4???-* diff test scripts." is rather discouraging. If you
are not convinced, how should we be?
However, I almost can excuse that, but...
> t/t4015-diff-whitespace.sh | 8 ++++----
> xdiff/xutils.c | 22 ++++++++++++----------
> 2 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index d7974d1..b9bda86 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -245,17 +245,19 @@ static unsigned long
> xdl_hash_record_with_whitespace(char const **data,
> while (ptr + 1 < top && isspace(ptr[1])
> && ptr[1] != '\n')
> ptr++;
> - if (flags & XDF_IGNORE_WHITESPACE_CHANGE
> - && ptr[1] != '\n') {
> - ha += (ha << 5);
> - ha ^= (unsigned long) ' ';
> - }
> - if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
> - && ptr[1] != '\n') {
> - while (ptr2 != ptr + 1) {
> + if( ! ( flags & XDF_IGNORE_WHITESPACE
... this is just plain ugly, not to mention breaking the coding style of
the surrounding code in a rather blatant way.
> )){
> + if( flags & XDF_IGNORE_WHITESPACE_CHANGE
> + && ptr[1] != '\n') {
> ha += (ha << 5);
> - ha ^= (unsigned long) *ptr2;
> - ptr2++;
> + ha ^= (unsigned long) ' ';
> + }
> + else if( flags & XDF_IGNORE_WHITESPACE_AT_EOL
> + && ptr[1] != '\n') {
> + while (ptr2 != ptr + 1) {
> + ha += (ha << 5);
> + ha ^= (unsigned long) *ptr2;
> + ptr2++;
> + }
Besides, I think what you actually wanted is
if (flags & XDF_IGNORE_WHITESPACE)
; /* already handled */
else if (flags & XDF_IGNORE_WHITESPACE_CHANGE)
...
else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL)
...
for improved readability both of the code and the patch.
Ciao,
Dscho
--
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
