Hi Junio,
On 30 Sep 2024, at 16:01, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
