Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

[Оля Тележная]

2018-03-15 23:47 GMT+03:00 Martin Ågren :
> I skimmed the first four patches of this v2. It seems that patches 1 and
> 4 are identical to v2. Patches 2 and 3 have very straightforward changes
> based on my earlier comments. Let's see what this patch is about. :-)

Yes, you are right.

>
> On 14 March 2018 at 20:04, Olga Telezhnaya  wrote:
>> Finish removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>>
>> Change the signature of get_ref_atom_value() and underlying functions
>> by adding return value and strbuf parameter for error message.
>>
>> It's important to mention that grab_objectname() returned 1 if
>> it gets objectname atom and 0 otherwise. Now this logic changed:
>> we return 0 if we have no error, -1 otherwise. If someone needs to
>> know whether it's objectname atom or not, he/she could use
>> starts_with() function. It duplicates this checking but it does not
>> sound like a really big overhead.
>>
>> Signed-off-by: Olga Telezhnaia 
>> ---
>>  ref-filter.c | 109 
>> +--
>>  1 file changed, 69 insertions(+), 40 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 62ea4adcd0ff1..3f0c3924273d5 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, 
>> struct object **obj, unsigned
>>  }
>>
>>  static int grab_objectname(const char *name, const unsigned char *sha1,
>> -  struct atom_value *v, struct used_atom *atom)
>> +  struct atom_value *v, struct used_atom *atom,
>> +  struct strbuf *err)
>>  {
>> if (starts_with(name, "objectname")) {
>> if (atom->u.objectname.option == O_SHORT) {
>> v->s = xstrdup(find_unique_abbrev(sha1, 
>> DEFAULT_ABBREV));
>> -   return 1;
>> } else if (atom->u.objectname.option == O_FULL) {
>> v->s = xstrdup(sha1_to_hex(sha1));
>> -   return 1;
>> } else if (atom->u.objectname.option == O_LENGTH) {
>> v->s = xstrdup(find_unique_abbrev(sha1, 
>> atom->u.objectname.length));
>> -   return 1;
>> -   } else
>> -   die("BUG: unknown %%(objectname) option");
>> +   } else {
>> +   strbuf_addstr(err, "BUG: unknown %(objectname) 
>> option");
>> +   return -1;
>> +   }
>> }
>> return 0;
>>  }
>
> This is interesting. This die() is never ever supposed to actually
> trigger, except to allow a developer adding some new O_xxx-value to
> quickly notice that they have forgotten to add code here.

Oh, cool, so I can revert this change, OK.

>
>>  /* See grab_values */
>> -static void grab_common_values(struct atom_value *val, int deref, struct 
>> object *obj, void *buf, unsigned long sz)
>> +static int grab_common_values(struct atom_value *val, int deref, struct 
>> object *obj,
>> + void *buf, unsigned long sz, struct strbuf 
>> *err)
>>  {
>> int i;
>>
>> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, 
>> int deref, struct object
>> v->s = xstrfmt("%lu", sz);
>> }
>> else if (deref)
>> -   grab_objectname(name, obj->oid.hash, v, 
>> _atom[i]);
>> +   if (grab_objectname(name, obj->oid.hash, v, 
>> _atom[i], err))
>> +   return -1;
>> }
>> +   return 0;
>>  }
>
> So if that conversion I commented on above had not happened, this would
> not have been necessary. Let's read on...

Of course, I will check and also revert functions that were touched
only because of other functions.

