Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object

Olga Telezhnaya [off-list ref] writes:

-static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-		      int deref, struct object **obj, struct strbuf *err)
+static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
+		      struct expand_data *oi, struct strbuf *err)
 {
-	int eaten;
-	int ret = 0;
-	unsigned long size;
-	enum object_type type;
-	void *buf = read_object_file(oid, &type, &size);
-	if (!buf)
-		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
-				      oid_to_hex(oid), ref->refname);
-	else {
-		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
-		if (!*obj)
-			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-					      oid_to_hex(oid), ref->refname);
-		else
-			grab_values(ref->value, deref, *obj, buf, size);
+	/* parse_object_buffer() will set eaten to 0 if free() will be needed */
+	int eaten = 1;

Hmph, doesn't this belong to the previous step?  In other words,
isn't the result of applying 1-3/4 has a bug that can leave eaten
uninitialized (and base decision to free(buf) later on it), and
isn't this change a fix for it?

+	if (oi->info.contentp) {
+		/* We need to know that to use parse_object_buffer properly */
+		oi->info.sizep = &oi->size;
+		oi->info.typep = &oi->type;
 	}
+	if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
+				     OBJECT_INFO_LOOKUP_REPLACE))
+		return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+				       oid_to_hex(&oi->oid), ref->refname);
+
+	if (oi->info.contentp) {
+		*obj = parse_object_buffer(&oi->oid, oi->type, oi->size, oi->content, &eaten);
+		if (!obj) {
+			if (!eaten)
+				free(oi->content);
+			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+					       oid_to_hex(&oi->oid), ref->refname);
+		}
+		grab_values(ref->value, deref, *obj, oi->content, oi->size);
+	}
+
+	grab_common_values(ref->value, deref, oi);
 	if (!eaten)
-		free(buf);
-	return ret;
+		free(oi->content);
+	return 0;
 }

2018-07-16 23:53 GMT+03:00 Junio C Hamano [off-list ref]:

Olga Telezhnaya [off-list ref] writes:

quoted

-static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-                   int deref, struct object **obj, struct strbuf *err)
+static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
+                   struct expand_data *oi, struct strbuf *err)
 {
-     int eaten;
-     int ret = 0;
-     unsigned long size;
-     enum object_type type;
-     void *buf = read_object_file(oid, &type, &size);
-     if (!buf)
-             ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
-                                   oid_to_hex(oid), ref->refname);
-     else {
-             *obj = parse_object_buffer(oid, type, size, buf, &eaten);
-             if (!*obj)
-                     ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-                                           oid_to_hex(oid), ref->refname);
-             else
-                     grab_values(ref->value, deref, *obj, buf, size);
+     /* parse_object_buffer() will set eaten to 0 if free() will be needed */
+     int eaten = 1;

Hmph, doesn't this belong to the previous step?  In other words,
isn't the result of applying 1-3/4 has a bug that can leave eaten
uninitialized (and base decision to free(buf) later on it), and
isn't this change a fix for it?

Oh. I was thinking that it was new bug created by me. Now I see that
previously we had the same problem. I guess it's better to make
separate commit to fix it. Hope I can do it in this patch (I don't
want to create separate patch because of so minor update).
Thank you! I will fix it soon.

quoted

+     if (oi->info.contentp) {
+             /* We need to know that to use parse_object_buffer properly */
+             oi->info.sizep = &oi->size;
+             oi->info.typep = &oi->type;
      }
+     if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
+                                  OBJECT_INFO_LOOKUP_REPLACE))
+             return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+                                    oid_to_hex(&oi->oid), ref->refname);
+
+     if (oi->info.contentp) {
+             *obj = parse_object_buffer(&oi->oid, oi->type, oi->size, oi->content, &eaten);
+             if (!obj) {
+                     if (!eaten)
+                             free(oi->content);
+                     return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+                                            oid_to_hex(&oi->oid), ref->refname);
+             }
+             grab_values(ref->value, deref, *obj, oi->content, oi->size);
+     }
+
+     grab_common_values(ref->value, deref, oi);
      if (!eaten)
-             free(buf);
-     return ret;
+             free(oi->content);
+     return 0;
 }