On Wed, Jan 08, 2025 at 02:21:47PM -0500, Taylor Blau wrote: > On Tue, Jan 07, 2025 at 01:06:20PM +0100, Patrick Steinhardt wrote: > > > > > I think we should perhaps combine forces here. My ideal end-state is to > > > > > have the unsafe_hash_algo() stuff land from my earlier series, then have > > > > > these two fixes (adjusted to the new world order as above), and finally > > > > > the Meson fixes after that. > > > > > > > > > > Does that seem like a plan to you? If so, I can put everything together > > > > > and send it out (if you're OK with me forging your s-o-b). > > > > > > > > I think the ideal state would be if the hashing function used was stored > > > > as part of `struct git_hash_ctx`. So the flow basically becomes for > > > > example: > > > > > > > > ``` > > > > struct git_hash_ctx ctx; > > > > struct object_id oid; > > > > > > > > git_hash_sha1_init(&ctx); > > > > git_hash_update(&ctx, data); > > > > git_hash_final_oid(&oid, &ctx); > > > > ``` > > > > > > > > Note how the intermediate calls don't need to know which hash function > > > > you used to initialize the `struct git_hash_ctx` -- the structure itself > > > > should remember what it has been initilized with and do the right thing. > > > > > > I'm not sure I'm following you here. In the stream_blob() function > > > within fast-import, the problem isn't that we're switching hash > > > functions mid-stream, but that we're initializing the hashfile_context > > > structure with the wrong hash function to begin with. > > > > True, but it would have been a non-issue if the hash context itself knew > > which hash function to use for updates. Sure, we would've used the slow > > variant of SHA1 instead of the fast-but-unsafe one. But that feels like > > the lesser evil compared to crashing. > > For posterity, Patrick and I used some of our monthly meeting this morning to > spend some time together pairing on this idea. > > It ended up being a dead-end, since this approach only protects you > against changing the hash function mid-stream, and not using the > incorrect context type from the union. > > That was along the lines of what I was originally thinking, and so I > resurrected my series to introduce 'unsafe_hash_algo()' here: > > https://lore.kernel.org/git/cover.1736363652.git.me@xxxxxxxxxxxx/ > > I got the impression that Patrick and I are on the same page there as > that being a good path forward, but I'll let him chime in in case I > misunderstood anything. No misunderstanding, we're both on the same page. Thanks! Patrick