>
>>  /* See grab_values */
>> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value 
>> *val)
>>   * pointed at by the ref itself; otherwise it is the object the
>>   * ref (which is a tag) refers to.
>>   */
>> -static void grab_values(struct atom_value *val, int deref, struct object 
>> *obj, void *buf, unsigned long sz)
>> +static int grab_values(struct atom_value *val, int deref, struct object 
>> *obj,
>> +  void *buf, unsigned long sz, struct strbuf *err)
>>  {
>> -   grab_common_values(val, deref, obj, buf, sz);
>> +   if (grab_common_values(val, deref, obj, buf, sz, err))
>> +   return -1;
>> switch (obj->type) {
>> case OBJ_TAG:
>> grab_tag_values(val, deref, obj, buf, sz);
>> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int 
>> deref, struct object *obj, v
>> /* grab_blob_values(val, deref, obj, buf, sz); */
>> break;
>> 

Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

[Оля Тележная]

2018-03-16 0:01 GMT+03:00 Eric Sunshine :
> On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren  wrote:
>> These are "real" errors and yield several more changes in the remainder.
>> Ignoring those BUG-type messages at the beginning of this patch would
>> give a patch like the one below.
>>
>> +static int get_object(struct ref_array_item *ref, const struct object_id 
>> *oid,
>> +  int deref, struct object **obj, struct strbuf *err)
>>  {
>> void *buf = get_obj(oid, obj, , );
>> -   if (!buf)
>> -   die(_("missing object %s for %s"),
>> -   oid_to_hex(oid), ref->refname);
>> -   if (!*obj)
>> -   die(_("parse_object_buffer failed on %s for %s"),
>> -   oid_to_hex(oid), ref->refname);
>> -
>> +   if (!buf) {
>> +   strbuf_addf(err, _("missing object %s for %s"), 
>> oid_to_hex(oid),
>> +   ref->refname);
>> +   return -1;
>> +   }
>> +   if (!*obj) {
>> +   strbuf_addf(err, _("parse_object_buffer failed on %s for 
>> %s"),
>> +   oid_to_hex(oid), ref->refname);
>> +   return -1;
>
> Doesn't this leak 'buf'?

Yes. Thanks a lot.
>
>> +   }
>> grab_values(ref->value, deref, *obj, buf, size);
>> if (!eaten)
>> free(buf);
>> +   return 0;
>>  }

Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

[Junio C Hamano]

Martin Ågren  writes:

>>  static int grab_objectname(const char *name, const unsigned char *sha1,
>> -  struct atom_value *v, struct used_atom *atom)
>> +  struct atom_value *v, struct used_atom *atom,
>> +  struct strbuf *err)
>>  {
>> ...
>> +   } else {
>> +   strbuf_addstr(err, "BUG: unknown %(objectname) 
>> option");
>> +   return -1;
>> +   }
>> }
>> return 0;
>>  }
>
> This is interesting. This die() is never ever supposed to actually
> trigger, except to allow a developer adding some new O_xxx-value to
> quickly notice that they have forgotten to add code here.

Yup, BUG() is meant for a case like this; the original code predates
the BUG("message") which in turn predates the die("BUG: message")
convention, I think.

>> default:
>> -   die("Eh?  Object of type %d?", obj->type);
>> +   strbuf_addf(err, "Eh?  Object of type %d?", obj->type);
>> +   return -1;
>> }
>> +   return 0;
>>  }
>
> This seems similar. The string here is quite sloppy, and I do not
> believe that the author intended this to be user-visible. I believe this
> is more like a very short way of saying "how could we possibly get
> here??". It could also be written as die("BUG: unknown object type %d",
> obj->type), or even better: BUG(...).

Likewise.

Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

[Eric Sunshine]

On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren  wrote:
> These are "real" errors and yield several more changes in the remainder.
> Ignoring those BUG-type messages at the beginning of this patch would
> give a patch like the one below.
>
> +static int get_object(struct ref_array_item *ref, const struct object_id 
> *oid,
> +  int deref, struct object **obj, struct strbuf *err)
>  {
> void *buf = get_obj(oid, obj, , );
> -   if (!buf)
> -   die(_("missing object %s for %s"),
> -   oid_to_hex(oid), ref->refname);
> -   if (!*obj)
> -   die(_("parse_object_buffer failed on %s for %s"),
> -   oid_to_hex(oid), ref->refname);
> -
> +   if (!buf) {
> +   strbuf_addf(err, _("missing object %s for %s"), 
> oid_to_hex(oid),
> +   ref->refname);
> +   return -1;
> +   }
> +   if (!*obj) {
> +   strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> +   oid_to_hex(oid), ref->refname);
> +   return -1;

Doesn't this leak 'buf'?

> +   }
> grab_values(ref->value, deref, *obj, buf, size);
> if (!eaten)
> free(buf);
> +   return 0;
>  }

Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

[Martin Ågren]

