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@xxxxxxxxxx>
---
As Jonathan noted in v1, the diff is quite large. I could shrink this
by forward-declaring setup_tracking() so that the function definitions
are in the same order; let me know if that would be preferred.
branch.c | 195 ++++++++++++++++++++++++++--------------------
branch.h | 13 +++-
builtin/branch.c | 7 +-
t/t3200-branch.sh | 17 ++++
4 files changed, 139 insertions(+), 93 deletions(-)
diff --git a/branch.c b/branch.c
index 07a46430b3..a635a60f8b 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,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);
@@ -281,7 +246,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)
@@ -289,12 +255,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:
@@ -302,37 +268,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 81b5c111cb..19f2845e7a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -821,12 +821,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 8c5c1ccf33..f97cf495ab 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
