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.  Also if we're strictly following style(9), there
should be a prototype somewhere, but I see that's not followed by most
functions...

> +option_conflict(char a, char b)
> +{
> +	errx(1, "-%c and -%c options are mutually exclusive", a, b);
> +}
> +
>  static const struct got_error *
>  cmd_clone(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,19 @@ 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');

Can we add the check for -f and -F at this time too?

>  	if (argc != 0)
>  		usage_histedit();
>  
>