git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/4] implement branch --recurse-submodules
@ 2021-11-22 22:32 Glen Choo
  2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
                   ` (3 more replies)
  0 siblings, 4 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-22 22:32 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, Josh Steadmon, Emily Shaffer, Glen Choo

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 uses child processes to support submodules. I initially
hoped to do this in-core and [1] and [2] were meant to prepare for that.
But even though in-core is tantalizingly close, [1] showed that there
is more work to be done on config.c before this is possible, and I would
like to get more feedback on the UX before converting this to in-core.

[1] https://lore.kernel.org/git/20211111171643.13805-1-chooglen@google.com/
[2] https://lore.kernel.org/git/20211118005325.64971-1-chooglen@google.com/

Glen Choo (4):
  submodule-config: add submodules_of_tree() helper
  branch: refactor out branch validation from create_branch()
  branch: add --dry-run option to branch
  branch: add --recurse-submodules option for branch creation

 Documentation/config/advice.txt    |   3 +
 Documentation/config/submodule.txt |   9 +
 Documentation/git-branch.txt       |   8 +-
 advice.c                           |   1 +
 advice.h                           |   1 +
 branch.c                           | 300 +++++++++++++++++++++--------
 branch.h                           |  41 +++-
 builtin/branch.c                   |  77 ++++++--
 builtin/submodule--helper.c        |  33 ++++
 submodule-config.c                 |  19 ++
 submodule-config.h                 |  13 ++
 t/t3200-branch.sh                  |  30 +++
 t/t3207-branch-submodule.sh        | 249 ++++++++++++++++++++++++
 13 files changed, 678 insertions(+), 106 deletions(-)
 create mode 100755 t/t3207-branch-submodule.sh

-- 
2.33.GIT


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/4] submodule-config: add submodules_of_tree() helper
  2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
@ 2021-11-22 22:32 ` Glen Choo
  2021-11-23  2:12   ` Jonathan Tan
                     ` (2 more replies)
  2021-11-22 22:32 ` [PATCH 2/4] branch: refactor out branch validation from create_branch() Glen Choo
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-22 22:32 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, Josh Steadmon, Emily Shaffer, Glen Choo

As we introduce a submodule UX with branches, we would like to be able
to get the submodule commit ids in a superproject tree because those ids
are the source of truth e.g. "git branch --recurse-submodules topic
start-point" should create branches based off the commit ids recorded in
the superproject's 'start-point' tree.

To make this easy, introduce a submodules_of_tree() helper function that
iterates through a tree and returns the tree's gitlink entries as a
list.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 submodule-config.c | 19 +++++++++++++++++++
 submodule-config.h | 13 +++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index f95344028b..97da373301 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -7,6 +7,7 @@
 #include "strbuf.h"
 #include "object-store.h"
 #include "parse-options.h"
+#include "tree-walk.h"
 
 /*
  * submodule cache lookup structure
@@ -726,6 +727,24 @@ const struct submodule *submodule_from_path(struct repository *r,
 	return config_from(r->submodule_cache, treeish_name, path, lookup_path);
 }
 
+struct submodule_entry_list *
+submodules_of_tree(struct repository *r, const struct object_id *treeish_name)
+{
+	struct tree_desc tree;
+	struct name_entry entry;
+	struct submodule_entry_list *ret;
+
+	CALLOC_ARRAY(ret, 1);
+	fill_tree_descriptor(r, &tree, treeish_name);
+	while (tree_entry(&tree, &entry)) {
+		if (!S_ISGITLINK(entry.mode))
+			continue;
+		ALLOC_GROW(ret->name_entries, ret->entry_nr + 1, ret->entry_alloc);
+		ret->name_entries[ret->entry_nr++] = entry;
+	}
+	return ret;
+}
+
 void submodule_free(struct repository *r)
 {
 	if (r->submodule_cache)
diff --git a/submodule-config.h b/submodule-config.h
index 65875b94ea..4379ec77e3 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -6,6 +6,7 @@
 #include "hashmap.h"
 #include "submodule.h"
 #include "strbuf.h"
+#include "tree-walk.h"
 
 /**
  * The submodule config cache API allows to read submodule
@@ -67,6 +68,18 @@ const struct submodule *submodule_from_name(struct repository *r,
 					    const struct object_id *commit_or_tree,
 					    const char *name);
 
+struct submodule_entry_list {
+	struct name_entry *name_entries;
+	int entry_nr;
+	int entry_alloc;
+};
+
+/**
+ * Given a tree-ish, return all submodules in the tree.
+ */
+struct submodule_entry_list *
+submodules_of_tree(struct repository *r, const struct object_id *treeish_name);
+
 /**
  * Given a tree-ish in the superproject and a path, return the submodule that
  * is bound at the path in the named tree.
-- 
2.33.GIT


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/4] branch: refactor out branch validation from create_branch()
  2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
  2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
@ 2021-11-22 22:32 ` Glen Choo
  2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
  2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
  3 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-22 22:32 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, Josh Steadmon, Emily Shaffer, Glen Choo

In a subsequent commit, we would like to be able to validate whether or
not a branch name is valid before we create it (--dry-run). This is
useful for `git branch --recurse-submodules topic` because it allows Git
to determine if the branch 'topic' can be created in all submodules
without creating the branch 'topic'.

A good starting point would be to refactor out the start point
validation and dwim logic in create_branch() in a
validate_branch_start() helper function. Once we do so, it becomes
clear that create_branch() is more complex than it needs to be -
create_branch() is also used to set tracking information when performing
`git branch --set-upstream-to`. This made more sense when
(the now unsupported) --set-upstream was first introduced in
4fc5006676 (Add branch --set-upstream, 2010-01-18), because
it would sometimes create a branch and sometimes update tracking
information without creating a branch.

Refactor out the branch validation and dwim logic from create_branch()
into validate_branch_start(), make it so that create_branch() always
tries to create a branch, and replace the now-incorrect create_branch()
call with setup_tracking(). Since there were none, add tests for
creating a branch with `--force`.

Signed-off-by: Glen Choo <chooglen@google.com>
---
In this refactor, I preserved the existing behavior by making
setup_tracking() call validate_branch_start(). setup_tracking() needs
the dwim behavior e.g. to expand 'origin/main' into
'refs/remotes/origin/main' but I'm doubtful that it needs the exact same
set of validation behavior as creating a new branch e.g. validating that
the object_id is a commit.

 branch.c          | 177 ++++++++++++++++++++++++----------------------
 branch.h          |  13 +++-
 builtin/branch.c  |   7 +-
 t/t3200-branch.sh |  17 +++++
 4 files changed, 121 insertions(+), 93 deletions(-)

diff --git a/branch.c b/branch.c
index 07a46430b3..f8b755513f 100644
--- a/branch.c
+++ b/branch.c
@@ -126,43 +126,6 @@ int install_branch_config(int flag, const char *local, const char *origin, const
 	return -1;
 }
 
-/*
- * This is called when new_ref is branched off of orig_ref, and tries
- * to infer the settings for branch.<new_ref>.{remote,merge} from the
- * config.
- */
-static void setup_tracking(const char *new_ref, const char *orig_ref,
-			   enum branch_track track, int quiet)
-{
-	struct tracking tracking;
-	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
-
-	memset(&tracking, 0, sizeof(tracking));
-	tracking.spec.dst = (char *)orig_ref;
-	if (for_each_remote(find_tracked_branch, &tracking))
-		return;
-
-	if (!tracking.matches)
-		switch (track) {
-		case BRANCH_TRACK_ALWAYS:
-		case BRANCH_TRACK_EXPLICIT:
-		case BRANCH_TRACK_OVERRIDE:
-			break;
-		default:
-			return;
-		}
-
-	if (tracking.matches > 1)
-		die(_("Not tracking: ambiguous information for ref %s"),
-		    orig_ref);
-
-	if (install_branch_config(config_flags, new_ref, tracking.remote,
-			      tracking.src ? tracking.src : orig_ref) < 0)
-		exit(-1);
-
-	free(tracking.src);
-}
-
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
 	char *v = NULL;
@@ -243,33 +206,17 @@ N_("\n"
 "will track its remote counterpart, you may want to use\n"
 "\"git push -u\" to set the upstream config as you push.");
 
