Re: [PATCH v2 5/9] [GSOC] ref-filter: teach get_object() return useful value
From: Junio C Hamano <hidden>
Date: 2021-06-16 07:36:07
"ZheNing Hu via GitGitGadget" [off-list ref] writes:
From: ZheNing Hu <redacted> 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. 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?
quoted hunk
@@ -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.
quoted hunk
@@ -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...
quoted hunk
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, ...
quoted hunk
} } 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".
} void pretty_print_ref(const char *name, const struct object_id *oid,