Re: [PATCH v5 1/2] pack-objects: give fetch_if_missing call sites access to 'repo'

Tian Yuchen <[email protected]> writes:

> 'option_parse_missing_action()' is registered as an OPT_CALLBACK, so
> its signature is fixed and cannot easily gain a 'repo' parameter of
> its own.

Is that true?  OPT_CALLBACK takes (short, long, value, arghelp,
help, callback), but the value parameter, which typically receives
the location to store the parsed value in, is NULL for
"--missing=<...>" option, like so:

		OPT_CALLBACK_F(0, "missing", NULL, N_("action"),
		  N_("handling for missing objects"), PARSE_OPT_NONEG,
		  option_parse_missing_action),

And "value" does not have to be the exact location; we can pass
the address of the repository instance there, and the callback can
dereference it to reach repo->fetch_if_missing member to make an
assignment, if it wants to.

Am I missing something?

> Let it only record 'arg_missing_action'. Instead, apply the
> side effect right after 'parse_options()' returns in
> 'cmd_pack_objects()', where 'repo' is available.

When I review a new iteration, I first apply and look at the
difference from the previous iteration before reading the new
iteration of patches or their proposed commit log messages, and I
did notice this business about "arg_missing_action" and wondering
what it was about (which by the way is precisely I read the patch
before the log message to ensure that the log message explains what
I find unclear in the code).  Thanks for describing this.  But if we
use OPT_CALLBACK() to pass the location of the variable, this may no
longer be necessary, no?