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)

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

[Jeff King]

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 
---
 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)
-- 
2.10.0.492.g14f803f