r.watershed: different output map options do not allow for fully qualified map names

[mlennert]

The different output map options do not allow for fully qualified map names (i.e. containing @mapset)

ex: GRASS 6.4.0svn (gisdemo_ncspm):~ > r.watershed --overwrite elevation=elevation@PERMANENT threshold=10000 basin=watersheds@sqlite Illegal filename. Character <@> not allowed. ERROR: <watersheds@sqlite> is an illegal file name

GRASS 6.4.0svn (gisdemo_ncspm):~ > r.watershed --overwrite elevation=elevation@PERMANENT threshold=10000 stream=streams@sqlite Illegal filename. Character <@> not allowed. ERROR: <streams@sqlite> is an illegal file name

Moritz

[hamish]

you may only write to the current mapset. therefore @othermapset is not needed for output maps.

if it is given, and the value is the current mapset, then perhaps it should be quietly ignored. hopefully it could be done at the library level. if it can't be handled at the lib level I'd be inclined to ignore it rather than add clutter to all raster modules.

Hamish

[mlennert]

Replying to hamish:

you may only write to the current mapset. therefore @othermapset is not needed for output maps.

if it is given, and the value is the current mapset, then perhaps it should be quietly ignored. hopefully it could be done at the library level. if it can't be handled at the lib level I'd be inclined to ignore it rather than add clutter to all raster modules.

Ok, but then the wxGUI should not automatically add @mapset when you chose an existing file for output. So, maybe this should be reclassified as a wxGUI bug.

Moritz

[hamish]

Replying to mlennert:

Ok, but then the wxGUI should not automatically add @mapset when you chose an existing file for output. So, maybe this should be reclassified as a wxGUI bug.

Output raster options with

 opt->gisprompt = "new,cell,raster";

should not be presenting the user with a list of existing maps to choose from in the first place.

Hamish

[mlennert]

Replying to hamish:

Replying to mlennert:

Ok, but then the wxGUI should not automatically add @mapset when you chose an existing file for output. So, maybe this should be reclassified as a wxGUI bug.

Output raster options with

 opt->gisprompt = "new,cell,raster";

should not be presenting the user with a list of existing maps to choose from in the first place.

Well, it _is_ convienient in the GUI to be able to chose an existing map if you want to replace it...

Moritz

[mmetz]

Replying to mlennert:

Replying to hamish:

Replying to mlennert:

Ok, but then the wxGUI should not automatically add @mapset when you chose an existing file for output. So, maybe this should be reclassified as a wxGUI bug.

Output raster options with

 opt->gisprompt = "new,cell,raster";

should not be presenting the user with a list of existing maps to choose from in the first place.

Well, it _is_ convienient in the GUI to be able to chose an existing map if you want to replace it...

I agree, but should it then be the responsibility of the module to strip any @mapset parts or the responsibility of the parser? It seems that this is a more general question that also applies to other raster as well as vector modules.

Markus M

[mlennert]

Replying to mmetz:

Replying to mlennert:

Replying to hamish:

Replying to mlennert:

Ok, but then the wxGUI should not automatically add @mapset when you chose an existing file for output. So, maybe this should be reclassified as a wxGUI bug.

Output raster options with

 opt->gisprompt = "new,cell,raster";

should not be presenting the user with a list of existing maps to choose from in the first place.

Well, it _is_ convienient in the GUI to be able to chose an existing map if you want to replace it...

I agree, but should it then be the responsibility of the module to strip any @mapset parts or the responsibility of the parser? It seems that this is a more general question that also applies to other raster as well as vector modules.

IIUC, the problem actually comes from the use of G_legal_filename() in these modules (r.reclass is another of them). So the question is: should modules check for legality of given file names ? And if yes, should they use G_legal_filename(). And if yes, should G_legal_filename() check for fully qualified names ?

Moritz

[glynn]

Replying to mlennert:

IIUC, the problem actually comes from the use of G_legal_filename() in these modules (r.reclass is another of them). So the question is: should modules check for legality of given file names ?

File names or map names? G_legal_filename() checks that the argument is valid as a single component of the path, i.e. a valid location name, mapset name, (unqualified) map name, or element name.

And if yes, should they use G_legal_filename().

Modules shouldn't normally use this function. It's meant for use by low-level library functions.

And if yes, should G_legal_filename() check for fully qualified names ?

No. It's called G_legal_filename() rather than e.g. G_legal_map_name() because it deals with filenames, not map names.

