[2/2] hex: drop sha1_to_hex()
diff mbox series

Message ID 20191111090418.GB12545@sigill.intra.peff.net
State New
Headers show
Series
  • getting rid of sha1_to_hex()
Related show

Commit Message

Jeff King Nov. 11, 2019, 9:04 a.m. UTC
There's only a single caller left of sha1_to_hex(), since everybody now
uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
print a hex sha1 when we find a collision. This one will always be sha1,
regardless of the current hash algorithm, so we can't use oid_to_hex()
here. In practice we'd probably not be running sha1 at all if it isn't
the current algorithm, but it's possible we might still occasionally
need to compute a sha1 in a post-sha256 world.

Since sha1_to_hex() is just a wrapper for hash_to_hex_algop(), let's
call that ourselves. There's value in getting rid of the sha1-specific
wrapper to de-clutter the global namespace, and to make sure nobody uses
it (and as with sha1_to_hex_r() in the previous patch, we'll drop the
coccinelle transformations, too).

The sha1_to_hex() function is mentioned in a comment; we can easily swap
that out for oid_to_hex() to give a better example. It's also mentioned
in some test vectors in t4100, but that's not runnable code, so there's
no point in trying to clean it up.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h                            |  3 +--
 contrib/coccinelle/object_id.cocci | 15 ---------------
 hex.c                              |  5 -----
 sha1dc_git.c                       |  2 +-
 4 files changed, 2 insertions(+), 23 deletions(-)

Comments

SZEDER Gábor Nov. 11, 2019, 2:18 p.m. UTC | #1
On Mon, Nov 11, 2019 at 04:04:18AM -0500, Jeff King wrote:
> There's only a single caller left of sha1_to_hex(), since everybody now
> uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
> print a hex sha1 when we find a collision. This one will always be sha1,
> regardless of the current hash algorithm, so we can't use oid_to_hex()

Nit: s/oid_to_hex/hash_to_hex/

We can't use oid_to_hex() because we don't have a 'struct object_id'
in the first place, as sha1dc only ever deals with 20 unsigned chars.

> here. In practice we'd probably not be running sha1 at all if it isn't
> the current algorithm, but it's possible we might still occasionally
> need to compute a sha1 in a post-sha256 world.
> 
> Since sha1_to_hex() is just a wrapper for hash_to_hex_algop(), let's
> call that ourselves. There's value in getting rid of the sha1-specific
> wrapper to de-clutter the global namespace, and to make sure nobody uses
> it (and as with sha1_to_hex_r() in the previous patch, we'll drop the
> coccinelle transformations, too).


> diff --git a/sha1dc_git.c b/sha1dc_git.c
> index e0cc9d988c..5c300e812e 100644
> --- a/sha1dc_git.c
> +++ b/sha1dc_git.c
> @@ -19,7 +19,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
>  	if (!SHA1DCFinal(hash, ctx))
>  		return;
>  	die("SHA-1 appears to be part of a collision attack: %s",
> -	    sha1_to_hex(hash));
> +	    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
>  }
>  
>  /*
> -- 
> 2.24.0.739.gb5632e4929
Jeff King Nov. 11, 2019, 2:29 p.m. UTC | #2
On Mon, Nov 11, 2019 at 03:18:05PM +0100, SZEDER Gábor wrote:

> On Mon, Nov 11, 2019 at 04:04:18AM -0500, Jeff King wrote:
> > There's only a single caller left of sha1_to_hex(), since everybody now
> > uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
> > print a hex sha1 when we find a collision. This one will always be sha1,
> > regardless of the current hash algorithm, so we can't use oid_to_hex()
> 
> Nit: s/oid_to_hex/hash_to_hex/
> 
> We can't use oid_to_hex() because we don't have a 'struct object_id'
> in the first place, as sha1dc only ever deals with 20 unsigned chars.

Ah, you're right. I admit I am still getting up to speed on all of the
new hash-agnostic versions of the various functions.

-Peff
Junio C Hamano Nov. 12, 2019, 4:13 a.m. UTC | #3
Jeff King <peff@peff.net> writes:

> On Mon, Nov 11, 2019 at 03:18:05PM +0100, SZEDER Gábor wrote:
>
>> On Mon, Nov 11, 2019 at 04:04:18AM -0500, Jeff King wrote:
>> > There's only a single caller left of sha1_to_hex(), since everybody now
>> > uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
>> > print a hex sha1 when we find a collision. This one will always be sha1,
>> > regardless of the current hash algorithm, so we can't use oid_to_hex()
>> 
>> Nit: s/oid_to_hex/hash_to_hex/
>> 
>> We can't use oid_to_hex() because we don't have a 'struct object_id'
>> in the first place, as sha1dc only ever deals with 20 unsigned chars.
>
> Ah, you're right. I admit I am still getting up to speed on all of the
> new hash-agnostic versions of the various functions.

Thanks.  I've amended this one and the range diff since the pushout
yesterday looks like this.

1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()
    @@ Commit message
         hex: drop sha1_to_hex()
     
         There's only a single caller left of sha1_to_hex(), since everybody now
    -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
    +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we
         print a hex sha1 when we find a collision. This one will always be sha1,
    -    regardless of the current hash algorithm, so we can't use oid_to_hex()
    +    regardless of the current hash algorithm, so we can't use hash_to_hex()
         here. In practice we'd probably not be running sha1 at all if it isn't
         the current algorithm, but it's possible we might still occasionally
         need to compute a sha1 in a post-sha256 world.
    @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
       * buffers, making it safe to make multiple calls for a single statement, like:
       *
     - *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
    -+ *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
    ++ *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
       */
      char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
      char *oid_to_hex_r(char *out, const struct object_id *oid);
