Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Jul 09, 2024 at 07:49:50PM +0200, Omar Polo wrote:
> > this will make my work for sha256 easier.  (i.e. will let gotd compile
> > with other changes I'm doing, pretty sure it will still explode at
> > runtime)
> > 
> > IMHO here it makes sense to pass around an id instead of buffer with the
> > hash, if only since it saves some lines ;)
> > 
> > ok?
> 
> Yes, just one remark:
> 
> > blob - c21d1c02dd99282f345f37e8dc6e2cb64c1e65b5
> > file + gotd/session_write.c
> > --- gotd/session_write.c
> > +++ gotd/session_write.c
> > @@ -523,23 +523,23 @@ forward_notification(struct gotd_session_client *clien
> >  		goto done;
> >  	}
> >  	if (notif->action == GOTD_NOTIF_ACTION_CREATED) {
> > -		if (memcmp(notif->new_id.sha1, icontent.new_id,
> > -		    SHA1_DIGEST_LENGTH) != 0) {
> > +		if (memcmp(&notif->new_id, &icontent.new_id,
> > +		    sizeof(notif->new_id)) != 0) {
> 
> In the future, we will need to ensure that all struct object_id fields get
> initialized even in the (short) sha1 case. Otherwise this comparison of
> sizeof() amount of bytes can go wrong.

That's the hard part in implementing sha256 support :)

there some bits where we can bikeshed, for example how to properly deal
with different digest sizes in pack files, but the majority of the
effort is dealing with all the code where we assume there's only a sha1
field of exactly SHA1_DIGEST_LENGTH bytes.  There still lot of code (in
privsep mostly) where we just pass around uint8_t buffers.  I'm slowly
getting all of them, and also spotting the places where got_object_id is
not properly initialized.


Thanks,

Omar Polo