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

From: Glen Choo <chooglen@google.com>
Date: Mar 30, 2022, 23:29 UTC
Message-ID: <kl6l4k3ft3y3.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqee2j6t5l.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 18 quoted lines

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

As you discussed here...

Show 9 quoted lines

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

We know the answer is "yes there is older code that relies on NEVER being 0". I believe this is the only instance though, which means this patch comprises the majority of instances of "if (!track)", so we can change it if you prefer. The older code is pretty old after all - the enum was introduced in 9ed36cfa35 (branch: optionally setup branch.*.merge from upstream local branches, 2008-02-19).

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

Hm, interesting, I hadn't considered that temptation. This is the latter, i.e. it is not correct to pass --track. I'll reword it for clarity, something like "Should not pass --track".

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

Yes, that's correct. This was missed because I only added tests for --track and the default case (and didn't add tests for --track=inherit or --no-track.