Glen Choo <chooglen@xxxxxxxxxx> 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.
> 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.
> @@ -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.
> @@ -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.
> @@ -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.
> +/**
> + * 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]
> @@ -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.
> + if (out_oid)
> + oidcpy(out_oid, &commit->object.oid);
> +
> + FREE_AND_NULL(real_ref);
> +}
Comparing dwim_branch_start()...
> +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...
> @@ -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.
