[SSSD] [PATCH] Remove trailing whitespace

Pavel Reichl preichl at redhat.com
Tue Aug 11 12:44:05 UTC 2015


On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> On (11/08/15 15:04), Nikolai Kondrashov wrote:
>> Hi Pavel,
>>
>> It's a good idea to have such a test, thank you! Please see my comments
>> regarding the script (as you requested) below.
>>
>> On 08/11/2015 01:46 PM, Pavel Reichl wrote:
>>> diff --git a/src/tests/whitespace_test.sh b/src/tests/whitespace_test.sh
>>> new file mode 100755
>> We don't really need an extension in an executable file name. It will make it
>> that bit difficult to change the implementation language if we ever need that.
>>
>> We don't put ".elf" on binary files, after all.
>>
>> It's not a big deal, of course, just ranting a little.
>>
>>> index 0000000000000000000000000000000000000000..4f34bdf981996440b89a3e7e5f99ad3b6b3eae34
>>> --- /dev/null
>>> +++ b/src/tests/whitespace_test.sh
>>> @@ -0,0 +1,13 @@
>>> +#!/bin/sh
>> Did you intend /bin/sh here? Most people expect Bash to run their scripts so
>> it might be safer to just specify it explicitly if you don't really need to
>> support the vanilla shell. It might prevent some unexpected failures on
>> systems where /bin/sh isn't Bash (e.g. Debian with Dash installed) in the
>> future, even though it works fine now.
>>
>>> +
>>> +# example: EXCLUDE_FILES="path1\|path2"
>>> +EXCLUDE_FILES="po/ca.po"
>> The name of the variable is somewhat confusing: it's not just files, it's a
>> regex, so there's more syntax, and care must be taken to escape special
>> characters. How about "PATH_EXCLUDE_REGEX", or similar? E.g. the regex above
>> will also match "po/catpoo".
>>
>>> +N=$(git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES | wc -l)
>> The "^.*" combination in the regex is a noop and can be removed for
>> readability. There is no need to use '[]' around the space. Matching just
>> space would skip trailing tabs, the general whitespace pattern '\s' might be
>> better here. So the pattern can become '\s+$'.
>>
>> We don't support running under a path with whitespace, but it's still better
>> to quote the "git rev-parse" command substitution. I.e. write this instead:
>>
>>     "$(git rev-parse --show-toplevel)"
>>
>> The grep pattern is not anchored to anything, which means that we're likely to
>> have unintended matches. E.g. a Makefile.am line with trailing whitespace,
>> mentioning matching file will get ignored. You can anchor it like this:
>>
>>     grep -v "^\\($EXCLUDE_FILES\\):"
>>
>>> +
>>> +if [ "$N" -ne 0 ]; then
>>> +    echo "Trailing whitespace found:"
>>> +    git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES
>>> +    exit 1
>>> +fi
>>> +
>>> +exit 0
>> Nobody likes duplicated code, and in this case there are several ways to avoid
>> that. I like the one using awk, which combined with the above comments can
>> look like this:
>>
>>     #!/bin/bash
>>
>>     # An AWK regex matching tracked file paths to be excluded from the search.
>>     # Example: '.*\.po|README'
>>     PATH_EXCLUDE_REGEX='.*\.po'
>>
>>     git grep -n -I -P '\s+$' -- "$(git rev-parse --show-toplevel)" |
>>         awk -- "
>>             BEGIN {
>>                 found = 0
>>             }
>>             ! /^($PATH_EXCLUDE_REGEX):/ {
>>                 if (!found) {
>>                     print \"Trailing whitespace found:\"
>>                     found = 1
>>                 }
>>                 print
>>             }
>>             END {
>>                 exit found
>>             }
>>         "
>>
>> Note that you don't need the "exit" command or checking the exit status after
>> this command, if it is the last one in the script. Its exit status will be
>> used as the script's exit status.
>>
>> BTW, shouldn't we take care to not use git, or to not run this test in case
>> we're running outside git tree, e.g. from unpacked tarball?
>>
> +1
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Nick, thank you for the comments.

Would you take over this patch? I understand that you already have a 
better solution and I think there's no need to update mine in that case. 
Would you send a patch with proposed solution? There's no time pressure 
on this effort so you would not have to hurry.

Thanks!

Thanks!



More information about the sssd-devel mailing list