Download raw body.
Cleanup of mutually exclusive flag errors
On Sat, Dec 12, 2020 at 09:58:00PM +0100, Stefan Sperling wrote:
> On Sat, Dec 12, 2020 at 03:44:58PM -0500, Josh Rickmar wrote:
> > On Sat, Dec 12, 2020 at 09:26:15PM +0100, Stefan Sperling wrote:
> > > On Sat, Dec 12, 2020 at 02:58:42PM -0500, Josh Rickmar wrote:
> > > > This adds a struct and some functions to detect and deal with mutually
> > > > exclusive flags presented by the user during the histedit and ref
> > > > commands, which allows the removal of many manual flag checks.
> > > >
> > > > Other commands don't have flag sets with so many mutually exclusive
> > > > options, so it didn't seem worthwhile to use the new mechanism there.
> > > >
> > > > The histedit command was the only one where these mutually exclusive
> > > > flag errors reported the command in use, so to be consistent with the
> > > > rest of the commands, only the options are told, without repeating the
> > > > command name.
> > > >
> > > > This diff fixes an issue I missed during my earlier histedit -f patch
> > > > where it did not error when histedit -f and -F were used together.
> > > >
> > > > ok?
> > >
> > > I see two downsides to this approach.
> > >
> > > The first downside is it doesn't seem flexible enough.
> > > I don't see how it could support a case where a flag does not exclude all
> > > of the other options, but only some of the other options. So, in general
> > > there will be cases that would need to be handled by another mechanism.
> > >
> > > The second downside is that I think we're losing readibility.
> > > To figure out which options don't work together, readers now have to reason
> > > about code flow instead of reading a condition which clearly states in which
> > > case the author of the code intended options to conflict, like this:
> > >
> > > if (abort_edit && continue_edit)
> > >
> > > If the verbosity of the code that generates the error is seen as a problem,
> > > there are other ways to improve this. For example, the patch below avoids
> > > repeition of the error message string.
> >
> > I do like this. Couple of comments below.
> >
> > >
> > > diff 9f6bb280654be7061fc00305743f6ace71f9a1cb /home/stsp/src/got
> > > blob - 7bdd9e142a5f26254c73c308684bb6d6da95ef20
> > > file + got/got.c
> > > --- got/got.c
> > > +++ got/got.c
> > > @@ -1324,6 +1324,12 @@ create_config_files(const char *proto, const char *hos
> > > mirror_references, repo);
> > > }
> > >
> > > +static void
> >
> > Should be __dead.
>
> Oh, yes.
>
> > Also if we're strictly following style(9), there
> > should be a prototype somewhere, but I see that's not followed by most
> > functions...
>
> Right. I have given up on prototypes for static functions across most
> of the code base (due to lazyness). Fixing that is for another day :)
>
> In the new patch below I simply moved this function much further up.
>
> > Can we add the check for -f and -F at this time too?
>
> Sure.
>
> diff 9f6bb280654be7061fc00305743f6ace71f9a1cb /home/stsp/src/got
> blob - 7bdd9e142a5f26254c73c308684bb6d6da95ef20
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -183,6 +183,12 @@ list_commands(FILE *fp)
> fputc('\n', fp);
> }
>
> +__dead static void
> +option_conflict(char a, char b)
> +{
> + errx(1, "-%c and -%c options are mutually exclusive", a, b);
> +}
> +
> int
> main(int argc, char *argv[])
> {
> @@ -1389,18 +1395,18 @@ cmd_clone(int argc, char *argv[])
> argv += optind;
>
> if (fetch_all_branches && !TAILQ_EMPTY(&wanted_branches))
> - errx(1, "-a and -b options are mutually exclusive");
> + option_conflict('a', 'b');
> if (list_refs_only) {
> if (!TAILQ_EMPTY(&wanted_branches))
> - errx(1, "-l and -b options are mutually exclusive");
> + option_conflict('l', 'b');
> if (fetch_all_branches)
> - errx(1, "-l and -a options are mutually exclusive");
> + option_conflict('l', 'a');
> if (mirror_references)
> - errx(1, "-l and -m options are mutually exclusive");
> + option_conflict('l', 'm');
> if (verbosity == -1)
> - errx(1, "-l and -q options are mutually exclusive");
> + option_conflict('l', 'q');
> if (!TAILQ_EMPTY(&wanted_refs))
> - errx(1, "-l and -R options are mutually exclusive");
> + option_conflict('l', 'R');
> }
>
> uri = argv[0];
> @@ -2062,16 +2068,16 @@ cmd_fetch(int argc, char *argv[])
> argv += optind;
>
> if (fetch_all_branches && !TAILQ_EMPTY(&wanted_branches))
> - errx(1, "-a and -b options are mutually exclusive");
> + option_conflict('a', 'b');
> if (list_refs_only) {
> if (!TAILQ_EMPTY(&wanted_branches))
> - errx(1, "-l and -b options are mutually exclusive");
> + option_conflict('l', 'b');
> if (fetch_all_branches)
> - errx(1, "-l and -a options are mutually exclusive");
> + option_conflict('l', 'a');
> if (delete_refs)
> - errx(1, "-l and -d options are mutually exclusive");
> + option_conflict('l', 'd');
> if (verbosity == -1)
> - errx(1, "-l and -q options are mutually exclusive");
> + option_conflict('l', 'q');
> }
>
> if (argc == 0)
> @@ -5195,17 +5201,17 @@ cmd_ref(int argc, char *argv[])
> }
>
> if (obj_arg && do_list)
> - errx(1, "-c and -l options are mutually exclusive");
> + option_conflict('c', 'l');
> if (obj_arg && do_delete)
> - errx(1, "-c and -d options are mutually exclusive");
> + option_conflict('c', 'd');
> if (obj_arg && symref_target)
> - errx(1, "-c and -s options are mutually exclusive");
> + option_conflict('c', 's');
> if (symref_target && do_delete)
> - errx(1, "-s and -d options are mutually exclusive");
> + option_conflict('s', 'd');
> if (symref_target && do_list)
> - errx(1, "-s and -l options are mutually exclusive");
> + option_conflict('s', 'l');
> if (do_delete && do_list)
> - errx(1, "-d and -l options are mutually exclusive");
> + option_conflict('d', 'l');
>
> argc -= optind;
> argv += optind;
> @@ -5531,7 +5537,7 @@ cmd_branch(int argc, char *argv[])
> }
>
> if (do_list && delref)
> - errx(1, "-l and -d options are mutually exclusive");
> + option_conflict('l', 'd');
>
> argc -= optind;
> argv += optind;
> @@ -6067,7 +6073,7 @@ cmd_tag(int argc, char *argv[])
> errx(1,
> "-c option can only be used when creating a tag");
> if (tagmsg)
> - errx(1, "-l and -m options are mutually exclusive");
> + option_conflict('l', 'm');
> if (argc > 0)
> usage_tag();
> } else if (argc != 1)
> @@ -8624,19 +8630,21 @@ cmd_histedit(int argc, char *argv[])
> err(1, "pledge");
> #endif
> if (abort_edit && continue_edit)
> - errx(1, "histedit's -a and -c options are mutually exclusive");
> + option_conflict('a', 'c');
> if (edit_script_path && edit_logmsg_only)
> - errx(1, "histedit's -F and -m options are mutually exclusive");
> + option_conflict('F', 'm');
> if (abort_edit && edit_logmsg_only)
> - errx(1, "histedit's -a and -m options are mutually exclusive");
> + option_conflict('a', 'm');
> if (continue_edit && edit_logmsg_only)
> - errx(1, "histedit's -c and -m options are mutually exclusive");
> + option_conflict('c', 'm');
> if (abort_edit && fold_only)
> - errx(1, "histedit's -a and -f options are mutually exclusive");
> + option_conflict('a', 'f');
> if (continue_edit && fold_only)
> - errx(1, "histedit's -c and -f options are mutually exclusive");
> + option_conflict('c', 'f');
> if (fold_only && edit_logmsg_only)
> - errx(1, "histedit's -f and -m options are mutually exclusive");
> + option_conflict('f', 'm');
> + if (edit_script_path && fold_only)
> + option_conflict('F', 'f');
> if (argc != 0)
> usage_histedit();
>
ok jrick
Cleanup of mutually exclusive flag errors