Re: [PATCH v3 02/23] ref-filter: add return value to some functions

2018-02-15 Thread Оля Тележная
2018-02-15 8:16 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Add return flag to format_ref_array_item(), show_ref_array_item(),
>> get_ref_array_info() and populate_value() for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
>
> OK. The source of these errors would eventually be calls in
> populate_value(), but we don't flag any errors there yet (well, we do,
> but they all end up in die() for now). So I'd expect to see later in the
> series those die() calls converted to errors (I haven't looked further
> yet; just making a note to myself).
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, 
>> struct ref_array_item *re
>>
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>> + * Return 0 if everything was successful, -1 otherwise.
>>   */
>
> We discussed off-list the concept that the caller may want to know one
> of three outcomes:
>
>   - we completed the request, having accessed the object
>   - we completed the request, but it didn't require accessing any
> objects
>   - an error occurred accessing the object
>
> Since callers like "cat-file" would need to check has_sha1_file()
> manually in the second case. Should this return value actually be an
> enum, which would make it easier to convert later to a tri-state?

I decided not to implement this particular scenario because all other
callers are waiting that everything will be printed inside ref-filter.
We just add support for cat-file there. I don't think that I need to
re-think all printing process and move printing logic to all other
callers so that cat-file will behave fine. In my opinion, in the final
version cat-file must accept all ref-filter logic parts and adapt to
them.

>
>> -static void populate_value(struct ref_array_item *ref)
>> +static int populate_value(struct ref_array_item *ref)
>>  {
>>   void *buf;
>>   struct object *obj;
>> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
>>   }
>>   }
>>   if (used_atom_cnt <= i)
>> - return;
>> + return 0;
>
> Most of these conversions are obviously correct, because they just turn
> a void return into one with a value. But this one is trickier:
>
>> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item 
>> *info,
>>   ep = strchr(sp, ')');
>>   if (cp < sp)
>>   append_literal(cp, sp, );
>> - get_ref_atom_value(info,
>> -parse_ref_filter_atom(format, sp + 2, ep),
>> -);
>> + if (get_ref_atom_value(info,
>> +parse_ref_filter_atom(format, sp + 2, 
>> ep),
>> +))
>> + return -1;
>>   atomv->handler(atomv, );
>>   }
>
> since it affects the control flow. Might we be skipping any necessary
> cleanup in the function if we see an error?
>
> It looks like we may have called push_stack_element(), but we'd never
> get to the end of the function where we call pop_stack_element(),
> causing us to leak.

Agree,  I will fix this.

>
> -Peff


Re: [PATCH v3 02/23] ref-filter: add return value to some functions

2018-02-14 Thread Jeff King
On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:

> Add return flag to format_ref_array_item(), show_ref_array_item(),
> get_ref_array_info() and populate_value() for further using.
> Need it to handle situations when item is broken but we can not invoke
> die() because we are in batch mode and all items need to be processed.

OK. The source of these errors would eventually be calls in
populate_value(), but we don't flag any errors there yet (well, we do,
but they all end up in die() for now). So I'd expect to see later in the
series those die() calls converted to errors (I haven't looked further
yet; just making a note to myself).

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, 
> struct ref_array_item *re
>  
>  /*
>   * Parse the object referred by ref, and grab needed value.
> + * Return 0 if everything was successful, -1 otherwise.
>   */

We discussed off-list the concept that the caller may want to know one
of three outcomes:

  - we completed the request, having accessed the object
  - we completed the request, but it didn't require accessing any
objects
  - an error occurred accessing the object

Since callers like "cat-file" would need to check has_sha1_file()
manually in the second case. Should this return value actually be an
enum, which would make it easier to convert later to a tri-state?

> -static void populate_value(struct ref_array_item *ref)
> +static int populate_value(struct ref_array_item *ref)
>  {
>   void *buf;
>   struct object *obj;
> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
>   }
>   }
>   if (used_atom_cnt <= i)
> - return;
> + return 0;

Most of these conversions are obviously correct, because they just turn
a void return into one with a value. But this one is trickier:

> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,
>   ep = strchr(sp, ')');
>   if (cp < sp)
>   append_literal(cp, sp, );
> - get_ref_atom_value(info,
> -parse_ref_filter_atom(format, sp + 2, ep),
> -);
> + if (get_ref_atom_value(info,
> +parse_ref_filter_atom(format, sp + 2, 
> ep),
> +))
> + return -1;
>   atomv->handler(atomv, );
>   }

since it affects the control flow. Might we be skipping any necessary
cleanup in the function if we see an error?

It looks like we may have called push_stack_element(), but we'd never
get to the end of the function where we call pop_stack_element(),
causing us to leak.

-Peff