git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 1/5] branch: move --set-upstream-to behavior to setup_tracking()

From
Glen Choo <chooglen@google.com>
Date
Dec 9, 2021, 18:49 UTC
Message-ID
<20211209184928.71413-2-chooglen@google.com>
In-Reply-To
<20211209184928.71413-1-chooglen@google.com>

This refactor is motivated by a desire to add a "dry_run" parameter to create_branch() that will validate whether or not a branch can be created without actually creating it - this behavior be used in a subsequent commit that adds `git branch --recurse-submodules topic`.

Adding "dry_run" is not obvious because create_branch() is also used to set tracking information without creating a branch, i.e. when using --set-upstream-to. This appears to be a leftover from 4fc5006676 (Add branch --set-upstream, 2010-01-18), when --set-upstream would sometimes create a branch and sometimes update tracking information without creating a branch. However, we no longer support --set-upstream, so it makes more sense to set tracking information with another function, like setup_tracking(), and use create_branch() only to create branches. When this is done, it will be trivial to add "dry_run".

Do this refactor by moving the branch validation and dwim logic from create_branch() into a new function, validate_branch_start(), and call it from setup_tracking(). Now that setup_tracking() can perform dwim and tracking setup without creating a branch, use it in `git branch --set-upstream-to` and remove unnecessary behavior from create_branch().

Since there were none, add tests for creating a branch with `--force`.
Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c          | 147 +++++++++++++++++++++++++++-------------------
 branch.h          |  13 +++-
 builtin/branch.c  |   7 +--
 t/t3200-branch.sh |  17 ++++++
 4 files changed, 117 insertions(+), 67 deletions(-)
diff --git a/branch.c b/branch.c
index 29e835d367..9429936734 100644
--- a/branch.c
+++ b/branch.c
@@ -209,20 +209,26 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 	return 0;
 }
 
-/*
- * 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)
+static void validate_branch_start(struct repository *r, const char *start_name,
+				  enum branch_track track,
+				  struct object_id *oid, char **real_ref);
+
+void setup_tracking(const char *new_ref, const char *orig_ref,
+			   enum branch_track track, int quiet, int expand_orig)
 {
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	char *full_orig_ref;
+	struct object_id unused_oid;
 
 	memset(&tracking, 0, sizeof(tracking));
-	tracking.spec.dst = (char *)orig_ref;
+	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;
 	tracking.srcs = &tracking_srcs;
 	if (track != BRANCH_TRACK_INHERIT)
 		for_each_remote(find_tracked_branch, &tracking);
@@ -236,20 +242,22 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		case BRANCH_TRACK_OVERRIDE:
 			break;
 		default:
-			return;
+			goto cleanup;
 		}
 
 	if (tracking.matches > 1)
 		die(_("Not tracking: ambiguous information for ref %s"),
-		    orig_ref);
+		    full_orig_ref);
 
 	if (tracking.srcs->nr < 1)
-		string_list_append(tracking.srcs, orig_ref);
+		string_list_append(tracking.srcs, full_orig_ref);
 	if (install_branch_config_multiple_remotes(config_flags, new_ref, tracking.remote,
 			      tracking.srcs) < 0)
 		exit(-1);
 
+cleanup:
 	string_list_clear(tracking.srcs, 0);
+	free(full_orig_ref);
 }
 
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
@@ -332,33 +340,35 @@ 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)
+/**
+ * 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. This is
+ *     expanded with DWIM and assigned to real_ref.
+ *
+ *   - 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
+ *
+ *   - real_ref is an out parameter containing the full, 'real' form of
+ *     start_name e.g. refs/heads/main instead of main
+ *
+ */
+static void validate_branch_start(struct repository *r, const char *start_name,
+				  enum branch_track track,
+				  struct object_id *oid, char **real_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);
@@ -370,7 +380,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, real_ref,
+			      0)) {
 	case 0:
 		/* Not branching from any existing branch */
 		if (explicit_tracking)
@@ -378,12 +389,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(*real_ref, "refs/heads/") &&
+		    validate_remote_tracking_branch(*real_ref)) {
 			if (explicit_tracking)
 				die(_(upstream_not_branch), start_name);
 			else
-				FREE_AND_NULL(real_ref);
+				FREE_AND_NULL(*real_ref);
 		}
 		break;
 	default:
