[RFC PATCH 3/5] branch: cleanup branch name validation

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Date: Sep 19, 2017, 07:15 UTC
Message-ID: <20170919071525.9404-4-kaarticsivaraam91196@gmail.com>
In-Reply-To: <20170919071525.9404-1-kaarticsivaraam91196@gmail.com>

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 updated. This doesn't bear the uncommunicative 'new'. Further replace the 'force' parameter with a 'could_exist' parameter which specifies whether the given branch name could exist or not (it's just a better name for 'force'). Also replace the 'attr_only' with 'clobber_head' which is a more communicative way of seeing "If the branch could exist, it's OK if it is the current branch".

Separate the validation of an existing branch 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@gmail.com>
---
 branch.c           | 41 ++++++++++++++++++++++++++++++-----------
 branch.h           | 20 ++++++++------------
 builtin/branch.c   |  2 +-
 builtin/checkout.c |  4 ++--
 t/t3200-branch.sh  |  4 ++++
 5 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/branch.c b/branch.c
index 703ded69c..2020dedf6 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_update(const char *name, struct strbuf *ref,
+			   int could_exist, int clobber_head)
 {
 	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 (!could_exist)
 		die(_("A branch named '%s' already exists."), ref->buf + strlen("refs/heads/"));
 
-	if (!attr_only) {
+	if (!clobber_head) {
 		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)) {
-		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_update(name, &ref, force, clobber_head);
 	}
 
 	real_ref = NULL;
diff --git a/branch.h b/branch.h
index 33b7f5d88..b4bfff84a 100644
--- a/branch.h
+++ b/branch.h
@@ -28,22 +28,18 @@ void create_branch(const char *name, const char *start_name,
 		   int clobber_head, 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. It returns the interpreted ref in ref.
  *
- * 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).
+ * could_exist indicates whether the branch could exist or not.
  *
- * NEEDSWORK: This needs to be split into two separate functions in the
- * longer run for sanity.
+ * if 'could_exist' is true, clobber_head indicates whether the branch could be the
+ * current branch; else 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_update(const char *name, struct strbuf *ref, int could_exist, int clobber_head);
 
 /*
  * Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index 355f9ef5d..27ddcad97 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_update(newname, &newref, force, clobber_head_ok);
 
 	reject_rebase_or_bisect_branch(oldref.buf);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 76859da9d..2e870ab4b 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;
 		int force = opts.new_branch_force != NULL;
 
-		opts.branch_exists = validate_new_branchname(opts.new_branch, &buf,
-							     force, force);
+		opts.branch_exists = validate_branch_update(opts.new_branch, &buf,
+							    force, 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.1006.g90ad9a07c