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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 11, 2022, 02:09 UTC
Message-ID: <20220111020953.1225789-1-jonathantanmy@google.com>
In-Reply-To: <20211220233459.45739-2-chooglen@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 will be used in a
> subsequent commit that adds `git branch --recurse-submodules topic`.

Makes sense.

> Adding "dry_run" is not obvious because create_branch() is also used to
> set tracking information without creating a branch, i.e. when using
> --set-upstream-to.

create_branch() is complicated, OK.

Show 8 quoted lines

> This appears to be a leftover from 4fc5006676 (Add
> branch --set-upstream, 2010-01-18), when --set-upstream would sometimes
> create a branch and sometimes update tracking information without
> creating a branch. However, we no longer support --set-upstream, so it
> makes more sense to set tracking information with another function and
> use create_branch() only to create branches. In a later commit, we will
> remove the now-unnecessary logic from create_branch() so that "dry_run"
> becomes trivial to implement.

What do you mean by "leftover"?

Aside from that, the pertinent information is that the mentioned commit changed create_branch() to no longer always create a branch, instead sometimes creating a branch and sometimes updating tracking information (and sometimes both). I don't think whether we support "--set-upstream" is material here.

Also, what is the now-unnecessary logic to be removed in a later commit?

> Introduce dwim_and_setup_tracking(), which replaces create_branch()
> in `git branch --set-upstream-to`. Ensure correctness by moving the DWIM
> and branch validation logic from create_branch() into a helper function,
> dwim_branch_start(), so that the logic is shared by both functions.

I think it's clearer to just say what we're refactoring instead of saying that we're introducing a function and making sure that it is correct, not by testing (as one would expect), but by moving logic.

I would write the commit message like this:

  This commit is in preparation for a future commit that adds a dry_run
  parameter to create_branch() (that is needed for supporting "git
  branch --recurse-submodules", to be introduced in another future
  commit).

  create_branch() used to always create a branch, but this was changed
  in 4fc5006676 (Add branch --set-upstream, 2010-01-18), when it was
  changed to be also able to set tracking information.

  Refactor the code that sets tracking information into its own
  functions dwim_branch_start() and dwim_and_setup_tracking(). Also
  change an invocation of create_branch() in cmd_branch() in
  builtin/branch.c to use dwim_and_setup_tracking(), since that
  invocation is only for setting tracking information.

And if this is true:

  As of this commit, create_branch() still sometimes does not create
  branches, but this will be fixed in a subsequent commit.

Show 15 quoted lines

> @@ -217,9 +217,11 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
>  }
>  
>  /*
> - * This is called when new_ref is branched off of orig_ref, and tries
> - * to infer the settings for branch.<new_ref>.{remote,merge} from the
> - * config.
> + * Used internally to set the branch.<new_ref>.{remote,merge} config
> + * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
> + * dwim_and_setup_tracking(), this does not do DWIM, i.e. "origin/main"
> + * will not be expanded to "refs/remotes/origin/main", so it is not safe
> + * for 'orig_ref' to be raw user input.
>   */
>  static void setup_tracking(const char *new_ref, const char *orig_ref,
>  			   enum branch_track track, int quiet)

The comment makes sense.

Show 17 quoted lines

> @@ -244,7 +246,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  		case BRANCH_TRACK_INHERIT:
>  			break;
>  		default:
> -			return;
> +			goto cleanup;
>  		}
>  
>  	if (tracking.matches > 1)
> @@ -257,6 +259,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  				tracking.remote, tracking.srcs) < 0)
>  		exit(-1);
>  
> +cleanup:
>  	string_list_clear(tracking.srcs, 0);
>  }
>

This seems like it's just for avoiding a memory leak, and is unrelated to this commit, so it should go into its own commit.

Show 8 quoted lines

> @@ -340,31 +343,37 @@ 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)

This seems to have the same parameters as the "+" version, but wrapped differently - don't rewrap unless you're also changing it.

Show 22 quoted lines

> +/**
> + * DWIMs a user-provided ref to determine the starting point for a
> + * branch and validates it, where:
> + *
> + *   - r is the repository to validate the branch for
> + *
> + *   - start_name is the ref that we would like to test. This is
> + *     expanded with DWIM and assigned to out_real_ref.
> + *
> + *   - track is the tracking mode of the new branch. If tracking is
> + *     explicitly requested, start_name must be a branch (because
> + *     otherwise start_name cannot be tracked)
> + *
> + *   - out_oid is an out parameter containing the object_id of start_name
> + *
> + *   - out_real_ref is an out parameter containing the full, 'real' form
> + *     of start_name e.g. refs/heads/main instead of main
> + *
> + */
> +static void dwim_branch_start(struct repository *r, const char *start_name,
> +			   enum branch_track track, char **out_real_ref,
> +			   struct object_id *out_oid)

[snip]

Show 7 quoted lines

> @@ -401,7 +410,34 @@ void create_branch(struct repository *r,
>  
>  	if ((commit = lookup_commit_reference(r, &oid)) == NULL)
>  		die(_("Not a valid branch point: '%s'."), start_name);
> -	oidcpy(&oid, &commit->object.oid);
> +	if (out_real_ref)
> +		*out_real_ref = real_ref ? xstrdup(real_ref) : NULL;

I think you can just write "*out_real_ref = real_ref; real_ref = NULL;" here, and then not need to xstrdup.

Show 5 quoted lines

> +	if (out_oid)
> +		oidcpy(out_oid, &commit->object.oid);
> +
> +	FREE_AND_NULL(real_ref);
> +}

Comparing dwim_branch_start()...

Show 8 quoted lines

> +void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
> +			     const char *orig_ref, enum branch_track track,
> +			     int quiet)
> +{
> +	char *real_orig_ref;
> +	dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);
> +	setup_tracking(new_ref, real_orig_ref, track, quiet);
> +}

...and this...

Show 16 quoted lines

> @@ -823,12 +823,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);
> +		dwim_and_setup_tracking(the_repository, branch->name,
> +					new_upstream, BRANCH_TRACK_OVERRIDE,
> +					quiet);
>  	} else if (unset_upstream) {
>  		struct branch *branch = branch_get(argv[0]);
>  		struct strbuf buf = STRBUF_INIT;

...looking at this, I can see that dwim_and_setup_tracking() indeed does everything that this create_branch() invocation would do, so overall the commit makes sense.