[RFC PATCH v3 0/4] give more useful error messages while renaming branch

[RFC PATCH v3 0/4] give more useful error messages while renaming branch

[Kaartic Sivaraam]

In builtin/branch, the error messages weren't handled directly by the branch
renaming function and was left to the other function. Though this avoids
redundancy this gave unclear error messages in some cases. So, make builtin/branch
give more useful error messages.

Changes in v3:

	Incorporated suggestions from v2 to improve code and commit message. To
	be more precise about the code part,

	In 2/4 slightly re-ordered the parameters to move the flag parameters to
	the end.

	In 3/4, changed the return type of the branchname validation functions to
	be the enum (whose values they return) as suggested by Stefan.

	Dropped the PATCH 3/5 of v2 as there was another series[1] that did the
	refactor and got merged to 'next'. I have now re-rolled the series over
	'next' [pointing at 273055501 (Sync with master, 2017-10-24)].
 
	This has made the code in 3/4 a little clumsy (at least to me) as I
	tried to achieve to achieve what the previous patches did with the new
	validate*_branchname functionS. Let me know, if it looks too bad.

So this could go on top of 'next' without any conflicts but in case I
missed something, let me know. The series could be found in my fork[2].


Any feedback welcome.

Thanks,
Kaartic

[1] : https://public-inbox.org/git/20171013051132.3973-1-gitster@pobox.com

[2] : https://github.com/sivaraam/git/tree/work/branch-revamp


Kaartic Sivaraam (4):
  branch: improve documentation and naming of 'create_branch()'
  branch: re-order function arguments to group related arguments
  branch: introduce dont_fail parameter for branchname validation
  builtin/branch: give more useful error messages when renaming

 branch.c           | 63 ++++++++++++++++++++++++++++++------------------------
 branch.h           | 57 ++++++++++++++++++++++++++++++++++++++----------
 builtin/branch.c   | 49 ++++++++++++++++++++++++++++++++++--------
 builtin/checkout.c | 11 +++++-----
 4 files changed, 127 insertions(+), 53 deletions(-)

-- 
2.15.0.rc2.401.g3db9995f9

[RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()'

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

The documentation for 'create_branch()' was incomplete as it didn't say
what certain parameters were used for. Further a parameter name wasn't
very communicative.

So, add missing documentation for the sake of completeness and easy
reference. Also, rename the concerned parameter to make its name more
communicative.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 branch.c | 4 ++--
 branch.h | 7 ++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index fe1e1c367..ea6e2b359 100644
--- a/branch.c
+++ b/branch.c
@@ -244,7 +244,7 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-		   int force, int reflog, int clobber_head,
+		   int force, int reflog, int clobber_head_ok,
 		   int quiet, enum branch_track track)
 {
 	struct commit *commit;
@@ -258,7 +258,7 @@ void create_branch(const char *name, const char *start_name,
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;
 
-	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
+	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
 	    ? validate_branchname(name, &ref)
 	    : validate_new_branchname(name, &ref, force)) {
 		if (!force)
diff --git a/branch.h b/branch.h
index be5e5d130..1512b78d1 100644
--- a/branch.h
+++ b/branch.h
@@ -15,12 +15,17 @@
  *
  *   - reflog creates a reflog for the branch
  *
+ *   - clobber_head_ok allows the currently checked out (hence existing)
+ *     branch to be overwritten; without 'force', it has no effect.
+ *
+ *   - quiet suppresses tracking information
+ *
  *   - track causes the new branch to be configured to merge the remote branch
  *     that start_name is a tracking branch for (if any).
  */
 void create_branch(const char *name, const char *start_name,
 		   int force, int reflog,
-		   int clobber_head, int quiet, enum branch_track track);
+		   int clobber_head_ok, int quiet, enum branch_track track);
 
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
-- 
2.15.0.461.gf957c703b.dirty

[RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

The ad-hoc patches to add new arguments to a function when needed
resulted in the related arguments not being close to each other.
This misleads the person reading the code to believe that there isn't
much relation between those arguments while it's not the case.

So, re-order the arguments to keep the related arguments close to each
other.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 branch.c           |  4 ++--
 branch.h           | 14 +++++++-------
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  6 +++---
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/branch.c b/branch.c
index ea6e2b359..7c8093041 100644
--- a/branch.c
+++ b/branch.c
@@ -244,8 +244,8 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-		   int force, int reflog, int clobber_head_ok,
-		   int quiet, enum branch_track track)
+		   enum branch_track track, int force, int clobber_head_ok,
+		   int reflog, int quiet)
 {
 	struct commit *commit;
 	struct object_id oid;
diff --git a/branch.h b/branch.h
index 1512b78d1..85052628b 100644
--- a/branch.h
+++ b/branch.h
@@ -11,21 +11,21 @@
  *   - start_name is the name of the existing branch that the new branch should
  *     start from
  *
- *   - force enables overwriting an existing (non-head) branch
+ *   - track causes the new branch to be configured to merge the remote branch
+ *     that start_name is a tracking branch for (if any).
  *
- *   - reflog creates a reflog for the branch
+ *   - 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.
  *
- *   - quiet suppresses tracking information
+ *   - reflog creates a reflog for the branch
  *
- *   - track causes the new branch to be configured to merge the remote branch
- *     that start_name is a tracking branch for (if any).
+ *   - quiet suppresses tracking information
  */
 void create_branch(const char *name, const char *start_name,
-		   int force, int reflog,
-		   int clobber_head_ok, int quiet, enum branch_track track);
+		   enum branch_track track, int force, int clobber_head_ok,
+		   int reflog, int quiet);
 
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
diff --git a/builtin/branch.c b/builtin/branch.c
index 5be40b384..df06ac968 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -766,7 +766,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		 * create_branch takes care of setting up the tracking
 		 * info and making sure new_upstream is correct
 		 */
