Re: Typesafer git hash patch

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Feb 28, 2017, 20:45 UTC
Message-ID: <20170228204528.glpmouvcx33zma65@genre.crustytoothpaste.net>
In-Reply-To: <CA+55aFzUhWinWqK30GBc1BKy-v6QtDdO2BLUODkiqg9XoKLrwA@mail.gmail.com>

On Tue, Feb 28, 2017 at 12:25:20PM -0800, Linus Torvalds wrote:

Show 19 quoted lines

> On Tue, Feb 28, 2017 at 11:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Sorry, but at this point in your description, you completely lost
> > me.  I thought "struct object_id" was what you call "hash_t" in the
> > above.
> 
> So what happened was that I started out just encapsulating
> 
>    unsigned char sha1[20];
> 
> as a
> 
>    hash_t hash;
> 
> and that made sense in a lot of situations. I always thought that code that used
> 
>     struct object_id oid;
> 
> is just too ugly to live, so I'm not actually all that big of a fan of
> the oid approach.

There was some discussion on the list about the best name to use, and object_id seemed like the most popular decision. I don't care if we add a typedef for it and prefer that typedef, but the existing code avoided typedefs in favor of explicit struct definitions.

I'm certainly not opposed to having less to type, because “object_id” is awkward to type, but I've generally tried to defer to existing uses in the codebase and what list regulars are comfortable with.

The only problem with using hash_t is that it's then not obvious as we transition (assuming we don't take an omnibus patch) what is converted and what isn't.

> But the two approaches really are pretty much equivalent logically,
> even if they don't look the same.

Yeah, I think they are.

Show 13 quoted lines

> So I wanted to unify things: "One type to bring them all and in the
> darkness bind them".
> 
> So I just basically made this:
> 
>     typedef struct object_id {
>             unsigned char hash[GIT_HASH_SIZE];
>     } hash_t;
> 
> to create one single data structure that doesn't make my eyes bleed.
> That "struct object_id" still exists, but I don't generally have to
> look at it when doing the conversion, and any current users "just
> work".

There is nothing that prevents us from doing a nice global search-and-replace in the future if we think the status quo is bad. That's something that could be automated with Coccinelle.

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