[Freeciv-Dev] Re: (PR#2922) Revised version of data-reorganizattion patch

["Jason Short via RT" <rt@xxxxxxxxxxxxxx>]

Eric S. Raymond wrote:
> Jason Short via RT <rt@xxxxxxxxxxxxxx>:
> 
>>Eric S. Raymond via RT wrote:
>>
>>>This revised version doesn't require top-level specfiles to be patched
>>>in order to work.  See the third FIXME comment for discussion
>>
>>But it does require that the tileset graphics be updated.  Trying to 
>>preserve backward-compatability without really doing so isn't very 
>>helpful, and in this case it prevents compatability checking.
> 
> 
> Well...my thinking was that if I committed the data sirectory, then
> not requiring the specfile change would mean people could easily swap
> the one C patch in and out for testing.

Hmm, true.  Perhaps it would be even better to add the new files, then 
provide a patch for the code and .tilespec files.  Whoever commits this 
patch can also remove the old .specfiles and associated .pngs (and 
change Makefile.am).

>>>@@ -265,7 +259,7 @@
>>>     free(minimap_intro_filename);
>>>     minimap_intro_filename = NULL;
>>>   }
>>>-  /* FIXME: free spec_filenames */
>>>+  /* FIXME: free sprite_dirs */
>>> }
>>
>>Can this be FIXed?
> 
> 
> Yes.  I'm a little puzzled that this wasn't done sooner; the registry.c code
> indicates that all that's needed here is a sipmple free().

Perhaps there is a reason.  But if so, I'd rather find it and document 
it rather than just leave it...

>>>+    strcpy(spritename, entry->d_name);
>>
>>Someone will have to compile freeciv on HURD some day, so all these 
>>little shortcuts will break :-).
> 
> 
> Eh?  This is straight POSIX, I think.  What looks breakable to you?

I'm pretty sure POSIX says something like "MAX_PATH_LEN, if defined, 
gives the maximum length of any path string".  And HURD does not define it.

But don't worry about it; if it's broken somebody will fix it someday...

>>>+          /* don't free old sprite, could be many tags pointing to it */
>>
>>Except that there can't (anymore), right?
> 
> 
> Right...but it also looks to me like the only time you actually want to
> free hashes is just before a reread.

Err, yes, but you don't want to wantonly leak memory before that 
(although this is rare, since there are not many duplicates loaded).  If 
the last pointer to a sprite goes away, it should be freed.  Before a 
duplicately-referenced sprite would only be loaded once, so there could 
be multiple references and it wasn't safe to free it when it was 
replaced.  But if that's no longer the case then we might as well do so.

Speaking of which, have you tried rereading tilesets with the patch?  Do 
this at runtime using the local option, and also try 
disconncting/reconneecting to running servers.

>>This looks like a reasonable patch.  But note that its application needs 
>>to be associated with the addition of 1454 new PNG files, lots of 
>>Makefile.am files (don't forget those!), lots of removed .PNG and .spec 
>>files, and some changed .tilespec files.
> 
> 
> I wasn't planning to remove the old files in the first patch.  Time
> enough for that once this has been tested.
> 
> I'm not sure I understabd the issue with Makefile.am files.  Since
> the graphics files have no dependency relationships that will show
> up in a makre production, why do they need to appear in an automake file?

They need to work with "make dist" and "make distcheck", which means 
they need to be in a Makefile.am under EXTRA_DIST.  I'm pretty sure 
using wildcards and such here will break "make distcheck" - I once tried 
to "simplify" things this way and eventually found out my error.

jason