-		create_branch(branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+		create_branch(branch->name, new_upstream, BRANCH_TRACK_OVERRIDE, 0, 0, 0, quiet);
 	} else if (unset_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 		struct strbuf buf = STRBUF_INIT;
@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
 
 		create_branch(argv[0], (argc == 2) ? argv[1] : head,
-			      force, reflog, 0, quiet, track);
+			      track, force, 0, reflog, quiet);
 
 	} else
 		usage_with_options(builtin_branch_usage, options);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8546d630b..5c34a9a0d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -639,11 +639,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 		}
 		else
 			create_branch(opts->new_branch, new->name,
+				      opts->track,
+				      opts->new_branch_force ? 1 : 0,
 				      opts->new_branch_force ? 1 : 0,
 				      opts->new_branch_log,
-				      opts->new_branch_force ? 1 : 0,
-				      opts->quiet,
-				      opts->track);
+				      opts->quiet);
 		new->name = opts->new_branch;
 		setup_branch_path(new);
 	}
-- 
2.15.0.461.gf957c703b.dirty

[RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

This parameter allows the branchname validation functions to
optionally return a flag specifying the reason for failure, when
requested. This allows the caller to know why it was about to die.
This allows more useful error messages to be given to the user when
trying to rename a branch.

The flags are specified in the form of an enum and values for success
flags have been assigned explicitly to clearly express that certain
callers rely on those values and they cannot be arbitrary.

Only the logic has been added but no caller has been made to use it, yet.
So, no functional changes.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 branch.c           | 62 +++++++++++++++++++++++++++++++-----------------------
 branch.h           | 60 ++++++++++++++++++++++++++++++++++++++++++----------
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  5 +++--
 4 files changed, 90 insertions(+), 41 deletions(-)

diff --git a/branch.c b/branch.c
index 7c8093041..7db2e3296 100644
--- a/branch.c
+++ b/branch.c
@@ -178,41 +178,51 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
 	return 0;
 }
 
