Re: [PATCH] ls - colorize files with capabilities

[Jim Meyering]

Kamil Dudka <address@hidden> wrote:
> From 4f14f5e39d7687ddb252f5a7560724179635c944 Mon Sep 17 00:00:00 2001
> From: Kamil Dudka <address@hidden>
> Date: Tue, 22 Jul 2008 12:53:00 +0200
> Subject: [PATCH] --color now highlights files with capabilities, too

Please make the first line of the log message start with
the name of the affected tool:

ls: --color now highlights files with capabilities, too

>  * configure.ac: --disable-libcap configure parameter, check for libcap 
> usability
>  * src/Makefile.am: libcap library linking
>  * src/ls.c (has_capability): new function for capability detection
>  * src/dircolors.c: updated color lists
>  * src/dircolors.hin: brief description of CAPABILITY color attribute
>  (print_color_indicator): colorize file with capability
>  * tests/ls/capability: test for ls - colorize file with capability (root 
> only)
>  * tests/Makefile.am (TESTS): Add ls/capability.
>  * NEWS: mentioned the change

While you're editing the log, please remove the spaces before each "*"
and adjust the rest to look like this: note changes in tense, wording,
punctuation, capitalization.

* configure.ac: New option: --disable-libcap.  Check for libcap usability.
* src/Makefile.am (dir_LDADD, ls_LDADD, ...): Append $(LIB_CAP).
* src/ls.c [HAVE_CAP] Include <sys/capability.h>.
(has_capability): New function for capability detection.
* src/dircolors.c: Update color lists.
* src/dircolors.hin: Mention new CAPABILITY color attribute.
(print_color_indicator): Colorize file with capability.
* tests/ls/capability: Test for ls - colorize file with capability.
* tests/Makefile.am (TESTS): Add ls/capability.
* NEWS: Mention the change.

> diff --git a/tests/ls/capability b/tests/ls/capability
...
> +. $srcdir/test-lib.sh
> +require_root_
> +
> +which setcap >/dev/null || skip_test_ "setcap utility not found"

It it not portable to use "which".  Do this instead:
[currently, it doesn't recognize --help, but someday it may]

(setcap --help) 2>&1 |grep 'usage: setcap' > /dev/null \
  || skip_test_ "setcap utility not found"

> +fail=0
> +
> +# Don't let a different umask perturb the results.
> +umask 22
> +
> +touch test
> +setcap cap_net_bind_service=ep test || \
> +  skip_test_ "can't set capability"

When you leave "||" at the end of the line,
the trailing "\" is not needed.  However, I find it
slightly more readable to put the operator at the beginning
of the next line, so keep the "\":
(also, once we know we have setcap, any failure is
worth more than a mere "skip", so use framework_failure):

  setcap cap_net_bind_service=ep test \
    || framework_failure

> +LS_COLORS='ca=30;41' \
> +  ls --color=always test > out || fail=1
> +printf "\033[0m\033[30;41mtest\033[0m\n\033[m" > out_ok

It's slightly better to factor out the "30;41", e.g.,
(also note that we must detect if/when printf fails; otherwise,
the test could mistakenly pass if ls were somehow to output nothing
in spite of a 0 exit status, and the printf failed e.g., due to disk
full)

  code='30;41'
  LS_COLORS="ca=$code" \
    ls --color=always test > out || fail=1
  printf "\033[0m\033[${code}mtest\033[0m\n\033[m" > out_ok || fail=1

> +diff out out_ok || fail=1

Use the "compare" functions, not "diff".

  compare out out_ok || fail=1

> +rm -f test

There is no need to remove temporary files, so please don't.
test-lib.sh arranges for each test to be run in its own directory,
and then removes that directory upon completion.

> +(exit $fail); exit $fail