[v2,4/4] archive: remove the_repository global variable

Commit Message

John Cai Sept. 30, 2024, 5:40 p.m. UTC

From: John Cai <[e-mail]>

Replace the_repository with the repository argument that gets passed
down through the builtin function.

Signed-off-by: John Cai <[e-mail]>
---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Sept. 30, 2024, 8:01 p.m. UTC | #1

"John Cai via GitGitGadget" <[e-mail]> writes:

> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "builtin.h"
>  #include "archive.h"
>  #include "gettext.h"
> @@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
>  int cmd_archive(int argc,
>  		const char **argv,
>  		const char *prefix,
> -		struct repository *repo UNUSED)
> +		struct repository *repo)
>  {
>  	const char *exec = "git-upload-archive";
>  	char *output = NULL;
> @@ -110,7 +109,7 @@ int cmd_archive(int argc,
>  
>  	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
>  
> -	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
> +	ret = write_archive(argc, argv, prefix, repo, output, 0);

This looks OK, but unlike the original, write_archive() now needs to
be prepared to see NULL in the repo parameter.  Is that reasonable?

Perdon me to think aloud for a bit.

The context before this hunk handles "git archive --remote" which
can be run outside a repository (and that is the whole reason why we
ask SETUP_GENTLY), but this part has to happen in a repository,
doesn't it?  Or is there some mode of operation of "git archive" I
am forgetting that can be done without a repository?

	... goes and looks ...

OK, write_archive() has its own setup_git_directory() call when
startup_info->have_repository is false, so this happens to be OK,
until the beginning part of archive.c:write_archive() will not
changed to start dereferencing "repo" pointer.

That sounds brittle, but probalby outside the scope of what this
patch series wants to address.  It also makes git_config() calls
even before it realizes there is no repository and dies, which
smells fishy without actually doing any harm.

So, after all, I think this step is probably OK.

Thanks.

John Cai Oct. 4, 2024, 8:05 p.m. UTC | #2

Hi Junio,

On 30 Sep 2024, at 16:01, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <[e-mail]> writes:
>
>> -#define USE_THE_REPOSITORY_VARIABLE
>>  #include "builtin.h"
>>  #include "archive.h"
>>  #include "gettext.h"
>> @@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
>>  int cmd_archive(int argc,
>>  		const char **argv,
>>  		const char *prefix,
>> -		struct repository *repo UNUSED)
>> +		struct repository *repo)
>>  {
>>  	const char *exec = "git-upload-archive";
>>  	char *output = NULL;
>> @@ -110,7 +109,7 @@ int cmd_archive(int argc,
>>
>>  	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
>>
>> -	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
>> +	ret = write_archive(argc, argv, prefix, repo, output, 0);
>
> This looks OK, but unlike the original, write_archive() now needs to
> be prepared to see NULL in the repo parameter.  Is that reasonable?
>
> Perdon me to think aloud for a bit.
>
> The context before this hunk handles "git archive --remote" which
> can be run outside a repository (and that is the whole reason why we
> ask SETUP_GENTLY), but this part has to happen in a repository,
> doesn't it?  Or is there some mode of operation of "git archive" I
> am forgetting that can be done without a repository?
>
>     ... goes and looks ...
>
> OK, write_archive() has its own setup_git_directory() call when
> startup_info->have_repository is false, so this happens to be OK,
> until the beginning part of archive.c:write_archive() will not
> changed to start dereferencing "repo" pointer.
>
> That sounds brittle, but probalby outside the scope of what this
> patch series wants to address.  It also makes git_config() calls
> even before it realizes there is no repository and dies, which
> smells fishy without actually doing any harm.
>
> So, after all, I think this step is probably OK.

Yeah I think these are issues we’ll need to address once removing the
the_repository global from archive code.

Thanks
John

>
> Thanks.

Patch

diff --git a/builtin/archive.c b/builtin/archive.c
index dc926d1a3df..13ea7308c8b 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -2,7 +2,6 @@ 
  * Copyright (c) 2006 Franck Bui-Huu
  * Copyright (c) 2006 Rene Scharfe
  */
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "archive.h"
 #include "gettext.h"
@@ -79,7 +78,7 @@  static int run_remote_archiver(int argc, const char **argv,
 int cmd_archive(int argc,
 		const char **argv,
 		const char *prefix,
-		struct repository *repo UNUSED)
+		struct repository *repo)
 {
 	const char *exec = "git-upload-archive";
 	char *output = NULL;
@@ -110,7 +109,7 @@  int cmd_archive(int argc,
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
+	ret = write_archive(argc, argv, prefix, repo, output, 0);
 
 out:
 	free(output);