-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)
+static void validate_branch_start(struct repository *r, const char *start_name,
+				  enum branch_track track,
+				  struct object_id *oid, char **full_ref)
 {
 	struct commit *commit;
-	struct object_id oid;
-	char *real_ref;
-	struct strbuf ref = STRBUF_INIT;
-	int forcing = 0;
-	int dont_change_ref = 0;
 	int explicit_tracking = 0;
 
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;
 
-	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
-	    ? validate_branchname(name, &ref)
-	    : validate_new_branchname(name, &ref, force)) {
-		if (!force)
-			dont_change_ref = 1;
-		else
-			forcing = 1;
-	}
-
-	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 +228,8 @@ 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, full_ref,
+			      0)) {
 	case 0:
 		/* Not branching from any existing branch */
 		if (explicit_tracking)
@@ -289,12 +237,12 @@ void create_branch(struct repository *r,
 		break;
 	case 1:
 		/* Unique completion -- good, only if it is a real branch */
-		if (!starts_with(real_ref, "refs/heads/") &&
-		    validate_remote_tracking_branch(real_ref)) {
+		if (!starts_with(*full_ref, "refs/heads/") &&
+		    validate_remote_tracking_branch(*full_ref)) {
 			if (explicit_tracking)
 				die(_(upstream_not_branch), start_name);
 			else
-				FREE_AND_NULL(real_ref);
+				FREE_AND_NULL(*full_ref);
 		}
 		break;
 	default:
@@ -302,37 +250,96 @@ void create_branch(struct repository *r,
 		break;
 	}
 
-	if ((commit = lookup_commit_reference(r, &oid)) == NULL)
+	if ((commit = lookup_commit_reference(r, oid)) == NULL)
 		die(_("Not a valid branch point: '%s'."), start_name);
-	oidcpy(&oid, &commit->object.oid);
+	oidcpy(oid, &commit->object.oid);
+}
+
+void setup_tracking(const char *new_ref, const char *orig_ref,
+			   enum branch_track track, int quiet, int expand_orig)
+{
+	struct tracking tracking;
+	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	char *full_orig_ref;
+	struct object_id unused_oid;
+
+	memset(&tracking, 0, sizeof(tracking));
+	if (expand_orig)
+		validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
+	else
+		full_orig_ref = xstrdup(orig_ref);
+
+	tracking.spec.dst = full_orig_ref;
+	if (for_each_remote(find_tracked_branch, &tracking))
+		goto cleanup;
+
+	if (!tracking.matches)
+		switch (track) {
+		case BRANCH_TRACK_ALWAYS:
+		case BRANCH_TRACK_EXPLICIT:
+		case BRANCH_TRACK_OVERRIDE:
+			break;
+		default:
+			goto cleanup;
+		}
+
+	if (tracking.matches > 1)
+		die(_("Not tracking: ambiguous information for ref %s"),
+		    full_orig_ref);
+
+	if (install_branch_config(config_flags, new_ref, tracking.remote,
+			      tracking.src ? tracking.src : full_orig_ref) < 0)
+		exit(-1);
+
+cleanup:
+	free(tracking.src);
+	free(full_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)
+{
+	struct object_id oid;
+	char *real_ref;
+	struct strbuf ref = STRBUF_INIT;
+	int forcing = 0;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
+	char *msg;
+
+	if (clobber_head_ok && !force)
+		BUG("'clobber_head_ok' can only be used with 'force'");
+
+	if (clobber_head_ok ?
+			  validate_branchname(name, &ref) :
+			  validate_new_branchname(name, &ref, force)) {
+		forcing = 1;
+	}
+
+	validate_branch_start(r, start_name, track, &oid, &real_ref);
 
 	if (reflog)
 		log_all_ref_updates = LOG_REFS_NORMAL;
 
-	if (!dont_change_ref) {
-		struct ref_transaction *transaction;
-		struct strbuf err = STRBUF_INIT;
-		char *msg;
-
-		if (forcing)
-			msg = xstrfmt("branch: Reset to %s", start_name);
-		else
-			msg = xstrfmt("branch: Created from %s", start_name);
-
-		transaction = ref_transaction_begin(&err);
-		if (!transaction ||
-		    ref_transaction_update(transaction, ref.buf,
-					   &oid, forcing ? NULL : null_oid(),
-					   0, msg, &err) ||
-		    ref_transaction_commit(transaction, &err))
-			die("%s", err.buf);
-		ref_transaction_free(transaction);
-		strbuf_release(&err);
-		free(msg);
-	}
+	if (forcing)
+		msg = xstrfmt("branch: Reset to %s", start_name);
+	else
+		msg = xstrfmt("branch: Created from %s", start_name);
+
+	transaction = ref_transaction_begin(&err);
+	if (!transaction ||
+		ref_transaction_update(transaction, ref.buf,
+					&oid, forcing ? NULL : null_oid(),
+					0, msg, &err) ||
+		ref_transaction_commit(transaction, &err))
+		die("%s", err.buf);
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
+	free(msg);
 
 	if (real_ref && track)
-		setup_tracking(ref.buf + 11, real_ref, track, quiet);
+		setup_tracking(ref.buf + 11, real_ref, track, quiet, 0);
 
 	strbuf_release(&ref);
 	free(real_ref);
diff --git a/branch.h b/branch.h
index df0be61506..75cefcdcbd 100644
--- a/branch.h
+++ b/branch.h
@@ -17,6 +17,15 @@ extern enum branch_track git_branch_track;
 
 /* Functions for acting on the information about branches. */
 
+/*
+ * This sets the branch.<new_ref>.{remote,merge} config settings so that
+ * branch 'new_ref' tracks 'orig_ref'. This is called when branches are
+ * created, or when branch configs are updated (e.g. with
+ * git branch --set-upstream-to).
+ */
+void setup_tracking(const char *new_ref, const char *orig_ref,
+		    enum branch_track track, int quiet, int expand_orig);
+
 /*
  * Creates a new branch, where:
  *
@@ -29,8 +38,8 @@ extern enum branch_track git_branch_track;
  *
  *   - force enables overwriting an existing (non-head) branch
  *
- *   - clobber_head_ok allows the currently checked out (hence existing)
- *     branch to be overwritten; without 'force', it has no effect.
+ *   - clobber_head_ok, when enabled with 'force', allows the currently
+ *     checked out (head) branch to be overwritten
  *
  *   - reflog creates a reflog for the branch
  *
diff --git a/builtin/branch.c b/builtin/branch.c
index 0b7ed82654..eb5c117a6e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -820,12 +820,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!ref_exists(branch->refname))
 			die(_("branch '%s' does not exist"), branch->name);
 
-		/*
-		 * create_branch takes care of setting up the tracking
-		 * info and making sure new_upstream is correct
-		 */
-		create_branch(the_repository, branch->name, new_upstream,
-			      0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+		setup_tracking(branch->name, new_upstream, BRANCH_TRACK_OVERRIDE, quiet, 1);
 	} else if (unset_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 		struct strbuf buf = STRBUF_INIT;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e575ffb4ff..6bf95a1707 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -42,6 +42,23 @@ test_expect_success 'git branch abc should create a branch' '
 	git branch abc && test_path_is_file .git/refs/heads/abc
 '
 
+test_expect_success 'git branch abc should fail when abc exists' '
+	test_must_fail git branch abc
+'
+
+test_expect_success 'git branch --force abc should fail when abc is checked out' '
+	test_when_finished git switch main &&
+	git switch abc &&
+	test_must_fail git branch --force abc HEAD~1
+'
+
+test_expect_success 'git branch --force abc should succeed when abc exists' '
+	git rev-parse HEAD~1 >expect &&
+	git branch --force abc HEAD~1 &&
+	git rev-parse abc >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git branch a/b/c should create a branch' '
 	git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c
 '
-- 
2.33.GIT


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/4] branch: add --dry-run option to branch
  2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
  2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
  2021-11-22 22:32 ` [PATCH 2/4] branch: refactor out branch validation from create_branch() Glen Choo
@ 2021-11-22 22:32 ` Glen Choo
  2021-11-23 10:42   ` Ævar Arnfjörð Bjarmason
  2021-11-23 23:10   ` Jonathan Tan
  2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
  3 siblings, 2 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-22 22:32 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, Josh Steadmon, Emily Shaffer, Glen Choo

When running "git branch --recurse-submodules topic", it would be useful
to know whether or not 'topic' is a valid branch for all repositories.
Currently there is no way to test this without actually creating the
branch.

Add a --dry-run option to branch creation that can check whether or not
a branch name and start point would be valid for a repository without
creating a branch. Refactor cmd_branch() to make the chosen action more
obvious.

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>
---
The --dry-run option is motivated mainly by --recurse-submodules. To my
knowledge, there isn't a strong existing demand, but this might be
mildly useful to some users.

 Documentation/git-branch.txt |  8 ++++++-
 branch.c                     |  6 ++---
 branch.h                     | 22 ++++++++++++++++++
 builtin/branch.c             | 44 ++++++++++++++++++++++++++----------
 t/t3200-branch.sh            | 13 +++++++++++
 5 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 5449767121..8cdc33c097 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 	[--points-at <object>] [--format=<format>]
 	[(-r | --remotes) | (-a | --all)]
 	[--list] [<pattern>...]
-'git branch' [--track | --no-track] [-f] <branchname> [<start-point>]
+'git branch' [--track | --no-track] [-f] [--dry-run | -n] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
 'git branch' --unset-upstream [<branchname>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
@@ -205,6 +205,12 @@ This option is only applicable in non-verbose mode.
 --no-abbrev::
 	Display the full sha1s in the output listing rather than abbreviating them.
 
+-n::
+--dry-run::
+	Can only be used when creating a branch. If the branch creation
+	would fail, show the relevant error message. If the branch
+	creation would succeed, show nothing.
+
 -t::
 --track::
 	When creating a new branch, set up `branch.<name>.remote` and
diff --git a/branch.c b/branch.c
index f8b755513f..528cb2d639 100644
--- a/branch.c
+++ b/branch.c
@@ -206,9 +206,9 @@ N_("\n"
 "will track its remote counterpart, you may want to use\n"
 "\"git push -u\" to set the upstream config as you push.");
 
-static void validate_branch_start(struct repository *r, const char *start_name,
-				  enum branch_track track,
-				  struct object_id *oid, char **full_ref)
+void validate_branch_start(struct repository *r, const char *start_name,
+			   enum branch_track track, struct object_id *oid,
+			   char **full_ref)
 {
 	struct commit *commit;
 	int explicit_tracking = 0;
diff --git a/branch.h b/branch.h
index 75cefcdcbd..d8e5ff4e28 100644
--- a/branch.h
+++ b/branch.h
@@ -3,6 +3,7 @@
 
 struct repository;
 struct strbuf;
+struct object_id;
 
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
@@ -17,6 +18,27 @@ extern enum branch_track git_branch_track;
 
 /* Functions for acting on the information about branches. */
 
+/*
+ * Validates whether a ref is a valid starting point for a branch, where:
+ *
+ *   - r is the repository to validate the branch for
+ *
+ *   - start_name is the ref that we would like to test
+ *
+ *   - track is the tracking mode of the new branch. If tracking is
+ *     explicitly requested, start_name must be a branch (because
+ *     otherwise start_name cannot be tracked)
+ *
+ *   - oid is an out parameter containing the object_id of start_name
+ *
+ *   - full_ref is an out parameter containing the 'full' form of
+ *     start_name e.g. refs/heads/main instead of main
+ *
+ */
+void validate_branch_start(struct repository *r, const char *start_name,
+			   enum branch_track track, struct object_id *oid,
+			   char **full_ref);
+
 /*
  * This sets the branch.<new_ref>.{remote,merge} config settings so that
  * branch 'new_ref' tracks 'orig_ref'. This is called when branches are
diff --git a/builtin/branch.c b/builtin/branch.c
index eb5c117a6e..5d4b9c82b4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -27,7 +27,8 @@
 
 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>] [-f] [--dry-run | -n] <branch-name> [<start-point>]"),
 	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>"),
@@ -616,14 +617,14 @@ static int edit_branch_description(const char *branch_name)
 
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
-	int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
-	int show_current = 0;
-	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, create = 0,
+	    unset_upstream = 0, show_current = 0, edit_description = 0;
+	/* possible options */
+	int reflog = 0, quiet = 0, dry_run = 0, icase = 0;
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	struct ref_filter filter;
-	int icase = 0;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	struct ref_format format = REF_FORMAT_INIT;
 
@@ -670,6 +671,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			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_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
+		OPT__DRY_RUN(&dry_run, N_("show whether the branch would be created")),
 		OPT_END(),
 	};
 
@@ -705,10 +707,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	    filter.reachable_from || filter.unreachable_from || filter.points_at.nr)
 		list = 1;
 
-	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
-	    list + edit_description + unset_upstream > 1)
+	create = 1 - (!!delete + !!rename + !!copy + !!new_upstream +
+		      !!show_current + !!list + !!edit_description +
+		      !!unset_upstream);
+	if (create < 0)
 		usage_with_options(builtin_branch_usage, options);
 
+	if (dry_run && !create)
+		die(_("--dry-run can only be used when creating branches"));
+
 	if (filter.abbrev == -1)
 		filter.abbrev = DEFAULT_ABBREV;
 	filter.ignore_case = icase;
@@ -844,7 +851,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&buf, "branch.%s.merge", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_release(&buf);
-	} else if (argc > 0 && argc <= 2) {
+	} else if (create && argc > 0 && argc <= 2) {
+		const char *branch_name = argv[0];
+		const char *start_name = (argc == 2) ? argv[1] : head;
+
 		if (filter.kind != FILTER_REFS_BRANCHES)
 			die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
 				  "Did you mean to use: -a|-r --list <pattern>?"));
@@ -852,10 +862,20 @@ 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);
+		if (dry_run) {
+			struct strbuf buf = STRBUF_INIT;
+			char *unused_full_ref;
+			struct object_id unused_oid;
 
+			validate_new_branchname(branch_name, &buf, force);
+			validate_branch_start(the_repository, start_name, track,
+					      &unused_oid, &unused_full_ref);
+			strbuf_release(&buf);
+			FREE_AND_NULL(unused_full_ref);
+			return 0;
+		}
+		create_branch(the_repository, branch_name, start_name, force, 0,
+			      reflog, quiet, track);
 	} else
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6bf95a1707..653891736a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -59,6 +59,19 @@ test_expect_success 'git branch --force abc should succeed when abc exists' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git branch --dry-run abc should fail when abc exists' '
+	test_must_fail git branch --dry-run abc
+'
+
+test_expect_success 'git branch --dry-run --force abc should succeed when abc exists' '
+	git branch --dry-run --force abc
+'
+
+test_expect_success 'git branch --dry-run def should not create a branch' '
+	git branch --dry-run def &&
+	test_must_fail git rev-parse def
+'
+
 test_expect_success 'git branch a/b/c should create a branch' '
 	git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c
 '
-- 
2.33.GIT


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
                   ` (2 preceding siblings ...)
  2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
@ 2021-11-22 22:32 ` Glen Choo
  2021-11-23 10:45   ` Ævar Arnfjörð Bjarmason
                     ` (2 more replies)
  3 siblings, 3 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-22 22:32 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, Josh Steadmon, Emily Shaffer, Glen Choo

Add a --recurse-submodules option when creating branches so that `git
branch --recurse-submodules topic` will create the "topic" branch in the
superproject and all submodules. Guard this (and future submodule
branching) behavior behind a new configuration value
'submodule.propagateBranches'.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 Documentation/config/advice.txt    |   3 +
 Documentation/config/submodule.txt |   9 ++
 advice.c                           |   1 +
 advice.h                           |   1 +
 branch.c                           | 123 ++++++++++++++
 branch.h                           |   6 +
 builtin/branch.c                   |  28 +++-
 builtin/submodule--helper.c        |  33 ++++
 t/t3207-branch-submodule.sh        | 249 +++++++++++++++++++++++++++++
 9 files changed, 452 insertions(+), 1 deletion(-)
 create mode 100755 t/t3207-branch-submodule.sh

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 063eec2511..e52262dc69 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -116,6 +116,9 @@ advice.*::
 	submoduleAlternateErrorStrategyDie::
 		Advice shown when a submodule.alternateErrorStrategy option
 		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.
 	addIgnoredFile::
 		Advice shown if a user attempts to add an ignored file to
 		the index.
diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
index ee454f8126..c318b849aa 100644
--- a/Documentation/config/submodule.txt
+++ b/Documentation/config/submodule.txt
@@ -72,6 +72,15 @@ submodule.recurse::
 	For these commands a workaround is to temporarily change the
 	configuration value by using `git -c submodule.recurse=0`.
 
+submodule.propagateBranches::
+	[EXPERIMENTAL] A boolean that enables branching support with
+	submodules. This allows certain commands to accept
+	`--recurse-submodules` (`git branch --recurse-submodules` will
+	create branches recursively), and certain commands that already
+	accept `--recurse-submodules` will now consider branches (`git
+	switch --recurse-submodules` will switch to the correct branch
+	in all submodules).
+
 submodule.fetchJobs::
 	Specifies how many submodules are fetched/cloned at the same time.
 	A positive integer allows up to that number of submodules fetched
diff --git a/advice.c b/advice.c
index 1dfc91d176..e00d30254c 100644
--- a/advice.c
+++ b/advice.c
@@ -70,6 +70,7 @@ static struct {
 	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
 	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
+	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated", 1 },
 	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
 };
diff --git a/advice.h b/advice.h
index 601265fd10..a7521d6087 100644
--- a/advice.h
+++ b/advice.h
@@ -44,6 +44,7 @@ struct string_list;
 	ADVICE_STATUS_HINTS,
 	ADVICE_STATUS_U_OPTION,
 	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+	ADVICE_SUBMODULES_NOT_UPDATED,
 	ADVICE_UPDATE_SPARSE_PATH,
 	ADVICE_WAITING_FOR_EDITOR,
 	ADVICE_SKIPPED_CHERRY_PICKS,
diff --git a/branch.c b/branch.c
index 528cb2d639..404766d01d 100644
--- a/branch.c
+++ b/branch.c
@@ -8,6 +8,8 @@
 #include "sequencer.h"
 #include "commit.h"
 #include "worktree.h"
+#include "submodule-config.h"
+#include "run-command.h"
 
 struct tracking {
 	struct refspec_item spec;
@@ -345,6 +347,127 @@ void create_branch(struct repository *r, const char *name,
 	free(real_ref);
 }
 
+static int submodule_validate_branchname(struct repository *r, const char *name,
+					 const char *start_name, int force,
+					 int quiet, char **err_msg)
+{
+	int ret = 0;
+	struct child_process child = CHILD_PROCESS_INIT;
+	struct strbuf child_err = STRBUF_INIT;
+	child.git_cmd = 1;
+	child.err = -1;
+
+	prepare_other_repo_env(&child.env_array, r->gitdir);
+	strvec_pushl(&child.args, "branch", "--dry-run", NULL);
+	if (force)
+		strvec_push(&child.args, "--force");
+	if (quiet)
+		strvec_push(&child.args, "--quiet");
+	strvec_pushl(&child.args, name, start_name, NULL);
+
+	if ((ret = start_command(&child)))
+		return ret;
+	ret = finish_command(&child);
+	strbuf_read(&child_err, child.err, 0);
+	*err_msg = strbuf_detach(&child_err, NULL);
+	return ret;
+}
+
+static int submodule_create_branch(struct repository *r, const char *name,
+				   const char *start_oid,
+				   const char *start_name, int force,
+				   int reflog, int quiet,
+				   enum branch_track track, char **err_msg)
+{
+	int ret = 0;
+	struct child_process child = CHILD_PROCESS_INIT;
+	struct strbuf child_err = STRBUF_INIT;
+	child.git_cmd = 1;
+	child.err = -1;
+
+	prepare_other_repo_env(&child.env_array, r->gitdir);
+	strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);
+	if (force)
+		strvec_push(&child.args, "--force");
+	if (quiet)
+		strvec_push(&child.args, "--quiet");
+	if (reflog)
+		strvec_push(&child.args, "--create-reflog");
+	if (track == BRANCH_TRACK_ALWAYS || track == BRANCH_TRACK_EXPLICIT)
+		strvec_push(&child.args, "--track");
+
+	strvec_pushl(&child.args, name, start_oid, start_name, NULL);
+
+	if ((ret = start_command(&child)))
+		return ret;
+	ret = finish_command(&child);
+	strbuf_read(&child_err, child.err, 0);
+	*err_msg = strbuf_detach(&child_err, NULL);
+	return ret;
+}
+
+void create_submodule_branches(struct repository *r, const char *name,
+			       const char *start_name, int force, int reflog,
+			       int quiet, enum branch_track track)
+{
+	int i = 0;
+	char *branch_point = NULL;
+	struct repository *subrepos;
+	struct submodule *submodules;
+	struct object_id super_oid;
+	struct submodule_entry_list *submodule_entry_list;
+	char *err_msg = NULL;
+
+	validate_branch_start(r, start_name, track, &super_oid, &branch_point);
+
+	submodule_entry_list = submodules_of_tree(r, &super_oid);
+	CALLOC_ARRAY(subrepos, submodule_entry_list->entry_nr);
+	CALLOC_ARRAY(submodules, submodule_entry_list->entry_nr);
+
+	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
+		submodules[i] = *submodule_from_path(
+			r, &super_oid,
+			submodule_entry_list->name_entries[i].path);
+
+		if (repo_submodule_init(
+			    &subrepos[i], r,
+			    submodule_entry_list->name_entries[i].path,
+			    &super_oid)) {
+			die(_("submodule %s: unable to find submodule"),
+			    submodules[i].name);
+			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
+				advise(_("You may try initializing the submodules using 'git checkout %s && git submodule update'"),
+				       start_name);
+		}
+
+		if (submodule_validate_branchname(
+			    &subrepos[i], name,
+			    oid_to_hex(
+				    &submodule_entry_list->name_entries[i].oid),
+			    force, quiet, &err_msg))
+			die(_("submodule %s: could not create branch '%s'\n\t%s"),
+			    submodules[i].name, name, err_msg);
+	}
+
+	create_branch(the_repository, name, start_name, force, 0, reflog, quiet,
+		      track);
+
+	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
+		printf_ln(_("submodule %s: creating branch '%s'"),
+			  submodules[i].name, name);
+		if (submodule_create_branch(
+			    &subrepos[i], name,
+			    oid_to_hex(
+				    &submodule_entry_list->name_entries[i].oid),
+			    branch_point, force, reflog, quiet, track,
+			    &err_msg))
+			die(_("submodule %s: could not create branch '%s'\n\t%s"),
+			    submodules[i].name, name, err_msg);
+
+		repo_clear(&subrepos[i]);
+	}
+}
+
 void remove_merge_branch_state(struct repository *r)
 {
 	unlink(git_path_merge_head(r));
diff --git a/branch.h b/branch.h
index d8e5ff4e28..1b4a635a2f 100644
--- a/branch.h
+++ b/branch.h
@@ -76,6 +76,12 @@ void create_branch(struct repository *r,
 		   int force, int clobber_head_ok,
 		   int reflog, int quiet, enum branch_track track);
 
+/*
+ * Creates a new branch in repository and its submodules.
+ */
+void create_submodule_branches(struct repository *r, const char *name,
+			       const char *start_name, int force, int reflog,
+			       int quiet, enum branch_track track);
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
  * Return 1 if the named branch already exists; return 0 otherwise.
diff --git a/builtin/branch.c b/builtin/branch.c
index 5d4b9c82b4..6a16bdb1a3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -39,6 +39,8 @@ static const char * const builtin_branch_usage[] = {
 
 static const char *head;
 static struct object_id head_oid;
+static int recurse_submodules = 0;
+static int submodule_propagate_branches = 0;
 
 static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
@@ -101,6 +103,15 @@ 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;
+	}
+	if (!strcasecmp(var, "submodule.propagateBranches")) {
+		submodule_propagate_branches = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_color_default_config(var, value, cb);
 }
 
@@ -621,7 +632,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int delete = 0, rename = 0, copy = 0, force = 0, list = 0, create = 0,
 	    unset_upstream = 0, show_current = 0, edit_description = 0;
 	/* possible options */
-	int reflog = 0, quiet = 0, dry_run = 0, icase = 0;
+	int reflog = 0, quiet = 0, dry_run = 0, icase = 0,
+	    recurse_submodules_explicit = 0;
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	struct ref_filter filter;
@@ -670,6 +682,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__DRY_RUN(&dry_run, N_("show whether the branch would be created")),
 		OPT_END(),
@@ -713,9 +726,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (create < 0)
 		usage_with_options(builtin_branch_usage, options);
 
+	if (recurse_submodules_explicit && submodule_propagate_branches &&
+	    !create)
+		die(_("--recurse-submodules can only be used to create branches"));
 	if (dry_run && !create)
 		die(_("--dry-run can only be used when creating branches"));
 
+	recurse_submodules =
+		(recurse_submodules || recurse_submodules_explicit) &&
+		submodule_propagate_branches;
+
 	if (filter.abbrev == -1)
 		filter.abbrev = DEFAULT_ABBREV;
 	filter.ignore_case = icase;
@@ -874,6 +894,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			FREE_AND_NULL(unused_full_ref);
 			return 0;
 		}