I skimmed the first four patches of this v2. It seems that patches 1 and
4 are identical to v2. Patches 2 and 3 have very straightforward changes
based on my earlier comments. Let's see what this patch is about. :-)

On 14 March 2018 at 20:04, Olga Telezhnaya  wrote:
> Finish removing any printing from ref-filter formatting logic,
> so that it could be more general.
>
> Change the signature of get_ref_atom_value() and underlying functions
> by adding return value and strbuf parameter for error message.
>
> It's important to mention that grab_objectname() returned 1 if
> it gets objectname atom and 0 otherwise. Now this logic changed:
> we return 0 if we have no error, -1 otherwise. If someone needs to
> know whether it's objectname atom or not, he/she could use
> starts_with() function. It duplicates this checking but it does not
> sound like a really big overhead.
>
> Signed-off-by: Olga Telezhnaia 
> ---
>  ref-filter.c | 109 
> +--
>  1 file changed, 69 insertions(+), 40 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 62ea4adcd0ff1..3f0c3924273d5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, 
> struct object **obj, unsigned
>  }
>
>  static int grab_objectname(const char *name, const unsigned char *sha1,
> -  struct atom_value *v, struct used_atom *atom)
> +  struct atom_value *v, struct used_atom *atom,
> +  struct strbuf *err)
>  {
> if (starts_with(name, "objectname")) {
> if (atom->u.objectname.option == O_SHORT) {
> v->s = xstrdup(find_unique_abbrev(sha1, 
> DEFAULT_ABBREV));
> -   return 1;
> } else if (atom->u.objectname.option == O_FULL) {
> v->s = xstrdup(sha1_to_hex(sha1));
> -   return 1;
> } else if (atom->u.objectname.option == O_LENGTH) {
> v->s = xstrdup(find_unique_abbrev(sha1, 
> atom->u.objectname.length));
> -   return 1;
> -   } else
> -   die("BUG: unknown %%(objectname) option");
> +   } else {
> +   strbuf_addstr(err, "BUG: unknown %(objectname) 
> option");
> +   return -1;
> +   }
> }
> return 0;
>  }

This is interesting. This die() is never ever supposed to actually
trigger, except to allow a developer adding some new O_xxx-value to
quickly notice that they have forgotten to add code here.

>  /* See grab_values */
> -static void grab_common_values(struct atom_value *val, int deref, struct 
> object *obj, void *buf, unsigned long sz)
> +static int grab_common_values(struct atom_value *val, int deref, struct 
> object *obj,
> + void *buf, unsigned long sz, struct strbuf *err)
>  {
> int i;
>
> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, 
> int deref, struct object
> v->s = xstrfmt("%lu", sz);
> }
> else if (deref)
> -   grab_objectname(name, obj->oid.hash, v, 
> _atom[i]);
> +   if (grab_objectname(name, obj->oid.hash, v, 
> _atom[i], err))
> +   return -1;
> }
> +   return 0;
>  }

So if that conversion I commented on above had not happened, this would
not have been necessary. Let's read on...

>  /* See grab_values */
> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)
>   * pointed at by the ref itself; otherwise it is the object the
>   * ref (which is a tag) refers to.
>   */
> -static void grab_values(struct atom_value *val, int deref, struct object 
> *obj, void *buf, unsigned long sz)
> +static int grab_values(struct atom_value *val, int deref, struct object *obj,
> +  void *buf, unsigned long sz, struct strbuf *err)
>  {
> -   grab_common_values(val, deref, obj, buf, sz);
> +   if (grab_common_values(val, deref, obj, buf, sz, err))
> +   return -1;
> switch (obj->type) {
> case OBJ_TAG:
> grab_tag_values(val, deref, obj, buf, sz);
> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int 
> deref, struct object *obj, v
> /* grab_blob_values(val, deref, obj, buf, sz); */
> break;
> default:
> -   die("Eh?  Object of type %d?", obj->type);
> +   strbuf_addf(err, "Eh?  Object of type %d?", obj->type);
> +   return -1;
> }
> +   return 0;
>  }

This seems similar. The string here is quite sloppy, and I do not
believe that the author intended this to be user-visible. I believe this
is more like a very