Re: [PATCH v2 5/9] [GSOC] ref-filter: teach get_object() return useful value
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 16, 2021, 07:36 UTC
- Message-ID
- <xmqq1r92qn18.fsf@gitster.g>
- In-Reply-To
- <49063372e0035c5384f834d78854da56f5726d13.1623763747.git.gitgitgadget@gmail.com>
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> From: ZheNing Hu <adlternative@gmail.com> > > 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?
Show 11 quoted lines
> @@ -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.
Show 5 quoted lines
> @@ -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...
Show 8 quoted lines
> 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, ...
Show 9 quoted lines
> }
> }
> 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,