If modules are passing map names to it, the modules should be fixed.

[mlennert]

Replying to glynn:

Replying to mlennert:

IIUC, the problem actually comes from the use of G_legal_filename() in these modules (r.reclass is another of them). So the question is: should modules check for legality of given file names ?

File names or map names?

I meant map names.

And if yes, should they use G_legal_filename().

Modules shouldn't normally use this function. It's meant for use by low-level library functions.

Can we just delete the check then ? In any case there will be an error at creation if the map name is illegal, or ?

If modules are passing map names to it, the modules should be fixed.

Grepping for G_legal_filename in grass6 (trunk and 64 releasebranch) throws up lots of occurences which at first sight seem to pass map names. None left in grass7, so I guess you have already cleaned up ?

Should this also be done in 6.* ?

Moritz

[hamish]

The problem is not G_legal_filename(). The problem is that you should not under normal circumstances be passing fully qualified map names to output= options.

And if you do, it would be useful for something* to strip it away, and exit with an error if the value given was not the current mapset.

[*] hopefully that something is done at the library level so a new fn() wouldn't have to be added to all modules, either in G_parser() or in the opening of a new map for write. I understand that opening a new map to write might be downstream of a G_legal_filename() check already in the module, but if you say that is already "cleaned" in GRASS 7 then all that needs to be done there is add the @ stripping code to either/both of G_parser() and or whatever the Rast_open_new_cell for write fn name is called now.

Should this also be done in 6.* ?

definitely not 6.4 -- critical bug fixes only at this point.

As you may imagine I am very skeptical to touch 6.5 in any big way for this small annoyance except for removing or enhancing the GUI map chooser button -- AFAIAC all 6.x is closed for refactoring. That is what SVN trunk is for.

Well, it _is_ convienient in the GUI to be able to chose an existing map if you want to replace it...

If you keep the module GUI window open it remains there. And you can do your Run,Run,Run trial and error again and again. Otherwise I'd just say if you want to do destruction of old data you must retype it yourself.

The user must take positive action to destroy their data. Convenience is not a priority in that situation- in fact you want the magnet to slightly repel.

Hamish

[mmetz]

Replying to hamish:

you may only write to the current mapset. therefore @othermapset is not needed for output maps.

if it is given, and the value is the current mapset, then perhaps it should be quietly ignored. hopefully it could be done at the library level. if it can't be handled at the lib level I'd be inclined to ignore it rather than add clutter to all raster modules.

FWIW, Vect_open_new() does that job for vectors, it removes any @mapset from fully qualified names. Should Rast_open_new() behave similarly? Then it would be handled on library level, raster modules need not be touched. And opening new raster maps and new vector maps would be more consistent.

Markus M

[glynn]

Replying to mlennert:

And if yes, should they use G_legal_filename().

Modules shouldn't normally use this function. It's meant for use by low-level library functions.

Can we just delete the check then ? In any case there will be an error at creation if the map name is illegal, or ?

The library functions which create maps, support files, etc should be calling G_legal_filename() as needed.

If modules are passing map names to it, the modules should be fixed.

Grepping for G_legal_filename in grass6 (trunk and 64 releasebranch) throws up lots of occurences which at first sight seem to pass map names. None left in grass7, so I guess you have already cleaned up ?

Should this also be done in 6.* ?

Ideally, yes.

[glynn]

Replying to hamish:

The problem is not G_legal_filename(). The problem is that you should not under normal circumstances be passing fully qualified map names to output= options.

Regardless of whether anything should or should not be doing this, passing a qualified map name for output maps is legal, so long as the mapset is the current mapset. E.g. G__open_raster_new() will happily accept map@mapset provided that mapset==G_mapset().

And if you do, it would be useful for something* to strip it away, and exit with an error if the value given was not the current mapset.

[*] hopefully that something is done at the library level so a new fn() wouldn't have to be added to all modules, either in G_parser() or in the opening of a new map for write. I understand that opening a new map to write might be downstream of a G_legal_filename() check already in the module, but if you say that is already "cleaned" in GRASS 7 then all that needs to be done there is add the @ stripping code to either/both of G_parser() and or whatever the Rast_open_new_cell for write fn name is called now.

There's no need to strip anything. Library functions should always accept qualified map names. For output maps, the mapset must be the current mapset, and unqualified names should use the current mapset rather than the mapset search path (which isn't required to include the current mapset).

The main problem is individual modules getting in the way and attempting to enforce bogus rules.