Re: [PATCH v3 8/9] archive: add --recurse-submodules to git-archive command
- From
Glen Choo <chooglen@google.com>
- Date
- Oct 26, 2022, 23:34 UTC
- Message-ID
- <kl6lh6zqqhvm.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <4672e3d958625cd76eb8056ab434e9a37f52661e.1665973401.git.gitgitgadget@gmail.com>
"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 26 quoted lines
> 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);
> +}
> +This bit is puzzling to me because we init the submodule, read its index, and then don't read objects from it at all. How does this work when we aren't reading objects from the submodule we init here? My guess is that read_tree() is already doing the heavy lifting of recursing into submodules, so we don't need to worry any more about init-ing submodules in archive.c, which is great.
So in effect, this is just checking whether we can read the submodule and its index. We can drop this check since we already do that check in read_tree().
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. My best guess at a reproduction would be to make a subdirectory in t5000.68 a submodule. If we do find such a reproducing case, we should add it to the test suite.
Show 26 quoted lines
> static int write_directory(
> struct repository *repo,
> struct archiver_context *c)
> @@ -228,9 +248,11 @@ static int write_directory(
> write_directory(repo, c) ||
> write_archive_entry(repo, &d->oid, d->path, d->baselen,
> d->path + d->baselen, d->mode,
> - c) != READ_TREE_RECURSIVE;
> + c);
> free(d);
> - return ret ? -1 : 0;
> + if (ret == READ_TREE_RECURSIVE)
> + return 0;
> + return ret;
> }
>
> static int queue_or_write_archive_entry(
> @@ -263,6 +285,11 @@ static int queue_or_write_archive_entry(
> return 0;
> queue_directory(oid, base, filename, mode, c);
> return READ_TREE_RECURSIVE;
> + } else if (c->args->recurse_submodules && S_ISGITLINK(mode)) {
> + if (is_submodule_active(r, filename)) {
> + queue_submodule(r, oid, base, filename, mode, c);
> + return READ_TREE_RECURSIVE;
> + }If we are omitting inactive submodules from the archive, we should test this behavior.
Show 50 quoted lines
> }
>
> if (write_directory(r, c))
> @@ -446,6 +473,7 @@ static void parse_pathspec_arg(
> PATHSPEC_PREFER_FULL,
> "", pathspec);
> ar_args->pathspec.recursive = 1;
> + ar_args->pathspec.recurse_submodules = ar_args->recurse_submodules;
> if (pathspec) {
> while (*pathspec) {
> if (**pathspec && !path_exists(repo, ar_args, *pathspec))
> @@ -609,6 +637,7 @@ static int parse_archive_args(int argc, const char **argv,
> int verbose = 0;
> int i;
> int list = 0;
> + int recurse_submodules = 0;
> int worktree_attributes = 0;
> struct option opts[] = {
> OPT_GROUP(""),
> @@ -623,6 +652,8 @@ static int parse_archive_args(int argc, const char **argv,
> add_file_cb, (intptr_t)&base },
> OPT_STRING('o', "output", &output, N_("file"),
> N_("write the archive to this file")),
> + OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
> + N_("include submodules in archive")),
> OPT_BOOL(0, "worktree-attributes", &worktree_attributes,
> N_("read .gitattributes in working directory")),
> OPT__VERBOSE(&verbose, N_("report archived files on stderr")),
> @@ -686,6 +717,7 @@ static int parse_archive_args(int argc, const char **argv,
> args->verbose = verbose;
> args->base = base;
> args->baselen = strlen(base);
> + args->recurse_submodules = recurse_submodules;
> args->worktree_attributes = worktree_attributes;
>
> return argc;
> diff --git a/archive.h b/archive.h
> index 540a3b12130..1b21484dda6 100644
> --- a/archive.h
> +++ b/archive.h
> @@ -18,6 +18,7 @@ struct archiver_args {
> timestamp_t time;
> struct pathspec pathspec;
> unsigned int verbose : 1;
> + unsigned int recurse_submodules : 1;
> unsigned int worktree_attributes : 1;
> unsigned int convert : 1;
> int compression_level;
> --
> gitgitgadget