| Message ID | 20211206215528.97050-2-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
Glen Choo <chooglen@google.com> writes: > As Jonathan noted in v1, the diff is quite large. I could shrink this > by forward-declaring setup_tracking() so that the function definitions > are in the same order; let me know if that would be preferred. If you are not making any changes to setup_tracking() and just moving, then the patch size inflated by the move is OK. If you are moving and changing at the same time, well, that would make it harder to read what is going on in the patch, so you want to find a way to avoid it. Splitting it the pure move into a separate patch or use of forward-declaration may be good ways to do so. > branch.c | 195 ++++++++++++++++++++++++++-------------------- > branch.h | 13 +++- > builtin/branch.c | 7 +- > t/t3200-branch.sh | 17 ++++ > 4 files changed, 139 insertions(+), 93 deletions(-) > > diff --git a/branch.c b/branch.c > index 07a46430b3..a635a60f8b 100644 > --- a/branch.c > +++ b/branch.c > @@ -126,43 +126,6 @@ int install_branch_config(int flag, const char *local, const char *origin, const > return -1; > } > > -/* > - * 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. > - */ > -static 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; > - > - memset(&tracking, 0, sizeof(tracking)); > - tracking.spec.dst = (char *)orig_ref; > - if (for_each_remote(find_tracked_branch, &tracking)) > - return; > - > - if (!tracking.matches) > - switch (track) { > - case BRANCH_TRACK_ALWAYS: > - case BRANCH_TRACK_EXPLICIT: > - case BRANCH_TRACK_OVERRIDE: > - break; > - default: > - return; > - } > - > - if (tracking.matches > 1) > - die(_("Not tracking: ambiguous information for ref %s"), > - orig_ref); > - > - if (install_branch_config(config_flags, new_ref, tracking.remote, > - tracking.src ? tracking.src : orig_ref) < 0) > - exit(-1); > - > - free(tracking.src); > -} > - > int read_branch_desc(struct strbuf *buf, const char *branch_name) > { > char *v = NULL; > @@ -243,33 +206,35 @@ 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) > +/** > + * Validates whether a ref is a valid starting point for a branch, 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 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) > + * > + * - oid is an out parameter containing the object_id of start_name > + * > + * - real_ref is an out parameter containing the full, 'real' form of > + * start_name e.g. refs/heads/main instead of main > + * > + */ Good description that will help reviewers and future developers. Very much appreciated. > +static void validate_branch_start(struct repository *r, const char *start_name, > + enum branch_track track, > + struct object_id *oid, char **real_ref) > { > 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 (repo_get_oid_mb(r, start_name, oid)) { > if (explicit_tracking) { > if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) { > error(_(upstream_missing), start_name); The post context continues with: advise(_(upstream_advice)); exit(1); } die(_(upstream_missing), start_name); This is not a problem with this patch, and it should not be fixed as part of this series, but since I noticed it, I'll mention it as a leftover low-hanging fruit to be fixed after the dust settles. The exit(1) looks wrong. We should exit with 128 just like die() does. Issuing of an advice message should not affect the exit code. > +void setup_tracking(const char *new_ref, const char *orig_ref, > + enum branch_track track, int quiet, int expand_orig) It is unclear what expand_orig option is supposed to do and how it would help the caller. Perhaps a comment before the function is in order (the comment in branch.h before the declaration of this function does not make it clear, either). > +{ > + struct tracking tracking; > + int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; > + char *full_orig_ref; > + struct object_id unused_oid; > + > + memset(&tracking, 0, sizeof(tracking)); > + if (expand_orig) > + validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref); So, the idea is, because we are setting up a new_ref to "track" orig_ref, we may be better off pretending that we are "creating a new branch from the orig_ref and tracking it", so that orig_ref that is not something we can track will be caught with the same logic? This will cause full_orig_ref to start with "refs/heads/" or "refs/remotes/" if 'track' is something that requires tracking. > + else > + full_orig_ref = xstrdup(orig_ref); Even though the variable claims to be FULL orig_ref, when this side of if/else is taken, nobody guarantees that full_orig_ref is in fact a full ref, or merely the name of the branch, no? Would that cause problems later? > + tracking.spec.dst = full_orig_ref; > + if (for_each_remote(find_tracked_branch, &tracking)) > + goto cleanup; > + > + if (!tracking.matches) > + switch (track) { > + case BRANCH_TRACK_ALWAYS: > + case BRANCH_TRACK_EXPLICIT: > + case BRANCH_TRACK_OVERRIDE: > + break; > + default: > + goto cleanup; > + } > + > + if (tracking.matches > 1) > + die(_("Not tracking: ambiguous information for ref %s"), > + full_orig_ref); What's the next step for the user to take, after seeing this message? Do we have the necessary info readily available to help them at this point in tracking.* structure (e.g. "it could be following X or Y and we cannot decide between the two for you"), or have we discarded the information already? If tracking.matches == 0, because we broke out of the switch() for some values of track, we will make this install_branch_config() using members of the tracking structure, which is a bit unnerving. > + if (install_branch_config(config_flags, new_ref, tracking.remote, > + tracking.src ? tracking.src : full_orig_ref) < 0) But tracking.src==NULL is substituted with full_orig_ref, so as long as the value in that variable is sensible, we would probably be ok on the 4th parameter. I am not sure who set tracking.remote or if it is always set to a sensible value. Especially when tracking.matches is zero. > + exit(-1); Don't exit with a negative value. > +cleanup: > + free(tracking.src); > + free(full_orig_ref); > +} I'll stop here for now. Thanks.
Glen Choo <chooglen@google.com> writes: > +void setup_tracking(const char *new_ref, const char *orig_ref, > + enum branch_track track, int quiet, int expand_orig) > +{ > + struct tracking tracking; > + int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; > + char *full_orig_ref; > + struct object_id unused_oid; > + > + memset(&tracking, 0, sizeof(tracking)); > + if (expand_orig) > + validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref); > + else > + full_orig_ref = xstrdup(orig_ref); > + > + tracking.spec.dst = full_orig_ref; > + if (for_each_remote(find_tracked_branch, &tracking)) > + goto cleanup; > + > + if (!tracking.matches) > + switch (track) { > + case BRANCH_TRACK_ALWAYS: > + case BRANCH_TRACK_EXPLICIT: > + case BRANCH_TRACK_OVERRIDE: > + break; This heavily conflicts with what another topic "inherit tracking info from the other branch" wants to do to this function. What's the status of that topic, by the way? Should we block this one waiting for the other, or the other way around? Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> +void setup_tracking(const char *new_ref, const char *orig_ref, >> + enum branch_track track, int quiet, int expand_orig) >> +{ >> + struct tracking tracking; >> + int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; >> + char *full_orig_ref; >> + struct object_id unused_oid; >> + >> + memset(&tracking, 0, sizeof(tracking)); >> + if (expand_orig) >> + validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref); >> + else >> + full_orig_ref = xstrdup(orig_ref); >> + >> + tracking.spec.dst = full_orig_ref; >> + if (for_each_remote(find_tracked_branch, &tracking)) >> + goto cleanup; >> + >> + if (!tracking.matches) >> + switch (track) { >> + case BRANCH_TRACK_ALWAYS: >> + case BRANCH_TRACK_EXPLICIT: >> + case BRANCH_TRACK_OVERRIDE: >> + break; > > This heavily conflicts with what another topic "inherit tracking > info from the other branch" wants to do to this function. What's > the status of that topic, by the way? Should we block this one > waiting for the other, or the other way around? > > Thanks. As mentioned in [1], I plan to rebase this series on top of the "inherit tracking info from the other branch" series. I'll send a re-roll soon, thanks! [1] https://lore.kernel.org/git/kl6lbl1rauw3.fsf@chooglen-macbookpro.roam.corp.google.com/
Junio C Hamano <gitster@pobox.com> writes: >> As Jonathan noted in v1, the diff is quite large. I could shrink this >> by forward-declaring setup_tracking() so that the function definitions >> are in the same order; let me know if that would be preferred. > > If you are not making any changes to setup_tracking() and just > moving, then the patch size inflated by the move is OK. > > If you are moving and changing at the same time, well, that would > make it harder to read what is going on in the patch, so you want to > find a way to avoid it. Splitting it the pure move into a separate > patch or use of forward-declaration may be good ways to do so. Thanks for the advice! I'll use one of these approaches. > The post context continues with: > > advise(_(upstream_advice)); > exit(1); > } > die(_(upstream_missing), start_name); > > This is not a problem with this patch, and it should not be fixed as > part of this series, but since I noticed it, I'll mention it as a > leftover low-hanging fruit to be fixed after the dust settles. The > exit(1) looks wrong. We should exit with 128 just like die() does. > Issuing of an advice message should not affect the exit code. I'll include an optional cleanup patch to address this and the exit(-1). >> +void setup_tracking(const char *new_ref, const char *orig_ref, >> + enum branch_track track, int quiet, int expand_orig) > > It is unclear what expand_orig option is supposed to do and how it > would help the caller. Perhaps a comment before the function is in > order (the comment in branch.h before the declaration of this > function does not make it clear, either). Ah, I will add extra clarification. This boolean parameter controls whether or not we should validate + DWIM orig_ref. This is a performance optimization to avoid expanding an orig_ref that has already been expanded (e.g. because create_branch() has already expanded it). But, as you point out later on, this may not be a good idea. >> +{ >> + struct tracking tracking; >> + int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; >> + char *full_orig_ref; >> + struct object_id unused_oid; >> + >> + memset(&tracking, 0, sizeof(tracking)); >> + if (expand_orig) >> + validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref); > > So, the idea is, because we are setting up a new_ref to "track" > orig_ref, we may be better off pretending that we are "creating a > new branch from the orig_ref and tracking it", so that orig_ref that > is not something we can track will be caught with the same logic? > > This will cause full_orig_ref to start with "refs/heads/" or > "refs/remotes/" if 'track' is something that requires tracking. Yes. >> + else >> + full_orig_ref = xstrdup(orig_ref); > > Even though the variable claims to be FULL orig_ref, when this side > of if/else is taken, nobody guarantees that full_orig_ref is in fact > a full ref, or merely the name of the branch, no? Would that cause > problems later? Yes, the assumption is that the caller has already done the work of making sure that orig_ref has been validated and expanded into its full form. As mentioned earlier, this is purely a performance optimzation, but it is not a very safe one because it requires the caller to pick the correct value for expand_orig. I chose this approach because I anticipate that the only callers of setup_tracking() will be create_branch() (which always wants expand_orig = 0) and cmd_branch() (which always wants expand_orig = 1), so the right course of action is clear for now. I don't think setup_tracking() will be useful to anyone else, but expand_orig is a potential sharp edge for new callers. My next-preferred option would be to remove "expand_orig" and always call validate_branch_start(). We might waste a few cycles sometimes, but the function becomes impossible to misuse. >> + tracking.spec.dst = full_orig_ref; >> + if (for_each_remote(find_tracked_branch, &tracking)) >> + goto cleanup; >> + >> + if (!tracking.matches) >> + switch (track) { >> + case BRANCH_TRACK_ALWAYS: >> + case BRANCH_TRACK_EXPLICIT: >> + case BRANCH_TRACK_OVERRIDE: >> + break; >> + default: >> + goto cleanup; >> + } >> + >> + if (tracking.matches > 1) >> + die(_("Not tracking: ambiguous information for ref %s"), >> + full_orig_ref); > > What's the next step for the user to take, after seeing this message? > Do we have the necessary info readily available to help them at this > point in tracking.* structure (e.g. "it could be following X or Y and > we cannot decide between the two for you"), or have we discarded the > information already? This information is discarded in expand_ref(). From the function signature: int expand_ref(struct repository *repo, const char *str, int len, struct object_id *oid, char **ref) The return value is the number of matched refs and "ref" is an out parameter containing the first matched ref. > If tracking.matches == 0, because we broke out of the switch() for > some values of track, we will make this install_branch_config() > using members of the tracking structure, which is a bit unnerving. > >> + if (install_branch_config(config_flags, new_ref, tracking.remote, >> + tracking.src ? tracking.src : full_orig_ref) < 0) > > But tracking.src==NULL is substituted with full_orig_ref, so as long > as the value in that variable is sensible, we would probably be ok > on the 4th parameter. When tracking.matches is zero, the assumption is that install_branch_config() should set up tracking based off a local branch. By default, we assume that users only want tracking for remote-tracking branches, so only track local branches if we are confident that the user wants this, aka track = BRANCH_TRACK_{EXPLICIT,ALWAYS,OVERRIDE} (and soon, BRANCH_TRACK_INHERIT). > I am not sure who set tracking.remote or if it is always set to a > sensible value. Especially when tracking.matches is zero. tracking.matches == 0 should imply that tracking.remote == NULL, which gives us the expected behavior of tracking a local branch in install_branch_config(). This is also a bit too implicit for my tastes, but I don't think this is the time for such a refactor :)
diff --git a/branch.c b/branch.c index 07a46430b3..a635a60f8b 100644 --- a/branch.c +++ b/branch.c @@ -126,43 +126,6 @@ int install_branch_config(int flag, const char *local, const char *origin, const return -1; } -/* - * 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. - */ -static 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; - - memset(&tracking, 0, sizeof(tracking)); - tracking.spec.dst = (char *)orig_ref; - if (for_each_remote(find_tracked_branch, &tracking)) - return; - - if (!tracking.matches) - switch (track) { - case BRANCH_TRACK_ALWAYS: - case BRANCH_TRACK_EXPLICIT: - case BRANCH_TRACK_OVERRIDE: - break; - default: - return; - } - - if (tracking.matches > 1) - die(_("Not tracking: ambiguous information for ref %s"), - orig_ref); - - if (install_branch_config(config_flags, new_ref, tracking.remote, - tracking.src ? tracking.src : orig_ref) < 0) - exit(-1); - - free(tracking.src); -} - int read_branch_desc(struct strbuf *buf, const char *branch_name) { char *v = NULL; @@ -243,33 +206,35 @@ 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) +/** + * Validates whether a ref is a valid starting point for a branch, 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 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) + * + * - oid is an out parameter containing the object_id of start_name + * + * - real_ref is an out parameter containing the full, 'real' form of + * start_name e.g. refs/heads/main instead of main + * + */ +static void validate_branch_start(struct repository *r, const char *start_name, + enum branch_track track, + struct object_id *oid, char **real_ref) { 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 (repo_get_oid_mb(r, start_name, oid)) { if (explicit_tracking) { if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) { error(_(upstream_missing), start_name); @@ -281,7 +246,8 @@ void create_branch(struct repository *r, die(_("Not a valid object name: '%s'."), start_name); } - switch (dwim_ref(start_name, strlen(start_name), &oid, &real_ref, 0)) { + switch (repo_dwim_ref(r, start_name, strlen(start_name), oid, real_ref, + 0)) { case 0: /* Not branching from any existing branch */ if (explicit_tracking) @@ -289,12 +255,12 @@ void create_branch(struct repository *r, break; case 1: /* Unique completion -- good, only if it is a real branch */ - if (!starts_with(real_ref, "refs/heads/") && - validate_remote_tracking_branch(real_ref)) { + if (!starts_with(*real_ref, "refs/heads/") && + validate_remote_tracking_branch(*real_ref)) { if (explicit_tracking) die(_(upstream_not_branch), start_name); else - FREE_AND_NULL(real_ref); + FREE_AND_NULL(*real_ref); } break; default: @@ -302,37 +268,96 @@ void create_branch(struct repository *r, break; } - if ((commit = lookup_commit_reference(r, &oid)) == NULL) + if ((commit = lookup_commit_reference(r, oid)) == NULL) die(_("Not a valid branch point: '%s'."), start_name); - oidcpy(&oid, &commit->object.oid); + oidcpy(oid, &commit->object.oid); +} + +void setup_tracking(const char *new_ref, const char *orig_ref, + enum branch_track track, int quiet, int expand_orig) +{ + struct tracking tracking; + int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; + char *full_orig_ref; + struct object_id unused_oid; + + memset(&tracking, 0, sizeof(tracking)); + if (expand_orig) + validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref); + else + full_orig_ref = xstrdup(orig_ref); + + tracking.spec.dst = full_orig_ref; + if (for_each_remote(find_tracked_branch, &tracking)) + goto cleanup; + + if (!tracking.matches) + switch (track) { + case BRANCH_TRACK_ALWAYS: + case BRANCH_TRACK_EXPLICIT: + case BRANCH_TRACK_OVERRIDE: + break; + default: + goto cleanup; + } + + if (tracking.matches > 1) + die(_("Not tracking: ambiguous information for ref %s"), + full_orig_ref); + + if (install_branch_config(config_flags, new_ref, tracking.remote, + tracking.src ? tracking.src : full_orig_ref) < 0) + exit(-1); + +cleanup: + free(tracking.src); + free(full_orig_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; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; + char *msg; + + if (clobber_head_ok && !force) + BUG("'clobber_head_ok' can only be used with 'force'"); + + if (clobber_head_ok ? + validate_branchname(name, &ref) : + validate_new_branchname(name, &ref, force)) { + forcing = 1; + } + + validate_branch_start(r, start_name, track, &oid, &real_ref); if (reflog) log_all_ref_updates = LOG_REFS_NORMAL; - if (!dont_change_ref) { - struct ref_transaction *transaction; - struct strbuf err = STRBUF_INIT; - char *msg; - - if (forcing) - msg = xstrfmt("branch: Reset to %s", start_name); - else - msg = xstrfmt("branch: Created from %s", start_name); - - transaction = ref_transaction_begin(&err); - if (!transaction || - ref_transaction_update(transaction, ref.buf, - &oid, forcing ? NULL : null_oid(), - 0, msg, &err) || - ref_transaction_commit(transaction, &err)) - die("%s", err.buf); - ref_transaction_free(transaction); - strbuf_release(&err); - free(msg); - } + if (forcing) + msg = xstrfmt("branch: Reset to %s", start_name); + else + msg = xstrfmt("branch: Created from %s", start_name); + + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, ref.buf, + &oid, forcing ? NULL : null_oid(), + 0, msg, &err) || + ref_transaction_commit(transaction, &err)) + die("%s", err.buf); + ref_transaction_free(transaction); + strbuf_release(&err); + free(msg); if (real_ref && track) - setup_tracking(ref.buf + 11, real_ref, track, quiet); + setup_tracking(ref.buf + 11, real_ref, track, quiet, 0); strbuf_release(&ref); free(real_ref); diff --git a/branch.h b/branch.h index df0be61506..75cefcdcbd 100644 --- a/branch.h +++ b/branch.h @@ -17,6 +17,15 @@ extern enum branch_track git_branch_track; /* Functions for acting on the information about branches. */ +/* + * This sets the branch.<new_ref>.{remote,merge} config settings so that + * branch 'new_ref' tracks 'orig_ref'. This is called when branches are + * created, or when branch configs are updated (e.g. with + * git branch --set-upstream-to). + */ +void setup_tracking(const char *new_ref, const char *orig_ref, + enum branch_track track, int quiet, int expand_orig); + /* * Creates a new branch, where: * @@ -29,8 +38,8 @@ extern enum branch_track git_branch_track; * * - force enables overwriting an existing (non-head) branch * - * - clobber_head_ok allows the currently checked out (hence existing) - * branch to be overwritten; without 'force', it has no effect. + * - clobber_head_ok, when enabled with 'force', allows the currently + * checked out (head) branch to be overwritten * * - reflog creates a reflog for the branch * diff --git a/builtin/branch.c b/builtin/branch.c index 81b5c111cb..19f2845e7a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -821,12 +821,7 @@ 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); + setup_tracking(branch->name, new_upstream, BRANCH_TRACK_OVERRIDE, quiet, 1); } else if (unset_upstream) { struct branch *branch = branch_get(argv[0]); struct strbuf buf = STRBUF_INIT; diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 8c5c1ccf33..f97cf495ab 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 +' + test_expect_success 'git branch a/b/c should create a branch' ' git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c '
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`. 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, like setup_tracking(), and use create_branch() only to create branches. When this is done, it will be trivial to add "dry_run". Do this refactor by moving the branch validation and dwim logic from create_branch() into a new function, validate_branch_start(), and call it from setup_tracking(). Now that setup_tracking() can perform dwim and tracking setup without creating a branch, use it in `git branch --set-upstream-to` and remove unnecessary behavior from create_branch(). Since there were none, add tests for creating a branch with `--force`. Signed-off-by: Glen Choo <chooglen@google.com> --- As Jonathan noted in v1, the diff is quite large. I could shrink this by forward-declaring setup_tracking() so that the function definitions are in the same order; let me know if that would be preferred. branch.c | 195 ++++++++++++++++++++++++++-------------------- branch.h | 13 +++- builtin/branch.c | 7 +- t/t3200-branch.sh | 17 ++++ 4 files changed, 139 insertions(+), 93 deletions(-)