Jeff King Nov. 12, 2019, 10:57 a.m. UTC | #4
On Tue, Nov 12, 2019 at 01:13:58PM +0900, Junio C Hamano wrote:

> >> We can't use oid_to_hex() because we don't have a 'struct object_id'
> >> in the first place, as sha1dc only ever deals with 20 unsigned chars.
> >
> > Ah, you're right. I admit I am still getting up to speed on all of the
> > new hash-agnostic versions of the various functions.
> 
> Thanks.  I've amended this one and the range diff since the pushout
> yesterday looks like this.

Thanks. This first hunk is what I would have done:

> 1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()
>     @@ Commit message
>          hex: drop sha1_to_hex()
>      
>          There's only a single caller left of sha1_to_hex(), since everybody now
>     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
>     +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we
>          print a hex sha1 when we find a collision. This one will always be sha1,
>     -    regardless of the current hash algorithm, so we can't use oid_to_hex()
>     +    regardless of the current hash algorithm, so we can't use hash_to_hex()
>          here. In practice we'd probably not be running sha1 at all if it isn't
>          the current algorithm, but it's possible we might still occasionally
>          need to compute a sha1 in a post-sha256 world.

This second one is OK, but not entirely necessary:

>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
>        * buffers, making it safe to make multiple calls for a single statement, like:
>        *
>      - *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
>     -+ *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
>     ++ *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
>        */
>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
>       char *oid_to_hex_r(char *out, const struct object_id *oid);

This one-liner leaves the types of "one" and "two" unspecified. :) So
it's not wrong to use hash_to_hex(), but maybe it's better to be pushing
people towards oid_to_hex() as their first choice? It probably doesn't
matter too much either way.

-Peff
SZEDER Gábor Nov. 12, 2019, 11:44 a.m. UTC | #5
On Tue, Nov 12, 2019 at 05:57:59AM -0500, Jeff King wrote:
> On Tue, Nov 12, 2019 at 01:13:58PM +0900, Junio C Hamano wrote:
> 
> > >> We can't use oid_to_hex() because we don't have a 'struct object_id'
> > >> in the first place, as sha1dc only ever deals with 20 unsigned chars.
> > >
> > > Ah, you're right. I admit I am still getting up to speed on all of the
> > > new hash-agnostic versions of the various functions.
> > 
> > Thanks.  I've amended this one and the range diff since the pushout
> > yesterday looks like this.
> 
> Thanks. This first hunk is what I would have done:
> 
> > 1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()
> >     @@ Commit message
> >          hex: drop sha1_to_hex()
> >      
> >          There's only a single caller left of sha1_to_hex(), since everybody now
> >     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
> >     +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we
> >          print a hex sha1 when we find a collision. This one will always be sha1,
> >     -    regardless of the current hash algorithm, so we can't use oid_to_hex()
> >     +    regardless of the current hash algorithm, so we can't use hash_to_hex()
> >          here. In practice we'd probably not be running sha1 at all if it isn't
> >          the current algorithm, but it's possible we might still occasionally
> >          need to compute a sha1 in a post-sha256 world.
> 
> This second one is OK, but not entirely necessary:
> 
> >     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
> >        * buffers, making it safe to make multiple calls for a single statement, like:
> >        *
> >      - *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
> >     -+ *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
> >     ++ *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
> >        */
> >       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
> >       char *oid_to_hex_r(char *out, const struct object_id *oid);
> 
> This one-liner leaves the types of "one" and "two" unspecified. :) So
> it's not wrong to use hash_to_hex(), but maybe it's better to be pushing
> people towards oid_to_hex() as their first choice? It probably doesn't
> matter too much either way.

