Re: [PATCH v2 05/36] resolve-undo: convert struct resolve_undo_info to object_id

[Duy Nguyen]

On Tue, Feb 27, 2018 at 9:01 AM, brian m. carlson
 wrote:
> 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
>>  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(, ..., 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

Re: [PATCH v2 05/36] resolve-undo: convert struct resolve_undo_info to object_id

[brian m. carlson]

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
>  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


signature.asc
Description: PGP signature

Re: [PATCH v2 05/36] resolve-undo: convert struct resolve_undo_info to object_id

[Duy Nguyen]

On Mon, Feb 26, 2018 at 4:11 AM, brian m. carlson
 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;
> }

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.
-- 
Duy

[PATCH v2 05/36] resolve-undo: convert struct resolve_undo_info to object_id

[brian m. carlson]

Convert the sha1 member of this struct to be an array of struct
object_id instead.  This change is needed to convert find_unique_abbrev.

Signed-off-by: brian m. carlson 
---
 builtin/ls-files.c | 2 +-
 resolve-undo.c | 8 
 resolve-undo.h | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 2fc836e330..9df66ba307 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -271,7 +271,7 @@ static void show_ru_info(const struct index_state *istate)
if (!ui->mode[i])
continue;
printf("%s%06o %s %d\t", tag_resolve_undo, ui->mode[i],
-  find_unique_abbrev(ui->sha1[i], abbrev),
+  find_unique_abbrev(ui->oid[i].hash, abbrev),
   i + 1);
write_name(path);
}
diff --git a/resolve-undo.c b/resolve-undo.c
index b40f3173d3..109c658a85 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -24,7 +24,7 @@ void record_resolve_undo(struct index_state *istate, struct 
cache_entry *ce)
if (!lost->util)
lost->util = xcalloc(1, sizeof(*ui));
ui = lost->util;
-   hashcpy(ui->sha1[stage - 1], ce->oid.hash);
+   oidcpy(>oid[stage - 1], >oid);
ui->mode[stage - 1] = ce->ce_mode;
 }
 
@@ -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;
}
@@ -145,7 +145,7 @@ int unmerge_index_entry_at(struct index_state *istate, int 
pos)
struct cache_entry *nce;
if (!ru->mode[i])
continue;
-   nce = make_cache_entry(ru->mode[i], ru->sha1[i],
+   nce = make_cache_entry(ru->mode[i], ru->oid[i].hash,
   name, i + 1, 0);
if (matched)
nce->ce_flags |= CE_MATCHED;
diff --git a/resolve-undo.h b/resolve-undo.h
index 46306455ed..87291904bd 100644
--- a/resolve-undo.h
+++ b/resolve-undo.h
@@ -3,7 +3,7 @@
 
 struct resolve_undo_info {
unsigned int mode[3];
-   unsigned char sha1[3][20];
+   struct object_id oid[3];
 };
 
 extern void record_resolve_undo(struct index_state *, struct cache_entry *);