Re: [RFC PATCH 2/2] hash API: add and use a hash_short_id_by_algo() function

From: Junio C Hamano <gitster@pobox.com>
Date: May 19, 2022, 15:50 UTC
Message-ID: <xmqqbkvtr079.fsf@gitster.g>
In-Reply-To: <RFC-patch-2.2-051f0612ab9-20220519T113538Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add and use a hash_short_id_by_algo() function. As noted in the
> comment being modified here (added in [1]) the intention wasn't to
> have these end up in on-disk formats, but since [2], [3] and [3]

double [3]?

Show 10 quoted lines

> that's been the case, and there's an outstanding patch to add another
> format that uses these[5].
>
> So let's expose this functionality as a documented utility function,
> instead of copy/pasting this code in various places.
>
> Replacing the die() in the existing functions with a BUG() might be
> overzelous, it's correct for the case of
> e.g. write_commit_graph_file() and write_midx_header(), but we also
> use this for parsing on-disk files, e.g. in parse_commit_graph().

If we know the offending data can come from outside, not from literals in the code, then there is no "might be", such a use of BUG() is simply wrong.

> We could add a "gently" version of this, but for now I think that
> worrying about the distinction would be worrying too much. If we ever
> end up parsing such files that'll almost certainly be a bug in our own
> writing code, so the distinction would be rather academic, even though

It would be a file written by an ancient buggy version of our code, or a buggy third-party reimplementation of Git. It could be that a new version of Git is using a yet-to-be-invented algorithm this version of Git does not know about.

The distinction matters in that "The version of Git I downloaded and built last week out of the latest release tag said BUG" should mean only one thing: that version that reports BUG() is the culprit, not some random other thing we do not even know where it came from that left a corrupt data on disk.