Am 09.08.23 um 16:09 schrieb Jeff King:

On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:

quoted

quoted

So before I sent a patch (either to switch to using opt->value, or to
add an UNUSED annotation), I wanted to see what you (or others) thought
between the two. I.e., should we have a rule of "try not to operate on
global data via option callbacks" or is that just being too pedantic for
one-off callbacks like this?

So, that was my preference, but I may be missing some obvious
downsides.  I am interested in hearing from René, who often shows
better taste than I do in these cases ;-)

Me too. :)

I'm pretty sure I didn't think much about it, copied the style from the
other callback functions in builtin/pack-objects.c and was glad to not
have to deal with void pointers.

And sorry for the unused parameter warning.  Just checked; there are
170+ of those remaining before we can enable it in developer mode.  :-/
Seems worthwhile, though, especially not slapping UNUSED blindly on
them.

The main downsides, I think, are:

  1. It's a little more ugly.

  2. We lose type safety, as the variable address passes through a void
     pointer (but that is true of all option callbacks).

An upside is that we can reuse the callback if we are careful to wire
it up to a variable of the correct type.  Another is that we can use
it on local variables.

Hmm, we could make these callbacks type-safe fairly easily by adding
pointers of all relevant types to struct option, like "int *value_int".
How many types would we need?

   $ git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort -u | wc -l
   37

Oh.  Do we really need all those?  Anyway, if we added at least the
most common ones, we'd be better off already, I'd expect:

   $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
     29 struct diff_options
     12 int
      7 struct grep_opt
      6 struct rebase_options
      6 struct apply_state
      5 struct strbuf
      5 char
      4 struct note_data
      3 struct string_list
      2 struct strvec

Increasing the size of the struct like that would increase the total
memory footprint by a few KB at most -- tolerable.

This line would assign opt->value_int instead...

... but the macro OP_CALLBACK_F could no longer be used, because we'd
need to select one of the many typed pointers.

Macros like OPT_BOOL could be changed to use the appropriate typed
pointer.

Thoughts?  Too much churn?