[PATCH v4 0/5] implement branch --recurse-submodules
- From
Glen Choo <chooglen@google.com>
- Date
- Dec 16, 2021, 00:32 UTC
- Message-ID
- <20211216003213.99135-1-chooglen@google.com>
- In-Reply-To
- <20211209184928.71413-1-chooglen@google.com>
Submodule branching RFC: https://lore.kernel.org/git/kl6lv912uvjv.fsf@chooglen-macbookpro.roam.corp.google.com/
Original Submodule UX RFC/Discussion: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/
Contributor Summit submodules Notes: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211148060.56@tvgsbejvaqbjf.bet/
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@chooglen-macbookpro.roam.corp.google.com/ [2] https://lore.kernel.org/git/kl6lv90ytd4v.fsf@chooglen-macbookpro.roam.corp.google.com/
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@google.com>
## 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@google.com>
@@ 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