diff mbox series

[v2,1/4] git: pass in repo for RUN_SETUP_GENTLY

Message ID 5d72c31c6f3b97b7f5f7d3b4fa9a8b1587597670.1727718030.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Remove the_repository global for am, annotate, apply, archive builtins | expand

Commit Message

John Cai Sept. 30, 2024, 5:40 p.m. UTC 
From: John Cai <johncai86@gmail.com>

commands that have RUN_SETUP_GENTLY potentially need a repository.
Modify the logic in run_builtin() to pass the repository to the builtin
if a builtin has the RUN_SETUP_GENTLY property.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 git.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Sept. 30, 2024, 7:40 p.m. UTC | #1 
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> commands that have RUN_SETUP_GENTLY potentially need a repository.
> Modify the logic in run_builtin() to pass the repository to the builtin
> if a builtin has the RUN_SETUP_GENTLY property.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  git.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/git.c b/git.c
> index 2fbea24ec92..f58f169f3c7 100644
> --- a/git.c
> +++ b/git.c
> @@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)
>  
>  static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
>  {
> -	int status, help;
> +	int status, help, repo_exists;
>  	struct stat st;
>  	const char *prefix;
>  	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
> @@ -455,9 +455,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  
>  	if (run_setup & RUN_SETUP) {
>  		prefix = setup_git_directory();
> +		repo_exists = 1;
>  	} else if (run_setup & RUN_SETUP_GENTLY) {
>  		int nongit_ok;
>  		prefix = setup_git_directory_gently(&nongit_ok);
> +
> +		if (!nongit_ok)
> +			repo_exists = 1;

Why not use the new variable and pass it directly to nongit_ok?  The
polarity of the new variable needs to be swapped, of course, but I
think it makes reading the code to call p->fn() easier to grok

i.e., 

 - rename repo_exists to "no_repo", and initialize it to non-zero.
 - remove "int nongit_ok"; pass &no_repo instead.
 - update the calling of p->fn() to

	p->fn(argc, argv, prefix, no_repo ? NULL : repo);

>  	} else {
>  		prefix = NULL;
>  	}
> @@ -480,7 +484,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  	trace2_cmd_name(p->cmd);
>  
>  	validate_cache_entries(repo->index);
> -	status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);
> +	status = p->fn(argc,
> +		       argv,
> +		       prefix,
> +		       repo_exists ? repo : NULL);

Keeping the call to a single line would be much better than spreding
it across four lines.

Thanks

>  	validate_cache_entries(repo->index);
>  
>  	if (status)
shejialuo Oct. 1, 2024, 4:21 a.m. UTC | #2 
On Mon, Sep 30, 2024 at 05:40:27PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
> 
> commands that have RUN_SETUP_GENTLY potentially need a repository.
> Modify the logic in run_builtin() to pass the repository to the builtin
> if a builtin has the RUN_SETUP_GENTLY property.
> 

We will parse the "repo" to the "run_builtin()" for "RUN_SETUP_GENTLY"
property only when we know we run the command in the repository. If we
run the command outside of the repository, we should pass the NULL.

However, the above commit message is not accurate. If a builtin has the
"RUN_SETUP_GENTLY" property. We may pass or not.

> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  git.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/git.c b/git.c
> index 2fbea24ec92..f58f169f3c7 100644
> --- a/git.c
> +++ b/git.c
> @@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)
>  
>  static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
>  {
> -	int status, help;
> +	int status, help, repo_exists;

This is wrong. We should initialize the "repo_exists" variable here.
Because we never set this variable to 0 in the later code path. It will
always be true for the following code:

    repo_exists ? repo : NULL

It will always evaluate to the "repo". This may could answer the
question raised by Junio in [PATCH v2 3/4].

Thanks,
Jialuo
diff mbox series

Patch

diff --git a/git.c b/git.c
index 2fbea24ec92..f58f169f3c7 100644
--- a/git.c
+++ b/git.c
@@ -443,7 +443,7 @@  static int handle_alias(int *argcp, const char ***argv)
 
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
 {
-	int status, help;
+	int status, help, repo_exists;
 	struct stat st;
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
@@ -455,9 +455,13 @@  static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 
 	if (run_setup & RUN_SETUP) {
 		prefix = setup_git_directory();
+		repo_exists = 1;
 	} else if (run_setup & RUN_SETUP_GENTLY) {
 		int nongit_ok;
 		prefix = setup_git_directory_gently(&nongit_ok);
+
+		if (!nongit_ok)
+			repo_exists = 1;
 	} else {
 		prefix = NULL;
 	}
@@ -480,7 +484,10 @@  static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	trace2_cmd_name(p->cmd);
 
 	validate_cache_entries(repo->index);
-	status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);
+	status = p->fn(argc,
+		       argv,
+		       prefix,
+		       repo_exists ? repo : NULL);
 	validate_cache_entries(repo->index);
 
 	if (status)