Yeah, most (over 96%) of the hashes that we print are actually object
ids, so oid_to_hex() is the right function to use most of the time.

And because of this the updated "since everybody uses hash_to_hex()"
in the first hunk sounds a bit wrong, because barely anybody actually
uses hash_to_hex().
Junio C Hamano Nov. 12, 2019, 11:49 a.m. UTC | #6
Jeff King <peff@peff.net> writes:

>>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
>>        * buffers, making it safe to make multiple calls for a single statement, like:
>>        *
>>      - *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
>>     -+ *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
>>     ++ *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
>>        */
>>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
>>       char *oid_to_hex_r(char *out, const struct object_id *oid);
>
> This one-liner leaves the types of "one" and "two" unspecified. :) So
> it's not wrong to use hash_to_hex(), but maybe it's better to be pushing
> people towards oid_to_hex() as their first choice? It probably doesn't
> matter too much either way.

The pre-context of that comment reads:

 * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,
 * and writes the NUL-terminated output to the buffer `out`, which must be at
 * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
 * convenience.

so I think the intent of the example that used to use sha1_to_hex()
has been the raw bytestring that is the object name, not its form
that is encapsulated in the "struct object_id()".
Jeff King Nov. 12, 2019, 12:12 p.m. UTC | #7
On Tue, Nov 12, 2019 at 12:44:58PM +0100, SZEDER Gábor wrote:

> > > 1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()
> > >     @@ Commit message
> > >          hex: drop sha1_to_hex()
> > >      
> > >          There's only a single caller left of sha1_to_hex(), since everybody now
> > >     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
> > >     +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we
> > >          print a hex sha1 when we find a collision. This one will always be sha1,
> > >     -    regardless of the current hash algorithm, so we can't use oid_to_hex()
> > >     +    regardless of the current hash algorithm, so we can't use hash_to_hex()
> > >          here. In practice we'd probably not be running sha1 at all if it isn't
> > >          the current algorithm, but it's possible we might still occasionally
> > >          need to compute a sha1 in a post-sha256 world.
> [...]
> And because of this the updated "since everybody uses hash_to_hex()"
> in the first hunk sounds a bit wrong, because barely anybody actually
> uses hash_to_hex().

You're right, I should have read more carefully.

-Peff
Jeff King Nov. 12, 2019, 12:15 p.m. UTC | #8
On Tue, Nov 12, 2019 at 08:49:44PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
> >>        * buffers, making it safe to make multiple calls for a single statement, like:
> >>        *
> >>      - *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
> >>     -+ *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
> >>     ++ *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
> >>        */
> >>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
> >>       char *oid_to_hex_r(char *out, const struct object_id *oid);
> >
> > This one-liner leaves the types of "one" and "two" unspecified. :) So
> > it's not wrong to use hash_to_hex(), but maybe it's better to be pushing
> > people towards oid_to_hex() as their first choice? It probably doesn't
> > matter too much either way.
> 
> The pre-context of that comment reads:
> 
>  * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,
>  * and writes the NUL-terminated output to the buffer `out`, which must be at
>  * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
>  * convenience.
> 
> so I think the intent of the example that used to use sha1_to_hex()
> has been the raw bytestring that is the object name, not its form
> that is encapsulated in the "struct object_id()".