-/*
- * Check if 'name' can be a valid name for a branch; die otherwise.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_branchname(const char *name, struct strbuf *ref)
+enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail)
 {
-	if (strbuf_check_branch_ref(ref, name))
-		die(_("'%s' is not a valid branch name."), name);
+	if (strbuf_check_branch_ref(ref, name)) {
+		if (dont_fail)
+			return INVALID_BRANCH_NAME;
+		else
+			die(_("'%s' is not a valid branch name."), name);
+	}
 
-	return ref_exists(ref->buf);
+	if(ref_exists(ref->buf))
+		return BRANCH_EXISTS;
+	else
+		return BRANCH_DOESNT_EXIST;
 }
 
-/*
- * Check if a branch 'name' can be created as a new branch; die otherwise.
- * 'force' can be used when it is OK for the named branch already exists.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail)
 {
 	const char *head;
 
-	if (!validate_branchname(name, ref))
-		return 0;
+	if(dont_fail) {
+		enum branch_validation_result res = validate_branchname(name, ref, 1);
+		if (res == INVALID_BRANCH_NAME || res == BRANCH_DOESNT_EXIST)
+				return res;
+	} else {
+		if(validate_branchname(name, ref, 0) == BRANCH_DOESNT_EXIST)
+			return BRANCH_DOESNT_EXIST;
+	}
 
-	if (!force)
-		die(_("A branch named '%s' already exists."),
-		    ref->buf + strlen("refs/heads/"));
+	if (!force) {
+		if (dont_fail)
+			return BRANCH_EXISTS_NO_FORCE;
+		else
+			die(_("A branch named '%s' already exists."),
+				ref->buf + strlen("refs/heads/"));
+	}
 
 	head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-		die(_("Cannot force update the current branch."));
+	if (!is_bare_repository() && head && !strcmp(head, ref->buf)) {
+		if (dont_fail)
+			return CANNOT_FORCE_UPDATE_CURRENT_BRANCH;
+		else
+			die(_("Cannot force update the current branch."));
+	}
 
-	return 1;
+	return BRANCH_EXISTS;
 }
 
 static int check_tracking_branch(struct remote *remote, void *cb_data)
@@ -259,8 +269,8 @@ void create_branch(const char *name, const char *start_name,
 		explicit_tracking = 1;
 
 	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
-	    ? validate_branchname(name, &ref)
-	    : validate_new_branchname(name, &ref, force)) {
+	    ? validate_branchname(name, &ref, 0)
+	    : validate_new_branchname(name, &ref, force, 0)) {
 		if (!force)
 			dont_change_ref = 1;
 		else
diff --git a/branch.h b/branch.h
index 85052628b..0c178ec5a 100644
--- a/branch.h
+++ b/branch.h
@@ -27,20 +27,58 @@ void create_branch(const char *name, const char *start_name,
 		   enum branch_track track, int force, int clobber_head_ok,
 		   int reflog, int quiet);
 
-/*
- * Check if 'name' can be a valid name for a branch; die otherwise.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-extern int validate_branchname(const char *name, struct strbuf *ref);
+enum branch_validation_result {
+	/* Flags that convey that validation FAILED */
+	BRANCH_EXISTS_NO_FORCE = -3,
+	CANNOT_FORCE_UPDATE_CURRENT_BRANCH,
+	INVALID_BRANCH_NAME,
+	/* Flags that convey that validation SUCCEEDED */
+	BRANCH_DOESNT_EXIST = 0,
+	BRANCH_EXISTS = 1,
+};
 
 /*
- * Check if a branch 'name' can be created as a new branch; die otherwise.
- * 'force' can be used when it is OK for the named branch already exists.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
+ * Check if 'name' can be a valid name for a branch; die otherwise.
+ *
+ *   - name is the new branch name
+ *
+ *   - ref is used to return the full refname for the branch
+ *
+ * The return values have the following meaning,
+ *
+ *   - If dont_fail is 0, the function dies in case of failure and returns flags of
+ *     'branch_validation_result' that indicate status of the given branch. The positive
+ *     non-zero flag implies that the branch exists.
+ *
+ *   - If dont_fail is 1, the function doesn't die in case of failure but returns flags
+ *     of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of
+ *     success is same as above.
+ *
  */
-extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
+extern enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail);
+
+/*
+ * Check if a branch 'name' can be created as a new branch.
+ *
+ *   - name is the new branch name
+ *
+ *   - ref is used to return the full refname for the branch
+ *
+ *   - force can be used when it is OK if the named branch already exists.
+ *     the currently checkout branch; with 'shouldnt_exist', it has no effect.
+ *
+ * The return values have the following meaning,
+ *
+ *   - If dont_fail is 0, the function dies in case of failure and returns flags of
+ *     'branch_validation_result' that indicate that convey status of given branch. The positive
+ *     non-zero flag implies that the branch can be force updated.
+ *
+ *   - If dont_fail is 1, the function doesn't die in case of failure but returns flags
+ *     of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of
+ *     success is same as above.
+ *
+ */
+extern enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail);
 
 /*
  * Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index df06ac968..7018e5d75 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -487,9 +487,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
 	 */
 	if (!strcmp(oldname, newname))
