All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <derrickstolee@github.com>,
	Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH] commit-graph: fix truncated generation numbers
Date: Tue, 28 Mar 2023 10:45:05 -0700	[thread overview]
Message-ID: <xmqqfs9og47y.fsf@gitster.g> (raw)
In-Reply-To: <f8a0a869e8b0882f05cac49d78f49ba3553d3c44.1679904401.git.ps@pks.im> (Patrick Steinhardt's message of "Mon, 27 Mar 2023 10:08:25 +0200")

Patrick Steinhardt <ps@pks.im> writes:

> In 80c928d947 (commit-graph: simplify compute_generation_numbers(),
> 2023-03-20), the code to compute generation numbers was simplified to
> use the same infrastructure as is used to compute topological levels.
> This refactoring introduced a bug where the generation numbers are
> truncated when they exceed UINT32_MAX because we explicitly cast the
> computed generation number to `uint32_t`. This is not required though:
> both the computed value and the field of `struct commit_graph_data` are
> of the same type `timestamp_t` already, so casting to `uint32_t` will
> cause truncation.
>
> This cast can cause us to miscompute generation data overflows:
>
>     1. Given a commit with no parents and committer date
>        `UINT32_MAX + 1`.
>
>     2. We compute its generation number as `UINT32_MAX + 1`, but
>        truncate it to `1`.
>
>     3. We calculate the generation offset via `$generation - $date`,
>        which is thus `1 - (UINT32_MAX + 1)`. The computation underflows
>        and we thus end up with an offset that is bigger than the maximum
>        allowed offset.
>
> As a result, we'd be writing generation data overflow information into
> the commit-graph that is bogus and ultimately not even required.
>
> Fix this bug by removing the needless cast.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> This commit applies on top of cbfe360b14 (commit-reach: add
> tips_reachable_from_bases(), 2023-03-20), which has recently been merged
> to next.

The patch is clearly explained and the change looks quite
straight-forward.  Derrick, Ack?

Thanks.

  reply	other threads:[~2023-03-28 17:45 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-27  8:08 [PATCH] commit-graph: fix truncated generation numbers Patrick Steinhardt
2023-03-28 17:45 ` Junio C Hamano [this message]
2023-03-28 18:32   ` Derrick Stolee
2023-03-28 18:39 ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqfs9og47y.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.