I guess you're keying on the phrase "binary hash" there (the
"GIT_MAX_HEXZ" bits only apply to the "_r" variants anyway). I'd read it
as encompassing all of the functions below, including oid_to_hex(). But
I'm OK with it either way.

-Peff
Junio C Hamano Nov. 13, 2019, 1:09 a.m. UTC | #9
Jeff King <peff@peff.net> writes:

> On Tue, Nov 12, 2019 at 08:49:44PM +0900, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
>> >>        * buffers, making it safe to make multiple calls for a single statement, like:
>> >>        *
>> >>      - *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
>> >>     -+ *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
>> >>     ++ *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
>> >>        */
>> >>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
>> >>       char *oid_to_hex_r(char *out, const struct object_id *oid);
>> >
>> > This one-liner leaves the types of "one" and "two" unspecified. :) So
>> > it's not wrong to use hash_to_hex(), but maybe it's better to be pushing
>> > people towards oid_to_hex() as their first choice? It probably doesn't
>> > matter too much either way.
>> 
>> The pre-context of that comment reads:
>> 
>>  * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,
>>  * and writes the NUL-terminated output to the buffer `out`, which must be at
>>  * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
>>  * convenience.
>> 
>> so I think the intent of the example that used to use sha1_to_hex()
>> has been the raw bytestring that is the object name, not its form
>> that is encapsulated in the "struct object_id()".
>
> I guess you're keying on the phrase "binary hash" there (the
> "GIT_MAX_HEXZ" bits only apply to the "_r" variants anyway). I'd read it
> as encompassing all of the functions below, including oid_to_hex(). But
> I'm OK with it either way.

Yes.  "binary hash" is about "unsigned char[]".  I think that's
historical accident---we added "struct object_id *" variants without
updating the comment.

Here is another try.  The "everybody uses oid_to_hex" in the log
message has also been updated.

1:  8a030f1796 ! 1:  b19f3fe9dd hex: drop sha1_to_hex()
    @@ Metadata
      ## Commit message ##
         hex: drop sha1_to_hex()
     
    -    There's only a single caller left of sha1_to_hex(), since everybody now
    -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
    -    print a hex sha1 when we find a collision. This one will always be sha1,
    -    regardless of the current hash algorithm, so we can't use oid_to_hex()
    -    here. In practice we'd probably not be running sha1 at all if it isn't
    -    the current algorithm, but it's possible we might still occasionally
    +    There's only a single caller left of sha1_to_hex(), since everybody
    +    that has an object name in "unsigned char[]" now uses hash_to_hex()
    +    instead.
    +
    +    This case is in the sha1dc wrapper, where we print a hex sha1 when
    +    we find a collision. This one will always be sha1, regardless of the
    +    current hash algorithm, so we can't use hash_to_hex() here. In
    +    practice we'd probably not be running sha1 at all if it isn't the
    +    current algorithm, but it's possible we might still occasionally
         need to compute a sha1 in a post-sha256 world.
     
         Since sha1_to_hex() is just a wrapper for hash_to_hex_algop(), let's
    @@ Commit message
         it (and as with sha1_to_hex_r() in the previous patch, we'll drop the
         coccinelle transformations, too).
     
    -    The sha1_to_hex() function is mentioned in a comment; we can easily swap
    -    that out for oid_to_hex() to give a better example. It's also mentioned
    -    in some test vectors in t4100, but that's not runnable code, so there's
    -    no point in trying to clean it up.
    +    The sha1_to_hex() function is mentioned in a comment; we can easily
    +    swap that out for oid_to_hex() to give a better example.  Also
    +    update the comment that was left stale when we added "struct
    +    object_id *" as a way to name an object and added functions to
    +    convert it to hex.
    +
    +    The function is also mentioned in some test vectors in t4100, but
    +    that's not runnable code, so there's no point in trying to clean it
    +    up.
     
         Signed-off-by: Jeff King <peff@peff.net>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## cache.h ##
    +@@ cache.h: int get_oid_hex(const char *hex, struct object_id *sha1);
    + int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
    + 
    + /*
    +- * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,
    ++ * Convert a binary hash in "unsigned char []" or an object name in
    ++ * "struct object_id *" to its hex equivalent. The `_r` variant is reentrant,
    +  * and writes the NUL-terminated output to the buffer `out`, which must be at
    +  * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
    +  * convenience.
     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
       * The non-`_r` variant returns a static buffer, but uses a ring of 4
       * buffers, making it safe to make multiple calls for a single statement, like:
       *
     - *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
    ++ *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
     + *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
       */
      char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
