Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks

From: Jeff King <peff@peff.net>
Date: Sep 5, 2023, 06:52 UTC
Message-ID: <20230905065237.GB199565@coredump.intra.peff.net>
In-Reply-To: <8f03fa13-11b3-1f9f-1e1f-8f7d8ce74a23@web.de>

On Sat, Sep 02, 2023 at 09:34:32AM +0200, René Scharfe wrote:

Show 9 quoted lines

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

Show 19 quoted lines

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