Re: [PATCH 3/8] midx-write: pass down repository to `write_midx_file[_only]`

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 18, 2024, 08:05 UTC
Message-ID: <Zzr1Z7UEL9nCrybP@pks.im>
In-Reply-To: <20241115-374-refactor-midx-c-and-midx-write-c-to-not-depend-on-global-state-v1-3-761f8a2c7775@gmail.com>

On Fri, Nov 15, 2024 at 02:42:16PM +0100, Karthik Nayak wrote:

> In the previous commit, we passed the repository field to all

Not quite accurate, since the preceding commit is instead passing the repo via the context, not adapting the subcommands.

Show 6 quoted lines

> subcommands in the `builtin/` directory. We utilize this to pass the
> repository field down to the `write_midx_file[_only]` functions to
> remove the usage of `the_repository` global variables.
> 
> With this, we remove all usage of global variables in `midx-write.c` and
> so we can remove the `USE_THE_REPOSITORY_VARIABLE` guard from the file.

Note that we typically use imperative commit messages, as if we were instructing the code to change. So you should likely drop the "we" both here and further up in "We utilize".

Show 13 quoted lines

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 85e40a4b6d3e47e9ec1ec27c094455e5ba75b5b0..2a938466f53aaa11096170554fe11a4ed46a25e4 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -120,7 +120,7 @@ static void read_packs_from_stdin(struct string_list *to)
>  
>  static int cmd_multi_pack_index_write(int argc, const char **argv,
>  				      const char *prefix,
> -				      struct repository *repo UNUSED)
> +				      struct repository *repo)
>  {
>  	struct option *options;
>  	static struct option builtin_multi_pack_index_write_options[] = {

One thing I've briefly wondered: do we actually need the whole repo, or do we only care about `packed_git`? I would have assumed that it should be the latter as an MIDX only gets written for packfiles, but that is likely only a naive assumption.

Show 10 quoted lines

> diff --git a/midx-write.c b/midx-write.c
> index a384f7ddc8a396d0cffd528132bb8fcdc6b37e24..5af29899bbe279c7c3ff4bc2c65330620ce37ee2 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1,5 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
> -
>  #include "git-compat-util.h"
>  #include "abspath.h"
>  #include "config.h"

Nice.

Patrick