Re: [PATCH v4] branch: add flags and config to inherit tracking

From: Glen Choo <chooglen@google.com>
Date: Nov 17, 2021, 00:33 UTC
Message-ID: <kl6lczmzipsf.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <7ad7507f183332cb2b5fdf2eb76fbbc9dd7199ef.1637085915.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

> I've addressed Glen's feedback from V3. However, this brings up a new
> issue that was not obvious before: "branch.<name>.merge" can be
> specified more than once. On the other hand, the existing tracking setup
> code supports only a single merge entry.

Yes, for istance, install_branch_config() uses git_config_set_gently(), which will override duplicate values.

> For now I'm defaulting to use the first merge entry listed in the
> branch struct, but I'm curious what people think the best solution
> would be.

I can think of at least two possibilities:

The first would be to parse the information into our native data structures. This is pretty much what you've done in v4, but insteaed of defaulting to the first merge entry, we would iterate over all of the possible merge entries and...

Show 11 quoted lines

> @@ -139,7 +166,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  
>  	memset(&tracking, 0, sizeof(tracking));
>  	tracking.spec.dst = (char *)orig_ref;
> -	if (for_each_remote(find_tracked_branch, &tracking))
> +	if (track != BRANCH_TRACK_INHERIT) {
> +		for_each_remote(find_tracked_branch, &tracking);
> +	} else if (inherit_tracking(&tracking, orig_ref))
>  		return;
>  
>  	if (!tracking.matches)

we get rid of the assumption that we can use a single 'struct tracking'. when track=BRANCH_TRACK_INHERIT. Of course, this isn't as simple as calling install_branch_config() repeatedly, because that would override "branch.<name>.merge" over and over.

> This may be another point in favor of Ævar's suggestion to
> reuse the copy-branch-config machinery.

This is the second option, which is pretty simple. Inheriting the branch tracking info is a matter of copying the config, which we already do when we copy branches in builtin/branch.c:

	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
	strbuf_release(&oldref);
	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
	strbuf_release(&newref);
	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
		die(_("Branch is renamed, but update of config-file failed"));

Between these two options, I think the first is a better long-term solution because I think that parsing the config into our own data structures is generally less error-prone than operating directly on a file (e.g. using the data structures was what made this bug obvious to us in the first place, using repo->config will handle multiple config files correctly). I don't see '--track=inherit' as being _that_ conceptually similar to copying a branch; I see it as a different mode of tracking that just so happens to be implementable by copying some sections in the branch configuration.

But as a practical matter, I don't see any obviously terrible short-term downsides to just copying the config. It's no less correct than our branch copying logic and I'm afaid of introducing unintended consequences by mucking around with install_branch_config().