{"thread":{"id":"60270","subject":"[PATCH] setup: Only allow extenions.objectFormat to be specified once","startedAt":"2023-09-26T16:01:06Z","lastAt":"2023-09-27T19:57:38Z","messageCount":4,"participants":["Eric W. Biederman","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482344","messageId":"87h6ngapqb.fsf@gmail.froward.int.ebiederm.org","threadId":"60270","inReplyTo":null,"subject":"[PATCH] setup: Only allow extenions.objectFormat to be specified once","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2023-09-26T16:01:00Z","receivedAt":"2023-09-26T16:01:06Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"\nToday there is no sanity checking of what happens when\nextensions.objectFormat is specified multiple times. Catch confused git\nconfigurations by only allowing this option to be specified once.\n\nSigned-off-by: \"Eric W. Biederman\" \n---\n setup.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 18927a847b86..ef9f79b8885e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,\n \tif (!strcmp(ext, \"noop-v1\")) {\n \t\treturn EXTENSION_OK;\n \t} else if (!strcmp(ext, \"objectformat\")) {\n+\t\tstruct string_list_item *item;\n \t\tint format;\n \n \t\tif (!value)\n@@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,\n \t\tif (format == GIT_HASH_UNKNOWN)\n \t\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n \t\t\t\t \"extensions.objectformat\", value);\n+\t\t/* Only support objectFormat being specified once. */\n+\t\tfor_each_string_list_item(item, &data->v1_only_extensions) {\n+\t\t\tif (!strcmp(item->string, \"objectformat\"))\n+\t\t\t\treturn error(_(\"'%s' already specified as '%s'\"),\n+\t\t\t\t\t\"extensions.objectformat\",\n+\t\t\t\t\thash_algos[data->hash_algo].name);\n+\t\t}\n \t\tdata->hash_algo = format;\n \t\treturn EXTENSION_OK;\n \t}\n-- \n2.41.0\n\n"},{"id":"482362","messageId":"xmqqr0mkmx9b.fsf@gitster.g","threadId":"60270","inReplyTo":"87h6ngapqb.fsf@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-26T21:37:36Z","receivedAt":"2023-09-27T02:40:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric W. Biederman\" writes:\n\n> Today there is no sanity checking of what happens when\n> extensions.objectFormat is specified multiple times. Catch confused git\n> configurations by only allowing this option to be specified once.\n\nHmph. I am not sure if this is worth doing, and especially only for\n\"objectformat\". Do we intend to apply different rules other than\n\"you can give it only once\" to other extensions, and if so where\nwill these rules be catalogued? I do not see particular harm to let\nthem follow the usual \"last one wins\".\n\nIf the patch were about trying to make sure that extensions, which\nare inherentaly per-repository, appear only in $GIT_DIR/config and\ncomplain if the code gets confused and tried to read them from the\nsystem or global configuration files, I would understand, and\nstrongly support such an effort, ithough.\n\nThe real sanity we want to enforce is that what is reported by\nrunning \"git config extensions.objectformat\" must match the object\nformat that is used in refs and object database. Manually futzing\nthe configuration file and adding an entry with a contradictory\nvalue certainly is one way to break that sanity, and this patch may\ncatch such a breakage, but once we start worrying about manually\nfutzing the configuration file, the check added here would easily\nmiss if the futzing is done by replacing instead of adding, so I am\nnot sure if this extra code is worth its bits.\n\nBut perhaps I am missing something and not seeing why it is worth\ninsisting on \"last one is the first one\" for this particular one.\n\nThanks.\n\n> Signed-off-by: \"Eric W. Biederman\" \n> ---\n> setup.c | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/setup.c b/setup.c\n> index 18927a847b86..ef9f79b8885e 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,\n> \tif (!strcmp(ext, \"noop-v1\")) {\n> \t\treturn EXTENSION_OK;\n> \t} else if (!strcmp(ext, \"objectformat\")) {\n> +\t\tstruct string_list_item *item;\n> \t\tint format;\n> \n> \t\tif (!value)\n> @@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,\n> \t\tif (format == GIT_HASH_UNKNOWN)\n> \t\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n> \t\t\t\t \"extensions.objectformat\", value);\n> +\t\t/* Only support objectFormat being specified once. */\n> +\t\tfor_each_string_list_item(item, &data->v1_only_extensions) {\n> +\t\t\tif (!strcmp(item->string, \"objectformat\"))\n> +\t\t\t\treturn error(_(\"'%s' already specified as '%s'\"),\n> +\t\t\t\t\t\"extensions.objectformat\",\n> +\t\t\t\t\thash_algos[data->hash_algo].name);\n> +\t\t}\n> \t\tdata->hash_algo = format;\n> \t\treturn EXTENSION_OK;\n> \t}\n"},{"id":"482372","messageId":"87r0mjn4ly.fsf@gmail.froward.int.ebiederm.org","threadId":"60270","inReplyTo":"xmqqr0mkmx9b.fsf@gitster.g","subject":"Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2023-09-27T13:11:05Z","receivedAt":"2023-09-27T13:11:14Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"Junio C Hamano writes:\n\n> \"Eric W. Biederman\" writes:\n>\n>> Today there is no sanity checking of what happens when\n>> extensions.objectFormat is specified multiple times. Catch confused git\n>> configurations by only allowing this option to be specified once.\n>\n> Hmph. I am not sure if this is worth doing, and especially only for\n> \"objectformat\". Do we intend to apply different rules other than\n> \"you can give it only once\" to other extensions, and if so where\n> will these rules be catalogued? I do not see particular harm to let\n> them follow the usual \"last one wins\".\n>\n> If the patch were about trying to make sure that extensions, which\n> are inherentaly per-repository, appear only in $GIT_DIR/config and\n> complain if the code gets confused and tried to read them from the\n> system or global configuration files, I would understand, and\n> strongly support such an effort, ithough.\n\nUnless I misread something, the code only checks for [extensions]\nin $GIT_DIR/config.\n\n> The real sanity we want to enforce is that what is reported by\n> running \"git config extensions.objectformat\" must match the object\n> format that is used in refs and object database.\n\nAgreed. Allowing git config extensions.objectformat to change the\nexisting value is allowing the repository to be corrupted.\n\n> Manually futzing\n> the configuration file and adding an entry with a contradictory\n> value certainly is one way to break that sanity, and this patch may\n> catch such a breakage, but once we start worrying about manually\n> futzing the configuration file, the check added here would easily\n> miss if the futzing is done by replacing instead of adding, so I am\n> not sure if this extra code is worth its bits.\n>\n> But perhaps I am missing something and not seeing why it is worth\n> insisting on \"last one is the first one\" for this particular one.\n\nI somewhat have blinders on. There are 3 configuration options I am\nconcerned with:\n\nextensions.objectFormat\nextensions.compatObjectFormat\ncore.historicObjectFormat (or whatever name we settle on).\n\nOne key concern I heard expressed in earlier reviews is that however we\nhandle these options we handle them in such a way as to give ourselves\nroom to rise to challenges in the future.\n\nWhatever we do with parsing we have the following logical\nconstraints:\n\nFor extensions.objectFormat: There can only be a single storage hash.\n\nFor extensions.compatObjectFormat: There can be no compatibility hash,\nthere can be a single compatibility hash, and depending how things go\nbetween now and the next hash function transition we might want multiple\ncompatibility hashes.\n\nFor core.historicObjectFormat: There can be no historic hash function, there\ncan be a single historic hash function, there can be multiple historic\nhash functions.\n\n\nFor the compatibility hash I think it is unlikely we will want to\nsupport more than one compatibility hash in practice but I can imagine\na scenario where we just get into the transition from SHA-1 to SHA-256\nand a serious break is discovered that requires switching to FutureHash\nASAP.\n\nFor historic object formats like SHA-1 will become post transition there\nare references embedded in commit comments, email messages, bug\ntrackers. All kinds of places that we can not update so there is\nfundamentally a need to be able to find which current objects correspond\nto the historic names. For a project each hash function transition will\ncreate more such objects.\n\n\nWhen I looked I saw two ways within current git to specify a list of\nvalues for a single configuration option.\n- Give that option multiple times.\n- Parse the option value in such a way as to generate a list.\n\nIt is my sense just specifying the compatObjectFormat multiple times to\nspecify multiple compatibility object formats makes the most sense.\nEspecially as all is needed today is to only allow a single value.\n\nAfter I had implemented the only allow once logic for compatObjectFormat\nI saw that objectFormat had nothing similar, and knowing it is a bug\nfor multiple objectFormat wrote a patch to enforce only appear once\nfor objectFormat as well.\n\nFor objectFormat I don't care very much. For compatObjectFormat I truly\ncare, and for even more for the option that allows finding the current\nobject from a historic oid (even a truncated one) I care very much.\n\nFor me the fundamental question is if we allow multiples compatibility\nhashes or historical hashes how do we specify them? Have the option\nappear more than once? A comma separated list?\n\nWhatever we decided I want to enforce that doesn't appear in current\nconfigurations so we can support for multiples later.\n\nEric\n\n>> Signed-off-by: \"Eric W. Biederman\" \n>> ---\n>> setup.c | 8 ++++++++\n>> 1 file changed, 8 insertions(+)\n>>\n>> diff --git a/setup.c b/setup.c\n>> index 18927a847b86..ef9f79b8885e 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,\n>> \tif (!strcmp(ext, \"noop-v1\")) {\n>> \t\treturn EXTENSION_OK;\n>> \t} else if (!strcmp(ext, \"objectformat\")) {\n>> +\t\tstruct string_list_item *item;\n>> \t\tint format;\n>> \n>> \t\tif (!value)\n>> @@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,\n>> \t\tif (format == GIT_HASH_UNKNOWN)\n>> \t\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n>> \t\t\t\t \"extensions.objectformat\", value);\n>> +\t\t/* Only support objectFormat being specified once. */\n>> +\t\tfor_each_string_list_item(item, &data->v1_only_extensions) {\n>> +\t\t\tif (!strcmp(item->string, \"objectformat\"))\n>> +\t\t\t\treturn error(_(\"'%s' already specified as '%s'\"),\n>> +\t\t\t\t\t\"extensions.objectformat\",\n>> +\t\t\t\t\thash_algos[data->hash_algo].name);\n>> +\t\t}\n>> \t\tdata->hash_algo = format;\n>> \t\treturn EXTENSION_OK;\n>> \t}\n"},{"id":"482411","messageId":"xmqqwmwbl79k.fsf@gitster.g","threadId":"60270","inReplyTo":"87r0mjn4ly.fsf@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-27T19:56:39Z","receivedAt":"2023-09-27T19:57:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric W. Biederman\" writes:\n\n> For me the fundamental question is if we allow multiples compatibility\n> hashes or historical hashes how do we specify them? Have the option\n> appear more than once? A comma separated list?\n\nAs you found out, we tend to use both, but the former does look more\nnatural to me.\n\nThe \"usual\" pros and cons [*] involve how easy it is to override the\nsettings given by more general low-priority configuration files with\nmore specific high-priority configuration files, and does not apply\nto the extensions.* stuff that are by definition repository\nspecific.\n\n\n[Footnote]\n\nAs I said, this does not apply to the topic of this discussion, but\njust for completeness:\n\n * comma separated list allows overriding everything that was said\n earlier wholesale; there is no ambiguity, which is a plus, but\n there is no incremental updates, which may be a minus when\n flexibility is desired.\n\n * multi-valued configuration variable allows incremental additions,\n but ad-hoc syntax needs to be invented if incremental\n subtractions or clearing the slate to start from scratch is\n needed.\n\n"}]}