@@ -391,37 +402,55 @@ 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 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 6484bda8a2..46951c446b 100644
--- a/branch.h
+++ b/branch.h
@@ -18,6 +18,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:
  *
@@ -30,8 +39,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 81a29edb4a..e19aab5356 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -823,12 +823,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 267a624671..18e285a876 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
Previous: Glen ChooNext: Jonathan Tan
Message 31 of 110 in “implement branch --recurse-submodules”
  1. 0/4 implement branch --recurse-submodulesGlen Choo, Nov 22, 2021
  2. 1/4 submodule-config: add submodules_of_tree() helperGlen Choo, Nov 22, 2021
  3. Jonathan TanNov 23, 2021
  4. Glen ChooNov 23, 2021
  5. Ævar Arnfjörð BjarmasonNov 23, 2021
  6. Glen ChooNov 23, 2021
  7. Junio C HamanoNov 23, 2021
  8. 2/4 branch: refactor out branch validation from create_branch()Glen Choo, Nov 22, 2021
  9. 3/4 branch: add --dry-run option to branchGlen Choo, Nov 22, 2021
  10. Ævar Arnfjörð BjarmasonNov 23, 2021
  11. Glen ChooNov 23, 2021
  12. Jonathan TanNov 23, 2021
  13. Glen ChooNov 24, 2021
  14. 4/4 branch: add --recurse-submodules option for branch creationGlen Choo, Nov 22, 2021
  15. Ævar Arnfjörð BjarmasonNov 23, 2021
  16. Glen ChooNov 23, 2021
  17. Philippe BlainNov 23, 2021
  18. Glen ChooNov 23, 2021
  19. Jonathan TanNov 24, 2021
  20. Glen ChooNov 24, 2021
  21. Jonathan TanNov 29, 2021
  22. 0/3 implement branch --recurse-submodulesGlen Choo, Dec 6, 2021
  23. 1/3 branch: move --set-upstream-to behavior to setup_tracking()Glen Choo, Dec 6, 2021
  24. Junio C HamanoDec 6, 2021
  25. Glen ChooDec 8, 2021
  26. Junio C HamanoDec 6, 2021
  27. Glen ChooDec 8, 2021
  28. 2/3 builtin/branch: clean up action-picking logic in cmd_branch()Glen Choo, Dec 6, 2021
  29. 3/3 branch: add --recurse-submodules option for branch creationGlen Choo, Dec 6, 2021
  30. 0/5 implement branch --recurse-submodulesGlen Choo, Dec 9, 2021
  31. 1/5 branch: move --set-upstream-to behavior to setup_tracking()Glen Choo, Dec 9, 2021
  32. Jonathan TanDec 9, 2021
  33. Glen ChooDec 9, 2021
  34. 2/5 branch: remove forward declaration of validate_branch_start()Glen Choo, Dec 9, 2021
  35. 3/5 builtin/branch: clean up action-picking logic in cmd_branch()Glen Choo, Dec 9, 2021
  36. Jonathan TanDec 9, 2021
  37. Glen ChooDec 9, 2021
  38. 4/5 branch: add --recurse-submodules option for branch creationGlen Choo, Dec 9, 2021
  39. Philippe BlainDec 11, 2021
  40. Glen ChooDec 14, 2021
  41. 5/5 branch.c: replace questionable exit() codesGlen Choo, Dec 9, 2021
  42. Ævar Arnfjörð BjarmasonDec 10, 2021
  43. Glen ChooDec 10, 2021
  44. Junio C HamanoDec 13, 2021
  45. Ævar Arnfjörð BjarmasonDec 13, 2021
  46. Junio C HamanoDec 13, 2021
  47. Jonathan TanDec 9, 2021
  48. Glen ChooDec 9, 2021
  49. Jonathan TanDec 13, 2021
  50. Glen ChooDec 14, 2021
  51. 0/5 implement branch --recurse-submodulesGlen Choo, Dec 16, 2021
  52. 1/5 branch: move --set-upstream-to behavior to dwim_and_setup_tracking()Glen Choo, Dec 16, 2021
  53. 2/5 branch: make create_branch() always create a branchGlen Choo, Dec 16, 2021
  54. 3/5 branch: add a dry_run parameter to create_branch()Glen Choo, Dec 16, 2021
  55. 4/5 builtin/branch: clean up action-picking logic in cmd_branch()Glen Choo, Dec 16, 2021
  56. 5/5 branch: add --recurse-submodules option for branch creationGlen Choo, Dec 16, 2021
  57. 0/5 implement branch --recurse-submodulesGlen Choo, Dec 16, 2021
  58. 1/5 branch: move --set-upstream-to behavior to dwim_and_setup_tracking()Glen Choo, Dec 16, 2021
  59. 2/5 branch: make create_branch() always create a branchGlen Choo, Dec 16, 2021
  60. 3/5 branch: add a dry_run parameter to create_branch()Glen Choo, Dec 16, 2021
  61. 4/5 builtin/branch: clean up action-picking logic in cmd_branch()Glen Choo, Dec 16, 2021
  62. 5/5 branch: add --recurse-submodules option for branch creationGlen Choo, Dec 16, 2021
  63. Junio C HamanoDec 17, 2021
  64. Junio C HamanoDec 17, 2021
  65. Glen ChooDec 20, 2021
  66. Junio C HamanoDec 20, 2021
  67. Glen ChooDec 20, 2021
  68. 0/5 implement branch --recurse-submodulesGlen Choo, Dec 20, 2021
  69. 1/5 branch: move --set-upstream-to behavior to dwim_and_setup_tracking()Glen Choo, Dec 20, 2021
  70. Jonathan TanJan 11, 2022
  71. Glen ChooJan 11, 2022
  72. Jonathan TanJan 11, 2022
  73. 2/5 branch: make create_branch() always create a branchGlen Choo, Dec 20, 2021
  74. Jonathan TanJan 11, 2022
  75. Glen ChooJan 11, 2022
  76. 3/5 branch: add a dry_run parameter to create_branch()Glen Choo, Dec 20, 2021
  77. 4/5 builtin/branch: clean up action-picking logic in cmd_branch()Glen Choo, Dec 20, 2021
  78. 5/5 branch: add --recurse-submodules option for branch creationGlen Choo, Dec 20, 2021
  79. Junio C HamanoDec 26, 2021
  80. Jonathan TanJan 11, 2022
  81. Glen ChooJan 11, 2022
  82. Jonathan TanJan 11, 2022
  83. Glen ChooJan 11, 2022
  84. Glen ChooDec 20, 2021
  85. Junio C HamanoDec 21, 2021
  86. Glen ChooDec 21, 2021
  87. 0/6 implement branch --recurse-submodulesGlen Choo, Jan 24, 2022
  88. 2/6 branch: make create_branch() always create a branchGlen Choo, Jan 24, 2022
  89. 1/6 branch: move --set-upstream-to behavior to dwim_and_setup_tracking()Glen Choo, Jan 24, 2022
  90. 3/6 branch: add a dry_run parameter to create_branch()Glen Choo, Jan 24, 2022
  91. 4/6 builtin/branch: consolidate action-picking logic in cmd_branch()Glen Choo, Jan 24, 2022
  92. 5/6 branch: add --recurse-submodules option for branch creationGlen Choo, Jan 24, 2022
  93. Jonathan TanJan 27, 2022
  94. Glen ChooJan 27, 2022
  95. Glen ChooJan 27, 2022
  96. 6/6 branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaksGlen Choo, Jan 24, 2022
  97. Junio C HamanoJan 27, 2022
  98. Glen ChooJan 28, 2022
  99. 0/6 implement branch --recurse-submodulesGlen Choo, Jan 29, 2022
  100. 1/6 branch: move --set-upstream-to behavior to dwim_and_setup_tracking()Glen Choo, Jan 29, 2022
  101. 2/6 branch: make create_branch() always create a branchGlen Choo, Jan 29, 2022
  102. Junio C HamanoFeb 1, 2022
  103. 3/6 branch: add a dry_run parameter to create_branch()Glen Choo, Jan 29, 2022
  104. 4/6 builtin/branch: consolidate action-picking logic in cmd_branch()Glen Choo, Jan 29, 2022
  105. 5/6 branch: add --recurse-submodules option for branch creationGlen Choo, Jan 29, 2022
  106. Glen ChooFeb 4, 2022
  107. Junio C HamanoFeb 4, 2022
  108. Glen ChooFeb 4, 2022
  109. 6/6 branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaksGlen Choo, Jan 29, 2022
  110. Jonathan TanFeb 1, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.