Re: [PATCH v14 2/6] branch: let delete_branches warn instead of error on bulk refusal

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Harald

On 09/06/2026 11:11, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> 
> Add a warn-only mode to delete_branches() and check_branch_commit()
> so a bulk caller can report branches that are not fully merged as a
> short warning and carry on, rather than erroring with the longer
> "use 'git branch -D'" advice that the plain "git branch -d" path
> emits. Existing callers are unaffected.

There is no mention here of the conversion to use a flags argument which 
should be a separate preparatory commit
  > @@ -189,20 +189,33 @@ static int branch_merged(int kind, const char 
*name,
>   	return merged;
>   }
>   
> +enum delete_branch_flags {
> +	DELETE_BRANCH_FORCE = (1 << 0),
> +	DELETE_BRANCH_QUIET = (1 << 1),
> +	DELETE_BRANCH_WARN_ONLY = (1 << 2),
> +};
> +
>   static int check_branch_commit(const char *branchname, const char *refname,
>   			       const struct object_id *oid, struct commit *head_rev,
> -			       int kinds, int force)
> +			       int kinds, unsigned int flags)
>   {
> +	int force = flags & DELETE_BRANCH_FORCE;

This is missing "!!" to keep the value the same (alternatively we could 
perhaps convert "force" to a bool, though I haven't looked too closely 
at how it is used in the rest of the function). Apart from that this 
good, unlike the conversion below it means the rest of the function sees 
the same variables in the same state as it did before the conversion. It 
would be a good idea to follow this pattern for the new flag.

	bool warn = flags & DELETE_BRANCH_WARN_ONLY;

and then just use "warn" later. It is a common pattern in our code to 
take a flags argument and split it out into various boolean variables at 
the start of the function to avoid a lot of awkward bit masks in the 
main body of the function.

> @@ -217,8 +230,8 @@ static void delete_branch_config(const char *branchname)
>   	strbuf_release(&buf);
>   }
>   
> -static int delete_branches(int argc, const char **argv, int force, int kinds,
> -			   int quiet)
> +static int delete_branches(int argc, const char **argv, int kinds,
> +			   unsigned int flags)
>   {
>   	struct commit *head_rev = NULL;
>   	struct object_id oid;
> @@ -227,6 +240,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>   	int i;
>   	int ret = 0;
>   	int remote_branch = 0;
> +	int force, quiet;

We should initialize these here using flags so that the rest of the code 
sees the same variables and values as it did before the conversion.

>   	struct strbuf bname = STRBUF_INIT;
>   	enum interpret_branch_kind allowed_interpret;
>   	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> @@ -241,7 +255,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>   		remote_branch = 1;
>   		allowed_interpret = INTERPRET_BRANCH_REMOTE;
>   
> -		force = 1;
> +		flags |= DELETE_BRANCH_FORCE;
>   		break;
>   	case FILTER_REFS_BRANCHES:
>   		fmt = "refs/heads/%s";
> @@ -252,12 +266,15 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>   	}
>   	branch_name_pos = strcspn(fmt, "%");
>   
> +	force = flags & DELETE_BRANCH_FORCE;
> +	quiet = flags & DELETE_BRANCH_QUIET;
> +
>   	if (!force)
>   		head_rev = lookup_commit_reference(the_repository, &head_oid);
>   
>   	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
>   		char *target = NULL;
> -		int flags = 0;
> +		int ref_flags = 0;

This is sensible so we don't shadow the new function argument but this 
added complexity is a good reason to split the flags change into its own 
commit before adding the warning flag.

I'll take a look at the other patches later this week - there is no need 
to send a new version before I've commented on the rest of the series.

Thanks

Phillip

>   
>   		copy_branchname(&bname, argv[i], allowed_interpret);
>   		free(name);
> @@ -279,7 +296,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>   					     RESOLVE_REF_READING
>   					     | RESOLVE_REF_NO_RECURSE
>   					     | RESOLVE_REF_ALLOW_BAD_NAME,
> -					     &oid, &flags);
> +					     &oid, &ref_flags);
>   		if (!target) {
>   			if (remote_branch) {
>   				error(_("remote-tracking branch '%s' not found"), bname.buf);
> @@ -291,7 +308,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>   									   | RESOLVE_REF_NO_RECURSE
>   									   | RESOLVE_REF_ALLOW_BAD_NAME,
>   									   &oid,
> -									   &flags);
> +									   &ref_flags);
>   				FREE_AND_NULL(virtual_name);
>   
>   				if (virtual_target)
> @@ -306,16 +323,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>   			continue;
>   		}
>   
> -		if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
> +		if (!(ref_flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
>   		    check_branch_commit(bname.buf, name, &oid, head_rev, kinds,
> -					force)) {
> -			ret = 1;
> +					flags)) {
> +			if (!(flags & DELETE_BRANCH_WARN_ONLY))
> +				ret = 1;
>   			goto next;
>   		}
>   
>   		item = string_list_append(&refs_to_delete, name);
> -		item->util = xstrdup((flags & REF_ISBROKEN) ? "broken"
> -				    : (flags & REF_ISSYMREF) ? target
> +		item->util = xstrdup((ref_flags & REF_ISBROKEN) ? "broken"
> +				    : (ref_flags & REF_ISSYMREF) ? target
>   				    : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV));
>   
>   	next:
> @@ -872,7 +890,9 @@ int cmd_branch(int argc,
>   	if (delete) {
>   		if (!argc)
>   			die(_("branch name required"));
> -		ret = delete_branches(argc, argv, delete > 1, filter.kind, quiet);
> +		ret = delete_branches(argc, argv, filter.kind,
> +				      (delete > 1 ? DELETE_BRANCH_FORCE : 0) |
> +				      (quiet ? DELETE_BRANCH_QUIET : 0));
>   		goto out;
>   	} else if (show_current) {
>   		print_current_branch_name();