diff mbox series

[3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs

Message ID 20231206115215.94467-4-l.s.r@web.de (mailing list archive)
State New, archived
Headers show
Series standardize incompatibility messages | expand

Commit Message

René Scharfe Dec. 6, 2023, 11:51 a.m. UTC 
The revision options --reverse is incompatibel with --walk-reflogs and
--graph is incompatible with both --reverse and --walk-reflogs.  So they
are all incompatible with each other.

Use the function for checking three mutually incompatible options,
die_for_incompatible_opt3(), to perform this check in one place and
without repetition.  This is shorter and clearer.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

--
2.43.0

Comments

Patrick Steinhardt Dec. 6, 2023, 1:08 p.m. UTC | #1 
On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:
> The revision options --reverse is incompatibel with --walk-reflogs and
> --graph is incompatible with both --reverse and --walk-reflogs.  So they
> are all incompatible with each other.
> 
> Use the function for checking three mutually incompatible options,
> die_for_incompatible_opt3(), to perform this check in one place and
> without repetition.  This is shorter and clearer.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  revision.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/revision.c b/revision.c
> index b2861474b1..1b7e1af6c6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		revs->grep_filter.ignore_locale = 1;
>  	compile_grep_patterns(&revs->grep_filter);
> 
> -	if (revs->reverse && revs->reflog_info)
> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
>  	if (revs->reflog_info && revs->limited)
>  		die("cannot combine --walk-reflogs with history-limiting options");
>  	if (revs->rewrite_parents && revs->children.name)
> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  	/*
>  	 * Limitations on the graph functionality
>  	 */
> -	if (revs->reverse && revs->graph)
> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
> +	die_for_incompatible_opt3(!!revs->graph, "--graph",
> +				  !!revs->reverse, "--reverse",
> +				  !!revs->reflog_info, "--walk-reflogs");

I've been wondering why we use `!!` here, as `die_for_incompatible_*()`
doesn't care for the actual value but only checks that it is non-zero.
Is it because of the type mismatch, where these flags here use unsigned
ints whereas `die_for_incompatible_*()` expect ints?

Patrick
René Scharfe Dec. 6, 2023, 1:47 p.m. UTC | #2 
Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
> On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:
>> The revision options --reverse is incompatibel with --walk-reflogs and
>> --graph is incompatible with both --reverse and --walk-reflogs.  So they
>> are all incompatible with each other.
>>
>> Use the function for checking three mutually incompatible options,
>> die_for_incompatible_opt3(), to perform this check in one place and
>> without repetition.  This is shorter and clearer.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  revision.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/revision.c b/revision.c
>> index b2861474b1..1b7e1af6c6 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>  		revs->grep_filter.ignore_locale = 1;
>>  	compile_grep_patterns(&revs->grep_filter);
>>
>> -	if (revs->reverse && revs->reflog_info)
>> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
>>  	if (revs->reflog_info && revs->limited)
>>  		die("cannot combine --walk-reflogs with history-limiting options");
>>  	if (revs->rewrite_parents && revs->children.name)
>> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>  	/*
>>  	 * Limitations on the graph functionality
>>  	 */
>> -	if (revs->reverse && revs->graph)
>> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
>> +	die_for_incompatible_opt3(!!revs->graph, "--graph",
>> +				  !!revs->reverse, "--reverse",
>> +				  !!revs->reflog_info, "--walk-reflogs");
>
> I've been wondering why we use `!!` here, as `die_for_incompatible_*()`
> doesn't care for the actual value but only checks that it is non-zero.
> Is it because of the type mismatch, where these flags here use unsigned
> ints whereas `die_for_incompatible_*()` expect ints?

->graph and ->reflog_info are pointers and clang reports an int-conversion
warning without the double negation.  ->reverse is an unsigned:1 and so
doesn't need it, but I gave it one anyway for aesthetic reasons.

René
Eric Sunshine Dec. 6, 2023, 5:21 p.m. UTC | #3 
On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:
> The revision options --reverse is incompatibel with --walk-reflogs and

s/incompatibel/incompatible/

> --graph is incompatible with both --reverse and --walk-reflogs.  So they
> are all incompatible with each other.
>
> Use the function for checking three mutually incompatible options,
> die_for_incompatible_opt3(), to perform this check in one place and
> without repetition.  This is shorter and clearer.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
René Scharfe Dec. 6, 2023, 5:29 p.m. UTC | #4 
Am 06.12.23 um 18:21 schrieb Eric Sunshine:
> On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:
>> The revision options --reverse is incompatibel with --walk-reflogs and
>
> s/incompatibel/incompatible/

And s/options/option/, *sigh*.

>
>> --graph is incompatible with both --reverse and --walk-reflogs.  So they
>> are all incompatible with each other.
>>
>> Use the function for checking three mutually incompatible options,
>> die_for_incompatible_opt3(), to perform this check in one place and
>> without repetition.  This is shorter and clearer.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
diff mbox series

Patch

diff --git a/revision.c b/revision.c
index b2861474b1..1b7e1af6c6 100644
--- a/revision.c
+++ b/revision.c
@@ -3036,8 +3036,6 @@  int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revs->grep_filter.ignore_locale = 1;
 	compile_grep_patterns(&revs->grep_filter);

-	if (revs->reverse && revs->reflog_info)
-		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
 	if (revs->reflog_info && revs->limited)
 		die("cannot combine --walk-reflogs with history-limiting options");
 	if (revs->rewrite_parents && revs->children.name)
@@ -3048,11 +3046,10 @@  int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	/*
 	 * Limitations on the graph functionality
 	 */
-	if (revs->reverse && revs->graph)
-		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
+	die_for_incompatible_opt3(!!revs->graph, "--graph",
+				  !!revs->reverse, "--reverse",
+				  !!revs->reflog_info, "--walk-reflogs");

-	if (revs->reflog_info && revs->graph)
-		die(_("options '%s' and '%s' cannot be used together"), "--walk-reflogs", "--graph");
 	if (revs->no_walk && revs->graph)
 		die(_("options '%s' and '%s' cannot be used together"), "--no-walk", "--graph");
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)