{"thread":{"id":"35409","subject":"Re: [PATCH] submodule recursion in git-archive","startedAt":"2013-11-27T05:03:09Z","lastAt":"2013-11-27T05:03:09Z","messageCount":1,"participants":["Nick Townsend"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"231159","messageId":"94296AEE-D806-4B67-AAA6-CE57B2CACCBE@mac.com","threadId":"35409","inReplyTo":"0MWW00M0GODZPV00@nk11p03mm-asmtp002.mac.com","subject":"Re: [PATCH] submodule recursion in git-archive","fromName":"Nick Townsend","fromEmail":"nick.townsend@mac.com","sentAt":"2013-11-27T05:03:09Z","receivedAt":"2013-11-27T05:03:09Z","isPatch":true,"sender":{"key":"nick.townsend@mac.com","avatar":"https://avatars.githubusercontent.com/u/44087510?v=4"},"body":"On 26 Nov 2013, at 07:17, René Scharfe <l.s.r@web.de> wrote:\n\n> Am 26.11.2013 01:04, schrieb Nick Townsend:\n>> My first git patch - so shout out if I’ve got the etiquette wrong! Or\n>> of course if I’ve missed something.\n> \n> Thanks for the patches!  Please send only one per message (the second\n> one as a reply to the first one, or both as replies to a cover letter),\n> though -- that makes commenting on them much easier.\n> \n> Side note: Documentation/SubmittingPatches doesn't mention that (yet),\n> AFAICS.\n> \n>> Subject: [PATCH 1/2] submodule: add_submodule_odb() usability\n>> \n>> Although add_submodule_odb() is documented as being\n>> externally usable, it is declared static and also\n>> has incorrect documentation.\n>> \n>> This commit fixes those and makes no changes to\n>> existing code using them. All tests still pass.\n> \n> Sign-off missing (see Documentation/SubmittingPatches).\n> \n>> ---\n>> Documentation/technical/api-ref-iteration.txt | 4 ++--\n>> submodule.c                                   | 2 +-\n>> submodule.h                                   | 1 +\n>> 3 files changed, 4 insertions(+), 3 deletions(-)\n>> \n>> diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt\n>> index aa1c50f..cbee624 100644\n>> --- a/Documentation/technical/api-ref-iteration.txt\n>> +++ b/Documentation/technical/api-ref-iteration.txt\n>> @@ -50,10 +50,10 @@ submodules object database. You can do this by a code-snippet like\n>> this:\n>> \n>> \tconst char *path = \"path/to/submodule\"\n>> -\tif (!add_submodule_odb(path))\n>> +\tif (add_submodule_odb(path))\n>> \t\tdie(\"Error submodule '%s' not populated.\", path);\n>> \n>> -`add_submodule_odb()` will return an non-zero value on success. If you\n>> +`add_submodule_odb()` will return a zero value on success. If you\n> \n> \"return zero on success\" instead?\n\nI like the brevity of your suggestion. Again, I just used what was there…\n\n> \n>> do not do this you will get an error for each ref that it does not point\n>> to a valid object.\n>> \n>> diff --git a/submodule.c b/submodule.c\n>> index 1905d75..1ea46be 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -143,7 +143,7 @@ void stage_updated_gitmodules(void)\n>> \t\tdie(_(\"staging updated .gitmodules failed\"));\n>> }\n>> \n>> -static int add_submodule_odb(const char *path)\n>> +int add_submodule_odb(const char *path)\n>> {\n>> \tstruct strbuf objects_directory = STRBUF_INIT;\n>> \tstruct alternate_object_database *alt_odb;\n>> diff --git a/submodule.h b/submodule.h\n>> index 7beec48..3e3cdca 100644\n>> --- a/submodule.h\n>> +++ b/submodule.h\n>> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n>> \t\tstruct string_list *needs_pushing);\n>> int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n>> void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n>> +int add_submodule_odb(const char *path);\n>> \n>> #endif\n> \n>> Subject: [PATCH 2/2] archive: allow submodule recursion on git-archive\n>> \n>> When using git-archive to produce a dump of a\n>> repository, the existing code does not recurse\n>> into a submodule when it encounters it in the tree\n>> traversal. These changes add a command line flag\n>> that permits this.\n>> \n>> Note that the submodules must be updated in the\n>> repository, otherwise this cannot take place.\n>> \n>> The feature is disabled for remote repositories as\n>> the git_work_tree fails. This is a possible future\n>> enhancement.\n> \n> Hmm, curious.  Why does it fail?  I guess that happens with bare\n> repositories, only, right?  (Which are the most likely kind of remote\n> repos to encounter, of course.)\n\nI’m not sure why it failed - I didn’t think it should - but it did.\nSee discussion in other email.\n\n> \n>> Two additional fields are added to archiver_args:\n>>  * recurse  - a boolean indicator\n>>  * treepath - the path part of the tree-ish\n>>               eg. the 'www' in HEAD:www\n>> \n>> The latter is used within the archive writer to\n>> determin the correct path for the submodule .git\n>> file.\n>> \n>> Signed-off-by: Nick Townsend <nick.townsend@mac.com>\n>> ---\n>> Documentation/git-archive.txt |  9 +++++++++\n>> archive.c                     | 38 ++++++++++++++++++++++++++++++++++++--\n>> archive.h                     |  2 ++\n>> 3 files changed, 47 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt\n>> index b97aaab..b4df735 100644\n>> --- a/Documentation/git-archive.txt\n>> +++ b/Documentation/git-archive.txt\n>> @@ -11,6 +11,7 @@ SYNOPSIS\n>> [verse]\n>> 'git archive' [--format=<fmt>] [--list] [--prefix=<prefix>/] [<extra>]\n>> \t      [-o <file> | --output=<file>] [--worktree-attributes]\n>> +\t      [--recursive|--recurse-submodules]\n> \n> I'd expect git archive --recurse to add subdirectories and their\n> contents, which it does right now, and --no-recurse to only archive the\n> specified objects, which is not implemented.  IAW: I wouldn't normally\n> associate an option with that name with submodules.  Would\n> --recurse-submodules alone suffice?\n> \n> Side note: With only one of the options defined you could shorten them\n> on the command line to e.g. --rec; with both you'd need to type at least\n> --recursi or --recurse to disambiguate -- even though they ultimately do\n> the same.\n\nSee other comments - I agree - recurse-submodules it is!\n\n> \n>> \t      [--remote=<repo> [--exec=<git-upload-archive>]] <tree-ish>\n>> \t      [<path>...]\n>> \n>> @@ -51,6 +52,14 @@ OPTIONS\n>> --prefix=<prefix>/::\n>> \tPrepend <prefix>/ to each filename in the archive.\n>> \n>> +--recursive::\n>> +--recurse-submodules::\n>> +\tArchive entries in submodules. Errors occur if the submodules\n>> +\thave not been initialized and updated.\n>> +\tRun `git submodule update --init --recursive` immediately after\n>> +\tthe clone is finished to avoid this.\n>> +\tThis option is not available with remote repositories.\n>> +\n>> -o <file>::\n>> --output=<file>::\n>> \tWrite the archive to <file> instead of stdout.\n>> diff --git a/archive.c b/archive.c\n>> index 346f3b2..f6313c9 100644\n>> --- a/archive.c\n>> +++ b/archive.c\n>> @@ -5,6 +5,7 @@\n>> #include \"archive.h\"\n>> #include \"parse-options.h\"\n>> #include \"unpack-trees.h\"\n>> +#include \"submodule.h\"\n>> \n>> static char const * const archive_usage[] = {\n>> \tN_(\"git archive [options] <tree-ish> [<path>...]\"),\n>> @@ -131,13 +132,32 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>> \t\targs->convert = ATTR_TRUE(check[1].value);\n>> \t}\n>> \n>> +\tif (S_ISGITLINK(mode) && args->recurse) {\n>> +\t\tconst char *work_tree = get_git_work_tree();\n>> +\t\tif (!work_tree) {\n>> +\t\t\t  die(\"Can't go recursive when no work dir\");\n>> +\t\t}\n> \n> Style nit: No curly braces around single-line statements, please.\n> \n> Perhaps mention submodules in the error message?\n> \n> It would be nicer to get_git_work_tree right after the parameters have\n> been parsed and before any archive contents have been written and error\n> out early.\n> \n>> +\t\tstatic struct strbuf dotgit = STRBUF_INIT;\n> \n> We avoid declarations after statements because older compilers don't\n> support it.\n> \n> You release the memory at the end of this block; that means there's no\n> advantage in making this strbuf static.  Allocating and freeing the\n> memory for the path of each submodule shouldn't cause any performance\n> issues, so please just drop static from the declaration.\n> \n>> +\t\tstrbuf_reset(&dotgit);\n> \n> ... and then you don't need to reset anymore.\n> \n>> +\t\tstrbuf_grow(&dotgit, PATH_MAX);\n> \n> I'd drop that as well; the number of submodules should be low enough\n> that the possibly avoided reallocations by giving this hint shouldn't be\n> noticeable.\n\nI just cut and pasted this code from another location - didn’t really think too hard\nand I wasn’t familiar with the strbuf routines in the codebase. Thanks for the input.\n\n> \n>> +\t\tstrbuf_addstr(&dotgit, work_tree);\n>> +\t\tstrbuf_addch(&dotgit, '/');\n>> +\t\tif (args->treepath) {\n>> +\t\t\t  strbuf_addstr(&dotgit, args->treepath);\n>> +\t\t\t  strbuf_addch(&dotgit, '/');\n>> +\t\t}\n>> +\t\tstrbuf_add(&dotgit, path_without_prefix,strlen(path_without_prefix)-1);\n>> +\t\tif (add_submodule_odb(dotgit.buf))\n>> +\t\t\t  die(\"Can't add submodule: %s\", dotgit.buf);\n> \n> Hmm, I wonder if we can traverse the tree and load all submodule object\n> databases before traversing it again to actually write file contents.\n> That would spare the user from getting half of an archive together with\n> that error message.\n\nSee comments in another email about where and how to parse submodules. If we do the\nparse .gitmodules approach then we could do something like this. The as-you-go\nmethod seemed easier and more accurate - after all you may not traverse into a\nsubmodule at all - so any time spent adding it’s objects would be wasted.\n\nIn the same conversation I agree that it’s best to warn when a submodule can’t be\nlocated, and continue with just a directory entry in its place (as currently)\n\n> \n>> +\t\tstrbuf_release(&dotgit);\n>> +\t}\n>> \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n>> \t\tif (args->verbose)\n>> \t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>> \t\terr = write_entry(args, sha1, path.buf, path.len, mode);\n>> \t\tif (err)\n>> \t\t\treturn err;\n>> -\t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n>> +\t\treturn (S_ISGITLINK(mode) && !args->recurse) ? 0: READ_TREE_RECURSIVE;\n> \n> This line is longer than 80 characters, which we tend to avoid.  How\n> about this?\n> \n> \t\tif (ISGITLINK(mode) && !args->recurse)\n> \t\t\treturn 0;\n> \t\treturn READ_TREE_RECURSIVE;\n> \n>> \t}\n\nAgreed. I don’t usually enjoy multiple returns (especially so close to the end!)\nbut if the style guidelines are OK with that then so am I.\n\n>> \n>> \tif (args->verbose)\n>> @@ -256,10 +276,16 @@ static void parse_treeish_arg(const char **argv,\n>> \tconst struct commit *commit;\n>> \tunsigned char sha1[20];\n>> \n>> +\tconst char *colon = strchr(name, ':');\n>> +\n>> +\t/* Store the path on the ref for later (required for --recursive) */\n>> +\tchar *treepath = NULL;\n>> +\tif (colon) {\n>> +\t\ttreepath = strdup(colon+1);\n>> +\t}\n> \n> Style: No curly braces, space around operators, use wrapper functions\n> for simple memory error handling:\n> \n> \tif (colon)\n> \t\ttreepath = xstrdup(colon + 1);\n\nIs that the standard practice? To use the wrapper functions?  \n\n> \n>> \t/* Remotes are only allowed to fetch actual refs */\n>> \tif (remote) {\n>> \t\tchar *ref = NULL;\n>> -\t\tconst char *colon = strchr(name, ':');\n>> \t\tint refnamelen = colon ? colon - name : strlen(name);\n>> \n>> \t\tif (!dwim_ref(name, refnamelen, sha1, &ref))\n>> @@ -296,9 +322,11 @@ static void parse_treeish_arg(const char **argv,\n>> \t\ttree = parse_tree_indirect(tree_sha1);\n>> \t}\n>> \tar_args->tree = tree;\n>> +\tar_args->treepath = treepath;\n>> \tar_args->commit_sha1 = commit_sha1;\n>> \tar_args->commit = commit;\n>> \tar_args->time = archive_time;\n>> +\n>> }\n> \n> Please don't add empty lines before the end of a block.\n> \n>> #define OPT__COMPR(s, v, h, p) \\\n>> @@ -318,6 +346,7 @@ static int parse_archive_args(int argc, const char **argv,\n>> \tconst char *exec = NULL;\n>> \tconst char *output = NULL;\n>> \tint compression_level = -1;\n>> +\tint recurse = 0;\n>> \tint verbose = 0;\n>> \tint i;\n>> \tint list = 0;\n>> @@ -331,6 +360,8 @@ static int parse_archive_args(int argc, const char **argv,\n>> \t\t\tN_(\"write the archive to this file\")),\n>> \t\tOPT_BOOL(0, \"worktree-attributes\", &worktree_attributes,\n>> \t\t\tN_(\"read .gitattributes in working directory\")),\n>> +\t\tOPT_BOOL(0, \"recursive\", &recurse, N_(\"include submodules in archive\")),\n>> +\t\tOPT_BOOL(0, \"recurse-submodules\", &recurse, N_(\"include submodules in archive\")),\n>> \t\tOPT__VERBOSE(&verbose, N_(\"report archived files on stderr\")),\n>> \t\tOPT__COMPR('0', &compression_level, N_(\"store only\"), 0),\n>> \t\tOPT__COMPR('1', &compression_level, N_(\"compress faster\"), 1),\n>> @@ -355,6 +386,8 @@ static int parse_archive_args(int argc, const char **argv,\n>> \n>> \targc = parse_options(argc, argv, NULL, opts, archive_usage, 0);\n>> \n>> +\tif (is_remote && recurse)\n>> +\t\tdie(\"Cannot include submodules with option --remote\");\n>> \tif (remote)\n>> \t\tdie(\"Unexpected option --remote\");\n>> \tif (exec)\n>> @@ -393,6 +426,7 @@ static int parse_archive_args(int argc, const char **argv,\n>> \t\t\t\t\tformat, compression_level);\n>> \t\t}\n>> \t}\n>> +\targs->recurse = recurse;\n>> \targs->verbose = verbose;\n>> \targs->base = base;\n>> \targs->baselen = strlen(base);\n>> diff --git a/archive.h b/archive.h\n>> index 4a791e1..577238d 100644\n>> --- a/archive.h\n>> +++ b/archive.h\n>> @@ -7,10 +7,12 @@ struct archiver_args {\n>> \tconst char *base;\n>> \tsize_t baselen;\n>> \tstruct tree *tree;\n>> +\tconst char *treepath;\n>> \tconst unsigned char *commit_sha1;\n>> \tconst struct commit *commit;\n>> \ttime_t time;\n>> \tstruct pathspec pathspec;\n>> +\tunsigned int recurse : 1;\n> \n> Name it recurse_submodules?\n> \n>> \tunsigned int verbose : 1;\n>> \tunsigned int worktree_attributes : 1;\n>> \tunsigned int convert : 1;\n>> -- 1.8.3.4 (Apple Git-47) \n> \n> A test script (t/t5005-archive-submodules.sh?) would be nice which\n> exercises the new option.\n\nI take all the style points noted, and will create a test script - I didn’t want to invest too much\nin this project until I’d received a response!\n\nThanks for your time in reviewing - these things are a great help.\n\nCheers\nNick\n\n> \n> René\n"}]}