On Wed, Dec 05, 2018 at 07:41:44PM +0100, René Scharfe wrote: > > If we start to convert those, there's a > > little bit of a rabbit hole, but it's actually not too bad. > > You don't need to crawl in just for quick_has_loose(), but eventually > everything has to be converted. It seems a bit much for one patch, but > perhaps that's just my ever-decreasing attention span speaking. Yeah, my normal process here is to dig to the bottom of the rabbit hole, and then break it into actual patches. I just shared the middle state here. ;) I suspect the http bits could be split off into their own thing. The bits in sha1-file.c I'd plan to mostly do all together, as they are a family of related functions. Mostly I wasn't sure how to wrap this up with the other changes. It's obviously pretty invasive, and I don't want it to get in the way of actual functional changes we've been discussing. > > @@ -879,15 +879,15 @@ int git_open_cloexec(const char *name, int flags) > > * Note that it may point to static storage and is only valid until another > > * call to stat_sha1_file(). > > */ > > -static int stat_sha1_file(struct repository *r, const unsigned char *sha1, > > - struct stat *st, const char **path) > > +static int stat_loose_object(struct repository *r, const struct object_id *oid, > > + struct stat *st, const char **path) > > Hmm, read_sha1_file() was renamed to read_object_file() earlier this > year, and I'd have expected this to become stat_object_file(). Names.. read_object_file() is about reading an object from _any_ source. These are specifically about loose objects, and I think that distinction is important (both here and for map_loose_object, etc). I'd actually argue that read_object_file() should just be read_object(), but that already exists. Sigh. (I think it's fixable, but obviously orthogonal to this topic). > Anyway, the comment above and one a few lines below should be updated > as well. Thanks, fixed. > > static int open_sha1_file(struct repository *r, > > - const unsigned char *sha1, const char **path) > > + const struct object_id *oid, const char **path) > > That function should lose the "sha1" in its name as well. Yep, fixed. > > -static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1) > > +static void *unpack_loose_rest(git_zstream *stream, > > + void *buffer, unsigned long size, > > + const struct object_id *oid) > > Hmm, both old and new name here look weird to me at this point. It makes more sense in the pairing of unpack_sha1_header() and unpack_sha1_rest(). Maybe "body" would be better than "rest". At any rate, it probably makes sense to rename them together (but I didn't touch the "header" one here). Maybe the name changes should come as a separate patch. I was mostly changing them here because I was changing the signatures anyway, and had to touch all of the callers. -Peff
