{"thread":{"id":"57390","subject":"[PATCH 0/1] checkout: teach \"git checkout\" to checkout submodule branches","startedAt":"2022-02-09T06:53:10Z","lastAt":"2022-02-09T06:56:33Z","messageCount":3,"participants":["Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"448032","messageId":"20220209065236.36494-1-chooglen@google.com","threadId":"57390","inReplyTo":null,"subject":"[PATCH 0/1] checkout: teach \"git checkout\" to checkout submodule branches","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-02-09T06:52:35Z","receivedAt":"2022-02-09T06:53:10Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"This series is based on gc/branch-recurse-submodules.\n\nSince gc/branch-recurse-submodules is queued for next, I was hoping to\nget some feedback on a followup feature - teaching \"git checkout\n--recurse-submodules\" to checkout the branches created by \"git branch\n--recurse-submodules\".\n\nWhen \"git branch --recurse-submodules\" was introduced [1], we noted that\nexisting Git commands ignore submodule branches and that we would teach\nthem to respect submodule branches if submodule.propagateBranches=true.\n\nThis series teaches \"git checkout\" to understand\nsubmodule.propagateBranches=true so that \"git checkout\" checks out the\nbranches created by \"git branch --recurse-submodules\". Specifically,\nthe behavior of \"git checkout\" follows the proposal laid out in the\nsubmodule branching RFC [2] (an abridged description can also be found\nin the commit message).\n\nThis series isn't ready for full review yet (see \"Future work\"), but the\napproach is unusual enough that I thought it would be useful to get\nearly feedback and evaluate alternatives (instead of polishing a\npotentially bad approach).\n\nI'd appreciate any feedback, though I'd like for reviewers to focus on\nfleshing out the design space. This could include answering the\nquestions in \"Questions\", or providing additional context to \"git\ncheckout\" that I missed.\n\n(Cc-ed some people who have shown some interest in submodules in the\npast)\n\n= Approach\n\nPresently, \"git checkout\" updates the superproject/submodule HEAD and\ntrees using different machinery:\n\n- unpack-trees updates the superproject tree, submodule tree and\n  submodule HEAD\n- cmd_checkout() porcelain (specifically, update_refs_for_switch()),\n  updates the superproject HEAD\n\nThis works well if we assume that \"git checkout\" always leaves\nsubmodule's HEAD pointing to a commit id, but this is no longer true\nwith submodule.propagateBranches=true, so this series updates submodules\ndifferently:\n\n- disable submodule updating in unpack-trees, so that neither the\n  submodule HEAD nor submodule tree are updated\n- update the submodules' HEAD and tree using a \"git checkout\" subprocess\n  in the cmd_checkout() porcelain (specifically,\n  update_refs_for_switch())\n\nDoing work in the cmd_checkout() porcelain lets us support complicated,\n\"git checkout\" workflows that don't make sense in unpack-trees:\n\n- updating the submodule's tree using the submodule's \"topic\" branch\n  instead of the gitlink of the superproject's \"topic\" branch\n- updating the submodule's HEAD to a ref instead of a commit id\n- checking out the gitlink when the submodule's HEAD points to a branch\n  pointing to the same commit (unpack-trees will ignore the submodule if\n  it notices that the HEAD's oid does not change)\n- preventing a checkout if a submodule is in detached HEAD and not\n  pointing to the superproject gitlink (Future work, see RFC for more\n  details [2])\n\nThe tradeoffs are:\n\n- we lose atomicity guarantees because we run multiple subprocesses\n  instead of a single unpack_trees()\n- overhead of spawning child processes\n- blurring of the lines between cmd_checkout() porcelain and\n  unpack-trees - superproject trees and HEAD are still updated\n  in different code locations (by cmd_checkout() porcelain and\n  unpack-trees respectively), but submodule trees and HEAD are both\n  updated by a single \"git checkout\" subprocess.\n\n= Questions\n\n- Is this an accurate summary of benefits and tradeoffs? Am I missing\n  something?\n- What alternatives exist and how viable are they?\n\n= Future work\n\n- Prevent a submodule checkout when the submodule's HEAD is detached and\n  not pointing to the superproject gitlink\n- Implement the checkout -b/-B flags i.e. create a new branch\n  recursively and check it out.\n- Refactor out the common bits between this series and \"git branch\n  --recurse-submodules\".\n- Check for checkout options that are incompatible with\n  --recurse-submodules\n- Parallelize the submodule checkout\n- Handle the submodule.recurse config value\n- Possibly refactor out the \"find submodules in the index\" logic from\n  builtin/submodule--helper.\n\n[1] 961b130d20 (branch: add --recurse-submodules option for branch creation, 2022-01-28)\n[2] https://lore.kernel.org/git/kl6lh7cjvpm3.fsf@chooglen-macbookpro.roam.corp.google.com\n\nGlen Choo (1):\n  checkout --recurse-submodules: checkout branches if configured\n\n builtin/checkout.c            | 142 +++++++++++++++++++++++++++++++++-\n submodule.c                   |   6 ++\n submodule.h                   |   9 +++\n t/t2013-checkout-submodule.sh |  90 +++++++++++++++++++++\n 4 files changed, 243 insertions(+), 4 deletions(-)\n\n\nbase-commit: 679e3693aba0c17af60c031f7eef68f2296b8dad\n-- \n2.33.GIT\n\n"},{"id":"448033","messageId":"20220209065236.36494-2-chooglen@google.com","threadId":"57390","inReplyTo":"20220209065236.36494-1-chooglen@google.com","subject":"[PATCH 1/1] checkout --recurse-submodules: checkout branches if configured","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-02-09T06:52:36Z","receivedAt":"2022-02-09T06:53:15Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Previously, we introduced \"git branch --recurse-submodules\", but unless\nthe config value \"submodule.propagateBranches\" is true, commands ignore\nthe submodule ref store and do not respect the submodule branches.\n\nTeach \"git [checkout|switch]\" to support\nsubmodule.propagateBranches=true, which makes \"git checkout\n--recurse-submodules foo\" checkout the branch \"foo\" in the superproject\nand all submodules. To support superproject branches that were not\ncreated with \"git branch --recurse-submodules\", \"git checkout\n--recurse-submodules foo\" does not fail if a submodule is missing the\nbranch \"foo\". Instead, the gitlink of the superproject's \"foo\" is\nchecked out (just like how \"--recurse-submodules\" already behaves).\n\nPresently, \"git checkout\" updates the superproject tree, submodule\ntrees and submodule HEAD using unpack-trees, while the superproject HEAD\nis updated in the cmd_checkout() porcelain. However, this does not work\nwell for submodule.propagateBranches=true because unpack-trees does not\nconsider submodule refs. Instead of introducing procelain concerns into\nunpack-trees, submodule updating is done in the cmd_checkout()\nporcelain like so:\n\n- disable submodule updating in unpack-trees, so that neither the\n  submodule HEAD nor submodule tree are updated\n- update the submodules' HEAD and tree using a \"git checkout\" subprocess\n  (specifically, update_refs_for_switch())\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n builtin/checkout.c            | 142 +++++++++++++++++++++++++++++++++-\n submodule.c                   |   6 ++\n submodule.h                   |   9 +++\n t/t2013-checkout-submodule.sh |  90 +++++++++++++++++++++\n 4 files changed, 243 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8600860629..0d1557da88 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -74,6 +74,8 @@ struct checkout_opts {\n \tint ignore_unmerged;\n \tint pathspec_file_nul;\n \tconst char *pathspec_from_file;\n+\tint recurse_submodules;\n+\tint submodule_propagate_branches;\n \n \tconst char *new_branch;\n \tconst char *new_branch_force;\n@@ -858,6 +860,123 @@ static void report_tracking(struct branch_info *new_branch_info)\n \tstrbuf_release(&sb);\n }\n \n+static int update_submodule(struct repository *subrepo, const struct submodule *submodule, const struct checkout_opts *opts, const char *new_head)\n+{\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\t/*\n+\t * NEEDSWORK: this output formatting is just copied from\n+\t * submodule_create_branch() - this should become a helper function\n+\t * instead.\n+\t */\n+\tint ret = 0;\n+\tstruct strbuf child_err = STRBUF_INIT;\n+\tstruct strbuf out_buf = STRBUF_INIT;\n+\tchar *out_prefix = xstrfmt(\"submodule '%s': \", submodule->name);\n+\tchild.err = -1;\n+\tchild.stdout_to_stderr = 1;\n+\n+\tchild.git_cmd = 1;\n+\tprepare_other_repo_env(&child.env_array, subrepo->gitdir);\n+\t/* NEEDSWORK: TODO do i need the -c? */\n+\tstrvec_pushl(&child.args, \"-c\", \"submodule.propagateBranches\", \"checkout\", \"--recurse-submodules\", NULL);\n+\n+\tif (opts->new_branch)\n+\t\tstrvec_push(&child.args, \"-c\");\n+\tif (opts->new_branch_force)\n+\t\tstrvec_push(&child.args, \"-C\");\n+\tif (opts->discard_changes)\n+\t\tstrvec_push(&child.args, \"--discard-changes\");\n+\tif (opts->show_progress)\n+\t\tstrvec_push(&child.args, \"--progress\");\n+\tif (opts->track == BRANCH_TRACK_EXPLICIT)\n+\t\tstrvec_push(&child.args, \"--track\");\n+\tif (opts->track == BRANCH_TRACK_INHERIT)\n+\t\tstrvec_pushl(&child.args, \"--track\", \"inherit\", NULL);\n+\n+\tif (opts->force)\n+\t\tstrvec_push(&child.args, \"--force\");\n+\tif (opts->new_orphan_branch)\n+\t\tstrvec_push(&child.args, \"--orphan\");\n+\tif (opts->ignore_other_worktrees)\n+\t\tstrvec_push(&child.args, \"--ignore-other-worktrees\");\n+\n+\tstrvec_push(&child.args, new_head);\n+\n+\t/*\n+\t * NEEDSWORK: handle the output correctly e.g. the child process\n+\t * shouldn't dump warnings about detached HEAD.\n+\t */\n+\tif ((ret = start_command(&child)))\n+\t\treturn ret;\n+\tret = finish_command(&child);\n+\tstrbuf_read(&child_err, child.err, 0);\n+\tstrbuf_add_lines(&out_buf, out_prefix, child_err.buf, child_err.len);\n+\n+\tif (ret)\n+\t\tfprintf(stderr, \"%s\", out_buf.buf);\n+\telse\n+\t\tprintf(\"%s\", out_buf.buf);\n+\n+\tstrbuf_release(&child_err);\n+\tstrbuf_release(&out_buf);\n+\treturn ret;\n+}\n+\n+static int update_submodules_recursively(const struct checkout_opts *opts,\n+\t\t\t\t   struct branch_info *old_branch_info,\n+\t\t\t\t   struct branch_info *new_branch_info)\n+{\n+\tint i;\n+\tstruct repository subrepo;\n+\tconst struct submodule *submodule;\n+\tconst char *new_head;\n+\n+\t/*\n+\t * NEEDSWORK: use parallel processes instead.\n+\t */\n+\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n+\t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n+\n+\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n+\t\tif (repo_submodule_init(&subrepo, the_repository, ce->name, null_oid())) {\n+\t\t\twarning(_(\"unable to checkout submodule '%s'\"), ce->name);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If the repo is in detached HEAD, submodules should\n+\t\t * checkout the gitlink.\n+\t\t */\n+\t\tif (!new_branch_info->path)\n+\t\t\tnew_head = oid_to_hex(&ce->oid);\n+\t\t/*\n+\t\t * Otherwise, the submodule should checkout the branch\n+\t\t * if possible.\n+\t\t */\n+\t\telse if (!refs_ref_exists(get_main_ref_store(&subrepo), new_branch_info->path)){\n+\t\t\t\twarning(_(\"branch '%s' does not exist in submodule '%s'\"), new_branch_info->name, ce->name);\n+\t\t\t\tnew_head = oid_to_hex(&ce->oid);\n+\t\t} else\n+\t\t\tnew_head = new_branch_info->name;\n+\n+\n+\t\t/*\n+\t\t * NEEDSWORK: Ironically, this doesn't use\n+\t\t * submodule_from_ce() because we've disabled submodule\n+\t\t * updating\n+\t\t */\n+\t\tsubmodule = submodule_from_path(the_repository, null_oid(), ce->name);\n+\t\tif (!submodule)\n+\t\t\tcontinue;\n+\n+\t\tupdate_submodule(&subrepo, submodule, opts, new_head);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\t   struct branch_info *old_branch_info,\n \t\t\t\t   struct branch_info *new_branch_info)\n@@ -947,6 +1066,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tdelete_reflog(old_branch_info->path);\n \t\t}\n \t}\n+\n+\tif (opts->submodule_propagate_branches && opts->recurse_submodules) {\n+\t\tupdate_submodules_recursively(opts, old_branch_info, new_branch_info);\n+\t}\n+\n \tremove_branch_state(the_repository, !opts->quiet);\n \tstrbuf_release(&msg);\n \tif (!opts->quiet &&\n@@ -1130,7 +1254,10 @@ static int git_checkout_config(const char *var, const char *value, void *cb)\n \t\topts->dwim_new_local_branch = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\n+\tif (!strcasecmp(var, \"submodule.propagateBranches\")) {\n+\t\topts->submodule_propagate_branches = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (starts_with(var, \"submodule.\"))\n \t\treturn git_default_submodule_config(var, value, NULL);\n \n@@ -1512,9 +1639,8 @@ static struct option *add_common_options(struct checkout_opts *opts,\n {\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opts->quiet, N_(\"suppress progress reporting\")),\n-\t\tOPT_CALLBACK_F(0, \"recurse-submodules\", NULL,\n-\t\t\t    \"checkout\", \"control recursive updating of submodules\",\n-\t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n+\t\tOPT_BOOL(0, \"recurse-submodules\", &opts->recurse_submodules,\n+\t\t\t    N_(\"control recursive updating of submodules\")),\n \t\tOPT_BOOL(0, \"progress\", &opts->show_progress, N_(\"force progress reporting\")),\n \t\tOPT_BOOL('m', \"merge\", &opts->merge, N_(\"perform a 3-way merge with the new branch\")),\n \t\tOPT_STRING(0, \"conflict\", &opts->conflict_style, N_(\"style\"),\n@@ -1588,6 +1714,8 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \topts->show_progress = -1;\n \n \tgit_config(git_checkout_config, opts);\n+\tif (opts->recurse_submodules && !opts->submodule_propagate_branches)\n+\t\tset_update_submodules(RECURSE_SUBMODULES_ON);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n@@ -1768,6 +1896,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t\tvalidate_new_branchname(opts->new_branch, &buf, 0);\n \t\tstrbuf_release(&buf);\n \t}\n+\t/*\n+\t * NEEDSWORK: Check for option comaptibility with\n+\t * --recurse-submodules + submodule.propagateBranches\n+\t *\n+\t * e.g. --merge, --detach\n+\t */\n \n \tUNLEAK(opts);\n \tif (opts->patch_mode || opts->pathspec.nr)\ndiff --git a/submodule.c b/submodule.c\nindex 5ace18a7d9..18a609de09 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -766,6 +766,12 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t}\n }\n \n+void set_update_submodules(int update_submodules)\n+{\n+\tconfig_update_recurse_submodules = update_submodules\n+\t\t? RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n+}\n+\n int should_update_submodules(void)\n {\n \treturn config_update_recurse_submodules == RECURSE_SUBMODULES_ON;\ndiff --git a/submodule.h b/submodule.h\nindex 784ceffc0e..239cdaf451 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -80,6 +80,15 @@ void show_submodule_diff_summary(struct diff_options *o, const char *path,\n void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t\t\t\tstruct object_id *one, struct object_id *two,\n \t\t\t\tunsigned dirty_submodule);\n+\n+/*\n+ * Explicitly set whether or not we want to update submodules.\n+ * This should only be used for very fine-grained control:\n+ * - if option parsing is enough, use\n+ *   option_parse_recurse_submodules_worktree_updater()\n+ * - if config parsing is enough, use git_default_submodule_config()\n+ */\n+void set_update_submodules(int update_submodules);\n /* Check if we want to update any submodule.*/\n int should_update_submodules(void);\n /*\ndiff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\nindex b2bdd1fcb4..db88bd3448 100755\n--- a/t/t2013-checkout-submodule.sh\n+++ b/t/t2013-checkout-submodule.sh\n@@ -72,4 +72,94 @@ test_submodule_switch \"checkout\"\n \n test_submodule_forced_switch \"checkout -f\"\n \n+test_expect_success 'setup tests with submodule.propagateBranches' '\n+\tmkdir submodule2 &&\n+\t(\n+\t\tcd submodule2 &&\n+\t\tgit init &&\n+\t\ttest_commit new_commit\n+\t) &&\n+\tmkdir super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit init &&\n+\t\tgit submodule add ../submodule sub1 &&\n+\t\t(\n+\t\t\tcd sub1 &&\n+\t\t\tgit submodule add ../submodule2 sub2 &&\n+\t\t\tgit commit -m \"add submodule\"\n+\t\t) &&\n+\t\tgit add sub1 &&\n+\t\tgit submodule add ../submodule2 sub2 &&\n+\t\tgit commit -m \"add submodules\"\n+\t) &&\n+\tgit -C super config submodule.propagateBranches true\n+'\n+\n+# Tests that HEAD is a literal value regardless of whether it points to\n+# a branch or commit id.\n+test_cmp_head () {\n+\tEXPECT=$1\n+\tDIR=$2\n+\trm -f actual-head expect-head &&\n+\t(git -C $DIR symbolic-ref HEAD || git -C $DIR rev-parse HEAD) \\\n+\t\t\t2>/dev/null >actual-head &&\n+\techo \"$EXPECT\" >expect-head &&\n+\ttest_cmp expect-head actual-head\n+}\n+\n+test_expect_success '\"checkout\" with submodule.propagateBranches checks out branch in all submodules if it exists' '\n+        test_when_finished \"git -C super checkout --recurse-submodules master\" &&\n+\tgit -C super branch --recurse-submodules branch-a master &&\n+\t(\n+\t\tcd super/sub1 &&\n+\t\tgit checkout branch-a &&\n+\t\ttest_commit --no-tag branch-a &&\n+\t\tgit checkout master\n+\t) &&\n+\tgit -C super checkout --recurse-submodules branch-a &&\n+\ttest_cmp_head refs/heads/branch-a super &&\n+\ttest_cmp_head refs/heads/branch-a super/sub1 &&\n+\ttest_cmp_head refs/heads/branch-a super/sub2 &&\n+\ttest_cmp_head refs/heads/branch-a super/sub1/sub2\n+'\n+\n+test_expect_success 'submodule.propagateBranches checks out gitlink if branch does not exist' '\n+        test_when_finished \"git -C super checkout --recurse-submodules master \\\n+\t\t&& rm expect* && rm actual*\" &&\n+\tgit -C super branch --recurse-submodules branch-b master &&\n+\tgit -C super/sub1 rev-parse refs/heads/branch-b >expect-sub1-head &&\n+\tgit -C super/sub1/sub2 rev-parse refs/heads/branch-b >expect-sub1-sub2-head &&\n+\t(\n+\t\tcd super/sub1 &&\n+\t\tgit checkout --detach branch-b &&\n+\t\ttest_commit --no-tag branch-b &&\n+\t\tgit branch -D branch-b\n+\t) &&\n+\tgit -C super checkout --recurse-submodules branch-b 2>actual-err &&\n+\ttest_cmp_head \"$(cat expect-sub1-head)\" super/sub1 &&\n+\tgrep \"warning: branch .branch-b. does not exist in submodule .sub1.\" actual-err &&\n+\ttest_cmp_head refs/heads/branch-b super &&\n+\ttest_cmp_head refs/heads/branch-b super/sub2 &&\n+\t# even though super/sub1/sub2 has branch-b, do not check it out\n+\t# because its containing repo did not checkout branch-b\n+\ttest_cmp_head \"$(cat expect-sub1-sub2-head)\" super/sub1/sub2\n+'\n+\n+test_expect_success 'submodule.propagateBranches ignores non-branches' '\n+        test_when_finished \"git -C super checkout --recurse-submodules master \\\n+\t\t&& rm expect* && rm actual*\" &&\n+\tgit -C super branch --recurse-submodules branch-c master &&\n+\tgit -C super/sub1 rev-parse refs/heads/branch-c >expect-sub1-head &&\n+\t(\n+\t\tcd super/sub1 &&\n+\t\tgit branch -D branch-c &&\n+\t\t# create a new commit with the tag \"branch-c\"\n+\t\ttest_commit branch-c\n+\t) &&\n+\tgit -C super checkout --recurse-submodules branch-c  2>actual-err &&\n+\ttest_cmp_head \"$(cat expect-sub1-head)\" super/sub1 &&\n+\tgrep \"warning: branch .branch-c. does not exist in submodule .sub1.\" actual-err\n+'\n+\n test_done\n-- \n2.33.GIT\n\n"},{"id":"448034","messageId":"kl6lczjwfrzc.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57390","inReplyTo":"20220209065236.36494-1-chooglen@google.com","subject":"Re: [PATCH 0/1] checkout: teach \"git checkout\" to checkout submodule branches","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-02-09T06:56:23Z","receivedAt":"2022-02-09T06:56:33Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> This series is based on gc/branch-recurse-submodules.\n>\n> Since gc/branch-recurse-submodules is queued for next, I was hoping to\n> get some feedback on a followup feature - teaching \"git checkout\n> --recurse-submodules\" to checkout the branches created by \"git branch\n> --recurse-submodules\".\n\nGah, I meant to send this with the RFC tag.\n"}]}