On Tue, Dec 1, 2015 at 11:46 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > 2015-11-30 15:17 GMT+01:00 Michael Paquier <michael.paquier@gmail.com>: >> Removing some items from the list of potential actions and creating a >> new sublist listing action types is a bit weird. Why not grouping them >> together and allow for example -l as well in the list of things that >> is considered as a repeatable action? It seems to me that we could >> simplify the code this way, and instead of ACT_NOTHING we could check >> if the list of actions is empty or not. > > > fixed
Thanks for the patch. I have to admit that adding ACT_LIST_DB in the list of actions was not actually a good idea. It makes the patch uglier.
Except that, I just had an in-depth look at this patch, and there are a couple of things that looked strange: - ACT_LIST_DB would live better if removed from the list of actions and be used as a separate, independent option. My previous suggestion was unadapted. Sorry. - There is not much meaning to have simple_action_list_append and all its structures in common.c and common.h. Its use is limited in startup.c (this code is basically a duplicate of dumputils.c still things are rather different, justifying the duplication) and centralized around parse_psql_options. - use_stdin is not necessary. It is sufficient to rely on actions.head == NULL instead. - The documentation is pretty clear. That's nice. Attached is a patch implementing those suggestions. This simplifies the code without changing its usefulness. If you are fine with those changes I will switch this patch as ready for committer. Regards,
