{"thread":{"id":"47627","subject":"[PATCH] packfile: use get_be64() for large offsets","startedAt":"2018-01-17T19:08:47Z","lastAt":"2018-01-19T19:13:02Z","messageCount":5,"participants":["Derrick Stolee","Jeff King","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"336713","messageId":"20180117190823.209469-1-dstolee@microsoft.com","threadId":"47627","inReplyTo":null,"subject":"[PATCH] packfile: use get_be64() for large offsets","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-01-17T19:08:23Z","receivedAt":"2018-01-17T19:08:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The pack-index version 2 format uses two 4-byte integers in network-byte order to represent one 8-byte value. The current implementation has several code clones for stitching these integers together.\n\nUse get_be64() to create an 8-byte integer from two 4-byte integers represented this way.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n pack-revindex.c | 6 ++----\n packfile.c      | 3 +--\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 1b7ebd8d7e..ff5f62c033 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -134,10 +134,8 @@ static void create_pack_revindex(struct packed_git *p)\n \t\t\tif (!(off & 0x80000000)) {\n \t\t\t\tp->revindex[i].offset = off;\n \t\t\t} else {\n-\t\t\t\tp->revindex[i].offset =\n-\t\t\t\t\t((uint64_t)ntohl(*off_64++)) << 32;\n-\t\t\t\tp->revindex[i].offset |=\n-\t\t\t\t\tntohl(*off_64++);\n+\t\t\t\tp->revindex[i].offset = get_be64(off_64);\n+\t\t\t\toff_64 += 2;\n \t\t\t}\n \t\t\tp->revindex[i].nr = i;\n \t\t}\ndiff --git a/packfile.c b/packfile.c\nindex 4a5fe7ab18..228ed0d59a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1702,8 +1702,7 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n \t\t\treturn off;\n \t\tindex += p->num_objects * 4 + (off & 0x7fffffff) * 8;\n \t\tcheck_pack_index_ptr(p, index);\n-\t\treturn (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |\n-\t\t\t\t   ntohl(*((uint32_t *)(index + 4)));\n+\t\treturn get_be64(index);\n \t}\n }\n \n-- \n2.15.0\n\n"},{"id":"336736","messageId":"20180117222953.GB16031@sigill.intra.peff.net","threadId":"47627","inReplyTo":"20180117190823.209469-1-dstolee@microsoft.com","subject":"Re: [PATCH] packfile: use get_be64() for large offsets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-17T22:29:53Z","receivedAt":"2018-01-17T22:29:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 17, 2018 at 02:08:23PM -0500, Derrick Stolee wrote:\n\n> The pack-index version 2 format uses two 4-byte integers in network-byte order to represent one 8-byte value. The current implementation has several code clones for stitching these integers together.\n\nYeah, this seems like a no-brainer, and the patch itself looks obviously\ncorrect.\n\nPlease wrap your commit messages a bit shorter, though (say, between\n60-72 chars).\n\n-Peff\n"},{"id":"336768","messageId":"20180118110419.210006-1-dstolee@microsoft.com","threadId":"47627","inReplyTo":"20180117222953.GB16031@sigill.intra.peff.net","subject":"[PATCH v2] packfile: use get_be64() for large offsets","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-01-18T11:04:19Z","receivedAt":"2018-01-18T11:04:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The pack-index version 2 format uses two 4-byte integers in network-\nbyte order to represent one 8-byte value. The current implementation\nhas several code clones for stitching these integers together.\n\nUse get_be64() to create an 8-byte integer from two 4-byte integers\nrepresented this way.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n pack-revindex.c | 6 ++----\n packfile.c      | 3 +--\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 1b7ebd8d7e..ff5f62c033 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -134,10 +134,8 @@ static void create_pack_revindex(struct packed_git *p)\n \t\t\tif (!(off & 0x80000000)) {\n \t\t\t\tp->revindex[i].offset = off;\n \t\t\t} else {\n-\t\t\t\tp->revindex[i].offset =\n-\t\t\t\t\t((uint64_t)ntohl(*off_64++)) << 32;\n-\t\t\t\tp->revindex[i].offset |=\n-\t\t\t\t\tntohl(*off_64++);\n+\t\t\t\tp->revindex[i].offset = get_be64(off_64);\n+\t\t\t\toff_64 += 2;\n \t\t\t}\n \t\t\tp->revindex[i].nr = i;\n \t\t}\ndiff --git a/packfile.c b/packfile.c\nindex 4a5fe7ab18..228ed0d59a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1702,8 +1702,7 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n \t\t\treturn off;\n \t\tindex += p->num_objects * 4 + (off & 0x7fffffff) * 8;\n \t\tcheck_pack_index_ptr(p, index);\n-\t\treturn (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |\n-\t\t\t\t   ntohl(*((uint32_t *)(index + 4)));\n+\t\treturn get_be64(index);\n \t}\n }\n \n-- \n2.15.0\n\n"},{"id":"336775","messageId":"20180118141522.18294-1-szeder.dev@gmail.com","threadId":"47627","inReplyTo":"20180118110419.210006-1-dstolee@microsoft.com","subject":"Re: [PATCH v2] packfile: use get_be64() for large offsets","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-18T14:15:22Z","receivedAt":"2018-01-18T14:16:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Junio,\n\n> The pack-index version 2 format uses two 4-byte integers in network-\n> byte order to represent one 8-byte value. The current implementation\n> has several code clones for stitching these integers together.\n> \n> Use get_be64() to create an 8-byte integer from two 4-byte integers\n> represented this way.\n> \n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  pack-revindex.c | 6 ++----\n>  packfile.c      | 3 +--\n>  2 files changed, 3 insertions(+), 6 deletions(-)\n> \n> diff --git a/pack-revindex.c b/pack-revindex.c\n> index 1b7ebd8d7e..ff5f62c033 100644\n> --- a/pack-revindex.c\n> +++ b/pack-revindex.c\n> @@ -134,10 +134,8 @@ static void create_pack_revindex(struct packed_git *p)\n>  \t\t\tif (!(off & 0x80000000)) {\n>  \t\t\t\tp->revindex[i].offset = off;\n>  \t\t\t} else {\n> -\t\t\t\tp->revindex[i].offset =\n> -\t\t\t\t\t((uint64_t)ntohl(*off_64++)) << 32;\n> -\t\t\t\tp->revindex[i].offset |=\n> -\t\t\t\t\tntohl(*off_64++);\n> +\t\t\t\tp->revindex[i].offset = get_be64(off_64);\n> +\t\t\t\toff_64 += 2;\n>  \t\t\t}\n>  \t\t\tp->revindex[i].nr = i;\n>  \t\t}\n> diff --git a/packfile.c b/packfile.c\n> index 4a5fe7ab18..228ed0d59a 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1702,8 +1702,7 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n>  \t\t\treturn off;\n>  \t\tindex += p->num_objects * 4 + (off & 0x7fffffff) * 8;\n>  \t\tcheck_pack_index_ptr(p, index);\n> -\t\treturn (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |\n> -\t\t\t\t   ntohl(*((uint32_t *)(index + 4)));\n> +\t\treturn get_be64(index);\n>  \t}\n>  }\n>  \n> -- \n> 2.15.0\n> \n\nThis patch can't be applied to 'maint' currently at 3013dff86 (Prepare\nfor 2.15.2, 2017-12-06), as it is in case of 'ds/use-get-be64',\nbecause 'maint' doesn't have get_be64() yet (b2e39d006 (bswap: add 64\nbit endianness helper get_be64, 2017-09-22)).\n\n\nGábor\n\n"},{"id":"336925","messageId":"xmqqvafxpsru.fsf@gitster.mtv.corp.google.com","threadId":"47627","inReplyTo":"20180118141522.18294-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2] packfile: use get_be64() for large offsets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-19T19:12:21Z","receivedAt":"2018-01-19T19:13:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> Junio,\n> ...\n> This patch can't be applied to 'maint' currently at 3013dff86 (Prepare\n> for 2.15.2, 2017-12-06), as it is in case of 'ds/use-get-be64',\n> because 'maint' doesn't have get_be64() yet (b2e39d006 (bswap: add 64\n> bit endianness helper get_be64, 2017-09-22)).\n\nThanks for stopping me.  \n\nI do not always get to test anything not in 'next' in isolation\nbefore I actually try to merge it there (at which time I would have\nnoticed), and you caught me in this one doing a short-cut \"just\nqueue it somewhere and throw it in 'pu'\".\n\n\n"}]}