Re: [PATCH v4] repository: move fetch_if_missing into struct repository

Tian Yuchen <[email protected]> writes:

> The global variable 'fetch_if_missing' controls whether a missing
> object check should prompt a lazy fetch from a promisor remote.
> In order to continue the libification effort, move it into
> 'struct repository' and initialize it to 1 by default to keep the
> previous behavior.
>
> Note that in builtin/fsck.c and builtin/index-pack.c, when running
> related commands with the '-h' parameter, the 'repo' pointer is not
> passed in. To prevent null pointer dereferences, we defer
> operations on the repo until after parameter parsing is complete.
>
> Additionally, update the partial clone documentation to reflect
> that this is now a per-repository flag.
>
> Mentored-by: Christian Couder <[email protected]>
> Mentored-by: Ayush Chandekar <[email protected]>
> Mentored-by: Olamide Caleb Bello <[email protected]>
> Signed-off-by: Tian Yuchen <[email protected]>
> ---
>
> Changes since v3:
>
>  - Use revs->repo in revision.c instead of the_repository.
>
>  - Coordinate the other topics. Specifically, for common-init.c, use
>  the_repository->fetch_if_missing in setup_environment(), etc. This patch
>  currently does not conflict with seen or next.
>
>  Documentation/technical/partial-clone.adoc |  2 +-
>  builtin/fetch-pack.c                       |  2 +-
>  builtin/fsck.c                             |  6 +++---
>  builtin/index-pack.c                       |  8 ++++----
>  builtin/pack-objects.c                     | 14 +++++++-------
>  builtin/prune.c                            |  2 +-
>  builtin/rev-list.c                         | 10 +++++-----
>  common-init.c                              |  2 +-
>  git.c                                      |  2 +-
>  midx-write.c                               |  2 +-
>  odb.c                                      |  4 +---
>  odb.h                                      |  8 --------
>  repository.c                               |  1 +
>  repository.h                               |  6 ++++++
>  revision.c                                 |  2 +-
>  15 files changed, 34 insertions(+), 37 deletions(-)

There still are references to the_repository->fetch_if_missing
remaining in the codebase with this change.

        $ git grep -l -e 'the_repository->fetch_if_missing'
        builtin/fetch-pack.c
        builtin/pack-objects.c
        builtin/rev-list.c
        common-init.c
        git.c

Some of them I suspect should just use the caller supplied 'repo',
possibly after removing the UNUSED marker.  For example:

        int cmd_fetch_pack(int argc,
                           const char **argv,
                           const char *prefix UNUSED,
                           struct repository *repo UNUSED)
        {
                int i, ret;
        ...
                enum protocol_version version;

                the_repository->fetch_if_missing = 0;

                packet_trace_identity("fetch-pack");

                memset(&args, 0, sizeof(args));
                list_objects_filter_init(&args.filter_options);
                args.uploadpack = "git-upload-pack";

                show_usage_if_asked(argc, argv, fetch_pack_usage);

As { "fetch-pack", cmd_fetch_pack } in the git.c:commands[] array is
marked as RUN_SETUP, repo will not be NULL unless "git fetch-pack -h"
is requested, and when repo is NULL, show_usage_if_asked() will give
the short help text and never return.

So I think it makes sense to set 'fetch_if_missing' *after* the call
to show_usage_if_asked() and set it in 'repo', not in 'the_repository'.

Other hits in the above "git grep" output looked similar.  The code
paths in pack-objects.c may need a preliminary clean-up patch (or
two) before moving fetch_if_missing to the repository instance.
I.e., pass repo through the call graph from cmd_pack_objects() to
read_stdin_packs(), and then update assignments to fetch_if_missing
variable to instead assign to repo->fetch_if_missing in a second
step.  There are other code paths that want similar clean-up.

HTH.