On 8/22/2022 2:20 PM, Junio C Hamano wrote:

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e376d547ce0..4280af6992e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -387,6 +387,8 @@ include::config/branch.txt[]
 
 include::config/browser.txt[]
 
+include::config/bundle.txt[]
+

The file that records a list of bundles may borrow the format of git
config files, but will we store their contents in configuration
files in the receiving (or originating) repository?  With the
presence of fields like "bundle.version", I somehow doubt it.

Should "git config --help" list them?

I suppose that at this point, they should be left out, since
writing them to your Git config does nothing.

In the future, having these config values present will advertise
the bundle list during the 'bundle-uri' protocol v2 command. That
could use some clarification in the documentation, too, perhaps
with a "bundle.*" item discussing how all of the other items are
related to that advertisement.

quoted

+/**
+ * Given a key-value pair, update the state of the given bundle list.
+ * Returns 0 if the key-value pair is understood. Returns 1 if the key
+ * is not understood or the value is malformed.

Let's stick to the "error is negative" if we do not have a strong
reason not to.

Right. Can do.
 

quoted

+ */
+MAYBE_UNUSED
+static int bundle_list_update(const char *key, const char *value,
+			      struct bundle_list *list)
+{
+	const char *pkey, *dot;
+	struct strbuf id = STRBUF_INIT;
+	struct remote_bundle_info lookup = REMOTE_BUNDLE_INFO_INIT;
+	struct remote_bundle_info *bundle;
+
+	if (!skip_prefix(key, "bundle.", &pkey))
+		return 1;
+	dot = strchr(pkey, '.');
+	if (!dot) {
+		if (!strcmp(pkey, "version")) {
+			int version = atoi(value);

Can atoi() safely fail?  Are we happy of pkey that says "1A" and we
parse it as "1"?

quoted

+			if (version != 1)
+				return 1;
+
+			list->version = version;
+			return 0;
+		}

Is it OK for a bundle list described in the config-file format to
have "bundle.version" twice, giving different values?  It feels
counter-intuitive to apply the "last one wins" rule that is usual
for configuration files.

...

This explicitly implements "the last one wins".  Would it really
make sense for a server to serve a bundle list that says redundant
and wasteful pieces of information, i.e.

    [bundle "1"]
	url = one
	url = two

It is not like doing so would allow us to reuse an otherwise mostly
good file by appending new information and that would be a performance
or storage win.  So I am not quite sure why we want "the last one wins"
rule here.  It instead looks like something we want to sanity check
and complain about.

I could switch this to "expect at most one value" and add warnings for
duplicate keys. Should duplicate keys then mean "the bundle list is
malformed, abort downloading bundles"? That seems reasonable to me.

Thanks,
-Stolee