Re: [PATCH v10 1/4] branch: add --forked <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 1572a4f9ef..1e24c95a69 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -28,6 +28,7 @@
>  #include "help.h"
>  #include "advice.h"
>  #include "commit-reach.h"
> +#include "wildmatch.h"
>  
>  static const char * const builtin_branch_usage[] = {
>  	N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"),
> @@ -38,6 +39,7 @@ static const char * const builtin_branch_usage[] = {
>  	N_("git branch [<options>] (-c | -C) [<old-branch>] <new-branch>"),
>  	N_("git branch [<options>] [-r | -a] [--points-at]"),
>  	N_("git branch [<options>] [-r | -a] [--format]"),
> +	N_("git branch [<options>] --forked <branch>..."),
>  	NULL
>  };
>  
> @@ -191,7 +193,8 @@ static int branch_merged(int kind, const char *name,
>  
>  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, int force, int warn_only,
> +			       int *n_not_merged)
>  {
>  	struct commit *rev = lookup_commit_reference(the_repository, oid);
>  	if (!force && !rev) {
> @@ -199,10 +202,18 @@ static int check_branch_commit(const char *branchname, const char *refname,
>  		return -1;
>  	}
>  	if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
> -		error(_("the branch '%s' is not fully merged"), branchname);
> -		advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
> -				  _("If you are sure you want to delete it, "
> -				  "run 'git branch -D %s'"), branchname);
> +		if (warn_only) {
> +			warning(_("the branch '%s' is not fully merged"),
> +				branchname);
> +		} else {
> +			error(_("the branch '%s' is not fully merged"),
> +			      branchname);
> +			advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
> +					  _("If you are sure you want to delete it, "
> +					  "run 'git branch -D %s'"), branchname);
> +		}
> +		if (n_not_merged)
> +			(*n_not_merged)++;
>  		return -1;
>  	}
>  	return 0;

This function originally was to see if "git branch -d <derived>" is
allowed to remove it by calling branch_merged(), which uses the
upstream association of <derived>, falling back to head_rev.  For
our purpose of "--forked" check, we are not deleting, so existing
calls to error() and the phrasing used in its messages that talks
about deletion are not appropriate.  Hence we introduce the
warn_only mode to just state the fact the branch is not fully merged
to its upstream.

It is inherited from the original code, and it is outside the scope
of this series, but after the dust settles, we may want to consider
saying what other branch this branch is expected to be merged to
(i.e. "not fully merged to %s").

It is unclear how n_not_merged would be useful.  Wouldn't the caller
be capable to count the failure returns from this function?

> @@ -218,7 +229,7 @@ static void delete_branch_config(const char *branchname)
>  }
>  
>  static int delete_branches(int argc, const char **argv, int force, int kinds,
> -			   int quiet)
> +			   int quiet, int warn_only, int *n_not_merged)

It is strange to see any need to touch delete_branches() when the
only thing you are adding is the "--forked" option.  For that
matter, the change to check_branch_commit() is also suspect.

In short, this does a lot more than necessary to add "--forked".
Why such a split in the series?

> +static void parse_forked_args(int argc, const char **argv,
> +			      struct string_list *upstream_patterns)
> +{
> +	int i;
> +
> +	for (i = 0; i < argc; i++) {
> +		const char *arg = argv[i];
> +		struct object_id oid;
> +		char *full_ref = NULL;
> +		const char *short_ref;
> +
> +		if (has_glob_specials(arg)) {
> +			string_list_insert(upstream_patterns, arg);
> +			continue;
> +		}
> +
> +		if (repo_dwim_ref(the_repository, arg, strlen(arg), &oid,
> +				  &full_ref, 0) == 1 &&
> +		    (skip_prefix(full_ref, "refs/heads/", &short_ref) ||
> +		     skip_prefix(full_ref, "refs/remotes/", &short_ref))) {
> +			string_list_insert(upstream_patterns, short_ref);
> +			free(full_ref);
> +			continue;
> +		}
> +		free(full_ref);
> +
> +		die(_("'%s' is not a valid branch or pattern"), arg);
> +	}
> +}

This one does look like very much relevant to "--forked".

> +struct forked_cb {
> +	const struct string_list *upstream_patterns;
> +	struct string_list *out;
> +};

So does this.

