{"thread":{"id":"42955","subject":"[PATCHv3 0/7] submodule update: allow '.' for branch value","startedAt":"2016-07-29T00:44:20Z","lastAt":"2016-08-03T20:22:33Z","messageCount":12,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"292475","messageId":"20160729004409.2072-1-sbeller@google.com","threadId":"42955","inReplyTo":null,"subject":"[PATCHv3 0/7] submodule update: allow '.' for branch value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:02Z","receivedAt":"2016-07-29T00:44:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This got bigger than expected, but I am happier with the results.\n\nThe meat is found in the last patch. (At least what I am interested in; others\nmay be more interested in the second patch which could be argued to be a real\nbug fix to be merged down to maint.)\n\nThanks Junio for the thourough review of the first patches,\nStefan\n\nStefan Beller (7):\n  t7406: future proof tests with hard coded depth\n  submodule update: respect depth in subsequent fetches\n  submodule update: narrow scope of local variable\n  submodule--helper: fix usage string for relative-path\n  submodule-config: keep configured branch around\n  submodule--helper: add remote-branch helper\n  submodule update: allow '.' for branch value\n\n builtin/submodule--helper.c | 48 ++++++++++++++++++++++++++++--\n git-submodule.sh            | 11 +++----\n submodule-config.c          | 11 ++++++-\n submodule-config.h          |  1 +\n t/t7406-submodule-update.sh | 71 +++++++++++++++++++++++++++++++++++++++------\n 5 files changed, 125 insertions(+), 17 deletions(-)\n\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292476","messageId":"20160729004409.2072-2-sbeller@google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"[PATCHv3 1/7] t7406: future proof tests with hard coded depth","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:03Z","receivedAt":"2016-07-29T00:44:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The prior hard coded depth was chosen to be exactly the length from the\nrecorded gitlink to the tip of the remote, so if you add more commits\nto the remote before, this test will not test its intention any more.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7406-submodule-update.sh | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 88e9750..8fc3a25 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -841,16 +841,19 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'\n '\n \n test_expect_success 'submodule update clone shallow submodule' '\n+\ttest_when_finished \"rm -rf super3\" &&\n+\tfirst=$(git -C cloned submodule status submodule |cut -c2-41) &&\n+\tsecond=$(git -C submodule rev-parse HEAD) &&\n+\tcommit_count=$(git -C submodule rev-list $first^..$second | wc -l) &&\n \tgit clone cloned super3 &&\n \tpwd=$(pwd) &&\n-\t(cd super3 &&\n-\t sed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n-\t mv -f .gitmodules.tmp .gitmodules &&\n-\t git submodule update --init --depth=3\n-\t (cd submodule &&\n-\t  test 1 = $(git log --oneline | wc -l)\n-\t )\n-)\n+\t(\n+\t\tcd super3 &&\n+\t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n+\t\tmv -f .gitmodules.tmp .gitmodules &&\n+\t\tgit submodule update --init --depth=$commit_count &&\n+\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t)\n '\n \n test_expect_success 'submodule update --recursive drops module name before recursing' '\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292477","messageId":"20160729004409.2072-3-sbeller@google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"[PATCHv3 2/7] submodule update: respect depth in subsequent fetches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:04Z","receivedAt":"2016-07-29T00:44:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When depth is given the user may have a reasonable expectation that\nany remote operation is using the given depth. Add a test to demonstrate\nwe still get the desired sha1 even if the depth is too short to\ninclude the actual commit.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh            |  9 +++++----\n t/t7406-submodule-update.sh | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4ec7546..174f4ea 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -481,7 +481,8 @@ fetch_in_submodule () (\n \t'')\n \t\tgit fetch ;;\n \t*)\n-\t\tgit fetch $(get_default_remote) \"$2\" ;;\n+\t\tshift\n+\t\tgit fetch $(get_default_remote) \"$@\" ;;\n \tesac\n )\n \n@@ -619,7 +620,7 @@ cmd_update()\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n \t\t\t\t# Fetch remote before determining tracking $sha1\n-\t\t\t\tfetch_in_submodule \"$sm_path\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n \t\t\tfi\n \t\t\tremote_name=$(sanitize_submodule_env; cd \"$sm_path\" && get_default_remote)\n@@ -642,13 +643,13 @@ cmd_update()\n \t\t\t\t# Run fetch only if $sha1 isn't present or it\n \t\t\t\t# is not reachable from a ref.\n \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \n \t\t\t\t# Now we tried the usual fetch, but $sha1 may\n \t\t\t\t# not be reachable from any of the refs\n \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" \"$sha1\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" $depth \"$sha1\" ||\n \t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n \t\t\tfi\n \ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 8fc3a25..1bb1f43 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -856,6 +856,22 @@ test_expect_success 'submodule update clone shallow submodule' '\n \t)\n '\n \n+test_expect_success 'submodule update clone shallow submodule outside of depth' '\n+\ttest_when_finished \"rm -rf super3\" &&\n+\tgit clone cloned super3 &&\n+\tpwd=$(pwd) &&\n+\t(\n+\t\tcd super3 &&\n+\t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n+\t\tmv -f .gitmodules.tmp .gitmodules &&\n+\t\ttest_must_fail git submodule update --init --depth=1 2>actual &&\n+\t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n+\t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n+\t\tgit submodule update --init --depth=1 >actual &&\n+\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t)\n+'\n+\n test_expect_success 'submodule update --recursive drops module name before recursing' '\n \t(cd super2 &&\n \t (cd deeper/submodule/subsubmodule &&\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292478","messageId":"20160729004409.2072-5-sbeller@google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"[PATCHv3 4/7] submodule--helper: fix usage string for relative-path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:06Z","receivedAt":"2016-07-29T00:44:30Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Internally we call the underscore version of relative_path, but externally\nwe present an API with no underscores.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b22352b..fb90c64 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -892,7 +892,7 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tif (argc != 3)\n-\t\tdie(\"submodule--helper relative_path takes exactly 2 arguments, got %d\", argc);\n+\t\tdie(\"submodule--helper relative-path takes exactly 2 arguments, got %d\", argc);\n \n \tprintf(\"%s\", relative_path(argv[1], argv[2], &sb));\n \tstrbuf_release(&sb);\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292479","messageId":"20160729004409.2072-6-sbeller@google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"[PATCHv3 5/7] submodule-config: keep configured branch around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:07Z","receivedAt":"2016-07-29T00:44:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The branch field will be used in a later patch by `submodule update`.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule-config.c | 11 ++++++++++-\n submodule-config.h |  1 +\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 077db40..ebee1e4 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -59,6 +59,7 @@ static void free_one_config(struct submodule_entry *entry)\n {\n \tfree((void *) entry->config->path);\n \tfree((void *) entry->config->name);\n+\tfree((void *) entry->config->branch);\n \tfree((void *) entry->config->update_strategy.command);\n \tfree(entry->config);\n }\n@@ -199,6 +200,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \tsubmodule->update_strategy.command = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n+\tsubmodule->branch = NULL;\n \tsubmodule->recommend_shallow = -1;\n \n \thashcpy(submodule->gitmodules_sha1, gitmodules_sha1);\n@@ -358,9 +360,16 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\tif (!me->overwrite && submodule->recommend_shallow != -1)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t     \"shallow\");\n-\t\telse {\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\t\t\t     \"branch\");\n+\t\telse {\n+\t\t\tfree((void *)submodule->branch);\n+\t\t\tsubmodule->branch = xstrdup(value);\n \t\t}\n \t}\n \ndiff --git a/submodule-config.h b/submodule-config.h\nindex b1fdcc0..d05c542 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -15,6 +15,7 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tconst char *branch;\n \tstruct submodule_update_strategy update_strategy;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292480","messageId":"20160729004409.2072-4-sbeller@google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"[PATCHv3 3/7] submodule update: narrow scope of local variable","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:05Z","receivedAt":"2016-07-29T00:44:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 174f4ea..0d4021f 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -591,7 +591,6 @@ cmd_update()\n \n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n-\t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tif ! test -z \"$update\"\n \t\tthen\n \t\t\tupdate_module=$update\n@@ -617,6 +616,7 @@ cmd_update()\n \n \t\tif test -n \"$remote\"\n \t\tthen\n+\t\t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n \t\t\t\t# Fetch remote before determining tracking $sha1\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292481","messageId":"20160729004409.2072-8-sbeller@google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"[PATCHv3 7/7] submodule update: allow '.' for branch value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:09Z","receivedAt":"2016-07-29T00:44:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Gerrit has a \"superproject subscription\" feature[1], that triggers a\ncommit in a superproject that is subscribed to its submodules.\nConceptually this Gerrit feature can be done on the client side with\nGit via (except for raciness, error handling etc):\n\n  while [ true ]; do\n    git -C <superproject> submodule update --remote --force\n    git -C <superproject> commit -a -m \"Update submodules\"\n    git -C <superproject> push\n  done\n\nfor each branch in the superproject. To ease the configuration in Gerrit\na special value of \".\" has been introduced for the submodule.<name>.branch\nto mean the same branch as the superproject[2], such that you can create a\nnew branch on both superproject and the submodule and this feature\ncontinues to work on that new branch.\n\nNow we find projects in the wild with such a .gitmodules file.\nThe .gitmodules used in these Gerrit projects do not conform\nto Gits understanding of how .gitmodules should look like.\nThis teaches Git to deal gracefully with this syntax as well.\n\nThe redefinition of \".\" does no harm to existing projects unaware of\nthis change, as \".\" is an invalid branch name in Git, so we do not\nexpect such projects to exist.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 18 ++++++++++++++++++\n t/t7406-submodule-update.sh | 36 +++++++++++++++++++++++++++++++++++-\n 2 files changed, 53 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 710048f..ae88eff 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -909,6 +909,24 @@ static const char *remote_submodule_branch(const char *path)\n \tif (!sub->branch)\n \t\treturn \"master\";\n \n+\tif (!strcmp(sub->branch, \".\")) {\n+\t\tunsigned char sha1[20];\n+\t\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, NULL);\n+\n+\t\tif (!refname)\n+\t\t\tdie(_(\"No such ref: %s\"), \"HEAD\");\n+\n+\t\t/* detached HEAD */\n+\t\tif (!strcmp(refname, \"HEAD\"))\n+\t\t\tdie(_(\"Submodule (%s) branch configured to inherit \"\n+\t\t\t      \"branch from superproject, but the superproject \"\n+\t\t\t      \"is not on any branch\"), sub->name);\n+\n+\t\tif (!skip_prefix(refname, \"refs/heads/\", &refname))\n+\t\t\tdie(_(\"Expecting a full ref name, got %s\"), refname);\n+\t\treturn refname;\n+\t}\n+\n \treturn sub->branch;\n }\n \ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 1bb1f43..1c4c1f2 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -209,9 +209,43 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '\n \t)\n '\n \n+test_expect_success 'submodule update --remote should fetch upstream changes with .' '\n+\t(\n+\t\tcd super &&\n+\t\tgit config -f .gitmodules submodule.\"submodule\".branch \".\" &&\n+\t\tgit add .gitmodules &&\n+\t\tgit commit -m \"submodules: update from the respective superproject branch\"\n+\t) &&\n+\t(\n+\t\tcd submodule &&\n+\t\techo line4a >> file &&\n+\t\tgit add file &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"upstream line4a\" &&\n+\t\tgit checkout -b test-branch &&\n+\t\ttest_commit on-test-branch\n+\t) &&\n+\t(\n+\t\tcd super &&\n+\t\tgit submodule update --remote --force submodule &&\n+\t\tgit -C submodule log -1 --oneline >actual\n+\t\tgit -C ../submodule log -1 --oneline master >expect\n+\t\ttest_cmp expect actual &&\n+\t\tgit checkout -b test-branch &&\n+\t\tgit submodule update --remote --force submodule &&\n+\t\tgit -C submodule log -1 --oneline >actual\n+\t\tgit -C ../submodule log -1 --oneline test-branch >expect\n+\t\ttest_cmp expect actual &&\n+\t\tgit checkout master &&\n+\t\tgit branch -d test-branch &&\n+\t\t#~ git -C ../submodule branch -d test-branch &&\n+\t\tgit reset --hard HEAD^\n+\t)\n+'\n+\n test_expect_success 'local config should override .gitmodules branch' '\n \t(cd submodule &&\n-\t git checkout -b test-branch &&\n+\t git checkout test-branch &&\n \t echo line5 >> file &&\n \t git add file &&\n \t test_tick &&\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292482","messageId":"20160729004409.2072-7-sbeller@google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"[PATCHv3 6/7] submodule--helper: add remote-branch helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-29T00:44:08Z","receivedAt":"2016-07-29T00:44:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we want to enhance the logic for the branch selection.\nRewrite the current logic to be in C, so we can directly use C when\nwe enhance the logic.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 28 +++++++++++++++++++++++++++-\n git-submodule.sh            |  2 +-\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fb90c64..710048f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -899,6 +899,31 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix\n \treturn 0;\n }\n \n+static const char *remote_submodule_branch(const char *path)\n+{\n+\tconst struct submodule *sub;\n+\tgitmodules_config();\n+\tgit_config(submodule_config, NULL);\n+\n+\tsub = submodule_from_path(null_sha1, path);\n+\tif (!sub->branch)\n+\t\treturn \"master\";\n+\n+\treturn sub->branch;\n+}\n+\n+static int resolve_remote_submodule_branch(int argc, const char **argv,\n+\t\tconst char *prefix)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tif (argc != 2)\n+\t\tdie(\"submodule--helper remote-branch takes exactly one arguments, got %d\", argc);\n+\n+\tprintf(\"%s\", remote_submodule_branch(argv[1]));\n+\tstrbuf_release(&sb);\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -912,7 +937,8 @@ static struct cmd_struct commands[] = {\n \t{\"relative-path\", resolve_relative_path},\n \t{\"resolve-relative-url\", resolve_relative_url},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test},\n-\t{\"init\", module_init}\n+\t{\"init\", module_init},\n+\t{\"remote-branch\", resolve_remote_submodule_branch}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 0d4021f..1e493a8 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -616,7 +616,7 @@ cmd_update()\n \n \t\tif test -n \"$remote\"\n \t\tthen\n-\t\t\tbranch=$(get_submodule_config \"$name\" branch master)\n+\t\t\tbranch=$(git submodule--helper remote-branch \"$sm_path\")\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n \t\t\t\t# Fetch remote before determining tracking $sha1\n-- \n2.9.2.472.g1ffb07c.dirty\n\n"},{"id":"292801","messageId":"xmqqmvkwgnxw.fsf@gitster.mtv.corp.google.com","threadId":"42955","inReplyTo":"20160729004409.2072-1-sbeller@google.com","subject":"Re: [PATCHv3 0/7] submodule update: allow '.' for branch value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-01T21:48:59Z","receivedAt":"2016-08-01T21:49:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This got bigger than expected, but I am happier with the results.\n>\n> The meat is found in the last patch. (At least what I am interested in; others\n> may be more interested in the second patch which could be argued to be a real\n> bug fix to be merged down to maint.)\n\nIndeed 2/7 and 7/7 are both interesting.\n\nWill queue; thanks.\n"},{"id":"292911","messageId":"20160803162512.veyff5g52kaqwlgx@sigill.intra.peff.net","threadId":"42955","inReplyTo":"20160729004409.2072-7-sbeller@google.com","subject":"Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-03T16:25:13Z","receivedAt":"2016-08-03T17:04:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 28, 2016 at 05:44:08PM -0700, Stefan Beller wrote:\n\n> +static const char *remote_submodule_branch(const char *path)\n> +{\n> +\tconst struct submodule *sub;\n> +\tgitmodules_config();\n> +\tgit_config(submodule_config, NULL);\n> +\n> +\tsub = submodule_from_path(null_sha1, path);\n> +\tif (!sub->branch)\n> +\t\treturn \"master\";\n> +\n> +\treturn sub->branch;\n> +}\n\nCoverity complains about \"sub\" being NULL here, and indeed, it seems\nlike an easy segfault:\n\n  $ ./git submodule--helper remote-branch foo\n  Segmentation fault\n\nI guess this should return NULL in that case. But then this...\n\n> +static int resolve_remote_submodule_branch(int argc, const char **argv,\n> +\t\tconst char *prefix)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tif (argc != 2)\n> +\t\tdie(\"submodule--helper remote-branch takes exactly one arguments, got %d\", argc);\n> +\n> +\tprintf(\"%s\", remote_submodule_branch(argv[1]));\n> +\tstrbuf_release(&sb);\n> +\treturn 0;\n> +}\n\nwould need to handle the NULL return, as well. So maybe \"master\" or the\nempty string would be better. I haven't followed the topic closely\nenough to have an opinion.\n\n-Peff\n"},{"id":"292979","messageId":"CAGZ79kYW0dwUh=QpKL9W+cv8k_Z9Vc2wxoLMqud5brDtS9DYpA@mail.gmail.com","threadId":"42955","inReplyTo":"20160803162512.veyff5g52kaqwlgx@sigill.intra.peff.net","subject":"Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-03T20:11:51Z","receivedAt":"2016-08-03T20:17:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 3, 2016 at 9:25 AM, Jeff King <peff@peff.net> wrote:\n> On Thu, Jul 28, 2016 at 05:44:08PM -0700, Stefan Beller wrote:\n>\n>> +static const char *remote_submodule_branch(const char *path)\n>> +{\n>> +     const struct submodule *sub;\n>> +     gitmodules_config();\n>> +     git_config(submodule_config, NULL);\n>> +\n>> +     sub = submodule_from_path(null_sha1, path);\n>> +     if (!sub->branch)\n>> +             return \"master\";\n>> +\n>> +     return sub->branch;\n>> +}\n>\n> Coverity complains about \"sub\" being NULL here, and indeed, it seems\n> like an easy segfault:\n>\n>   $ ./git submodule--helper remote-branch foo\n>   Segmentation fault\n>\n> I guess this should return NULL in that case. But then this...\n\nI thought about checking for (!sub && !sub->branch) to return \"master\";\nHowever I disregarded that implementation as the submodule-config is\nreliable for any correct input. Though we need to handle wrong input as\nwell. So how about:\n\n  if (!sub)\n    die(_(\"A submodule doesn't exist at path %s\"), path);\n  if (!sub->branch)\n    return \"master\";\n\n...\n\nI think that explains best why we even check for !sub.\n\n\n>\n>> +static int resolve_remote_submodule_branch(int argc, const char **argv,\n>> +             const char *prefix)\n>> +{\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     if (argc != 2)\n>> +             die(\"submodule--helper remote-branch takes exactly one arguments, got %d\", argc);\n>> +\n>> +     printf(\"%s\", remote_submodule_branch(argv[1]));\n>> +     strbuf_release(&sb);\n>> +     return 0;\n>> +}\n>\n> would need to handle the NULL return, as well. So maybe \"master\" or the\n> empty string would be better. I haven't followed the topic closely\n> enough to have an opinion.\n\nOh I see, this can turn into a discussion what the API for\nremote_submodule_branch is. I think handling the NULL here and dying makes\nmore sense as then we can keep remote_submodule_branch pure to its\nintended use case. (If in the far future we have all the submodule stuff in C,\nwe may want to call remote_submodule_branch when we already know\nthat the path is valid, so no need to check it in there. Also not dying in there\nis more libified.)\n\nThanks for this review,\nStefan\n\n>\n> -Peff\n"},{"id":"292984","messageId":"20160803201544.7sljaikzwws7rekv@sigill.intra.peff.net","threadId":"42955","inReplyTo":"CAGZ79kYW0dwUh=QpKL9W+cv8k_Z9Vc2wxoLMqud5brDtS9DYpA@mail.gmail.com","subject":"Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-03T20:15:44Z","receivedAt":"2016-08-03T20:22:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 03, 2016 at 01:11:51PM -0700, Stefan Beller wrote:\n\n> > Coverity complains about \"sub\" being NULL here, and indeed, it seems\n> > like an easy segfault:\n> >\n> >   $ ./git submodule--helper remote-branch foo\n> >   Segmentation fault\n> >\n> > I guess this should return NULL in that case. But then this...\n> \n> I thought about checking for (!sub && !sub->branch) to return \"master\";\n> However I disregarded that implementation as the submodule-config is\n> reliable for any correct input. Though we need to handle wrong input as\n> well. So how about:\n> \n>   if (!sub)\n>     die(_(\"A submodule doesn't exist at path %s\"), path);\n>   if (!sub->branch)\n>     return \"master\";\n\nOK by me, as long as the caller of submodule--helper handles the death\nreasonably. Since it's an internal helper program, we really only have\nto worry about what git-submodule.sh does with it, which is good.\n\n> > would need to handle the NULL return, as well. So maybe \"master\" or the\n> > empty string would be better. I haven't followed the topic closely\n> > enough to have an opinion.\n> \n> Oh I see, this can turn into a discussion what the API for\n> remote_submodule_branch is. I think handling the NULL here and dying makes\n> more sense as then we can keep remote_submodule_branch pure to its\n> intended use case. (If in the far future we have all the submodule stuff in C,\n> we may want to call remote_submodule_branch when we already know\n> that the path is valid, so no need to check it in there. Also not dying in there\n> is more libified.)\n\nYep, that makes sense to me, too.\n\n-Peff\n"}]}