-		validate_branchname(newname, &newref);
+		validate_branchname(newname, &newref, 0);
 	else
-		validate_new_branchname(newname, &newref, force);
+		validate_new_branchname(newname, &newref, force, 0);
 
 	reject_rebase_or_bisect_branch(oldref.buf);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5c34a9a0d..4adab3814 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1288,10 +1288,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		struct strbuf buf = STRBUF_INIT;
 
 		if (opts.new_branch_force)
-			opts.branch_exists = validate_branchname(opts.new_branch, &buf);
+			opts.branch_exists = validate_branchname(opts.new_branch, &buf, 0);
 		else
 			opts.branch_exists =
-				validate_new_branchname(opts.new_branch, &buf, 0);
+				validate_new_branchname(opts.new_branch, &buf, 0, 0);
+
 		strbuf_release(&buf);
 	}
 
-- 
2.15.0.461.gf957c703b.dirty

[RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

When trying to rename an inexistent branch to with a name of a branch
that already exists the rename failed specifying the new branch name
exists rather than specifying that the branch trying to be renamed
doesn't exist.

    $ git branch -m tset master
    fatal: A branch named 'master' already exists.

It's conventional to report that 'tset' doesn't exist rather than
reporting that 'master' exists, the same way the 'mv' command does.

    (hypothetical)
    $ git branch -m tset master
    fatal: branch 'tset' doesn't exist.

That has the problem that the error about an existing branch is shown
only after the user corrects the error about inexistent branch.

    $ git branch -m test master
    fatal: A branch named 'master' already exists.

This isn't useful either because the user would have corrected this error in
a single go if he had been told this alongside the first error. So, give
more useful error messages by giving errors about old branch name and new
branch name at the same time. This is possible as the branch name validation
functions now return the reason they were about to die, when requested.

    $ git branch -m tset master
    fatal: branch 'tset' doesn't exist, and branch 'master' already exists

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 builtin/branch.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7018e5d75..c2bbf8c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char *target)
 	free_worktrees(worktrees);
 }
 
+static void get_error_msg(struct strbuf* error_msg, const char* oldname, unsigned old_branch_exists,
+			  const char* newname, enum branch_validation_result res)
+{
+	const char* connector_string = _(", and ");
+
+	if (!old_branch_exists) {
+		strbuf_addf(error_msg, _("branch '%s' doesn't exist"), oldname);
+	}
+
+	switch (res) {
+		case BRANCH_EXISTS_NO_FORCE:
+			strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : "");
+			strbuf_addf(error_msg,_("branch '%s' already exists"), newname);
+			break;
+		case CANNOT_FORCE_UPDATE_CURRENT_BRANCH:
+			strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : "");
+			strbuf_addstr(error_msg, _("cannot force update the current branch"));
+			break;
+		case INVALID_BRANCH_NAME:
+			strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : "");
+			strbuf_addf(error_msg, _("branch name '%s' is invalid"), newname);
+			break;
+		/* not necessary to handle success cases */
+		case BRANCH_EXISTS:
+		case BRANCH_DOESNT_EXIST:
+			break;
+	}
+}
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
 	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
 	int recovery = 0;
+	struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
+	enum branch_validation_result res;
 
 	if (!oldname) {
 		if (copy)
@@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			die(_("cannot rename the current branch while not on any."));
 	}
 
-	if (strbuf_check_branch_ref(&oldref, oldname)) {
+	if (strbuf_check_branch_ref(&oldref, oldname) && ref_exists(oldref.buf))
+	{
 		/*
 		 * Bad name --- this could be an attempt to rename a
 		 * ref that we used to allow to be created by accident.
 		 */
-		if (ref_exists(oldref.buf))
-			recovery = 1;
-		else
-			die(_("Invalid branch name: '%s'"), oldname);
+		recovery = 1;
 	}
 
 	/*
@@ -487,9 +516,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
 	 */
 	if (!strcmp(oldname, newname))
-		validate_branchname(newname, &newref, 0);
+		res = validate_branchname(newname, &newref, 1);
 	else
-		validate_new_branchname(newname, &newref, force, 0);
+		res = validate_new_branchname(newname, &newref, force, 1);
+
+	get_error_msg(&error_msg, oldname, ref_exists(oldref.buf), newname, res);
+	if (strbuf_cmp(&error_msg, &empty))
+		die("%s", error_msg.buf);
 
 	reject_rebase_or_bisect_branch(oldref.buf);
 
@@ -530,6 +563,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 		die(_("Branch is copied, but update of config-file failed"));
 	strbuf_release(&oldsection);
 	strbuf_release(&newsection);
+	strbuf_release(&error_msg);
+	strbuf_release(&empty);
 }
 
 static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION")
