| Message ID | 20210921232529.81811-3-chooglen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | branch: implement in-process --recurse-submodules | expand |
On Tue, Sep 21 2021, Glen Choo wrote: > static const char *head; > static struct object_id head_oid; > +static int recurse_submodules = 0; Nit: just s/ = 0// will do here, and is the convention typically... > + r_start_name = strcmp(start_name, "HEAD") ? start_name : > + refs_resolve_refdup(get_main_ref_store(r), start_name, 0, NULL, NULL); IMO clearer just as an if/else. > + if (strcmp(r_start_name, "HEAD")) > + skip_prefix(r_start_name, "refs/heads/", &r_start_name); > + create_branch(r, name, r_start_name, force, clobber_head_ok, reflog, > + quiet, track); > + > + if (!recurse_submodules) { > + return; > + } Can lose the braces here... > + > + if (repo_read_index(r) < 0) > + die(_("index file corrupt")); ...Just as you do here.. > + > + for (i = 0; i < r->index->cache_nr; i++) { > + const struct cache_entry *ce = r->index->cache[i]; > + if (!S_ISGITLINK(ce->ce_mode)) > + continue; > + sub = submodule_from_path(r, null_oid(), ce->name); > + if (repo_submodule_init(&subrepo, r, sub) < 0) { > + warning(_("Unable to initialize subrepo %s, skipping."), ce->name); > + continue; > + } > + /* > + * NEEDSWORK: branch tracking is not supported for > + * submodules because it is not possible to get the > + * remotes of a submodule. > + */ It isn't? $ git -C sha1collisiondetection/ remote -v origin https://github.com/cr-marcstevens/sha1collisiondetection.git > [...] > +test_expect_success 'setup superproject and submodule' ' > + git init parent && > + test_commit -C parent bar && > + git init child && > + test_commit -C child bar && > + git -C parent submodule add ../child sub && > + git -C parent commit -m "add submodule" > +' > + > +test_expect_success '--recurse-submodules should create branches' ' > + ( > + cd parent && > + git branch --recurse-submodules abc && > + test_path_is_file .git/refs/heads/abc && > + test_path_is_file .git/modules/sub/refs/heads/abc > + ) > +' All this manual file checking should depend on REFFILES, but better yet is there a reason this can't use rev-parse? I.e. why can't we inpect this state with 'for-each-ref', 'rev-parse' and the like? Does this test need to assert that these files end up in these specific locations, or just the ref store? Ditto for the later ones. > [...] > + cd parent && > + git -c branch.autoSetupMerge=always branch --recurse-submodules ghi main && > + test_path_is_file .git/modules/sub/refs/heads/ghi && > + test "$(git config branch.ghi.remote)" = . && > + test "$(git config branch.ghi.merge)" = refs/heads/main && > + test "z$(git -C ../child config branch.ghi.remote)" = z && > + test "z$(git -C ../child config branch.ghi.merge)" = z Use test_cmp, also for this sort of thing the test "x$y" = "x" idiom isn't needed unless you've got a computer from the mid-90s or something :)
Hi Glen, Le 2021-09-21 à 19:25, Glen Choo a écrit : > When working with a superproject and submodules, it can be helpful to > create topic branches to coordinate the work between each repository. > > Teach cmd_branch to accept the --recurse-submodules option when creating > branches. When specified, like > > git branch --recurse-submodules topic > > git-branch will create the "topic" branch in the superproject and all > submodules. > > recurse_submodules is only supported for creating a branch. git-branch small nit: we usually don't refer to the dashed form of commands anymore: 'git branch' would be preferable. > will fail if the user attempts to use --recurse-submodules with anything > other than creating a branch. If a user has submodule.recurse in their > config, git-branch will use --recurse-submodules for creating a branch, > but will treat it as unset for any other operation. That seems like a good starting point. However, I think that for this new feature, I would prefer *also* having a new config 'branch.recurseSubmodules' (or similar) that would allow more granular control than 'submodule.recurse', which influences several commands. Personnally I have 'submodule.recurse' set to true in my '~/.gitconfig', because I want the submodules working trees to be updated when I use 'git checkout' to change branches, 'git grep' to search the working tree, etc., but I usually do not *work* on the submodules in my project and I would not like new branches being created in them every time I create a new branch in the superproject. In other words, 'branch.recurseSubmodules=false' would have higher priority than 'submodule.recurse=true'. > > There is no easy way to get the remotes of a submodule because remotes.c > stores state as static variables. As such, branch tracking is disabled > when using --recurse-submodules. > > Signed-off-by: Glen Choo <chooglen@google.com> > --- > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index e575ffb4ff..858831d4cf 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -1421,5 +1421,63 @@ test_expect_success 'invalid sort parameter in configuration' ' > test_must_fail git branch > ) > ' > +# Submodule tests > + > Most tests for submodules are usually in separate test files. I don't think this is a set-in-stone rule, but if more tests are coming in the future, maybe a new test file t????-branch-submodule.sh would be appropriate ? Just a small suggestion. Thanks for working on this! It's exciting to see new developments in the submodule area :) Philippe.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> static const char *head; >> static struct object_id head_oid; >> +static int recurse_submodules = 0; > > Nit: just s/ = 0// will do here, and is the convention typically... Thanks for the catch! >> + r_start_name = strcmp(start_name, "HEAD") ? start_name : >> + refs_resolve_refdup(get_main_ref_store(r), start_name, 0, NULL, NULL); > > IMO clearer just as an if/else. Sounds good, I'll use an if/else instead >> + if (!recurse_submodules) { >> + return; >> + } > > Can lose the braces here... Ah, yes. Old habits die hard.. Thanks! >> + >> + for (i = 0; i < r->index->cache_nr; i++) { >> + const struct cache_entry *ce = r->index->cache[i]; >> + if (!S_ISGITLINK(ce->ce_mode)) >> + continue; >> + sub = submodule_from_path(r, null_oid(), ce->name); >> + if (repo_submodule_init(&subrepo, r, sub) < 0) { >> + warning(_("Unable to initialize subrepo %s, skipping."), ce->name); >> + continue; >> + } >> + /* >> + * NEEDSWORK: branch tracking is not supported for >> + * submodules because it is not possible to get the >> + * remotes of a submodule. >> + */ > > It isn't? > > $ git -C sha1collisiondetection/ remote -v > origin https://github.com/cr-marcstevens/sha1collisiondetection.git Ah, my comment is ambiguous. I meant that we cannot get submodule remotes in-process because remotes.c stores its state in static variables I believe it implicitly refers to the remotes of the_repository and can't be reused for submodules. Of course I hope I am wrong, because that would make this task a lot easier :) > All this manual file checking should depend on REFFILES, but better yet > is there a reason this can't use rev-parse? I.e. why can't we inpect > this state with 'for-each-ref', 'rev-parse' and the like? Does this test > need to assert that these files end up in these specific locations, or > just the ref store? Ditto for the later ones. > Use test_cmp, also for this sort of thing the test "x$y" = "x" idiom > isn't needed unless you've got a computer from the mid-90s or something > :) The only reason the tests look this way is that I have copied the surrounding tests. From your comments, it seems clear that these tests are fairly out-of-date, so I should probably model them after something else. I will incorporate Philippe's suggestion Most tests for submodules are usually in separate test files. I don't think this is a set-in-stone rule, but if more tests are coming in the future, maybe a new test file t????-branch-submodule.sh would be appropriate ? Just a small suggestion. Then at least my new tests won't look so out of place with the other branch tests. >> In this patchset, branching works slightly differently between the >> superproject and submodule (skip ahead for specifics). There are two >> very obvious alternatives that can address this: >> * A: only implement --recurse-submodules behavior after we are able to >> eliminate any kind of dependence on the_repository/global state that >> shouldn't be shared. >> * B: implement --recurse-submodules as child processes, which won't be >> bothered by global state. I was wondering if you had thoughts on this bit in particular. It seems unpleasant for branching to behave differently between superproject and submodule, so I'd like to discard this RFC (or at least the 'disable remotes' behavior) ASAP and start work on a version that serves us better.
Philippe Blain <levraiphilippeblain@gmail.com> writes: >> git-branch will create the "topic" branch in the superproject and all >> submodules. >> >> recurse_submodules is only supported for creating a branch. git-branch > > small nit: we usually don't refer to the dashed form of commands anymore: 'git branch' > would be preferable. Ah, thanks! I really appreciate the style pointers. > However, I think that for this new feature, I would prefer *also* having > a new config 'branch.recurseSubmodules' (or similar) that would allow > more granular control than 'submodule.recurse', which influences several > commands. Personnally I have 'submodule.recurse' set to true in my '~/.gitconfig', > because I want the submodules working trees to be updated when I use 'git checkout' > to change branches, 'git grep' to search the working tree, etc., but I usually > do not *work* on the submodules in my project and I would not like new branches > being created in them every time I create a new branch in the superproject. > In other words, 'branch.recurseSubmodules=false' would have higher priority > than 'submodule.recurse=true'. That's a good point. This behavior of git branch --recurse-submodules has the potential to directly conflict with established workflows. Opting out via 'branch.recurseSubmodules seems reasonable and I think we definitely want to give users that fine-grained control. I wonder if we should take this even further - perhaps --recurse-submodules is too disruptive to be controlled by the config value as checkout, fetch, pull, etc. Perhaps submodule.recurse should be more fine-grained, so we can ship good defaults for different workflows e.g. options that map to the current true/false behavior and another option that includes branching (I wish I could give these names, but I have no clue what to name them). > Most tests for submodules are usually in separate test files. I don't think > this is a set-in-stone rule, but if more tests are coming in the future, maybe > a new test file t????-branch-submodule.sh would be appropriate ? Just a small suggestion. I think that's a good call. It also looks like the tests in this file are somewhat old, so it would be nice to start on a clean slate :) >> In this patchset, branching works slightly differently between the >> superproject and submodule (skip ahead for specifics). There are two >> very obvious alternatives that can address this: >> * A: only implement --recurse-submodules behavior after we are able to >> eliminate any kind of dependence on the_repository/global state that >> shouldn't be shared. >> * B: implement --recurse-submodules as child processes, which won't be >> bothered by global state. Your comments on the UX make a lot of sense, and I'll think about that more carefully. Let me know if you also have thoughts on the implementation itself, particular on in-process vs child process :)
diff --git a/branch.c b/branch.c index 07a46430b3..1acf4aa407 100644 --- a/branch.c +++ b/branch.c @@ -137,6 +137,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, struct tracking tracking; int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; + if (track == BRANCH_TRACK_NEVER) + return; + memset(&tracking, 0, sizeof(tracking)); tracking.spec.dst = (char *)orig_ref; if (for_each_remote(find_tracked_branch, &tracking)) @@ -183,12 +186,12 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name) * Return 1 if the named branch already exists; return 0 otherwise. * Fill ref with the full refname for the branch. */ -int validate_branchname(const char *name, struct strbuf *ref) +int validate_branchname(struct repository *r, const char *name, struct strbuf *ref) { if (strbuf_check_branch_ref(ref, name)) die(_("'%s' is not a valid branch name."), name); - return ref_exists(ref->buf); + return refs_ref_exists(get_main_ref_store(r), ref->buf); } /* @@ -197,11 +200,11 @@ int validate_branchname(const char *name, struct strbuf *ref) * Return 1 if the named branch already exists; return 0 otherwise. * Fill ref with the full refname for the branch. */ -int validate_new_branchname(const char *name, struct strbuf *ref, int force) +int validate_new_branchname(struct repository *r, const char *name, struct strbuf *ref, int force) { const char *head; - if (!validate_branchname(name, ref)) + if (!validate_branchname(r, name, ref)) return 0; if (!force) @@ -209,6 +212,9 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force) ref->buf + strlen("refs/heads/")); head = resolve_ref_unsafe("HEAD", 0, NULL, NULL); + /* + * NEEDSWORK is_bare_repository() references the_repository. + */ if (!is_bare_repository() && head && !strcmp(head, ref->buf)) die(_("Cannot force update the current branch.")); @@ -260,8 +266,8 @@ void create_branch(struct repository *r, explicit_tracking = 1; if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok) - ? validate_branchname(name, &ref) - : validate_new_branchname(name, &ref, force)) { + ? validate_branchname(r, name, &ref) + : validate_new_branchname(r, name, &ref, force)) { if (!force) dont_change_ref = 1; else @@ -269,7 +275,7 @@ void create_branch(struct repository *r, } real_ref = NULL; - if (get_oid_mb(start_name, &oid)) { + if (repo_get_oid_mb(r, start_name, &oid)) { if (explicit_tracking) { if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) { error(_(upstream_missing), start_name); @@ -281,7 +287,7 @@ void create_branch(struct repository *r, die(_("Not a valid object name: '%s'."), start_name); } - switch (dwim_ref(start_name, strlen(start_name), &oid, &real_ref, 0)) { + switch (repo_dwim_ref(r, start_name, strlen(start_name), &oid, &real_ref, 0)) { case 0: /* Not branching from any existing branch */ if (explicit_tracking) @@ -319,12 +325,12 @@ void create_branch(struct repository *r, else msg = xstrfmt("branch: Created from %s", start_name); - transaction = ref_transaction_begin(&err); + transaction = ref_store_transaction_begin(get_main_ref_store(r), &err); if (!transaction || ref_transaction_update(transaction, ref.buf, &oid, forcing ? NULL : null_oid(), 0, msg, &err) || - ref_transaction_commit(transaction, &err)) + repo_ref_transaction_commit(r, transaction, &err)) die("%s", err.buf); ref_transaction_free(transaction); strbuf_release(&err); diff --git a/branch.h b/branch.h index df0be61506..5fb9ed7e03 100644 --- a/branch.h +++ b/branch.h @@ -50,7 +50,7 @@ void create_branch(struct repository *r, * Return 1 if the named branch already exists; return 0 otherwise. * Fill ref with the full refname for the branch. */ -int validate_branchname(const char *name, struct strbuf *ref); +int validate_branchname(struct repository *r, const char *name, struct strbuf *ref); /* * Check if a branch 'name' can be created as a new branch; die otherwise. @@ -58,7 +58,7 @@ int validate_branchname(const char *name, struct strbuf *ref); * Return 1 if the named branch already exists; return 0 otherwise. * Fill ref with the full refname for the branch. */ -int validate_new_branchname(const char *name, struct strbuf *ref, int force); +int validate_new_branchname(struct repository *r, const char *name, struct strbuf *ref, int force); /* * Remove information about the merge state on the current diff --git a/builtin/branch.c b/builtin/branch.c index 03c7b7253a..58a730b275 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -24,6 +24,7 @@ #include "worktree.h" #include "help.h" #include "commit-reach.h" +#include "submodule-config.h" static const char * const builtin_branch_usage[] = { N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"), @@ -38,6 +39,7 @@ static const char * const builtin_branch_usage[] = { static const char *head; static struct object_id head_oid; +static int recurse_submodules = 0; static int branch_use_color = -1; static char branch_colors[][COLOR_MAXLEN] = { @@ -100,6 +102,11 @@ static int git_branch_config(const char *var, const char *value, void *cb) return config_error_nonbool(var); return color_parse(value, branch_colors[slot]); } + if (!strcmp(var, "submodule.recurse")) { + recurse_submodules = git_config_bool(var, value); + return 0; + } + return git_color_default_config(var, value, cb); } @@ -190,6 +197,50 @@ static void delete_branch_config(const char *branchname) strbuf_release(&buf); } +static void create_branches(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 i = 0; + struct repository subrepo; + const char *r_start_name; + const struct submodule *sub; + + r_start_name = strcmp(start_name, "HEAD") ? start_name : + refs_resolve_refdup(get_main_ref_store(r), start_name, 0, NULL, NULL); + if (strcmp(r_start_name, "HEAD")) + skip_prefix(r_start_name, "refs/heads/", &r_start_name); + create_branch(r, name, r_start_name, force, clobber_head_ok, reflog, + quiet, track); + + if (!recurse_submodules) { + return; + } + + if (repo_read_index(r) < 0) + die(_("index file corrupt")); + + for (i = 0; i < r->index->cache_nr; i++) { + const struct cache_entry *ce = r->index->cache[i]; + if (!S_ISGITLINK(ce->ce_mode)) + continue; + sub = submodule_from_path(r, null_oid(), ce->name); + if (repo_submodule_init(&subrepo, r, sub) < 0) { + warning(_("Unable to initialize subrepo %s, skipping."), ce->name); + continue; + } + /* + * NEEDSWORK: branch tracking is not supported for + * submodules because it is not possible to get the + * remotes of a submodule. + */ + create_branches(&subrepo, name, start_name, force, + clobber_head_ok, reflog, quiet, BRANCH_TRACK_NEVER); + repo_clear(&subrepo); + } +} + static int delete_branches(int argc, const char **argv, int force, int kinds, int quiet) { @@ -531,9 +582,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int * cause the worktree to become inconsistent with HEAD, so allow it. */ if (!strcmp(oldname, newname)) - validate_branchname(newname, &newref); + validate_branchname(the_repository, newname, &newref); else - validate_new_branchname(newname, &newref, force); + validate_new_branchname(the_repository, newname, &newref, force); reject_rebase_or_bisect_branch(oldref.buf); @@ -626,6 +677,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) int icase = 0; static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; struct ref_format format = REF_FORMAT_INIT; + int recurse_submodules_explicit = 0; struct option options[] = { OPT_GROUP(N_("Generic options")), @@ -669,6 +721,7 @@ 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), OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")), + OPT_BOOL(0, "recurse-submodules", &recurse_submodules_explicit, N_("recurse through submodules")), OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")), OPT_END(), }; @@ -708,6 +761,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current + list + edit_description + unset_upstream > 1) usage_with_options(builtin_branch_usage, options); + else if (recurse_submodules_explicit && + (delete || rename || copy || new_upstream || show_current || list + || edit_description || unset_upstream)) + die(_("--recurse-submodules can only be used to create branches")); + + recurse_submodules |= !!recurse_submodules_explicit; if (filter.abbrev == -1) filter.abbrev = DEFAULT_ABBREV; @@ -857,9 +916,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (track == BRANCH_TRACK_OVERRIDE) die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.")); - create_branch(the_repository, - argv[0], (argc == 2) ? argv[1] : head, - force, 0, reflog, quiet, track); + create_branches(the_repository, + argv[0], (argc == 2) ? argv[1] : "HEAD", + force, 0, reflog, quiet, track); } else usage_with_options(builtin_branch_usage, options); diff --git a/builtin/checkout.c b/builtin/checkout.c index 8c69dcdf72..8859076a14 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1761,10 +1761,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix, struct strbuf buf = STRBUF_INIT; if (opts->new_branch_force) - opts->branch_exists = validate_branchname(opts->new_branch, &buf); + opts->branch_exists = validate_branchname(the_repository, opts->new_branch, &buf); else opts->branch_exists = - validate_new_branchname(opts->new_branch, &buf, 0); + validate_new_branchname(the_repository, opts->new_branch, &buf, 0); strbuf_release(&buf); } diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index e575ffb4ff..858831d4cf 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1421,5 +1421,63 @@ test_expect_success 'invalid sort parameter in configuration' ' test_must_fail git branch ) ' +# Submodule tests + +test_expect_success 'setup superproject and submodule' ' + git init parent && + test_commit -C parent bar && + git init child && + test_commit -C child bar && + git -C parent submodule add ../child sub && + git -C parent commit -m "add submodule" +' + +test_expect_success '--recurse-submodules should create branches' ' + ( + cd parent && + git branch --recurse-submodules abc && + test_path_is_file .git/refs/heads/abc && + test_path_is_file .git/modules/sub/refs/heads/abc + ) +' + +test_expect_success '--recurse-submodules should fail when not creating branches' ' + ( + cd parent && + test_must_fail git branch --recurse-submodules -D abc && + test_path_is_file .git/refs/heads/abc && + test_path_is_file .git/modules/sub/refs/heads/abc + ) +' + +test_expect_success 'should respect submodule.recurse when creating branches' ' + ( + cd parent && + git -c submodule.recurse=true branch def && + test_path_is_file .git/refs/heads/def && + test_path_is_file .git/modules/sub/refs/heads/def + ) +' + +test_expect_success 'should ignore submodule.recurse when not creating branches' ' + ( + cd parent && + git -c submodule.recurse=true branch -D def && + test_path_is_missing .git/refs/heads/def && + test_path_is_file .git/modules/sub/refs/heads/def + ) +' + +test_expect_success 'should not set up tracking for new submodule branches' ' + ( + cd parent && + git -c branch.autoSetupMerge=always branch --recurse-submodules ghi main && + test_path_is_file .git/modules/sub/refs/heads/ghi && + test "$(git config branch.ghi.remote)" = . && + test "$(git config branch.ghi.merge)" = refs/heads/main && + test "z$(git -C ../child config branch.ghi.remote)" = z && + test "z$(git -C ../child config branch.ghi.merge)" = z + ) +' test_done
When working with a superproject and submodules, it can be helpful to create topic branches to coordinate the work between each repository. Teach cmd_branch to accept the --recurse-submodules option when creating branches. When specified, like git branch --recurse-submodules topic git-branch will create the "topic" branch in the superproject and all submodules. recurse_submodules is only supported for creating a branch. git-branch will fail if the user attempts to use --recurse-submodules with anything other than creating a branch. If a user has submodule.recurse in their config, git-branch will use --recurse-submodules for creating a branch, but will treat it as unset for any other operation. There is no easy way to get the remotes of a submodule because remotes.c stores state as static variables. As such, branch tracking is disabled when using --recurse-submodules. Signed-off-by: Glen Choo <chooglen@google.com> --- branch.c | 26 ++++++++++------- branch.h | 4 +-- builtin/branch.c | 69 ++++++++++++++++++++++++++++++++++++++++++---- builtin/checkout.c | 4 +-- t/t3200-branch.sh | 58 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 19 deletions(-)