"brian m. carlson" <sand...@crustytoothpaste.net> writes:
I was trying to make sure there is no misconversion, but some lines
that got wrapped were distracting. For example:
> @@ -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.
> @@ -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?
> @@ -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?
> @@ -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.
> @@ -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?
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
