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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2022, 21:13 UTC
Message-ID: <xmqqee2j6t5l.fsf@gitster.g>
In-Reply-To: <0b682c173c8cfa7f49ba17b2d71049ac702ec747.1648584079.git.gitgitgadget@gmail.com>

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

Show 10 quoted lines

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

Show 21 quoted lines

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

Show 5 quoted lines

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

Show 7 quoted lines

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

Show 7 quoted lines

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