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

"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?

The new variable only needs to be in this scope, and does not have
to be shown to the entire function.

This is dubious...

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

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

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