+		if (recurse_submodules) {
+			create_submodule_branches(the_repository, branch_name,
+						  start_name, force, reflog,
+						  quiet, track);
+			return 0;
+		}
 		create_branch(the_repository, branch_name, start_name, force, 0,
 			      reflog, quiet, track);
 	} else
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6298cbdd4e..3ea1e8cc96 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -20,6 +20,7 @@
 #include "diff.h"
 #include "object-store.h"
 #include "advice.h"
+#include "branch.h"
 
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
@@ -2983,6 +2984,37 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+static int module_create_branch(int argc, const char **argv, const char *prefix)
+{
+	enum branch_track track;
+	int quiet = 0, force = 0, reflog = 0;
+
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("print only error messages")),
+		OPT__FORCE(&force, N_("force creation"), 0),
+		OPT_BOOL(0, "create-reflog", &reflog,
+			 N_("create the branch's reflog")),
+		OPT_SET_INT('t', "track", &track,
+			    N_("set up tracking mode (see git-pull(1))"),
+			    BRANCH_TRACK_EXPLICIT),
+		OPT_END()
+	};
+	const char *const usage[] = {
+		N_("git submodule--helper create-branch [-f|--force] [--create-reflog] [-q|--quiet] [-t|--track] <name> <start_oid> <start_name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 3)
+		usage_with_options(usage, options);
+
+	create_branch(the_repository, argv[0], argv[1], force, 0, reflog, quiet,
+		      BRANCH_TRACK_NEVER);
+	setup_tracking(argv[0], argv[2], track, quiet, 0);
+
+	return 0;
+}
 struct add_data {
 	const char *prefix;
 	const char *branch;
@@ -3379,6 +3411,7 @@ static struct cmd_struct commands[] = {
 	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
 	{"set-branch", module_set_branch, 0},
+	{"create-branch", module_create_branch, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh
new file mode 100755
index 0000000000..14ff066e91
--- /dev/null
+++ b/t/t3207-branch-submodule.sh
@@ -0,0 +1,249 @@
+#!/bin/sh
+
+test_description='git branch submodule tests'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup superproject and submodule' '
+	git init super &&
+	test_commit foo &&
+	git init sub-upstream &&
+	test_commit -C sub-upstream foo &&
+	git -C super submodule add ../sub-upstream sub &&
+	git -C super commit -m "add submodule" &&
+	git -C super config submodule.propagateBranches true
+'
+
+cleanup_branches() {
+	super_dir="$1"
+	shift
+	(
+		cd "$super_dir" &&
+		git checkout main &&
+		for branch_name in "$@"; do
+			git branch -D "$branch_name"
+			git submodule foreach "(git checkout main && git branch -D $branch_name) || true"
+		done
+	)
+} >/dev/null 2>/dev/null
+
+# Test the argument parsing
+test_expect_success '--recurse-submodules should create branches' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		cd super &&
+		git branch --recurse-submodules branch-a &&
+		git rev-parse --abbrev-ref branch-a &&
+		git -C sub rev-parse --abbrev-ref branch-a
+	)
+'
+
+test_expect_success '--recurse-submodules should be ignored if submodule.propagateBranches is false' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		cd super &&
+		git -c submodule.propagateBranches=false branch --recurse-submodules branch-a &&
+		git rev-parse branch-a &&
+		test_must_fail git -C sub rev-parse branch-a
+	)
+'
+
+test_expect_success '--recurse-submodules should fail when not creating branches' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		cd super &&
+		git branch --recurse-submodules branch-a &&
+		test_must_fail git branch --recurse-submodules -D branch-a &&
+		# Assert that the branches were not deleted
+		git rev-parse --abbrev-ref branch-a &&
+		git -C sub rev-parse --abbrev-ref branch-a
+	)
+'
+
+test_expect_success 'should respect submodule.recurse when creating branches' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		cd super &&
+		git -c submodule.recurse=true branch branch-a &&
+		git rev-parse --abbrev-ref branch-a &&
+		git -C sub rev-parse --abbrev-ref branch-a
+	)
+'
+
+test_expect_success 'should ignore submodule.recurse when not creating branches' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		cd super &&
+		git branch --recurse-submodules branch-a &&
+		git -c submodule.recurse=true branch -D branch-a &&
+		test_must_fail git rev-parse --abbrev-ref branch-a &&
+		git -C sub rev-parse --abbrev-ref branch-a
+	)
+'
+
+# Test branch creation behavior
+test_expect_success 'should create branches based off commit id in superproject' '
+	test_when_finished "cleanup_branches super branch-a branch-b" &&
+	(
+		cd super &&
+		git branch --recurse-submodules branch-a &&
+		git checkout --recurse-submodules branch-a &&
+		git -C sub rev-parse HEAD >expected &&
+		# Move the tip of sub:branch-a so that it no longer matches the commit in super:branch-a
+		git -C sub checkout branch-a &&
+		test_commit -C sub bar &&
+		# Create a new branch-b branch with start-point=branch-a
+		git branch --recurse-submodules branch-b branch-a &&
+		git rev-parse branch-b &&
+		git -C sub rev-parse branch-b >actual &&
+		# Assert that the commit id of sub:second-branch matches super:branch-a and not sub:branch-a
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'should not create any branches if branch is not valid for all repos' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		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 &&
+
+		cat >expected <<EOF &&
+fatal: submodule sub: could not create branch ${SQ}branch-a${SQ}
+	fatal: A branch named ${SQ}branch-a${SQ} already exists.
+
+EOF
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'should create branches if branch exists and --force is given' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		cd super &&
+		git -C sub rev-parse HEAD >expected &&
+		test_commit -C sub baz &&
+		git -C sub branch branch-a HEAD~1 &&
+		git branch --recurse-submodules --force branch-a &&
+		git rev-parse branch-a &&
+		# assert that sub:branch-a was moved
+		git -C sub rev-parse branch-a >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
+	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
+	(
+		cd super &&
+		git branch branch-a &&
+		git checkout -b branch-b &&
+		git submodule add ../sub-upstream sub2 &&
+		# branch-b now has a committed submodule not in branch-a
+		git commit -m "add second submodule" &&
+		git checkout branch-a &&
+		git branch --recurse-submodules branch-c branch-b &&
+		git rev-parse branch-c &&
+		git -C sub rev-parse branch-c &&
+		git checkout --recurse-submodules branch-c &&
+		git -C sub2 rev-parse branch-c
+	)
+'
+
+test_expect_success 'should set up tracking of local branches with track=always' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		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_expect_success 'should set up tracking of local branches with explicit track' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		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_expect_success 'should not set up unnecessary tracking of local branches' '
+	test_when_finished "cleanup_branches super branch-a" &&
+	(
+		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_expect_success 'setup remote-tracking tests' '
+	(
+		cd super &&
+		git branch branch-a &&
+		git checkout -b branch-b &&
+		git submodule add ../sub-upstream sub2 &&
+		# branch-b now has a committed submodule not in branch-a
+		git commit -m "add second submodule"
+	) &&
+	(
+		cd sub-upstream &&
+		git branch branch-a
+	) &&
+	git clone --branch main --recurse-submodules super super-clone &&
+	git -C super-clone config submodule.propagateBranches true
+'
+
+test_expect_success 'should not create branch when submodule is not in .git/modules' '
+	# The cleanup needs to delete sub2:branch-b in particular because main does not have sub2
+	test_when_finished "git -C super-clone/sub2 branch -D branch-b && \
+		cleanup_branches super-clone branch-a branch-b" &&
+	(
+		cd super-clone &&
+		# This should succeed because super-clone has sub.
+		git branch --recurse-submodules branch-a origin/branch-a &&
+		# This should fail because super-clone does not have sub2.
+		test_must_fail git branch --recurse-submodules branch-b origin/branch-b 2>actual &&
+		cat >expected <<-EOF &&
+		fatal: submodule sub: unable to find submodule
+		You may reinitialize the submodules using ${SQ}git checkout origin/branch-b && git submodule update${SQ}
+		EOF
+		test_must_fail git rev-parse branch-b &&
+		test_must_fail git -C sub rev-parse branch-b &&
+		# User can fix themselves by initializing the submodule
+		git checkout origin/branch-b &&
+		git submodule update &&
+		git branch --recurse-submodules branch-b origin/branch-b
+	)
+'
+
+test_expect_success 'should set up tracking of remote-tracking branches' '
+	test_when_finished "cleanup_branches super-clone branch-a" &&
+	(
+		cd super-clone &&
+		git branch --recurse-submodules branch-a origin/branch-a &&
+		test "$(git -C sub config branch.branch-a.remote)" = origin &&
+		test "$(git -C sub config branch.branch-a.merge)" = refs/heads/branch-a
+	)
+'
+
+test_expect_success 'should not fail when unable to set up tracking in submodule' '
+	test_when_finished "cleanup_branches super-clone branch-b" &&
+	(
+		cd super-clone &&
+		git branch --recurse-submodules branch-b origin/branch-b
+	)
+'
+
+test_done
-- 
2.33.GIT


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] submodule-config: add submodules_of_tree() helper
  2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
@ 2021-11-23  2:12   ` Jonathan Tan
  2021-11-23 19:48     ` Glen Choo
  2021-11-23 10:53   ` Ævar Arnfjörð Bjarmason
  2021-11-23 22:46   ` Junio C Hamano
  2 siblings, 1 reply; 21+ messages in thread
From: Jonathan Tan @ 2021-11-23  2:12 UTC (permalink / raw)
  To: chooglen; +Cc: git, jonathantanmy, steadmon, emilyshaffer

Glen Choo <chooglen@google.com> writes:
> +struct submodule_entry_list *
> +submodules_of_tree(struct repository *r, const struct object_id *treeish_name)
> +{
> +	struct tree_desc tree;
> +	struct name_entry entry;
> +	struct submodule_entry_list *ret;
> +
> +	CALLOC_ARRAY(ret, 1);
> +	fill_tree_descriptor(r, &tree, treeish_name);
> +	while (tree_entry(&tree, &entry)) {

I think that tree_entry() doesn't recurse into subtrees, but in any case we
should test this. (I looked at patch 4 and I think that the submodules are
always in the root tree.)

This reminded me of a similar thing when fetching submodules recursively and we
needed the "before" and "after" of submodule gitlinks. You can look at the code
(collect_changed_submodules_cb() and the functions that use it in submodule.c)
but it may not be useful - in particular, that uses diff since we need to see
differences there, but we don't need that here.

I'll review the other patches tomorrow.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] branch: add --dry-run option to branch
  2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
@ 2021-11-23 10:42   ` Ævar Arnfjörð Bjarmason
  2021-11-23 18:42     ` Glen Choo
  2021-11-23 23:10   ` Jonathan Tan
  1 sibling, 1 reply; 21+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 10:42 UTC (permalink / raw)
  To: Glen Choo; +Cc: git, Jonathan Tan, Josh Steadmon, Emily Shaffer


On Mon, Nov 22 2021, Glen Choo wrote:

> Add a --dry-run option to branch creation that can check whether or not
> a branch name and start point would be valid for a repository without
> creating a branch. Refactor cmd_branch() to make the chosen action more
> obvious.
> [...]
> -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>]
> +'git branch' [--track | --no-track] [-f] [--dry-run | -n] <branchname> [<start-point>]
>  'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
>  'git branch' --unset-upstream [<branchname>]
>  'git branch' (-m | -M) [<oldbranch>] <newbranch>
> @@ -205,6 +205,12 @@ This option is only applicable in non-verbose mode.
>  --no-abbrev::
>  	Display the full sha1s in the output listing rather than abbreviating them.
>  
> +-n::
> +--dry-run::
> +	Can only be used when creating a branch. If the branch creation
> +	would fail, show the relevant error message. If the branch
> +	creation would succeed, show nothing.
> +

The usage & test show that we've got --dry-run for branch creation, but
not the "creation" we do on --copy or --move.

The former is just a "create from source", but "move" maybe not.

In any case, any reason to leave those out?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
@ 2021-11-23 10:45   ` Ævar Arnfjörð Bjarmason
  2021-11-23 18:56     ` Glen Choo
  2021-11-23 19:41   ` Philippe Blain
  2021-11-24  1:31   ` Jonathan Tan
  2 siblings, 1 reply; 21+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 10:45 UTC (permalink / raw)
  To: Glen Choo; +Cc: git, Jonathan Tan, Josh Steadmon, Emily Shaffer


On Mon, Nov 22 2021, Glen Choo wrote:

>  	submoduleAlternateErrorStrategyDie::
>  		Advice shown when a submodule.alternateErrorStrategy option
>  		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.
>  	addIgnoredFile::
>  		Advice shown if a user attempts to add an ignored file to
>  		the index.

Does it need to be submodule*s*NotUpdated? I.e. the existing error is
submodule.. (non-plural), and we surely error on this per-submodule? The
plural would make senes if the advice aggregates them to the end, let's
look at the implementation...

> [...]
> +	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
> +		submodules[i] = *submodule_from_path(
> +			r, &super_oid,
> +			submodule_entry_list->name_entries[i].path);
> +
> +		if (repo_submodule_init(
> +			    &subrepos[i], r,
> +			    submodule_entry_list->name_entries[i].path,
> +			    &super_oid)) {
> +			die(_("submodule %s: unable to find submodule"),
> +			    submodules[i].name);
> +			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
> +				advise(_("You may try initializing the submodules using 'git checkout %s && git submodule update'"),
> +				       start_name);
> +		}

Uh, a call to advise() after die()? :) That code isn't reachable.

It would be good to add test for what the output is in the next
iteration, which would be a forcing function for making sure this code
works.

One thing I find quite annoying about submodules currently is the
verbosity of the output when we do N operations. E.g. I've got a repo
with 15-20 small submodules, cloning it prints out the usual "git clone"
verbosity, which isn't so much when cloning one repo, but with 15-20 it
fills up your screen.

Operations like these should also behave more like "git fetch --all",
surely? I.e. let's try to run the operation on all, but if some failed
along the way let's have our exit code reflect that, and ideally print
out an error summary, not N number of error() calls.

That would also justify the plural "submodulesNotUpdated", if such an
advise() printed out a summary of the N that failed at the end.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] submodule-config: add submodules_of_tree() helper
  2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
  2021-11-23  2:12   ` Jonathan Tan
@ 2021-11-23 10:53   ` Ævar Arnfjörð Bjarmason
  2021-11-23 18:35     ` Glen Choo
  2021-11-23 22:46   ` Junio C Hamano
  2 siblings, 1 reply; 21+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 10:53 UTC (permalink / raw)
  To: Glen Choo; +Cc: git, Jonathan Tan, Josh Steadmon, Emily Shaffer


On Mon, Nov 22 2021, Glen Choo wrote:

> As we introduce a submodule UX with branches, we would like to be able
> to get the submodule commit ids in a superproject tree because those ids
> are the source of truth e.g. "git branch --recurse-submodules topic
> start-point" should create branches based off the commit ids recorded in
> the superproject's 'start-point' tree.
>
> To make this easy, introduce a submodules_of_tree() helper function that
> iterates through a tree and returns the tree's gitlink entries as a
> list.
>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  submodule-config.c | 19 +++++++++++++++++++
>  submodule-config.h | 13 +++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index f95344028b..97da373301 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -7,6 +7,7 @@
>  #include "strbuf.h"
>  #include "object-store.h"
>  #include "parse-options.h"
> +#include "tree-walk.h"
>  
>  /*
>   * submodule cache lookup structure
> @@ -726,6 +727,24 @@ const struct submodule *submodule_from_path(struct repository *r,
>  	return config_from(r->submodule_cache, treeish_name, path, lookup_path);
>  }
>  
> +struct submodule_entry_list *
> +submodules_of_tree(struct repository *r, const struct object_id *treeish_name)
> +{
> +	struct tree_desc tree;
> +	struct name_entry entry;
> +	struct submodule_entry_list *ret;
> +
> +	CALLOC_ARRAY(ret, 1);
> +	fill_tree_descriptor(r, &tree, treeish_name);
> +	while (tree_entry(&tree, &entry)) {
> +		if (!S_ISGITLINK(entry.mode))
> +			continue;
> +		ALLOC_GROW(ret->name_entries, ret->entry_nr + 1, ret->entry_alloc);
> +		ret->name_entries[ret->entry_nr++] = entry;
> +	}
> +	return ret;
> +}
> +
>  void submodule_free(struct repository *r)
>  {
>  	if (r->submodule_cache)
> diff --git a/submodule-config.h b/submodule-config.h
> index 65875b94ea..4379ec77e3 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -6,6 +6,7 @@
>  #include "hashmap.h"
>  #include "submodule.h"
>  #include "strbuf.h"
> +#include "tree-walk.h"
>  
>  /**
>   * The submodule config cache API allows to read submodule
> @@ -67,6 +68,18 @@ const struct submodule *submodule_from_name(struct repository *r,
>  					    const struct object_id *commit_or_tree,
>  					    const char *name);
>  
> +struct submodule_entry_list {
> +	struct name_entry *name_entries;
> +	int entry_nr;
> +	int entry_alloc;
> +};
> +
> +/**
> + * Given a tree-ish, return all submodules in the tree.
> + */
> +struct submodule_entry_list *
> +submodules_of_tree(struct repository *r, const struct object_id *treeish_name);
> +
>  /**
>   * Given a tree-ish in the superproject and a path, return the submodule that
>   * is bound at the path in the named tree.

Having skimmed through this topic isn't this in 4/4 the only resulting caller:
	
	+void create_submodule_branches(struct repository *r, const char *name,
	+			       const char *start_name, int force, int reflog,
	+			       int quiet, enum branch_track track)
	+{
	+	int i = 0;
	+	char *branch_point = NULL;
	+	struct repository *subrepos;
	+	struct submodule *submodules;
	+	struct object_id super_oid;
	+	struct submodule_entry_list *submodule_entry_list;
	+	char *err_msg = NULL;
	+
	+	validate_branch_start(r, start_name, track, &super_oid, &branch_point);
	+
	+	submodule_entry_list = submodules_of_tree(r, &super_oid);
	+	CALLOC_ARRAY(subrepos, submodule_entry_list->entry_nr);
	+	CALLOC_ARRAY(submodules, submodule_entry_list->entry_nr);
	+
	+	for (i = 0; i < submodule_entry_list->entry_nr; i++) {

I think it would be better to just intorduce this function at the same
time as its (only?) user, which also makes it clear how it's used.

In this case this seems like quite a bit of over-allocation. I.e. we
return a malloc'd pointer, and iterate with tree_entry(), the caller
then needs to loop over that and do its own allocations of "struct
repository *" and "struct submodule *".

Wouldn't it be better just to have this new submodule_entry_list contain
a list of not "struct name_entry", but:

    struct new_thingy {
        struct name_entry *entry;
        struct repository *repo;
        struct submodule *submodule;
    }

Then have the caller allocate the container on the stack, pass it to
this function.

Maybe not, just musings while doing some light reading. I was surprised
at what are effectively two loops over the same data, first allocating
1/3, then the other doing the other 2/3...

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] submodule-config: add submodules_of_tree() helper
  2021-11-23 10:53   ` Ævar Arnfjörð Bjarmason
@ 2021-11-23 18:35     ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-23 18:35 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jonathan Tan, Josh Steadmon, Emily Shaffer

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Having skimmed through this topic isn't this in 4/4 the only resulting caller:
> 	
> 	+void create_submodule_branches(struct repository *r, const char *name,
> 	+			       const char *start_name, int force, int reflog,
> 	+			       int quiet, enum branch_track track)
> 	+{
> 	+	int i = 0;
> 	+	char *branch_point = NULL;
> 	+	struct repository *subrepos;
> 	+	struct submodule *submodules;
> 	+	struct object_id super_oid;
> 	+	struct submodule_entry_list *submodule_entry_list;
> 	+	char *err_msg = NULL;
> 	+
> 	+	validate_branch_start(r, start_name, track, &super_oid, &branch_point);
> 	+
> 	+	submodule_entry_list = submodules_of_tree(r, &super_oid);
> 	+	CALLOC_ARRAY(subrepos, submodule_entry_list->entry_nr);
> 	+	CALLOC_ARRAY(submodules, submodule_entry_list->entry_nr);
> 	+
> 	+	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
>
> I think it would be better to just intorduce this function at the same
> time as its (only?) user, which also makes it clear how it's used.

Yes that makes sense. That is the only user (for now). 

> In this case this seems like quite a bit of over-allocation. I.e. we
> return a malloc'd pointer, and iterate with tree_entry(), the caller
> then needs to loop over that and do its own allocations of "struct
> repository *" and "struct submodule *".
>
> Wouldn't it be better just to have this new submodule_entry_list contain
> a list of not "struct name_entry", but:
>
>     struct new_thingy {
>         struct name_entry *entry;
>         struct repository *repo;
>         struct submodule *submodule;
>     }
>
> Then have the caller allocate the container on the stack, pass it to
> this function.

I thought about it as well. "struct new_thingy" is obviously the right
struct for create_submodule_branches(), but I'm not sure if it is the
right thing for other future callers e.g. "struct submodule" is only
used to give a submodule name to users in help messages.

But chances are, any caller that needs 'submodules of a tree' will need
very similar pieces of information, so it seems reasonable to do what
you said instead of over-allocating in all of the callers.

> Maybe not, just musings while doing some light reading. I was surprised
> at what are effectively two loops over the same data, first allocating
> 1/3, then the other doing the other 2/3...

The first loop validates all submodules before creating any branches
(and also happens to allocate). If we didn't have the validation step,
allocation + creating branches could just be one loop :)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] branch: add --dry-run option to branch
  2021-11-23 10:42   ` Ævar Arnfjörð Bjarmason
@ 2021-11-23 18:42     ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-23 18:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jonathan Tan, Josh Steadmon, Emily Shaffer

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Nov 22 2021, Glen Choo wrote:
>
>> Add a --dry-run option to branch creation that can check whether or not
>> a branch name and start point would be valid for a repository without
>> creating a branch. Refactor cmd_branch() to make the chosen action more
>> obvious.
>> [...]
>> -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>]
>> +'git branch' [--track | --no-track] [-f] [--dry-run | -n] <branchname> [<start-point>]
>>  'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
>>  'git branch' --unset-upstream [<branchname>]
>>  'git branch' (-m | -M) [<oldbranch>] <newbranch>
>> @@ -205,6 +205,12 @@ This option is only applicable in non-verbose mode.
>>  --no-abbrev::
>>  	Display the full sha1s in the output listing rather than abbreviating them.
>>  
>> +-n::
>> +--dry-run::
>> +	Can only be used when creating a branch. If the branch creation
>> +	would fail, show the relevant error message. If the branch
>> +	creation would succeed, show nothing.
>> +
>
> The usage & test show that we've got --dry-run for branch creation, but
> not the "creation" we do on --copy or --move.

Perhaps this is more of a wording issue i.e. 'creating a branch' is too
unspecific. Maybe 

	Can only be used when creating a new branch (without copying or moving
	an existing branch). If the branch creation would fail, show the
	relevant error message. If the branch creation would succeed, show
	nothing.

> In any case, any reason to leave those out?

No long term reason. I left those out because "create brand new branch
with --dry-run" was needed right now, but the others are not. For
consistency, we'd want --dry-run for all other actions, including copy,
move, delete, etc.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-23 10:45   ` Ævar Arnfjörð Bjarmason
@ 2021-11-23 18:56     ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-23 18:56 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jonathan Tan, Josh Steadmon, Emily Shaffer

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Nov 22 2021, Glen Choo wrote:
>
>>  	submoduleAlternateErrorStrategyDie::
>>  		Advice shown when a submodule.alternateErrorStrategy option
>>  		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.
>>  	addIgnoredFile::
>>  		Advice shown if a user attempts to add an ignored file to
>>  		the index.
>
> Does it need to be submodule*s*NotUpdated? I.e. the existing error is
> submodule.. (non-plural), and we surely error on this per-submodule?

From the user's perspective, failing on a per-submodule basis looks like
an implementation detail. As a user, I am looking for advice on errors
that could be avoided by running "git submodule update"; I'd want the
advice option to describe what "git submodule update" does, which is
update submodule*s*.

>> [...]
>> +	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
>> +		submodules[i] = *submodule_from_path(
>> +			r, &super_oid,
>> +			submodule_entry_list->name_entries[i].path);
>> +
>> +		if (repo_submodule_init(
>> +			    &subrepos[i], r,
>> +			    submodule_entry_list->name_entries[i].path,
>> +			    &super_oid)) {
>> +			die(_("submodule %s: unable to find submodule"),
>> +			    submodules[i].name);
>> +			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
>> +				advise(_("You may try initializing the submodules using 'git checkout %s && git submodule update'"),
>> +				       start_name);
>> +		}
>
> Uh, a call to advise() after die()? :) That code isn't reachable.
>
> It would be good to add test for what the output is in the next
> iteration, which would be a forcing function for making sure this code
> works.

Whoops. Good catch. Yes I should test that.

> One thing I find quite annoying about submodules currently is the
> verbosity of the output when we do N operations. E.g. I've got a repo
> with 15-20 small submodules, cloning it prints out the usual "git clone"
> verbosity, which isn't so much when cloning one repo, but with 15-20 it
> fills up your screen.
>
> Operations like these should also behave more like "git fetch --all",
> surely? I.e. let's try to run the operation on all, but if some failed
> along the way let's have our exit code reflect that, and ideally print
> out an error summary, not N number of error() calls.

That's a valid criticism, and one we're concerned about as well. I'm
not 100% satisfied with how I've structured the output either, but as a
practical matter, figuring out an *ideal* output format and
standardizing it takes up too much valuable iteration time that could be
spent on improving the UX instead.

So the approach here is to have output that is 'good enough' for now,
and to create better and standardized output when we've nailed down more
of the UX.

> That would also justify the plural "submodulesNotUpdated", if such an
> advise() printed out a summary of the N that failed at the end.

Fair. We could treat an uninitialized submodule the same as any other
failure and summarize all failures.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
  2021-11-23 10:45   ` Ævar Arnfjörð Bjarmason
@ 2021-11-23 19:41   ` Philippe Blain
  2021-11-23 23:43     ` Glen Choo
  2021-11-24  1:31   ` Jonathan Tan
  2 siblings, 1 reply; 21+ messages in thread
From: Philippe Blain @ 2021-11-23 19:41 UTC (permalink / raw)
  To: Glen Choo, git
  Cc: Jonathan Tan, Josh Steadmon, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Hi Glen,

Le 2021-11-22 à 17:32, Glen Choo a écrit :
> Add a --recurse-submodules option when creating branches so that `git
> branch --recurse-submodules topic` will create the "topic" branch in the
> superproject and all submodules. Guard this (and future submodule
> branching) behavior behind a new configuration value
> 'submodule.propagateBranches'.
> 
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>   Documentation/config/advice.txt    |   3 +
>   Documentation/config/submodule.txt |   9 ++

We would need to add the new flag to Documentation/git-branch.txt,
and also probably update the documentation of 'submodule.recurse'
in 'Documentation/config/submodule.txt'.

>   advice.c                           |   1 +
>   advice.h                           |   1 +
>   branch.c                           | 123 ++++++++++++++
>   branch.h                           |   6 +
>   builtin/branch.c                   |  28 +++-
>   builtin/submodule--helper.c        |  33 ++++
>   t/t3207-branch-submodule.sh        | 249 +++++++++++++++++++++++++++++
>   9 files changed, 452 insertions(+), 1 deletion(-)
>   create mode 100755 t/t3207-branch-submodule.sh
> 
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index 063eec2511..e52262dc69 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -116,6 +116,9 @@ advice.*::
>   	submoduleAlternateErrorStrategyDie::
>   		Advice shown when a submodule.alternateErrorStrategy option
>   		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.
>   	addIgnoredFile::
>   		Advice shown if a user attempts to add an ignored file to
>   		the index.
> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> index ee454f8126..c318b849aa 100644
> --- a/Documentation/config/submodule.txt
> +++ b/Documentation/config/submodule.txt
> @@ -72,6 +72,15 @@ submodule.recurse::
>   	For these commands a workaround is to temporarily change the
>   	configuration value by using `git -c submodule.recurse=0`.
>   
> +submodule.propagateBranches::
> +	[EXPERIMENTAL] A boolean that enables branching support with
> +	submodules. This allows certain commands to accept
> +	`--recurse-submodules` (`git branch --recurse-submodules` will
> +	create branches recursively), and certain commands that already
> +	accept `--recurse-submodules` will now consider branches (`git
> +	switch --recurse-submodules` will switch to the correct branch
> +	in all submodules).

Looking at the rest of the patch, this just implements 'branch --recurse-submodules', right ?
i.e. 'git switch' and 'git checkout' are left alone for
now, so I think this addition to the doc should only mention 'git branch'.

> +
>   submodule.fetchJobs::
>   	Specifies how many submodules are fetched/cloned at the same time.
>   	A positive integer allows up to that number of submodules fetched
> diff --git a/advice.c b/advice.c
> index 1dfc91d176..e00d30254c 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -70,6 +70,7 @@ static struct {
>   	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
>   	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
>   	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
> +	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated", 1 },
>   	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
>   	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
>   };
> diff --git a/advice.h b/advice.h
> index 601265fd10..a7521d6087 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -44,6 +44,7 @@ struct string_list;
>   	ADVICE_STATUS_HINTS,
>   	ADVICE_STATUS_U_OPTION,
>   	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
> +	ADVICE_SUBMODULES_NOT_UPDATED,
>   	ADVICE_UPDATE_SPARSE_PATH,
>   	ADVICE_WAITING_FOR_EDITOR,
>   	ADVICE_SKIPPED_CHERRY_PICKS,
> diff --git a/branch.c b/branch.c
> index 528cb2d639..404766d01d 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -8,6 +8,8 @@
>   #include "sequencer.h"
>   #include "commit.h"
>   #include "worktree.h"
> +#include "submodule-config.h"
> +#include "run-command.h"
>   
>   struct tracking {
>   	struct refspec_item spec;
> @@ -345,6 +347,127 @@ void create_branch(struct repository *r, const char *name,
>   	free(real_ref);
>   }
>   
> +static int submodule_validate_branchname(struct repository *r, const char *name,
> +					 const char *start_name, int force,
> +					 int quiet, char **err_msg)
> +{
> +	int ret = 0;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	struct strbuf child_err = STRBUF_INIT;
> +	child.git_cmd = 1;
> +	child.err = -1;
> +
> +	prepare_other_repo_env(&child.env_array, r->gitdir);
> +	strvec_pushl(&child.args, "branch", "--dry-run", NULL);
> +	if (force)
> +		strvec_push(&child.args, "--force");
> +	if (quiet)
> +		strvec_push(&child.args, "--quiet");
> +	strvec_pushl(&child.args, name, start_name, NULL);
> +
> +	if ((ret = start_command(&child)))
> +		return ret;
> +	ret = finish_command(&child);
> +	strbuf_read(&child_err, child.err, 0);
> +	*err_msg = strbuf_detach(&child_err, NULL);
> +	return ret;
> +}
> +
> +static int submodule_create_branch(struct repository *r, const char *name,
> +				   const char *start_oid,
> +				   const char *start_name, int force,
> +				   int reflog, int quiet,
> +				   enum branch_track track, char **err_msg)
> +{
> +	int ret = 0;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	struct strbuf child_err = STRBUF_INIT;
> +	child.git_cmd = 1;
> +	child.err = -1;
> +
> +	prepare_other_repo_env(&child.env_array, r->gitdir);
> +	strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);
> +	if (force)
> +		strvec_push(&child.args, "--force");
> +	if (quiet)
> +		strvec_push(&child.args, "--quiet");
> +	if (reflog)
> +		strvec_push(&child.args, "--create-reflog");
> +	if (track == BRANCH_TRACK_ALWAYS || track == BRANCH_TRACK_EXPLICIT)
> +		strvec_push(&child.args, "--track");
> +
> +	strvec_pushl(&child.args, name, start_oid, start_name, NULL);
> +
> +	if ((ret = start_command(&child)))
> +		return ret;
> +	ret = finish_command(&child);
> +	strbuf_read(&child_err, child.err, 0);
> +	*err_msg = strbuf_detach(&child_err, NULL);
> +	return ret;
> +}
> +
> +void create_submodule_branches(struct repository *r, const char *name,
> +			       const char *start_name, int force, int reflog,
> +			       int quiet, enum branch_track track)
> +{
> +	int i = 0;
> +	char *branch_point = NULL;
> +	struct repository *subrepos;
> +	struct submodule *submodules;
> +	struct object_id super_oid;
> +	struct submodule_entry_list *submodule_entry_list;
> +	char *err_msg = NULL;
> +
> +	validate_branch_start(r, start_name, track, &super_oid, &branch_point);
> +
> +	submodule_entry_list = submodules_of_tree(r, &super_oid);
> +	CALLOC_ARRAY(subrepos, submodule_entry_list->entry_nr);
> +	CALLOC_ARRAY(submodules, submodule_entry_list->entry_nr);
> +
> +	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
> +		submodules[i] = *submodule_from_path(
> +			r, &super_oid,
> +			submodule_entry_list->name_entries[i].path);
> +
> +		if (repo_submodule_init(
> +			    &subrepos[i], r,
> +			    submodule_entry_list->name_entries[i].path,
> +			    &super_oid)) {
> +			die(_("submodule %s: unable to find submodule"),
> +			    submodules[i].name);
> +			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
> +				advise(_("You may try initializing the submodules using 'git checkout %s && git submodule update'"),
> +				       start_name);

Apart from what Ævar pointed out about advise() being called after die(),
I'm not sure this is the right advice, because if repo_submodule_init fails
it means there is no .git/modules/<name> directory corresponding to the submodule's
Git repository, i.e. the submodule was never cloned. So it's not guaranteed
that 'git checkout $start_name && git submodule update' would initialize (and clone) it,
not without '--init'.

> +		}
> +
> +		if (submodule_validate_branchname(
> +			    &subrepos[i], name,
> +			    oid_to_hex(
> +				    &submodule_entry_list->name_entries[i].oid),
> +			    force, quiet, &err_msg))
> +			die(_("submodule %s: could not create branch '%s'\n\t%s"),
> +			    submodules[i].name, name, err_msg);

minor nit abour wording: we did not try to create the branch here, we just checked if
creating is possible. So it *might* be confusing (maybe not). Maybe just "can not"
instead of "could not" ?

> +	}
> +
> +	create_branch(the_repository, name, start_name, force, 0, reflog, quiet,
> +		      track);

OK, here we create the superproject branch. This might not be expected when
just reading the name of the function...

> +
> +	for (i = 0; i < submodule_entry_list->entry_nr; i++) {

Here we loop over all submodules, so branches are created even in
inactive submodules... this might not be wanted.

> +		printf_ln(_("submodule %s: creating branch '%s'"),
> +			  submodules[i].name, name);
> +		if (submodule_create_branch(
> +			    &subrepos[i], name,
> +			    oid_to_hex(
> +				    &submodule_entry_list->name_entries[i].oid),
> +			    branch_point, force, reflog, quiet, track,
> +			    &err_msg))
> +			die(_("submodule %s: could not create branch '%s'\n\t%s"),
> +			    submodules[i].name, name, err_msg);
> +
> +		repo_clear(&subrepos[i]);
> +	}
> +}
> +
>   void remove_merge_branch_state(struct repository *r)
>   {
>   	unlink(git_path_merge_head(r));
> diff --git a/branch.h b/branch.h
> index d8e5ff4e28..1b4a635a2f 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -76,6 +76,12 @@ void create_branch(struct repository *r,
>   		   int force, int clobber_head_ok,
>   		   int reflog, int quiet, enum branch_track track);
>   
> +/*
> + * Creates a new branch in repository and its submodules.
> + */
> +void create_submodule_branches(struct repository *r, const char *name,
> +			       const char *start_name, int force, int reflog,
> +			       int quiet, enum branch_track track);
>   /*
>    * Check if 'name' can be a valid name for a branch; die otherwise.
>    * Return 1 if the named branch already exists; return 0 otherwise.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 5d4b9c82b4..6a16bdb1a3 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -39,6 +39,8 @@ static const char * const builtin_branch_usage[] = {
>   
>   static const char *head;
>   static struct object_id head_oid;
> +static int recurse_submodules = 0;
> +static int submodule_propagate_branches = 0;
>   
>   static int branch_use_color = -1;
>   static char branch_colors[][COLOR_MAXLEN] = {
> @@ -101,6 +103,15 @@ 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;
> +	}
> +	if (!strcasecmp(var, "submodule.propagateBranches")) {
> +		submodule_propagate_branches = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>   	return git_color_default_config(var, value, cb);
>   }
>   
> @@ -621,7 +632,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>   	int delete = 0, rename = 0, copy = 0, force = 0, list = 0, create = 0,
>   	    unset_upstream = 0, show_current = 0, edit_description = 0;
>   	/* possible options */
> -	int reflog = 0, quiet = 0, dry_run = 0, icase = 0;
> +	int reflog = 0, quiet = 0, dry_run = 0, icase = 0,
> +	    recurse_submodules_explicit = 0;
>   	const char *new_upstream = NULL;
>   	enum branch_track track;
>   	struct ref_filter filter;
> @@ -670,6 +682,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__DRY_RUN(&dry_run, N_("show whether the branch would be created")),
>   		OPT_END(),
> @@ -713,9 +726,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>   	if (create < 0)
>   		usage_with_options(builtin_branch_usage, options);
>   
> +	if (recurse_submodules_explicit && submodule_propagate_branches &&
> +	    !create)
> +		die(_("--recurse-submodules can only be used to create branches"));
>   	if (dry_run && !create)
>   		die(_("--dry-run can only be used when creating branches"));
>   
> +	recurse_submodules =
> +		(recurse_submodules || recurse_submodules_explicit) &&
> +		submodule_propagate_branches;
> +

OK, so we get the new behaviour if either --recurse-submodules was used, or 'submodule.recurse' is true,
and in both case we also need the new submodule.propagateBranches config set.

Why not adding 'branch.recurseSubmodules' instead, with a higher priority than submodule.recurse ?
Is it because then it would be mildly confusing for 'git checkout / git switch' to also honor
a setting named 'branch.*' when they learn the new behaviour ? (I don't think this would be the
first time that 'git foo' honors 'bar.*', so it might be worth mentioning).

Also, why do we quietly ignore '--recurse-submodules' if submodule.propagateBranches is unset ?
Wouldn't it be better to warn the user "hey, if you want this new behaviour you need to
set that config !" ?

I don't have a strong opinion about the fact that you need to set the config in the first
place, but I think it should be mentioned in the commit message why you chose to implement
it that way (meaning, why do we need a config set, instead of adding the config but defaulting it
to true, so that you get the new behaviour by default, but you still can disable it if you do not
want it)...

>   	if (filter.abbrev == -1)
>   		filter.abbrev = DEFAULT_ABBREV;
>   	filter.ignore_case = icase;
> @@ -874,6 +894,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>   			FREE_AND_NULL(unused_full_ref);
>   			return 0;
>   		}
> +		if (recurse_submodules) {
> +			create_submodule_branches(the_repository, branch_name,
> +						  start_name, force, reflog,
> +						  quiet, track);
> +			return 0;
> +		}
>   		create_branch(the_repository, branch_name, start_name, force, 0,
>   			      reflog, quiet, track);
>   	} else
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 6298cbdd4e..3ea1e8cc96 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -20,6 +20,7 @@
>   #include "diff.h"
>   #include "object-store.h"
>   #include "advice.h"
> +#include "branch.h"
>   
>   #define OPT_QUIET (1 << 0)
>   #define OPT_CACHED (1 << 1)
> @@ -2983,6 +2984,37 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>   	return !!ret;
>   }
>   
> +static int module_create_branch(int argc, const char **argv, const char *prefix)
> +{
> +	enum branch_track track;
> +	int quiet = 0, force = 0, reflog = 0;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet, N_("print only error messages")),
> +		OPT__FORCE(&force, N_("force creation"), 0),
> +		OPT_BOOL(0, "create-reflog", &reflog,
> +			 N_("create the branch's reflog")),
> +		OPT_SET_INT('t', "track", &track,
> +			    N_("set up tracking mode (see git-pull(1))"),
> +			    BRANCH_TRACK_EXPLICIT),
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper create-branch [-f|--force] [--create-reflog] [-q|--quiet] [-t|--track] <name> <start_oid> <start_name>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +
> +	if (argc != 3)
> +		usage_with_options(usage, options);
> +
> +	create_branch(the_repository, argv[0], argv[1], force, 0, reflog, quiet,
> +		      BRANCH_TRACK_NEVER);
> +	setup_tracking(argv[0], argv[2], track, quiet, 0);
> +
> +	return 0;
> +}
>   struct add_data {
>   	const char *prefix;
>   	const char *branch;
> @@ -3379,6 +3411,7 @@ static struct cmd_struct commands[] = {
>   	{"config", module_config, 0},
>   	{"set-url", module_set_url, 0},
>   	{"set-branch", module_set_branch, 0},
> +	{"create-branch", module_create_branch, 0},
>   };
>   
>   int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh
> new file mode 100755
> index 0000000000..14ff066e91
> --- /dev/null
> +++ b/t/t3207-branch-submodule.sh
> @@ -0,0 +1,249 @@
> +#!/bin/sh
> +
> +test_description='git branch submodule tests'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
> +
> +test_expect_success 'setup superproject and submodule' '
> +	git init super &&
> +	test_commit foo &&
> +	git init sub-upstream &&
> +	test_commit -C sub-upstream foo &&
> +	git -C super submodule add ../sub-upstream sub &&
> +	git -C super commit -m "add submodule" &&
> +	git -C super config submodule.propagateBranches true
> +'
> +
> +cleanup_branches() {
> +	super_dir="$1"
> +	shift
> +	(
> +		cd "$super_dir" &&
> +		git checkout main &&
> +		for branch_name in "$@"; do
> +			git branch -D "$branch_name"
> +			git submodule foreach "(git checkout main && git branch -D $branch_name) || true"
> +		done
> +	)
> +} >/dev/null 2>/dev/null
> +
> +# Test the argument parsing
> +test_expect_success '--recurse-submodules should create branches' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git branch --recurse-submodules branch-a &&
> +		git rev-parse --abbrev-ref branch-a &&
> +		git -C sub rev-parse --abbrev-ref branch-a
> +	)
> +'
> +
> +test_expect_success '--recurse-submodules should be ignored if submodule.propagateBranches is false' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git -c submodule.propagateBranches=false branch --recurse-submodules branch-a &&
> +		git rev-parse branch-a &&
> +		test_must_fail git -C sub rev-parse branch-a
> +	)
> +'
> +
> +test_expect_success '--recurse-submodules should fail when not creating branches' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git branch --recurse-submodules branch-a &&
> +		test_must_fail git branch --recurse-submodules -D branch-a &&
> +		# Assert that the branches were not deleted
> +		git rev-parse --abbrev-ref branch-a &&
> +		git -C sub rev-parse --abbrev-ref branch-a
> +	)
> +'
> +
> +test_expect_success 'should respect submodule.recurse when creating branches' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git -c submodule.recurse=true branch branch-a &&
> +		git rev-parse --abbrev-ref branch-a &&
> +		git -C sub rev-parse --abbrev-ref branch-a
> +	)
> +'
> +
> +test_expect_success 'should ignore submodule.recurse when not creating branches' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git branch --recurse-submodules branch-a &&
> +		git -c submodule.recurse=true branch -D branch-a &&
> +		test_must_fail git rev-parse --abbrev-ref branch-a &&
> +		git -C sub rev-parse --abbrev-ref branch-a
> +	)
> +'
> +
> +# Test branch creation behavior
> +test_expect_success 'should create branches based off commit id in superproject' '
> +	test_when_finished "cleanup_branches super branch-a branch-b" &&
> +	(
> +		cd super &&
> +		git branch --recurse-submodules branch-a &&
> +		git checkout --recurse-submodules branch-a &&
> +		git -C sub rev-parse HEAD >expected &&
> +		# Move the tip of sub:branch-a so that it no longer matches the commit in super:branch-a
> +		git -C sub checkout branch-a &&
> +		test_commit -C sub bar &&
> +		# Create a new branch-b branch with start-point=branch-a
> +		git branch --recurse-submodules branch-b branch-a &&
> +		git rev-parse branch-b &&
> +		git -C sub rev-parse branch-b >actual &&
> +		# Assert that the commit id of sub:second-branch matches super:branch-a and not sub:branch-a
> +		test_cmp expected actual
> +	)
> +'
> +
> +test_expect_success 'should not create any branches if branch is not valid for all repos' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		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 &&
> +
> +		cat >expected <<EOF &&
> +fatal: submodule sub: could not create branch ${SQ}branch-a${SQ}
> +	fatal: A branch named ${SQ}branch-a${SQ} already exists.
> +
> +EOF
> +		test_cmp expected actual
> +	)
> +'
> +
> +test_expect_success 'should create branches if branch exists and --force is given' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git -C sub rev-parse HEAD >expected &&
> +		test_commit -C sub baz &&
> +		git -C sub branch branch-a HEAD~1 &&
> +		git branch --recurse-submodules --force branch-a &&
> +		git rev-parse branch-a &&
> +		# assert that sub:branch-a was moved
> +		git -C sub rev-parse branch-a >actual &&
> +		test_cmp expected actual
> +	)
> +'
> +
> +test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
> +	(
> +		cd super &&
> +		git branch branch-a &&
> +		git checkout -b branch-b &&
> +		git submodule add ../sub-upstream sub2 &&
> +		# branch-b now has a committed submodule not in branch-a
> +		git commit -m "add second submodule" &&
> +		git checkout branch-a &&
> +		git branch --recurse-submodules branch-c branch-b &&
> +		git rev-parse branch-c &&
> +		git -C sub rev-parse branch-c &&
> +		git checkout --recurse-submodules branch-c &&
> +		git -C sub2 rev-parse branch-c
> +	)
> +'
> +
> +test_expect_success 'should set up tracking of local branches with track=always' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		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_expect_success 'should set up tracking of local branches with explicit track' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		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_expect_success 'should not set up unnecessary tracking of local branches' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		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)" = ""
> +	)

