On Sat, Sep 02, 2023 at 09:34:32AM +0200, René Scharfe wrote:
> > static int option_parse_if_missing(const struct option *opt,
> > const char *arg, int unset)
> > {
> > - return trailer_set_if_missing(&if_missing, arg);
> > + return trailer_set_if_missing(opt->value, arg);
> > }
>
> Not your fault, but these all silently exit if "arg" contains an
> unrecognized value. Reporting the error would be better.
Hmm, yeah. On the config side, git_trailer_default_config() issues a
warning after the functions return -1. I'd have guessed we at least
printed a usage message or something here, but we don't even do that.
You get a silent 129 exit code.
Gross, but it's something I think we should fix separately, as it's
orthogonal to this series.
> > static void new_trailers_clear(struct list_head *trailers)
> > @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> > OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
> > OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
> >
> > - OPT_CALLBACK(0, "where", NULL, N_("action"),
> > + OPT_CALLBACK(0, "where", &where, N_("action"),
> > N_("where to place the new trailer"), option_parse_where),
> > - OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
> > + OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
> > N_("action if trailer already exists"), option_parse_if_exists),
> > - OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
> > + OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
> > N_("action if trailer is missing"), option_parse_if_missing),
>
> And I wonder if "action" should be replaced by "(after|before|end|start)",
> "(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)" and
> "(doNothing|add)", respectively. Gets a bit long in the middle, but would
> be more helpful. #leftoverbits
I don't have a strong opinion. It is sometimes nice to provide more
detail to save the user having to look it up separately. But sometimes
those details can be overwhelming and make it hard to read, especially
if they grow over time. I'm thinking less of "-h" output and more of
people like to put:
git foo [--optionA|--optionB|--optionC]
and so on, until eventually the options block is like 4 lines long. Just
saying [options] and then listing them is more friendly. I'm not sure if
we're approaching that kind of problem here or not.
-Peff
