[GRASS-dev] making g.remove less verbose

GRASS GIS GRASS GIS developer
1

Hallo,

since nobody complained, i commited the "verbosity" patch for g.remove
to cvs.

if there will be no objections, i'll rewrite g.copy and g.rename on
similar way

one question: in cmd/remove.c, lines 61 to 64 list of raster files is
printed to stderr:

            G_warning(
               _("[%s@%s] is a base map. Remove reclassed map%s first:"),
                    name, mapset, (nrmaps > 1 ? "s" : ""));

            fprintf(stderr, " %s", *rmaps);
            for(rmaps++; *rmaps; rmaps++)
                fprintf(stderr, ",%s", *rmaps);
            fprintf(stderr, "\n");

which leds to

g.remove rast=pokus
WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
tmp@jachym

would it make sence, rewrite this fprintfs to G_warning too, so the
result would be:

WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
WARNING: tmp@jachym

?

jachym

----- Forwarded message from Jachym Cepicky <jachym.cepicky@centrum.cz> -----

From: Jachym Cepicky <jachym.cepicky@centrum.cz>
Subject: Re: [GRASS-dev] making grass modules less verbose
Date: Sat, 23 Sep 2006 23:23:48 +0200
To: grass-dev@grass.itc.it

I tryed to rewrite g.remove, so it can be managed via --v/--q flags.

could anybody review this patch (grass6/general/manage/lib/do_remove.c)?
it works but i'm shure, G_messge() text could be generated on some
better way.

ones this is done, I'll look at do_rename.c & friends.

thanks

jachym

On Sat, Sep 23, 2006 at 01:23:38PM +0100, Glynn Clements wrote:

Brad Douglas wrote:

> > Anything which is using fprintf(stderr, ...) should be changed to use
> > G_message() instead. Then the problem goes away, right?
>
> G_message("\n") = '\n\n' on screen. G_message(" ") seems a bit obscure.
> But yes, that particular problem would go away, but potentially creating
> a new one.
>
> Also, in loops outputting text, G_message() is generally not desirable
> because of it's "auto-CR" feature.
>
> Would be better to add a function [eg. G_message2()] that does not
> automatically append '\n' to text?

Are you referring to:

a) printing multiple strings without newlines, then adding a newline
at the end, or

b) including blank lines in the output, or

c) something else?

I don't consider either a) or b) to be sufficiently important to
justify bypassing G_message() in favour of explicit fprintf() calls.

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

_______________________________________________
grass-dev mailing list
grass-dev@grass.itc.it
http://grass.itc.it/mailman/listinfo/grass-dev

--
Jachym Cepicky
e-mail: jachym.cepicky@centrum.cz
URL: http://les-ejk.cz
GPG: http://les-ejk.cz/gnupg_public_key/jachym_cepicky-gpg_public_key.asc
-----------------------------------------
OFFICE:
GDF-Hannover
Mengendamm 16d
30177 Hannover
Germany
e-mail: cepicky@gdf-hannover.de
URL: http://gdf-hannover.de
Tel.: +49 511-39088507

Index: do_remove.c

RCS file: /grassrepository/grass6/general/manage/lib/do_remove.c,v
retrieving revision 1.6
diff -u -r1.6 do_remove.c
--- do_remove.c 3 May 2006 09:46:33 -0000 1.6
+++ do_remove.c 23 Sep 2006 21:22:53 -0000
@@ -15,7 +15,7 @@
     char *mapset;
     int result = 0;

- fprintf (stdout,"REMOVE [%s]\n", old);
+ G_message ("REMOVE [%s]", old);

     len = get_description_len(n);

