2018-01-26 23:19 GMT+03:00 Junio C Hamano <gitster@xxxxxxxxx>:
> Olga Telezhnaya <olyatelezhnaya@xxxxxxxxx> writes:
>
>> Get rid of goto command in ref-filter for better readability.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@xxxxxxxxx>
>> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
>> Mentored by: Jeff King <peff@xxxxxxxx>
>> ---
>
> How was this patch "mentored by" these two folks? Have they already
> reviewed and gave you OK, or are you asking them to also help reviewing
> with this message? Mostly just being curious.
Christian and Jeff help me when I have different sort of difficulties.
Not sure that they were helping me with that commit separately.
Both of them reviewed my code and said that it's ready for a final
review (actually, Christian said, but it's usual situation when I ask
for help/review and one of them helps me. The other one could add
something, but, as I understand, if he totally agree, he will keep
silence, and I find that behavior logical).
Do I need to delete these lines from some of commits where I do not
remember help from them?
>
> It is not convincning that this splitting the last part of a single
> function into a separate helper function that is called from only
> one place improves readability. If better readability is the
> purpose, I would even say
>
> for (i = 0; i < used_atom_cnt; i++) {
> if (...)
> - goto need_obj;
> + break;
> }
> - return;
> + if (used_atom_cnt <= i)
> return;
>
> -need_obj:
>
> would make the result easier to follow with a much less impact.
It's hard for me to read the code with goto, and as I know, it's not
only my problem, it's usual situation to try to get rid of gotos. I
always need to re-check whether we use that piece of code somewhere
else or not, and how we do that. I also think that it's good that most
of variables in the beginning of the function populate_value go to new
function.
>
> If we later in the series will use this new helper function from
> other places, it certainly makes sense to create a new helper like
> this patch does, but then "get rid of goto for readability" is not
> the justification for such a change.
We don't use that new function anywhere else further. So, I can delete
this commit or I can change commit message (if so, please give me some
ideas what I need to mention there).
>
>> ref-filter.c | 103 ++++++++++++++++++++++++++++++-----------------------------
>> 1 file changed, 53 insertions(+), 50 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index f9e25aea7a97e..37337b57aacf4 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1354,16 +1354,60 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>> return show_ref(&atom->u.refname, ref->refname);
>> }
>>
>> +static void need_object(struct ref_array_item *ref) {
>
> Style. The opening brace at the beginning of the function sits on
> its own line alone.
Thanks, I will fix that when we decide how to finally improve that commit.
Olga
