On Mon, Feb 26, 2018 at 06:25:24PM +0700, Duy Nguyen wrote:
> On Mon, Feb 26, 2018 at 4:11 AM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > @@ -44,7 +44,7 @@ void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)
> >                 for (i = 0; i < 3; i++) {
> >                         if (!ui->mode[i])
> >                                 continue;
> > -                       strbuf_add(sb, ui->sha1[i], 20);
> > +                       strbuf_add(sb, ui->oid[i].hash, the_hash_algo->rawsz);
> >                 }
> >         }
> >  }
> > @@ -89,7 +89,7 @@ struct string_list *resolve_undo_read(const char *data, unsigned long size)
> >                                 continue;
> >                         if (size < 20)
> >                                 goto error;
> > -                       hashcpy(ui->sha1[i], (const unsigned char *)data);
> > +                       hashcpy(ui->oid[i].hash, (const unsigned char *)data);
> >                         size -= 20;
> >                         data += 20;
> >                 }

It looks like I may have missed a conversion there.  I'll fix that in a
reroll.

> Here we see the same pattern again, but this time the @@ lines give
> better context: these are actually hash I/O. Maybe it's about time we
> add
> 
> int oidwrite(char *, size_t , const struct object_id *);
> // optionally, void strbuf_addoid(struct strbuf *, const struct object_id *);
> int oidread(struct object_id *, const char *, size_t);
> 
> for conversion from between an object_id in memory and on disk? It
> would probably be a straight memcpy for all hash algorithms so we
> don't really need new function pointers in git_hash_algo for this.

I don't have a strong opinion about adding those or not adding them; if
people think it makes the code cleaner to read, I'm happy to add them.
It would probably makes sense to make them inline if we do, so that the
compiler can optimize them best.

I think we do need to preserve hashcpy anyway for a handful of cases
(such as pack checksums and rerere) that aren't technically object IDs
and won't use those functions.  I encountered a similar experience with
get_sha1_hex recently: there are a handful of cases that want to read
the name of a pack or a rerere cache, which are not object IDs.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204