Submodule branching RFC: https://lore.kernel.org/git/kl6lv912uvjv.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ Original Submodule UX RFC/Discussion: https://lore.kernel.org/git/YHofmWcIAidkvJiD@xxxxxxxxxx/ Contributor Summit submodules Notes: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211148060.56@xxxxxxxxxxxxxxxxx/ Submodule UX overhaul updates: https://lore.kernel.org/git/?q=Submodule+UX+overhaul+update This series implements branch --recurse-submodules as laid out in the Submodule branching RFC (linked above). If there are concerns about the UX/behavior, I would appreciate feedback on the RFC thread as well :) This series is based off js/branch-track-inherit. Future work: * `git branch -d --recurse-submodules` so that users can clean up extraneous branches. * `git [checkout | switch] --recurse-submodules` + submodule.propagateBranches so that users can actually checkout the branches. * After [1], it seems clear that --recurse-submodules parsing could really benefit from some standardization. It's not obvious which RECURSE_SUBMODULE_* enums are applicable to which commands, and there is no way to distinguish between an explicit --recurse-submodules from argv vs submodule.recurse from the config. I chose not to use them in this series because their usage is already inconsistent (grep.c doesn't use them either), and it would be _more_ confusing to use the enum (handling RECURSE_SUBMODULES_DEFAULT = 1 is trickier than boolean 0 and 1). At this point, I think it would be too noisy to introduce the enum, but this would be a nice cleanup to do later. * As documented in branch.c, we create branches using a child process only because install_branch_config() does not support submodules. It should be possible to remove the child process once we make the appropriate changes to config.c. I attempted this in [2] but chose to punt it because it was too time-consuming at the time. Changes since v3: * Split up the old patch 1. Patch 1 had a big diff because it used to move lines, remove dead code and introduce repo_* functions (thanks Jonathan!) ** repo_* functions have been dropped; they added noise and are not necessary for correctness. * Use a new, harder-to-misuse function in --set-upstream-to, dwim_and_setup_tracking(). Now, setup_tracking() never does DWIM and dwim_and_setup_tracking() always does DWIM. * Move create_branch() dry_run to its own patch. * Fix an oversight where submodules in subtrees were ignored. This was because submodules_of_tree() and tree_entry() didn't recurse into subtrees. Test this accordingly (thanks Jonathan!). * cmd_branch() possible actions are more consistently ordered. * Documentation fixes (thanks Philippe!). * Additional comments and explanation. * Drop patch 5 (optional cleanup). * Rebase onto js/branch-track-inherit v6. Changes since v2: * Rebase onto js/branch-track-inherit. This series should continue to be the case going forward. * Patch 1 has a smaller diff because the introduction of validate_branch_start() no longer changes the function order thanks to a forward declaration. This artificial forward declaration is removed in a patch 2 (which can just be squashed into patch 1). * Optional cleanup: fix questionable exit codes in patch 5. Changes since v1: * Move the functionality of "git branch --dry-run" into "git submodule-helper create-branch --dry-run" * Add more fields to the submodules_of_tree() struct to reduce the number of allocations made by the caller. Move this functionality to patch 3 (formerly patch 4) and drop patch 1. * Make submodules_of_tree() ignore inactive submodules * Structure the output of the submodules a bit better by adding prefixes to the child process' output (instead of inconsistently indenting the output). ** I wasn't able to find a good way to interleave stdout/stderr correctly, so a less-than-desirable workaround was to route the child process output to stdout/stderr depending on the exit code. ** Eventually, I would like to structure the output of submodules in a report, as Ævar suggested. But at this stage, I think that it's better to spend time getting user feedback on the submodules branching UX and it'll be easier to standardize the output when we've implemented more of the UX :) [1] https://lore.kernel.org/git/kl6lbl1p9zjf.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ [2] https://lore.kernel.org/git/kl6lv90ytd4v.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ Glen Choo (5): branch: move --set-upstream-to behavior to dwim_and_setup_tracking() branch: make create_branch() always create a branch branch: add a dry_run parameter to create_branch() builtin/branch: clean up action-picking logic in cmd_branch() branch: add --recurse-submodules option for branch creation Documentation/config/advice.txt | 3 + Documentation/config/submodule.txt | 24 ++- Documentation/git-branch.txt | 11 +- advice.c | 1 + advice.h | 1 + branch.c | 257 ++++++++++++++++++++----- branch.h | 57 +++++- builtin/branch.c | 70 +++++-- builtin/checkout.c | 3 +- builtin/submodule--helper.c | 38 ++++ submodule-config.c | 60 ++++++ submodule-config.h | 34 ++++ submodule.c | 11 +- submodule.h | 3 + t/t3200-branch.sh | 17 ++ t/t3207-branch-submodule.sh | 291 +++++++++++++++++++++++++++++ 16 files changed, 805 insertions(+), 76 deletions(-) create mode 100755 t/t3207-branch-submodule.sh Range-diff against v3: 1: 8241c0b51a < -: ---------- branch: move --set-upstream-to behavior to setup_tracking() 2: b74bcbaade < -: ---------- branch: remove forward declaration of validate_branch_start() -: ---------- > 1: dfdbbaaca5 branch: move --set-upstream-to behavior to dwim_and_setup_tracking() -: ---------- > 2: e22a177cb7 branch: make create_branch() always create a branch -: ---------- > 3: 8a895aa401 branch: add a dry_run parameter to create_branch() 3: 235173efc9 ! 4: 971c53ec85 builtin/branch: clean up action-picking logic in cmd_branch() @@ Commit message Such an option does not exist yet, but one will be introduced in a subsequent commit. - Incidentally, fix an incorrect usage string that combined the 'list' - usage of git branch (-l) with the 'create' usage; this string has been - incorrect since its inception, a8dfd5eac4 (Make builtin-branch.c use - parse_options., 2007-10-07). - Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> ## builtin/branch.c ## -@@ - - static const char * const builtin_branch_usage[] = { - N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"), -- N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"), -+ N_("git branch [<options>] [-l] [<pattern>...]"), - N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."), - N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"), - N_("git branch [<options>] (-c | -C) [<old-branch>] <new-branch>"), @@ builtin/branch.c: static int edit_branch_description(const char *branch_name) int cmd_branch(int argc, const char **argv, const char *prefix) @@ builtin/branch.c: static int edit_branch_description(const char *branch_name) - int reflog = 0, edit_description = 0; - int quiet = 0, unset_upstream = 0; + /* possible actions */ -+ int delete = 0, rename = 0, copy = 0, force = 0, list = 0, ++ int delete = 0, rename = 0, copy = 0, list = 0, + unset_upstream = 0, show_current = 0, edit_description = 0; + const char *new_upstream = NULL; + int noncreate_actions = 0; + /* possible options */ -+ int reflog = 0, quiet = 0, icase = 0; - const char *new_upstream = NULL; ++ int reflog = 0, quiet = 0, icase = 0, force = 0; enum branch_track track; struct ref_filter filter; - int icase = 0; 4: 3dabb8e2fa ! 5: cd88f3ad92 branch: add --recurse-submodules option for branch creation @@ Commit message * add the "submoduleNotUpdated" advice to advise users to update the submodules in their trees - Other changes - - * add a "dry_run" parameter to create_branch() in order to support - `git submodule--helper create-branch --dry-run` + Incidentally, fix an incorrect usage string that combined the 'list' + usage of git branch (-l) with the 'create' usage; this string has been + incorrect since its inception, a8dfd5eac4 (Make builtin-branch.c use + parse_options., 2007-10-07). Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> @@ Documentation/config/advice.txt: advice.*:: configured to "die" causes a fatal error. + submodulesNotUpdated:: + Advice shown when a user runs a submodule command that fails -+ because `git submodule update` was not run. ++ because `git submodule update --init` was not run. addIgnoredFile:: Advice shown if a user attempts to add an ignored file to the index. ## Documentation/config/submodule.txt ## +@@ Documentation/config/submodule.txt: submodule.active:: + + submodule.recurse:: + A boolean indicating if commands should enable the `--recurse-submodules` +- option by default. +- Applies to all commands that support this option +- (`checkout`, `fetch`, `grep`, `pull`, `push`, `read-tree`, `reset`, +- `restore` and `switch`) except `clone` and `ls-files`. +- Defaults to false. ++ option by default. Defaults to false. ++ + + When set to true, it can be deactivated via the + `--no-recurse-submodules` option. Note that some Git commands + lacking this option may call some of the above commands affected by @@ Documentation/config/submodule.txt: submodule.recurse:: + `git fetch` but does not have a `--no-recurse-submodules` option. For these commands a workaround is to temporarily change the configuration value by using `git -c submodule.recurse=0`. - ++ + ++ The following list shows the commands that accept ++ `--recurse-submodules` and whether they are supported by this ++ setting. ++ * `checkout`, `fetch`, `grep`, `pull`, `push`, `read-tree`, ++ `reset`, `restore` and `switch` are always supported. ++ * `clone` and `ls-files` are not supported. ++ * `branch` is supported only if `submodule.propagateBranches` is ++ enabled ++ +submodule.propagateBranches:: + [EXPERIMENTAL] A boolean that enables branching support when + using `--recurse-submodules` or `submodule.recurse=true`. @@ Documentation/config/submodule.txt: submodule.recurse:: + `--recurse-submodules` and certain commands that already accept + `--recurse-submodules` will now consider branches. + Defaults to false. -+ + submodule.fetchJobs:: Specifies how many submodules are fetched/cloned at the same time. - A positive integer allows up to that number of submodules fetched + + ## Documentation/git-branch.txt ## +@@ Documentation/git-branch.txt: SYNOPSIS + [--points-at <object>] [--format=<format>] + [(-r | --remotes) | (-a | --all)] + [--list] [<pattern>...] +-'git branch' [--track [direct|inherit] | --no-track] [-f] <branchname> [<start-point>] ++'git branch' [--track [direct|inherit] | --no-track] [-f] ++ [--recurse-submodules] <branchname> [<start-point>] + 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>] + 'git branch' --unset-upstream [<branchname>] + 'git branch' (-m | -M) [<oldbranch>] <newbranch> +@@ Documentation/git-branch.txt: how the `branch.<name>.remote` and `branch.<name>.merge` options are used. + Do not set up "upstream" configuration, even if the + branch.autoSetupMerge configuration variable is set. + ++--recurse-submodules:: ++ THIS OPTION IS EXPERIMENTAL! Causes the current command to ++ recurse into submodules if `submodule.propagateBranches` is ++ enabled. See `submodule.propagateBranches` in ++ linkgit:git-config[1]. ++ + ++ Currently, only branch creation is supported. ++ + --set-upstream:: + As this option had confusing syntax, it is no longer supported. + Please use `--track` or `--set-upstream-to` instead. ## advice.c ## @@ advice.c: static struct { @@ branch.c struct tracking { struct refspec_item spec; -@@ branch.c: void setup_tracking(const char *new_ref, const char *orig_ref, - - void create_branch(struct repository *r, const char *name, - const char *start_name, int force, int clobber_head_ok, -- int reflog, int quiet, enum branch_track track) -+ int reflog, int quiet, enum branch_track track, int dry_run) - { - struct object_id oid; - char *real_ref; -@@ branch.c: void create_branch(struct repository *r, const char *name, - } - - validate_branch_start(r, start_name, track, &oid, &real_ref); -+ if (dry_run) -+ goto cleanup; - - if (reflog) - log_all_ref_updates = LOG_REFS_NORMAL; -@@ branch.c: void create_branch(struct repository *r, const char *name, - if (real_ref && track) - setup_tracking(ref.buf + 11, real_ref, track, quiet, 0); - -+cleanup: - strbuf_release(&ref); - free(real_ref); +@@ branch.c: void dwim_and_setup_tracking(struct repository *r, const char *new_ref, + setup_tracking(new_ref, real_orig_ref, track, quiet); } ++/** ++ * Creates a branch in a submodule by calling ++ * create_branches_recursively() in a child process. The child process ++ * is necessary because install_branch_config() (and its variants) do ++ * not support writing configs to submodules. ++ */ +static int submodule_create_branch(struct repository *r, + const struct submodule *submodule, + const char *name, const char *start_oid, @@ branch.c: void create_branch(struct repository *r, const char *name, + struct submodule_entry_list submodule_entry_list; + + /* Perform dwim on start_name to get super_oid and branch_point. */ -+ validate_branch_start(r, start_name, BRANCH_TRACK_NEVER, &super_oid, -+ &branch_point); ++ dwim_branch_start(r, start_name, BRANCH_TRACK_NEVER, &branch_point, ++ &super_oid); + + /* + * If we were not given an explicit name to track, then assume we are at @@ branch.c: void create_branch(struct repository *r, const char *name, + * tedious to determine whether or not tracking was set up in the + * superproject. + */ -+ setup_tracking(name, tracking_name, track, quiet, 0); ++ setup_tracking(name, tracking_name, track, quiet); + + for (i = 0; i < submodule_entry_list.entry_nr; i++) { + if (submodule_create_branch( @@ branch.c: void create_branch(struct repository *r, const char *name, unlink(git_path_merge_head(r)); ## branch.h ## -@@ branch.h: void setup_tracking(const char *new_ref, const char *orig_ref, - * - track causes the new branch to be configured to merge the remote branch - * that start_name is a tracking branch for (if any). - * -+ * - dry_run causes the branch to be validated but not created. -+ * - */ --void create_branch(struct repository *r, -- const char *name, const char *start_name, -- int force, int clobber_head_ok, -- int reflog, int quiet, enum branch_track track); -+void create_branch(struct repository *r, const char *name, -+ const char *start_name, int force, int clobber_head_ok, -+ int reflog, int quiet, enum branch_track track, int dry_run); +@@ branch.h: void create_branch(struct repository *r, const char *name, + const char *start_name, int force, int clobber_head_ok, + int reflog, int quiet, enum branch_track track, int dry_run); +/* + * Creates a new branch in repository and its submodules (and its @@ branch.h: void setup_tracking(const char *new_ref, const char *orig_ref, * Return 1 if the named branch already exists; return 0 otherwise. ## builtin/branch.c ## +@@ + + static const char * const builtin_branch_usage[] = { + N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"), +- N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"), ++ N_("git branch [<options>] [-f] [--recurse-submodules] <branch-name> [<start-point>]"), ++ N_("git branch [<options>] [-l] [<pattern>...]"), + N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."), + N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"), + N_("git branch [<options>] (-c | -C) [<old-branch>] <new-branch>"), @@ builtin/branch.c: static const char * const builtin_branch_usage[] = { static const char *head; @@ builtin/branch.c: static int git_branch_config(const char *var, const char *valu } @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix) - unset_upstream = 0, show_current = 0, edit_description = 0; + const char *new_upstream = NULL; int noncreate_actions = 0; /* possible options */ -- int reflog = 0, quiet = 0, icase = 0; -+ int reflog = 0, quiet = 0, icase = 0, recurse_submodules_explicit = 0; - const char *new_upstream = NULL; +- int reflog = 0, quiet = 0, icase = 0, force = 0; ++ int reflog = 0, quiet = 0, icase = 0, force = 0, ++ recurse_submodules_explicit = 0; enum branch_track track; struct ref_filter filter; + static struct ref_sorting *sorting; @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix) OPT_CALLBACK(0, "points-at", &filter.points_at, N_("object"), N_("print only branches of the object"), parse_opt_object_name), @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix - create_branch(the_repository, - argv[0], (argc == 2) ? argv[1] : head, -- force, 0, reflog, quiet, track); +- force, 0, reflog, quiet, track, 0); - + if (recurse_submodules) { + create_branches_recursively(the_repository, branch_name, @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix usage_with_options(builtin_branch_usage, options); - ## builtin/checkout.c ## -@@ builtin/checkout.c: static void update_refs_for_switch(const struct checkout_opts *opts, - opts->new_branch_force ? 1 : 0, - opts->new_branch_log, - opts->quiet, -- opts->track); -+ opts->track, -+ 0); - new_branch_info->name = opts->new_branch; - setup_branch_path(new_branch_info); - } - ## builtin/submodule--helper.c ## @@ #include "diff.h" @@ submodule-config.c: const struct submodule *submodule_from_path(struct repositor return config_from(r->submodule_cache, treeish_name, path, lookup_path); } -+void submodules_of_tree(struct repository *r, -+ const struct object_id *treeish_name, -+ struct submodule_entry_list *out) ++/** ++ * Used internally by submodules_of_tree(). Recurses into 'treeish_name' ++ * and appends submodule entries to 'out'. The submodule_cache expects ++ * a root-level treeish_name and paths, so keep track of these values ++ * with 'root_tree' and 'prefix'. ++ */ ++static void traverse_tree_submodules(struct repository *r, ++ const struct object_id *root_tree, ++ char *prefix, ++ const struct object_id *treeish_name, ++ struct submodule_entry_list *out) +{ + struct tree_desc tree; + struct submodule_tree_entry *st_entry; + struct name_entry *name_entry; ++ char *tree_path = NULL; + + name_entry = xmalloc(sizeof(*name_entry)); + ++ fill_tree_descriptor(r, &tree, treeish_name); ++ while (tree_entry(&tree, name_entry)) { ++ if (prefix) ++ tree_path = ++ mkpathdup("%s/%s", prefix, name_entry->path); ++ else ++ tree_path = xstrdup(name_entry->path); ++ ++ if (S_ISGITLINK(name_entry->mode) && ++ is_tree_submodule_active(r, root_tree, tree_path)) { ++ st_entry = xmalloc(sizeof(*st_entry)); ++ st_entry->name_entry = name_entry; ++ st_entry->submodule = ++ submodule_from_path(r, root_tree, tree_path); ++ st_entry->repo = xmalloc(sizeof(*st_entry->repo)); ++ if (repo_submodule_init(st_entry->repo, r, tree_path, ++ root_tree)) ++ FREE_AND_NULL(st_entry->repo); ++ ++ ALLOC_GROW(out->entries, out->entry_nr + 1, ++ out->entry_alloc); ++ out->entries[out->entry_nr++] = *st_entry; ++ } else if (S_ISDIR(name_entry->mode)) ++ traverse_tree_submodules(r, root_tree, tree_path, ++ &name_entry->oid, out); ++ free(tree_path); ++ } ++} ++ ++void submodules_of_tree(struct repository *r, ++ const struct object_id *treeish_name, ++ struct submodule_entry_list *out) ++{ + CALLOC_ARRAY(out->entries, 0); + out->entry_nr = 0; + out->entry_alloc = 0; + -+ fill_tree_descriptor(r, &tree, treeish_name); -+ while (tree_entry(&tree, name_entry)) { -+ if (!S_ISGITLINK(name_entry->mode) || !is_tree_submodule_active(r, treeish_name, name_entry->path)) { -+ continue; -+ } -+ -+ st_entry = xmalloc(sizeof(*st_entry)); -+ st_entry->name_entry = name_entry; -+ st_entry->submodule = -+ submodule_from_path(r, treeish_name, name_entry->path); -+ st_entry->repo = xmalloc(sizeof(*st_entry->repo)); -+ if (repo_submodule_init(st_entry->repo, r, name_entry->path, -+ treeish_name)) -+ FREE_AND_NULL(st_entry->repo); -+ -+ ALLOC_GROW(out->entries, out->entry_nr + 1, out->entry_alloc); -+ out->entries[out->entry_nr++] = *st_entry; -+ } ++ traverse_tree_submodules(r, treeish_name, NULL, treeish_name, out); +} + void submodule_free(struct repository *r) @@ submodule-config.h: int check_submodule_name(const char *name); +}; + +/** -+ * Given a treeish, return all submodules in the tree. This only reads -+ * one level of the tree, so it will not return nested submodules; -+ * callers that require nested submodules are expected to handle the -+ * recursion themselves. ++ * Given a treeish, return all submodules in the tree and its subtrees, ++ * but excluding nested submodules. Callers that require nested ++ * submodules are expected to recurse into the submodules themselves. + */ +void submodules_of_tree(struct repository *r, + const struct object_id *treeish_name, @@ t/t3207-branch-submodule.sh (new) + git init sub-sub-upstream && + test_commit -C sub-sub-upstream foo && + git init sub-upstream && ++ # Submodule in a submodule + git -C sub-upstream submodule add "$TRASH_DIRECTORY/sub-sub-upstream" sub-sub && + git -C sub-upstream commit -m "add submodule" && ++ # Regular submodule + git -C super submodule add "$TRASH_DIRECTORY/sub-upstream" sub && ++ # Submodule in a subdirectory ++ git -C super submodule add "$TRASH_DIRECTORY/sub-sub-upstream" second/sub && + git -C super commit -m "add submodule" && + git -C super config submodule.propagateBranches true && + git -C super/sub submodule update --init @@ t/t3207-branch-submodule.sh (new) + git branch --recurse-submodules branch-a && + git rev-parse branch-a && + git -C sub rev-parse branch-a && -+ git -C sub/sub-sub rev-parse branch-a ++ git -C sub/sub-sub rev-parse branch-a && ++ git -C second/sub rev-parse branch-a + ) +' + @@ t/t3207-branch-submodule.sh (new) + ) +' + -+test_expect_success 'should create branch when submodule is not in HEAD .gitmodules' ' ++test_expect_success 'should create branch when submodule is not in HEAD:.gitmodules' ' + test_when_finished "cleanup_branches super branch-a branch-b branch-c" && + ( + cd super && @@ t/t3207-branch-submodule.sh (new) + git branch --recurse-submodules branch-c branch-b && + git rev-parse branch-c && + git -C sub rev-parse branch-c && ++ git -C second/sub rev-parse branch-c && + git checkout --recurse-submodules branch-c && -+ git -C sub2 rev-parse branch-c ++ git -C sub2 rev-parse branch-c && ++ git -C sub2/sub-sub rev-parse branch-c + ) +' + 5: 70fb03f882 < -: ---------- branch.c: replace questionable exit() codes -- 2.33.GIT