don't we have a "config is empty" test helper or something similar ?

> +'
> +
> +test_expect_success 'setup remote-tracking tests' '
> +	(
> +		cd super &&
> +		git branch branch-a &&
> +		git checkout -b branch-b &&
> +		git submodule add ../sub-upstream sub2 &&
> +		# branch-b now has a committed submodule not in branch-a
> +		git commit -m "add second submodule"
> +	) &&
> +	(
> +		cd sub-upstream &&
> +		git branch branch-a
> +	) &&
> +	git clone --branch main --recurse-submodules super super-clone &&
> +	git -C super-clone config submodule.propagateBranches true
> +'
> +
> +test_expect_success 'should not create branch when submodule is not in .git/modules' '
> +	# The cleanup needs to delete sub2:branch-b in particular because main does not have sub2
> +	test_when_finished "git -C super-clone/sub2 branch -D branch-b && \
> +		cleanup_branches super-clone branch-a branch-b" &&
> +	(
> +		cd super-clone &&
> +		# This should succeed because super-clone has sub.
> +		git branch --recurse-submodules branch-a origin/branch-a &&
> +		# This should fail because super-clone does not have sub2.
> +		test_must_fail git branch --recurse-submodules branch-b origin/branch-b 2>actual &&
> +		cat >expected <<-EOF &&
> +		fatal: submodule sub: unable to find submodule
> +		You may reinitialize the submodules using ${SQ}git checkout origin/branch-b && git submodule update${SQ}
> +		EOF
> +		test_must_fail git rev-parse branch-b &&
> +		test_must_fail git -C sub rev-parse branch-b &&
> +		# User can fix themselves by initializing the submodule
> +		git checkout origin/branch-b &&
> +		git submodule update &&
> +		git branch --recurse-submodules branch-b origin/branch-b
> +	)

