Re: [PATCH 01/10] get_sha1: detect buggy calls with multiple disambiguators

[Junio C Hamano]

Jeff King  writes:

>> Other than your reinvention of HAS_MULTI_BITS(), which has been with
>> us since db7244bd ("parse-options new features.", 2007-11-07), this
>> looks like a reasonable thing to do.
>
> Heh, I _thought_ we had something like that but couldn't find it. I
> grepped for "[^&]& .*-", which does match it, but stupidly did it only
> in '*.c'. Definitely it should use the existing macro instead.

OK, I'll queue this on top to be squashed, so no need to resend only
for this one.

 sha1_name.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index f9812ff..0ff83a9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -310,11 +310,6 @@ static int prepare_prefixes(const char *name, int len,
return 0;
 }
 
-static int multiple_bits_set(unsigned flags)
-{
-   return !!(flags & (flags - 1));
-}
-
 static int get_short_sha1(const char *name, int len, unsigned char *sha1,
  unsigned flags)
 {
@@ -333,7 +328,7 @@ static int get_short_sha1(const char *name, int len, 
unsigned char *sha1,
 
memset(, 0, sizeof(ds));
 
-   if (multiple_bits_set(flags & GET_SHA1_DISAMBIGUATORS))
+   if (HAS_MULTI_BITS(flags & GET_SHA1_DISAMBIGUATORS))
die("BUG: multiple get_short_sha1 disambiguator flags");
 
if (flags & GET_SHA1_COMMIT)

Re: [PATCH 01/10] get_sha1: detect buggy calls with multiple disambiguators

[Jeff King]

On Mon, Sep 26, 2016 at 09:37:10AM -0700, Junio C Hamano wrote:

> > We can do the check easily with some bit-twiddling, and as a
> > bonus, the bit-mask of disambiguators will come in handy in
> > a future patch.
> >
> > Signed-off-by: Jeff King 
> > ---
> 
> Other than your reinvention of HAS_MULTI_BITS(), which has been with
> us since db7244bd ("parse-options new features.", 2007-11-07), this
> looks like a reasonable thing to do.

Heh, I _thought_ we had something like that but couldn't find it. I
grepped for "[^&]& .*-", which does match it, but stupidly did it only
in '*.c'. Definitely it should use the existing macro instead.

-Peff

Re: [PATCH 01/10] get_sha1: detect buggy calls with multiple disambiguators

[Junio C Hamano]

Jeff King  writes:

> The get_sha1() family of functions takes a flags field, but
> some of the flags are mutually exclusive. In particular, we
> can only handle one disambiguating function, and the flags
> quietly override each other. Let's instead detect these as
> programming bugs.
>
> Technically some of the flags are supersets of the others,
> so treating COMMITTISH|TREEISH as just COMMITTISH is not
> wrong, but it's a good sign the caller is confused. And
> certainly asking for BLOB|TREE does not work.
>
> We can do the check easily with some bit-twiddling, and as a
> bonus, the bit-mask of disambiguators will come in handy in
> a future patch.
>
> Signed-off-by: Jeff King 
> ---

Other than your reinvention of HAS_MULTI_BITS(), which has been with
us since db7244bd ("parse-options new features.", 2007-11-07), this
looks like a reasonable thing to do.

;-)

>  cache.h | 5 +
>  sha1_name.c | 9 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index d0494c8..7bd78ca 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1203,6 +1203,11 @@ struct object_context {
>  #define GET_SHA1_FOLLOW_SYMLINKS 0100
>  #define GET_SHA1_ONLY_TO_DIE04000
>  
> +#define GET_SHA1_DISAMBIGUATORS \
> + (GET_SHA1_COMMIT | GET_SHA1_COMMITTISH | \
> + GET_SHA1_TREE | GET_SHA1_TREEISH | \
> + GET_SHA1_BLOB)
> +
>  extern int get_sha1(const char *str, unsigned char *sha1);
>  extern int get_sha1_commit(const char *str, unsigned char *sha1);
>  extern int get_sha1_committish(const char *str, unsigned char *sha1);
> diff --git a/sha1_name.c b/sha1_name.c
> index faf873c..f9812ff 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -310,6 +310,11 @@ static int prepare_prefixes(const char *name, int len,
>   return 0;
>  }
>  
> +static int multiple_bits_set(unsigned flags)
> +{
> + return !!(flags & (flags - 1));
> +}
> +
>  static int get_short_sha1(const char *name, int len, unsigned char *sha1,
> unsigned flags)
>  {
> @@ -327,6 +332,10 @@ static int get_short_sha1(const char *name, int len, 
> unsigned char *sha1,
>   prepare_alt_odb();
>  
>   memset(, 0, sizeof(ds));
> +
> + if (multiple_bits_set(flags & GET_SHA1_DISAMBIGUATORS))
> + die("BUG: multiple get_short_sha1 disambiguator flags");
> +
>   if (flags & GET_SHA1_COMMIT)
>   ds.fn = disambiguate_commit_only;
>   else if (flags & GET_SHA1_COMMITTISH)