Re: [PATCH] maint: use predetermined NON_ROOT_GROUP in tests

[Pádraig Brady]

On 06/26/2014 01:25 PM, Bernhard Voelker wrote:
> On 06/26/2014 01:47 PM, Pádraig Brady wrote:
>> * tests/misc/chroot-credentials.sh: Avoid gid lookup.
> 
> Good catch!
> 
>> * tests/misc/truncate-owned-by-other.sh: Likewise.
>> * tests/touch/now-owned-by-other.sh: Likewise.
>> * tests/id/setgid.sh: Use previously looked up gid as a more
>> accurate base for the subseuqent adjustment, and move
> 
> s/subseuqent/subsequent/
> 
>> the uid lookup within chroot, rather than having the overhead
>> of a separate `id` invocation.
>> ---
>>   tests/id/setgid.sh                    |    7 +++----
>>   tests/misc/chroot-credentials.sh      |    6 +++---
>>   tests/misc/truncate-owned-by-other.sh |    4 +---
>>   tests/touch/now-owned-by-other.sh     |    4 +---
>>   4 files changed, 8 insertions(+), 13 deletions(-)
>>
>> diff --git a/tests/id/setgid.sh b/tests/id/setgid.sh
>> index 0664c47..ce8f83a 100755
>> --- a/tests/id/setgid.sh
>> +++ b/tests/id/setgid.sh
>> @@ -20,8 +20,7 @@
>>   print_ver_ id
>>   require_root_
>>
>> -u=$(id -u $NON_ROOT_USERNAME) || framework_failure_
>> -g=$u
>> +g=$NON_ROOT_GROUP
> 
> Why not avoid the variable 'g' at all, then?
> It's just used once ... here:
> 
>>
>>   # Construct a different group number.
>>   gp1=$(expr $g + 1)
> 
> 
>> @@ -29,12 +28,12 @@ gp1=$(expr $g + 1)
>>   echo $gp1 > exp || framework_failure_
>>
>>   # With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
>> -chroot --user=+$u:+$gp1 --groups='' / env PATH="$PATH" \
>> +chroot --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / env PATH="$PATH" \
>>     id -G > out || fail=1
>>   compare exp out || { cat out; fail=1; }
> 
> While at it, no 'cat out' needed here.
> 
>>   # With coreutils-8.22 and earlier, id would erroneously print groups=$g
>> -chroot --user=+$u:+$gp1 --groups='' / env PATH="$PATH" \
>> +chroot --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / env PATH="$PATH" \
>>     id > out || fail=1
>>   grep -F "groups=$gp1" out || { cat out; fail=1; }
>>
>> diff --git a/tests/misc/chroot-credentials.sh 
>> b/tests/misc/chroot-credentials.sh
>> index d50704c..dd08b5c 100755
>> --- a/tests/misc/chroot-credentials.sh
>> +++ b/tests/misc/chroot-credentials.sh
>> @@ -29,7 +29,7 @@ root=$(id -nu 0) || skip_ "Couldn't look up root username"
>>
>>   # verify numeric IDs looked up similarly to names
>>   NON_ROOT_UID=$(id -u $NON_ROOT_USERNAME)
>> -NON_ROOT_GID=$(id -g $NON_ROOT_USERNAME)
>> +NON_ROOT_GID=$NON_ROOT_GROUP
> 
> same here:
> why not use NON_ROOT_GROUP directly - or even better
> s/NON_ROOT_GROUP/NON_ROOT_GID/ everywhere?
> 
> Thanks & have a nice day,
> Berny
> 

Pushed with those adjustments.
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=acb422bdd

thanks!
Pádraig.