Re: [PATCH 5/5] diff: properly error out when combining multiple pickaxe options

[Stefan Beller]

On Wed, Jan 3, 2018 at 2:08 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
> ;
>> + count = 0;
>> +
>> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
>> + count++;
>> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
>> + count++;
>> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
>> + count++;
>> + if (count > 1)
>> + die(_("-G, -S, --find-object are mutually exclusive"));
>
> I thought the reason you defined pickaxe-kind bitmask was so that
> you can mask this field to grab these (and only these) bits.

Originally I only wanted to mask out the 'case independency'
bit and keep it future proof for any similar bits.

> Once you have that mask, you should be able to use HAS_MULTI_BITS()
> on the masked result without counting, no?

Oh, what a nice macro! Thanks for pointing at it.

As soon as I figured out the right place where to put this check,
I saw the lines above, whose style I imitated.
(I guess there is just no mask defined for "--name-only, --name-status,
--check and -s", nor would it make sense to do so; though that given
macro should work just fine even for non-continuous masks)

Re: [PATCH 5/5] diff: properly error out when combining multiple pickaxe options

[Junio C Hamano]

Stefan Beller  writes:

;
> + count = 0;
> +
> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
> + count++;
> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
> + count++;
> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
> + count++;
> + if (count > 1)
> + die(_("-G, -S, --find-object are mutually exclusive"));

I thought the reason you defined pickaxe-kind bitmask was so that
you can mask this field to grab these (and only these) bits.
Once you have that mask, you should be able to use HAS_MULTI_BITS()
on the masked result without counting, no?

Re: [PATCH 5/5] diff: properly error out when combining multiple pickaxe options

[Jacob Keller]

On Tue, Jan 2, 2018 at 4:46 PM, Stefan Beller  wrote:
> die(_("--name-only, --name-status, --check and -s are 
> mutually exclusive"));
> +   count = 0;
> +
> +   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
> +   count++;
> +   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
> +   count++;
> +   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
> +   count++;
> +   if (count > 1)
> +   die(_("-G, -S, --find-object are mutually exclusive"));

Nit: I'd put an "and" in the sentence.

[PATCH 5/5] diff: properly error out when combining multiple pickaxe options

[Stefan Beller]

In f506b8e8b5 (git log/diff: add -G that greps in the patch text,
2010-08-23) we were hesitant to check if the user requests both -S and
-G at the same time. Now that the pickaxe family also offers --find-object,
which looks slightly more different than the former two, let's add a check
that those are not used at the same time.

Signed-off-by: Stefan Beller 
---
 diff.c | 10 ++
 diffcore-pickaxe.c |  1 -
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index a872bdcac1..29973e78d6 100644
--- a/diff.c
+++ b/diff.c
@@ -4122,6 +4122,16 @@ void diff_setup_done(struct diff_options *options)
count++;
if (count > 1)
die(_("--name-only, --name-status, --check and -s are mutually 
exclusive"));
+   count = 0;
+
+   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
+   count++;
+   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
+   count++;
+   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
+   count++;
+   if (count > 1)
+   die(_("-G, -S, --find-object are mutually exclusive"));
 
/*
 * Most of the time we can say "there are changes"
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 72bb5a9514..239ce5122b 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -251,7 +251,6 @@ void diffcore_pickaxe(struct diff_options *o)
}
}
 
-   /* Might want to warn when both S and G are on; I don't care... */
pickaxe(_queued_diff, o, regexp, kws,
(opts & DIFF_PICKAXE_KIND_G) ? diff_grep : has_changes);
 
-- 
2.15.1.620.gb9897f4670-goog