Re: [GSoC PATCH v1] builtin: stop using the_repository

[Junio C Hamano <gitster@pobox.com>]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> The builtins use the_repository global variable which might
> not work well when running many repos in the same process at once.

This is true, but ...

> Stop using the_repository in these builtins to align with the goal of
> libification of Git.

... in general, each file under builtin/ is about a single command
that _uses_ libified part of Git.  So it is perfectly fine for the
libification goal to include "libified functions should not assume
that it works on the_repository, but they should accept a repo
parameter to tell them which repository to work with".  But it is
not necessary, and I would say it is harmful, to subject builtin/*.c
to the same criteria.  The builtin command implementations can call
libified function by passing the_repository to libified API function
that expects a repo parameter.

> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> ---
>  builtin/bugreport.c    | 13 ++++++-------
>  builtin/bundle.c       | 13 ++++++-------
>  builtin/check-attr.c   | 26 +++++++++++++-------------
>  builtin/check-ignore.c | 27 +++++++++++++++------------
>  4 files changed, 40 insertions(+), 39 deletions(-)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index f78c3f2aed..77eb8bd9c1 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "builtin.h"
>  #include "abspath.h"
>  #include "editor.h"
> @@ -37,7 +36,7 @@ static void get_system_info(struct strbuf *sys_info)
>  		    shell ? shell : "<unset>");
>  }
>  
> -static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> +static void get_populated_hooks(struct repository *repo, struct strbuf *hook_info, int nongit)
>  {
>  	const char **p;
>  
> @@ -50,7 +49,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  	for (p = hook_name_list; *p; p++) {
>  		const char *hook = *p;
>  
> -		if (hook_exists(the_repository, hook))
> +		if (hook_exists(repo, hook))
>  			strbuf_addf(hook_info, "%s\n", hook);
>  	}
>  }

It is not strictly necessary to churn a file-scope static function
like this one into taking an arbitrary repo parameter, as the only
caller of the function, presumably cmd_foo() in the builtin/foo.c
file, would pass the_repository anyway, whether it explicitly names
the_repository or passes the repo parameter that it got from its
caller, git.c:run_builtin().  We _can_ consider a change like the
above as a preparation to potentially move these functions to the
libified part of Git, so even though I said it is not necessary, it
is also OK to perform such a change.

> @@ -93,7 +92,7 @@ static void get_header(struct strbuf *buf, const char *title)
>  int cmd_bugreport(int argc,
>  		  const char **argv,
>  		  const char *prefix,
> -		  struct repository *repo UNUSED)
> +		  struct repository *repo)
>  {
>  	struct strbuf buffer = STRBUF_INIT;
>  	struct strbuf report_path = STRBUF_INIT;
> @@ -141,7 +140,7 @@ int cmd_bugreport(int argc,
>  	}
>  	strbuf_addstr(&report_path, ".txt");
>  
> -	switch (safe_create_leading_directories(the_repository, report_path.buf)) {
> +	switch (safe_create_leading_directories(repo, report_path.buf)) {
>  	case SCLD_OK:
>  	case SCLD_EXISTS:
>  		break;
> @@ -158,7 +157,7 @@ int cmd_bugreport(int argc,
>  		strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);
>  		strbuf_addstr(&zip_path, ".zip");
>  
> -		if (create_diagnostics_archive(the_repository, &zip_path, diagnose))
> +		if (create_diagnostics_archive(repo, &zip_path, diagnose))
>  			die_errno(_("unable to create diagnostics archive %s"), zip_path.buf);
>  
>  		strbuf_release(&zip_path);
> @@ -171,7 +170,7 @@ int cmd_bugreport(int argc,
>  	get_system_info(&buffer);
>  
>  	get_header(&buffer, _("Enabled Hooks"));
> -	get_populated_hooks(&buffer, !startup_info->have_repository);
> +	get_populated_hooks(repo, &buffer, !startup_info->have_repository);
>  
>  	/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
>  	report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);

All of the above look fine.

> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 1e170e9278..ef21ccfd89 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c

Is this patch meant as a microproject in preparation for applying
for GSoC?  If so, we ask to limit one quality focused one per
applicant.

https://git.github.io/General-Microproject-Information/#only-one-quality-focused-microproject-per-applicant