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

[Glen Choo <chooglen@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...

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