Re: [PATCH v3 1/5] branch: move --set-upstream-to behavior to setup_tracking()

[Jonathan Tan <jonathantanmy@google.com>]

Glen Choo <chooglen@google.com> writes:
> This refactor is motivated by a desire to add a "dry_run" parameter to
> create_branch() that will validate whether or not a branch can be
> created without actually creating it - this behavior be used in a
> subsequent commit that adds `git branch --recurse-submodules topic`.

When I said that the patch was doing too much [1], I meant that for a
patch that is supposed to be about refactoring, there were many lines
that didn't seem to be a straightforward move. I've attached a version
of the patch that I expected - notice that a reviewer can see that the
lines are straightforwardly moved.

[1] https://lore.kernel.org/git/20211123231035.3607109-1-jonathantanmy@google.com/

> +void setup_tracking(const char *new_ref, const char *orig_ref,
> +			   enum branch_track track, int quiet, int expand_orig)
>  {
>  	struct tracking tracking;
>  	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
> +	char *full_orig_ref;
> +	struct object_id unused_oid;
>  
>  	memset(&tracking, 0, sizeof(tracking));
> -	tracking.spec.dst = (char *)orig_ref;
> +	if (expand_orig)
> +		validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
> +	else
> +		full_orig_ref = xstrdup(orig_ref);

Having two meanings for a parameter (and which meaning used depending on
another parameter) is quite confusing - I think it's better to call
another function to expand the ref if necessary. See my patch below for
an example.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 267a624671..18e285a876 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -42,6 +42,23 @@ test_expect_success 'git branch abc should create a branch' '
>  	git branch abc && test_path_is_file .git/refs/heads/abc
>  '
>  
> +test_expect_success 'git branch abc should fail when abc exists' '
> +	test_must_fail git branch abc
> +'
> +
> +test_expect_success 'git branch --force abc should fail when abc is checked out' '
> +	test_when_finished git switch main &&
> +	git switch abc &&
> +	test_must_fail git branch --force abc HEAD~1
> +'
> +
> +test_expect_success 'git branch --force abc should succeed when abc exists' '
> +	git rev-parse HEAD~1 >expect &&
> +	git branch --force abc HEAD~1 &&
> +	git rev-parse abc >actual &&
> +	test_cmp expect actual
> +'

This seems like an unrelated test for a refactoring that doesn't seem to
touch "force" (in builtin/branch.c, the line removed hardcodes force to
0).

Below is my diff, as mentioned.

---
diff --git a/branch.c b/branch.c
index 07a46430b3..a6803e9900 100644
--- a/branch.c
+++ b/branch.c
@@ -131,8 +131,8 @@ int install_branch_config(int flag, const char *local, const char *origin, const
  * to infer the settings for branch.<new_ref>.{remote,merge} from the
  * config.
  */
-static void setup_tracking(const char *new_ref, const char *orig_ref,
-			   enum branch_track track, int quiet)
+void setup_tracking(const char *new_ref, const char *orig_ref,
+		    enum branch_track track, int quiet)
 {
 	struct tracking tracking;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
@@ -243,30 +243,12 @@ N_("\n"
 "will track its remote counterpart, you may want to use\n"
 "\"git push -u\" to set the upstream config as you push.");
 
-void create_branch(struct repository *r,
-		   const char *name, const char *start_name,
-		   int force, int clobber_head_ok, int reflog,
-		   int quiet, enum branch_track track)
+void TODO_name(struct repository *r, const char *start_name, int explicit_tracking,
+	       char **out_real_ref, struct commit **out_commit)
 {
 	struct commit *commit;
 	struct object_id oid;
 	char *real_ref;
-	struct strbuf ref = STRBUF_INIT;
-	int forcing = 0;
-	int dont_change_ref = 0;
-	int explicit_tracking = 0;
-
-	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
-		explicit_tracking = 1;
-
-	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
-	    ? validate_branchname(name, &ref)
-	    : validate_new_branchname(name, &ref, force)) {
-		if (!force)
-			dont_change_ref = 1;
-		else
-			forcing = 1;
-	}
 
 	real_ref = NULL;
 	if (get_oid_mb(start_name, &oid)) {
@@ -304,6 +286,38 @@ void create_branch(struct repository *r,
 
 	if ((commit = lookup_commit_reference(r, &oid)) == NULL)
 		die(_("Not a valid branch point: '%s'."), start_name);
+	if (out_real_ref)
+		*out_real_ref = real_ref;
+	if (out_commit)
+		*out_commit = commit;
+}
+
+void create_branch(struct repository *r,
+		   const char *name, const char *start_name,
+		   int force, int clobber_head_ok, int reflog,
+		   int quiet, enum branch_track track)
+{
+	struct commit *commit;
+	struct object_id oid;
+	char *real_ref = NULL;
+	struct strbuf ref = STRBUF_INIT;
+	int forcing = 0;
+	int dont_change_ref = 0;
+	int explicit_tracking = 0;
+
+	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
+		explicit_tracking = 1;
+
+	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
+	    ? validate_branchname(name, &ref)
+	    : validate_new_branchname(name, &ref, force)) {
+		if (!force)
+			dont_change_ref = 1;
+		else
+			forcing = 1;
+	}
+
+	TODO_name(r, start_name, explicit_tracking, &real_ref, &commit);
 	oidcpy(&oid, &commit->object.oid);
 
 	if (reflog)
diff --git a/branch.h b/branch.h
index df0be61506..577483012b 100644
--- a/branch.h
+++ b/branch.h
@@ -1,6 +1,7 @@
 #ifndef BRANCH_H
 #define BRANCH_H
 
+struct commit;
 struct repository;
 struct strbuf;
 
@@ -17,6 +18,11 @@ extern enum branch_track git_branch_track;
 
 /* Functions for acting on the information about branches. */
 
+void setup_tracking(const char *new_ref, const char *orig_ref,
+		    enum branch_track track, int quiet);
+void TODO_name(struct repository *r, const char *start_name, int explicit_tracking,
+	       char **out_real_ref, struct commit **out_commit);
+
 /*
  * Creates a new branch, where:
  *
diff --git a/builtin/branch.c b/builtin/branch.c
index 81b5c111cb..78f5c1d17e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -806,6 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch = branch_get(argv[0]);
+		char *real_ref;
 
 		if (argc > 1)
 			die(_("too many arguments to set new upstream"));
@@ -821,12 +822,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!ref_exists(branch->refname))
 			die(_("branch '%s' does not exist"), branch->name);
 
-		/*
-		 * create_branch takes care of setting up the tracking
-		 * info and making sure new_upstream is correct
-		 */
-		create_branch(the_repository, branch->name, new_upstream,
-			      0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+		TODO_name(the_repository, new_upstream, 1, &real_ref, NULL);
+		setup_tracking(branch->name, real_ref, BRANCH_TRACK_OVERRIDE, quiet);
+		free(real_ref);
 	} else if (unset_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 		struct strbuf buf = STRBUF_INIT;