branch: -d protects upstream branches
cover branch: -d protects upstream branches
Protect local branches from git branch -d when a surviving branch depends on them through a local upstream chain.
Harald Nordgren (2):
branch: move stacked branch helpers
branch: protect local upstreams from -d
Documentation/git-branch.adoc | 4 +- builtin/branch.c | 142 +++++++++++++++++++++------------- t/t1507-rev-parse-upstream.sh | 4 +- t/t3200-branch.sh | 43 ++++++++++ t/t6040-tracking-info.sh | 2 +- 5 files changed, 137 insertions(+), 58 deletions(-)
base-commit: 2c3adbb2c475981e340c79fdc5e7f4f9b5d9054e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2365%2FHaraldNordgren%2Fbranch-d-protect-stacked-upstreams-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2365/HaraldNordgren/branch-d-protect-stacked-upstreams-v1
Pull-Request: https://github.com/git/git/pull/2365
Signature · 1 line
gitgitgadget
PATCH 1/2 branch: move stacked branch helpers
From: Harald Nordgren <[email protected]>
Move the stacked branch helpers earlier so delete_branches() can use them without a forward declaration.
--- builtin/branch.c | 104 +++++++++++++++++++++++------------------------ 1 file changed, 52 insertions(+), 52 deletions(-)
builtin/branch.c +52 -52
@@ -203,6 +203,58 @@ enum delete_branch_flags { DELETE_BRANCH_DRY_RUN = (1 << 4), }; +struct stacked_branch_data { +struct strset *deletable_branch_names; +struct strset *protected_branch_names; +}; + +static int collect_stacked_branch_base (const struct reference *ref, +void *cb_data) +{ +struct stacked_branch_data *data = cb_data; +const char *branch_name; +struct branch *branch; +const char *upstream_refname; +const char *upstream_branch_name; + +if (!skip_prefix (ref->name,"refs/heads/" , &branch_name)) +BUG ("expected local branch ref, got '%s'" , ref->name); +if (strset_contains (data->deletable_branch_names, branch_name)) +return 0 ; + + branch =branch_get (branch_name); + upstream_refname =branch_get_upstream (branch,NULL ); +if (!upstream_refname || + !skip_prefix (upstream_refname,"refs/heads/" , + &upstream_branch_name) || + !strset_contains (data->deletable_branch_names, + upstream_branch_name)) +return 0 ; + +strset_add (data->protected_branch_names, upstream_branch_name); +return 0 ; +} + +static void protect_stacked_branch_bases (struct ref_store *refs, +struct strset *deletable_branch_names, +struct strset *protected_branch_names) +{ +struct stacked_branch_data data = { + .deletable_branch_names = deletable_branch_names, + .protected_branch_names = protected_branch_names, + }; +struct refs_for_each_ref_options opts = { + .prefix ="refs/heads/" , + }; +struct hashmap_iter iter; +struct strmap_entry *entry; + +refs_for_each_ref_ext (refs, collect_stacked_branch_base, &data, &opts); + +strset_for_each_entry (protected_branch_names, &iter, entry) +strset_remove (deletable_branch_names, entry->key); +} +static int check_branch_commit (const char *branchname,const char *refname,const struct object_id *oid,struct commit *head_rev,int kinds,unsigned int flags)
@@ -718,58 +770,6 @@ static int parse_opt_forked(const struct option *opt, const char *arg, int unsetreturn 0 ; } -struct stacked_branch_data { -struct strset *deletable_branch_names; -struct strset *protected_branch_names; -}; - -static int collect_stacked_branch_base (const struct reference *ref, -void *cb_data) -{ -struct stacked_branch_data *data = cb_data; -const char *branch_name; -struct branch *branch; -const char *upstream_refname; -const char *upstream_branch_name; - -if (!skip_prefix (ref->name,"refs/heads/" , &branch_name)) -BUG ("expected local branch ref, got '%s'" , ref->name); -if (strset_contains (data->deletable_branch_names, branch_name)) -return 0 ; - - branch =branch_get (branch_name); - upstream_refname =branch_get_upstream (branch,NULL ); -if (!upstream_refname || - !skip_prefix (upstream_refname,"refs/heads/" , - &upstream_branch_name) || - !strset_contains (data->deletable_branch_names, - upstream_branch_name)) -return 0 ; - -strset_add (data->protected_branch_names, upstream_branch_name); -return 0 ; -} - -static void protect_stacked_branch_bases (struct ref_store *refs, -struct strset *deletable_branch_names, -struct strset *protected_branch_names) -{ -struct stacked_branch_data data = { - .deletable_branch_names = deletable_branch_names, - .protected_branch_names = protected_branch_names, - }; -struct refs_for_each_ref_options opts = { - .prefix ="refs/heads/" , - }; -struct hashmap_iter iter; -struct strmap_entry *entry; - -refs_for_each_ref_ext (refs, collect_stacked_branch_base, &data, &opts); - -strset_for_each_entry (protected_branch_names, &iter, entry) -strset_remove (deletable_branch_names, entry->key); -} -static void clear_deleted_upstreams (struct strset *protected_branch_names,struct strset *deletable_branch_names) {
Signature · 1 line
gitgitgadget
PATCH 2/2 branch: protect local upstreams from -d
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
A local branch may be fully merged into its own upstream while still serving as the base of a surviving stacked branch. Deleting it with "git branch -d" then leaves the surviving branch with a missing upstream.
Use the existing stacked-branch protection after checking every requested deletion. This makes multi-branch deletion independent of argument order: a branch that fails its safety check remains available to protect its upstream. Allow -D to override the protection, and allow a complete stack to be deleted together.
--- Documentation/git-branch.adoc | 4 +++- builtin/branch.c | 38 +++++++++++++++++++++++++++++-- t/t1507-rev-parse-upstream.sh | 4 ++-- t/t3200-branch.sh | 43 +++++++++++++++++++++++++++++++++++ t/t6040-tracking-info.sh | 2 +- 5 files changed, 85 insertions(+), 6 deletions(-)
Documentation/git-branch.adoc +3 -1
@@ -102,7 +102,9 @@ OPTIONS `--delete`:: Delete a branch. The branch must be fully merged in its upstream branch, or in `HEAD` if no upstream was set with - `--track` or `--set-upstream-to`. + `--track` or `--set-upstream-to`, and must not be an upstream, + directly or indirectly, of another local branch that will remain + after the operation. `-D`:: Shortcut for `--delete --force`.
builtin/branch.c +36 -2
@@ -294,12 +294,13 @@ static int delete_branches(int argc, const char **argv, int kinds,struct object_id oid;char *name =NULL ;const char *fmt; -int i;int ret =0 ;int remote_branch =0 ;struct strbuf bname =STRBUF_INIT ;enum interpret_branch_kind allowed_interpret;struct string_list refs_to_delete =STRING_LIST_INIT_DUP ; +struct strset deletable_branch_names =STRSET_INIT ; +struct strset protected_branch_names =STRSET_INIT ;struct string_list_item *item;int branch_name_pos;const char *fmt_remotes ="refs/remotes/%s" ;
@@ -326,7 +327,7 @@ static int delete_branches(int argc, const char **argv, int kinds, !(flags & DELETE_BRANCH_NO_HEAD_FALLBACK)) head_rev =lookup_commit_reference (the_repository, &head_oid); -for (i =0 ; i < argc; i++,strbuf_reset (&bname)) { +for (int i =0 ; i < argc; i++,strbuf_reset (&bname)) {char *target =NULL ;int ref_flags =0 ;
@@ -397,11 +398,42 @@ static int delete_branches(int argc, const char **argv, int kinds, item->util =xstrdup ((ref_flags &REF_ISBROKEN ) ?"broken" : (ref_flags &REF_ISSYMREF ) ? target :repo_find_unique_abbrev (the_repository, &oid,DEFAULT_ABBREV )); +if (!remote_branch && !(flags & (DELETE_BRANCH_FORCE | +DELETE_BRANCH_SKIP_UNMERGED ))) +strset_add (&deletable_branch_names, bname.buf); next:free (target); } +if (!remote_branch && + !(flags & (DELETE_BRANCH_FORCE |DELETE_BRANCH_SKIP_UNMERGED )) && + refs_to_delete.nr) { +protect_stacked_branch_bases (get_main_ref_store (the_repository), + &deletable_branch_names, &protected_branch_names); +for (size_t i = refs_to_delete.nr; i; i--) { +const char *branch_name; + + item = &refs_to_delete.items[i -1 ]; +if (!skip_prefix (item->string,"refs/heads/" , + &branch_name)) +BUG ("expected local branch ref, got '%s'" , + item->string); +if (strset_contains (&deletable_branch_names, branch_name)) +continue ; + +error (_ ("the branch '%s' is an upstream of another branch" ), + branch_name); +advise_if_enabled (ADVICE_FORCE_DELETE_BRANCH , +_ ("If you are sure you want to delete it, " +"run 'git branch -D %s'" ), + branch_name); + ret =1 ; +unsorted_string_list_delete_item (&refs_to_delete, i -1 , +1 ); + } + } +if (!(flags &DELETE_BRANCH_DRY_RUN ) &&refs_delete_refs (get_main_ref_store (the_repository),NULL , &refs_to_delete,REF_NO_DEREF )) ret =1 ;
@@ -428,6 +460,8 @@ static int delete_branches(int argc, const char **argv, int kinds,free (describe_ref); }string_list_clear (&refs_to_delete,0 ); +strset_clear (&deletable_branch_names); +strset_clear (&protected_branch_names);free (name);strbuf_release (&bname);
t/t1507-rev-parse-upstream.sh +2 -2
@@ -146,9 +146,9 @@ test_expect_success 'merge my-side@{u} records the correct name' ' ) ' -test_expect_success 'branch -d other@{u}' ' +test_expect_success 'branch -D other@{u}' ' git checkout -t -b other main && - git branch -d @{u} && + git branch -D @{u} && git for-each-ref refs/heads/main >actual && test_must_be_empty actual '
t/t3200-branch.sh +43 -0
49 lines not on this page · raw message
t/t6040-tracking-info.sh +1 -1
@@ -34,7 +34,7 @@ test_expect_success setup ' git checkout -b brokenbase origin && git checkout -b b5 --track brokenbase && advance g && - git branch -d brokenbase && + git branch -D brokenbase && git checkout -b b6 origin ) && git checkout -b follower --track main &&
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] branch: protect local upstreams from -d
Quoted 7 lines · Harald Nordgren via GitGitGadget
From: Harald Nordgren <[email protected]>
A local branch may be fully merged into its own upstream while still serving as the base of a surviving stacked branch. Deleting it with "git branch -d" then leaves the surviving branch with a missing upstream.
Quoted 5 lines
Use the existing stacked-branch protection after checking every requested deletion. This makes multi-branch deletion independent of argument order: a branch that fails its safety check remains available to protect its upstream. Allow -D to override the protection, and allow a complete stack to be deleted together.
It may be a good thing to optionally be able to do this, but changing the long-established semantics of what the '-d' option means would lead to serious breakage to the end-user workflows people depend on, I am afraid, and...
t/t1507-rev-parse-upstream.sh test_expect_success 'merge my-side@{u} records the correct name' ' · line 154
Quoted 8 lines
t/t1507-rev-parse-upstream.sh +2
-2-test_expect_success 'branch -d other@{u}' ' +test_expect_success 'branch -D other@{u}' ' git checkout -t -b other main && - git branch -d @{u} && + git branch -D @{u} && git for-each-ref refs/heads/main >actual && test_must_be_empty actual '
... having to adjust the test command sequence like this is a very clear illustration of why it is not a safe thing to do. Our change just broke what the user wanted to do, i.e., removing the branch @{u}, which they have happily been doing with '-d' while guarded by the original safety feature '-d' already had. Now they have to use '-D' to remove it unconditionally without safety -- that is not exactly progress. In addition, depending on the version of Git, our change makes 'git branch -d' behave differently, making it less predictable.
And no, a configuration variable to tweak the behaviour of '-d' is unwelcome here; it would make the behavior of the command and the option even less predictable.
Re: [PATCH 2/2] branch: protect local upstreams from -d
Hmm, it makes sense what you are saying, but I could have sworn that you asked for this (likely some very different version from this) when I was working on delete-merged, i.e. to extend branch protections to '-d' as well. Phillip brought it up recently as well. Maybe I misunderstood.
Harald
Re: [PATCH 2/2] branch: protect local upstreams from -d
Quoted 8 lines · Harald Nordgren
Hmm, it makes sense what you are saying, but I could have sworn that you asked for this (likely some very different version from this) when I was working on delete-merged, i.e. to extend branch protections to '-d' as well. Phillip brought it up recently as well. Maybe I misunderstood.
Harald
In <[email protected]> Junio wrote:
} [...]
}
} Do we also need the same safety around "git branch -d feature1" by } the way? The "-d" option with safety checks the same "is feature1 } already merged (to its upstream)?" condition, so it can protect the } feature2 branch the same way, by saying either "oops, you cannot } delete feature1 because you still have other branches like feature2 } that depend on it", or "ok, featur2 used to depend on feature1, but } because we are deleting feature1 based on it being in origin/master, } we will make feature2 depend on origin/master from now on". }
Harald, perhaps that's the passage you thought of?
Signature · 1 line
Tuomas
Re: [PATCH 2/2] branch: protect local upstreams from -d
Quoted 11 lines
} Do we also need the same safety around "git branch -d feature1" by } the way? The "-d" option with safety checks the same "is feature1 } already merged (to its upstream)?" condition, so it can protect the } feature2 branch the same way, by saying either "oops, you cannot } delete feature1 because you still have other branches like feature2 } that depend on it", or "ok, featur2 used to depend on feature1, but } because we are deleting feature1 based on it being in origin/master, } we will make feature2 depend on origin/master from now on". }
Harald, perhaps that's the passage you thought of?
Yes, exactly. Thanks for finding it!
Harald
Re: [PATCH 2/2] branch: protect local upstreams from -d
In reply to Harald NordgrenQuoted 5 lines · Harald Nordgren
Hmm, it makes sense what you are saying, but I could have sworn that you asked for this (likely some very different version from this) when I was working on delete-merged, i.e. to extend branch protections to '-d' as well. Phillip brought it up recently as well. Maybe I misunderstood.
It is more likely that I misstated. I do appreciate that we now have a machinery that allows us to offer an "improved" protection feature that may be "better" than 'branch -d' to users. It is a different matter in what shape we offer the feature while balancing the need to avoid breaking established end-user workflow.
Thanks for working on this topic.
Re: [PATCH 2/2] branch: protect local upstreams from -d
In reply to Harald Nordgren via GitGitGadgetQuoted 10 lines · Harald Nordgren via GitGitGadget
A local branch may be fully merged into its own upstream while still serving as the base of a surviving stacked branch. Deleting it with "git branch -d" then leaves the surviving branch with a missing upstream.
Use the existing stacked-branch protection after checking every requested deletion. This makes multi-branch deletion independent of argument order: a branch that fails its safety check remains available to protect its upstream.
[...]
Documentation/git-branch.adoc OPTIONS · line 107
Quoted 12 lines
Documentation/git-branch.adoc +3
-1@@ -102,7 +102,9 @@ OPTIONS `--delete`:: Delete a branch. The branch must be fully merged in its upstream branch, or in `HEAD` if no upstream was set with - `--track` or `--set-upstream-to`. + `--track` or `--set-upstream-to`, and must not be an upstream, + directly or indirectly, of another local branch that will remain + after the operation.
With this patch applied:
$ git init -q repo && cd repo $ git commit --allow-empty -m base [master (root-commit) b9a0882] base $ git branch A $ git branch B $ git branch C $ git branch --set-upstream-to=A B branch 'B' set up to track 'A'. $ git branch --set-upstream-to=B C branch 'C' set up to track 'B'. $ ~/floss/git-review/bin-wrappers/git branch -d A B error: the branch 'B' is an upstream of another branch hint: If you are sure you want to delete it, run 'git branch -D B' hint: Disable this message with "git config set advice.forceDeleteBranch false" Deleted branch A (was b9a0882).
So, C had B as an upstream and git did protect B from being deleted. That matches the claims above.
However, B had A as an upstream and git didn't protect A; it deleted it. That doesn't match the claims above.
Verifying:
$ git config branch.B.merge refs/heads/A $ git rev-parse -q --verify refs/heads/A ; echo $? 1 $ git rev-parse -q --verify refs/heads/B ; echo $? b9a088270710b2494f5fa0668fc7e81a40aebd35 0