> +static int collect_forked_branch(const struct reference *ref, void *cb_data)
> +{
> +	struct forked_cb *cb = cb_data;
> +	struct branch *branch;
> +	const char *upstream, *short_upstream;
> +	const struct string_list_item *item;
> +
> +	if (ref->flags & REF_ISSYMREF)
> +		return 0;
> +	branch = branch_get(ref->name);
> +	if (!branch)
> +		return 0;
> +	upstream = branch_get_upstream(branch, NULL);
> +	if (!upstream)
> +		return 0;
> +	short_upstream = upstream;
> +	(void)(skip_prefix(short_upstream, "refs/heads/", &short_upstream) ||
> +	       skip_prefix(short_upstream, "refs/remotes/", &short_upstream));
> +
> +	for_each_string_list_item(item, cb->upstream_patterns)

Sholdn't each element of upstream_patterns remember if it is
suitable for wildmatch or not when it gets parsed in the earlier
function we saw, so that this loop can refrain from calling
wildmatch() for non-wildcard elements?

> +		if (!wildmatch(item->string, short_upstream, WM_PATHNAME)) {
> +			string_list_append(cb->out, ref->name)->util =
> +				xstrdup(upstream);
> +			return 0;
> +		}
> +	return 0;
> +}
> +
> +static void collect_forked_set(int argc, const char **argv,
> +			       struct string_list *out)
> +{
> +	struct string_list upstream_patterns = STRING_LIST_INIT_DUP;
> +	struct forked_cb cb = {
> +		.upstream_patterns = &upstream_patterns,
> +		.out = out,
> +	};
> +
> +	parse_forked_args(argc, argv, &upstream_patterns);
> +
> +	refs_for_each_branch_ref(get_main_ref_store(the_repository),
> +				 collect_forked_branch, &cb);
> +
> +	string_list_clear(&upstream_patterns, 0);
> +}
> +
> +static int list_forked_branches(int argc, const char **argv)
> +{
> +	struct string_list out = STRING_LIST_INIT_DUP;
> +	struct string_list_item *item;
> +
> +	if (!argc)
> +		die(_("--forked requires at least one <branch>"));
> +
> +	collect_forked_set(argc, argv, &out);
> +	for_each_string_list_item(item, &out)
> +		puts(item->string);
> +
> +	string_list_clear(&out, 1);
> +	return 0;
> +}

Up to point the changes are very much appropriate for adding the
"--forked" option.

> @@ -714,6 +822,7 @@ int cmd_branch(int argc,
>  	/* possible actions */
>  	int delete = 0, rename = 0, copy = 0, list = 0,
>  	    unset_upstream = 0, show_current = 0, edit_description = 0;
> +	int forked = 0;
>  	const char *new_upstream = NULL;
>  	int noncreate_actions = 0;
>  	/* possible options */
> @@ -767,6 +876,8 @@ int cmd_branch(int argc,
>  		OPT_BOOL(0, "create-reflog", &reflog, N_("create the branch's reflog")),
>  		OPT_BOOL(0, "edit-description", &edit_description,
>  			 N_("edit the description for the branch")),
> +		OPT_BOOL(0, "forked", &forked,
> +			N_("list local branches whose upstream matches the given <branch>...")),
>  		OPT__FORCE(&force, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE),
>  		OPT_MERGED(&filter, N_("print only branches that are merged")),
>  		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
> @@ -811,7 +922,7 @@ int cmd_branch(int argc,
>  			     0);
>  
>  	if (!delete && !rename && !copy && !edit_description && !new_upstream &&
> -	    !show_current && !unset_upstream && argc == 0)
> +	    !show_current && !unset_upstream && !forked && argc == 0)
>  		list = 1;
>  
>  	if (filter.with_commit || filter.no_commit ||
> @@ -820,7 +931,7 @@ int cmd_branch(int argc,
>  
>  	noncreate_actions = !!delete + !!rename + !!copy + !!new_upstream +
>  			    !!show_current + !!list + !!edit_description +
> -			    !!unset_upstream;
> +			    !!unset_upstream + !!forked;
>  	if (noncreate_actions > 1)
>  		usage_with_options(builtin_branch_usage, options);
>  
> @@ -858,7 +969,11 @@ 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, delete > 1, filter.kind,
> +				      quiet, 0, NULL);
> +		goto out;

This does not belong to "--forked" as far as I can tell.

> +	} else if (forked) {
> +		ret = list_forked_branches(argc, argv);
>  		goto out;
>  	} else if (show_current) {
>  		print_current_branch_name();