| Message ID | 20211220233459.45739-2-chooglen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
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. > 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.
Jonathan Tan <jonathantanmy@google.com> writes: > Glen Choo <chooglen@google.com> writes: >> 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. I was hoping to explain why we made the decision to reuse create_branch() to set tracking information (it made sense with --set-upstream) and why it now makes sense to use another function (because we no longer have --set-upstream). But maybe this justification is unnecessary, and the desire to add a dry_run parameter is enough. > > Also, what is the now-unnecessary logic to be removed in a later commit? This would be the logic that makes create_branch() not create branches, which is addressed in your proposed commit message. >> 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. Hm, I see. This makes sense, I'll incorporate some of your suggestions :) >> @@ -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. Thanks for the catch. > >> @@ -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. Ah, I didn't realize it had rewrapped. Thanks for the catch. >> +/** >> + * 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. Hm, you are right. The xstrdup was added because the original function calls FREE_AND_NULL(real_ref) and then checks if real_ref is NULL. But after the refactor, real_ref is not referenced after the FREE_AND_NULL(real_ref), so that call can be removed. I intend to remove the xstrdup, though it will introduce a bit of noise because that block will no longer be moved wholesale.
Glen Choo <chooglen@google.com> writes: > >> @@ -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. > > Hm, you are right. The xstrdup was added because the original function > calls FREE_AND_NULL(real_ref) and then checks if real_ref is NULL. But > after the refactor, real_ref is not referenced after the > FREE_AND_NULL(real_ref), so that call can be removed. > > I intend to remove the xstrdup, though it will introduce a bit of noise > because that block will no longer be moved wholesale. Ah, thanks for watching out for the diff. I think in this case, we'll be fine, since the xstrdup line is a new line ("xstrdup" only appears once in the diff, here).
diff --git a/branch.c b/branch.c index fa339b64e2..e271a4e0a7 100644 --- a/branch.c +++ b/branch.c @@ -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) @@ -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); } @@ -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) +/** + * 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) { 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)) { if (explicit_tracking) { @@ -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; + if (out_oid) + oidcpy(out_oid, &commit->object.oid); + + FREE_AND_NULL(real_ref); +} + +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 object_id oid; + char *real_ref; + struct strbuf ref = STRBUF_INIT; + int forcing = 0; + int dont_change_ref = 0; + + 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; + } + + dwim_branch_start(r, start_name, track, &real_ref, &oid); if (reflog) log_all_ref_updates = LOG_REFS_NORMAL; @@ -435,6 +471,15 @@ void create_branch(struct repository *r, free(real_ref); } +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); +} + void remove_merge_branch_state(struct repository *r) { unlink(git_path_merge_head(r)); diff --git a/branch.h b/branch.h index 815dcd40c0..ab2315c611 100644 --- a/branch.h +++ b/branch.h @@ -18,6 +18,28 @@ extern enum branch_track git_branch_track; /* Functions for acting on the information about branches. */ +/** + * Sets branch.<new_ref>.{remote,merge} config settings such that + * new_ref tracks orig_ref according to the specified tracking mode. + * + * - new_ref is the name of the branch that we are setting tracking + * for. + * + * - orig_ref is the name of the ref that is 'upstream' of new_ref. + * orig_ref will be expanded with DWIM so that the config settings + * are in the correct format e.g. "refs/remotes/origin/main" instead + * of "origin/main". + * + * - track is the tracking mode e.g. BRANCH_TRACK_REMOTE causes + * new_ref to track orig_ref directly, whereas BRANCH_TRACK_INHERIT + * causes new_ref to track whatever orig_ref tracks. + * + * - quiet suppresses tracking information. + */ +void dwim_and_setup_tracking(struct repository *r, const char *new_ref, + const char *orig_ref, enum branch_track track, + int quiet); + /* * Creates a new branch, where: * diff --git a/builtin/branch.c b/builtin/branch.c index 81a29edb4a..16a7e80df5 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -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;
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`. 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. 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. 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. Signed-off-by: Glen Choo <chooglen@google.com> --- branch.c | 87 ++++++++++++++++++++++++++++++++++++------------ branch.h | 22 ++++++++++++ builtin/branch.c | 9 ++--- 3 files changed, 91 insertions(+), 27 deletions(-)