On 04/03/2012 08:01 AM, Stephen Gallagher wrote:
> On Tue, 2012-04-03 at 10:08 +0200, Jan Zelený wrote:
>>> Ok the whole set once more:
>>> 1) Fixed the trailing spaces on patch 15 - they showed as warning when
>>> I applied the patch in a different repo
>>> 2) The problem with permissions that Jan have seen was caused by the
>>> fact that when the repo is created as root the permissions on the files
>>> that are used in tests are different. For now I added a different
>>> check. In future there might be a need for some improvement in this
>>> area. I created a task https://fedorahosted.org/sssd/ticket/1284
>>> track this effort.
>>> 3) Added two missing cleanup calls to patch 27 as Jan rightly indicated
>>> that they were missing
>>> All the rest is the same.
>> I'm not completely comfortable with your solution of the access check.
>> You are just assuming in the comment that the file was created by root
>> and has therefore different permissions. But what if it wasn't created
>> by root and still has these different permissions? I think the test
>> could then let an error in the code pass through. Please check that the
>> file indeed belongs to root just to be sure. Also error2 is not
>> necessary IMO, you can re-user error.
>> This is the last minor change I'm asking for. If you want, I'll do it
>> for you. Other than that all patches are fine.
> Actually, I think it's more likely that what's happening here is that
> root and a casual user have a different default umask on most systems.
> So this probably IS a bug in the code. You should be setting the umask
> before creating the file (and resetting the umask to its old value
> afterwards) in order to guarantee that it is always written with the
> proper permissions.
The file is a part of the repo. It is checked into the source control. I
do not have control on its permissions.
I think it is the umask but not during execution but rather during git
Yes this area needs more thought but IMO in a separate patch. And may be
done differently. For example create a file on the fly and make sure it
has specific permissions. But this is way out of the scope of the
current patch. This is why I opened a separate ticket to handle it.
Ok, thanks for clarification. Since the issue is in the test suite and it has
been worked around, I give these patches a green light.
Ack to all