-- 
2.15.0.461.gf957c703b.dirty

Re: [RFC PATCH v3 0/4] give more useful error messages while renaming branch

[Kaartic Sivaraam]

Sorry, ignore this mails. I actually have re-sent it to the correct
thread.
 
-- 
Kaartic

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:24 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> We usually use the word "gently" to when we enhance an operation
> that used to always die on failure.  When there are tons of callsite
> to the original operation F(), we introduce F_gently() variant and
> do something like
> 
> 	F(...)
> 	{
> 		if (F_gently(...))
> 			die(...);
> 	}
> 
> so that the callers do not have to change.  If there aren't that
> many, it is OK to change the function signature of F() to tell it
> not to die without adding a new F_gently() function, which is the
> approach more appropriate for this change.  The extra parameter used
> for that purpose should be called "gently", perhaps.
> 

Good suggestion, wasn't aware of it before. Renamed it.


> > +	if(ref_exists(ref->buf))
> > +		return BRANCH_EXISTS;
> > +	else
> > +		return BRANCH_DOESNT_EXIST;
> 
> Always have SP between "if" (and other keyword like "while") and its
> condition.
>
> For this one, however, this might be easier to follow:
> 
> 	return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST;
> 

Done.


> The names of the enum values may need further thought.  They must
> clearly convey two things, in addition to what kind of status they
> represent; the current names only convey the status.  From the names
> of these values, it must be clear that they are part of the same
> enum (e.g. by sharing a common prefix), and also from the names of
> these values, it must be clear which ones are error conditions and
> which are not, without knowing their actual values.  A reader cannot
> immediately tell from "BRANCH_EXISTS" if that is a good thing or
> not.
> 

I've added the prefix of "VALIDATION_{FAIL,PASS}" as appropriate to the
enum values. This made the names to have the form,

    VALIDATION_(kind of status)_(reason)

This made the names a bit long but I couldn't come up with a better
prefix for now. Any suggestions are welcome.


Thanks for the detailed review!

---
Kaartic

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> This parameter allows the branchname validation functions to
> optionally return a flag specifying the reason for failure, when
> requested. This allows the caller to know why it was about to die.
> This allows more useful error messages to be given to the user when
> trying to rename a branch.
>
> The flags are specified in the form of an enum and values for success
> flags have been assigned explicitly to clearly express that certain
> callers rely on those values and they cannot be arbitrary.
>
> Only the logic has been added but no caller has been made to use it, yet.
> So, no functional changes.

This step makes sense, and nicely done.

We usually use the word "gently" to when we enhance an operation
that used to always die on failure.  When there are tons of callsite
to the original operation F(), we introduce F_gently() variant and
do something like

	F(...)
	{
		if (F_gently(...))
			die(...);
	}

so that the callers do not have to change.  If there aren't that
many, it is OK to change the function signature of F() to tell it
not to die without adding a new F_gently() function, which is the
approach more appropriate for this change.  The extra parameter used
for that purpose should be called "gently", perhaps.

> +	if(ref_exists(ref->buf))
> +		return BRANCH_EXISTS;
> +	else
> +		return BRANCH_DOESNT_EXIST;

Always have SP between "if" (and other keyword like "while") and its
condition.

For this one, however, this might be easier to follow:

	return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST;

The names of the enum values may need further thought.  They must
clearly convey two things, in addition to what kind of status they
represent; the current names only convey the status.  From the names
of these values, it must be clear that they are part of the same
enum (e.g. by sharing a common prefix), and also from the names of
these values, it must be clear which ones are error conditions and
which are not, without knowing their actual values.  A reader cannot
immediately tell from "BRANCH_EXISTS" if that is a good thing or
not.

Other than that, looks fairly cleanly done.  I like what this step
wants to achieve.

Thanks.

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

On Friday 03 November 2017 12:12 AM, Stefan Beller wrote:
> On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>> On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote:
>>>
>>> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
>>> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>>
>>
>> I just now saw this small glitch as a consequence of recently
>> changing my email address. I would prefer to keep the second one
>> but as the other patches have the first one it's better to keep
>> the first one for now.
> 
> If you prefer one of them, you may have incentive to
> add an entry into .mailmap file, otherwise I'd kindly ask you
> to. :) (c.f. `git log --no-merges -- .mailmap`)
> 

