Re: [PATCH 11/53] fast-import: convert to struct object_id

[brian m. carlson]

On Mon, Apr 24, 2017 at 10:20:27AM -0700, Stefan Beller wrote:
> On Sun, Apr 23, 2017 at 2:34 PM, brian m. carlson
>  wrote:
> > Signed-off-by: brian m. carlson 
> > ---
> 
> > @@ -2823,12 +2821,10 @@ static void parse_new_commit(const char *arg)
> > strbuf_addf(_data, "tree %s\n",
> > oid_to_hex(>branch_tree.versions[1].oid));
> > if (!is_null_oid(>oid))
> > -   strbuf_addf(_data, "parent %s\n",
> > -   oid_to_hex(>oid));
> > +   strbuf_addf(_data, "parent %s\n", oid_to_hex(>oid));
> > while (merge_list) {
> > struct hash_list *next = merge_list->next;
> > -   strbuf_addf(_data, "parent %s\n",
> > -   oid_to_hex(_list->oid));
> > +   strbuf_addf(_data, "parent %s\n", 
> > oid_to_hex(_list->oid));
> > free(merge_list);
> > merge_list = next;
> > }
> 
> This is a funny one. The only change is line rewrapping, as it fits
> into 80 cols easily.
> I was reviewing this series using colored --word-diff output, and this hunk 
> does
> not produce any red or green. I wonder if this is the intended
> behavior of the word diffing
> or if we rather want to insert a  - \n - .

I used Coccinelle for part of this, I think, so that would be why.
Sometimes it doesn't do everything I want, but it lets me do less manual
work.  I'll revert that change in the reroll.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 11/53] fast-import: convert to struct object_id

[Stefan Beller]

On Sun, Apr 23, 2017 at 2:34 PM, brian m. carlson
 wrote:
> Convert the remaining parts of fast-import.c to use struct object_id.
> Convert several instances of get_sha1_hex to parse_oid_hex to avoid
> needing to specify constants.  Convert other hardcoded values to named
> constants.  Finally, use the is_empty_tree_oid function instead of a
> direct comparison against a fixed string.
>
> Note that the odd computation with GIT_MAX_HEXSZ is due to the insertion
> of a slash between every two hex digits in the path, plus one for the
> terminating NUL.

Up to and including this patch are
Reviewed-by: Stefan Beller 

I may continue reviewing this series once my eyes are refreshed.

>
> Signed-off-by: brian m. carlson 
> ---

> @@ -2823,12 +2821,10 @@ static void parse_new_commit(const char *arg)
> strbuf_addf(_data, "tree %s\n",
> oid_to_hex(>branch_tree.versions[1].oid));
> if (!is_null_oid(>oid))
> -   strbuf_addf(_data, "parent %s\n",
> -   oid_to_hex(>oid));
> +   strbuf_addf(_data, "parent %s\n", oid_to_hex(>oid));
> while (merge_list) {
> struct hash_list *next = merge_list->next;
> -   strbuf_addf(_data, "parent %s\n",
> -   oid_to_hex(_list->oid));
> +   strbuf_addf(_data, "parent %s\n", 
> oid_to_hex(_list->oid));
> free(merge_list);
> merge_list = next;
> }

This is a funny one. The only change is line rewrapping, as it fits
into 80 cols easily.
I was reviewing this series using colored --word-diff output, and this hunk does
not produce any red or green. I wonder if this is the intended
behavior of the word diffing
or if we rather want to insert a  - \n - .

Thanks,
Stefan

[PATCH 11/53] fast-import: convert to struct object_id

[brian m. carlson]

Convert the remaining parts of fast-import.c to use struct object_id.
Convert several instances of get_sha1_hex to parse_oid_hex to avoid
needing to specify constants.  Convert other hardcoded values to named
constants.  Finally, use the is_empty_tree_oid function instead of a
direct comparison against a fixed string.

Note that the odd computation with GIT_MAX_HEXSZ is due to the insertion
of a slash between every two hex digits in the path, plus one for the
terminating NUL.

Signed-off-by: brian m. carlson 
---
 fast-import.c | 327 +-
 1 file changed, 161 insertions(+), 166 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 052c59d82..6b0117e51 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -391,10 +391,8 @@ static void write_branch_report(FILE *rpt, struct branch 
*b)
fputc('\n', rpt);
 
