[GRASS GIS] #676: r.watershed: different output map options do not allow for fully qualified map names

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-------------------------+--------------------------------------------------
 Reporter:  mlennert     |       Owner:  [hidden email]
     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

 Moritz

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/676>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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              
-----------------------+----------------------------------------------------
Changes (by hamish):

  * keywords:  r.mapcalc => r.watershed
  * component:  default => Raster

Comment:

 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:1>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.

 Moritz

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:2>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.


 Hamish

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:3>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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...

 Moritz

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/676#comment:4>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.

 Markus M

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:5>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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 ?

 Moritz

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/676#comment:6>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/676#comment:7>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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 ?

 Should this also be done in 6.* ?

 Moritz

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:8>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.


 Hamish

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:9>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.

 Markus M

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/676#comment:10>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.* ?

 Ideally, yes.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:11>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev

[GRASS GIS]

#676: r.watershed: different output map options do not allow for fully qualified
map names
-----------------------+----------------------------------------------------
  Reporter:  mlennert  |       Owner:  [hidden email]
      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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/676#comment:12>
GRASS GIS <http://grass.osgeo.org>

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev