Re: [PATCH v2 5/9] [GSOC] ref-filter: teach get_object() return useful value

Junio C Hamano [off-list ref] 于2021年6月16日周三 下午3:36写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

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.

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.

quoted

@@ -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.

quoted

@@ -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

      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 ...

quoted

                  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

              }
      }
      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