Considering what has been pointed out above, I'm not sure why this test passes...
Unless I'm missing something.

> +'
> +
> +test_expect_success 'should set up tracking of remote-tracking branches' '
> +	test_when_finished "cleanup_branches super-clone branch-a" &&
> +	(
> +		cd super-clone &&
> +		git branch --recurse-submodules branch-a origin/branch-a &&
> +		test "$(git -C sub config branch.branch-a.remote)" = origin &&
> +		test "$(git -C sub config branch.branch-a.merge)" = refs/heads/branch-a
> +	)
> +'
> +
> +test_expect_success 'should not fail when unable to set up tracking in submodule' '
> +	test_when_finished "cleanup_branches super-clone branch-b" &&
> +	(
> +		cd super-clone &&
> +		git branch --recurse-submodules branch-b origin/branch-b
> +	)
> +'
> +
> +test_done
> 

Cheers,

Philippe.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] submodule-config: add submodules_of_tree() helper
  2021-11-23  2:12   ` Jonathan Tan
@ 2021-11-23 19:48     ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-23 19:48 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, jonathantanmy, steadmon, emilyshaffer

Jonathan Tan <jonathantanmy@google.com> writes:

> I think that tree_entry() doesn't recurse into subtrees, but in any case we
> should test this. (I looked at patch 4 and I think that the submodules are
> always in the root tree.)

I've tested this and indeed it doesn't work. I've attached my test case
below.

> This reminded me of a similar thing when fetching submodules recursively and we
> needed the "before" and "after" of submodule gitlinks. You can look at the code
> (collect_changed_submodules_cb() and the functions that use it in submodule.c)
> but it may not be useful - in particular, that uses diff since we need to see
> differences there, but we don't need that here.

Thanks for the hint. If that fails, I could also implement it via the
helper methods in submodule--helper.

---- >8 ----

diff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh
index 14ff066e91..0e086f716d 100755
--- a/t/t3207-branch-submodule.sh
+++ b/t/t3207-branch-submodule.sh
@@ -11,11 +11,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 test_expect_success 'setup superproject and submodule' '
 	git init super &&
 	test_commit foo &&
+	git init sub-sub-upstream &&
+	test_commit -C sub-sub-upstream foo &&
 	git init sub-upstream &&
-	test_commit -C sub-upstream foo &&
-	git -C super submodule add ../sub-upstream sub &&
+	git -C sub-upstream submodule add "$TRASH_DIRECTORY/sub-sub-upstream" sub-sub &&
+	git -C sub-upstream commit -m "add submodule" &&
+	git -C super submodule add "$TRASH_DIRECTORY/sub-upstream" sub &&
 	git -C super commit -m "add submodule" &&
-	git -C super config submodule.propagateBranches true
+	git -C super config submodule.propagateBranches true &&
+	git -C super/sub submodule update --init
 '
 
 cleanup_branches() {
@@ -26,7 +30,7 @@ cleanup_branches() {
 		git checkout main &&
 		for branch_name in "$@"; do
 			git branch -D "$branch_name"
-			git submodule foreach "(git checkout main && git branch -D $branch_name) || true"
+			git submodule foreach "cleanup_branches . $branch_name || true"
 		done
 	)
 } >/dev/null 2>/dev/null
@@ -37,8 +41,9 @@ test_expect_success '--recurse-submodules should create branches' '
 	(
 		cd super &&
 		git branch --recurse-submodules branch-a &&
-		git rev-parse --abbrev-ref branch-a &&
-		git -C sub rev-parse --abbrev-ref branch-a
+		git rev-parse branch-a &&
+		git -C sub rev-parse branch-a &&
+		git -C sub/sub-sub rev-parse branch-a
 	)
 '
 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] submodule-config: add submodules_of_tree() helper
  2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
  2021-11-23  2:12   ` Jonathan Tan
  2021-11-23 10:53   ` Ævar Arnfjörð Bjarmason
@ 2021-11-23 22:46   ` Junio C Hamano
  2 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-11-23 22:46 UTC (permalink / raw)
  To: Glen Choo; +Cc: git, Jonathan Tan, Josh Steadmon, Emily Shaffer

