git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
brian m. carlson <sandals@crustytoothpaste.net>
Date
Jun 24, 2016, 15:27 UTC
Message-ID
<20160624152742.GA10808@vauxhall.crustytoothpaste.net>
In-Reply-To
<xmqqmvmenpwz.fsf@gitster.mtv.corp.google.com>
On Tue, Jun 21, 2016 at 03:22:04PM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> "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:
> 
> > @@ -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.

Show 8 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?
Probably so.  I can add that in a reroll.
Show 10 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?
I can certainly do that in a reroll.
Show 15 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.

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.

Show 19 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?
Sure.  I can do that.
Show 13 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.
That's one of the following patches.
Show 8 quoted lines
> 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
Previous: Junio C HamanoNext: brian m. carlson
Message 11 of 20 in “object_id part 4”
  1. 0/8 object_id part 4brian m. carlson, Jun 18, 2016
  2. 6/8 merge-recursive: convert struct merge_file_info to object_idbrian m. carlson, Jun 18, 2016
  3. 4/8 Rename struct diff_filespec's sha1_valid member.brian m. carlson, Jun 18, 2016
  4. 2/8 Apply object_id Coccinelle transformations.brian m. carlson, Jun 18, 2016
  5. Junio C HamanoJun 21, 2016
  6. 8/8 merge-recursive: convert merge_recursive_generic to object_idbrian m. carlson, Jun 18, 2016
  7. 7/8 merge-recursive: convert leaf functions to use struct object_idbrian m. carlson, Jun 18, 2016
  8. 5/8 merge-recursive: convert struct stage_data to use object_idbrian m. carlson, Jun 18, 2016
  9. 3/8 Convert struct diff_filespec to struct object_idbrian m. carlson, Jun 18, 2016
  10. Junio C HamanoJun 21, 2016
  11. brian m. carlsonJun 24, 2016
  12. 1/8 Add basic Coccinelle transforms.brian m. carlson, Jun 18, 2016
  13. Johannes SixtJun 19, 2016
  14. Jeff KingJun 19, 2016
  15. brian m. carlsonJun 19, 2016
  16. Johannes SchindelinJun 20, 2016
  17. Jeff KingJun 20, 2016
  18. Junio C HamanoJun 20, 2016
  19. Junio C HamanoJun 21, 2016
  20. brian m. carlsonJun 22, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.