| Message ID | 20210410152140.3525040-10-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SHA-256 / SHA-1 interop, part 1 | expand |
On Sat, Apr 10 2021, brian m. carlson wrote: > const struct object_id null_oid; > static const struct object_id empty_tree_oid = { > - EMPTY_TREE_SHA1_BIN_LITERAL > + EMPTY_TREE_SHA1_BIN_LITERAL, > + GIT_HASH_SHA1, > }; > static const struct object_id empty_blob_oid = { > - EMPTY_BLOB_SHA1_BIN_LITERAL > + EMPTY_BLOB_SHA1_BIN_LITERAL, > + GIT_HASH_SHA1, > }; > static const struct object_id empty_tree_oid_sha256 = { > - EMPTY_TREE_SHA256_BIN_LITERAL > + EMPTY_TREE_SHA256_BIN_LITERAL, > + GIT_HASH_SHA256, > }; > static const struct object_id empty_blob_oid_sha256 = { > - EMPTY_BLOB_SHA256_BIN_LITERAL > + EMPTY_BLOB_SHA256_BIN_LITERAL, > + GIT_HASH_SHA256, > }; In this and some other patches we're continuing to add new fields to structs without using designated initializers. Not a new problem at all, just a note that if you re-roll I for one would very much appreciate starting by migrating over to that. It makes for much easier reading in subsequent patches in this series, and in future ones.
On 2021-04-11 at 11:57:30, Ævar Arnfjörð Bjarmason wrote: > > On Sat, Apr 10 2021, brian m. carlson wrote: > > > const struct object_id null_oid; > > static const struct object_id empty_tree_oid = { > > - EMPTY_TREE_SHA1_BIN_LITERAL > > + EMPTY_TREE_SHA1_BIN_LITERAL, > > + GIT_HASH_SHA1, > > }; > > static const struct object_id empty_blob_oid = { > > - EMPTY_BLOB_SHA1_BIN_LITERAL > > + EMPTY_BLOB_SHA1_BIN_LITERAL, > > + GIT_HASH_SHA1, > > }; > > static const struct object_id empty_tree_oid_sha256 = { > > - EMPTY_TREE_SHA256_BIN_LITERAL > > + EMPTY_TREE_SHA256_BIN_LITERAL, > > + GIT_HASH_SHA256, > > }; > > static const struct object_id empty_blob_oid_sha256 = { > > - EMPTY_BLOB_SHA256_BIN_LITERAL > > + EMPTY_BLOB_SHA256_BIN_LITERAL, > > + GIT_HASH_SHA256, > > }; > > In this and some other patches we're continuing to add new fields to > structs without using designated initializers. > > Not a new problem at all, just a note that if you re-roll I for one > would very much appreciate starting by migrating over to that. It makes > for much easier reading in subsequent patches in this series, and in > future ones. I'm happy to do that. I thought we were not allowed to use C99 features because only recent versions of MSVC support modern C. I was previously under the impression that MSVC didn't support anything but C89, but they now support C11 and C17 in their latest release[0], much to my surprise. If we're willing to require C99 features, then I'm happy to add those. I'll also send a follow-up series to require C99 support, which I think is overdue considering the standard is 22 years old. [0] https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/
On Sun, Apr 11 2021, brian m. carlson wrote: > On 2021-04-11 at 11:57:30, Ævar Arnfjörð Bjarmason wrote: >> >> On Sat, Apr 10 2021, brian m. carlson wrote: >> >> > const struct object_id null_oid; >> > static const struct object_id empty_tree_oid = { >> > - EMPTY_TREE_SHA1_BIN_LITERAL >> > + EMPTY_TREE_SHA1_BIN_LITERAL, >> > + GIT_HASH_SHA1, >> > }; >> > static const struct object_id empty_blob_oid = { >> > - EMPTY_BLOB_SHA1_BIN_LITERAL >> > + EMPTY_BLOB_SHA1_BIN_LITERAL, >> > + GIT_HASH_SHA1, >> > }; >> > static const struct object_id empty_tree_oid_sha256 = { >> > - EMPTY_TREE_SHA256_BIN_LITERAL >> > + EMPTY_TREE_SHA256_BIN_LITERAL, >> > + GIT_HASH_SHA256, >> > }; >> > static const struct object_id empty_blob_oid_sha256 = { >> > - EMPTY_BLOB_SHA256_BIN_LITERAL >> > + EMPTY_BLOB_SHA256_BIN_LITERAL, >> > + GIT_HASH_SHA256, >> > }; >> >> In this and some other patches we're continuing to add new fields to >> structs without using designated initializers. >> >> Not a new problem at all, just a note that if you re-roll I for one >> would very much appreciate starting by migrating over to that. It makes >> for much easier reading in subsequent patches in this series, and in >> future ones. > > I'm happy to do that. I thought we were not allowed to use C99 features > because only recent versions of MSVC support modern C. I was previously > under the impression that MSVC didn't support anything but C89, but they > now support C11 and C17 in their latest release[0], much to my surprise. > > If we're willing to require C99 features, then I'm happy to add those. > I'll also send a follow-up series to require C99 support, which I think > is overdue considering the standard is 22 years old. > > [0] https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/ I don't think we can in general require C99, e.g. I found just the other day that our CI's MSVC will fail on %zu (to print size_t without %lu & a cast). But we can use some subset of C99 features, and happily designated initializers is one of those, see cbc0f81d96f (strbuf: use designated initializers in STRBUF_INIT, 2017-07-10). It's been used all over the place since then. See e.g.: git grep -P '^\s+\.\S+ = ' -- '*.[ch]'
On 2021-04-11 at 22:12:38, Ævar Arnfjörð Bjarmason wrote: > > On Sun, Apr 11 2021, brian m. carlson wrote: > > > On 2021-04-11 at 11:57:30, Ævar Arnfjörð Bjarmason wrote: > >> > >> In this and some other patches we're continuing to add new fields to > >> structs without using designated initializers. > >> > >> Not a new problem at all, just a note that if you re-roll I for one > >> would very much appreciate starting by migrating over to that. It makes > >> for much easier reading in subsequent patches in this series, and in > >> future ones. > > > > I'm happy to do that. I thought we were not allowed to use C99 features > > because only recent versions of MSVC support modern C. I was previously > > under the impression that MSVC didn't support anything but C89, but they > > now support C11 and C17 in their latest release[0], much to my surprise. > > > > If we're willing to require C99 features, then I'm happy to add those. > > I'll also send a follow-up series to require C99 support, which I think > > is overdue considering the standard is 22 years old. > > > > [0] https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/ > > I don't think we can in general require C99, e.g. I found just the other > day that our CI's MSVC will fail on %zu (to print size_t without %lu & a > cast). That's a shame. I think I'd like to try, though, and ask people to upgrade MSVC to a suitable version if we're going to continue to support it. It's not like there aren't alternatives. So I'm going to send out that series anyway, I think. That's independent of this series, though, so I'll add the designated initializers in v2.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > But we can use some subset of C99 features, and happily designated > initializers is one of those, see cbc0f81d96f (strbuf: use designated > initializers in STRBUF_INIT, 2017-07-10). It's been used all over the > place since then. Good advice to cite a commit that on purpose used a feature and documented that it is allowed. Also see Documentation/CodingGuidelines ;-) The document should give the authoritative blessing for features allowed to be used (add any missing with a proposed patch). Thanks.
On Mon, Apr 12 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> But we can use some subset of C99 features, and happily designated >> initializers is one of those, see cbc0f81d96f (strbuf: use designated >> initializers in STRBUF_INIT, 2017-07-10). It's been used all over the >> place since then. > > Good advice to cite a commit that on purpose used a feature and > documented that it is allowed. > > Also see Documentation/CodingGuidelines ;-) The document should > give the authoritative blessing for features allowed to be used (add > any missing with a proposed patch). Our E-Mails probably crossed, my initial motivation for just-submitted http://lore.kernel.org/git/cover-0.2-00000000000-20210412T105422Z-avarab@gmail.com was going to CodingGuidelines, and vaguely remembering that there was some other C99 thing that wasn't listed there, and then (re-)discovering the recent variadic macro commit from Jeff King. As noted there maybe 2/2 of that is too aggressive, but in that case it would make sense to have a V2 of that which just carved off the CodingGuidelines change.
diff --git a/hash.h b/hash.h index 04eba5c56b..3b114f053e 100644 --- a/hash.h +++ b/hash.h @@ -237,6 +237,7 @@ static inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src) static inline void oidcpy(struct object_id *dst, const struct object_id *src) { memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ); + dst->algo = src->algo; } static inline struct object_id *oiddup(const struct object_id *src) @@ -254,6 +255,7 @@ static inline void hashclr(unsigned char *hash) static inline void oidclr(struct object_id *oid) { memset(oid->hash, 0, GIT_MAX_RAWSZ); + oid->algo = hash_algo_by_ptr(the_hash_algo); } static inline void oidread(struct object_id *oid, const unsigned char *hash) @@ -261,6 +263,7 @@ static inline void oidread(struct object_id *oid, const unsigned char *hash) size_t rawsz = the_hash_algo->rawsz; memcpy(oid->hash, hash, rawsz); memset(oid->hash + rawsz, 0, GIT_MAX_RAWSZ - rawsz); + oid->algo = hash_algo_by_ptr(the_hash_algo); } static inline int is_empty_blob_sha1(const unsigned char *sha1) @@ -286,6 +289,7 @@ static inline int is_empty_tree_oid(const struct object_id *oid) static inline void oid_pad_buffer(struct object_id *oid, const struct git_hash_algo *algop) { memset(oid->hash + algop->rawsz, 0, GIT_MAX_RAWSZ - algop->rawsz); + oid->algo = hash_algo_by_ptr(algop); } const char *empty_tree_oid_hex(void); diff --git a/object-file.c b/object-file.c index 8e338247cc..5f1fa05c4e 100644 --- a/object-file.c +++ b/object-file.c @@ -57,16 +57,20 @@ const struct object_id null_oid; static const struct object_id empty_tree_oid = { - EMPTY_TREE_SHA1_BIN_LITERAL + EMPTY_TREE_SHA1_BIN_LITERAL, + GIT_HASH_SHA1, }; static const struct object_id empty_blob_oid = { - EMPTY_BLOB_SHA1_BIN_LITERAL + EMPTY_BLOB_SHA1_BIN_LITERAL, + GIT_HASH_SHA1, }; static const struct object_id empty_tree_oid_sha256 = { - EMPTY_TREE_SHA256_BIN_LITERAL + EMPTY_TREE_SHA256_BIN_LITERAL, + GIT_HASH_SHA256, }; static const struct object_id empty_blob_oid_sha256 = { - EMPTY_BLOB_SHA256_BIN_LITERAL + EMPTY_BLOB_SHA256_BIN_LITERAL, + GIT_HASH_SHA256, }; static void git_hash_sha1_init(git_hash_ctx *ctx) @@ -93,6 +97,7 @@ static void git_hash_sha1_final_oid(struct object_id *oid, git_hash_ctx *ctx) { git_SHA1_Final(oid->hash, &ctx->sha1); memset(oid->hash + GIT_SHA1_RAWSZ, 0, GIT_MAX_RAWSZ - GIT_SHA1_RAWSZ); + oid->algo = GIT_HASH_SHA1; } @@ -124,6 +129,7 @@ static void git_hash_sha256_final_oid(struct object_id *oid, git_hash_ctx *ctx) * but keep it in case we extend the hash size again. */ memset(oid->hash + GIT_SHA256_RAWSZ, 0, GIT_MAX_RAWSZ - GIT_SHA256_RAWSZ); + oid->algo = GIT_HASH_SHA256; } static void git_hash_unknown_init(git_hash_ctx *ctx)
Now that struct object_id has an algorithm field, we should populate it. This will allow us to handle object IDs in any supported algorithm and distinguish between them. Ensure that the field is written whenever we write an object ID by storing it explicitly every time we write an object. Set values for the empty blob and tree values as well. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- hash.h | 4 ++++ object-file.c | 14 ++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-)