Re: [PATCH v2 3/8] Convert struct diff_filespec to struct object_id
From: Junio C Hamano <hidden>
Date: 2016-06-21 22:22:12
"brian m. carlson" [off-list ref] writes: I was trying to make sure there is no misconversion, but some lines that got wrapped were distracting. For example:
quoted hunk
@@ -2721,7 +2722,8 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only) if (s->dirty_submodule) dirty = "-dirty"; - strbuf_addf(&buf, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), dirty); + strbuf_addf(&buf, "Subproject commit %s%s\n", + oid_to_hex(&s->oid), dirty);
This would have been
- strbuf_addf(&buf, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), dirty); + strbuf_addf(&buf, "Subproject commit %s%s\n", oid_to_hex(&s->oid), dirty);
which the conversion made the line _shorter_. If the original's line length was acceptable, there is no reason to wrap the result.
- die("unable to read %s", sha1_to_hex(s->sha1));
+ die("unable to read %s",
+ oid_to_hex(&s->oid));Likewise.
quoted hunk
@@ -2937,7 +2940,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name, if (!one->sha1_valid) sha1_to_hex_r(temp->hex, null_sha1); else - sha1_to_hex_r(temp->hex, one->sha1); + sha1_to_hex_r(temp->hex, one->oid.hash);
This suggests that oid_to_hex_r() is needed, perhaps?
quoted hunk
@@ -2952,7 +2955,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name, if (diff_populate_filespec(one, 0)) die("cannot read data blob for %s", one->path); prep_temp_blob(name, temp, one->data, one->size, - one->sha1, one->mode); + one->oid.hash, one->mode);
prep_temp_blob() is a file-scope static with only two callers. It probably would not take too much effort to make it take &one->oid instead?
quoted hunk
@@ -3075,8 +3078,8 @@ static void fill_metainfo(struct strbuf *msg, abbrev = 40; } strbuf_addf(msg, "%s%sindex %s..", line_prefix, set, - find_unique_abbrev(one->sha1, abbrev)); - strbuf_addstr(msg, find_unique_abbrev(two->sha1, abbrev)); + find_unique_abbrev(one->oid.hash, abbrev)); + strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
Good. As more and more callers of find_unique_abbrev() is converted to pass oid.hash to it, eventually we will have only a handful of callers that have a raw "const unsigned char sha1[20]" to pass it, and we can eventually make the function take &oid. It seems we are not quite there yet, by the looks of 'git grep find_unique_abbrev' output, but we are getting closer.
quoted hunk
@@ -3134,17 +3137,17 @@ static void diff_fill_sha1_info(struct diff_filespec *one) if (!one->sha1_valid) { struct stat st; if (one->is_stdin) { - hashcpy(one->sha1, null_sha1); + hashcpy(one->oid.hash, null_sha1); return; }
oidclr()?
Perhaps a preparatory step of
unsigned char *E1;
-hashcpy(E1, null_sha1);
+hashclr(E1)
would help?
quoted hunk
@@ -902,13 +904,13 @@ static struct merge_file_info merge_file_1(struct merge_options *o, result.clean = 0; if (S_ISREG(a->mode)) { result.mode = a->mode; - hashcpy(result.sha, a->sha1); + hashcpy(result.sha, a->oid.hash); } else { result.mode = b->mode; - hashcpy(result.sha, b->sha1); + hashcpy(result.sha, b->oid.hash);
merge_file_info is a file-scope-static type, and it shouldn't take too much effort to replace its sha1 with an oid, I would think.
- if (!sha_eq(a->sha1, one->sha1) && !sha_eq(b->sha1, one->sha1)) + if (!sha_eq(a->oid.hash, one->oid.hash) && !sha_eq(b->oid.hash, one->oid.hash))
sha_eq() knows that either of its two parameters could be NULL, but a->sha1 is diff_filespec.sha1 which cannot be NULL. So !sha_eq() here can become oidcmp(). There are some calls to sha_eq() that could pass NULL (e.g. a_sha could be NULL in blob_unchanged()), but many other sha_eq() can become !oidcmp(). Am I reading the conversion correctly? I'll stop here for now and will come back to the remainder of this patch sometime later. Thanks.
quoted hunk
diff --git a/notes-merge.c b/notes-merge.c index 34bfac0c..62c23d8a 100644 --- a/notes-merge.c +++ b/notes-merge.c