[GRASS-dev] [GRASS GIS] #2289: v.colors not working

[Glynn Clements]

Huidae Cho wrote:

> I agree. Maybe, Markus M has a better idea why we needed r60149?

AFAICT, the reason for the logic change is so that it doesn't print a
warning if the file simply doesn't exist.

The previous version would only call G_find_file2() if the mapset
couldn't otherwise be determined, i.e. the name was unqualified and no
mapset was given. If the mapset could be determined either from a
qualified name or a non-empty mapset parameter, a non-existent file
wouldn't be detected until the open() call failed.

The new version always calls G_find_file2() first, so open() should
never be called for a non-existent file (or one which is inaccessible
due to the current process lacking execute (search) permission on any
ancestor directory). So a failed open() would indicate something less
mundane than a missing file.

I'm assuming that some code used the functions for a combined "check
whether it exists, and if so, open it" function. If that's the case,
warning about non-existent files would be a nuisance.

I'm still not sure whether this behaviour is a good idea, or whether
we should always generate the warning and require callers to perform
an explicit G_find_* call if the file's existence is merely "useful"
rather than either "required" or at least "expected".

And none of this changes the fact that Vect_read_colors() was broken
prior to r60225, and is still broken now.

If a function has parameters which happen to be used as the various
components of a filename, code shouldn't assume that they're just
going to get glued together and that it doesn't matter about the
individual pieces.

It's still broken now because the first argument to Rast__read_colors
is supposed to be an "element name", not two directory names glued
together. That it happens to work as a coincidence of implementation
details doesn't make it correct (in fact, "happens to work as a
coincidence of implementation details" is pretty much the definition
of a "hack").

Given that the current prototype for Rast__read_colors() doesn't
actually support reading colours for a vector map (because of the
vector code using the <mapset>/vector/<map>/<element> layout rather
than <mapset>/<element>/<map>), the correct solution is to add a new
function which takes a FILE* and let Vlib deal with opening (and
closing) the file.

-- 
Glynn Clements <glynn at gclements.plus.com>