Re: [PATCH 1/4] branch: support more tracking modes when recursing

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/branch.c b/branch.c
> index 6b31df539a5..7377b9f451a 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -233,6 +233,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
>  
> +	if (!track)
> +		BUG("asked to set up tracking, but tracking is disallowed");

I am wondering if this wants to be

	if (track == BRANCH_TRACK_NEVER)

instead.  Do we elsewhere rely on the fact that NEVER is assigned 0?

> @@ -534,8 +537,27 @@ static int submodule_create_branch(struct repository *r,
>  		strvec_push(&child.args, "--quiet");
>  	if (reflog)
>  		strvec_push(&child.args, "--create-reflog");
> -	if (track == BRANCH_TRACK_ALWAYS || track == BRANCH_TRACK_EXPLICIT)
> -		strvec_push(&child.args, "--track");
> +
> +	switch (track) {
> +	case BRANCH_TRACK_NEVER:
> +		strvec_push(&child.args, "--no-track");
> +		break;
> +	case BRANCH_TRACK_ALWAYS:
> +	case BRANCH_TRACK_EXPLICIT:
> +		strvec_push(&child.args, "--track=direct");
> +		break;
> +	case BRANCH_TRACK_OVERRIDE:
> +		BUG("BRANCH_TRACK_OVERRIDE cannot be used when creating a branch.");
> +		break;
> +	case BRANCH_TRACK_INHERIT:
> +		strvec_push(&child.args, "--track=inherit");
> +		break;

OK.

> +	case BRANCH_TRACK_UNSPECIFIED:
> +		/* Default for "git checkout". No need to pass --track. */
> +	case BRANCH_TRACK_REMOTE:
> +		/* Default for "git branch". No need to pass --track. */
> +		break;

Is that "no need to pass", or "no need to, and it will be detrimental to, pass"?

IOW, if we are relying on the command spawned via start_command()
interface to read and honor the configured default for themselves,
then passing explicit --track=whatever from this caller will be not
just necessary but is wrong, right?  I am worried about "No need to"
tempting "helpful" developers into doing unnecessary things, just to
be more explicit, for example. 

> @@ -614,7 +636,8 @@ void create_branches_recursively(struct repository *r, const char *name,
>  	 * tedious to determine whether or not tracking was set up in the
>  	 * superproject.
>  	 */
> -	setup_tracking(name, tracking_name, track, quiet);
> +	if (track)
> +		setup_tracking(name, tracking_name, track, quiet);

Here we do rely on the fact that NEVER has the value of 0.  If there
are other instances of code elsewhere that does so, then this one
and the other one at the top of this message are both fine.

Given that we started simple and then gradually added more features,
I would not be surprised if the older code written back when there
were only 0 (no track) and 1 (track) assumed 0 means no.  There is
one in create_branch() where we do

	if (real_ref && track)
		setup_tracking(ref.buf + 11, real_ref, track, quiet);

which also relies on the fact that NEVER is 0.

> -		OPT_SET_INT('t', "track", &track,
> -			    N_("set up tracking mode (see git-pull(1))"),
> -			    BRANCH_TRACK_EXPLICIT),
> +		OPT_CALLBACK_F('t', "track",  &track, "(direct|inherit)",
> +			N_("set branch tracking configuration"),
> +			PARSE_OPT_OPTARG,
> +			parse_opt_tracking_mode),

Hmph, this is quite curious.  How did the whole thing even worked
without this?

Ah, OK, this is in submodule--helper.c and tracking specification in
the top-level were OK.  Just that we forgot to correctly pass it
down when calling down to submodules.  Makes sense.

Thanks.