[PATCH v8 0/6] implement branch --recurse-submodules
- From
Glen Choo <chooglen@google.com>
- Date
- Jan 29, 2022, 00:04 UTC
- Message-ID
- <20220129000446.99261-1-chooglen@google.com>
- In-Reply-To
- <20220124204442.39353-1-chooglen@google.com>
Thanks again for the feedback, Jonathan!
This version is rebased onto the updated js/branch-track-inherit, from 44f14a9d24 (config: require lowercase for branch.*.autosetupmerge, 2021-12-20) to 6327f0efed (branch,checkout: fix --track documentation, 2022-01-20). As such, the range-diff for patch 5 shows some additional changes in the usage string lines.
Junio spotted some issues in the context that are worth fixing, but out of scope for this series (https://lore.kernel.org/git/xmqqfsp8zuyg.fsf@gitster.g). I plan to send fixes for them after this series is merged. Patch 6 arguably fits in better with that fixup series than this one, but I'll leave the decision to keep/drop it to Junio.
= Description
This series implements "git branch --recurse-submodules", which is part of the submodule branching RFC (see "See also"). "git branch" (and some other features in that RFC) are incompatible with existing "--recurse-submodules" commands; patch 5 describes this incompatibility in more detail and how we plan to introduce this new functionality in a manner.
This series is based off js/branch-track-inherit.
Patches 1-4 are preparation for the implementation of "branch --recurse-submodules" in patch 5. Patch 6 cleans up a memory leak that I encountered on js/branch-track-inherit as I was moving code around; it isn't neeeded by "branch --recurse-submodules".
See the previous cover letter (https://lore.kernel.org/git/20211220233459.45739-1-chooglen@google.com) for discussions of future work.
= See also
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
= Changes
Changes since v7: * Reword patch 5's commit message * In comments and Documentation/git-branch.txt, clarify how branch --recurse-submodules behaves and rename a parameter to better reflect this behavior * In t3207, always assert on error messages when using test_must_fail * In t3207, assert on config values using test_cmp_config. This avoids the silent failures discouraged by t/README:671. * In patch 6, be more explicit about freeing the stack-allocated variable.
Changes since v6: * Move memory leak fix into its own patch (patch 6) * Eliminate unnecessary rewrapping and small style fixes * In branch --recurse-submodules tests, setup test repo anew instead of deleting the unused branches * Numerous commit message wording changes
Changes since v5: * Rebase onto v7 of js/branch-track-inherit (https://lore.kernel.org/git/cover.1639717481.git.steadmon@google.com)
Changes since v4:
* Rebase correctly onto 'gitster/seen^{/^Merge branch .js/branch-track-inherit.}'
(see base-commit) as suggested in [1] (thanks Junio!)
* These patches were also verified on top of 'next'.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/xmqqlf0lz6os.fsf@gitster.g
Glen Choo (6): 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: consolidate action-picking logic in cmd_branch() branch: add --recurse-submodules option for branch creation branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks
Documentation/config/advice.txt | 3 + Documentation/config/submodule.txt | 37 ++-- Documentation/git-branch.txt | 19 +- advice.c | 1 + advice.h | 1 + branch.c | 277 ++++++++++++++++++++++----- branch.h | 60 +++++- 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 | 292 +++++++++++++++++++++++++++++ 16 files changed, 845 insertions(+), 81 deletions(-) create mode 100755 t/t3207-branch-submodule.sh
Range-diff against v7:
1: 206175cfb3 = 1: 547055d55e branch: move --set-upstream-to behavior to dwim_and_setup_tracking()
2: 2e02885eb3 = 2: 016f62681d branch: make create_branch() always create a branch
3: cd43a9aaaa = 3: a459a030f6 branch: add a dry_run parameter to create_branch()
4: 8e5f750162 = 4: 4903f8eb4b builtin/branch: consolidate action-picking logic in cmd_branch()
5: c59de1fd9c ! 5: b95b77b7c1 branch: add --recurse-submodules option for branch creation
@@ Commit message
topic" will create the `topic` branch in the superproject and its
submodules.
- Although this commit does not introduce breaking changes, it is
- incompatible with existing --recurse-submodules commands because "git
+ Although this commit does not introduce breaking changes, it does not
+ work well with existing --recurse-submodules commands because "git
branch --recurse-submodules" writes to the submodule ref store, but most
commands only consider the superproject gitlink and ignore the submodule
ref store. For example, "git checkout --recurse-submodules" will check
@@ 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]
+-'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>]
@@ Documentation/git-branch.txt: how the `branch.<name>.remote` and `branch.<name>.
+ 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.
++ linkgit:git-config[1]. Currently, only branch creation is
++ supported.
+++
++When used in branch creation, a new branch <branchname> will be created
++in the superproject and all of the submodules in the superproject's
++<start-point>. In submodules, the branch will point to the submodule
++commit in the superproject's <start-point> but the branch's tracking
++information will be set up based on the submodule's branches and remotes
++e.g. `git branch --recurse-submodules topic origin/main` will create the
++submodule branch "topic" that points to the submodule commit in the
++superproject's "origin/main", but tracks the submodule's "origin/main".
+
--set-upstream::
As this option had confusing syntax, it is no longer supported.
@@ branch.c: void dwim_and_setup_tracking(struct repository *r, const char *new_ref
+static int submodule_create_branch(struct repository *r,
+ const struct submodule *submodule,
+ const char *name, const char *start_oid,
-+ const char *start_name, int force,
++ const char *tracking_name, int force,
+ int reflog, int quiet,
+ enum branch_track track, int dry_run)
+{
@@ branch.c: void dwim_and_setup_tracking(struct repository *r, const char *new_ref
+ /*
+ * submodule_create_branch() is indirectly invoked by "git
+ * branch", but we cannot invoke "git branch" in the child
-+ * process because it does not let us set start_name and
-+ * start_oid separately (see create_branches_recursively()).
++ * process. "git branch" accepts a branch name and start point,
++ * where the start point is assumed to provide both the OID
++ * (start_oid) and the branch to use for tracking
++ * (tracking_name). But when recursing through submodules,
++ * start_oid and tracking name need to be specified separately
++ * (see create_branches_recursively()).
+ */
+ strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);
+ if (dry_run)
@@ branch.c: void dwim_and_setup_tracking(struct repository *r, const char *new_ref
+ if (track == BRANCH_TRACK_ALWAYS || track == BRANCH_TRACK_EXPLICIT)
+ strvec_push(&child.args, "--track");
+
-+ strvec_pushl(&child.args, name, start_oid, start_name, NULL);
++ strvec_pushl(&child.args, name, start_oid, tracking_name, NULL);
+
+ if ((ret = start_command(&child)))
+ return ret;
@@ branch.c: void dwim_and_setup_tracking(struct repository *r, const char *new_ref
+}
+
+void create_branches_recursively(struct repository *r, const char *name,
-+ const char *start_name,
++ const char *start_commitish,
+ const char *tracking_name, int force,
+ int reflog, int quiet, enum branch_track track,
+ int dry_run)
@@ branch.c: void dwim_and_setup_tracking(struct repository *r, const char *new_ref
+ struct object_id super_oid;
+ struct submodule_entry_list submodule_entry_list;
+
-+ /* Perform dwim on start_name to get super_oid and branch_point. */
-+ dwim_branch_start(r, start_name, BRANCH_TRACK_NEVER, &branch_point,
-+ &super_oid);
++ /* Perform dwim on start_commitish to get super_oid and branch_point. */
++ dwim_branch_start(r, start_commitish, 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 dwim_and_setup_tracking(struct repository *r, const char *new_ref
+ if (submodule_entry_list.entries[i].repo == NULL) {
+ if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
+ advise(_("You may try updating the submodules using 'git checkout %s && git submodule update --init'"),
-+ start_name);
++ start_commitish);
+ die(_("submodule '%s': unable to find submodule"),
+ submodule_entry_list.entries[i].submodule->name);
+ }
@@ branch.c: void dwim_and_setup_tracking(struct repository *r, const char *new_ref
+ name);
+ }
+
-+ create_branch(the_repository, name, start_name, force, 0, reflog, quiet,
++ create_branch(the_repository, name, start_commitish, force, 0, reflog, quiet,
+ BRANCH_TRACK_NEVER, dry_run);
+ if (dry_run)
+ return;
@@ branch.h: void create_branch(struct repository *r,
int dry_run);
+/*
-+ * Creates a new branch in repository and its submodules (and its
-+ * submodules, recursively). Besides these exceptions, the parameters
-+ * function identically to create_branch():
++ * Creates a new branch in a repository and its submodules (and its
++ * submodules, recursively). The parameters are mostly analogous to
++ * those of create_branch() except for start_name, which is represented
++ * by two different parameters:
+ *
-+ * - start_name is the name of the ref, in repository r, that the new
-+ * branch should start from. In submodules, branches will start from
-+ * the respective gitlink commit ids in start_name's tree.
++ * - start_commitish is the commit-ish, in repository r, that determines
++ * which commits the branches will point to. The superproject branch
++ * will point to the commit of start_commitish and the submodule
++ * branches will point to the gitlink commit oids in start_commitish's
++ * tree.
+ *
-+ * - tracking_name is the name used of the ref that will be used to set
-+ * up tracking, e.g. origin/main. This is propagated to submodules so
-+ * that tracking information will appear as if the branch branched off
-+ * tracking_name instead of start_name (which is a plain commit id for
-+ * submodules). If omitted, start_name is used for tracking (just like
-+ * create_branch()).
++ * - tracking_name is the name of the ref, in repository r, that will be
++ * used to set up tracking information. This value is propagated to
++ * all submodules, which will evaluate the ref using their own ref
++ * stores. If NULL, this defaults to start_commitish.
+ *
++ * When this function is called on the superproject, start_commitish
++ * can be any user-provided ref and tracking_name can be NULL (similar
++ * to create_branches()). But when recursing through submodules,
++ * start_commitish is the plain gitlink commit oid. Since the oid cannot
++ * be used for tracking information, tracking_name is propagated and
++ * used for tracking instead.
+ */
+void create_branches_recursively(struct repository *r, const char *name,
-+ const char *start_name,
++ const char *start_commitish,
+ const char *tracking_name, int force,
+ int reflog, int quiet, enum branch_track track,
+ int dry_run);
@@ t/t3207-branch-submodule.sh (new)
+ cp -r test_dirs/* .
+}
+
++# Tests that the expected branch does not exist
++test_no_branch () {
++ DIR=$1 &&
++ BRANCH_NAME=$2 &&
++ test_must_fail git -C "$DIR" rev-parse "$BRANCH_NAME" 2>err &&
++ grep "ambiguous argument .$BRANCH_NAME." err
++}
++
+test_expect_success 'setup superproject and submodule' '
+ mkdir test_dirs &&
+ (
@@ t/t3207-branch-submodule.sh (new)
+ (
+ cd super &&
+ git branch --recurse-submodules branch-a &&
-+ test_must_fail git branch --recurse-submodules -D branch-a &&
++ echo "fatal: --recurse-submodules can only be used to create branches" >expected &&
++ test_must_fail git branch --recurse-submodules -D branch-a 2>actual &&
++ test_cmp expected actual &&
+ # Assert that the branches were not deleted
+ git rev-parse branch-a &&
+ git -C sub rev-parse branch-a
@@ t/t3207-branch-submodule.sh (new)
+ cd super &&
+ git branch --recurse-submodules branch-a &&
+ git -c submodule.recurse=true branch -D branch-a &&
-+ test_must_fail git rev-parse branch-a &&
++ test_no_branch . branch-a &&
+ git -C sub rev-parse branch-a
+ )
+'
@@ t/t3207-branch-submodule.sh (new)
+ cd super &&
+ git -C sub branch branch-a &&
+ test_must_fail git branch --recurse-submodules branch-a 2>actual &&
-+ test_must_fail git rev-parse branch-a &&
++ test_no_branch . branch-a &&
+ grep "submodule .sub.: fatal: A branch named .branch-a. already exists" actual
+ )
+'
@@ t/t3207-branch-submodule.sh (new)
+ cd super &&
+ git branch --recurse-submodules branch-a &&
+ git rev-parse branch-a &&
-+ test_must_fail git -C sub branch-a
++ test_no_branch sub branch-a
+ )
+'
+
@@ t/t3207-branch-submodule.sh (new)
+ cd super &&
+ git -c branch.autoSetupMerge=always branch --recurse-submodules branch-a main &&
+ git -C sub rev-parse main &&
-+ test "$(git -C sub config branch.branch-a.remote)" = . &&
-+ test "$(git -C sub config branch.branch-a.merge)" = refs/heads/main
++ test_cmp_config -C sub . branch.branch-a.remote &&
++ test_cmp_config -C sub refs/heads/main branch.branch-a.merge
+ )
+'
+
@@ t/t3207-branch-submodule.sh (new)
+ cd super &&
+ git branch --track --recurse-submodules branch-a main &&
+ git -C sub rev-parse main &&
-+ test "$(git -C sub config branch.branch-a.remote)" = . &&
-+ test "$(git -C sub config branch.branch-a.merge)" = refs/heads/main
++ test_cmp_config -C sub . branch.branch-a.remote &&
++ test_cmp_config -C sub refs/heads/main branch.branch-a.merge
+ )
+'
+
@@ t/t3207-branch-submodule.sh (new)
+ cd super &&
+ git branch --recurse-submodules branch-a main &&
+ git -C sub rev-parse main &&
-+ test "$(git -C sub config branch.branch-a.remote)" = "" &&
-+ test "$(git -C sub config branch.branch-a.merge)" = ""
++ test_cmp_config -C sub "" --default "" branch.branch-a.remote &&
++ test_cmp_config -C sub "" --default "" branch.branch-a.merge
+ )
+'
+
@@ t/t3207-branch-submodule.sh (new)
+ # This should fail because super-clone does not have sub2 .git/modules
+ test_must_fail git branch --recurse-submodules branch-b origin/branch-b 2>actual &&
+ grep "fatal: submodule .sub2.: unable to find submodule" actual &&
-+ test_must_fail git rev-parse branch-b &&
-+ test_must_fail git -C sub rev-parse branch-b &&
++ test_no_branch . branch-b &&
++ test_no_branch sub branch-b &&
+ # User can fix themselves by initializing the submodule
+ git checkout origin/branch-b &&
+ git submodule update --init --recursive &&
@@ t/t3207-branch-submodule.sh (new)
+ (
+ cd super-clone &&
+ git branch --recurse-submodules branch-a origin/branch-a &&
-+ test "$(git config branch.branch-a.remote)" = origin &&
-+ test "$(git config branch.branch-a.merge)" = refs/heads/branch-a &&
++ test_cmp_config origin branch.branch-a.remote &&
++ test_cmp_config refs/heads/branch-a branch.branch-a.merge &&
+ # "origin/branch-a" does not exist for "sub", but it matches the refspec
+ # so tracking should be set up
-+ test "$(git -C sub config branch.branch-a.remote)" = origin &&
-+ test "$(git -C sub config branch.branch-a.merge)" = refs/heads/branch-a &&
-+ test "$(git -C sub/sub-sub config branch.branch-a.remote)" = origin &&
-+ test "$(git -C sub/sub-sub config branch.branch-a.merge)" = refs/heads/branch-a
++ test_cmp_config -C sub origin branch.branch-a.remote &&
++ test_cmp_config -C sub refs/heads/branch-a branch.branch-a.merge &&
++ test_cmp_config -C sub/sub-sub origin branch.branch-a.remote &&
++ test_cmp_config -C sub/sub-sub refs/heads/branch-a branch.branch-a.merge
+ )
+'
+
@@ t/t3207-branch-submodule.sh (new)
+ cd super-clone &&
+ git remote rename origin ex-origin &&
+ git branch --recurse-submodules branch-a ex-origin/branch-a &&
-+ test "$(git config branch.branch-a.remote)" = ex-origin &&
-+ test "$(git config branch.branch-a.merge)" = refs/heads/branch-a &&
-+ test "$(git -C sub config branch.branch-a.remote)" = "" &&
-+ test "$(git -C sub config branch.branch-a.merge)" = ""
++ test_cmp_config ex-origin branch.branch-a.remote &&
++ test_cmp_config refs/heads/branch-a branch.branch-a.merge &&
++ test_cmp_config -C sub "" --default "" branch.branch-a.remote &&
++ test_cmp_config -C sub "" --default "" branch.branch-a.merge
+ )
+'
+
6: a4634f0493 ! 6: bdf7be52be branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks
@@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
tracking.remote, tracking.srcs) < 0)
exit(-1);
+- string_list_clear(tracking.srcs, 0);
+cleanup:
- string_list_clear(tracking.srcs, 0);
++ string_list_clear(&tracking_srcs, 0);
}
+ int read_branch_desc(struct strbuf *buf, const char *branch_name)base-commit: 6327f0efed36c64d98a140110171362b7cb75a52
-- 2.33.GIT