Re: [PATCH v2 14/14] update-index: remove static globals from callbacks

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 4, 2021 at 11:43 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> 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@microsoft.com>
> ---
> 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.