Re: [PATCH v2 3/8] Convert struct diff_filespec to struct object_id

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

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.

This suggests that oid_to_hex_r() is needed, perhaps?

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?

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.

oidclr()?

Perhaps a preparatory step of

	unsigned char *E1;

	-hashcpy(E1, null_sha1);
        +hashclr(E1)

would help?

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.

On Tue, Jun 21, 2016 at 03:22:04PM -0700, Junio C Hamano wrote:

"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

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

quoted

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

I do tend to agree.  Coccinelle wrapped the line automatically, but I'm
not sure why it did that.  I can see if there's an option that disables
that if you'd like.  I'm a bit reticent to manually fix up the line
wrapping as I want others to be able to run Coccinelle themselves and
get identical output.

quoted

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

Probably so.  I can add that in a reroll.

quoted

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

I can certainly do that in a reroll.

quoted

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

Yes, I'd noticed that as well.  One thing I observed from these
conversions is that it sometimes takes a huge amount of work to get all
the callers to change.  Often, it's only one or two call sites that end
up being a bunch of work.

quoted

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

Sure.  I can do that.

quoted

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

That's one of the following patches.

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 think that's accurate.  I'll make that change.
-- 
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