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 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@xxxxxxxxxx/ > +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;
