[PATCH 0/2] branch: -d protects upstream branches

[PATCH 0/2] branch: -d protects upstream branches

[Harald Nordgren via GitGitGadget]

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
-- 
gitgitgadget

[PATCH 1/2] branch: move stacked branch helpers

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

Move the stacked branch helpers earlier so delete_branches() can use
them without a forward declaration.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 builtin/branch.c | 104 +++++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a613148fc7..87f0aa4051 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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 unset
 	return 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)
 {
-- 
gitgitgadget

[PATCH 2/2] branch: protect local upstreams from -d

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

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.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 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(-)

diff --git a/Documentation/git-branch.adoc b/Documentation/git-branch.adoc
index bfdf459329..5c2a3339b2 100644
--- a/Documentation/git-branch.adoc
+++ b/Documentation/git-branch.adoc
@@ -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`.
diff --git a/builtin/branch.c b/builtin/branch.c
index 87f0aa4051..7f76789027 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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);
diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index cb9ef7e329..04abfb6f94 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -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
 '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cdb6c6a634..a3d492ffcd 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -2173,6 +2173,49 @@ test_expect_success "branch -d still deletes a deleteMerged=false branch" '
 	)
 '
 
+test_expect_success 'branch -d keeps the upstream of a surviving branch' '
+	setup_repo_for_delete_merged &&
+	(
+		cd repo &&
+		git branch foundation origin/next --track &&
+		git checkout -b topic foundation --track &&
+		git commit --allow-empty -m "topic work" &&
+		git checkout --detach &&
+
+		test_must_fail git branch -d foundation 2>err &&
+		test_grep "branch .foundation. is an upstream of another branch" err &&
+		test_ref_exists refs/heads/foundation &&
+		test_ref_exists refs/heads/topic &&
+
+		git branch -D foundation &&
+		test_ref_missing refs/heads/foundation &&
+		test_ref_exists refs/heads/topic
+	)
+'
+
+test_expect_success 'branch -d protects a base when another deletion fails' '
+	setup_repo_for_delete_merged &&
+	(
+		cd repo &&
+		git branch foundation origin/next --track &&
+		git checkout -b topic foundation --track &&
+		git commit --allow-empty -m "topic work" &&
+		git checkout --detach &&
+
+		test_must_fail git branch -d foundation topic 2>err &&
+		test_grep "branch .foundation. is an upstream of another branch" err &&
+		test_grep "branch .topic. is not fully merged" err &&
+		test_ref_exists refs/heads/foundation &&
+		test_ref_exists refs/heads/topic &&
+
+		test_must_fail git branch -d topic foundation 2>err &&
+		test_grep "branch .foundation. is an upstream of another branch" err &&
+		test_grep "branch .topic. is not fully merged" err &&
+		test_ref_exists refs/heads/foundation &&
+		test_ref_exists refs/heads/topic
+	)
+'
+
 test_expect_success '--dry-run without --delete-merged is rejected' '
 	test_must_fail git -C forked branch --dry-run 2>err &&
 	test_grep "requires --delete-merged" err
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index e95d420972..01145f6681 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -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 &&
-- 
gitgitgadget

Re: [PATCH 2/2] branch: protect local upstreams from -d

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> 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.

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...

> -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

[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

Re: [PATCH 2/2] branch: protect local upstreams from -d

[Tuomas Ahola]

Harald Nordgren <haraldnordgren@gmail.com> wrote:

> 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 <xmqq33yimsdp.fsf@gitster.g> 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?

-- 
Tuomas

Re: [PATCH 2/2] branch: protect local upstreams from -d

[Harald Nordgren]

> } 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

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

> 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

[Elijah Newren]

On Tue, Aug 25, 2026 at 2:30 PM Harald Nordgren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.
[...]
> diff --git a/Documentation/git-branch.adoc b/Documentation/git-branch.adoc
> index bfdf459329..5c2a3339b2 100644
> --- a/Documentation/git-branch.adoc
> +++ b/Documentation/git-branch.adoc
> @@ -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