| Message ID | 7419e4ac7053ab2d89a4cdc4612e5baeca48ce9f.1670532905.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Don't lazy-fetch commits when parsing them | expand |
On Thu, Dec 08, 2022 at 12:57:06PM -0800, Jonathan Tan wrote: > This function can do 3 things: > 1. Gets an fd given a path > 2. Simultaneously gets a path and fd given an OID > 3. Memory maps an fd > > Split this function up. Only one caller needs 1, so inline that. As for > 2, a future patch will also need this functionality and, in addition, > the calculated path, so extract this into a separate function with an > out parameter for the path. This is moving in a good direction. I like the name "map_fd" for the helper. Being able to give it a useful name like that is a good clue that it is doing a more focused and understandable job than the generic map_loose_object_1(). :) In fact... > +static void *map_loose_object_1(struct repository *r, > + const struct object_id *oid, > + unsigned long *size, > + const char **path) > +{ > + const char *p; > + int fd = open_loose_object(r, oid, &p); > + > + if (fd < 0) > + return NULL; > + if (path) > + *path = p; > + return map_fd(fd, p, size); > +} > + > void *map_loose_object(struct repository *r, > const struct object_id *oid, > unsigned long *size) > { > - return map_loose_object_1(r, NULL, oid, size); > + return map_loose_object_1(r, oid, size, NULL); > } If you take my suggestion on patch 3, then the only other caller of map_loose_object_1() goes away, and we can fold it all into one reasonably-named function: diff --git a/object-file.c b/object-file.c index d99d05839f..429e3a746d 100644 --- a/object-file.c +++ b/object-file.c @@ -1233,28 +1233,18 @@ static void *map_fd(int fd, const char *path, unsigned long *size) return map; } -static void *map_loose_object_1(struct repository *r, - const struct object_id *oid, - unsigned long *size, - const char **path) +void *map_loose_object(struct repository *r, + const struct object_id *oid, + unsigned long *size) { const char *p; int fd = open_loose_object(r, oid, &p); if (fd < 0) return NULL; - if (path) - *path = p; return map_fd(fd, p, size); } -void *map_loose_object(struct repository *r, - const struct object_id *oid, - unsigned long *size) -{ - return map_loose_object_1(r, oid, size, NULL); -} - enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, -Peff
Jeff King <peff@peff.net> writes: > If you take my suggestion on patch 3, then the only other caller of > map_loose_object_1() goes away, and we can fold it all into one > reasonably-named function: Ah, that is true as of this patch, but patch 3 introduces another caller of this function. I tried to allude to it in the commit message, but if there is a clearer way to explain that, please let me know.
On Fri, Dec 09, 2022 at 10:17:04AM -0800, Jonathan Tan wrote: > Jeff King <peff@peff.net> writes: > > If you take my suggestion on patch 3, then the only other caller of > > map_loose_object_1() goes away, and we can fold it all into one > > reasonably-named function: > > Ah, that is true as of this patch, but patch 3 introduces another caller > of this function. I tried to allude to it in the commit message, but if > there is a clearer way to explain that, please let me know. Yes, that's the "other caller" I was referring to. :) Hopefully it is more clear after you read my comments on v3. -Peff
On Fri, Dec 09, 2022 at 03:27:24PM -0500, Jeff King wrote: > On Fri, Dec 09, 2022 at 10:17:04AM -0800, Jonathan Tan wrote: > > > Jeff King <peff@peff.net> writes: > > > If you take my suggestion on patch 3, then the only other caller of > > > map_loose_object_1() goes away, and we can fold it all into one > > > reasonably-named function: > > > > Ah, that is true as of this patch, but patch 3 introduces another caller > > of this function. I tried to allude to it in the commit message, but if > > there is a clearer way to explain that, please let me know. > > Yes, that's the "other caller" I was referring to. :) Hopefully it is > more clear after you read my comments on v3. Er, that should be "...comments on patch 3", of course, not "v3". -Peff
diff --git a/object-file.c b/object-file.c index cf724bc19b..d99d05839f 100644 --- a/object-file.c +++ b/object-file.c @@ -1211,43 +1211,48 @@ static int quick_has_loose(struct repository *r, } /* - * Map the loose object at "path" if it is not NULL, or the path found by - * searching for a loose object named "oid". + * Map and close the given loose object fd. The path argument is used for + * error reporting. */ -static void *map_loose_object_1(struct repository *r, const char *path, - const struct object_id *oid, unsigned long *size) +static void *map_fd(int fd, const char *path, unsigned long *size) { - void *map; - int fd; - - if (path) - fd = git_open(path); - else - fd = open_loose_object(r, oid, &path); - map = NULL; - if (fd >= 0) { - struct stat st; + void *map = NULL; + struct stat st; - if (!fstat(fd, &st)) { - *size = xsize_t(st.st_size); - if (!*size) { - /* mmap() is forbidden on empty files */ - error(_("object file %s is empty"), path); - close(fd); - return NULL; - } - map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0); + if (!fstat(fd, &st)) { + *size = xsize_t(st.st_size); + if (!*size) { + /* mmap() is forbidden on empty files */ + error(_("object file %s is empty"), path); + close(fd); + return NULL; } - close(fd); + map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0); } + close(fd); return map; } +static void *map_loose_object_1(struct repository *r, + const struct object_id *oid, + unsigned long *size, + const char **path) +{ + const char *p; + int fd = open_loose_object(r, oid, &p); + + if (fd < 0) + return NULL; + if (path) + *path = p; + return map_fd(fd, p, size); +} + void *map_loose_object(struct repository *r, const struct object_id *oid, unsigned long *size) { - return map_loose_object_1(r, NULL, oid, size); + return map_loose_object_1(r, oid, size, NULL); } enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, @@ -2789,13 +2794,16 @@ int read_loose_object(const char *path, struct object_info *oi) { int ret = -1; + int fd; void *map = NULL; unsigned long mapsize; git_zstream stream; char hdr[MAX_HEADER_LEN]; unsigned long *size = oi->sizep; - map = map_loose_object_1(the_repository, path, NULL, &mapsize); + fd = git_open(path); + if (fd >= 0) + map = map_fd(fd, path, &mapsize); if (!map) { error_errno(_("unable to mmap %s"), path); goto out;
This function can do 3 things: 1. Gets an fd given a path 2. Simultaneously gets a path and fd given an OID 3. Memory maps an fd Split this function up. Only one caller needs 1, so inline that. As for 2, a future patch will also need this functionality and, in addition, the calculated path, so extract this into a separate function with an out parameter for the path. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- object-file.c | 60 +++++++++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 26 deletions(-)