Re: proposal: possibility to read dumped table's name from file

On Tue, Jun 09, 2020 at 11:46:24AM +0200, Pavel Stehule wrote:
> po 8. 6. 2020 v 23:30 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com> napsal:

> I still wonder if a better syntax would use a unified --filter option, whose
> > argument would allow including/excluding any type of object:
> I tried to implement simple format "[+-][tndf] objectname"

Thanks.

> + /* ignore empty rows */
> + if (*line != '\0')

Maybe: if line=='\0': continue

We should also support comments.

> + bool include_filter = false;
> + bool exclude_filter = false;

I think we only need one bool.
You could call it: bool is_exclude = false

> +
> + if (chars < 4)
> + invalid_filter_format(optarg, line, lineno);

I think that check is too lax.
I think it's ok if we require the first char to be [-+] and the 2nd char to be
[dntf]

> + objecttype = line[1];

... but I think this is inadequately "liberal in what it accepts"; I think it
should skip spaces. In my proposed scheme, someone might reasonably write:

> +
> + objectname = &line[3];
> +
> + /* skip initial spaces */
> + while (*objectname == ' ')
> + objectname++;

I suggest to use isspace()

I think we should check that *objectname != '\0', rather than chars>=4, above.

> + if (include_filter)
> + {
> + simple_string_list_append(&table_include_patterns, objectname);
> + dopt.include_everything = false;
> + }
> + else if (exclude_filter)
> + simple_string_list_append(&table_exclude_patterns, objectname);

If you use bool is_exclude, then this becomes "else" and you don't need to
think about checking if (!include && !exclude).

> + else if (objecttype == 'f')
> + {
> + if (include_filter)
> + simple_string_list_append(&foreign_servers_include_patterns, objectname);
> + else if (exclude_filter)
> + invalid_filter_format(optarg, line, lineno);
> + }

I would handle invalid object types as "else: invalid_filter_format()" here,
rather than duplicating above as: !=ALL('d','n','t','f')

> +
> + if (ferror(f))
> + fatal("could not read from file \"%s\": %s",
> + f == stdin ? "stdin" : optarg,
> + strerror(errno));

I think we're allowed to use %m here ?

> + printf(_(" --filter=FILENAME read object names from file\n"));

Object name filter expression, or something..

> + * getline is originaly GNU function, and should not be everywhere still.
originally

> + * Use own reduced implementation.

Did you "reduce" this from another implementation? Where?
What is its license ?

Maybe a line-reader already exists in the frontend (?) .. or maybe it should.

--
Justin