Re: NSUser.m: Bug on Windows handling spaces in environement variable values?

[Sheldon Gill]

> Is it worth submitting such changes separately ?  It's generally easier to
> process and apply small changes.  I'd recommend sending in many small
> separate patches rather than large rewritings whenever you can. :-)

Generally, I whole-heartedly agree that small changes are better and easier to 
deal with:

1) NSTimeZone:  Documentation
2) NSProcess:  coding violations
3) NSTimeZone:  Win32 modifications, noted preliminary because (1) wasn't yet 
processed and I knew (4) was pending which has an impact...
4) NSPathUtilities

The fourth change is unfortunately large but such is the nature of the change.  
Sometimes larger changes are necessary:

For my changes:

Wanted to get paths from a configuration file. Create function 'GSReadConf' to 
turn file into a dictionary of (spec,path) pairs.

The 'ImportPath' function takes two arguments, but in really it takes only one 
or the other. It's not supposed to take both. Cleaner to have two separate 
functions with clearly defined behaviour. They are then smaller and good 
candidates for inline.

Now 'userDirectory' in NSUser runs to 196 lines with a lot of duplication. 
Consider what it's supposed to do. It's a prime candiate for re-factoring. 
Makes sense to do so with the first two changes.

Turns out you don't need 'userDirectory' at all with the setup changes so it 
goes away. The functions which currently depend on it have to change.

The 'GSStringFromWin32EnvironmentVariable' function does the right thing WRT 
memory handling but is only locally available in NSUser. It should be used 
whenever you're obtaining an NSString from a Win32 environment variable. So 
it makes sense to move it to a module for wider use. It doesn't really belong 
in amongst the path stuff.

The functions which depend on that have to change.

It goes on from there.

I suppose I could have tried to achieve the same result in small patches but 
that would have taken a lot of work and a lot of time. Compounded by delays 
in getting each patch committed before the next could apply.

I also think that some of the changes wouldn't have made a whole lot of sense 
in an incremental update.

There was too much to change for me to do it reasonably in an incremental 
fashion. Whole-scale was the only way. A module-level re-write seemed the 
best way and I did it in an afternoon ending up with less code that did more, 
faster.

Anyway, my point in replying to Roland was to let people know:
1) I agree it's a bug to be fixed (no need for debate)
2) I've fixed it for my sources so it'll be included with my changes if they 
are accepted. (ie: won't regress)


Regards,
Sheldon

BTW, look at 'parseDef' in gsdoc.m.  That single function runs to 570 lines! I 
think it's a prime candidate for re-factoring, too...