Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object
Оля Тележная writes: >> 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. The original said something like: int eaten; void *buf = get_obj(..., ); ... if (!eaten) free(buf); and get_obj() left eaten untouched when it returned NULL. As a random uninitialized cruft in eaten that happened to be "true" would just cause free(NULL) on many archs, there was no practical problem in such a code, but it is undefined behaviour nevertheless. And the previous step made it a bit more alarming ;-)
Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object
2018-07-16 23:53 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya 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, , );
>> - 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, );
>> - 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.
>
>> + if (oi->info.contentp) {
>> + /* We need to know that to use parse_object_buffer properly */
>> + oi->info.sizep = >size;
>> + oi->info.typep = >type;
>> }
>> + if (oid_object_info_extended(the_repository, >oid, >info,
>> + OBJECT_INFO_LOOKUP_REPLACE))
>> + return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>> +oid_to_hex(>oid), ref->refname);
>> +
>> + if (oi->info.contentp) {
>> + *obj = parse_object_buffer(>oid, oi->type, oi->size,
>> oi->content, );
>> + if (!obj) {
>> + if (!eaten)
>> + free(oi->content);
>> + return strbuf_addf_ret(err, -1, _("parse_object_buffer
>> failed on %s for %s"),
>> +oid_to_hex(>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;
>> }
>
Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object
Olga Telezhnaya 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, , );
> - 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, );
> - 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 = >size;
> + oi->info.typep = >type;
> }
> + if (oid_object_info_extended(the_repository, >oid, >info,
> + OBJECT_INFO_LOOKUP_REPLACE))
> + return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
> +oid_to_hex(>oid), ref->refname);
> +
> + if (oi->info.contentp) {
> + *obj = parse_object_buffer(>oid, oi->type, oi->size,
> oi->content, );
> + if (!obj) {
> + if (!eaten)
> + free(oi->content);
> + return strbuf_addf_ret(err, -1, _("parse_object_buffer
> failed on %s for %s"),
> +oid_to_hex(>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;
> }
[PATCH v2 4/4] ref-filter: use oid_object_info() to get object
Use oid_object_info_extended() to get object info instead of
read_object_file().
It will help to handle some requests faster (e.g., we do not need to
parse whole object if we need to know %(objectsize)).
It could also help us to add new atoms such as %(objectsize:disk)
and %(deltabase).
Signed-off-by: Olga Telezhnaia
---
ref-filter.c | 123 ++-
1 file changed, 89 insertions(+), 34 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index f04169f0ea0e3..112955f006648 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -61,6 +61,17 @@ struct refname_atom {
int lstrip, rstrip;
};
+static struct expand_data {
+ struct object_id oid;
+ enum object_type type;
+ unsigned long size;
+ off_t disk_size;
+ struct object_id delta_base_oid;
+ void *content;
+
+ struct object_info info;
+} oi, oi_deref;
+
/*
* An atom is a valid field atom listed below, possibly prefixed with
* a "*" to denote deref_tag().
@@ -202,6 +213,30 @@ static int remote_ref_atom_parser(const struct ref_format
*format, struct used_a
return 0;
}
+static int objecttype_atom_parser(const struct ref_format *format, struct
used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
+ if (arg)
+ return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take
arguments"));
+ if (*atom->name == '*')
+ oi_deref.info.typep = _deref.type;
+ else
+ oi.info.typep =
+ return 0;
+}
+
+static int objectsize_atom_parser(const struct ref_format *format, struct
used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
+ if (arg)
+ return strbuf_addf_ret(err, -1, _("%%(objectsize) does not take
arguments"));
+ if (*atom->name == '*')
+ oi_deref.info.sizep = _deref.size;
+ else
+ oi.info.sizep =
+ return 0;
+}
+
static int body_atom_parser(const struct ref_format *format, struct used_atom
*atom,
const char *arg, struct strbuf *err)
{
@@ -388,8 +423,8 @@ static struct {
const char *arg, struct strbuf *err);
} valid_atom[] = {
{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
- { "objecttype", SOURCE_OTHER },
- { "objectsize", SOURCE_OTHER, FIELD_ULONG },
+ { "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+ { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
{ "tree", SOURCE_OBJ },
{ "parent", SOURCE_OBJ },
@@ -502,6 +537,12 @@ static int parse_ref_filter_atom(const struct ref_format
*format,
used_atom[at].name = xmemdupz(atom, ep - atom);
used_atom[at].type = valid_atom[i].cmp_type;
used_atom[at].source = valid_atom[i].source;
+ if (used_atom[at].source == SOURCE_OBJ) {
+ if (*atom == '*')
+ oi_deref.info.contentp = _deref.content;
+ else
+ oi.info.contentp =
+ }
if (arg) {
arg = used_atom[at].name + (arg - atom) + 1;
if (!*arg) {
@@ -817,7 +858,7 @@ static int grab_objectname(const char *name, const struct
object_id *oid,
}
/* See grab_values */
-static void grab_common_values(struct atom_value *val, int deref, struct
object *obj, void *buf, unsigned long sz)
+static void grab_common_values(struct atom_value *val, int deref, struct
expand_data *oi)
{
int i;
@@ -829,13 +870,13 @@ static void grab_common_values(struct atom_value *val,
int deref, struct object
if (deref)
name++;
if (!strcmp(name, "objecttype"))
- v->s = type_name(obj->type);
+ v->s = type_name(oi->type);
else if (!strcmp(name, "objectsize")) {
- v->value = sz;
- v->s = xstrfmt("%lu", sz);
+ v->value = oi->size;
+ v->s = xstrfmt("%lu", oi->size);
}
else if (deref)
- grab_objectname(name, >oid, v, _atom[i]);
+ grab_objectname(name, >oid, v, _atom[i]);
}
}
@@ -1194,7 +1235,6 @@ static void fill_missing_values(struct atom_value *val)
*/
static void grab_values(struct atom_value *val, int deref, struct object *obj,
void *buf, unsigned long sz)
{
- grab_common_values(val, deref, obj, buf, sz);
switch (obj->type) {
case OBJ_TAG:
grab_tag_values(val, deref, obj, buf, sz);
@@ -1418,28 +1458,36 @@ static const char *get_refname(struct used_atom *atom,
struct ref_array_item *re
return show_ref(>u.refname, ref->refname);
}
-static int get_object(struct ref_array_item
