{"thread":{"id":"65283","subject":"[PATCH] commit-graph: fix writing generations with dates exceeding 34 bits","startedAt":"2026-03-17T19:03:12Z","lastAt":"2026-03-26T10:02:11Z","messageCount":8,"participants":["Patrick Steinhardt","Junio C Hamano","Derrick Stolee","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"539253","messageId":"20260317-pks-commit-graph-overflow-v1-1-e6bee22cd826@pks.im","threadId":"65283","inReplyTo":null,"subject":"[PATCH] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-17T19:02:52Z","receivedAt":"2026-03-17T19:03:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":null},"body":"The `timestamp_t` type is declared as `uintmax_t` and thus typically has\n64 bits of precision. Usually, the full precision of such dates is not\nrequired: it would be comforting to know that Git is still around in\nmillions of years, but all in all the chance is rather low.\n\nWe abuse this fact in the commit-graph: instead of storing the full 64\nbits of precision, committer dates only store 34 bits. This is still\nplenty of headroom, as it means that we can represent dates until year\n2514. Commits which are dated beyond that year will simply get a date\nwhose remaining bits are masked.\n\nThe result of this is somewhat curious: the committer date will be\ndifferent depending on whether a commit gets parsed via the commit-graph\nor via the object database. This isn't really too much of an issue in\ngeneral though, as we don't typically use the date parsed from the\ncommit-graph in user-facing output.\n\nBut with 024b4c9697 (commit: make `repo_parse_commit_no_graph()` more\nrobust, 2026-02-16) it started to become a problem when writing the\ncommit-graph itself. This commit changed `repo_parse_commit_no_graph()`\nso that we re-parse the commit via the object database in case it was\nalready parsed beforehand via the commit-graph.\n\nThe consequence is that we may now act with two different commit dates\nat different stages:\n\n  - Initially, we use the 34-bit precision timestamp when writing the\n    chunk generation data. We thus correctly compute the offsets\n    relative to the on-disk timestamp here.\n\n  - Later, when writing the overflow data, we may end up with the\n    full-precision timestamp. When the date is larger than 34 bits the\n    result of this is an underflow when computing the offset.\n\nThis causes a mismatch in the number of generation data overflow records\nwe want to write, and that ultimately causes Git to die.\n\nIntroduce a new helper function that computes the generation offset for\na commit while correctly masking the date to 34 bits. This makes the\npreviously-implicit assumptions about the commit date precision explicit\nand thus hopefully less fragile going forward.\n\nAdapt sites that compute the offset to use the function.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis fixes a regression recently introduced by myself in 024b4c9697\n(commit: make `repo_parse_commit_no_graph()` more robust, 2026-02-16).\nThe regression was found by GitLab's tests suite, see [1].\n\nThanks!\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/gitlab/-/jobs/13522328632\n---\n commit-graph.c          | 31 ++++++++++++++++++++++++++++---\n t/t5318-commit-graph.sh | 20 ++++++++++++++++++++\n 2 files changed, 48 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f8e24145a5..7e293a1775 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1319,6 +1319,31 @@ static int write_graph_chunk_data(struct hashfile *f,\n \treturn 0;\n }\n \n+/*\n+ * Compute the generation offset between the commit date and its generation.\n+ * This is what's ultimately stored as generation number in the commit graph.\n+ *\n+ * Note that the computation of the commit date is more involved than you might\n+ * think. Instead of using the full commit date, we're in fact masking bits so\n+ * that only the 34 lowest bits are considered. This results from the fact that\n+ * commit graphs themselves only ever store 34 bits of the commit date\n+ * themselves.\n+ *\n+ * This means that if we have a commit date that exceeds 34 bits we'll end up\n+ * in situations where depending on whether the commit has been parsed from the\n+ * object database or the commit graph we'll have different dates, where the\n+ * ones parsed from the object database would have full 64 bit precision.\n+ *\n+ * But ultimately, we only ever want the offset to be relative to what we\n+ * actually end up storing on disk, and hence we have to mask all the other\n+ * bits.\n+ */\n+static timestamp_t compute_generation_offset(struct commit *c)\n+{\n+\ttimestamp_t masked_date = c->date & (((timestamp_t) 1 << 34) - 1);\n+\treturn commit_graph_data_at(c)->generation - masked_date;\n+}\n+\n static int write_graph_chunk_generation_data(struct hashfile *f,\n \t\t\t\t\t     void *data)\n {\n@@ -1329,7 +1354,7 @@ static int write_graph_chunk_generation_data(struct hashfile *f,\n \t\tstruct commit *c = ctx->commits.items[i];\n \t\ttimestamp_t offset;\n \t\trepo_parse_commit(ctx->r, c);\n-\t\toffset = commit_graph_data_at(c)->generation - c->date;\n+\t\toffset = compute_generation_offset(c);\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n@@ -1350,7 +1375,7 @@ static int write_graph_chunk_generation_data_overflow(struct hashfile *f,\n \tint i;\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.items[i];\n-\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\n+\t\ttimestamp_t offset = compute_generation_offset(c);\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n@@ -1741,7 +1766,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.items[i];\n-\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\n+\t\ttimestamp_t offset = compute_generation_offset(c);\n \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX)\n \t\t\tctx->num_generation_data_overflows++;\n \t}\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 98c6910963..1c40f904f8 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -417,6 +417,26 @@ test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'lower layers have overflow ch\n \ttest_cmp full/.git/objects/info/commit-graph commit-graph-upgraded\n '\n \n+test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'overflow chunk when replacing commit-graph' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tcat >commit <<-EOF &&\n+\t\ttree $(test_oid empty_tree)\n+\t\tauthor Example <committer@example.com> 9223372036854775 +0000\n+\t\tcommitter Example <committer@example.com> 9223372036854775 +0000\n+\n+\t\tWeird commit date\n+\t\tEOF\n+\t\tcommit_id=$(git hash-object -t commit -w commit) &&\n+\t\tgit reset --hard \"$commit_id\" &&\n+\t\tgit commit-graph write --reachable &&\n+\t\tgit commit-graph write --reachable --split=replace &&\n+\t\tgit log\n+\t)\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n\n---\nbase-commit: ca1db8a0f7dc0dbea892e99f5b37c5fe5861be71\nchange-id: 20260317-pks-commit-graph-overflow-09c3fb6e259a\n\n"},{"id":"539255","messageId":"xmqq341ykzyc.fsf@gitster.g","threadId":"65283","inReplyTo":"20260317-pks-commit-graph-overflow-v1-1-e6bee22cd826@pks.im","subject":"Re: [PATCH] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-17T19:39:39Z","receivedAt":"2026-03-17T19:39:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":null},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> this fixes a regression recently introduced by myself in 024b4c9697\n> (commit: make `repo_parse_commit_no_graph()` more robust, 2026-02-16).\n> The regression was found by GitLab's tests suite, see [1].\n\nCurious.  GitLab's test suite runs pretending that it is way past\nyear 2600 or something?  \n\n> [1]: https://gitlab.com/gitlab-org/gitlab/-/jobs/13522328632\n> ---\n>  commit-graph.c          | 31 ++++++++++++++++++++++++++++---\n>  t/t5318-commit-graph.sh | 20 ++++++++++++++++++++\n>  2 files changed, 48 insertions(+), 3 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f8e24145a5..7e293a1775 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1319,6 +1319,31 @@ static int write_graph_chunk_data(struct hashfile *f,\n>  \treturn 0;\n>  }\n>  \n> +/*\n> + * Compute the generation offset between the commit date and its generation.\n> + * This is what's ultimately stored as generation number in the commit graph.\n> ...\n> + * actually end up storing on disk, and hence we have to mask all the other\n> + * bits.\n> + */\n> +static timestamp_t compute_generation_offset(struct commit *c)\n> +{\n> +\ttimestamp_t masked_date = c->date & (((timestamp_t) 1 << 34) - 1);\n> +\treturn commit_graph_data_at(c)->generation - masked_date;\n> +}\n> +\n>  static int write_graph_chunk_generation_data(struct hashfile *f,\n>  \t\t\t\t\t     void *data)\n>  {\n\nThe code and explanation are both in line with why we want to do\nthis change in the proposed log message.\n\n> +test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'overflow chunk when replacing commit-graph' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tcat >commit <<-EOF &&\n> +\t\ttree $(test_oid empty_tree)\n> +\t\tauthor Example <committer@example.com> 9223372036854775 +0000\n> +\t\tcommitter Example <committer@example.com> 9223372036854775 +0000\n\nThis timestamp is way longer than 34-bit for sure.\n\n100000110001001001101110100101111000110101001111110111 (base 2)\n\n> +\n> +\t\tWeird commit date\n> +\t\tEOF\n> +\t\tcommit_id=$(git hash-object -t commit -w commit) &&\n> +\t\tgit reset --hard \"$commit_id\" &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\tgit commit-graph write --reachable --split=replace &&\n> +\t\tgit log\n> +\t)\n> +'\n> +\n>  # the verify tests below expect the commit-graph to contain\n>  # exactly the commits reachable from the commits/8 branch.\n>  # If the file changes the set of commits in the list, then the\n\nWill queue.  Thanks.\n"},{"id":"539367","messageId":"abueHjVHCztQtL6b@pks.im","threadId":"65283","inReplyTo":"xmqq341ykzyc.fsf@gitster.g","subject":"Re: [PATCH] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-19T06:56:30Z","receivedAt":"2026-03-19T06:56:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":null},"body":"On Tue, Mar 17, 2026 at 12:39:39PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > this fixes a regression recently introduced by myself in 024b4c9697\n> > (commit: make `repo_parse_commit_no_graph()` more robust, 2026-02-16).\n> > The regression was found by GitLab's tests suite, see [1].\n> \n> Curious.  GitLab's test suite runs pretending that it is way past\n> year 2600 or something?  \n\nNo, but one of the test repositories that we're running tests with [1]\ncontains such a commit.\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/gitlab-test\n"},{"id":"539572","messageId":"0566e178-4f12-4647-a257-29631a769b49@gmail.com","threadId":"65283","inReplyTo":"abueHjVHCztQtL6b@pks.im","subject":"Re: [PATCH] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-03-20T17:21:50Z","receivedAt":"2026-03-20T17:21:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":null},"body":"On 3/19/2026 2:56 AM, Patrick Steinhardt wrote:\n> On Tue, Mar 17, 2026 at 12:39:39PM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt <ps@pks.im> writes:\n>>\n>>> this fixes a regression recently introduced by myself in 024b4c9697\n>>> (commit: make `repo_parse_commit_no_graph()` more robust, 2026-02-16).\n>>> The regression was found by GitLab's tests suite, see [1].\n>>\n>> Curious.  GitLab's test suite runs pretending that it is way past\n>> year 2600 or something?  \n> \n> No, but one of the test repositories that we're running tests with [1]\n> contains such a commit.\n> \n> Patrick\n> \n> [1]: https://gitlab.com/gitlab-org/gitlab-test\n \nI'm glad you have this test! Thanks for bringing such a data shape\ninto the Git test suite with this change so we can correct problems\nthis way in our inner loop testing.\n\nI agree with Junio that the code looks good.\n\nThanks,\n-Stolee\n\n"},{"id":"539806","messageId":"20260324-pks-commit-graph-overflow-v2-1-843568cf8780@pks.im","threadId":"65283","inReplyTo":"20260317-pks-commit-graph-overflow-v1-1-e6bee22cd826@pks.im","subject":"[PATCH v2] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-24T06:18:26Z","receivedAt":"2026-03-24T06:18:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":null},"body":"The `timestamp_t` type is declared as `uintmax_t` and thus typically has\n64 bits of precision. Usually, the full precision of such dates is not\nrequired: it would be comforting to know that Git is still around in\nmillions of years, but all in all the chance is rather low.\n\nWe abuse this fact in the commit-graph: instead of storing the full 64\nbits of precision, committer dates only store 34 bits. This is still\nplenty of headroom, as it means that we can represent dates until year\n2514. Commits which are dated beyond that year will simply get a date\nwhose remaining bits are masked.\n\nThe result of this is somewhat curious: the committer date will be\ndifferent depending on whether a commit gets parsed via the commit-graph\nor via the object database. This isn't really too much of an issue in\ngeneral though, as we don't typically use the date parsed from the\ncommit-graph in user-facing output.\n\nBut with 024b4c9697 (commit: make `repo_parse_commit_no_graph()` more\nrobust, 2026-02-16) it started to become a problem when writing the\ncommit-graph itself. This commit changed `repo_parse_commit_no_graph()`\nso that we re-parse the commit via the object database in case it was\nalready parsed beforehand via the commit-graph.\n\nThe consequence is that we may now act with two different commit dates\nat different stages:\n\n  - Initially, we use the 34-bit precision timestamp when writing the\n    chunk generation data. We thus correctly compute the offsets\n    relative to the on-disk timestamp here.\n\n  - Later, when writing the overflow data, we may end up with the\n    full-precision timestamp. When the date is larger than 34 bits the\n    result of this is an underflow when computing the offset.\n\nThis causes a mismatch in the number of generation data overflow records\nwe want to write, and that ultimately causes Git to die.\n\nIntroduce a new helper function that computes the generation offset for\na commit while correctly masking the date to 34 bits. This makes the\npreviously-implicit assumptions about the commit date precision explicit\nand thus hopefully less fragile going forward.\n\nAdapt sites that compute the offset to use the function.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis fixes a regression recently introduced by myself in 024b4c9697\n(commit: make `repo_parse_commit_no_graph()` more robust, 2026-02-16).\nThe regression was found by GitLab's tests suite, see [1].\n\nChanges in v2:\n  - Account for platforms where `timestamp_t` has 32 bit precision. This\n    matches logic in `write_graph_chunk_data()`, where we also depend on\n    the size of the commit timestamps.\n  - Link to v1: https://lore.kernel.org/r/20260317-pks-commit-graph-overflow-v1-1-e6bee22cd826@pks.im\n\nThanks!\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/gitlab/-/jobs/13522328632\n---\n commit-graph.c          | 37 ++++++++++++++++++++++++++++++++++---\n t/t5318-commit-graph.sh | 20 ++++++++++++++++++++\n 2 files changed, 54 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f8e24145a5..cb514bfb60 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1319,6 +1319,37 @@ static int write_graph_chunk_data(struct hashfile *f,\n \treturn 0;\n }\n \n+/*\n+ * Compute the generation offset between the commit date and its generation.\n+ * This is what's ultimately stored as generation number in the commit graph.\n+ *\n+ * Note that the computation of the commit date is more involved than you might\n+ * think. Instead of using the full commit date, we're in fact masking bits so\n+ * that only the 34 lowest bits are considered. This results from the fact that\n+ * commit graphs themselves only ever store 34 bits of the commit date\n+ * themselves.\n+ *\n+ * This means that if we have a commit date that exceeds 34 bits we'll end up\n+ * in situations where depending on whether the commit has been parsed from the\n+ * object database or the commit graph we'll have different dates, where the\n+ * ones parsed from the object database would have full 64 bit precision.\n+ *\n+ * But ultimately, we only ever want the offset to be relative to what we\n+ * actually end up storing on disk, and hence we have to mask all the other\n+ * bits.\n+ */\n+static timestamp_t compute_generation_offset(struct commit *c)\n+{\n+\ttimestamp_t masked_date;\n+\n+\tif (sizeof(timestamp_t) > 4)\n+\t\tmasked_date = c->date & (((timestamp_t) 1 << 34) - 1);\n+\telse\n+\t\tmasked_date = c->date;\n+\n+\treturn commit_graph_data_at(c)->generation - masked_date;\n+}\n+\n static int write_graph_chunk_generation_data(struct hashfile *f,\n \t\t\t\t\t     void *data)\n {\n@@ -1329,7 +1360,7 @@ static int write_graph_chunk_generation_data(struct hashfile *f,\n \t\tstruct commit *c = ctx->commits.items[i];\n \t\ttimestamp_t offset;\n \t\trepo_parse_commit(ctx->r, c);\n-\t\toffset = commit_graph_data_at(c)->generation - c->date;\n+\t\toffset = compute_generation_offset(c);\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n@@ -1350,7 +1381,7 @@ static int write_graph_chunk_generation_data_overflow(struct hashfile *f,\n \tint i;\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.items[i];\n-\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\n+\t\ttimestamp_t offset = compute_generation_offset(c);\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n@@ -1741,7 +1772,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.items[i];\n-\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\n+\t\ttimestamp_t offset = compute_generation_offset(c);\n \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX)\n \t\t\tctx->num_generation_data_overflows++;\n \t}\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 98c6910963..1c40f904f8 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -417,6 +417,26 @@ test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'lower layers have overflow ch\n \ttest_cmp full/.git/objects/info/commit-graph commit-graph-upgraded\n '\n \n+test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'overflow chunk when replacing commit-graph' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tcat >commit <<-EOF &&\n+\t\ttree $(test_oid empty_tree)\n+\t\tauthor Example <committer@example.com> 9223372036854775 +0000\n+\t\tcommitter Example <committer@example.com> 9223372036854775 +0000\n+\n+\t\tWeird commit date\n+\t\tEOF\n+\t\tcommit_id=$(git hash-object -t commit -w commit) &&\n+\t\tgit reset --hard \"$commit_id\" &&\n+\t\tgit commit-graph write --reachable &&\n+\t\tgit commit-graph write --reachable --split=replace &&\n+\t\tgit log\n+\t)\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n\n---\nbase-commit: ca1db8a0f7dc0dbea892e99f5b37c5fe5861be71\nchange-id: 20260317-pks-commit-graph-overflow-09c3fb6e259a\n\n"},{"id":"539849","messageId":"xmqq1ph92pzs.fsf@gitster.g","threadId":"65283","inReplyTo":"20260324-pks-commit-graph-overflow-v2-1-843568cf8780@pks.im","subject":"Re: [PATCH v2] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T15:42:31Z","receivedAt":"2026-03-24T15:42:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":null},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Changes in v2:\n>   - Account for platforms where `timestamp_t` has 32 bit precision. This\n>     matches logic in `write_graph_chunk_data()`, where we also depend on\n>     the size of the commit timestamps.\n\n> +static timestamp_t compute_generation_offset(struct commit *c)\n> +{\n> +\ttimestamp_t masked_date;\n> +\n> +\tif (sizeof(timestamp_t) > 4)\n> +\t\tmasked_date = c->date & (((timestamp_t) 1 << 34) - 1);\n> +\telse\n> +\t\tmasked_date = c->date;\n\nIt is a bit surprising that on a platform where timestamp_t is only\n32-bit wide, a smart-enough compiler would not find (1<<34) as\nsuspicious.  IOW, I would have expected this to be done not with\nruntime switch but with conditional compilation.\n\n"},{"id":"539894","messageId":"acN0efJvZ9liex8d@pks.im","threadId":"65283","inReplyTo":"xmqq1ph92pzs.fsf@gitster.g","subject":"Re: [PATCH v2] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-25T05:36:57Z","receivedAt":"2026-03-25T05:37:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":null},"body":"On Tue, Mar 24, 2026 at 08:42:31AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Changes in v2:\n> >   - Account for platforms where `timestamp_t` has 32 bit precision. This\n> >     matches logic in `write_graph_chunk_data()`, where we also depend on\n> >     the size of the commit timestamps.\n> \n> > +static timestamp_t compute_generation_offset(struct commit *c)\n> > +{\n> > +\ttimestamp_t masked_date;\n> > +\n> > +\tif (sizeof(timestamp_t) > 4)\n> > +\t\tmasked_date = c->date & (((timestamp_t) 1 << 34) - 1);\n> > +\telse\n> > +\t\tmasked_date = c->date;\n> \n> It is a bit surprising that on a platform where timestamp_t is only\n> 32-bit wide, a smart-enough compiler would not find (1<<34) as\n> suspicious. \n\nIt probably would, but we don't test on any system where that's the case\nto the best of my knowledge. `timestamp_t` is defined as `uintmax_t`,\nand that should be 64 bit on almost all platforms. There's going to be\nexceptions though, even though I'm not sure whether such platforms even\nmatter to us. But I later realized that we had similar checks elsewhere,\nso I decided to adapt accordingly.\n\n> IOW, I would have expected this to be done not with runtime switch but\n> with conditional compilation.\n\nWell, it's written as a runtime switch, but I would expect all compilers\nto essentially turn this into conditional compilation. They should note\nthat the condition is always true (or false) on a given platform. And\nchecking Godbolt my assumption seems to hold. On x86-64 using GCC [1]:\n\n    compute_generation_offset(unsigned long, unsigned long):\n            movabs  rax, 17179869183\n            and     rdi, rax\n            mov     rax, rsi\n            sub     rax, rdi\n            ret\n\nThanks!\n\nPatrick\n\n[1]: https://godbolt.org/z/oa4xPohGb\n"},{"id":"540039","messageId":"CAOLa=ZRhLxBV+2ab8_yiENd_mYRZM3W5X-3_5xqnkoxPk+XCRA@mail.gmail.com","threadId":"65283","inReplyTo":"20260324-pks-commit-graph-overflow-v2-1-843568cf8780@pks.im","subject":"Re: [PATCH v2] commit-graph: fix writing generations with dates exceeding 34 bits","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-03-26T10:02:08Z","receivedAt":"2026-03-26T10:02:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":null},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The `timestamp_t` type is declared as `uintmax_t` and thus typically has\n> 64 bits of precision. Usually, the full precision of such dates is not\n> required: it would be comforting to know that Git is still around in\n> millions of years, but all in all the chance is rather low.\n>\n> We abuse this fact in the commit-graph: instead of storing the full 64\n> bits of precision, committer dates only store 34 bits. This is still\n> plenty of headroom, as it means that we can represent dates until year\n> 2514. Commits which are dated beyond that year will simply get a date\n> whose remaining bits are masked.\n>\n\nOkay so the structure in the commit graph is:\n\nbase + 8  => generation number (30 bits) + date high (2 bits)\nbase + 12 => date low (30 bits)\n\ndate = date_high << 32 | date_low;\n\nThis can be seen in `write_graph_chunk_data()`, so makes sense.\n\n> The result of this is somewhat curious: the committer date will be\n> different depending on whether a commit gets parsed via the commit-graph\n> or via the object database. This isn't really too much of an issue in\n> general though, as we don't typically use the date parsed from the\n> commit-graph in user-facing output.\n>\n> But with 024b4c9697 (commit: make `repo_parse_commit_no_graph()` more\n> robust, 2026-02-16) it started to become a problem when writing the\n> commit-graph itself. This commit changed `repo_parse_commit_no_graph()`\n> so that we re-parse the commit via the object database in case it was\n> already parsed beforehand via the commit-graph.\n>\n> The consequence is that we may now act with two different commit dates\n> at different stages:\n>\n>   - Initially, we use the 34-bit precision timestamp when writing the\n>     chunk generation data. We thus correctly compute the offsets\n>     relative to the on-disk timestamp here.\n>\n>   - Later, when writing the overflow data, we may end up with the\n>     full-precision timestamp. When the date is larger than 34 bits the\n>     result of this is an underflow when computing the offset.\n>\n> This causes a mismatch in the number of generation data overflow records\n> we want to write, and that ultimately causes Git to die.\n>\n> Introduce a new helper function that computes the generation offset for\n> a commit while correctly masking the date to 34 bits. This makes the\n> previously-implicit assumptions about the commit date precision explicit\n> and thus hopefully less fragile going forward.\n>\n> Adapt sites that compute the offset to use the function.\n>\n\nWell explained.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> Hi,\n>\n> this fixes a regression recently introduced by myself in 024b4c9697\n> (commit: make `repo_parse_commit_no_graph()` more robust, 2026-02-16).\n> The regression was found by GitLab's tests suite, see [1].\n>\n> Changes in v2:\n>   - Account for platforms where `timestamp_t` has 32 bit precision. This\n>     matches logic in `write_graph_chunk_data()`, where we also depend on\n>     the size of the commit timestamps.\n>   - Link to v1: https://lore.kernel.org/r/20260317-pks-commit-graph-overflow-v1-1-e6bee22cd826@pks.im\n>\n> Thanks!\n>\n> Patrick\n>\n> [1]: https://gitlab.com/gitlab-org/gitlab/-/jobs/13522328632\n> ---\n>  commit-graph.c          | 37 ++++++++++++++++++++++++++++++++++---\n>  t/t5318-commit-graph.sh | 20 ++++++++++++++++++++\n>  2 files changed, 54 insertions(+), 3 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f8e24145a5..cb514bfb60 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1319,6 +1319,37 @@ static int write_graph_chunk_data(struct hashfile *f,\n>  \treturn 0;\n>  }\n>\n> +/*\n> + * Compute the generation offset between the commit date and its generation.\n> + * This is what's ultimately stored as generation number in the commit graph.\n> + *\n> + * Note that the computation of the commit date is more involved than you might\n> + * think. Instead of using the full commit date, we're in fact masking bits so\n> + * that only the 34 lowest bits are considered. This results from the fact that\n> + * commit graphs themselves only ever store 34 bits of the commit date\n> + * themselves.\n> + *\n> + * This means that if we have a commit date that exceeds 34 bits we'll end up\n> + * in situations where depending on whether the commit has been parsed from the\n> + * object database or the commit graph we'll have different dates, where the\n> + * ones parsed from the object database would have full 64 bit precision.\n> + *\n> + * But ultimately, we only ever want the offset to be relative to what we\n> + * actually end up storing on disk, and hence we have to mask all the other\n> + * bits.\n> + */\n> +static timestamp_t compute_generation_offset(struct commit *c)\n> +{\n> +\ttimestamp_t masked_date;\n> +\n> +\tif (sizeof(timestamp_t) > 4)\n> +\t\tmasked_date = c->date & (((timestamp_t) 1 << 34) - 1);\n> +\telse\n> +\t\tmasked_date = c->date;\n> +\n> +\treturn commit_graph_data_at(c)->generation - masked_date;\n> +}\n> +\n\nLooks good.\n\n>  static int write_graph_chunk_generation_data(struct hashfile *f,\n>  \t\t\t\t\t     void *data)\n>  {\n> @@ -1329,7 +1360,7 @@ static int write_graph_chunk_generation_data(struct hashfile *f,\n>  \t\tstruct commit *c = ctx->commits.items[i];\n>  \t\ttimestamp_t offset;\n>  \t\trepo_parse_commit(ctx->r, c);\n> -\t\toffset = commit_graph_data_at(c)->generation - c->date;\n> +\t\toffset = compute_generation_offset(c);\n>  \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n>\n>  \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n> @@ -1350,7 +1381,7 @@ static int write_graph_chunk_generation_data_overflow(struct hashfile *f,\n>  \tint i;\n>  \tfor (i = 0; i < ctx->commits.nr; i++) {\n>  \t\tstruct commit *c = ctx->commits.items[i];\n> -\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\n> +\t\ttimestamp_t offset = compute_generation_offset(c);\n>  \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n>\n>  \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n> @@ -1741,7 +1772,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n>\n>  \tfor (i = 0; i < ctx->commits.nr; i++) {\n>  \t\tstruct commit *c = ctx->commits.items[i];\n> -\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\n> +\t\ttimestamp_t offset = compute_generation_offset(c);\n>  \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX)\n>  \t\t\tctx->num_generation_data_overflows++;\n>  \t}\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 98c6910963..1c40f904f8 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -417,6 +417,26 @@ test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'lower layers have overflow ch\n>  \ttest_cmp full/.git/objects/info/commit-graph commit-graph-upgraded\n>  '\n>\n> +test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'overflow chunk when replacing commit-graph' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tcat >commit <<-EOF &&\n> +\t\ttree $(test_oid empty_tree)\n> +\t\tauthor Example <committer@example.com> 9223372036854775 +0000\n> +\t\tcommitter Example <committer@example.com> 9223372036854775 +0000\n> +\n> +\t\tWeird commit date\n> +\t\tEOF\n> +\t\tcommit_id=$(git hash-object -t commit -w commit) &&\n> +\t\tgit reset --hard \"$commit_id\" &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\tgit commit-graph write --reachable --split=replace &&\n> +\t\tgit log\n> +\t)\n> +'\n\nSo we first write the commit graph once and then rewrite it. The second\nwrite must recompute the overflow chunk. Then we run 'git log' to\nexercise the commit graph.\n\nLooks good!\n\n> +\n>  # the verify tests below expect the commit-graph to contain\n>  # exactly the commits reachable from the commits/8 branch.\n>  # If the file changes the set of commits in the list, then the\n>\n> ---\n> base-commit: ca1db8a0f7dc0dbea892e99f5b37c5fe5861be71\n> change-id: 20260317-pks-commit-graph-overflow-09c3fb6e259a\n"}]}