{"thread":{"id":"52234","subject":"[PATCH 0/2] getting rid of sha1_to_hex()","startedAt":"2019-11-11T09:03:35Z","lastAt":"2019-11-13T01:15:09Z","messageCount":17,"participants":["Jeff King","Junio C Hamano","SZEDER Gábor","Johannes Schindelin","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"385895","messageId":"20191111090332.GA2275@sigill.intra.peff.net","threadId":"52234","inReplyTo":null,"subject":"[PATCH 0/2] getting rid of sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-11T09:03:32Z","receivedAt":"2019-11-11T09:03:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I happened to be looking at the oid_to_hex() family of functions today,\nand noticed that we can drop some of the sha1-specific ones.\n\nIt's a minor cleanup in terms of text, but it feels like a nice\nmilestone. :)\n\n  [1/2]: hex: drop sha1_to_hex_r()\n  [2/2]: hex: drop sha1_to_hex()\n\n cache.h                            |  4 +---\n contrib/coccinelle/object_id.cocci | 32 ------------------------------\n hex.c                              | 10 ----------\n sha1dc_git.c                       |  2 +-\n 4 files changed, 2 insertions(+), 46 deletions(-)\n\n-Peff\n"},{"id":"385896","messageId":"20191111090411.GA12545@sigill.intra.peff.net","threadId":"52234","inReplyTo":"20191111090332.GA2275@sigill.intra.peff.net","subject":"[PATCH 1/2] hex: drop sha1_to_hex_r()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-11T09:04:11Z","receivedAt":"2019-11-11T09:04:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are no callers left; everybody uses oid_to_hex_r() or\nhash_to_hex_algop_r(). This used to actually be the underlying\nimplementation for oid_to_hex_r(), but that's no longer the case since\n47edb64997 (hex: introduce functions to print arbitrary hashes,\n2018-11-14).\n\nLet's get rid of it to de-clutter and to make sure nobody uses it.\nLikewise we can drop the coccinelle rules that mention it, since the\ncompiler will make it quite clear that the code does not work.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h                            |  1 -\n contrib/coccinelle/object_id.cocci | 17 -----------------\n hex.c                              |  5 -----\n 3 files changed, 23 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 04cabaac11..6a4eb221b3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1462,7 +1462,6 @@ int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n  *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n  */\n char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n-char *sha1_to_hex_r(char *out, const unsigned char *sha1);\n char *oid_to_hex_r(char *out, const struct object_id *oid);\n char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *);\t/* static buffer result! */\n char *sha1_to_hex(const unsigned char *sha1);\t\t\t\t\t\t/* same static buffer */\ndiff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci\nindex 3e536a9834..6c0d21d8e2 100644\n--- a/contrib/coccinelle/object_id.cocci\n+++ b/contrib/coccinelle/object_id.cocci\n@@ -25,23 +25,6 @@ struct object_id *OIDPTR;\n + oid_to_hex(OIDPTR)\n   ...>}\n \n-@@\n-expression E;\n-struct object_id OID;\n-@@\n-- sha1_to_hex_r(E, OID.hash)\n-+ oid_to_hex_r(E, &OID)\n-\n-@@\n-identifier f != oid_to_hex_r;\n-expression E;\n-struct object_id *OIDPTR;\n-@@\n-   f(...) {<...\n-- sha1_to_hex_r(E, OIDPTR->hash)\n-+ oid_to_hex_r(E, OIDPTR)\n-  ...>}\n-\n @@\n struct object_id OID;\n @@\ndiff --git a/hex.c b/hex.c\nindex 7850a8879d..8c3f06a192 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -90,11 +90,6 @@ char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash,\n \treturn buffer;\n }\n \n-char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)\n-{\n-\treturn hash_to_hex_algop_r(buffer, sha1, &hash_algos[GIT_HASH_SHA1]);\n-}\n-\n char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n {\n \treturn hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);\n-- \n2.24.0.739.gb5632e4929\n\n"},{"id":"385897","messageId":"20191111090418.GB12545@sigill.intra.peff.net","threadId":"52234","inReplyTo":"20191111090332.GA2275@sigill.intra.peff.net","subject":"[PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-11T09:04:18Z","receivedAt":"2019-11-11T09:04:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's only a single caller left of sha1_to_hex(), since everybody now\nuses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\nprint a hex sha1 when we find a collision. This one will always be sha1,\nregardless of the current hash algorithm, so we can't use oid_to_hex()\nhere. In practice we'd probably not be running sha1 at all if it isn't\nthe current algorithm, but it's possible we might still occasionally\nneed to compute a sha1 in a post-sha256 world.\n\nSince sha1_to_hex() is just a wrapper for hash_to_hex_algop(), let's\ncall that ourselves. There's value in getting rid of the sha1-specific\nwrapper to de-clutter the global namespace, and to make sure nobody uses\nit (and as with sha1_to_hex_r() in the previous patch, we'll drop the\ncoccinelle transformations, too).\n\nThe sha1_to_hex() function is mentioned in a comment; we can easily swap\nthat out for oid_to_hex() to give a better example. It's also mentioned\nin some test vectors in t4100, but that's not runnable code, so there's\nno point in trying to clean it up.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h                            |  3 +--\n contrib/coccinelle/object_id.cocci | 15 ---------------\n hex.c                              |  5 -----\n sha1dc_git.c                       |  2 +-\n 4 files changed, 2 insertions(+), 23 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6a4eb221b3..a2ab10503f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1459,12 +1459,11 @@ int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n  * The non-`_r` variant returns a static buffer, but uses a ring of 4\n  * buffers, making it safe to make multiple calls for a single statement, like:\n  *\n- *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n+ *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n  */\n char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n char *oid_to_hex_r(char *out, const struct object_id *oid);\n char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *);\t/* static buffer result! */\n-char *sha1_to_hex(const unsigned char *sha1);\t\t\t\t\t\t/* same static buffer */\n char *hash_to_hex(const unsigned char *hash);\t\t\t\t\t\t/* same static buffer */\n char *oid_to_hex(const struct object_id *oid);\t\t\t\t\t\t/* same static buffer */\n \ndiff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci\nindex 6c0d21d8e2..ddf4f22bd7 100644\n--- a/contrib/coccinelle/object_id.cocci\n+++ b/contrib/coccinelle/object_id.cocci\n@@ -10,21 +10,6 @@ struct object_id *OIDPTR;\n - is_null_sha1(OIDPTR->hash)\n + is_null_oid(OIDPTR)\n \n-@@\n-struct object_id OID;\n-@@\n-- sha1_to_hex(OID.hash)\n-+ oid_to_hex(&OID)\n-\n-@@\n-identifier f != oid_to_hex;\n-struct object_id *OIDPTR;\n-@@\n-  f(...) {<...\n-- sha1_to_hex(OIDPTR->hash)\n-+ oid_to_hex(OIDPTR)\n-  ...>}\n-\n @@\n struct object_id OID;\n @@\ndiff --git a/hex.c b/hex.c\nindex 8c3f06a192..fd7f00c43f 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -103,11 +103,6 @@ char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *a\n \treturn hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);\n }\n \n-char *sha1_to_hex(const unsigned char *sha1)\n-{\n-\treturn hash_to_hex_algop(sha1, &hash_algos[GIT_HASH_SHA1]);\n-}\n-\n char *hash_to_hex(const unsigned char *hash)\n {\n \treturn hash_to_hex_algop(hash, the_hash_algo);\ndiff --git a/sha1dc_git.c b/sha1dc_git.c\nindex e0cc9d988c..5c300e812e 100644\n--- a/sha1dc_git.c\n+++ b/sha1dc_git.c\n@@ -19,7 +19,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)\n \tif (!SHA1DCFinal(hash, ctx))\n \t\treturn;\n \tdie(\"SHA-1 appears to be part of a collision attack: %s\",\n-\t    sha1_to_hex(hash));\n+\t    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));\n }\n \n /*\n-- \n2.24.0.739.gb5632e4929\n"},{"id":"385899","messageId":"xmqqmud2hj9l.fsf@gitster-ct.c.googlers.com","threadId":"52234","inReplyTo":"20191111090332.GA2275@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] getting rid of sha1_to_hex()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-11T09:09:10Z","receivedAt":"2019-11-11T09:09:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I happened to be looking at the oid_to_hex() family of functions today,\n> and noticed that we can drop some of the sha1-specific ones.\n>\n> It's a minor cleanup in terms of text, but it feels like a nice\n> milestone. :)\n>\n>   [1/2]: hex: drop sha1_to_hex_r()\n>   [2/2]: hex: drop sha1_to_hex()\n\nNice indeed.\n\nThanks, Brian! ;-)  oh, and Peff, of courese ;-)\n"},{"id":"385901","messageId":"20191111092144.GA12607@sigill.intra.peff.net","threadId":"52234","inReplyTo":"xmqqmud2hj9l.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/2] getting rid of sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-11T09:21:44Z","receivedAt":"2019-11-11T09:21:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 11, 2019 at 06:09:10PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I happened to be looking at the oid_to_hex() family of functions today,\n> > and noticed that we can drop some of the sha1-specific ones.\n> >\n> > It's a minor cleanup in terms of text, but it feels like a nice\n> > milestone. :)\n> >\n> >   [1/2]: hex: drop sha1_to_hex_r()\n> >   [2/2]: hex: drop sha1_to_hex()\n> \n> Nice indeed.\n> \n> Thanks, Brian! ;-)  oh, and Peff, of courese ;-)\n\nI actually worried that I might be stealing brian's thunder. But AFAICT\nthis cleanup has been waiting for almost a year, so I figured it was\nfair game. :)\n\n-Peff\n"},{"id":"385907","messageId":"20191111141805.GK4348@szeder.dev","threadId":"52234","inReplyTo":"20191111090418.GB12545@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-11T14:18:05Z","receivedAt":"2019-11-11T14:18:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 11, 2019 at 04:04:18AM -0500, Jeff King wrote:\n> There's only a single caller left of sha1_to_hex(), since everybody now\n> uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n> print a hex sha1 when we find a collision. This one will always be sha1,\n> regardless of the current hash algorithm, so we can't use oid_to_hex()\n\nNit: s/oid_to_hex/hash_to_hex/\n\nWe can't use oid_to_hex() because we don't have a 'struct object_id'\nin the first place, as sha1dc only ever deals with 20 unsigned chars.\n\n> here. In practice we'd probably not be running sha1 at all if it isn't\n> the current algorithm, but it's possible we might still occasionally\n> need to compute a sha1 in a post-sha256 world.\n> \n> Since sha1_to_hex() is just a wrapper for hash_to_hex_algop(), let's\n> call that ourselves. There's value in getting rid of the sha1-specific\n> wrapper to de-clutter the global namespace, and to make sure nobody uses\n> it (and as with sha1_to_hex_r() in the previous patch, we'll drop the\n> coccinelle transformations, too).\n\n\n> diff --git a/sha1dc_git.c b/sha1dc_git.c\n> index e0cc9d988c..5c300e812e 100644\n> --- a/sha1dc_git.c\n> +++ b/sha1dc_git.c\n> @@ -19,7 +19,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)\n>  \tif (!SHA1DCFinal(hash, ctx))\n>  \t\treturn;\n>  \tdie(\"SHA-1 appears to be part of a collision attack: %s\",\n> -\t    sha1_to_hex(hash));\n> +\t    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));\n>  }\n>  \n>  /*\n> -- \n> 2.24.0.739.gb5632e4929\n"},{"id":"385910","messageId":"20191111142904.GB1934@sigill.intra.peff.net","threadId":"52234","inReplyTo":"20191111141805.GK4348@szeder.dev","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-11T14:29:04Z","receivedAt":"2019-11-11T14:29:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 11, 2019 at 03:18:05PM +0100, SZEDER Gábor wrote:\n\n> On Mon, Nov 11, 2019 at 04:04:18AM -0500, Jeff King wrote:\n> > There's only a single caller left of sha1_to_hex(), since everybody now\n> > uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n> > print a hex sha1 when we find a collision. This one will always be sha1,\n> > regardless of the current hash algorithm, so we can't use oid_to_hex()\n> \n> Nit: s/oid_to_hex/hash_to_hex/\n> \n> We can't use oid_to_hex() because we don't have a 'struct object_id'\n> in the first place, as sha1dc only ever deals with 20 unsigned chars.\n\nAh, you're right. I admit I am still getting up to speed on all of the\nnew hash-agnostic versions of the various functions.\n\n-Peff\n"},{"id":"385921","messageId":"nycvar.QRO.7.76.6.1911111929550.46@tvgsbejvaqbjf.bet","threadId":"52234","inReplyTo":"20191111090411.GA12545@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] hex: drop sha1_to_hex_r()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-11T18:30:10Z","receivedAt":"2019-11-11T19:11:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 11 Nov 2019, Jeff King wrote:\n\n> There are no callers left; everybody uses oid_to_hex_r() or\n> hash_to_hex_algop_r(). This used to actually be the underlying\n> implementation for oid_to_hex_r(), but that's no longer the case since\n> 47edb64997 (hex: introduce functions to print arbitrary hashes,\n> 2018-11-14).\n>\n> Let's get rid of it to de-clutter and to make sure nobody uses it.\n> Likewise we can drop the coccinelle rules that mention it, since the\n> compiler will make it quite clear that the code does not work.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  cache.h                            |  1 -\n>  contrib/coccinelle/object_id.cocci | 17 -----------------\n>  hex.c                              |  5 -----\n>  3 files changed, 23 deletions(-)\n\nI really like that diffstat.\n\nThanks,\nDscho\n\n>\n> diff --git a/cache.h b/cache.h\n> index 04cabaac11..6a4eb221b3 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1462,7 +1462,6 @@ int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n>   *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n>   */\n>  char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n> -char *sha1_to_hex_r(char *out, const unsigned char *sha1);\n>  char *oid_to_hex_r(char *out, const struct object_id *oid);\n>  char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *);\t/* static buffer result! */\n>  char *sha1_to_hex(const unsigned char *sha1);\t\t\t\t\t\t/* same static buffer */\n> diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci\n> index 3e536a9834..6c0d21d8e2 100644\n> --- a/contrib/coccinelle/object_id.cocci\n> +++ b/contrib/coccinelle/object_id.cocci\n> @@ -25,23 +25,6 @@ struct object_id *OIDPTR;\n>  + oid_to_hex(OIDPTR)\n>    ...>}\n>\n> -@@\n> -expression E;\n> -struct object_id OID;\n> -@@\n> -- sha1_to_hex_r(E, OID.hash)\n> -+ oid_to_hex_r(E, &OID)\n> -\n> -@@\n> -identifier f != oid_to_hex_r;\n> -expression E;\n> -struct object_id *OIDPTR;\n> -@@\n> -   f(...) {<...\n> -- sha1_to_hex_r(E, OIDPTR->hash)\n> -+ oid_to_hex_r(E, OIDPTR)\n> -  ...>}\n> -\n>  @@\n>  struct object_id OID;\n>  @@\n> diff --git a/hex.c b/hex.c\n> index 7850a8879d..8c3f06a192 100644\n> --- a/hex.c\n> +++ b/hex.c\n> @@ -90,11 +90,6 @@ char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash,\n>  \treturn buffer;\n>  }\n>\n> -char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)\n> -{\n> -\treturn hash_to_hex_algop_r(buffer, sha1, &hash_algos[GIT_HASH_SHA1]);\n> -}\n> -\n>  char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n>  {\n>  \treturn hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);\n> --\n> 2.24.0.739.gb5632e4929\n>\n>\n"},{"id":"385955","messageId":"20191111235338.GA9216@camp.crustytoothpaste.net","threadId":"52234","inReplyTo":"20191111092144.GA12607@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] getting rid of sha1_to_hex()","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-11-11T23:53:38Z","receivedAt":"2019-11-11T23:53:47Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-11-11 at 09:21:44, Jeff King wrote:\n> I actually worried that I might be stealing brian's thunder. But AFAICT\n> this cleanup has been waiting for almost a year, so I figured it was\n> fair game. :)\n\nOh, no, not at all.  I'm delighted that it can go, and thanks for\nsending the patches.\n\nI think I had intended to remove it but hadn't because I was worried\nabout affecting topics in flight, and then promptly forgot about it.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"385974","messageId":"xmqqa791hgu1.fsf@gitster-ct.c.googlers.com","threadId":"52234","inReplyTo":"20191111142904.GB1934@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-12T04:13:58Z","receivedAt":"2019-11-12T04:14:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Nov 11, 2019 at 03:18:05PM +0100, SZEDER Gábor wrote:\n>\n>> On Mon, Nov 11, 2019 at 04:04:18AM -0500, Jeff King wrote:\n>> > There's only a single caller left of sha1_to_hex(), since everybody now\n>> > uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n>> > print a hex sha1 when we find a collision. This one will always be sha1,\n>> > regardless of the current hash algorithm, so we can't use oid_to_hex()\n>> \n>> Nit: s/oid_to_hex/hash_to_hex/\n>> \n>> We can't use oid_to_hex() because we don't have a 'struct object_id'\n>> in the first place, as sha1dc only ever deals with 20 unsigned chars.\n>\n> Ah, you're right. I admit I am still getting up to speed on all of the\n> new hash-agnostic versions of the various functions.\n\nThanks.  I've amended this one and the range diff since the pushout\nyesterday looks like this.\n\n1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()\n    @@ Commit message\n         hex: drop sha1_to_hex()\n     \n         There's only a single caller left of sha1_to_hex(), since everybody now\n    -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n    +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we\n         print a hex sha1 when we find a collision. This one will always be sha1,\n    -    regardless of the current hash algorithm, so we can't use oid_to_hex()\n    +    regardless of the current hash algorithm, so we can't use hash_to_hex()\n         here. In practice we'd probably not be running sha1 at all if it isn't\n         the current algorithm, but it's possible we might still occasionally\n         need to compute a sha1 in a post-sha256 world.\n    @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n       * buffers, making it safe to make multiple calls for a single statement, like:\n       *\n     - *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n    -+ *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n    ++ *   printf(\"%s -> %s\", hash_to_hex(one), hash_to_hex(two));\n       */\n      char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n      char *oid_to_hex_r(char *out, const struct object_id *oid);\n"},{"id":"386004","messageId":"20191112105759.GA9714@sigill.intra.peff.net","threadId":"52234","inReplyTo":"xmqqa791hgu1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-12T10:57:59Z","receivedAt":"2019-11-12T10:58:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 12, 2019 at 01:13:58PM +0900, Junio C Hamano wrote:\n\n> >> We can't use oid_to_hex() because we don't have a 'struct object_id'\n> >> in the first place, as sha1dc only ever deals with 20 unsigned chars.\n> >\n> > Ah, you're right. I admit I am still getting up to speed on all of the\n> > new hash-agnostic versions of the various functions.\n> \n> Thanks.  I've amended this one and the range diff since the pushout\n> yesterday looks like this.\n\nThanks. This first hunk is what I would have done:\n\n> 1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()\n>     @@ Commit message\n>          hex: drop sha1_to_hex()\n>      \n>          There's only a single caller left of sha1_to_hex(), since everybody now\n>     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n>     +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we\n>          print a hex sha1 when we find a collision. This one will always be sha1,\n>     -    regardless of the current hash algorithm, so we can't use oid_to_hex()\n>     +    regardless of the current hash algorithm, so we can't use hash_to_hex()\n>          here. In practice we'd probably not be running sha1 at all if it isn't\n>          the current algorithm, but it's possible we might still occasionally\n>          need to compute a sha1 in a post-sha256 world.\n\nThis second one is OK, but not entirely necessary:\n\n>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n>        * buffers, making it safe to make multiple calls for a single statement, like:\n>        *\n>      - *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n>     -+ *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n>     ++ *   printf(\"%s -> %s\", hash_to_hex(one), hash_to_hex(two));\n>        */\n>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n>       char *oid_to_hex_r(char *out, const struct object_id *oid);\n\nThis one-liner leaves the types of \"one\" and \"two\" unspecified. :) So\nit's not wrong to use hash_to_hex(), but maybe it's better to be pushing\npeople towards oid_to_hex() as their first choice? It probably doesn't\nmatter too much either way.\n\n-Peff\n"},{"id":"386007","messageId":"20191112114458.GP4348@szeder.dev","threadId":"52234","inReplyTo":"20191112105759.GA9714@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-12T11:44:58Z","receivedAt":"2019-11-12T11:45:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 12, 2019 at 05:57:59AM -0500, Jeff King wrote:\n> On Tue, Nov 12, 2019 at 01:13:58PM +0900, Junio C Hamano wrote:\n> \n> > >> We can't use oid_to_hex() because we don't have a 'struct object_id'\n> > >> in the first place, as sha1dc only ever deals with 20 unsigned chars.\n> > >\n> > > Ah, you're right. I admit I am still getting up to speed on all of the\n> > > new hash-agnostic versions of the various functions.\n> > \n> > Thanks.  I've amended this one and the range diff since the pushout\n> > yesterday looks like this.\n> \n> Thanks. This first hunk is what I would have done:\n> \n> > 1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()\n> >     @@ Commit message\n> >          hex: drop sha1_to_hex()\n> >      \n> >          There's only a single caller left of sha1_to_hex(), since everybody now\n> >     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n> >     +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we\n> >          print a hex sha1 when we find a collision. This one will always be sha1,\n> >     -    regardless of the current hash algorithm, so we can't use oid_to_hex()\n> >     +    regardless of the current hash algorithm, so we can't use hash_to_hex()\n> >          here. In practice we'd probably not be running sha1 at all if it isn't\n> >          the current algorithm, but it's possible we might still occasionally\n> >          need to compute a sha1 in a post-sha256 world.\n> \n> This second one is OK, but not entirely necessary:\n> \n> >     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n> >        * buffers, making it safe to make multiple calls for a single statement, like:\n> >        *\n> >      - *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n> >     -+ *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n> >     ++ *   printf(\"%s -> %s\", hash_to_hex(one), hash_to_hex(two));\n> >        */\n> >       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n> >       char *oid_to_hex_r(char *out, const struct object_id *oid);\n> \n> This one-liner leaves the types of \"one\" and \"two\" unspecified. :) So\n> it's not wrong to use hash_to_hex(), but maybe it's better to be pushing\n> people towards oid_to_hex() as their first choice? It probably doesn't\n> matter too much either way.\n\nYeah, most (over 96%) of the hashes that we print are actually object\nids, so oid_to_hex() is the right function to use most of the time.\n\nAnd because of this the updated \"since everybody uses hash_to_hex()\"\nin the first hunk sounds a bit wrong, because barely anybody actually\nuses hash_to_hex().\n\n"},{"id":"386008","messageId":"xmqqimnpe2lj.fsf@gitster-ct.c.googlers.com","threadId":"52234","inReplyTo":"20191112105759.GA9714@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-12T11:49:44Z","receivedAt":"2019-11-12T11:49:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n>>        * buffers, making it safe to make multiple calls for a single statement, like:\n>>        *\n>>      - *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n>>     -+ *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n>>     ++ *   printf(\"%s -> %s\", hash_to_hex(one), hash_to_hex(two));\n>>        */\n>>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n>>       char *oid_to_hex_r(char *out, const struct object_id *oid);\n>\n> This one-liner leaves the types of \"one\" and \"two\" unspecified. :) So\n> it's not wrong to use hash_to_hex(), but maybe it's better to be pushing\n> people towards oid_to_hex() as their first choice? It probably doesn't\n> matter too much either way.\n\nThe pre-context of that comment reads:\n\n * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,\n * and writes the NUL-terminated output to the buffer `out`, which must be at\n * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for\n * convenience.\n\nso I think the intent of the example that used to use sha1_to_hex()\nhas been the raw bytestring that is the object name, not its form\nthat is encapsulated in the \"struct object_id()\".\n\n"},{"id":"386009","messageId":"20191112121247.GA12074@sigill.intra.peff.net","threadId":"52234","inReplyTo":"20191112114458.GP4348@szeder.dev","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-12T12:12:47Z","receivedAt":"2019-11-12T12:12:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 12, 2019 at 12:44:58PM +0100, SZEDER Gábor wrote:\n\n> > > 1:  8a030f1796 ! 1:  02d21d4117 hex: drop sha1_to_hex()\n> > >     @@ Commit message\n> > >          hex: drop sha1_to_hex()\n> > >      \n> > >          There's only a single caller left of sha1_to_hex(), since everybody now\n> > >     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n> > >     +    uses hash_to_hex() instead. This case is in the sha1dc wrapper, where we\n> > >          print a hex sha1 when we find a collision. This one will always be sha1,\n> > >     -    regardless of the current hash algorithm, so we can't use oid_to_hex()\n> > >     +    regardless of the current hash algorithm, so we can't use hash_to_hex()\n> > >          here. In practice we'd probably not be running sha1 at all if it isn't\n> > >          the current algorithm, but it's possible we might still occasionally\n> > >          need to compute a sha1 in a post-sha256 world.\n> [...]\n> And because of this the updated \"since everybody uses hash_to_hex()\"\n> in the first hunk sounds a bit wrong, because barely anybody actually\n> uses hash_to_hex().\n\nYou're right, I should have read more carefully.\n\n-Peff\n"},{"id":"386010","messageId":"20191112121537.GB12074@sigill.intra.peff.net","threadId":"52234","inReplyTo":"xmqqimnpe2lj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-12T12:15:37Z","receivedAt":"2019-11-12T12:15:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 12, 2019 at 08:49:44PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n> >>        * buffers, making it safe to make multiple calls for a single statement, like:\n> >>        *\n> >>      - *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n> >>     -+ *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n> >>     ++ *   printf(\"%s -> %s\", hash_to_hex(one), hash_to_hex(two));\n> >>        */\n> >>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n> >>       char *oid_to_hex_r(char *out, const struct object_id *oid);\n> >\n> > This one-liner leaves the types of \"one\" and \"two\" unspecified. :) So\n> > it's not wrong to use hash_to_hex(), but maybe it's better to be pushing\n> > people towards oid_to_hex() as their first choice? It probably doesn't\n> > matter too much either way.\n> \n> The pre-context of that comment reads:\n> \n>  * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,\n>  * and writes the NUL-terminated output to the buffer `out`, which must be at\n>  * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for\n>  * convenience.\n> \n> so I think the intent of the example that used to use sha1_to_hex()\n> has been the raw bytestring that is the object name, not its form\n> that is encapsulated in the \"struct object_id()\".\n\nI guess you're keying on the phrase \"binary hash\" there (the\n\"GIT_MAX_HEXZ\" bits only apply to the \"_r\" variants anyway). I'd read it\nas encompassing all of the functions below, including oid_to_hex(). But\nI'm OK with it either way.\n\n-Peff\n"},{"id":"386075","messageId":"xmqqbltgeg58.fsf@gitster-ct.c.googlers.com","threadId":"52234","inReplyTo":"20191112121537.GB12074@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T01:09:23Z","receivedAt":"2019-11-13T01:09:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Nov 12, 2019 at 08:49:44PM +0900, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> >>     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n>> >>        * buffers, making it safe to make multiple calls for a single statement, like:\n>> >>        *\n>> >>      - *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n>> >>     -+ *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n>> >>     ++ *   printf(\"%s -> %s\", hash_to_hex(one), hash_to_hex(two));\n>> >>        */\n>> >>       char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n>> >>       char *oid_to_hex_r(char *out, const struct object_id *oid);\n>> >\n>> > This one-liner leaves the types of \"one\" and \"two\" unspecified. :) So\n>> > it's not wrong to use hash_to_hex(), but maybe it's better to be pushing\n>> > people towards oid_to_hex() as their first choice? It probably doesn't\n>> > matter too much either way.\n>> \n>> The pre-context of that comment reads:\n>> \n>>  * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,\n>>  * and writes the NUL-terminated output to the buffer `out`, which must be at\n>>  * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for\n>>  * convenience.\n>> \n>> so I think the intent of the example that used to use sha1_to_hex()\n>> has been the raw bytestring that is the object name, not its form\n>> that is encapsulated in the \"struct object_id()\".\n>\n> I guess you're keying on the phrase \"binary hash\" there (the\n> \"GIT_MAX_HEXZ\" bits only apply to the \"_r\" variants anyway). I'd read it\n> as encompassing all of the functions below, including oid_to_hex(). But\n> I'm OK with it either way.\n\nYes.  \"binary hash\" is about \"unsigned char[]\".  I think that's\nhistorical accident---we added \"struct object_id *\" variants without\nupdating the comment.\n\nHere is another try.  The \"everybody uses oid_to_hex\" in the log\nmessage has also been updated.\n\n1:  8a030f1796 ! 1:  b19f3fe9dd hex: drop sha1_to_hex()\n    @@ Metadata\n      ## Commit message ##\n         hex: drop sha1_to_hex()\n     \n    -    There's only a single caller left of sha1_to_hex(), since everybody now\n    -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n    -    print a hex sha1 when we find a collision. This one will always be sha1,\n    -    regardless of the current hash algorithm, so we can't use oid_to_hex()\n    -    here. In practice we'd probably not be running sha1 at all if it isn't\n    -    the current algorithm, but it's possible we might still occasionally\n    +    There's only a single caller left of sha1_to_hex(), since everybody\n    +    that has an object name in \"unsigned char[]\" now uses hash_to_hex()\n    +    instead.\n    +\n    +    This case is in the sha1dc wrapper, where we print a hex sha1 when\n    +    we find a collision. This one will always be sha1, regardless of the\n    +    current hash algorithm, so we can't use hash_to_hex() here. In\n    +    practice we'd probably not be running sha1 at all if it isn't the\n    +    current algorithm, but it's possible we might still occasionally\n         need to compute a sha1 in a post-sha256 world.\n     \n         Since sha1_to_hex() is just a wrapper for hash_to_hex_algop(), let's\n    @@ Commit message\n         it (and as with sha1_to_hex_r() in the previous patch, we'll drop the\n         coccinelle transformations, too).\n     \n    -    The sha1_to_hex() function is mentioned in a comment; we can easily swap\n    -    that out for oid_to_hex() to give a better example. It's also mentioned\n    -    in some test vectors in t4100, but that's not runnable code, so there's\n    -    no point in trying to clean it up.\n    +    The sha1_to_hex() function is mentioned in a comment; we can easily\n    +    swap that out for oid_to_hex() to give a better example.  Also\n    +    update the comment that was left stale when we added \"struct\n    +    object_id *\" as a way to name an object and added functions to\n    +    convert it to hex.\n    +\n    +    The function is also mentioned in some test vectors in t4100, but\n    +    that's not runnable code, so there's no point in trying to clean it\n    +    up.\n     \n         Signed-off-by: Jeff King <peff@peff.net>\n         Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      ## cache.h ##\n    +@@ cache.h: int get_oid_hex(const char *hex, struct object_id *sha1);\n    + int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n    + \n    + /*\n    +- * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,\n    ++ * Convert a binary hash in \"unsigned char []\" or an object name in\n    ++ * \"struct object_id *\" to its hex equivalent. The `_r` variant is reentrant,\n    +  * and writes the NUL-terminated output to the buffer `out`, which must be at\n    +  * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for\n    +  * convenience.\n     @@ cache.h: int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);\n       * The non-`_r` variant returns a static buffer, but uses a ring of 4\n       * buffers, making it safe to make multiple calls for a single statement, like:\n       *\n     - *   printf(\"%s -> %s\", sha1_to_hex(one), sha1_to_hex(two));\n    ++ *   printf(\"%s -> %s\", hash_to_hex(one), hash_to_hex(two));\n     + *   printf(\"%s -> %s\", oid_to_hex(one), oid_to_hex(two));\n       */\n      char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);\n"},{"id":"386078","messageId":"20191113011507.GC20431@sigill.intra.peff.net","threadId":"52234","inReplyTo":"xmqqbltgeg58.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] hex: drop sha1_to_hex()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-13T01:15:07Z","receivedAt":"2019-11-13T01:15:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 13, 2019 at 10:09:23AM +0900, Junio C Hamano wrote:\n\n> Yes.  \"binary hash\" is about \"unsigned char[]\".  I think that's\n> historical accident---we added \"struct object_id *\" variants without\n> updating the comment.\n> \n> Here is another try.  The \"everybody uses oid_to_hex\" in the log\n> message has also been updated.\n\nThis looks good to me (and thank you for cleaning up my messy commit\nmessage).\n\n> 1:  8a030f1796 ! 1:  b19f3fe9dd hex: drop sha1_to_hex()\n>     @@ Metadata\n>       ## Commit message ##\n>          hex: drop sha1_to_hex()\n>      \n>     -    There's only a single caller left of sha1_to_hex(), since everybody now\n>     -    uses oid_to_hex() instead. This case is in the sha1dc wrapper, where we\n>     -    print a hex sha1 when we find a collision. This one will always be sha1,\n>     -    regardless of the current hash algorithm, so we can't use oid_to_hex()\n>     -    here. In practice we'd probably not be running sha1 at all if it isn't\n>     -    the current algorithm, but it's possible we might still occasionally\n>     +    There's only a single caller left of sha1_to_hex(), since everybody\n>     +    that has an object name in \"unsigned char[]\" now uses hash_to_hex()\n>     +    instead.\n\nThat makes sense. I mentioned oid_to_hex() originally because most of\nthe callers _did_ switch from sha1_to_hex(oid->hash) to oid_to_hex(oid).\nBut that happened far enough in the past that the more interesting\nchange is using the generic hash_to_hex().\n\n-Peff\n"}]}