On Tue, Jun 21, 2016 at 03:22:04PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@xxxxxxxxxxxxxxxxxxxx> 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.
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.
> > @@ -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.
> > @@ -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.
> > @@ -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.
> > @@ -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.
> > @@ -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
Attachment:
signature.asc
Description: PGP signature