Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument

"Hariom Verma via GitGitGadget" [off-list ref] writes:

 		} else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
 			   !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
 			   !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
-			   !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only))
-			break;
+			   !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) {
+			size_t invalid_arg_len = strcspn(*arg, ",)");
+			*invalid_arg = xstrndup(*arg, invalid_arg_len);
+			return 1;
+		}
 	}
 	return 0;
 }

Would the existing caller be OK with this change?

It used to be that this parsing code simply _ignored_ unrecognised
trailer keyword because the very original just did a "break" and
fell though, but now because this returns non-zero, it causes the
caller rewritten in the patch [v2 1/3] to "goto trailer_out".

It is not clear from your proposed log message if this would result
in behaviour change, and if so if that behaviour change was intended.

I suspect that the behaviour change the code implements may be OK,
but the log message needs to discuss why it is OK.

Thanks.

Hi,

On Sat, Jan 30, 2021 at 5:37 AM Junio C Hamano [off-list ref] wrote:

"Hariom Verma via GitGitGadget" [off-list ref] writes:

quoted

              } else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
                         !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
                         !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
-                        !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only))
-                     break;
+                        !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) {
+                     size_t invalid_arg_len = strcspn(*arg, ",)");
+                     *invalid_arg = xstrndup(*arg, invalid_arg_len);
+                     return 1;
+             }
      }
      return 0;
 }

Would the existing caller be OK with this change?

Yes, I made sure of that.

It used to be that this parsing code simply _ignored_ unrecognised
trailer keyword because the very original just did a "break" and
fell though, but now because this returns non-zero, it causes the
caller rewritten in the patch [v2 1/3] to "goto trailer_out".

It is not clear from your proposed log message if this would result
in behaviour change, and if so if that behaviour change was intended.

I suspect that the behaviour change the code implements may be OK,
but the log message needs to discuss why it is OK.

I should have included that change in behaviour in the commit message.
Will change that too.

Thanks,
Hariom Verma