{"thread":{"id":"44526","subject":"[PATCHv2 0/3] submodule-config: clarify/cleanup docs and header","startedAt":"2016-11-21T23:27:16Z","lastAt":"2016-11-22T20:03:39Z","messageCount":12,"participants":["Stefan Beller","Brandon Williams","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"306263","messageId":"20161121232709.8906-1-sbeller@google.com","threadId":"44526","inReplyTo":null,"subject":"[PATCHv2 0/3] submodule-config: clarify/cleanup docs and header","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T23:27:06Z","receivedAt":"2016-11-21T23:27:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"replacing sb/submodule-config-cleanup\n\nv2:\naddressed Jacobs concerns in patch2, fixing all occurrences of commit_sha1.\n\nThanks,\nStefan\n\ninterdiff to v1:\ndiff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt\nindex 1df7a827ff..a91c1f085e 100644\n--- a/Documentation/technical/api-submodule-config.txt\n+++ b/Documentation/technical/api-submodule-config.txt\n@@ -49,7 +49,7 @@ Functions\n \n `const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::\n \n- Lookup values for one submodule by its commit_sha1 and path.\n+ Lookup values for one submodule by its commit_or_tree and path.\n \n `const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::\n\n\nv1:\nA small series that would have helped me understand the submodule config\nonce again.\n\nThanks,\nStefan\n\nStefan Beller (3):\n submodule config: inline config_from_{name, path}\n submodule-config: rename commit_sha1 to commit_or_tree\n submodule-config: clarify parsing of null_sha1 element\n\n Documentation/technical/api-submodule-config.txt | 13 ++++--\n submodule-config.c | 59 ++++++++++--------------\n submodule-config.h | 4 +-\n t/t7411-submodule-config.sh | 11 +++++\n 4 files changed, 45 insertions(+), 42 deletions(-)\n\n-- \n2.11.0.rc2.18.g0126045.dirty\n\n"},{"id":"306264","messageId":"20161121232709.8906-2-sbeller@google.com","threadId":"44526","inReplyTo":"20161121232709.8906-1-sbeller@google.com","subject":"[PATCHv2 1/3] submodule config: inline config_from_{name, path}","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T23:27:07Z","receivedAt":"2016-11-21T23:27:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There is no other user of config_from_{name, path}, such that there is no\nreason for the existence of these one liner functions. Just inline these\nto increase readability.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule-config.c | 16 ++--------------\n 1 file changed, 2 insertions(+), 14 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 098085be69..15ffab6af4 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -471,18 +471,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \treturn submodule;\n }\n \n-static const struct submodule *config_from_path(struct submodule_cache *cache,\n-\t\tconst unsigned char *commit_sha1, const char *path)\n-{\n-\treturn config_from(cache, commit_sha1, path, lookup_path);\n-}\n-\n-static const struct submodule *config_from_name(struct submodule_cache *cache,\n-\t\tconst unsigned char *commit_sha1, const char *name)\n-{\n-\treturn config_from(cache, commit_sha1, name, lookup_name);\n-}\n-\n static void ensure_cache_init(void)\n {\n \tif (is_cache_init)\n@@ -508,14 +496,14 @@ const struct submodule *submodule_from_name(const unsigned char *commit_sha1,\n \t\tconst char *name)\n {\n \tensure_cache_init();\n-\treturn config_from_name(&the_submodule_cache, commit_sha1, name);\n+\treturn config_from(&the_submodule_cache, commit_sha1, name, lookup_name);\n }\n \n const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n \t\tconst char *path)\n {\n \tensure_cache_init();\n-\treturn config_from_path(&the_submodule_cache, commit_sha1, path);\n+\treturn config_from(&the_submodule_cache, commit_sha1, path, lookup_path);\n }\n \n void submodule_free(void)\n-- \n2.11.0.rc2.18.g0126045.dirty\n\n"},{"id":"306265","messageId":"20161121232709.8906-3-sbeller@google.com","threadId":"44526","inReplyTo":"20161121232709.8906-1-sbeller@google.com","subject":"[PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T23:27:08Z","receivedAt":"2016-11-21T23:27:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It is also possible to pass in a tree hash to lookup a submodule config.\nMake it clear by naming the variables accrodingly. Looking up a submodule\nconfig by tree hash will come in handy in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/technical/api-submodule-config.txt | 8 ++--\n submodule-config.c | 47 ++++++++++++------------\n submodule-config.h | 4 +-\n t/t7411-submodule-config.sh | 11 ++++++\n 4 files changed, 41 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt\nindex 941fa178dd..768458580f 100644\n--- a/Documentation/technical/api-submodule-config.txt\n+++ b/Documentation/technical/api-submodule-config.txt\n@@ -47,15 +47,15 @@ Functions\n \tCan be passed to the config parsing infrastructure to parse\n \tlocal (worktree) submodule configurations.\n \n-`const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`::\n+`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::\n \n-\tLookup values for one submodule by its commit_sha1 and path.\n+\tLookup values for one submodule by its commit_or_tree and path.\n \n-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::\n+`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::\n \n \tThe same as above but lookup by name.\n \n-If given the null_sha1 as commit_sha1 the local configuration of a\n+If given the null_sha1 as commit_or_tree the local configuration of a\n submodule will be returned (e.g. consolidated values from local git\n configuration and the .gitmodules file in the worktree).\n \ndiff --git a/submodule-config.c b/submodule-config.c\nindex 15ffab6af4..4c5f5d074b 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -263,12 +263,12 @@ int parse_push_recurse_submodules_arg(const char *opt, const char *arg)\n \treturn parse_push_recurse(opt, arg, 1);\n }\n \n-static void warn_multiple_config(const unsigned char *commit_sha1,\n+static void warn_multiple_config(const unsigned char *commit_or_tree,\n \t\t\t\t const char *name, const char *option)\n {\n \tconst char *commit_string = \"WORKTREE\";\n-\tif (commit_sha1)\n-\t\tcommit_string = sha1_to_hex(commit_sha1);\n+\tif (commit_or_tree)\n+\t\tcommit_string = sha1_to_hex(commit_or_tree);\n \twarning(\"%s:.gitmodules, multiple configurations found for \"\n \t\t\t\"'submodule.%s.%s'. Skipping second one!\",\n \t\t\tcommit_string, name, option);\n@@ -276,7 +276,7 @@ static void warn_multiple_config(const unsigned char *commit_sha1,\n \n struct parse_config_parameter {\n \tstruct submodule_cache *cache;\n-\tconst unsigned char *commit_sha1;\n+\tconst unsigned char *commit_or_tree;\n \tconst unsigned char *gitmodules_sha1;\n \tint overwrite;\n };\n@@ -300,7 +300,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->path)\n-\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\twarn_multiple_config(me->commit_or_tree, submodule->name,\n \t\t\t\t\t\"path\");\n \t\telse {\n \t\t\tif (submodule->path)\n@@ -314,7 +314,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n \t\t submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n-\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\twarn_multiple_config(me->commit_or_tree, submodule->name,\n \t\t\t\t\t\"fetchrecursesubmodules\");\n \t\telse\n \t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n@@ -324,7 +324,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->ignore)\n-\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\twarn_multiple_config(me->commit_or_tree, submodule->name,\n \t\t\t\t\t\"ignore\");\n \t\telse if (strcmp(value, \"untracked\") &&\n \t\t\t strcmp(value, \"dirty\") &&\n@@ -340,7 +340,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n \t\t} else if (!me->overwrite && submodule->url) {\n-\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\twarn_multiple_config(me->commit_or_tree, submodule->name,\n \t\t\t\t\t\"url\");\n \t\t} else {\n \t\t\tfree((void *) submodule->url);\n@@ -351,21 +351,21 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite &&\n \t\t\t submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)\n-\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\twarn_multiple_config(me->commit_or_tree, submodule->name,\n \t\t\t\t\t \"update\");\n \t\telse if (parse_submodule_update_strategy(value,\n \t\t\t &submodule->update_strategy) < 0)\n \t\t\t\tdie(_(\"invalid value for %s\"), var);\n \t} else if (!strcmp(item.buf, \"shallow\")) {\n \t\tif (!me->overwrite && submodule->recommend_shallow != -1)\n-\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\twarn_multiple_config(me->commit_or_tree, submodule->name,\n \t\t\t\t\t \"shallow\");\n \t\telse\n \t\t\tsubmodule->recommend_shallow =\n \t\t\t\tgit_config_bool(var, value);\n \t} else if (!strcmp(item.buf, \"branch\")) {\n \t\tif (!me->overwrite && submodule->branch)\n-\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\twarn_multiple_config(me->commit_or_tree, submodule->name,\n \t\t\t\t\t \"branch\");\n \t\telse {\n \t\t\tfree((void *)submodule->branch);\n@@ -379,18 +379,18 @@ static int parse_config(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n-static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n+static int gitmodule_sha1_from_commit(const unsigned char *commit_or_tree,\n \t\t\t\t unsigned char *gitmodules_sha1,\n \t\t\t\t struct strbuf *rev)\n {\n \tint ret = 0;\n \n-\tif (is_null_sha1(commit_sha1)) {\n+\tif (is_null_sha1(commit_or_tree)) {\n \t\thashclr(gitmodules_sha1);\n \t\treturn 1;\n \t}\n \n-\tstrbuf_addf(rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n+\tstrbuf_addf(rev, \"%s:.gitmodules\", sha1_to_hex(commit_or_tree));\n \tif (get_sha1(rev->buf, gitmodules_sha1) >= 0)\n \t\tret = 1;\n \n@@ -402,7 +402,7 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n * revisions.\n */\n static const struct submodule *config_from(struct submodule_cache *cache,\n-\t\tconst unsigned char *commit_sha1, const char *key,\n+\t\tconst unsigned char *commit_or_tree, const char *key,\n \t\tenum lookup_type lookup_type)\n {\n \tstruct strbuf rev = STRBUF_INIT;\n@@ -418,7 +418,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t * return the first submodule. Can be used to check whether\n \t * there are any submodules parsed.\n \t */\n-\tif (!commit_sha1 || !key) {\n+\tif (!commit_or_tree || !key) {\n \t\tstruct hashmap_iter iter;\n \t\tstruct submodule_entry *entry;\n \n@@ -428,7 +428,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\treturn entry->config;\n \t}\n \n-\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev))\n+\tif (!gitmodule_sha1_from_commit(commit_or_tree, sha1, &rev))\n \t\tgoto out;\n \n \tswitch (lookup_type) {\n@@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \n \t/* fill the submodule config into the cache */\n \tparameter.cache = cache;\n-\tparameter.commit_sha1 = commit_sha1;\n+\t// todo: get the actual tree here:\n+\tparameter.commit_or_tree = commit_or_tree;\n \tparameter.gitmodules_sha1 = sha1;\n \tparameter.overwrite = 0;\n \tgit_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,\n@@ -484,7 +485,7 @@ int parse_submodule_config_option(const char *var, const char *value)\n {\n \tstruct parse_config_parameter parameter;\n \tparameter.cache = &the_submodule_cache;\n-\tparameter.commit_sha1 = NULL;\n+\tparameter.commit_or_tree = NULL;\n \tparameter.gitmodules_sha1 = null_sha1;\n \tparameter.overwrite = 1;\n \n@@ -492,18 +493,18 @@ int parse_submodule_config_option(const char *var, const char *value)\n \treturn parse_config(var, value, ¶meter);\n }\n \n-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,\n+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,\n \t\tconst char *name)\n {\n \tensure_cache_init();\n-\treturn config_from(&the_submodule_cache, commit_sha1, name, lookup_name);\n+\treturn config_from(&the_submodule_cache, commit_or_tree, name, lookup_name);\n }\n \n-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,\n \t\tconst char *path)\n {\n \tensure_cache_init();\n-\treturn config_from(&the_submodule_cache, commit_sha1, path, lookup_path);\n+\treturn config_from(&the_submodule_cache, commit_or_tree, path, lookup_path);\n }\n \n void submodule_free(void)\ndiff --git a/submodule-config.h b/submodule-config.h\nindex d05c542d2c..99df8e593c 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -25,9 +25,9 @@ struct submodule {\n int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);\n int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n int parse_submodule_config_option(const char *var, const char *value);\n-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,\n+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,\n \t\tconst char *name);\n-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,\n \t\tconst char *path);\n void submodule_free(void);\n \ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 47562ce465..301ed5e48f 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -93,6 +93,17 @@ test_expect_success 'error message contains blob reference' '\n \t)\n '\n \n+test_expect_success 'using tree sha1 works' '\n+\t(\n+\t\tcd super &&\n+\t\ttree=$(git rev-parse HEAD^{tree}) &&\n+\t\tcommit=$(git rev-parse HEAD^{commit}) &&\n+\t\ttest-submodule-config $commit b >expect &&\n+\t\ttest-submodule-config $tree b >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n cat >super/expect_url <\nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/api-submodule-config.txt | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt\nindex 768458580f..a91c1f085e 100644\n--- a/Documentation/technical/api-submodule-config.txt\n+++ b/Documentation/technical/api-submodule-config.txt\n@@ -55,8 +55,11 @@ Functions\n \n \tThe same as above but lookup by name.\n \n-If given the null_sha1 as commit_or_tree the local configuration of a\n-submodule will be returned (e.g. consolidated values from local git\n+Whenever a submodule configuration is parsed in `parse_submodule_config_option`\n+via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1\n+zeroed out. So in the normal case, when HEAD:.gitmodules is parsed first and\n+then overlayed with the repository configuration, the null_sha1 entry contains\n+the local configuration of a submodule (e.g. consolidated values from local git\n configuration and the .gitmodules file in the worktree).\n \n For an example usage see test-submodule-config.c.\n-- \n2.11.0.rc2.18.g0126045.dirty\n\n"},{"id":"306269","messageId":"20161122000944.GC149321@google.com","threadId":"44526","inReplyTo":"20161121232709.8906-4-sbeller@google.com","subject":"Re: [PATCHv2 3/3] submodule-config: clarify parsing of null_sha1 element","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-22T00:09:44Z","receivedAt":"2016-11-22T00:09:52Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/21, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/technical/api-submodule-config.txt | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt\n> index 768458580f..a91c1f085e 100644\n> --- a/Documentation/technical/api-submodule-config.txt\n> +++ b/Documentation/technical/api-submodule-config.txt\n> @@ -55,8 +55,11 @@ Functions\n> \n> \tThe same as above but lookup by name.\n> \n> -If given the null_sha1 as commit_or_tree the local configuration of a\n> -submodule will be returned (e.g. consolidated values from local git\n> +Whenever a submodule configuration is parsed in `parse_submodule_config_option`\n> +via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1\n\ns/will be overwrite/will overwrite\n\n> +zeroed out. So in the normal case, when HEAD:.gitmodules is parsed first and\n> +then overlayed with the repository configuration, the null_sha1 entry contains\n> +the local configuration of a submodule (e.g. consolidated values from local git\n> configuration and the .gitmodules file in the worktree).\n> \n> For an example usage see test-submodule-config.c.\n> -- \n> 2.11.0.rc2.18.g0126045.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"306270","messageId":"20161122001148.GD149321@google.com","threadId":"44526","inReplyTo":"20161121232709.8906-3-sbeller@google.com","subject":"Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-22T00:11:48Z","receivedAt":"2016-11-22T00:11:55Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/21, Stefan Beller wrote:\n> \n> \tswitch (lookup_type) {\n> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> \n> \t/* fill the submodule config into the cache */\n> \tparameter.cache = cache;\n> -\tparameter.commit_sha1 = commit_sha1;\n> +\t// todo: get the actual tree here:\n\ns/todo/TODO\n\nMakes it more clear that this is a TODO\n\n\n-- \nBrandon Williams\n"},{"id":"306272","messageId":"CAGZ79kY+0WHFgXfVmEmw7H-qwuUcWcSssRyMzg2XqsGSDq+FCw@mail.gmail.com","threadId":"44526","inReplyTo":"20161122001148.GD149321@google.com","subject":"Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-22T00:15:32Z","receivedAt":"2016-11-22T00:15:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams wrote:\n> On 11/21, Stefan Beller wrote:\n>>\n>> switch (lookup_type) {\n>> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n>>\n>> /* fill the submodule config into the cache */\n>> parameter.cache = cache;\n>> - parameter.commit_sha1 = commit_sha1;\n>> + // todo: get the actual tree here:\n>\n> s/todo/TODO\n>\n> Makes it more clear that this is a TODO\n>\n\nThe // is more annoying here. I'll review these changes closely\nbefore sending out v3.\n"},{"id":"306273","messageId":"20161122001658.GE149321@google.com","threadId":"44526","inReplyTo":"CAGZ79kY+0WHFgXfVmEmw7H-qwuUcWcSssRyMzg2XqsGSDq+FCw@mail.gmail.com","subject":"Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-22T00:16:58Z","receivedAt":"2016-11-22T00:17:05Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/21, Stefan Beller wrote:\n> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams wrote:\n> > On 11/21, Stefan Beller wrote:\n> >>\n> >> switch (lookup_type) {\n> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> >>\n> >> /* fill the submodule config into the cache */\n> >> parameter.cache = cache;\n> >> - parameter.commit_sha1 = commit_sha1;\n> >> + // todo: get the actual tree here:\n> >\n> > s/todo/TODO\n> >\n> > Makes it more clear that this is a TODO\n> >\n> \n> The // is more annoying here. I'll review these changes closely\n> before sending out v3.\n\nWell I prefer // to /* */ but that's not the style we use :)\n\n-- \nBrandon Williams\n"},{"id":"306276","messageId":"CAGZ79kZL9_FUbePBf26diOykPaux5PHOosnaOwzfRevoeLqZFQ@mail.gmail.com","threadId":"44526","inReplyTo":"20161122001658.GE149321@google.com","subject":"Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-22T01:25:22Z","receivedAt":"2016-11-22T01:25:28Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 4:16 PM, Brandon Williams wrote:\n> On 11/21, Stefan Beller wrote:\n>> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams wrote:\n>> > On 11/21, Stefan Beller wrote:\n>> >>\n>> >> switch (lookup_type) {\n>> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n>> >>\n>> >> /* fill the submodule config into the cache */\n>> >> parameter.cache = cache;\n>> >> - parameter.commit_sha1 = commit_sha1;\n>> >> + // todo: get the actual tree here:\n>> >\n>> > s/todo/TODO\n>> >\n>> > Makes it more clear that this is a TODO\n>> >\n>>\n>> The // is more annoying here. I'll review these changes closely\n>> before sending out v3.\n>\n> Well I prefer // to /* */ but that's not the style we use :)\n\nbackground: Initially I assume we would need to do work here as that part\nis exposed to the user in error messages, such that we maybe want to\ngo the reverse direction and not state a tree but instead the commit containing\nit. Since then I decided that it is not worth to optimize for some\nhypothetical future\nand hence I did not go with the internal todo note I put there. Then I\nforgot about\nit and it just showed up in the patch here.\n\nHaving looked through the patch again, the rest looks clean to me.\n\nStefan\n\n>\n> --\n> Brandon Williams\n"},{"id":"306283","messageId":"xmqqd1hogpo1.fsf@gitster.mtv.corp.google.com","threadId":"44526","inReplyTo":"20161121232709.8906-2-sbeller@google.com","subject":"Re: [PATCHv2 1/3] submodule config: inline config_from_{name, path}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T03:27:10Z","receivedAt":"2016-11-22T03:27:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> There is no other user of config_from_{name, path}, such that there is no\n> reason for the existence of these one liner functions. Just inline these\n> to increase readability.\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n\nMakes sense.\n"},{"id":"306284","messageId":"xmqq8tscgp75.fsf@gitster.mtv.corp.google.com","threadId":"44526","inReplyTo":"20161121232709.8906-3-sbeller@google.com","subject":"Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T03:37:18Z","receivedAt":"2016-11-22T03:37:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It is also possible to pass in a tree hash to lookup a submodule config.\n> Make it clear by naming the variables accrodingly. Looking up a submodule\n> config by tree hash will come in handy in a later patch.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nYeah, I noticed that while reading the previous round, too, but...\n\n> -`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::\n> +`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::\n> \n> \tThe same as above but lookup by name.\n\nDoesn't (the) \"above\", aka submodule_from_path() want the same\ntreatment? \n\nAlso the explanation of \"above\" has room for improvement. Namely it\nsays:\n\n Lookup values for one submodule by its commit_sha1 and path.\n\nI do not think the commit-sha1 (or commit-or-tree-object-name) is\n\"ITS\" object name for the submodule. The name belongs to the\ncontaining superproject commit (or tree), no?\n\n Given a tree-ish in the superproject and a path, return the\n submodule that is bound at the path in the named tree.\n\nis what I would write for that one. Thinking about it a bit\nfurther, \"treeish_name\" would be a much better name for the variable\nthan \"commit_or_tree\", as \"treeish\" is an established short and\nsweet word that means \"commit_or_tree\", and having a \"name\"\nsomewhere in the variable name makes it clear that we are not\npassing the pointer to an in-core object (e.g. \"struct commit *\").\n\n> +test_expect_success 'using tree sha1 works' '\n> +\t(\n> +\t\tcd super &&\n> +\t\ttree=$(git rev-parse HEAD^{tree}) &&\n> +\t\tcommit=$(git rev-parse HEAD^{commit}) &&\n> +\t\ttest-submodule-config $commit b >expect &&\n> +\t\ttest-submodule-config $tree b >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\nPerhaps also test a tag that points at the commit?\n"},{"id":"306347","messageId":"CAGZ79kavcVs2utBut=tHaKoWzRJSEwBpEjZR9Eh=BbH8Bh_NiQ@mail.gmail.com","threadId":"44526","inReplyTo":"xmqq8tscgp75.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-22T20:03:31Z","receivedAt":"2016-11-22T20:03:39Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 7:37 PM, Junio C Hamano wrote:\n\n> Doesn't (the) \"above\", aka submodule_from_path() want the same\n> treatment?\n\nfixed in a resend.\n\n> I do not think the commit-sha1 (or commit-or-tree-object-name) is\n> \"ITS\" object name for the submodule. The name belongs to the\n> containing superproject commit (or tree), no?\n>\n> Given a tree-ish in the superproject and a path, return the\n> submodule that is bound at the path in the named tree.\n\nI'll take this.\n\n>\n> is what I would write for that one. Thinking about it a bit\n> further, \"treeish_name\" would be a much better name for the variable\n\nok, so treeish_name it is.\n\n>> +'\n>\n> Perhaps also test a tag that points at the commit?\n\nwill fix in a resend.\n"}]}