Re: [PATCH v2 3/8] Convert struct diff_filespec to struct object_id
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 21, 2016, 22:22 UTC
- Message-ID
- <xmqqmvmenpwz.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160618221407.1046188-4-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
I was trying to make sure there is no misconversion, but some lines that got wrapped were distracting. For example:
Show 7 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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?
Show 6 quoted lines
> @@ -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?
Show 8 quoted lines
> @@ -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.
Show 8 quoted lines
> @@ -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?
Show 10 quoted lines
> @@ -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.
> diff --git a/notes-merge.c b/notes-merge.c > index 34bfac0c..62c23d8a 100644 > --- a/notes-merge.c > +++ b/notes-merge.c