Re: [PATCH v2 05/36] resolve-undo: convert struct resolve_undo_info to object_id
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Feb 28, 2018, 09:38 UTC
- Message-ID
- <CACsJy8DyrMqNYU7W_cB3U7EEfWUrzWOWx7zGa078EBE38u8Pdw@mail.gmail.com>
- In-Reply-To
- <20180227020111.GG4620@genre.crustytoothpaste.net>
On Tue, Feb 27, 2018 at 9:01 AM, brian m. carlson <sandals@crustytoothpaste.net> wrote:
Show 41 quoted lines
> 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.FWIW I'm totally ok with a memcpy(&oid.hash, ..., rawsz); here and not adding oidread/oidwrite. It's probably best to not adding them this early anyway. We can always grep memcpy.*rawsz and refactor later.
-- Duy