@@ -29,22 +29,25 @@
             result = 1;
   }
     } else {
- for (i = 0; i < list[n].nelem; i++)
- {
- fprintf (stdout," %-*s ", len, list[n].desc[i]);
- fflush (stdout);
+ if ((mapset = G_find_cell2 (old, "")) == NULL)
+ G_fatal_error(_("Raster file <%s> not found"), old);
+
+ for (i = 0; i < list[n].nelem; i++) {
+

       switch (G_remove (list[n].element[i], old))
       {
       case -1:
- fprintf (stdout,"COULD NOT REMOVE");
+ G_warning (" %-*s %s", len, list[n].desc[i],_("COULD NOT REMOVE"));
                 result = 1;
     break;
       case 0:
- fprintf (stdout,"MISSING");
+ G_message (" %-*s %s", len, list[n].desc[i],_("MISSING"));
+ break;
+ case 1:
+ G_message (" %-*s ", len, list[n].desc[i]);
     break;
       }
- fprintf (stdout,"\n");
   }
     }
     if (strcmp (list[n].element[0], "cell") == 0)

_______________________________________________
grass-dev mailing list
grass-dev@grass.itc.it
http://grass.itc.it/mailman/listinfo/grass-dev

----- End forwarded message -----

--
Jachym Cepicky
e-mail: jachym.cepicky@centrum.cz
URL: http://les-ejk.cz
GPG: http://les-ejk.cz/gnupg_public_key/jachym_cepicky-gpg_public_key.asc
-----------------------------------------
OFFICE:
GDF-Hannover
Mengendamm 16d
30177 Hannover
Germany
e-mail: cepicky@gdf-hannover.de
URL: http://gdf-hannover.de
Tel.: +49 511-39088507

2

On Wed, Sep 27, 2006 at 02:08:48PM +0200, Jachym Cepicky wrote:

Hallo,

since nobody complained, i commited the "verbosity" patch for g.remove
to cvs.

if there will be no objections, i'll rewrite g.copy and g.rename on
similar way

one question: in cmd/remove.c, lines 61 to 64 list of raster files is
printed to stderr:

            G_warning(
               _("[%s@%s] is a base map. Remove reclassed map%s first:"),
                    name, mapset, (nrmaps > 1 ? "s" : ""));

            fprintf(stderr, " %s", *rmaps);
            for(rmaps++; *rmaps; rmaps++)
                fprintf(stderr, ",%s", *rmaps);
            fprintf(stderr, "\n");

which leds to

g.remove rast=pokus
WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
tmp@jachym

would it make sence, rewrite this fprintfs to G_warning too, so the
result would be:

WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
WARNING: tmp@jachym

... this should be avoided somehow. Only one time the "WARNING"
should appear to avoid confusion.

Markus

?

jachym

3

Jachym Cepicky wrote:

one question: in cmd/remove.c, lines 61 to 64 list of raster files is
printed to stderr:

            G_warning(
               _("[%s@%s] is a base map. Remove reclassed map%s
               first:"),
                    name, mapset, (nrmaps > 1 ? "s" : ""));

(_("map%s"), >1?"s":"") is bad. "s" to make plural doesn't always translate.

            fprintf(stderr, " %s", *rmaps);
            for(rmaps++; *rmaps; rmaps++)
                fprintf(stderr, ",%s", *rmaps);
            fprintf(stderr, "\n");

which leds to

g.remove rast=pokus
WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
tmp@jachym

would it make sence, rewrite this fprintfs to G_warning too, so the
result would be:

WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
WARNING: tmp@jachym

Try and include all text in one G_warning(). It will wrap and indent
multi-line text correctly. The above code might overflow it's buffer
though, which would be bad.. so maybe keep the existing way?

Hamish

4

Jachym Cepicky wrote:

since nobody complained, i commited the "verbosity" patch for g.remove
to cvs.

Good.

if there will be no objections, i'll rewrite g.copy and g.rename on
similar way

No objections from me.

one question: in cmd/remove.c, lines 61 to 64 list of raster files is
printed to stderr:

            G_warning(
               _("[%s@%s] is a base map. Remove reclassed map%s first:"),
                    name, mapset, (nrmaps > 1 ? "s" : ""));

            fprintf(stderr, " %s", *rmaps);
            for(rmaps++; *rmaps; rmaps++)
                fprintf(stderr, ",%s", *rmaps);
            fprintf(stderr, "\n");

which leds to

g.remove rast=pokus
WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
tmp@jachym

would it make sence, rewrite this fprintfs to G_warning too, so the
result would be:

WARNING: [pokus@jachym] is a base map. Remove reclassed map first:
WARNING: tmp@jachym

Merge the two inside the loop, so if you have multiple maps you would
get e.g.:

  WARNING: [pokus@jachym] is a base map. Remove reclassed map first: foo@jachym
  WARNING: [pokus@jachym] is a base map. Remove reclassed map first: bar@jachym

However, there are more important issues than the format of the
warning:

1. There should be some way to override this check in case you cannot
remove the reclass map(s) (e.g. due to filesystem permissions). As it
stands, if someone creates a reclass of one of your maps, you can no
longer remove that map with g.remove.

[You can get around this problem by ensuring that other users can't
write reclassed_to files in your mapset directories, but that requires
being wise before the fact.]

2. In the case where the map being removed is a reclass map, the code
doesn't check that the fopen(".../reclassed_to") succeeds; if the base
map is in a different mapset (e.g. PERMANENT), the fopen() may well
fail due to lack of write permission.

3. A related issue appears to apply to g.rename, i.e. it tries to
update any reclass maps with the new name of the base map, and
neglects to check whether the fopen() succeeds.

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

5

hallo,
On Wed, Sep 27, 2006 at 05:13:28PM +0100, Glynn Clements wrote:

Merge the two inside the loop, so if you have multiple maps you would
get e.g.:

  WARNING: [pokus@jachym] is a base map. Remove reclassed map first: foo@jachym
  WARNING: [pokus@jachym] is a base map. Remove reclassed map first: bar@jachym

However, there are more important issues than the format of the
warning:

1. There should be some way to override this check in case you cannot
remove the reclass map(s) (e.g. due to filesystem permissions). As it
stands, if someone creates a reclass of one of your maps, you can no
longer remove that map with g.remove.

[You can get around this problem by ensuring that other users can't
write reclassed_to files in your mapset directories, but that requires
being wise before the fact.]

2. In the case where the map being removed is a reclass map, the code
doesn't check that the fopen(".../reclassed_to") succeeds; if the base
map is in a different mapset (e.g. PERMANENT), the fopen() may well
fail due to lack of write permission.

3. A related issue appears to apply to g.rename, i.e. it tries to
update any reclass maps with the new name of the base map, and
neglects to check whether the fopen() succeeds.

i tryed to update remove.c accoring to your hints. I

    - added -f flag forforce remove of raster map and
    - added some warnings, when removing/rewriting of to_reclass file
      fails

i'm not shure if it works like desired, plese test, patch in attachment

if there will be no objections, i'll continue with g.rename on similar
way

thank you

jachym
--
Jachym Cepicky
e-mail: jachym.cepicky@centrum.cz
URL: http://les-ejk.cz
GPG: http://les-ejk.cz/gnupg_public_key/jachym_cepicky-gpg_public_key.asc
-----------------------------------------
OFFICE:
GDF-Hannover
Mengendamm 16d
30177 Hannover
Germany
e-mail: cepicky@gdf-hannover.de
URL: http://gdf-hannover.de
Tel.: +49 511-39088507

(attachments)

remove.c.patch (3.59 KB)

6

Relevant to discussions about the r.mask script. Is what Jachym is doing a
way to provide a user checkbox in the GUI for overwriting the earlier map
but not give confusion with the global --o overwrite flag?

That is, use -f for "force replacement of old mask with new"

??

Michale
__________________________________________
Michael Barton, Professor of Anthropology
School of Human Evolution & Social Change
Center for Social Dynamics and Complexity
Arizona State University

phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

From: Jachym Cepicky <jachym.cepicky@centrum.cz>
Date: Thu, 28 Sep 2006 13:46:23 +0200
To: Glynn Clements <glynn@gclements.plus.com>
Cc: <grass-dev@grass.itc.it>
Subject: Re: [GRASS-dev] making g.remove less verbose

3. A related issue appears to apply to g.rename, i.e. it tries to
update any reclass maps with the new name of the base map, and
neglects to check whether the fopen() succeeds.

i tryed to update remove.c accoring to your hints. I

    - added -f flag forforce remove of raster map and
    - added some warnings, when removing/rewriting of to_reclass file
      fails

i'm not shure if it works like desired, plese test, patch in attachment

if there will be no objections, i'll continue with g.rename on similar
way

7

Michael Barton wrote:

Relevant to discussions about the r.mask script. Is what Jachym is doing a
way to provide a user checkbox in the GUI for overwriting the earlier map
but not give confusion with the global --o overwrite flag?

That is, use -f for "force replacement of old mask with new"

No. The specific issue here is that g.remove currently won't let you
remove a map which is known to be the base map for one or more reclass
maps. It prints a warning telling you to remove the reclass maps
first.

This is a problem if the reclass maps belong to another user.

The proposed -f switch to g.remove allows you to override the warning
and remove the map anyway (with the consequence that the reclass maps
won't work because they refer to a base map which no longer exists).

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

8

hallo,

On Fri, Sep 29, 2006 at 06:06:59PM +0100, Glynn Clements wrote:

Michael Barton wrote:

> Relevant to discussions about the r.mask script. Is what Jachym is doing a
> way to provide a user checkbox in the GUI for overwriting the earlier map
> but not give confusion with the global --o overwrite flag?
>
> That is, use -f for "force replacement of old mask with new"

No. The specific issue here is that g.remove currently won't let you
remove a map which is known to be the base map for one or more reclass
maps. It prints a warning telling you to remove the reclass maps
first.

This is a problem if the reclass maps belong to another user.

The proposed -f switch to g.remove allows you to override the warning
and remove the map anyway (with the consequence that the reclass maps
won't work because they refer to a base map which no longer exists).

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

if i understand this well, nobody is really protesting against this
    patch, so i commited it.

g.remove --help

...
    -f Force remove
...

jachym

--
Jachym Cepicky
e-mail: jachym.cepicky@centrum.cz
URL: http://les-ejk.cz
GPG: http://les-ejk.cz/gnupg_public_key/jachym_cepicky-gpg_public_key.asc
-----------------------------------------
OFFICE:
Department of Geoinformation Technologies
Zemedelska 3
613 00, Brno
Czech Republick
e-mail: xcepicky@node.mendelu.cz
URL: http://mapserver.mendelu.cz
Tel.: +420 545 134 514