Re: [PATCH v3 8/9] archive: add --recurse-submodules to git-archive command
- From
Glen Choo <chooglen@google.com>
- Date
- Oct 27, 2022, 17:33 UTC
- Message-ID
- <kl6l8rl1qihg.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <fa90e7cf-4ddc-573a-1859-9e2b6b203f95@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 48 quoted lines
> Am 27.10.22 um 01:34 schrieb Glen Choo:
>> "Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> index 34549d849f1..f81ef741487 100644
>>> --- a/archive.c
>>> +++ b/archive.c
>>> @@ -213,6 +214,25 @@ static void queue_directory(const struct object_id *oid,
>>> oidcpy(&d->oid, oid);
>>> }
>>>
>>> +static void queue_submodule(
>>> + struct repository *superproject,
>>> + const struct object_id *oid,
>>> + struct strbuf *base, const char *filename,
>>> + unsigned mode, struct archiver_context *c)
>>> +{
>>> + struct repository subrepo;
>>> +
>>> + if (repo_submodule_init(&subrepo, superproject, filename, null_oid()))
>>> + return;
>>> +
>>> + if (repo_read_index(&subrepo) < 0)
>>> + die("index file corrupt");
>>> +
>>> + queue_directory(oid, base, filename, mode, c);
>>> +
>>> + repo_clear(&subrepo);
>>> +}
>>> +
>
>> What's much more surprising is that you can delete the entire function
>> body (even queue_directory()!) and the tests still pass! The tests are
>> definitely testing what they say they are (I've also checked the
>> tarballs), so I'm not sure what's going on.
>>
>> I commented out queue_directory() in the S_ISDIR case, and the only test
>> failures I saw were:
>>
>> - t5000.68, which uses a glob in its pathspec. I tried using a glob for
>> in the archive submodule tests, but I couldn't reproduce the failure.
>> - t5004.11, which is a really big test case that I didn't bother looking
>> deeply into.
>>
>> So I'm at a loss as to what queue_directory() actually does.
> An archive doesn't strictly need directory entries. If it contains a
> file with a deeply nested path then extractors will create the parent
> directory hierarchy regardless. diff(1) won't notice any difference.
> Directory entries are mainly included to specify the permission bits.Thanks. In that case, we should probably also test the case where there are empty directories (e.g. when a file is excluded by a pathspec), and we should also check the permission bits.
Show 6 quoted lines
> > t5000.68 checks for the directory entries in the output given by the > option --verbose of git archive. t5004.11 checks the number of archive > entries (including directories) using "zipinfo -h". > > René