| Message ID | 20220129000446.99261-1-chooglen@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
Glen Choo <chooglen@google.com> writes: > 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. I had minor comments [1] which have been addressed, so: Reviewed-by: Jonathan Tan <jonathantanmy@google.com> I'm OK with 6/6 being included or not being included.
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