Sure, I'll do that. My intuition says this doesn't remove the duplicated 
  sign-off line. Anyways, there's for sure a v4 that's going to update 
the connector string in [4/4] and another update. I'll be careful to 
address these issues in v4.

---
Kaartic

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Stefan Beller]

On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote:
>>
>> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
>> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>
>
> I just now saw this small glitch as a consequence of recently
> changing my email address. I would prefer to keep the second one
> but as the other patches have the first one it's better to keep
> the first one for now.

If you prefer one of them, you may have incentive to
add an entry into .mailmap file, otherwise I'd kindly ask you
to. :) (c.f. `git log --no-merges -- .mailmap`)

> But wait, it seems that this commit also has a different author
> identity (the email adress part). If this is a big enough issue,
> I'll fix that and send a v4 (possibly with any other suggested
> changes) else I'll leave it as it is.
>
> BTW, I added the Ccs to this mail which I forgot to do when
> sending the patches, hope it's not an issue.

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote:
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>

I just now saw this small glitch as a consequence of recently
changing my email address. I would prefer to keep the second one
but as the other patches have the first one it's better to keep
the first one for now.

But wait, it seems that this commit also has a different author
identity (the email adress part). If this is a big enough issue,
I'll fix that and send a v4 (possibly with any other suggested
changes) else I'll leave it as it is.

BTW, I added the Ccs to this mail which I forgot to do when
sending the patches, hope it's not an issue.

---
Kaartic

[RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

This parameter allows the branchname validation functions to
optionally return a flag specifying the reason for failure, when
requested. This allows the caller to know why it was about to die.
This allows more useful error messages to be given to the user when
trying to rename a branch.

The flags are specified in the form of an enum and values for success
flags have been assigned explicitly to clearly express that certain
callers rely on those values and they cannot be arbitrary.

Only the logic has been added but no caller has been made to use it, yet.
So, no functional changes.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 branch.c           | 62 +++++++++++++++++++++++++++++++-----------------------
 branch.h           | 60 ++++++++++++++++++++++++++++++++++++++++++----------
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  5 +++--
 4 files changed, 90 insertions(+), 41 deletions(-)

diff --git a/branch.c b/branch.c
index 7c8093041..7db2e3296 100644
--- a/branch.c
+++ b/branch.c
@@ -178,41 +178,51 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
 	return 0;
 }
 
