Junio C Hamano <gitster@xxxxxxxxx> 于2021年6月16日周三 下午3:36写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: ZheNing Hu <adlternative@xxxxxxxxx>
> >
> > Let `populate_value()`, `get_ref_atom_value()` and
> > `format_ref_array_item()` get the return value of `get_object()`
> > correctly.
>
> The "get" the value correctly, I think. What you are teaching them
> is to pass the return value from get_object() through the callchain
> to their callers.
>
Yes, this is exactly what I meant.
> The readers will be helped if you say what kind of errors
> get_object() wants to tell its callers, not just "-1" is for error,
> which is what populate_value() assumes to be sufficient. In other
> words, which non-zero returns from get_object() are interesting and
> why?
>
As stated in 765337a, We can just print the error without exiting if the
return value of format_ref_array_item() is greater than 0. Therefore,
the current patch is to make get_object() return a value other than
-1 when an error occurs.
> > @@ -1997,9 +1997,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
> > static int get_ref_atom_value(struct ref_array_item *ref, int atom,
> > struct atom_value **v, struct strbuf *err)
> > {
> > + int ret = 0;
> > +
> > if (!ref->value) {
> > - if (populate_value(ref, err))
> > - return -1;
> > + if ((ret = populate_value(ref, err)))
> > + return ret;
>
> The new variable only needs to be in this scope, and does not have
> to be shown to the entire function.
>
Makes sense.
> > @@ -2573,6 +2575,7 @@ int format_ref_array_item(struct ref_array_item *info,
> > {
> > const char *cp, *sp, *ep;
> > struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
> > + int ret = 0;
>
> This is dubious...
>
> > state.quote_style = format->quote_style;
> > push_stack_element(&state.stack);
> > @@ -2585,10 +2588,10 @@ int format_ref_array_item(struct ref_array_item *info,
> > if (cp < sp)
> > append_literal(cp, sp, &state);
> > pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
> > - if (pos < 0 || get_ref_atom_value(info, pos, &atomv, error_buf) ||
> > + if (pos < 0 || (ret = get_ref_atom_value(info, pos, &atomv, error_buf)) ||
>
> Here, if "ret" gets assigned any non-zero value, the condition is
> satisfied, and ...
>
> > atomv->handler(atomv, &state, error_buf)) {
> > pop_stack_element(&state.stack);
> > - return -1;
> > + return ret ? ret : -1;
>
> ... the control flow will leave this function. Therefore, ...
>
> > }
> > }
> > if (*cp) {
> > @@ -2610,7 +2613,7 @@ int format_ref_array_item(struct ref_array_item *info,
> > }
> > strbuf_addbuf(final_buf, &state.stack->output);
> > pop_stack_element(&state.stack);
> > - return 0;
> > + return ret;
>
> ... at this point, "ret" can never be anything other than zero. Am
> I misreading the patch?
>
> If I am not misreading the patch, then "ret" does not have to be
> globally visible in this function---it can have the same scope as
> "pos".
>
You are right, It is correct to only return 0 here at the moment.
Thanks.
--
ZheNing Hu
