Date: Tue, 9 Jun 2020 17:30:42 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: proposal: possibility to read dumped table's name from file
Message-ID: <20200609223042.GF26811@telsasoft.com>
References: 
 <CAFj8pRB10wvW0CC9Xq=1XDs=zCQxer3cbLcNZa+qiX4cUH-G_A@mail.gmail.com>
 <20200529182514.GL17850@telsasoft.com>
 <CAFj8pRDGTeb9OtXqFiE-tnYHCV=HyZQ2T_7S5v_50eJqAnLDwg@mail.gmail.com>
 <20200608212957.GB22473@telsasoft.com>
 <CAFj8pRBP105Qa=2q9MEy_PWmCZGaWd4nt4nB2Gv8WgBO7UbkwA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAFj8pRBP105Qa=2q9MEy_PWmCZGaWd4nt4nB2Gv8WgBO7UbkwA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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@telsasoft.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