Glen Choo <chooglen@google.com> writes:

> +struct submodule_entry_list *
> +submodules_of_tree(struct repository *r, const struct object_id *treeish_name)
> +{
> +	struct tree_desc tree;
> +	struct name_entry entry;
> +	struct submodule_entry_list *ret;
> +
> +	CALLOC_ARRAY(ret, 1);
> +	fill_tree_descriptor(r, &tree, treeish_name);
> +	while (tree_entry(&tree, &entry)) {
> +		if (!S_ISGITLINK(entry.mode))
> +			continue;
> +		ALLOC_GROW(ret->name_entries, ret->entry_nr + 1, ret->entry_alloc);
> +		ret->name_entries[ret->entry_nr++] = entry;
> +	}
> +	return ret;
> +}

This only looks at the root level of the tree, doesn't it?  Without
any caller in the same step, it is impossible to tell if that is an
outright bug, or merely an incomplete code that will gain recursion
in a later step.

If it is the latter, I do not think that is a patch series
organization that is very friendly to reviewers.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] branch: add --dry-run option to branch
  2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
  2021-11-23 10:42   ` Ævar Arnfjörð Bjarmason
@ 2021-11-23 23:10   ` Jonathan Tan
  2021-11-24  0:52     ` Glen Choo
  1 sibling, 1 reply; 21+ messages in thread
From: Jonathan Tan @ 2021-11-23 23:10 UTC (permalink / raw)
  To: chooglen; +Cc: git, jonathantanmy, steadmon, emilyshaffer

Glen Choo <chooglen@google.com> writes:
> When running "git branch --recurse-submodules topic"

At this point, this argument has not been introduced yet, so better to
just say "A future patch will introduce branch creation that recurses
into submodules, so..."

> +-n::
> +--dry-run::
> +	Can only be used when creating a branch. If the branch creation
> +	would fail, show the relevant error message. If the branch
> +	creation would succeed, show nothing.

Right now we only plan to use this internally so it's not worth using a
single character argument for this right now, I think. We can always add
it later if we find it useful.

> -	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
> -	    list + edit_description + unset_upstream > 1)
> +	create = 1 - (!!delete + !!rename + !!copy + !!new_upstream +
> +		      !!show_current + !!list + !!edit_description +
> +		      !!unset_upstream);
> +	if (create < 0)
>  		usage_with_options(builtin_branch_usage, options);

Hmm...I think it would be clearer just to call it noncreate_options and
print usage if it is greater than 1. Then whenever you want to check if
it's create, check `!noncreate_options` instead.

> @@ -852,10 +862,20 @@ 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);
> +		if (dry_run) {
> +			struct strbuf buf = STRBUF_INIT;
> +			char *unused_full_ref;
> +			struct object_id unused_oid;
>  
> +			validate_new_branchname(branch_name, &buf, force);
> +			validate_branch_start(the_repository, start_name, track,
> +					      &unused_oid, &unused_full_ref);
> +			strbuf_release(&buf);
> +			FREE_AND_NULL(unused_full_ref);
> +			return 0;
> +		}
> +		create_branch(the_repository, branch_name, start_name, force, 0,
> +			      reflog, quiet, track);
>  	} else
>  		usage_with_options(builtin_branch_usage, options);
>  

I don't think we should use separate code paths for the dry run and the
regular run - could create_branch() take a dry_run parameter instead?
(If there are too many boolean parameters, it might be time to convert
some or all to a struct.)

This suggestion would require a reworking of patch 2, which is why I
didn't comment there. But if we are not going to use this suggestion and
are going to stick with patch 2, then my comment on it is that it seems
to be doing too much: I ran "git show --color-moved" on it and saw that
quite a few lines were newly introduced (not just moved around).

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-23 19:41   ` Philippe Blain
@ 2021-11-23 23:43     ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-23 23:43 UTC (permalink / raw)
  To: Philippe Blain, git
  Cc: Jonathan Tan, Josh Steadmon, Emily Shaffer,
	Ævar Arnfjörð Bjarmason


Thanks for the thorough review! I really appreciate it, Philippe :)

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Hi Glen,
>
> Le 2021-11-22 à 17:32, Glen Choo a écrit :
>> Add a --recurse-submodules option when creating branches so that `git
>> branch --recurse-submodules topic` will create the "topic" branch in the
>> superproject and all submodules. Guard this (and future submodule
>> branching) behavior behind a new configuration value
>> 'submodule.propagateBranches'.
>> 
>> Signed-off-by: Glen Choo <chooglen@google.com>
>> ---
>>   Documentation/config/advice.txt    |   3 +
>>   Documentation/config/submodule.txt |   9 ++
>
> We would need to add the new flag to Documentation/git-branch.txt,
> and also probably update the documentation of 'submodule.recurse'
> in 'Documentation/config/submodule.txt'.

Yes, thanks for the catch.

>> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
>> index ee454f8126..c318b849aa 100644
>> --- a/Documentation/config/submodule.txt
>> +++ b/Documentation/config/submodule.txt
>> @@ -72,6 +72,15 @@ submodule.recurse::
>>   	For these commands a workaround is to temporarily change the
>>   	configuration value by using `git -c submodule.recurse=0`.
>>   
>> +submodule.propagateBranches::
>> +	[EXPERIMENTAL] A boolean that enables branching support with
>> +	submodules. This allows certain commands to accept
>> +	`--recurse-submodules` (`git branch --recurse-submodules` will
>> +	create branches recursively), and certain commands that already
>> +	accept `--recurse-submodules` will now consider branches (`git
>> +	switch --recurse-submodules` will switch to the correct branch
>> +	in all submodules).
>
> Looking at the rest of the patch, this just implements 'branch --recurse-submodules', right ?
> i.e. 'git switch' and 'git checkout' are left alone for
> now, so I think this addition to the doc should only mention 'git
> branch'.

That sounds reasonable. I can move this description into the commit
message instead.

>> diff --git a/advice.h b/advice.h
>> index 601265fd10..a7521d6087 100644
>> --- a/advice.h
>> +
>> +		if (repo_submodule_init(
>> +			    &subrepos[i], r,
>> +			    submodule_entry_list->name_entries[i].path,
>> +			    &super_oid)) {
>> +			die(_("submodule %s: unable to find submodule"),
>> +			    submodules[i].name);
>> +			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
>> +				advise(_("You may try initializing the submodules using 'git checkout %s && git submodule update'"),
>> +				       start_name);
>
> Apart from what Ævar pointed out about advise() being called after die(),
> I'm not sure this is the right advice, because if repo_submodule_init fails
> it means there is no .git/modules/<name> directory corresponding to the submodule's
> Git repository, i.e. the submodule was never cloned. So it's not guaranteed
> that 'git checkout $start_name && git submodule update' would initialize (and clone) it,
> not without '--init'.

After further testing, it seems that --init might be required for
recursive submodules, but as you note later on, it's not needed for the
test case I have created. Using --init is still good advice though, so I
will add that.

>> +	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
>
> Here we loop over all submodules, so branches are created even in
> inactive submodules... this might not be wanted.

Yes, we should ignore inactive submodules. This is a bug.

>> @@ -713,9 +726,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>   	if (create < 0)
>>   		usage_with_options(builtin_branch_usage, options);
>>   
>> +	if (recurse_submodules_explicit && submodule_propagate_branches &&
>> +	    !create)
>> +		die(_("--recurse-submodules can only be used to create branches"));
>>   	if (dry_run && !create)
>>   		die(_("--dry-run can only be used when creating branches"));
>>   
>> +	recurse_submodules =
>> +		(recurse_submodules || recurse_submodules_explicit) &&
>> +		submodule_propagate_branches;
>> +
>
> OK, so we get the new behaviour if either --recurse-submodules was used, or 'submodule.recurse' is true,
> and in both case we also need the new submodule.propagateBranches config set.
>
> Why not adding 'branch.recurseSubmodules' instead, with a higher priority than submodule.recurse ?
> Is it because then it would be mildly confusing for 'git checkout / git switch' to also honor
> a setting named 'branch.*' when they learn the new behaviour ? (I don't think this would be the
> first time that 'git foo' honors 'bar.*', so it might be worth mentioning).

I am avoiding the prefix 'branch.' because that might suggest that the
functionality is centered around the 'git branch' command. I chose the
'submodule.' prefix because what we are feature flagging is an entirely
redesigned UX for _submodules_ that uses branches; we also have work
planned for other commands like push/merge/rebase/reset.

> Also, why do we quietly ignore '--recurse-submodules' if submodule.propagateBranches is unset ?
> Wouldn't it be better to warn the user "hey, if you want this new behaviour you need to
> set that config !" ?

Ah, yes this is an oversight on my part.

> I don't have a strong opinion about the fact that you need to set the config in the first
> place, but I think it should be mentioned in the commit message why you chose to implement
> it that way (meaning, why do we need a config set, instead of adding the config but defaulting it
> to true, so that you get the new behaviour by default, but you still can disable it if you do not
> want it)...

It seems self-evident to me that experimental features should not be
shipped to users by default.

>> +test_expect_success 'should not set up unnecessary tracking of local branches' '
>> +	test_when_finished "cleanup_branches super branch-a" &&
>> +	(
>> +		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)" = ""
>> +	)
>
> don't we have a "config is empty" test helper or something similar ?

Hm, I couldn't find one, but there is test_cmp_config(). That's probably
better than calling test() directly.

>> +test_expect_success 'should not create branch when submodule is not in .git/modules' '
>> +	# The cleanup needs to delete sub2:branch-b in particular because main does not have sub2
>> +	test_when_finished "git -C super-clone/sub2 branch -D branch-b && \
>> +		cleanup_branches super-clone branch-a branch-b" &&
>> +	(
>> +		cd super-clone &&
>> +		# This should succeed because super-clone has sub.
>> +		git branch --recurse-submodules branch-a origin/branch-a &&
>> +		# This should fail because super-clone does not have sub2.
>> +		test_must_fail git branch --recurse-submodules branch-b origin/branch-b 2>actual &&
>> +		cat >expected <<-EOF &&
>> +		fatal: submodule sub: unable to find submodule
>> +		You may reinitialize the submodules using ${SQ}git checkout origin/branch-b && git submodule update${SQ}
>> +		EOF
>> +		test_must_fail git rev-parse branch-b &&
>> +		test_must_fail git -C sub rev-parse branch-b &&
>> +		# User can fix themselves by initializing the submodule
>> +		git checkout origin/branch-b &&
>> +		git submodule update &&
>> +		git branch --recurse-submodules branch-b origin/branch-b
>> +	)
>
> Considering what has been pointed out above, I'm not sure why this test passes...
> Unless I'm missing something.

As I understand it, --init is used to set values in .git/config. My best
guess is that 'git submodule update' doesn't use .git/config at all - it
looks for submodules in the index and .gitmodules and clones the
submodules as expected.

I still think that we should promote --init, but I still find this
situation very strange and inconsistent.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] branch: add --dry-run option to branch
  2021-11-23 23:10   ` Jonathan Tan
@ 2021-11-24  0:52     ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-11-24  0:52 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, jonathantanmy, steadmon, emilyshaffer

Jonathan Tan <jonathantanmy@google.com> writes:

> Glen Choo <chooglen@google.com> writes:
>> When running "git branch --recurse-submodules topic"
>
> At this point, this argument has not been introduced yet, so better to
> just say "A future patch will introduce branch creation that recurses
> into submodules, so..."
>
>> +-n::
>> +--dry-run::
>> +	Can only be used when creating a branch. If the branch creation
>> +	would fail, show the relevant error message. If the branch
>> +	creation would succeed, show nothing.
>
> Right now we only plan to use this internally so it's not worth using a
> single character argument for this right now, I think. We can always add
> it later if we find it useful.

For the reasons you specified, I didn't intend to add -n. However, -n is
automatically added by OPT__DRY_RUN, so I thought it was appropriate to
document it.

>> -	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
>> -	    list + edit_description + unset_upstream > 1)
>> +	create = 1 - (!!delete + !!rename + !!copy + !!new_upstream +
>> +		      !!show_current + !!list + !!edit_description +
>> +		      !!unset_upstream);
>> +	if (create < 0)
>>  		usage_with_options(builtin_branch_usage, options);
>
> Hmm...I think it would be clearer just to call it noncreate_options and
> print usage if it is greater than 1. Then whenever you want to check if
> it's create, check `!noncreate_options` instead.

Sounds good.

>> @@ -852,10 +862,20 @@ 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);
>> +		if (dry_run) {
>> +			struct strbuf buf = STRBUF_INIT;
>> +			char *unused_full_ref;
>> +			struct object_id unused_oid;
>>  
>> +			validate_new_branchname(branch_name, &buf, force);
>> +			validate_branch_start(the_repository, start_name, track,
>> +					      &unused_oid, &unused_full_ref);
>> +			strbuf_release(&buf);
>> +			FREE_AND_NULL(unused_full_ref);
>> +			return 0;
>> +		}
>> +		create_branch(the_repository, branch_name, start_name, force, 0,
>> +			      reflog, quiet, track);
>>  	} else
>>  		usage_with_options(builtin_branch_usage, options);
>>  
>
> I don't think we should use separate code paths for the dry run and the
> regular run - could create_branch() take a dry_run parameter instead?
> (If there are too many boolean parameters, it might be time to convert
> some or all to a struct.)

That sounds reasonable, it would be good not to have duplicate code
paths.

> This suggestion would require a reworking of patch 2, which is why I
> didn't comment there. But if we are not going to use this suggestion and
> are going to stick with patch 2, then my comment on it is that it seems
> to be doing too much: I ran "git show --color-moved" on it and saw that
> quite a few lines were newly introduced (not just moved around).

I will do the reworking, but the final result will probably look very
similar to the one in patch 2. Does it look more acceptable with
--color-moved-ws=ignore-all-space? Some text had to be reindented
(because I removed one conditional), but I also replaced some functions
with repo_* versions.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
  2021-11-23 10:45   ` Ævar Arnfjörð Bjarmason
  2021-11-23 19:41   ` Philippe Blain
@ 2021-11-24  1:31   ` Jonathan Tan
  2021-11-24 18:18     ` Glen Choo
  2 siblings, 1 reply; 21+ messages in thread
From: Jonathan Tan @ 2021-11-24  1:31 UTC (permalink / raw)
  To: chooglen; +Cc: git, jonathantanmy, steadmon, emilyshaffer

Glen Choo <chooglen@google.com> writes:
> +submodule.propagateBranches::
> +	[EXPERIMENTAL] A boolean that enables branching support with
> +	submodules. This allows certain commands to accept
> +	`--recurse-submodules` (`git branch --recurse-submodules` will
> +	create branches recursively), and certain commands that already
> +	accept `--recurse-submodules` will now consider branches (`git
> +	switch --recurse-submodules` will switch to the correct branch
> +	in all submodules).

After some thought, I think that the way in this patch is the best way
to do it, even if it's unfortunate that a natural-looking "git branch
--recurse-submodules" wouldn't work without
"submodule.propagateBranches" (or whatever we decide to call the
variable).

Right now, as far as I know, "--recurse-submodules" in all Git commands
(and equivalently, the configuration variable "submodule.recurse") does
the same thing but in submodules too. For example, "fetch" fetches in
submodules, "switch" switches in submodules, and so on. It would be
plausible for "branch --recurse-submodules" to also create a branch in
submodules, but if a user just used it as-is, it would be surprising if
subsequent commands like "switch --recurse-submodules" or "-c
submodule.recurse=true switch" didn't use the branches that were just
created.

So OK, this makes sense.

> +static int submodule_create_branch(struct repository *r, const char *name,
> +				   const char *start_oid,
> +				   const char *start_name, int force,
> +				   int reflog, int quiet,
> +				   enum branch_track track, char **err_msg)
> +{
> +	int ret = 0;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	struct strbuf child_err = STRBUF_INIT;
> +	child.git_cmd = 1;
> +	child.err = -1;
> +
> +	prepare_other_repo_env(&child.env_array, r->gitdir);
> +	strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);

Before this function is a function that calls "git branch" directly -
couldn't this function do the same? (And then you can make both of them
into one function.) The functionality should be exactly the same except
that one has "--dry-run" and the other doesn't.

> @@ -874,6 +894,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			FREE_AND_NULL(unused_full_ref);
>  			return 0;
>  		}
> +		if (recurse_submodules) {
> +			create_submodule_branches(the_repository, branch_name,
> +						  start_name, force, reflog,
> +						  quiet, track);
> +			return 0;
> +		}
>  		create_branch(the_repository, branch_name, start_name, force, 0,
>  			      reflog, quiet, track);
>  	} else

create_submodule_branches() here is a bit misleading since it also
creates branches in the_repository. Might be better to write explicitly
check in submodules -> create in main repository -> create in
submodules. Or, if you want to combine all the submodule code in one
function, (check in submodules -> create in submodules) -> create in
main repository.

> +test_expect_success 'setup superproject and submodule' '
> +	git init super &&
> +	test_commit foo &&
> +	git init sub-upstream &&
> +	test_commit -C sub-upstream foo &&
> +	git -C super submodule add ../sub-upstream sub &&
> +	git -C super commit -m "add submodule" &&
> +	git -C super config submodule.propagateBranches true
> +'

If making each test independent is important (which seems like a good
goal to me, although I know that the Git tests are inconsistent on
that), we could make this into a bash function (with test_when_finished)
that gets called in every test. It doesn't violate the t/README request
to put all test code inside test_expect_success assertions (since the
function is still being run inside an assertion).

In the general case, it will make test code slower to run, but if you're
going to have to cleanup branches, I think it's better to just recreate
the repo. In any case, for the general case, I can start a separate
email thread for this discussion.

> +test_expect_success '--recurse-submodules should be ignored if submodule.propagateBranches is false' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git -c submodule.propagateBranches=false branch --recurse-submodules branch-a &&
> +		git rev-parse branch-a &&
> +		test_must_fail git -C sub rev-parse branch-a
> +	)
> +'

This doesn't sound like the right behavior to me - I think it's fine if
it was the config "submodule.recurse" instead of "--recurse-submodules",
but if the argument is given on CLI, it should be a fatal error.

> +test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
> +	(
> +		cd super &&
> +		git branch branch-a &&
> +		git checkout -b branch-b &&
> +		git submodule add ../sub-upstream sub2 &&
> +		# branch-b now has a committed submodule not in branch-a
> +		git commit -m "add second submodule" &&
> +		git checkout branch-a &&
> +		git branch --recurse-submodules branch-c branch-b &&
> +		git rev-parse branch-c &&
> +		git -C sub rev-parse branch-c &&
> +		git checkout --recurse-submodules branch-c &&
> +		git -C sub2 rev-parse branch-c
> +	)
> +'

Hmm...how is this submodule in .git/modules but not .gitmodules? It
looks like a normal submodule to me.

> +test_expect_success 'should not create branch when submodule is not in .git/modules' '

The title of this test contradicts the title of the test that I quoted
previously.

> +test_expect_success 'should not fail when unable to set up tracking in submodule' '
> +	test_when_finished "cleanup_branches super-clone branch-b" &&
> +	(
> +		cd super-clone &&
> +		git branch --recurse-submodules branch-b origin/branch-b
> +	)
> +'

Is there a warning printed that we can check?

Also, this patch set doesn't discuss the case in which the branch in a
submodule already exists, but it points to the exact commit that we
want. What is the functionality in that case? I would say that the user
should be able to recursively create the branch in this case, but am
open to other opinions. In any case, that case should be tested.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-24  1:31   ` Jonathan Tan
@ 2021-11-24 18:18     ` Glen Choo
  2021-11-29 21:01       ` Jonathan Tan
  0 siblings, 1 reply; 21+ messages in thread
From: Glen Choo @ 2021-11-24 18:18 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, jonathantanmy, steadmon, emilyshaffer


Thanks! The feedback is really useful.

Jonathan Tan <jonathantanmy@google.com> writes:

