Re: [PATCH v4 5/8] sha1_file: refactor read_object

[Junio C Hamano]

Jonathan Tan  writes:

> @@ -2914,6 +2912,7 @@ static int sha1_loose_object_info(const unsigned char 
> *sha1,
>   git_zstream stream;
>   char hdr[32];
>   struct strbuf hdrbuf = STRBUF_INIT;
> + unsigned long size_scratch;
>  
>   if (oi->delta_base_sha1)
>   hashclr(oi->delta_base_sha1);
> @@ -2939,6 +2938,10 @@ static int sha1_loose_object_info(const unsigned char 
> *sha1,
>   map = map_sha1_file(sha1, );
>   if (!map)
>   return -1;
> +
> + if (!oi->sizep)
> + oi->sizep = _scratch;
> +
>   if (oi->disk_sizep)
>   *oi->disk_sizep = mapsize;
>   if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) {
>   if (status && oi->typep)
>   *oi->typep = status;
> + if (oi->sizep == _scratch)
> + oi->sizep = NULL;

This looked somewhat unusual but nevertheless is correct.  Because
of the way parse_sha1_header_extended() interacts with its callers,
the usual fn(oi->sizep ? oi->sizep : ) pattern does not apply
to this codepath.

> @@ -3077,28 +3090,15 @@ int pretend_sha1_file(void *buf, unsigned long len, 
> enum object_type type,
>  static void *read_object(const unsigned char *sha1, enum object_type *type,
>unsigned long *size)
>  {
> - unsigned long mapsize;
> - void *map, *buf;
> - struct cached_object *co;
> -
> - co = find_cached_object(sha1);
> - if (co) {
> - *type = co->type;
> - *size = co->size;
> - return xmemdupz(co->buf, co->size);
> - }
> + struct object_info oi = OBJECT_INFO_INIT;
> + void *content;
> + oi.typep = type;
> + oi.sizep = size;
> + oi.contentp = 
>  
> - buf = read_packed_sha1(sha1, type, size);
> - if (buf)
> - return buf;
> - map = map_sha1_file(sha1, );
> - if (map) {
> - buf = unpack_sha1_file(map, mapsize, type, size, sha1);
> - munmap(map, mapsize);
> - return buf;
> - }
> - reprepare_packed_git();
> - return read_packed_sha1(sha1, type, size);
> + if (sha1_object_info_extended(sha1, , 0))
> + return NULL;
> + return content;
>  }

Nice code reduction; it is somewhat funny to think that a function
meant to gather 'object info' does so much, but we can always say
the contents is part of the information about the object ;-).

Same comment as the other one applies here; the definition of how an
error is reported by sha1_object_info_extended() should be kept
consistent with existing callers.

Thanks.

[PATCH v4 5/8] sha1_file: refactor read_object

[Jonathan Tan]

read_object() and sha1_object_info_extended() both implement mechanisms
such as object replacement, retrying the packed store after failing to
find the object in the packed store then the loose store, and being able
to mark a packed object as bad and then retrying the whole process.
Consolidating these mechanisms would be a great help to maintainability.

Therefore, consolidate them by extending sha1_object_info_extended() to
support the functionality needed, and then modifying read_object() to
use sha1_object_info_extended().

Signed-off-by: Jonathan Tan 
---
 cache.h |  1 +
 sha1_file.c | 84 ++---
 2 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/cache.h b/cache.h
index a3631b237..48aea923b 100644
--- a/cache.h
+++ b/cache.h
@@ -1827,6 +1827,7 @@ struct object_info {
off_t *disk_sizep;
unsigned char *delta_base_sha1;
struct strbuf *typename;
+   void **contentp;
 
/* Response */
enum {
diff --git a/sha1_file.c b/sha1_file.c
index a7be45efe..4d5033c48 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2005,19 +2005,6 @@ int parse_sha1_header(const char *hdr, unsigned long 
*sizep)
return parse_sha1_header_extended(hdr, , 0);
 }
 
-static void *unpack_sha1_file(void *map, unsigned long mapsize, enum 
object_type *type, unsigned long *size, const unsigned char *sha1)
-{
-   int ret;
-   git_zstream stream;
-   char hdr[8192];
-
-   ret = unpack_sha1_header(, map, mapsize, hdr, sizeof(hdr));
-   if (ret < Z_OK || (*type = parse_sha1_header(hdr, size)) < 0)
-   return NULL;
-
-   return unpack_sha1_rest(, hdr, *size, sha1);
-}
-
 unsigned long get_size_from_delta(struct packed_git *p,
  struct pack_window **w_curs,
  off_t curpos)
@@ -2326,8 +2313,10 @@ static void *cache_or_unpack_entry(struct packed_git *p, 
off_t base_offset,
if (!ent)
return unpack_entry(p, base_offset, type, base_size);
 
-   *type = ent->type;
-   *base_size = ent->size;
+   if (type)
+   *type = ent->type;
+   if (base_size)
+   *base_size = ent->size;
return xmemdupz(ent->data, ent->size);
 }
 
@@ -2388,9 +2377,16 @@ int packed_object_info(struct packed_git *p, off_t 
obj_offset,
 * We always get the representation type, but only convert it to
 * a "real" type later if the caller is interested.
 */
-   type = unpack_object_header(p, _curs, , );
+   if (oi->contentp) {
+   *oi->contentp = cache_or_unpack_entry(p, obj_offset, oi->sizep,
+ );
+   if (!*oi->contentp)
+   type = OBJ_BAD;
+   } else {
+   type = unpack_object_header(p, _curs, , );
+   }
 
-   if (oi->sizep) {
+   if (!oi->contentp && oi->sizep) {
if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
off_t tmp_pos = curpos;
off_t base_offset = get_delta_base(p, _curs, _pos,
@@ -2679,8 +2675,10 @@ void *unpack_entry(struct packed_git *p, off_t 
obj_offset,
free(external_base);
}
 
-   *final_type = type;
-   *final_size = size;
+   if (final_type)
+   *final_type = type;
+   if (final_size)
+   *final_size = size;
 
unuse_pack(_curs);
 
@@ -2914,6 +2912,7 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
git_zstream stream;
char hdr[32];
struct strbuf hdrbuf = STRBUF_INIT;
+   unsigned long size_scratch;
 
if (oi->delta_base_sha1)
hashclr(oi->delta_base_sha1);
@@ -2926,7 +2925,7 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
 * return value implicitly indicates whether the
 * object even exists.
 */
-   if (!oi->typep && !oi->typename && !oi->sizep) {
+   if (!oi->typep && !oi->typename && !oi->sizep && !oi->contentp) {
const char *path;
struct stat st;
if (stat_sha1_file(sha1, , ) < 0)
@@ -2939,6 +2938,10 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
map = map_sha1_file(sha1, );
if (!map)
return -1;
+
+   if (!oi->sizep)
+   oi->sizep = _scratch;
+
if (oi->disk_sizep)
*oi->disk_sizep = mapsize;
if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) {
@@ -2956,10 +2959,18 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
   sha1_to_hex(sha1));
} else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
status = error("unable to parse %s header", sha1_to_hex(sha1));
-   git_inflate_end();
+
+   if (status >= 0 && oi->contentp)
+