From: "Martin Ågren" <email@example.com> To: Derrick Stolee <firstname.lastname@example.org> Cc: "brian m. carlson" <email@example.com>, Git Mailing List <firstname.lastname@example.org>, Junio C Hamano <email@example.com>, Taylor Blau <firstname.lastname@example.org>, Abhishek Kumar <email@example.com> Subject: Re: [PATCH 5/5] commit-graph-format.txt: fix "Hash Version" description Date: Fri, 14 Aug 2020 16:10:35 +0200 [thread overview] Message-ID: <CAN0heSp024=Kyy7gdQ2VSetk_5iVhj_qdT8CMVPcry_AwWrhHQ@mail.gmail.com> (raw) In-Reply-To: <firstname.lastname@example.org> On Fri, 14 Aug 2020 at 14:37, Derrick Stolee <email@example.com> wrote: > > On 8/14/2020 8:21 AM, Martin Ågren wrote: > > We say that value 1 means "SHA-1", but in fact, it means "whatever > > the_hash_algo is", see commit c166599862 ("commit-graph: convert to > > using the_hash_algo", 2018-11-14). > > > > Signed-off-by: Martin Ågren <firstname.lastname@example.org> > > --- > > If we want to be more fine-grained in the future, we'll need to say, > > e.g., "2 means SHA-1, 3 means SHA-256" or, perhaps preferrably, bump the > > version number. > > > > I wonder: Should we instead say "1 means SHA-1, 2 means SHA-256"? It > > could be implemented as "easily" as "if (value_from_header != > > value_from_the_hash_algo) die(...);" for now. Might that pay off in the > > long run? > > > > This relates to Stolee's "in a vacuum" comment  ... so maybe we're > > fine. > > I think that was the intention of the byte, but that is not what ended > up happening. When I wrote this patch, I did go with "fix <thing>" rather than "document SHA-256". For the other patches, it's sort of obvious how those formats are so old that it's no wonder they assumed SHA-1. But here, we did go to some trouble to try and future-proof things and already had NewHash in mind. So that calls for "fix <thing>". But I'm more and more starting to think that it's the implementation that should be fixed and that the docs should just be extended to add "2 means SHA-256". > If we want that to be the case, then we should do that > work as part of the 2.29 cycle before we release with the ability to > create SHA-256 repos (which will lock the commit-graph format for these > repos). Part of my reasoning behind  was that in exactly a situation like this, we'd be able to say With extensions.objectFormat=sha256, Git 2.29-2.30 will barf at the commit-graph files that Git 2.31+ generate, and the other way around. Users will be able to remove "old" files and regenerate them, and shouldn't use a mixed environment. and know that those users knew this might happen. But certainly, if we can avoid it altogether by changing behavior already in 2.29, that would be better.  https://email@example.com/ > (By "we" I mean that I would try to do this work in a way that minimizes > conflicts with the current commit-graph work in flight  .) None of those seems to touch `oid_version()`, so if we can just tweak that function to return 1 or 2 instead of always 1, I guess that's one way. >  https://firstname.lastname@example.org/ > >  https://email@example.com/ > > > > > - 1-byte Hash Version (1 = SHA-1) > > - We infer the hash length (H) from this value. > > + 1-byte Hash Version (1 = SHA-1 in SHA-1 repo, SHA-256 in SHA-256 repo) > > + We infer the hash length (H) from the hash algo derived from this value. > > If we are _not_ changing the format to have a meaningful value in > this byte, then this documentation should be updated to state that > this byte must always have value 1, as it does not provide any > information. We could still go 1 means whatever extensions.objectFormat says, 2 means SHA-1, 3 means SHA-256, ... But maybe that would just be crazy. Thanks for all your comments Martin
next prev parent reply other threads:[~2020-08-14 14:10 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-13 22:48 [PATCH 0/2] Documentation updates for SHA-256 brian m. carlson 2020-08-13 22:49 ` [PATCH 1/2] docs: document SHA-256 pack and indices brian m. carlson 2020-08-14 2:26 ` Derrick Stolee 2020-08-13 22:49 ` [PATCH 2/2] docs: fix step in transition plan brian m. carlson 2020-08-14 12:21 ` Martin Ågren 2020-08-14 2:33 ` [PATCH 0/2] Documentation updates for SHA-256 Derrick Stolee 2020-08-14 4:47 ` Junio C Hamano 2020-08-14 20:20 ` brian m. carlson 2020-08-14 20:25 ` Junio C Hamano 2020-08-14 12:21 ` [PATCH 0/5] more SHA-256 documentation Martin Ågren 2020-08-14 12:21 ` [PATCH 1/5] http-protocol.txt: document SHA-256 "want"/"have" format Martin Ågren 2020-08-14 17:28 ` Junio C Hamano 2020-08-14 20:23 ` brian m. carlson 2020-08-14 20:32 ` Martin Ågren 2020-08-14 20:39 ` Junio C Hamano 2020-08-14 20:47 ` Junio C Hamano 2020-08-14 12:21 ` [PATCH 2/5] index-format.txt: document SHA-256 index format Martin Ågren 2020-08-14 12:28 ` Derrick Stolee 2020-08-14 14:05 ` Martin Ågren 2020-08-14 12:21 ` [PATCH 3/5] protocol-capabilities.txt: clarify "allow-x-sha1-in-want" re SHA-256 Martin Ågren 2020-08-14 12:31 ` Derrick Stolee 2020-08-14 14:05 ` Martin Ågren 2020-08-14 17:33 ` Junio C Hamano 2020-08-14 20:35 ` Martin Ågren 2020-08-14 20:43 ` Junio C Hamano 2020-08-14 12:21 ` [PATCH 4/5] shallow.txt: document SHA-256 shallow format Martin Ågren 2020-08-14 12:21 ` [PATCH 5/5] commit-graph-format.txt: fix "Hash Version" description Martin Ågren 2020-08-14 12:37 ` Derrick Stolee 2020-08-14 14:10 ` Martin Ågren [this message] 2020-08-14 20:28 ` [PATCH 0/5] more SHA-256 documentation brian m. carlson 2020-08-15 16:05 ` [PATCH v2 0/4] " Martin Ågren 2020-08-15 16:05 ` [PATCH v2 1/4] http-protocol.txt: document SHA-256 "want"/"have" format Martin Ågren 2020-08-15 16:06 ` [PATCH v2 2/4] index-format.txt: document SHA-256 index format Martin Ågren 2020-08-15 16:06 ` [PATCH v2 3/4] protocol-capabilities.txt: clarify "allow-x-sha1-in-want" re SHA-256 Martin Ågren 2020-08-15 16:06 ` [PATCH v2 4/4] shallow.txt: document SHA-256 shallow format Martin Ågren
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 List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAN0heSp024=Kyy7gdQ2VSetk_5iVhj_qdT8CMVPcry_AwWrhHQ@mail.gmail.com' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 5/5] commit-graph-format.txt: fix "Hash Version" description' \ /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
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).