fprintf(rpt, "  tip commit  : %s\n", oid_to_hex(>oid));
-   fprintf(rpt, "  old tree: %s\n",
-   oid_to_hex(>branch_tree.versions[0].oid));
-   fprintf(rpt, "  cur tree: %s\n",
-   oid_to_hex(>branch_tree.versions[1].oid));
+   fprintf(rpt, "  old tree: %s\n", 
oid_to_hex(>branch_tree.versions[0].oid));
+   fprintf(rpt, "  cur tree: %s\n", 
oid_to_hex(>branch_tree.versions[1].oid));
fprintf(rpt, "  commit clock: %" PRIuMAX "\n", b->last_commit);
 
fputs("  last pack   : ", rpt);
@@ -557,7 +555,7 @@ static void alloc_objects(unsigned int cnt)
alloc_count += cnt;
 }
 
-static struct object_entry *new_object(unsigned char *sha1)
+static struct object_entry *new_object(struct object_id *oid)
 {
struct object_entry *e;
 
@@ -565,32 +563,32 @@ static struct object_entry *new_object(unsigned char 
*sha1)
alloc_objects(object_entry_alloc);
 
e = blocks->next_free++;
-   hashcpy(e->idx.sha1, sha1);
+   hashcpy(e->idx.sha1, oid->hash);
return e;
 }
 
-static struct object_entry *find_object(unsigned char *sha1)
+static struct object_entry *find_object(struct object_id *oid)
 {
-   unsigned int h = sha1[0] << 8 | sha1[1];
+   unsigned int h = oid->hash[0] << 8 | oid->hash[1];
struct object_entry *e;
for (e = object_table[h]; e; e = e->next)
-   if (!hashcmp(sha1, e->idx.sha1))
+   if (!hashcmp(oid->hash, e->idx.sha1))
return e;
return NULL;
 }
 
-static struct object_entry *insert_object(unsigned char *sha1)
+static struct object_entry *insert_object(struct object_id *oid)
 {
-   unsigned int h = sha1[0] << 8 | sha1[1];
+   unsigned int h = oid->hash[0] << 8 | oid->hash[1];
struct object_entry *e = object_table[h];
 
while (e) {
-   if (!hashcmp(sha1, e->idx.sha1))
+   if (!hashcmp(oid->hash, e->idx.sha1))
return e;
e = e->next;
}
 
-   e = new_object(sha1);
+   e = new_object(oid);
e->next = object_table[h];
e->idx.offset = 0;
object_table[h] = e;
@@ -1007,17 +1005,17 @@ static void end_packfile(void)
clear_delta_base_cache();
if (object_count) {
struct packed_git *new_p;
-   unsigned char cur_pack_sha1[20];
+   struct object_id cur_pack_oid;
char *idx_name;
int i;
struct branch *b;
struct tag *t;
 
close_pack_windows(pack_data);
-   sha1close(pack_file, cur_pack_sha1, 0);
+   sha1close(pack_file, cur_pack_oid.hash, 0);
fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,
pack_data->pack_name, object_count,
-   cur_pack_sha1, pack_size);
+   cur_pack_oid.hash, pack_size);
 
if (object_count <= unpack_limit) {
if (!loosen_small_pack(pack_data)) {
@@ -1083,13 +1081,13 @@ static int store_object(
enum object_type type,
struct strbuf *dat,
struct last_object *last,
-   unsigned char *sha1out,
+   struct object_id *oidout,
uintmax_t mark)
 {
void *out, *delta;
struct object_entry *e;
unsigned char hdr[96];
-   unsigned char sha1[20];
+   struct object_id oid;
unsigned long hdrlen, deltalen;
git_SHA_CTX c;
git_zstream s;
@@ -1099,17 +1097,17 @@ static int store_object(
git_SHA1_Init();
git_SHA1_Update(, hdr, hdrlen);
git_SHA1_Update(, dat->buf, dat->len);
-   git_SHA1_Final(sha1, );
-   if (sha1out)
-   hashcpy(sha1out, sha1);
+   git_SHA1_Final(oid.hash, );
+   if (oidout)
+   oidcpy(oidout, );
 
-   e = insert_object(sha1);
+   e = insert_object();
if (mark)
insert_mark(mark, e);