Hi Junio,

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

"John Cai via GitGitGadget" [off-list ref] writes:

quoted

-#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.