>> +static int submodule_create_branch(struct repository *r, const char *name,
>> +				   const char *start_oid,
>> +				   const char *start_name, int force,
>> +				   int reflog, int quiet,
>> +				   enum branch_track track, char **err_msg)
>> +{
>> +	int ret = 0;
>> +	struct child_process child = CHILD_PROCESS_INIT;
>> +	struct strbuf child_err = STRBUF_INIT;
>> +	child.git_cmd = 1;
>> +	child.err = -1;
>> +
>> +	prepare_other_repo_env(&child.env_array, r->gitdir);
>> +	strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);
>
> Before this function is a function that calls "git branch" directly -
> couldn't this function do the same? (And then you can make both of them
> into one function.) The functionality should be exactly the same except
> that one has "--dry-run" and the other doesn't.

I see two somewhat valid interpretations, so I will address both.

If you are suggesting that I should call "git branch" instead of a new
"git submodule--helper create-branch", that unfortunately does not work.
Because of how we've defined the semantics, we want the submodule to
branch off the commit in the superproject tree (which is a bare object
id), but we want to set up tracking based off the ref that the user gave
(evaluating it in the context of the submodule). This is why
submodule--helper.c:module_create_branch() makes two calls like so:

	create_branch(the_repository, "<branch name>", "<object id>", force, 0, reflog, quiet,
		      BRANCH_TRACK_NEVER);
	setup_tracking("<branch name>", "<tracking name>", track, quiet, 0);

On the other hand, you might be suggesting that I should just add
--dry-run to "git submodule--helper create-branch". That way, the
dry-run form of the command is validating the non dry-run form (instead
of using "git branch --dry-run" to validate "git submodule--helper
create-branch"). That's a reasonable suggestion that avoids
bikeshedding around "git branch --dry-run".

>> @@ -874,6 +894,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  			FREE_AND_NULL(unused_full_ref);
>>  			return 0;
>>  		}
>> +		if (recurse_submodules) {
>> +			create_submodule_branches(the_repository, branch_name,
>> +						  start_name, force, reflog,
>> +						  quiet, track);
>> +			return 0;
>> +		}
>>  		create_branch(the_repository, branch_name, start_name, force, 0,
>>  			      reflog, quiet, track);
>>  	} else
>
> create_submodule_branches() here is a bit misleading since it also
> creates branches in the_repository. Might be better to write explicitly
> check in submodules -> create in main repository -> create in
> submodules. Or, if you want to combine all the submodule code in one
> function, (check in submodules -> create in submodules) -> create in
> main repository.

Philippe had a similar comment, I will rename it.

>> +test_expect_success 'setup superproject and submodule' '
>> +	git init super &&
>> +	test_commit foo &&
>> +	git init sub-upstream &&
>> +	test_commit -C sub-upstream foo &&
>> +	git -C super submodule add ../sub-upstream sub &&
>> +	git -C super commit -m "add submodule" &&
>> +	git -C super config submodule.propagateBranches true
>> +'
>
> If making each test independent is important (which seems like a good
> goal to me, although I know that the Git tests are inconsistent on
> that), we could make this into a bash function (with test_when_finished)
> that gets called in every test. It doesn't violate the t/README request
> to put all test code inside test_expect_success assertions (since the
> function is still being run inside an assertion).

That's an interesting idea and it's more likely to be correct than my
approach. I think it lines up better with testing best practices.
However...

> In the general case, it will make test code slower to run, but if you're
> going to have to cleanup branches, I think it's better to just recreate
> the repo. In any case, for the general case, I can start a separate
> email thread for this discussion.

I'm concerned about the same thing and I suspect that recreating the
repo won't be kindly received by some reviewers, even if they might be
able to stomach cleanup_branches(). I think your suggestion is a better
long-term direction, but I'd like to see discussion on the general case
before changing the tests. A separate thread sounds good.

>> +test_expect_success '--recurse-submodules should be ignored if submodule.propagateBranches is false' '
>> +	test_when_finished "cleanup_branches super branch-a" &&
>> +	(
>> +		cd super &&
>> +		git -c submodule.propagateBranches=false branch --recurse-submodules branch-a &&
>> +		git rev-parse branch-a &&
>> +		test_must_fail git -C sub rev-parse branch-a
>> +	)
>> +'
>
> This doesn't sound like the right behavior to me - I think it's fine if
> it was the config "submodule.recurse" instead of "--recurse-submodules",
> but if the argument is given on CLI, it should be a fatal error.

Philippe mentioned the same thing, which sounds right to me.

>> +test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
>> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
>> +	(
>> +		cd super &&
>> +		git branch branch-a &&
>> +		git checkout -b branch-b &&
>> +		git submodule add ../sub-upstream sub2 &&
>> +		# branch-b now has a committed submodule not in branch-a
>> +		git commit -m "add second submodule" &&
>> +		git checkout branch-a &&
>> +		git branch --recurse-submodules branch-c branch-b &&
>> +		git rev-parse branch-c &&
>> +		git -C sub rev-parse branch-c &&
>> +		git checkout --recurse-submodules branch-c &&
>> +		git -C sub2 rev-parse branch-c
>> +	)
>> +'
>
> Hmm...how is this submodule in .git/modules but not .gitmodules? It
> looks like a normal submodule to me.

The test title is probably too terse - the submodule is not in the
working tree's .gitmodules, but it is in branch-b's .gitmodules. I'll
reword the title.

>> +test_expect_success 'should not create branch when submodule is not in .git/modules' '
>
> The title of this test contradicts the title of the test that I quoted
> previously.

I'm not sure how this is a contradiction, from before..

  should create branch when submodule is in .git/modules but not
  [the working tree's] .gitmodules

meaning "we should create the branch if we can find the submodule in
.git/modules", i.e. the implication is: presence in .git/modules => can
create branch.

Whereas

  should not create branch when submodule is not in .git/modules

meaning "we should not create the branch if we cannot find the submodule
in .git/modules", i.e. the implication is: absence in .git/modules =>
cannot create branch.

Taken together, they assert that presence in .git/modules is a necessary
condition for the subodule branch to be created.

>> +test_expect_success 'should not fail when unable to set up tracking in submodule' '
>> +	test_when_finished "cleanup_branches super-clone branch-b" &&
>> +	(
>> +		cd super-clone &&
>> +		git branch --recurse-submodules branch-b origin/branch-b
>> +	)
>> +'
>
> Is there a warning printed that we can check?

"git branch" does not warn if tracking is not set up when it is not
explicitly required, so this does not warn. However, I can imagine that
if the superproject branch has tracking set up, a user might expect that
all submodules would also have tracking set up, and thus a warning might
be useful. I don't think it will be _that_ useful for most users, but at
least some users would probably appreciate it.

For slightly unrelated reasons, I tried to get the tracking info of a
newly created branch and it is tedious. For this reason and the fact
that I'm not sure if the benefit is that great, I'm tempted *not* to add
the warning, but perhaps you feel more strongly than I do?

> Also, this patch set doesn't discuss the case in which the branch in a
> submodule already exists, but it points to the exact commit that we
> want. What is the functionality in that case?

The behavior is identical to the general case where the branch already
exists - branch validation (git branch --dry-run) fails.

> I would say that the user should be able to recursively create the
> branch in this case, but am open to other opinions.

I'm inclined to disagree. To illustrate this in the real world, say a
user wants to create a 'new-feature' branch recursively, but there is
already a 'new-feature in a submodule. Here are two possible sets of
events that could lead to this situation:

1) the 'new-feature' branch was intended for the same work as the new
   branch but the user just happened to create the 'new-feature'
   subomdule branch first
2) the existing 'new-feature' branch doesn't actually contain the same
   work (maybe it's an overloaded term, or maybe the user used a branch
   to bookmark a commit before starting work on it)

What would happen if we allowed the branch to be created? In case 1,
the user would be more than happy (because we read their mind! woohoo!)
But in case 2, the user might not realize that their bookmark is getting
clobbered by the new recursive branch - they might try revisiting their
bookmark only to realize that they've accidentally committed on top of
it.

I think the incidence of case 2 is far lower than case 1's, but I think
it's reasonable to be defensive by default. In any case, we can change
the default after more user testing.

> In any case, that case should be tested.

Testing this case would make the intended behavior clearer, so I will
add this test.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation
  2021-11-24 18:18     ` Glen Choo
@ 2021-11-29 21:01       ` Jonathan Tan
  0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Tan @ 2021-11-29 21:01 UTC (permalink / raw)
  To: chooglen; +Cc: jonathantanmy, git, steadmon, emilyshaffer

Glen Choo <chooglen@google.com> writes:
> > Before this function is a function that calls "git branch" directly -
> > couldn't this function do the same? (And then you can make both of them
> > into one function.) The functionality should be exactly the same except
> > that one has "--dry-run" and the other doesn't.
> 
> I see two somewhat valid interpretations, so I will address both.
> 
> If you are suggesting that I should call "git branch" instead of a new
> "git submodule--helper create-branch", that unfortunately does not work.
> Because of how we've defined the semantics, we want the submodule to
> branch off the commit in the superproject tree (which is a bare object
> id), but we want to set up tracking based off the ref that the user gave
> (evaluating it in the context of the submodule). This is why
> submodule--helper.c:module_create_branch() makes two calls like so:
> 
> 	create_branch(the_repository, "<branch name>", "<object id>", force, 0, reflog, quiet,
> 		      BRANCH_TRACK_NEVER);
> 	setup_tracking("<branch name>", "<tracking name>", track, quiet, 0);

Ah, I meant this interpretation. In that case, could we do it like this:

 - Add a dry_run parameter to submodule_create_branch(). It should stay
   exactly the same (except passing dry_run), so that the same code path
   is executed with and without dry_run. Delete
   submodule_validate_branchname(). (This makes sense conceptually
   because we're validating that we can later create the branches with
   the exact same parameters.)

 - When actually creating the branches, call submodule_create_branch(),
   then another command to set up tracking. I think it makes more sense
   if this is done directly through a Git command. If you want to use
   setup_tracking() through submodule--helper, I think that needs an
   explanation as to why a Git command wouldn't work.

> On the other hand, you might be suggesting that I should just add
> --dry-run to "git submodule--helper create-branch". That way, the
> dry-run form of the command is validating the non dry-run form (instead
> of using "git branch --dry-run" to validate "git submodule--helper
> create-branch"). That's a reasonable suggestion that avoids
> bikeshedding around "git branch --dry-run".

I didn't mean this interpretation, but this idea is reasonable if we
needed such functionality.

> >> +test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
> >> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
> >> +	(
> >> +		cd super &&
> >> +		git branch branch-a &&
> >> +		git checkout -b branch-b &&
> >> +		git submodule add ../sub-upstream sub2 &&
> >> +		# branch-b now has a committed submodule not in branch-a
> >> +		git commit -m "add second submodule" &&
> >> +		git checkout branch-a &&
> >> +		git branch --recurse-submodules branch-c branch-b &&
> >> +		git rev-parse branch-c &&
> >> +		git -C sub rev-parse branch-c &&
> >> +		git checkout --recurse-submodules branch-c &&
> >> +		git -C sub2 rev-parse branch-c
> >> +	)
> >> +'
> >
> > Hmm...how is this submodule in .git/modules but not .gitmodules? It
> > looks like a normal submodule to me.
> 
> The test title is probably too terse - the submodule is not in the
> working tree's .gitmodules, but it is in branch-b's .gitmodules. I'll
> reword the title.

Ah, I see. I think just delete the "is in .git/modules" part, so
something like "should create branch when submodule is not in HEAD's
.gitmodules".

> >> +test_expect_success 'should not create branch when submodule is not in .git/modules' '
> >
> > The title of this test contradicts the title of the test that I quoted
> > previously.
> 
> I'm not sure how this is a contradiction, from before..

[snip]

Ah...yes you're right.

> >> +test_expect_success 'should not fail when unable to set up tracking in submodule' '
> >> +	test_when_finished "cleanup_branches super-clone branch-b" &&
> >> +	(
> >> +		cd super-clone &&
> >> +		git branch --recurse-submodules branch-b origin/branch-b
> >> +	)
> >> +'
> >
> > Is there a warning printed that we can check?
> 
> "git branch" does not warn if tracking is not set up when it is not
> explicitly required, so this does not warn. However, I can imagine that
> if the superproject branch has tracking set up, a user might expect that
> all submodules would also have tracking set up, and thus a warning might
> be useful. I don't think it will be _that_ useful for most users, but at
> least some users would probably appreciate it.
> 
> For slightly unrelated reasons, I tried to get the tracking info of a
> newly created branch and it is tedious. For this reason and the fact
> that I'm not sure if the benefit is that great, I'm tempted *not* to add
> the warning, but perhaps you feel more strongly than I do?

In that case, maybe explain why tracking cannot be set up and verify in
the test that no tracking was set up.

> > Also, this patch set doesn't discuss the case in which the branch in a
> > submodule already exists, but it points to the exact commit that we
> > want. What is the functionality in that case?
> 
> The behavior is identical to the general case where the branch already
> exists - branch validation (git branch --dry-run) fails.
> 
> > I would say that the user should be able to recursively create the
> > branch in this case, but am open to other opinions.
> 
> I'm inclined to disagree. To illustrate this in the real world, say a
> user wants to create a 'new-feature' branch recursively, but there is
> already a 'new-feature in a submodule. Here are two possible sets of
> events that could lead to this situation:
> 
> 1) the 'new-feature' branch was intended for the same work as the new
>    branch but the user just happened to create the 'new-feature'
>    subomdule branch first
> 2) the existing 'new-feature' branch doesn't actually contain the same
>    work (maybe it's an overloaded term, or maybe the user used a branch
>    to bookmark a commit before starting work on it)
> 
> What would happen if we allowed the branch to be created? In case 1,
> the user would be more than happy (because we read their mind! woohoo!)
> But in case 2, the user might not realize that their bookmark is getting
> clobbered by the new recursive branch - they might try revisiting their
> bookmark only to realize that they've accidentally committed on top of
> it.
> 
> I think the incidence of case 2 is far lower than case 1's, but I think
> it's reasonable to be defensive by default. In any case, we can change
> the default after more user testing.

OK, this makes sense.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-11-29 22:12 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
2021-11-23  2:12   ` Jonathan Tan
2021-11-23 19:48     ` Glen Choo
2021-11-23 10:53   ` Ævar Arnfjörð Bjarmason
2021-11-23 18:35     ` Glen Choo
2021-11-23 22:46   ` Junio C Hamano
2021-11-22 22:32 ` [PATCH 2/4] branch: refactor out branch validation from create_branch() Glen Choo
2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
2021-11-23 10:42   ` Ævar Arnfjörð Bjarmason
2021-11-23 18:42     ` Glen Choo
2021-11-23 23:10   ` Jonathan Tan
2021-11-24  0:52     ` Glen Choo
2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
2021-11-23 10:45   ` Ævar Arnfjörð Bjarmason
2021-11-23 18:56     ` Glen Choo
2021-11-23 19:41   ` Philippe Blain
2021-11-23 23:43     ` Glen Choo
2021-11-24  1:31   ` Jonathan Tan
2021-11-24 18:18     ` Glen Choo
2021-11-29 21:01       ` Jonathan Tan

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).