Re: [PATCH v10 2/4] branch: add --prune-merged <branch>

[Junio C Hamano <gitster@pobox.com>]

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

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 1e24c95a69..29d38e9060 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -172,8 +174,8 @@ static int branch_merged(int kind, const char *name,
>  	 * any of the following code, but during the transition period,
>  	 * a gentle reminder is in order.
>  	 */
> -	if (head_rev != reference_rev) {
> -		int expect = head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0;
> +	if (head_rev && head_rev != reference_rev) {
> +		int expect = repo_in_merge_bases(the_repository, rev, head_rev);

Any caller who passed head_rev==NULL still used to come into this
block, set expect to 0, and have it compared with merged.  If merged
is 0 (which is what happens when reference_rev is NULL and head_rev
is NULL), the control left this block silently due to /* okay */
below (which is in the post-context that is not shown).

The updated code refuses control to come into this block when
head_rev is NULL.  I am not sure why the change in this hunk is
needed.

> @@ -748,6 +750,25 @@ static int collect_forked_branch(const struct reference *ref, void *cb_data)
>  	return 0;
>  }
>  
> +static int collect_default_branch_name(struct remote *remote, void *cb_data)
> +{
> +	struct string_list *protected = cb_data;
> +	struct ref_store *refs = get_main_ref_store(the_repository);
> +	struct strbuf head = STRBUF_INIT;
> +	const char *target;
> +
> +	strbuf_addf(&head, "refs/remotes/%s/HEAD", remote->name);
> +	target = refs_resolve_ref_unsafe(refs, head.buf,
> +					 RESOLVE_REF_NO_RECURSE, NULL, NULL);
> +	if (target) {
> +		const char *leaf = strrchr(target, '/');
> +		if (leaf)
> +			string_list_insert(protected, leaf + 1);
> +	}
> +	strbuf_release(&head);
> +	return 0;
> +}

It is strange to assume that whatever upstream repository happens to
use as its primary branch name has anything to do with how the local
repository names its primary branch.  Shouldn't this instead use
init.defaultBranch configuration or something?

> @@ -781,6 +802,63 @@ static int list_forked_branches(int argc, const char **argv)
>  	return 0;
>  }
>  
> +static int prune_merged_branches(int argc, const char **argv, int quiet)
> +{
> +	struct ref_store *refs = get_main_ref_store(the_repository);
> +	struct string_list candidates = STRING_LIST_INIT_DUP;
> +	struct string_list protected_default_names = STRING_LIST_INIT_DUP;
> +	struct strvec deletable = STRVEC_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	struct string_list_item *item;
> +	int n_not_merged = 0;
> +	int ret = 0;
> +
> +	if (!argc)
> +		die(_("--prune-merged requires at least one <branch>"));
> +
> +	collect_forked_set(argc, argv, &candidates);

Due to poor separation of changes, all the necessary information to
assess how sane the above code is is hidden in [1/4] and not here.

> +	for_each_remote(collect_default_branch_name, &protected_default_names);

This looks inefficient, and worse, incorrect.

If we have multiple branches in argv[], they may have come from
different upstream, and because you pay attention to what the
refs/remotes/<upstream>/HEAD symrefs point at, you have multiple
such "protected" default names.  You'd compare each and every
candidates against these names using linear search in the
string_list to see if they are protected (inefficient), and you
reject removal of argv[1] even when it happens to be the same name
as the default in the repository from which the upstream of argv[3]
came from, i.e., has no relationship with argv[1] (incorrect).

> +	for_each_string_list_item(item, &candidates) {
> +		const char *short_name = item->string;
> +		const char *upstream = item->util;
> +
> +		strbuf_reset(&buf);
> +		strbuf_addf(&buf, "refs/heads/%s", short_name);
> +		if (branch_checked_out(buf.buf))
> +			continue;
> +
> +		if (string_list_has_string(&protected_default_names,
> +					   short_name))
> +			continue;
> +
> +		if (!refs_ref_exists(refs, upstream))
> +			continue;
> +
> +		strvec_push(&deletable, short_name);
> +	}
> +	strbuf_release(&buf);
> +
> +	if (deletable.nr)
> +		ret = delete_branches(deletable.nr, deletable.v,
> +				      0, FILTER_REFS_BRANCHES, quiet,
> +				      1, &n_not_merged);

Add comments on parameters, perhaps, to make it readable?

		ret = delete_branches(deletable.nr, deletable.v,
				      0, /* no force */
				      FILTER_REFS_BRANCHES,
				      quiet,
				      1, /* warn only */
				      &n_not_merged);

or something?

Unfortunately the body of delete_branches() updated by this series
is not visible in this patch, making a sensible review impossible,
but if I recall correctly from what I saw in [1/4], with no-force
set, it does not leave head_rev NULL, and instead reads the HEAD
into it, and that is eventually passed to check_branch_commit()
call.

Now, check_branch_commit() passes head_rev to branch_merged(), which
falls back to "HEAD" when head_rev is given.  That sounds incorrect
for at least two reasons.  We are only dealing with branches that do
have upstream, so fallback should not trigger and we shouldn't be
allowing head_rev to be passed.  It _might_ be debatable that it is
prudent to still expect branch_merged() not to find the upstream to
detect errors in the logic, but falling back to head_rev in such a
case does not make any sense.

> +	if (n_not_merged && !quiet)
> +		fprintf(stderr,
> +			Q_("Skipped %d branch that is not fully merged; "
> +			   "delete it with 'git branch -D' if you are sure.\n",
> +			   "Skipped %d branches that are not fully merged; "
> +			   "delete them with 'git branch -D' if you are sure.\n",
> +			   n_not_merged),
> +			n_not_merged);

I do not think we unconditionally want to see this, and "--quiet"
shouldn't be the onlyl way to squelch this message.

When !quiet, the warn_only call to check_branch_commit() would
already have reported which branches are not fully merged, and
after seeing this message a few times, even the most novice user
would know how to use "git branch -D" to remove unneeded branches.

Use of advice_if_enabled() may make it more palatable.