[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

[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

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 2/4] branch: re-order function arguments to group related arguments

[Junio C Hamano]

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

> 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.

I do not get what this wants to say.  "I am sending this ad-hoc
patch that scrambles the order of parameters for no real reason" is
certainly not how you are selling this step.

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

This sentence (without "So,", obviously, because I do not get the
previous paragraph) I do understand and agree with as a goal.

I think the only two things that should be kept together are "force"
and "clobber_head_ok" because the previous 1/4 changed the meaning
of "clobber_head" to "it is OK if I am renaming the currently
checked-out branch", i.e. closer to what "force" means.

I certainly would not mind the order used in the result of this
patch (in other words, if somebody posted a patch to add
create_branch() function to the codebase that lacked it, with its
parameters listed in the order this patch uses, I wouldn't
complain), but it would have equally been OK if "reflog" and "force"
were swapped without making any other change this patch makes.

I dunno.

> 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);
>  	}

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

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:06 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> > 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.
> 
> I do not get what this wants to say.  "I am sending this ad-hoc
> patch that scrambles the order of parameters for no real reason" is
> certainly not how you are selling this step.
> 
> > So, re-order the arguments to keep the related arguments close to each
> > other.
> 
> This sentence (without "So,", obviously, because I do not get the
> previous paragraph) I do understand and agree with as a goal.
> 

I've tried to improve it, does the following paragraph sound clear
enough?

    branch: group related arguments of create_branch()
        
    New arguments were added to create_branch() whenever the need
    arised and they were added to tail of the argument list. This
    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 is
    not the case.
        
    So, re-order the arguments to keep the related arguments close
    to each other.


> I think the only two things that should be kept together are "force"
> and "clobber_head_ok" because the previous 1/4 changed the meaning
> of "clobber_head" to "it is OK if I am renaming the currently
> checked-out branch", i.e. closer to what "force" means.
> 
> I certainly would not mind the order used in the result of this
> patch (in other words, if somebody posted a patch to add
> create_branch() function to the codebase that lacked it, with its
> parameters listed in the order this patch uses, I wouldn't
> complain), but it would have equally been OK if "reflog" and "force"
> were swapped without making any other change this patch makes.
> 

Makes sense. The unwanted shuffling was a consequence of my attempt to
see if the signature of the function did change when the position of
the 'enum' was changed. It seems there isn't change in its signature as
it is possible to use integers for enums and vice versa due to liberal
checking for misuses.

I've changed the signature back to keep alone "force" and
"clobber_head_ok" together.


Thanks,
Kaartic

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

[Junio C Hamano]

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

> I've tried to improve it, does the following paragraph sound clear
> enough?
>
>     branch: group related arguments of create_branch()
>         
>     New arguments were added to create_branch() whenever the need
>     arised and they were added to tail of the argument list. This
>     resulted in the related arguments not being close to each other.

OK, I understand what you wanted to say.  But I do not think that is
based on a true history.

 - f9a482e6 ("checkout: suppress tracking message with "-q"",
   2012-03-26) adds 'quiet' just after 'clobber_head', exactly
   because they are related, and leaves 'track' at the end.

 - 39bd6f72 ("Allow checkout -B <current-branch> to update the
   current branch", 2011-11-26) adds 'clobber_head' not at the end but
   before 'track', which is left at the end.  

 - c847f537 ("Detached HEAD (experimental)", 2007-01-01) split 'start'
   into 'start_name' and 'start_sha1' (the latter was laster removed)
   and this was not a mindless "add at the end", either.

 - 0746d19a ("git-branch, git-checkout: autosetup for remote branch
   tracking", 2007-03-08) did add track at the end, but that is
   justifiable, as it has no relation to any other parameter.

You could call 39bd6f72 somewhat questionable as 'clobber_head' is
related to 'force' more strongly than it is to 'reflog' [*1*], but
it is unfair to blame anything else having done a mindless "add at
the end".



[Footnote]

*1* I actually think the commit added 'clobber_head' because for the
    purpose of what the commit did, it closely was related to 'track',
    turning <force, reflog, track> into <force, reflog, clobber, track>.
    Arguably, it could have done <force, clobber, track, reflog> instead.

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

[Kaartic Sivaraam]

On Mon, 2017-11-13 at 11:32 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> > I've tried to improve it, does the following paragraph sound clear
> > enough?
> > 
> >     branch: group related arguments of create_branch()
> >         
> >     New arguments were added to create_branch() whenever the need
> >     arised and they were added to tail of the argument list. This
> >     resulted in the related arguments not being close to each other.
> 
> OK, I understand what you wanted to say.  But I do not think that is
> based on a true history.
> 
>  - f9a482e6 ("checkout: suppress tracking message with "-q"",
>    2012-03-26) adds 'quiet' just after 'clobber_head', exactly
>    because they are related, and leaves 'track' at the end.
> 
>  - 39bd6f72 ("Allow checkout -B <current-branch> to update the
>    current branch", 2011-11-26) adds 'clobber_head' not at the end but
>    before 'track', which is left at the end.  
> 
>  - c847f537 ("Detached HEAD (experimental)", 2007-01-01) split 'start'
>    into 'start_name' and 'start_sha1' (the latter was laster removed)
>    and this was not a mindless "add at the end", either.
> 
>  - 0746d19a ("git-branch, git-checkout: autosetup for remote branch
>    tracking", 2007-03-08) did add track at the end, but that is
>    justifiable, as it has no relation to any other parameter.
> 

Seems I wasn't careful enough in noticing how the arguments were added.
I seemed to have overlooked the fact that 39bd6f72 added 'clobber_head'
"before" track which resulted in the vague commit message. Anyways,
thanks for taking the time to dig into this.


> You could call 39bd6f72 somewhat questionable as 'clobber_head' is
> related to 'force' more strongly than it is to 'reflog' [*1*], but
> it is unfair to blame anything else having done a mindless "add at
> the end".
> 

Yep, you're right. How does the following sound?

    branch: group related arguments of create_branch()
    
    39bd6f726 (Allow checkout -B <current-branch> to update the current
    branch, 2011-11-26) added 'clobber_head' (now, 'clobber_head_ok')
    "before" 'track' as 'track' was closely related 'clobber_head' for
    the purpose the commit wanted to achieve. Looking from the perspective
    of how the arguments are used it turns out that 'clobber_head' is
    more related to 'force' than it is to 'track'.
    
    So, re-order the arguments to keep the related arguments close
    to each other.
    
-- 
Kaartic