Re: NSPathUtilities Patch - 6 - NSUserDefaults

[David Ayers]

Sheldon Gill wrote:

> Changes to NSUserDefaults implementation which:
>
> Simplifies establishing defaults database path
>
> Removed superfluous behaviours like creating additional paths which are 
> created when needed elsewhere anyway.
>
>
> Regards,
> Sheldon
>
>
> ------------------------------------------------------------------------
>
> --- ../cvsver/core/base/Source/NSUserDefaults.m 2004-02-10 13:17:10.000000000 
> +0800
> +++ ../core/base/Source/NSUserDefaults.m        2004-01-20 23:05:28.000000000 
> +0800
> @@ -239,7 +239,7 @@
>   * user ID.  Needed by setuid processes which change the user they
>   * are running as.<br />
>   * In GNUstep you should call GSSetUserName() when changing your
> - * effective user ID, and that class will call this function for you.
> + * effective user ID, and that function will call this function for you.
>   */
>  + (void) resetStandardUserDefaults
>  {
> @@ -613,25 +613,22 @@
>    return [self initWithUser: NSUserName()];
>  }
>  
> +/* Returns the path to the user's ".GNUstepDefaults file" */
>  static NSString        *pathForUser(NSString *user)
>  {
>    NSString     *database = @".GNUstepDefaults";
>    NSFileManager        *mgr = [NSFileManager defaultManager];
> -  NSString     *home;
>    NSString     *path;
> -  NSString     *old;
>    unsigned     desired;
>    NSDictionary *attr;
>    BOOL         isDir;
>  
> -  home = GSDefaultsRootForUser(user);
> -  if (home == nil)
> +  path = GSDefaultsRootForUser(user);
> +  if (path == nil)
>      {
> -      /* Probably on MINGW. Where to put it? */
> -      NSLog(@"Could not get user root. Using NSOpenStepRootDirectory()");
> -      home = NSOpenStepRootDirectory();
> +      return [NSSearchPathForDirectoriesInDomains(NSUserDirectory,
> +               NSUserDomainMask, YES) lastObject];

I don't understand this but it may very well have historical background. 
 We are asking for the path of a specific user and here we'd be 
returning the home directory of the current user.  If this function is 
only used for the current user then I think the parameter is 
superfluous.  Returning the current user seems very wrong at least w/o 
checking whether 'user' is the current user.  (OTOH, I can't think of a 
legitimate usage to access a different users defaults database with the 
exception of some specialized administrative setuid applications that 
I'm not sure we should worry too much about in -base).

>      }
> -  path = [home stringByAppendingPathComponent: @"Defaults"];
>  
>  #if	!(defined(S_IRUSR) && defined(S_IWUSR) && defined(S_IXUSR) \
>    && defined(S_IRGRP) && defined(S_IXGRP) \
> @@ -645,80 +642,28 @@
>      [NSNumberClass numberWithUnsignedLong: desired], NSFilePosixPermissions,
>      nil];
>  
> -  if ([mgr fileExistsAtPath: home isDirectory: &isDir] == NO)
> -    {
> -      if ([mgr createDirectoryAtPath: home attributes: attr] == NO)
> -       {
> -         NSLog(@"Defaults home '%@' does not exist - failed to create it.",
> -           home);
> -         return nil;
> -       }
> -      else
> -       {
> -         NSLog(@"Defaults home '%@' did not exist - created it", home);
> -         isDir = YES;
> -       }
> -    }
> -  if (isDir == NO)
> -    {
> -      NSLog(@"ERROR - defaults home '%@' is not a directory!", home);
> -      return nil;
> -    }
> -
>    if ([mgr fileExistsAtPath: path isDirectory: &isDir] == NO)
>      {
>        if ([mgr createDirectoryAtPath: path attributes: attr] == NO)
> -       {
> -         NSLog(@"Defaults path '%@' does not exist - failed to create it.",
> -           path);
> -         return nil;
> -       }
> +        {
> +          NSLog(@"Defaults home '%@' does not exist - failed to create it.",
> +            path);
> +          return nil;
> +        }
>        else
> -       {
> -         NSLog(@"Defaults path '%@' did not exist - created it", path);
> -         isDir = YES;
> -       }
> +        {
> +          NSLog(@"Defaults home '%@' did not exist - created it", path);
> +          isDir = YES;
> +        }

This seems like whitespace.  I think you can commit/post this as obvious.

>      }
>    if (isDir == NO)
>      {
> -      NSLog(@"ERROR - Defaults path '%@' is not a directory!", path);
> +      NSLog(@"ERROR - defaults home '%@' is not a directory!", path);

Ditto.

>        return nil;
>      }
>  
> +//  NSLog(@"Determined Defaults home to be '%@'\n",path);

Could you remove this or use some NSDebugLog function.

>    path = [path stringByAppendingPathComponent: database];
> -  old = [home stringByAppendingPathComponent: database];
> -  if ([mgr fileExistsAtPath: path] == NO)
> -    {
> -      if ([mgr fileExistsAtPath: old] == YES)
> -       {
> -         if ([mgr movePath: old toPath: path handler: nil] == YES)
> -           {
> -             NSLog(@"Moved defaults database from old location (%@) to %@",
> -               old, path);
> -           }
> -       }
> -    }
> -  if ([mgr fileExistsAtPath: old] == YES)
> -    {
> -      NSLog(@"Warning - ignoring old defaults database in %@", old);
> -    }
> -
> -  /*
> -   * Try to create standard directory hierarchy if necessary
> -   */
> -  home = [NSSearchPathForDirectoriesInDomains(NSUserDirectory,
> -    NSUserDomainMask, YES) lastObject];
> -  if (home != nil)
> -    {
> -      NSString *p;
> -
> -      p = [home stringByAppendingPathComponent: @"Library"];
> -      if ([mgr fileExistsAtPath: p isDirectory: &isDir] == NO)
> -       {
> -         [mgr createDirectoryAtPath: p attributes: attr];
> -       }
> -    }
> -

I haven't followed closely, but has the location change seen a major 
(stable) release cycle?

>    return path;
>  }
>  
> @@ -1431,7 +1376,7 @@
>         {
>           NSLog(@"Creating defaults database file %@", _defaultsDatabase);
>           [newDict writeToFile: _defaultsDatabase atomically: YES];
> -         attr = [mgr fileAttributesAtPath: _defaultsDatabase
> +            attr = [mgr fileAttributesAtPath: _defaultsDatabase

This looks unintentional.

>                               traverseLink: YES];
>         }
>      }
> @@ -1452,6 +1397,7 @@
>         }
>      }
>  
> +#ifdef  OPTION_ENFORCE_DEFAULTS_PERMS

Do we really want these conditional here?  I think I remember the 
discussion that someone my want to change the permissions of his 
defaults database.  I can't say that I agree but if it must be, could we 
please make it dependent on a default instead of conditional 
compilation?  When someone installs a GNUstep package, they should worry 
about about configuration options used to build it as little as possible.

>    /*
>     * We enforce the permission mode 0600 on the defaults database
>     */
> @@ -1476,6 +1422,7 @@
>        [mgr changeFileAttributes: enforced_attributes
>                          atPath: _defaultsDatabase];
>      }
> +#endif // OPTION_ENFORCE_DEFAULTS_PERMS
>  
>    if (_changedDomains != nil)
>      {           // Synchronize both dictionaries