Download raw body.
Cleanup of mutually exclusive flag errors
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();
Cleanup of mutually exclusive flag errors