Re: [PATCH v5 0/2] branch: inherit tracking configs

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Josh Steadmon <steadmon@google.com>,
	git@vger.kernel.org, chooglen@google.com,
	emilyshaffer@google.com, avarab@gmail.com
Subject: Re: [PATCH v5 0/2] branch: inherit tracking configs
Date: Fri, 10 Dec 2021 23:48:59 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2112102344370.90@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqq4k7k8cz3.fsf@gitster.g>

Hi,

On Tue, 7 Dec 2021, Junio C Hamano wrote:

> Josh Steadmon <steadmon@google.com> writes:
>
> > I've addressed feedback from V4. Since 2/3 reviewers seemed to (at least
> > slightly) prefer handling multiple upstream branches in the existing
> > tracking setup, I've gone that direction rather than repurposing the
> > branch copy code. None of the other issues were controversial.
> >
> > In this version, I'd appreciate feedback mainly on patch 1:
> > * Is the combination of `git_config_set_gently()` +
> >   `git_config_set_multivar_gently() the best way to write multiple
> >   config entries for the same key?
>
> IIRC git_config_set_*() is Dscho's brainchild.  If he is available
> to comment, it may be a valuable input.

The `git_config_set_multivar_gently()` function was really only intended
to add one key/value pair.

Currently, there is no function to add multiple key/value pairs, and while
it is slightly wasteful to lock the config multiple times to write a bunch
of key/value pairs, it is not the worst in the world for a small use case
like this one.

So yes, for the moment I would go with the suggested design.

One thing you might want to do is to avoid the extra
`git_config_set_gently()` before the `for` loop, simply by passing `i == 0
? 0 : CONFIG_FLAGS_MULTI_REPLACE` as `flags` parameter to the multivar
version of the function.

But that would optimize for code size rather than for readability, and I
would actually prefer the more verbose version.

Ciao,
Dscho