On Mon, Jan 4, 2021 at 11:43 PM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> In order to remove index compatibility macros cleanly, we relied upon
> static globals 'repo' and 'istate' to be pointers to the_repository and
> the_index, respectively. We remove these static globals inside the
> option parsing callbacks, which are the final uses in update-index.
>
> The callbacks cannot change their method signature, so we must use the
> value member of 'struct option', assigned in the array of option macros.
> There are several callback methods that require at least one of 'repo'
> and 'istate', but they use a variety of different data types for the
> callback value.
>
> Unify these callback methods to use a consistent 'struct callback_data'
> that contains 'repo' and 'istate', ready to use. This takes the place of
> the previous 'struct refresh_params' which served only to group the
> 'flags' and 'has_errors' ints. We also collect other one-off settings,
> but only those that require access to the index or repository in their
> operation.
Makes sense. The patch itself is necessarily a bit noisy, but there's
nothing particularly complicated in that noise.
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> @@ -784,19 +784,21 @@ static int do_reupdate(struct repository *repo,
> -struct refresh_params {
> +struct callback_data {
> + struct repository *repo;
> + struct index_state *istate;
> +
> unsigned int flags;
> - int *has_errors;
> + unsigned int has_errors;
> + unsigned nul_term_line;
> + unsigned read_from_stdin;
> };
The only mildly unexpected thing here is that `has_errors` is now a
simple value rather than a pointer to a value, but you handle that
easily enough by always accessing `has_error` directly from the
structure, even within the function in which `has_error` used to be a
local variable. Fine.
> @@ -818,7 +820,7 @@ static int really_refresh_callback(const struct option *opt,
> static int chmod_callback(const struct option *opt,
> - const char *arg, int unset)
> + const char *arg, int unset)
> @@ -829,11 +831,12 @@ static int chmod_callback(const struct option *opt,
> static int resolve_undo_clear_callback(const struct option *opt,
> - const char *arg, int unset)
> + const char *arg, int unset)
A couple drive-by indentation fixes. Okay.
> @@ -1098,8 +1103,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> - istate = repo->index;
> + cd.repo = repo;
> + cd.istate = istate = repo->index;
Will there ever be a case in which `cd.istate` will be different from
`cd.repo->index`? If not, then we could get by with having only
`cd.repo`; callers requiring access to `istate` can fetch it from
`cd.repo`. If, on the other hand, `cd.istate` can be different from
`cd.repo->istate` -- or if that might become a possibility in the
future -- then having `cd.istate` makes sense. Not a big deal, though.
Just generally curious about it.
