| Message ID | 20211220233459.45739-3-chooglen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
Glen Choo <chooglen@google.com> writes: > create_branch() was formerly used to set tracking without creating a > branch. Since the previous commit replaces this use case with > dwim_and_setup_tracking(), we can simplify create_branch() so that it > always creates a branch. > > Do this simplification, in particular: > > * remove the special handling of BRANCH_TRACK_OVERRIDE because it is no > longer used > * assert that clobber_head_ok can only be provided with force > * check that we're handling clobber_head_ok and force correctly by > introducing tests for `git branch --force` This might have been more simply explained as: With the previous commit, these are true of create_branch(): * BRANCH_TRACK_OVERRIDE is no longer passed * if clobber_head_ok is true, force is also true Assert these situations, delete dead code, and ensure that we're handling clobber_head_ok and force correctly by introducing tests for `git branch --force`. This also means that create_branch() now always creates a branch. > @@ -426,15 +426,17 @@ void create_branch(struct repository *r, const char *name, > 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; > + 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; > } Also assert that track is not BRANCH_TRACK_OVERRIDE. > @@ -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 > +' These tests make sense.
Jonathan Tan <jonathantanmy@google.com> writes: > Glen Choo <chooglen@google.com> writes: >> create_branch() was formerly used to set tracking without creating a >> branch. Since the previous commit replaces this use case with >> dwim_and_setup_tracking(), we can simplify create_branch() so that it >> always creates a branch. >> >> Do this simplification, in particular: >> >> * remove the special handling of BRANCH_TRACK_OVERRIDE because it is no >> longer used >> * assert that clobber_head_ok can only be provided with force >> * check that we're handling clobber_head_ok and force correctly by >> introducing tests for `git branch --force` > > This might have been more simply explained as: > > With the previous commit, these are true of create_branch(): > * BRANCH_TRACK_OVERRIDE is no longer passed > * if clobber_head_ok is true, force is also true > > Assert these situations, delete dead code, and ensure that we're > handling clobber_head_ok and force correctly by introducing tests for > `git branch --force`. This also means that create_branch() now always > creates a branch. This is a great suggestion, I'll incorporate some of this. As an aside, it has always been the case that clobber_head_ok is only true when force is true, so it might be misleading to include it under the umbrella of 'With the previous commit'. I'll move things around accordingly. >> @@ -426,15 +426,17 @@ void create_branch(struct repository *r, const char *name, >> 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; >> + 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; >> } > > Also assert that track is not BRANCH_TRACK_OVERRIDE. Makes sense, I'll do that.
diff --git a/branch.c b/branch.c index e271a4e0a7..de680f311d 100644 --- a/branch.c +++ b/branch.c @@ -426,15 +426,17 @@ void create_branch(struct repository *r, const char *name, 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; + 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; } dwim_branch_start(r, start_name, track, &real_ref, &oid); @@ -442,27 +444,20 @@ void create_branch(struct repository *r, const char *name, 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); diff --git a/branch.h b/branch.h index ab2315c611..cf3a4d3ff3 100644 --- a/branch.h +++ b/branch.h @@ -52,8 +52,8 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref, * * - 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/t/t3200-branch.sh b/t/t3200-branch.sh index 09ab132377..71a72efcb2 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 '
create_branch() was formerly used to set tracking without creating a branch. Since the previous commit replaces this use case with dwim_and_setup_tracking(), we can simplify create_branch() so that it always creates a branch. Do this simplification, in particular: * remove the special handling of BRANCH_TRACK_OVERRIDE because it is no longer used * assert that clobber_head_ok can only be provided with force * check that we're handling clobber_head_ok and force correctly by introducing tests for `git branch --force` Signed-off-by: Glen Choo <chooglen@google.com> --- branch.c | 55 +++++++++++++++++++++-------------------------- branch.h | 4 ++-- t/t3200-branch.sh | 17 +++++++++++++++ 3 files changed, 44 insertions(+), 32 deletions(-)