Jeff King Nov. 13, 2019, 1:15 a.m. UTC | #10
On Wed, Nov 13, 2019 at 10:09:23AM +0900, Junio C Hamano wrote:

> Yes.  "binary hash" is about "unsigned char[]".  I think that's
> historical accident---we added "struct object_id *" variants without
> updating the comment.
> 
> Here is another try.  The "everybody uses oid_to_hex" in the log
> message has also been updated.

This looks good to me (and thank you for cleaning up my messy commit
message).

> 1:  8a030f1796 ! 1:  b19f3fe9dd hex: drop sha1_to_hex()
>     @@ Metadata
>       ## Commit message ##
>          hex: drop sha1_to_hex()
>      
>     -    There's only a single caller left of sha1_to_hex(), since everybody now
>     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we
>     -    print a hex sha1 when we find a collision. This one will always be sha1,
>     -    regardless of the current hash algorithm, so we can't use oid_to_hex()
>     -    here. In practice we'd probably not be running sha1 at all if it isn't
>     -    the current algorithm, but it's possible we might still occasionally
>     +    There's only a single caller left of sha1_to_hex(), since everybody
>     +    that has an object name in "unsigned char[]" now uses hash_to_hex()
>     +    instead.

That makes sense. I mentioned oid_to_hex() originally because most of
the callers _did_ switch from sha1_to_hex(oid->hash) to oid_to_hex(oid).
But that happened far enough in the past that the more interesting
change is using the generic hash_to_hex().

-Peff

Patch
diff mbox series

diff --git a/cache.h b/cache.h
index 6a4eb221b3..a2ab10503f 100644
--- a/cache.h
+++ b/cache.h
@@ -1459,12 +1459,11 @@  int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
  * The non-`_r` variant returns a static buffer, but uses a ring of 4
  * buffers, making it safe to make multiple calls for a single statement, like:
  *
- *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
+ *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
  */
 char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
 char *oid_to_hex_r(char *out, const struct object_id *oid);
 char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *);	/* static buffer result! */
-char *sha1_to_hex(const unsigned char *sha1);						/* same static buffer */
 char *hash_to_hex(const unsigned char *hash);						/* same static buffer */
 char *oid_to_hex(const struct object_id *oid);						/* same static buffer */
 
diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci
index 6c0d21d8e2..ddf4f22bd7 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -10,21 +10,6 @@  struct object_id *OIDPTR;
 - is_null_sha1(OIDPTR->hash)
 + is_null_oid(OIDPTR)
 
-@@
-struct object_id OID;
-@@
-- sha1_to_hex(OID.hash)
-+ oid_to_hex(&OID)
-
-@@
-identifier f != oid_to_hex;
-struct object_id *OIDPTR;
-@@
-  f(...) {<...
-- sha1_to_hex(OIDPTR->hash)
-+ oid_to_hex(OIDPTR)
-  ...>}
-
 @@
 struct object_id OID;
 @@
diff --git a/hex.c b/hex.c
index 8c3f06a192..fd7f00c43f 100644
--- a/hex.c
+++ b/hex.c
@@ -103,11 +103,6 @@  char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *a
 	return hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);
 }
 
-char *sha1_to_hex(const unsigned char *sha1)
-{
-	return hash_to_hex_algop(sha1, &hash_algos[GIT_HASH_SHA1]);
-}
-
 char *hash_to_hex(const unsigned char *hash)
 {
 	return hash_to_hex_algop(hash, the_hash_algo);
diff --git a/sha1dc_git.c b/sha1dc_git.c
index e0cc9d988c..5c300e812e 100644
--- a/sha1dc_git.c
+++ b/sha1dc_git.c
@@ -19,7 +19,7 @@  void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
 	if (!SHA1DCFinal(hash, ctx))
 		return;
 	die("SHA-1 appears to be part of a collision attack: %s",
-	    sha1_to_hex(hash));
+	    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
 }
 
 /*