-/*
- * Check if 'name' can be a valid name for a branch; die otherwise.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_branchname(const char *name, struct strbuf *ref)
+enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail)
 {
-	if (strbuf_check_branch_ref(ref, name))
-		die(_("'%s' is not a valid branch name."), name);
+	if (strbuf_check_branch_ref(ref, name)) {
+		if (dont_fail)
+			return INVALID_BRANCH_NAME;
+		else
+			die(_("'%s' is not a valid branch name."), name);
+	}
 
-	return ref_exists(ref->buf);
+	if(ref_exists(ref->buf))
+		return BRANCH_EXISTS;
+	else
+		return BRANCH_DOESNT_EXIST;
 }
 
-/*
- * Check if a branch 'name' can be created as a new branch; die otherwise.
- * 'force' can be used when it is OK for the named branch already exists.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail)
 {
 	const char *head;
 
-	if (!validate_branchname(name, ref))
-		return 0;
+	if(dont_fail) {
+		enum branch_validation_result res = validate_branchname(name, ref, 1);
+		if (res == INVALID_BRANCH_NAME || res == BRANCH_DOESNT_EXIST)
+				return res;
+	} else {
+		if(validate_branchname(name, ref, 0) == BRANCH_DOESNT_EXIST)
+			return BRANCH_DOESNT_EXIST;
+	}
 
-	if (!force)
-		die(_("A branch named '%s' already exists."),
-		    ref->buf + strlen("refs/heads/"));
+	if (!force) {
+		if (dont_fail)
+			return BRANCH_EXISTS_NO_FORCE;
+		else
+			die(_("A branch named '%s' already exists."),
+				ref->buf + strlen("refs/heads/"));
+	}
 
 	head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-		die(_("Cannot force update the current branch."));
+	if (!is_bare_repository() && head && !strcmp(head, ref->buf)) {
+		if (dont_fail)
+			return CANNOT_FORCE_UPDATE_CURRENT_BRANCH;
+		else
+			die(_("Cannot force update the current branch."));
+	}
 
-	return 1;
+	return BRANCH_EXISTS;
 }
 
 static int check_tracking_branch(struct remote *remote, void *cb_data)
@@ -259,8 +269,8 @@ void create_branch(const char *name, const char *start_name,
 		explicit_tracking = 1;
 
 	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
-	    ? validate_branchname(name, &ref)
-	    : validate_new_branchname(name, &ref, force)) {
+	    ? validate_branchname(name, &ref, 0)
+	    : validate_new_branchname(name, &ref, force, 0)) {
 		if (!force)
 			dont_change_ref = 1;
 		else
diff --git a/branch.h b/branch.h
index 85052628b..0c178ec5a 100644
--- a/branch.h
+++ b/branch.h
@@ -27,20 +27,58 @@ void create_branch(const char *name, const char *start_name,
 		   enum branch_track track, int force, int clobber_head_ok,
 		   int reflog, int quiet);
 
-/*
- * Check if 'name' can be a valid name for a branch; die otherwise.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-extern int validate_branchname(const char *name, struct strbuf *ref);
+enum branch_validation_result {
+	/* Flags that convey that validation FAILED */
+	BRANCH_EXISTS_NO_FORCE = -3,
+	CANNOT_FORCE_UPDATE_CURRENT_BRANCH,
+	INVALID_BRANCH_NAME,
+	/* Flags that convey that validation SUCCEEDED */
+	BRANCH_DOESNT_EXIST = 0,
+	BRANCH_EXISTS = 1,
+};
 
 /*
- * Check if a branch 'name' can be created as a new branch; die otherwise.
- * 'force' can be used when it is OK for the named branch already exists.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
+ * Check if 'name' can be a valid name for a branch; die otherwise.
+ *
+ *   - name is the new branch name
+ *
+ *   - ref is used to return the full refname for the branch
+ *
+ * The return values have the following meaning,
+ *
+ *   - If dont_fail is 0, the function dies in case of failure and returns flags of
+ *     'branch_validation_result' that indicate status of the given branch. The positive
+ *     non-zero flag implies that the branch exists.
+ *
+ *   - If dont_fail is 1, the function doesn't die in case of failure but returns flags
+ *     of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of
+ *     success is same as above.
+ *
  */
-extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
+extern enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail);
+
+/*
+ * Check if a branch 'name' can be created as a new branch.
+ *
+ *   - name is the new branch name
+ *
+ *   - ref is used to return the full refname for the branch
+ *
+ *   - force can be used when it is OK if the named branch already exists.
+ *     the currently checkout branch; with 'shouldnt_exist', it has no effect.
+ *
+ * The return values have the following meaning,
+ *
+ *   - If dont_fail is 0, the function dies in case of failure and returns flags of
+ *     'branch_validation_result' that indicate that convey status of given branch. The positive
+ *     non-zero flag implies that the branch can be force updated.
+ *
+ *   - If dont_fail is 1, the function doesn't die in case of failure but returns flags
+ *     of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of
+ *     success is same as above.
+ *
+ */
+extern enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail);
 
 /*
  * Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index df06ac968..7018e5d75 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -487,9 +487,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
 	 */
 	if (!strcmp(oldname, newname))
-		validate_branchname(newname, &newref);
+		validate_branchname(newname, &newref, 0);
 	else
-		validate_new_branchname(newname, &newref, force);
+		validate_new_branchname(newname, &newref, force, 0);
 
 	reject_rebase_or_bisect_branch(oldref.buf);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5c34a9a0d..4adab3814 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1288,10 +1288,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		struct strbuf buf = STRBUF_INIT;
 
 		if (opts.new_branch_force)
-			opts.branch_exists = validate_branchname(opts.new_branch, &buf);
+			opts.branch_exists = validate_branchname(opts.new_branch, &buf, 0);
 		else
 			opts.branch_exists =
-				validate_new_branchname(opts.new_branch, &buf, 0);
+				validate_new_branchname(opts.new_branch, &buf, 0, 0);
+
 		strbuf_release(&buf);
 	}
 
-- 
2.15.0.461.gf957c703b.dirty