#676: r.watershed: different output map options do not allow for fully qualified
map names
-------------------------+--------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: default | Version: 6.4.0 RCs
Keywords: r.mapcalc | Platform: Linux
Cpu: Unspecified |
-------------------------+--------------------------------------------------
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
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.
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by mlennert):
Replying to [comment:1 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.
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by hamish):
Replying to [comment:2 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.
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by mlennert):
Replying to [comment:3 hamish]:
> Replying to [comment:2 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...
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by mmetz):
Replying to [comment:4 mlennert]:
> Replying to [comment:3 hamish]:
> > Replying to [comment:2 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.
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by mlennert):
Replying to [comment:5 mmetz]:
> Replying to [comment:4 mlennert]:
> > Replying to [comment:3 hamish]:
> > > Replying to [comment:2 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 ?
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by glynn):
Replying to [comment:6 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.
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by mlennert):
Replying to [comment:7 glynn]:
> Replying to [comment:6 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 ?
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by 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.
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by mmetz):
Replying to [comment:1 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.
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by glynn):
Replying to [comment:8 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.* ?
#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: normal | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed
Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by glynn):
Replying to [comment:9 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.