The function that validates a new branch name was clumsy because,
1. It did more than just validating the branch name.
2. Despite it's name, it is more often than not used to validate
existing branch names through the 'force' and 'attr_only'
parameters (whose names by the way weren't communicative).
3. The 'attr_only' parameter should have been "true" only when
callers like to update some attribute of the branch and "false"
when they were updating where the branch points to. It was misused
by callers by setting it to "true" (to skip tests) even though they
were force updating where the current branch was pointing to!
This is an example of spoiling code clarity by making the caller
rely on how the function is implemented thus making it hard to
modify/maintain.
This makes it unclear what the function does at all and would confuse
the people who would ever want to it for the first time.
So, refactor it into a function that validates whether the branch could
be created. This doesn't bear the uncommunicative 'new'. Further replace
the 'force' parameter with a 'shouldnt_exist' parameter which specifies
that a another branch with the given branch name should not (IOW, it's
just the opposite of 'force') Also replace the 'attr_only' with
'clobber_head_ok' which is a more communicative way of saying "If
another branch could exist, it's OK if it is the current branch".
Separate the validation of an existing branch (partly) into another function.
This (at last!) addresses the NEEDSWORK that was added in fa7993767
(branch --set-upstream: regression fix, 2011-09-16)
This refactor has only one functional change. It enforces strictly that
an existing branch should be updated only with the 'force' switch. So,
it's no more possible to do,
$ git branch -m master master
(which doesn't seem that useful). This strongly enforces the following
statement of the 'git branch' documentation,
"If <newbranch> exists, -M must be used to force the
rename to happen."
Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@xxxxxxxxx>
---
branch.c | 41 ++++++++++++++++++++++++++++++-----------
branch.h | 25 +++++++++++++------------
builtin/branch.c | 2 +-
builtin/checkout.c | 4 ++--
t/t3200-branch.sh | 4 ++++
5 files changed, 50 insertions(+), 26 deletions(-)
diff --git a/branch.c b/branch.c
index 0ea105b55..db2abeb7e 100644
--- a/branch.c
+++ b/branch.c
@@ -178,18 +178,19 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
return 0;
}
-int validate_new_branchname(const char *name, struct strbuf *ref,
- int force, int attr_only)
+int validate_branch_creation(const char *name, struct strbuf *ref,
+ int shouldnt_exist, int clobber_head_ok)
{
if (strbuf_check_branch_ref(ref, name))
die(_("'%s' is not a valid branch name."), name);
if (!ref_exists(ref->buf))
return 0;
- else if (!force && !attr_only)
+
+ if (shouldnt_exist)
die(_("A branch named '%s' already exists."), ref->buf + strlen("refs/heads/"));
- if (!attr_only) {
+ if (!clobber_head_ok) {
const char *head;
struct object_id oid;
@@ -197,9 +198,29 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
die(_("Cannot force update the current branch."));
}
+
return 1;
}
+/*
+ * Validates whether the branch with the given name exists, returning the
+ * interpreted ref in ref.
+ *
+ * This method is invoked if the caller merely wants to know if it is OK
+ * to change some attribute for the named branch (e.g. tracking upstream).
+ *
+ */
+static void validate_existing_branch(const char *name, struct strbuf *ref)
+{
+ if (strbuf_check_branch_ref(ref, name))
+ die(_("'%s' is not a valid branch name."), name);
+
+ if (ref_exists(ref->buf))
+ return;
+ else
+ die(_("branch '%s' doesn't exist"), name);
+}
+
static int check_tracking_branch(struct remote *remote, void *cb_data)
{
char *tracking_branch = cb_data;
@@ -243,13 +264,11 @@ void create_branch(const char *name, const char *start_name,
if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
explicit_tracking = 1;
- if (validate_new_branchname(name, &ref, force,
- track == BRANCH_TRACK_OVERRIDE ||
- clobber_head_ok)) {
- if (!force)
- dont_change_ref = 1;
- else
- forcing = 1;
+ if (track == BRANCH_TRACK_OVERRIDE) {
+ validate_existing_branch(name, &ref);
+ dont_change_ref = 1;
+ } else {
+ forcing = validate_branch_creation(name, &ref, !force, clobber_head_ok);
}
real_ref = NULL;
diff --git a/branch.h b/branch.h
index a6740bb9f..a6dde552c 100644
--- a/branch.h
+++ b/branch.h
@@ -28,22 +28,23 @@ void create_branch(const char *name, const char *start_name,
int reflog, int quiet, enum branch_track track);
/*
- * Validates that the requested branch may be created, returning the
- * interpreted ref in ref, force indicates whether (non-head) branches
- * may be overwritten. A non-zero return value indicates that the force
- * parameter was non-zero and the branch already exists.
+ * Validates whether the branch with the given name may be updated (created, renamed etc.,)
+ * with respect to the given conditions.
*
- * Contrary to all of the above, when attr_only is 1, the caller is
- * not interested in verifying if it is Ok to update the named
- * branch to point at a potentially different commit. It is merely
- * asking if it is OK to change some attribute for the named branch
- * (e.g. tracking upstream).
+ * - name is the new branch name
*
- * NEEDSWORK: This needs to be split into two separate functions in the
- * longer run for sanity.
+ * - ref contains the interpreted ref for the given name
+ *
+ * - shouldnt_exist indicates that another branch with the given name
+ * should not exist
+ *
+ * - clobber_head_ok allows another branch with given branch name to be
+ * the currently checkout branch; with 'shouldnt_exist', it has no effect.
+ *
+ * A non-zero return value indicates that a branch already exists and can be force updated.
*
*/
-int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);
+int validate_branch_creation(const char *name, struct strbuf *ref, int shouldnt_exist, int clobber_head_ok);
/*
* Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index 62c311478..aa2f36519 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -483,7 +483,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
*/
clobber_head_ok = !strcmp(oldname, newname);
- validate_new_branchname(newname, &newref, force, clobber_head_ok);
+ validate_branch_creation(newname, &newref, !force, clobber_head_ok);
reject_rebase_or_bisect_branch(oldref.buf);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4ef7a47e1..fcbbbb1fa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1283,8 +1283,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
struct strbuf buf = STRBUF_INIT;
opts.branch_exists =
- validate_new_branchname(opts.new_branch, &buf,
- !!opts.new_branch_force,
+ validate_branch_creation(opts.new_branch, &buf,
+ !opts.new_branch_force,
!!opts.new_branch_force);
strbuf_release(&buf);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index d97164997..ec85cd959 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -137,6 +137,10 @@ test_expect_success 'git branch -m -f o/q o/p should work when o/p exists' '
git branch -m -f o/q o/p
'
+test_expect_success 'git branch -m o/o o/o should fail when o/o exists' '
+ test_must_fail git branch -m o/o o/o
+'
+
test_expect_success 'git branch -m q r/q should fail when r exists' '
git branch q &&
git branch r &&
--
2.14.1.935.ge2b2bcd8a
