Giuseppe Scrivano wrote:
> Jim, thank for the review.
>
> Jim Meyering <[email protected]> writes:
>
>> Perhaps "prev_wd" would be more accurate?
>
> I fixed it. The same name is used in `tail_forever', that is why I used
> it, should it be changed in `tail_forever' too?
>
>
>> Another regression:
>>
>> touch k; chmod 0 k; tail -F k
>>
>> fails to open "k" (as it must), but even
>> when I run "chmod u+r k" in another window,
>> the inotify-based version of tail fails to open it.
>
> I fixed it.
>
> This version includes, among other things, a refactoring of the new
> tests as previously reported.
...
Thanks for yet another round.
I'll look more closely tomorrow, but here are things I noticed right away:
Comment style. You added comments like this. Thanks!
/* Add an inotify watch for each watched file. If -F is specified then watch
* its parent directory too, in this way when they re-appear we can add them
* again to the watch list. */
Please remove the leading '*'s:
/* Add an inotify watch for each watched file. If -F is specified then watch
its parent directory too, in this way when they re-appear we can add them
again to the watch list. */
> diff --git a/tests/test-lib.sh b/tests/test-lib.sh
> index a765bd6..a9684fb 100644
> --- a/tests/test-lib.sh
> +++ b/tests/test-lib.sh
> @@ -122,6 +122,11 @@ uid_is_privileged_()
> esac
> }
>
> +get_process_status_()
> +{
> + echo $(sed -n '/^State:[ \t]*\([[:alpha:]]\)\(.*\)/s//\1/p'
> /proc/$1/status)
> +}
No need for echo and the subshell.
Sadly, \t is not portable
No need for 2nd \(...\) group
get_process_status_()
{
sed -n '/^State:[ ]*\([[:alpha:]]\).*/s//\1/p' /proc/$1/status
}
> # Convert an ls-style permission string, like drwxr----x and -rw-r-x-wx
> # to the equivalent chmod --mode (-m) argument, (=,u=rwx,g=r,o=x and
> # =,u=rw,g=rx,o=wx). Ignore ACLs.
> @@ -234,6 +239,17 @@ of group names or numbers. E.g.,
> esac
> }
>
> +# Is /proc/$PID/status supported?
> +require_proc_pid_status_()
> +{
> + sleep 2 &
> + local pid=$!
> + sleep .5
> + grep '^State:[ ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
> + skip_test_ "/proc/$pid/status: missing or 'different'"
Indent a statement like that to make it easier to see that
it is conditional:
grep '^State:[ ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
skip_test_ "/proc/$pid/status: missing or 'different'"
> + kill $pid
> +}
> +
> # Does the current (working-dir) file system support sparse files?
> require_sparse_support_()
> {
_______________________________________________
Bug-coreutils mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/bug-coreutils
