{"thread":{"id":"61337","subject":"[PATCH 00/11] Stop relying on SHA1 fallback for `the_hash_algo`","startedAt":"2024-04-19T09:51:09Z","lastAt":"2024-05-07T04:53:49Z","messageCount":67,"participants":["Patrick Steinhardt","brian m. carlson","Junio C Hamano","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"493204","messageId":"cover.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":null,"subject":"[PATCH 00/11] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:03Z","receivedAt":"2024-04-19T09:51:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwhen starting up, Git will initialize `the_repository` by calling\n`initialize_the_repository()`. Part of this is also that we set the hash\nalgo of `the_repository` to SHA1, which implicitly sets `the_hash_algo`\nbecause it is a macro expanding to `the_repository->hash_algo`.\n\nUsually, we eventually set up the correct hash algorithm here once we\nhave properly set up `the_repository` via `setup_git_directory()`. But\nin some commands we actually don't require a Git repository, and thus we\nwill leave the SHA1 hash algorithm in place.\n\nThis has led to some subtle bugs when the context really asks for a\nSHA256 repository, which this patch series corrects for most of the\npart. Some commands need further work, like for example git-diff(1),\nwhere the user might want to have the ability to pick a hash function\nwhen run outside of a repository.\n\nUltimately, the last patch then drops the setup of the fallback hash\nalgorithm completely. This will cause `the_hash_algo` to be a `NULL`\npointer unless explicitly configured, and thus we now start to crash\nwhen it gets accessed without doing so beforehand. This is a rather\nrisky thing to do, but it does catch bugs where we might otherwise\naccidentally do the wrong thing. And even though I think it is the right\nthing to do even conceptually, I'd be okay to drop it if folks think\nthat the risk is not worth it.\n\nPatrick\n\nPatrick Steinhardt (11):\n  path: harden validation of HEAD with non-standard hashes\n  parse-options-cb: only abbreviate hashes when hash algo is known\n  attr: don't recompute default attribute source\n  attr: fix BUG() when parsing attrs outside of repo\n  remote-curl: fix parsing of detached SHA256 heads\n  builtin/rev-parse: allow shortening to more than 40 hex characters\n  builtin/blame: don't access potentially unitialized `the_hash_algo`\n  builtin/bundle: abort \"verify\" early when there is no repository\n  builtin/diff: explicitly set hash algo when there is no repo\n  builtin/shortlog: don't set up revisions without repo\n  repository: stop setting SHA1 as the default object hash\n\n attr.c                     | 31 ++++++++++++++++++++++---------\n builtin/blame.c            |  5 ++---\n builtin/bundle.c           |  5 +++++\n builtin/diff.c             |  9 +++++++++\n builtin/rev-parse.c        |  5 ++---\n builtin/shortlog.c         |  2 +-\n parse-options-cb.c         |  3 ++-\n path.c                     |  2 +-\n remote-curl.c              | 19 ++++++++++++++++++-\n repository.c               |  2 --\n t/t0003-attributes.sh      | 15 +++++++++++++++\n t/t0040-parse-options.sh   | 17 +++++++++++++++++\n t/t1500-rev-parse.sh       |  6 ++++++\n t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++\n 14 files changed, 115 insertions(+), 21 deletions(-)\n\n-- \n2.44.GIT\n\n"},{"id":"493205","messageId":"aa4d6f508b4af3923813e19ff82a4e8484d5ff11.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 01/11] path: harden validation of HEAD with non-standard hashes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:10Z","receivedAt":"2024-04-19T09:51:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `validate_headref()` function takes a path to a supposed \"HEAD\" file\nand checks whether its format is something that we understand. It is\nused as part of our repository discovery to check whether a specific\ndirectory is a Git directory or not.\n\nPart of the validation is a check for a detached HEAD that contains a\nplain object ID. To do this validation we use `get_oid_hex()`, which\nrelies on `the_hash_algo`. At this point in time the hash algo cannot\nyet be initialized though because we didn't yet read the Git config.\nConsequently, it will always be the SHA1 hash algorithm.\n\nIn practice this works alright because `get_oid_hex()` only ends up\nchecking whether the prefix of the buffer is a valid object ID. And\nbecause SHA1 is shorter than SHA256, the function will successfully\nparse SHA256 object IDs, as well.\n\nIt is somewhat fragile though and not really the intent to only check\nfor SHA1. With this in mind, harden the code to use `get_oid_hex_any()`\nto check whether the \"HEAD\" file parses as any known hash.\n\nOne might be hard pressed to tighten the check even further and fully\nvalidate the file contents, not only the prefix. In practice though that\nwouldn't make a lot of sense as it could be that the repository uses a\nhash function that produces longer hashes than SHA256, but which the\ncurrent version of Git doesn't understand yet. We'd still want to detect\nthe repository as proper Git repository in that case, and we will fail\neventually with a proper error message that the hash isn't understood\nwhen trying to set up the repostiory format.\n\nIt follows that we could just leave the current code intact, as in\npractice the code change doesn't have any user visible impact. But it\nalso prepares us for `the_hash_algo` being unset when there is no\nrepositroy.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n path.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/path.c b/path.c\nindex 67229edb9c..cc02165530 100644\n--- a/path.c\n+++ b/path.c\n@@ -693,7 +693,7 @@ int validate_headref(const char *path)\n \t/*\n \t * Is this a detached HEAD?\n \t */\n-\tif (!get_oid_hex(buffer, &oid))\n+\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n \t\treturn 0;\n \n \treturn -1;\n-- \n2.44.GIT\n\n"},{"id":"493206","messageId":"5daaaed2b961841d539eb88e104db57ed95809f9.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 02/11] parse-options-cb: only abbreviate hashes when hash algo is known","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:15Z","receivedAt":"2024-04-19T09:51:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPT__ABBREV()` option can be used to add an option that abbreviates\nobject IDs. When given an length longer than `the_hash_algo->hexsz`,\nthen it will instead set the length to that maximum length.\n\nIt may not always be guaranteed that we have `the_hash_algo` initialized\nproperly as the hash algortihm can only be set up after we have set up\n`the_repository`. In that case, the hash would always be truncated to\nthe hex length of SHA1, which may not be what the user desires.\n\nIn practice it's not a problem as all commands that use `OPT__ABBREV()`\nalso have `RUN_SETUP` set and thus cannot work without a repository.\nConsequently, both `the_repository` and `the_hash_algo` would be\nproperly set up.\n\nRegardless of that, harden the code to not truncate the length when we\ndidn't set up a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n parse-options-cb.c       |  3 ++-\n t/t0040-parse-options.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex bdc7fae497..d99d688d3c 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -7,6 +7,7 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"setup.h\"\n #include \"string-list.h\"\n #include \"strvec.h\"\n #include \"oid-array.h\"\n@@ -29,7 +30,7 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \t\t\t\t     opt->long_name);\n \t\tif (v && v < MINIMUM_ABBREV)\n \t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (v > the_hash_algo->hexsz)\n+\t\telse if (startup_info->have_repository && v > the_hash_algo->hexsz)\n \t\t\tv = the_hash_algo->hexsz;\n \t}\n \t*(int *)(opt->value) = v;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 8bb2a8b453..45a773642f 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -176,6 +176,23 @@ test_expect_success 'long options' '\n \ttest_cmp expect output\n '\n \n+test_expect_success 'abbreviate to something longer than SHA1 length' '\n+\tcat >expect <<-EOF &&\n+\tboolean: 0\n+\tinteger: 0\n+\tmagnitude: 0\n+\ttimestamp: 0\n+\tstring: (not set)\n+\tabbrev: 100\n+\tverbose: -1\n+\tquiet: 0\n+\tdry run: no\n+\tfile: (not set)\n+\tEOF\n+\ttest-tool parse-options --abbrev=100 >output &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'missing required value' '\n \tcat >expect <<-\\EOF &&\n \terror: switch `s'\\'' requires a value\n-- \n2.44.GIT\n\n"},{"id":"493207","messageId":"ae91a27ffc0158f36758cbaf903db2422df21d74.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 03/11] attr: don't recompute default attribute source","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:20Z","receivedAt":"2024-04-19T09:51:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `default_attr_source()` function lazily computes the attr source\nsupposedly once, only. This is done via a static variable `attr_source`\nthat contains the resolved object ID of the attr source's tree. If the\nvariable is the null object ID then we try to look up the attr source,\notherwise we skip over it.\n\nThis has approach is flawed though: the variable will never be set to\nanything else but the null object ID in case there is no attr source.\nConsequently, we re-compute the information on every call. And in the\nworst case, when we silently ignore bad trees, this will cause us to try\nand look up the treeish every single time.\n\nImprove this by introducing a separate variable `has_attr_source` to\ntrack whether we already computed the attr source and, if so, whether we\nhave an attr source or not.\n\nThis also allows us to convert the `ignore_bad_attr_tree` to not be\nstatic anymore as the code will only be executed once anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 679e42258c..9d911aeb31 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1206,15 +1206,16 @@ static void collect_some_attrs(struct index_state *istate,\n }\n \n static const char *default_attr_source_tree_object_name;\n-static int ignore_bad_attr_tree;\n \n void set_git_attr_source(const char *tree_object_name)\n {\n \tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n }\n \n-static void compute_default_attr_source(struct object_id *attr_source)\n+static int compute_default_attr_source(struct object_id *attr_source)\n {\n+\tint ignore_bad_attr_tree = 0;\n+\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n@@ -1230,22 +1231,28 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \t\tignore_bad_attr_tree = 1;\n \t}\n \n-\tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n-\t\treturn;\n+\tif (!default_attr_source_tree_object_name)\n+\t\treturn 0;\n \n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n-\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n-\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\t\t\t attr_source)) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n }\n \n static struct object_id *default_attr_source(void)\n {\n \tstatic struct object_id attr_source;\n+\tstatic int has_attr_source = -1;\n \n-\tif (is_null_oid(&attr_source))\n-\t\tcompute_default_attr_source(&attr_source);\n-\tif (is_null_oid(&attr_source))\n+\tif (has_attr_source < 0)\n+\t\thas_attr_source = compute_default_attr_source(&attr_source);\n+\tif (!has_attr_source)\n \t\treturn NULL;\n \treturn &attr_source;\n }\n-- \n2.44.GIT\n\n"},{"id":"493208","messageId":"53c8e1cd7c62e641439b19447525194137a9285d.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 04/11] attr: fix BUG() when parsing attrs outside of repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:25Z","receivedAt":"2024-04-19T09:51:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are\nset, then `compute_default_attr_source()` will try to look up the value\nas a treeish. It is possible to hit that function while outside of a Git\nrepository though, for example when using `git grep --no-index`. In that\ncase, Git will hit a bug because we try to look up the main ref store\noutside of a repository.\n\nHandle the case gracefully and detect when we try to look up an attr\nsource without a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c                |  6 ++++++\n t/t0003-attributes.sh | 15 +++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/attr.c b/attr.c\nindex 9d911aeb31..4bd09bcb4b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1234,6 +1234,12 @@ static int compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\treturn 0;\n \n+\tif (!startup_info->have_repository) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"cannot use --attr-source or GIT_ATTR_SOURCE without repo\"));\n+\t\treturn 0;\n+\t}\n+\n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n \t\t\t\t attr_source)) {\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 774b52c298..3efdec54dd 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -428,6 +428,21 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre\n \t)\n '\n \n+test_expect_success 'diff without repository with attr source' '\n+\tmkdir -p \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t(\n+\t\tcd \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\ttouch file &&\n+\t\tcat >expect <<-EOF &&\n+\t\tfatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo\n+\t\tEOF\n+\t\ttest_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n-- \n2.44.GIT\n\n"},{"id":"493209","messageId":"32a429fb60e4a7e80afdbcf9f874d2b2d921badd.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 05/11] remote-curl: fix parsing of detached SHA256 heads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:29Z","receivedAt":"2024-04-19T09:51:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The dumb HTTP transport tries to read the remote HEAD reference by\ndownloading the \"HEAD\" file and then parsing it via `http_fetch_ref()`.\nThis function will either parse the file as an object ID in case it is\nexactly `the_hash_algo->hexsz` long, or otherwise it will check whether\nthe reference starts with \"ref :\" and parse it as a symbolic ref.\n\nThis is broken when parsing detached HEADs of a remote SHA256 repository\nbecause we never update `the_hash_algo` to the discovered remote object\nhash. Consequently, `the_hash_algo` will always be the fallback SHA1\nhash algorithm, which will cause us to fail parsing HEAD altogteher when\nit contains a SHA256 object ID.\n\nFix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`.\nWhile at it, let's make the expected SHA1 fallback explicit in our code,\nwhich also addresses an upcoming issue where we are going to remove the\nSHA1 fallback for `the_hash_algo`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n remote-curl.c              | 19 ++++++++++++++++++-\n t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0b6d7815fd..004b707fdf 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -266,12 +266,23 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \treturn list;\n }\n \n+/*\n+ * Try to detect the hash algorithm used by the remote repository when using\n+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash\n+ * directly have to derive it from the advertised ref lengths.\n+ */\n static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)\n {\n \tconst char *p = memchr(heads->buf, '\\t', heads->len);\n \tint algo;\n+\n+\t/*\n+\t * In case the remote has no refs we have no way to reliably determine\n+\t * the object hash used by that repository. In that case we simply fall\n+\t * back to SHA1, which may or may not be correct.\n+\t */\n \tif (!p)\n-\t\treturn the_hash_algo;\n+\t\treturn &hash_algos[GIT_HASH_SHA1];\n \n \talgo = hash_algo_by_length((p - heads->buf) / 2);\n \tif (algo == GIT_HASH_UNKNOWN)\n@@ -295,6 +306,12 @@ static struct ref *parse_info_refs(struct discovery *heads)\n \t\t    \"is this a git repository?\",\n \t\t    transport_anonymize_url(url.buf));\n \n+\t/*\n+\t * Set the repository's hash algo to whatever we have just detected.\n+\t * This ensures that we can correctly parse the remote references.\n+\t */\n+\trepo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));\n+\n \tdata = heads->buf;\n \tstart = NULL;\n \tmid = data;\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 4c3b32785d..5f16cbc58d 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -55,6 +55,21 @@ test_expect_success 'list refs from outside any repository' '\n \ttest_cmp expect actual\n '\n \n+\n+test_expect_success 'list detached HEAD from outside any repository' '\n+\tgit clone --mirror \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" \\\n+\t\tupdate-ref --no-deref HEAD refs/heads/main &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" update-server-info &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\tHEAD\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tnongit git ls-remote \"$HTTPD_URL/dumb/repo-detached.git\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create password-protected repository' '\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\n \tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n-- \n2.44.GIT\n\n"},{"id":"493210","messageId":"9cb7baa50c6df77adfc2017a16883b3cb0c587e2.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 06/11] builtin/rev-parse: allow shortening to more than 40 hex characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:34Z","receivedAt":"2024-04-19T09:51:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--short=` option for git-rev-parse(1) allows the user to specify\nto how many characters object IDs should be shortened to. The option is\nbroken though for SHA256 repositories because we set the maximum allowed\nhash size to `the_hash_algo->hexsz` before we have even set up the repo.\nConsequently, `the_hash_algo` will always be SHA1 and thus we truncate\nevery hash after at most 40 characters.\n\nFix this by accessing `the_hash_algo` only after we have set up the\nrepo.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/rev-parse.c  | 5 ++---\n t/t1500-rev-parse.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 624182e507..d7b87c605e 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -687,7 +687,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tconst char *name = NULL;\n \tstruct object_context unused;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tint seen_end_of_options = 0;\n \tenum format_type format = FORMAT_DEFAULT;\n \n@@ -863,8 +862,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tabbrev = strtoul(arg, NULL, 10);\n \t\t\t\tif (abbrev < MINIMUM_ABBREV)\n \t\t\t\t\tabbrev = MINIMUM_ABBREV;\n-\t\t\t\telse if (hexsz <= abbrev)\n-\t\t\t\t\tabbrev = hexsz;\n+\t\t\t\telse if ((int)the_hash_algo->hexsz <= abbrev)\n+\t\t\t\t\tabbrev = the_hash_algo->hexsz;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--sq\")) {\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex a669e592f1..30c31918fd 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -304,4 +304,10 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--short= truncates to the actual hash length' '\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse --short=100 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.44.GIT\n\n"},{"id":"493211","messageId":"e189a4ad15f45906bc96cd9d3372762bad01e928.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 07/11] builtin/blame: don't access potentially unitialized `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:38Z","receivedAt":"2024-04-19T09:51:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We access `the_hash_algo` in git-blame(1) before we have executed\n`parse_options_start()`, which may not be properly set up in case we\nhave no repository. This is fine for most of the part because all the\ncall paths that lead to it (git-blame(1), git-annotate(1) as well as\ngit-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.\n\nThere is one exception though, namely when passing `-h` to print the\nhelp. Here we will access `the_hash_algo` even if there is no repo.\nThis works fine right now because `the_hash_algo` gets sets up to point\nto the SHA1 algorithm via `initialize_repository()`. But we're about to\nstop doing this, and thus the code would lead to a `NULL` pointer\nexception.\n\nPrepare the code for this and only access `the_hash_algo` after we are\nsure that there is a proper repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/blame.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 9aa74680a3..e325825936 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -915,7 +915,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct range_set ranges;\n \tunsigned int range_i;\n \tlong anchor;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tlong num_lines = 0;\n \tconst char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;\n \tconst char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;\n@@ -973,11 +972,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t} else if (show_progress < 0)\n \t\tshow_progress = isatty(2);\n \n-\tif (0 < abbrev && abbrev < hexsz)\n+\tif (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\n \telse if (!abbrev)\n-\t\tabbrev = hexsz;\n+\t\tabbrev = the_hash_algo->hexsz;\n \n \tif (revs_file && read_ancestry(revs_file))\n \t\tdie_errno(\"reading graft file '%s' failed\", revs_file);\n-- \n2.44.GIT\n\n"},{"id":"493212","messageId":"bc4bda3508d22038fa05c510a5ffae53b0af1ceb.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 08/11] builtin/bundle: abort \"verify\" early when there is no repository","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:43Z","receivedAt":"2024-04-19T09:51:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Verifying a bundle requires us to have a repository. This is encoded in\n`verify_bundle()`, which will return an error if there is no repository.\nWe call `open_bundle()` before we call `verify_bundle()` though, which\nalready performs some verifications even though we may ultimately abort\ndue to a missing repository.\n\nThis is problematic because `open_bundle()` already reads the bundle\nheader and verifies that it contains a properly formatted hash. When\nthere is no repository we have no clue what hash function to expect\nthough, so we always end up assuming SHA1 here, which may or may not be\ncorrect. Furthermore, we are about to stop initializing `the_hash_algo`\nwhen there is no repository, which will lead to segfaults.\n\nCheck early on whether we have a repository to fix this issue.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/bundle.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 3ad11dc5d0..d5d41a8f67 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -140,6 +140,11 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n+\tif (!startup_info->have_repository) {\n+\t\tret = error(_(\"need a repository to verify a bundle\"));\n+\t\tgoto cleanup;\n+\t}\n+\n \tif ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n-- \n2.44.GIT\n\n"},{"id":"493213","messageId":"39e56dab621a2a1e0cbaf67f8de44614c354dcb1.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 09/11] builtin/diff: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:48Z","receivedAt":"2024-04-19T09:51:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-diff(1) command can be used outside repositories to diff two\nfiles with each other. But even if there is no repository we will end up\nhashing the files that we are diffing so that we can print the \"index\"\nline:\n\n```\ndiff --git a/a b/b\nindex 7898192..6178079 100644\n--- a/a\n+++ b/b\n@@ -1 +1 @@\n-a\n+b\n```\n\nWe implicitly use SHA1 to calculate the hash here, which is because\n`the_repository` gets initialized with SHA1 during the startup routine.\nWe are about to stop doing this though such that `the_repository` only\never has a hash function when it was properly initialized via a repo's\nconfiguration.\n\nTo give full control to our users, we would ideally add a new switch to\ngit-diff(1) that allows them to specify the hash function when executed\noutside of a repository. But for now, we only convert the code to make\nthis explicit such that we can stop setting the default hash algorithm\nfor `the_repository`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/diff.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 6e196e0c7d..58ec7e5da2 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \t}\n \n+\t/*\n+\t * When operating outside of a Git repository we need to have a hash\n+\t * algorithm at hand so that we can generate the blob hashes. We\n+\t * default to SHA1 here, but may eventually want to change this to be\n+\t * configurable via a command line option.\n+\t */\n+\tif (nongit)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tinit_diff_ui_defaults();\n \tgit_config(git_diff_ui_config, NULL);\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n-- \n2.44.GIT\n\n"},{"id":"493214","messageId":"508e28ed1e21e9c646cec8473a65137d33f1df23.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 10/11] builtin/shortlog: don't set up revisions without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:52Z","receivedAt":"2024-04-19T09:51:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is possible to run git-shortlog(1) outside of a repository by passing\nit output from git-log(1) via standard input. Obviously, as there is no\nrepository in that context, it is thus unsupported to pass any revisions\nas arguments.\n\nReghardless of that we still end up calling `setup_revisions()`. While\nthat works alright, it is somewhat strange. Furthermore, this is about\nto cause problems when we unset the default object hash.\n\nRefactor the code to only call `setup_revisions()` when we have a\nrepository. This is safe to do as we already verify that there are no\narguments when running outside of a repository anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3c7cd2d6ef..d4daf31e22 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -435,7 +435,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n \n-\tif (setup_revisions(argc, argv, &rev, NULL) != 1) {\n+\tif (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {\n \t\terror(_(\"unrecognized argument: %s\"), argv[1]);\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n-- \n2.44.GIT\n\n"},{"id":"493215","messageId":"f86a6ff3ba1b910c4f050598b7187cc917e77428.1713519789.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH 11/11] repository: stop setting SHA1 as the default object hash","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-19T09:51:57Z","receivedAt":"2024-04-19T09:52:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During the startup of Git, we call `initialize_the_repository()` to set\nup `the_repository` as well as `the_index`. Part of this setup is also\nto set the default object hash of the repository to SHA1. This has the\neffect that `the_hash_algo` is getting initialized to SHA1, as well.\nThis default hash algorithm eventually gets overridden by most Git\ncommands via `setup_git_directory()`, which also detects the actual hash\nalgorithm used by the repository.\n\nThere are some commands though that don't access a repository at all, or\nat a later point only, and thus retain the default hash function for\nsome amount of time. As some of the the preceding commits demonstrate,\nthis can lead to subtle issues when we access `the_hash_algo` when no\nrepository has been set up.\n\nAddress this issue by dropping the set up of the default hash algorithm\ncompletely. The effect of this is that `the_hash_algo` will map to a\n`NULL` pointer and thus cause Git to crash when something tries to\naccess the hash algorithm without it being properly initialized. It thus\nforces all Git commands to explicitly set up the hash algorithm in case\nthere is no repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repository.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/repository.c b/repository.c\nindex e15b416944..b65b1a8c8b 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -35,8 +35,6 @@ void initialize_the_repository(void)\n \tthe_repo.parsed_objects = parsed_object_pool_new();\n \n \tindex_state_init(&the_index, the_repository);\n-\n-\trepo_set_hash_algo(&the_repo, GIT_HASH_SHA1);\n }\n \n static void expand_base_dir(char **out, const char *in,\n-- \n2.44.GIT\n\n"},{"id":"493233","messageId":"ZiLABRgJPlNnm-uY@tapette.crustytoothpaste.net","threadId":"61337","inReplyTo":"aa4d6f508b4af3923813e19ff82a4e8484d5ff11.1713519789.git.ps@pks.im","subject":"Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-04-19T19:03:33Z","receivedAt":"2024-04-19T19:03:42Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-04-19 at 09:51:10, Patrick Steinhardt wrote:\n> It follows that we could just leave the current code intact, as in\n> practice the code change doesn't have any user visible impact. But it\n> also prepares us for `the_hash_algo` being unset when there is no\n> repositroy.\n\nThe patch looks fine and is well explained, but you have a typo\n(\"repositroy\").\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"493234","messageId":"ZiLCO8Wn0-4jyDdp@tapette.crustytoothpaste.net","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"Re: [PATCH 00/11] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-04-19T19:12:59Z","receivedAt":"2024-04-19T19:13:02Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-04-19 at 09:51:03, Patrick Steinhardt wrote:\n> Hi,\n> \n> when starting up, Git will initialize `the_repository` by calling\n> `initialize_the_repository()`. Part of this is also that we set the hash\n> algo of `the_repository` to SHA1, which implicitly sets `the_hash_algo`\n> because it is a macro expanding to `the_repository->hash_algo`.\n> \n> Usually, we eventually set up the correct hash algorithm here once we\n> have properly set up `the_repository` via `setup_git_directory()`. But\n> in some commands we actually don't require a Git repository, and thus we\n> will leave the SHA1 hash algorithm in place.\n> \n> This has led to some subtle bugs when the context really asks for a\n> SHA256 repository, which this patch series corrects for most of the\n> part. Some commands need further work, like for example git-diff(1),\n> where the user might want to have the ability to pick a hash function\n> when run outside of a repository.\n> \n> Ultimately, the last patch then drops the setup of the fallback hash\n> algorithm completely. This will cause `the_hash_algo` to be a `NULL`\n> pointer unless explicitly configured, and thus we now start to crash\n> when it gets accessed without doing so beforehand. This is a rather\n> risky thing to do, but it does catch bugs where we might otherwise\n> accidentally do the wrong thing. And even though I think it is the right\n> thing to do even conceptually, I'd be okay to drop it if folks think\n> that the risk is not worth it.\n\nI've taken a look, and other than the minor typo I noted, this seems\nfine.  I'm in favour of getting rid of the SHA-1 default, even though my\ngut tells me we might find a bug or two along the way where things\naren't initialized properly.  I still think that'll be okay and it's\nworth doing, since it'll help us prepare for the case in the future\nwhere we want to switch the default and also for libification, where we\nwon't want to make those kinds of assumptions.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"493235","messageId":"xmqq34rh6u4f.fsf@gitster.g","threadId":"61337","inReplyTo":"ZiLCO8Wn0-4jyDdp@tapette.crustytoothpaste.net","subject":"Re: [PATCH 00/11] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-19T19:16:16Z","receivedAt":"2024-04-19T19:16:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> I've taken a look, and other than the minor typo I noted, this seems\n> fine.  I'm in favour of getting rid of the SHA-1 default, even though my\n> gut tells me we might find a bug or two along the way where things\n> aren't initialized properly.  I still think that'll be okay and it's\n> worth doing, since it'll help us prepare for the case in the future\n> where we want to switch the default and also for libification, where we\n> won't want to make those kinds of assumptions.\n\nYup, thanks for reviewing (and thanks Patrick for writing the\nseries).\n"},{"id":"493296","messageId":"ZiXt49eOx5AfLbZb@tanuki","threadId":"61337","inReplyTo":"ZiLCO8Wn0-4jyDdp@tapette.crustytoothpaste.net","subject":"Re: [PATCH 00/11] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-22T04:56:03Z","receivedAt":"2024-04-22T04:56:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Apr 19, 2024 at 07:12:59PM +0000, brian m. carlson wrote:\n> On 2024-04-19 at 09:51:03, Patrick Steinhardt wrote:\n> > Hi,\n> > \n> > when starting up, Git will initialize `the_repository` by calling\n> > `initialize_the_repository()`. Part of this is also that we set the hash\n> > algo of `the_repository` to SHA1, which implicitly sets `the_hash_algo`\n> > because it is a macro expanding to `the_repository->hash_algo`.\n> > \n> > Usually, we eventually set up the correct hash algorithm here once we\n> > have properly set up `the_repository` via `setup_git_directory()`. But\n> > in some commands we actually don't require a Git repository, and thus we\n> > will leave the SHA1 hash algorithm in place.\n> > \n> > This has led to some subtle bugs when the context really asks for a\n> > SHA256 repository, which this patch series corrects for most of the\n> > part. Some commands need further work, like for example git-diff(1),\n> > where the user might want to have the ability to pick a hash function\n> > when run outside of a repository.\n> > \n> > Ultimately, the last patch then drops the setup of the fallback hash\n> > algorithm completely. This will cause `the_hash_algo` to be a `NULL`\n> > pointer unless explicitly configured, and thus we now start to crash\n> > when it gets accessed without doing so beforehand. This is a rather\n> > risky thing to do, but it does catch bugs where we might otherwise\n> > accidentally do the wrong thing. And even though I think it is the right\n> > thing to do even conceptually, I'd be okay to drop it if folks think\n> > that the risk is not worth it.\n> \n> I've taken a look, and other than the minor typo I noted, this seems\n> fine.  I'm in favour of getting rid of the SHA-1 default, even though my\n> gut tells me we might find a bug or two along the way where things\n> aren't initialized properly.  I still think that'll be okay and it's\n> worth doing, since it'll help us prepare for the case in the future\n> where we want to switch the default and also for libification, where we\n> won't want to make those kinds of assumptions.\n\nYeah, I would expect there to be a few more bugs that I just didn't\nspot. But I think if we do this early in the next release cycle it would\ngive us plenty of time to detect any such issues. And in the worst case\nwe would still be able to revert the last patch of this series. So all\nin all it hopefully shouldn't be all that bad and prepares us for the\nfuture.\n\nThanks for your review!\n\nPatrick\n"},{"id":"493297","messageId":"ZiXt6w0TCZnkIqbW@tanuki","threadId":"61337","inReplyTo":"ZiLABRgJPlNnm-uY@tapette.crustytoothpaste.net","subject":"Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-22T04:56:11Z","receivedAt":"2024-04-22T04:56:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Apr 19, 2024 at 07:03:33PM +0000, brian m. carlson wrote:\n> On 2024-04-19 at 09:51:10, Patrick Steinhardt wrote:\n> > It follows that we could just leave the current code intact, as in\n> > practice the code change doesn't have any user visible impact. But it\n> > also prepares us for `the_hash_algo` being unset when there is no\n> > repositroy.\n> \n> The patch looks fine and is well explained, but you have a typo\n> (\"repositroy\").\n\nThanks, fixed! I'll refrain from sending out another version just to\naddress this typo though.\n\nPatrick\n"},{"id":"493311","messageId":"xmqqjzkpxtjm.fsf@gitster.g","threadId":"61337","inReplyTo":"aa4d6f508b4af3923813e19ff82a4e8484d5ff11.1713519789.git.ps@pks.im","subject":"Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T16:15:41Z","receivedAt":"2024-04-22T16:15:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The `validate_headref()` function takes a path to a supposed \"HEAD\" file\n> and checks whether its format is something that we understand. It is\n> used as part of our repository discovery to check whether a specific\n> directory is a Git directory or not.\n>\n> Part of the validation is a check for a detached HEAD that contains a\n> plain object ID. To do this validation we use `get_oid_hex()`, which\n> relies on `the_hash_algo`. At this point in time the hash algo cannot\n> yet be initialized though because we didn't yet read the Git config.\n> Consequently, it will always be the SHA1 hash algorithm.\n>\n> In practice this works alright because `get_oid_hex()` only ends up\n> checking whether the prefix of the buffer is a valid object ID. And\n> because SHA1 is shorter than SHA256, the function will successfully\n> parse SHA256 object IDs, as well.\n>\n> It is somewhat fragile though and not really the intent to only check\n> for SHA1. With this in mind, harden the code to use `get_oid_hex_any()`\n> to check whether the \"HEAD\" file parses as any known hash.\n\nAll makes sense, and given the above, I strongly suspect that we\nwould want to make the validate_headref() function a file-scope\nstatic in setup.c to make sure it is only called/callable from the\nrepository discovery codepath.  Especially that if somebody calls\nthis function again after we find out that the repository uses\nSHA-256, we would want to let the caller know that the detached HEAD\nrecords SHA-1 and we are in an inconsistent state.\n\n> It follows that we could just leave the current code intact, as in\n> practice the code change doesn't have any user visible impact. But it\n> also prepares us for `the_hash_algo` being unset when there is no\n> repositroy.\n\nOr perhaps we use get_oid_hex_any() != GIT_HASH_UNKNOWN when\nthe_hash_algo has not been determined, and use !get_oid_hex() after\nwe have determined which algorithm we are using?  It may be what you\ndid in a later step in the series, so let me read on.\n\nThanks.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  path.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/path.c b/path.c\n> index 67229edb9c..cc02165530 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -693,7 +693,7 @@ int validate_headref(const char *path)\n>  \t/*\n>  \t * Is this a detached HEAD?\n>  \t */\n> -\tif (!get_oid_hex(buffer, &oid))\n> +\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n>  \t\treturn 0;\n>  \n>  \treturn -1;\n"},{"id":"493313","messageId":"xmqqedaxxmsa.fsf@gitster.g","threadId":"61337","inReplyTo":"39e56dab621a2a1e0cbaf67f8de44614c354dcb1.1713519789.git.ps@pks.im","subject":"Re: [PATCH 09/11] builtin/diff: explicitly set hash algo when there is no repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T18:41:41Z","receivedAt":"2024-04-22T18:41:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The git-diff(1) command can be used outside repositories to diff two\n> files with each other. But even if there is no repository we will end up\n> hashing the files that we are diffing so that we can print the \"index\"\n> line:\n>\n> ```\n> diff --git a/a b/b\n> index 7898192..6178079 100644\n> --- a/a\n> +++ b/b\n> @@ -1 +1 @@\n> -a\n> +b\n> ```\n\nThis will break \"git am\"; it is customary to indent such sample\npatch block in a log message.\n\n> We implicitly use SHA1 to calculate the hash here, which is because\n> `the_repository` gets initialized with SHA1 during the startup routine.\n> We are about to stop doing this though such that `the_repository` only\n> ever has a hash function when it was properly initialized via a repo's\n> configuration.\n>\n> To give full control to our users, we would ideally add a new switch to\n> git-diff(1) that allows them to specify the hash function when executed\n> outside of a repository. But for now, we only convert the code to make\n> this explicit such that we can stop setting the default hash algorithm\n> for `the_repository`.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/diff.c | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 6e196e0c7d..58ec7e5da2 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>  \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n>  \t}\n>  \n> +\t/*\n> +\t * When operating outside of a Git repository we need to have a hash\n> +\t * algorithm at hand so that we can generate the blob hashes. We\n> +\t * default to SHA1 here, but may eventually want to change this to be\n> +\t * configurable via a command line option.\n> +\t */\n> +\tif (nongit)\n> +\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n> +\n>  \tinit_diff_ui_defaults();\n>  \tgit_config(git_diff_ui_config, NULL);\n>  \tprefix = precompose_argv_prefix(argc, argv, prefix);\n"},{"id":"493334","messageId":"d2zwlfocsyywyqmwqejcsp6636zjo6e6adfpcoltnqrhkjurrl@spx3ggc6eofc","threadId":"61337","inReplyTo":"5daaaed2b961841d539eb88e104db57ed95809f9.1713519789.git.ps@pks.im","subject":"Re: [PATCH 02/11] parse-options-cb: only abbreviate hashes when hash algo is known","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-23T00:30:22Z","receivedAt":"2024-04-23T00:31:34Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/04/19 11:51AM, Patrick Steinhardt wrote:\n> The `OPT__ABBREV()` option can be used to add an option that abbreviates\n> object IDs. When given an length longer than `the_hash_algo->hexsz`,\n\ns/an length/a length/\n\n> then it will instead set the length to that maximum length.\n> \n> It may not always be guaranteed that we have `the_hash_algo` initialized\n> properly as the hash algortihm can only be set up after we have set up\n\ns/algortihm/algorithm/\n\n> `the_repository`. In that case, the hash would always be truncated to\n> the hex length of SHA1, which may not be what the user desires.\n> \n> In practice it's not a problem as all commands that use `OPT__ABBREV()`\n> also have `RUN_SETUP` set and thus cannot work without a repository.\n> Consequently, both `the_repository` and `the_hash_algo` would be\n> properly set up.\n> \n> Regardless of that, harden the code to not truncate the length when we\n> didn't set up a repository.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n...\n\nA couple of typos I noticed.\n\n-Justin\n"},{"id":"493335","messageId":"2id3biajad2mw7xvq7bfaturhfot66y24cs5l5zxpc3q4jf4i3@duhrmrpdq3gv","threadId":"61337","inReplyTo":"ae91a27ffc0158f36758cbaf903db2422df21d74.1713519789.git.ps@pks.im","subject":"Re: [PATCH 03/11] attr: don't recompute default attribute source","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-23T00:32:28Z","receivedAt":"2024-04-23T00:33:39Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/04/19 11:51AM, Patrick Steinhardt wrote:\n> The `default_attr_source()` function lazily computes the attr source\n> supposedly once, only. This is done via a static variable `attr_source`\n> that contains the resolved object ID of the attr source's tree. If the\n> variable is the null object ID then we try to look up the attr source,\n> otherwise we skip over it.\n> \n> This has approach is flawed though: the variable will never be set to\n\nRemove \"has\"\n\n-Justin\n\n> anything else but the null object ID in case there is no attr source.\n> Consequently, we re-compute the information on every call. And in the\n> worst case, when we silently ignore bad trees, this will cause us to try\n> and look up the treeish every single time.\n> \n> Improve this by introducing a separate variable `has_attr_source` to\n> track whether we already computed the attr source and, if so, whether we\n> have an attr source or not.\n> \n> This also allows us to convert the `ignore_bad_attr_tree` to not be\n> static anymore as the code will only be executed once anyway.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n...\n"},{"id":"493336","messageId":"sy5j67uqsqepg3blhsioiustxvb6j6cx2ghlcsfrn6wjrwduzs@6qfhdbavsz35","threadId":"61337","inReplyTo":"508e28ed1e21e9c646cec8473a65137d33f1df23.1713519789.git.ps@pks.im","subject":"Re: [PATCH 10/11] builtin/shortlog: don't set up revisions without repo","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-23T00:35:21Z","receivedAt":"2024-04-23T00:36:32Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/04/19 11:51AM, Patrick Steinhardt wrote:\n> It is possible to run git-shortlog(1) outside of a repository by passing\n> it output from git-log(1) via standard input. Obviously, as there is no\n> repository in that context, it is thus unsupported to pass any revisions\n> as arguments.\n> \n> Reghardless of that we still end up calling `setup_revisions()`. While\n\ns/Reghardless/Regardless/\n\n> that works alright, it is somewhat strange. Furthermore, this is about\n> to cause problems when we unset the default object hash.\n> \n> Refactor the code to only call `setup_revisions()` when we have a\n> repository. This is safe to do as we already verify that there are no\n> arguments when running outside of a repository anyway.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n...\n"},{"id":"493338","messageId":"Zic-B7oSlv1PwvOL@tanuki","threadId":"61337","inReplyTo":"xmqqjzkpxtjm.fsf@gitster.g","subject":"Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T04:50:15Z","receivedAt":"2024-04-23T04:50:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 22, 2024 at 09:15:41AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The `validate_headref()` function takes a path to a supposed \"HEAD\" file\n> > and checks whether its format is something that we understand. It is\n> > used as part of our repository discovery to check whether a specific\n> > directory is a Git directory or not.\n> >\n> > Part of the validation is a check for a detached HEAD that contains a\n> > plain object ID. To do this validation we use `get_oid_hex()`, which\n> > relies on `the_hash_algo`. At this point in time the hash algo cannot\n> > yet be initialized though because we didn't yet read the Git config.\n> > Consequently, it will always be the SHA1 hash algorithm.\n> >\n> > In practice this works alright because `get_oid_hex()` only ends up\n> > checking whether the prefix of the buffer is a valid object ID. And\n> > because SHA1 is shorter than SHA256, the function will successfully\n> > parse SHA256 object IDs, as well.\n> >\n> > It is somewhat fragile though and not really the intent to only check\n> > for SHA1. With this in mind, harden the code to use `get_oid_hex_any()`\n> > to check whether the \"HEAD\" file parses as any known hash.\n> \n> All makes sense, and given the above, I strongly suspect that we\n> would want to make the validate_headref() function a file-scope\n> static in setup.c to make sure it is only called/callable from the\n> repository discovery codepath.  Especially that if somebody calls\n> this function again after we find out that the repository uses\n> SHA-256, we would want to let the caller know that the detached HEAD\n> records SHA-1 and we are in an inconsistent state.\n\nFair enough, we can definitely do so. It only has a single callsite\nanyway.\n\n> > It follows that we could just leave the current code intact, as in\n> > practice the code change doesn't have any user visible impact. But it\n> > also prepares us for `the_hash_algo` being unset when there is no\n> > repositroy.\n> \n> Or perhaps we use get_oid_hex_any() != GIT_HASH_UNKNOWN when\n> the_hash_algo has not been determined, and use !get_oid_hex() after\n> we have determined which algorithm we are using?  It may be what you\n> did in a later step in the series, so let me read on.\n\nI didn't, and given that it's only used from `is_git_directory()` it\nwould probably not make much sense, either. I'll rather add another\npatch on top that moves the function around to clarify that it is only\nexpected to be called from \"setup.c\".\n\nPatrick\n\n> Thanks.\n> \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  path.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/path.c b/path.c\n> > index 67229edb9c..cc02165530 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -693,7 +693,7 @@ int validate_headref(const char *path)\n> >  \t/*\n> >  \t * Is this a detached HEAD?\n> >  \t */\n> > -\tif (!get_oid_hex(buffer, &oid))\n> > +\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n> >  \t\treturn 0;\n> >  \n> >  \treturn -1;\n"},{"id":"493339","messageId":"cover.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH v2 00/12] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:20Z","receivedAt":"2024-04-23T05:07:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that causes us to stop\nrelying on the SHA1 default hash.\n\nChanges compared to v1:\n\n    - Various typo fixes in commit messages.\n\n    - Added another patch that moves `validate_headref()` into \"setup.c\"\n      to clarify that it is only used during repository discovery.\n\n    - Indented a diff in a commit message so that git-am(1) is happy.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (12):\n  path: harden validation of HEAD with non-standard hashes\n  path: move `validate_headref()` to its only user\n  parse-options-cb: only abbreviate hashes when hash algo is known\n  attr: don't recompute default attribute source\n  attr: fix BUG() when parsing attrs outside of repo\n  remote-curl: fix parsing of detached SHA256 heads\n  builtin/rev-parse: allow shortening to more than 40 hex characters\n  builtin/blame: don't access potentially unitialized `the_hash_algo`\n  builtin/bundle: abort \"verify\" early when there is no repository\n  builtin/diff: explicitly set hash algo when there is no repo\n  builtin/shortlog: don't set up revisions without repo\n  repository: stop setting SHA1 as the default object hash\n\n attr.c                     | 31 +++++++++++++++-------\n builtin/blame.c            |  5 ++--\n builtin/bundle.c           |  5 ++++\n builtin/diff.c             |  9 +++++++\n builtin/rev-parse.c        |  5 ++--\n builtin/shortlog.c         |  2 +-\n parse-options-cb.c         |  3 ++-\n path.c                     | 53 --------------------------------------\n path.h                     |  1 -\n remote-curl.c              | 19 +++++++++++++-\n repository.c               |  2 --\n setup.c                    | 53 ++++++++++++++++++++++++++++++++++++++\n t/t0003-attributes.sh      | 15 +++++++++++\n t/t0040-parse-options.sh   | 17 ++++++++++++\n t/t1500-rev-parse.sh       |  6 +++++\n t/t5550-http-fetch-dumb.sh | 15 +++++++++++\n 16 files changed, 167 insertions(+), 74 deletions(-)\n\nRange-diff against v1:\n 1:  aa4d6f508b !  1:  a986b464d3 path: harden validation of HEAD with non-standard hashes\n    @@ Commit message\n         current version of Git doesn't understand yet. We'd still want to detect\n         the repository as proper Git repository in that case, and we will fail\n         eventually with a proper error message that the hash isn't understood\n    -    when trying to set up the repostiory format.\n    +    when trying to set up the repository format.\n     \n         It follows that we could just leave the current code intact, as in\n         practice the code change doesn't have any user visible impact. But it\n         also prepares us for `the_hash_algo` being unset when there is no\n    -    repositroy.\n    +    repository.\n     \n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n -:  ---------- >  2:  a347c7e6ca path: move `validate_headref()` to its only user\n 2:  5daaaed2b9 !  3:  c0a15b2fa6 parse-options-cb: only abbreviate hashes when hash algo is known\n    @@ Commit message\n         parse-options-cb: only abbreviate hashes when hash algo is known\n     \n         The `OPT__ABBREV()` option can be used to add an option that abbreviates\n    -    object IDs. When given an length longer than `the_hash_algo->hexsz`,\n    -    then it will instead set the length to that maximum length.\n    +    object IDs. When given a length longer than `the_hash_algo->hexsz`, then\n    +    it will instead set the length to that maximum length.\n     \n         It may not always be guaranteed that we have `the_hash_algo` initialized\n    -    properly as the hash algortihm can only be set up after we have set up\n    +    properly as the hash algorithm can only be set up after we have set up\n         `the_repository`. In that case, the hash would always be truncated to\n         the hex length of SHA1, which may not be what the user desires.\n     \n 3:  ae91a27ffc !  4:  1b5f904eed attr: don't recompute default attribute source\n    @@ Commit message\n         variable is the null object ID then we try to look up the attr source,\n         otherwise we skip over it.\n     \n    -    This has approach is flawed though: the variable will never be set to\n    +    This approach is flawed though: the variable will never be set to\n         anything else but the null object ID in case there is no attr source.\n         Consequently, we re-compute the information on every call. And in the\n         worst case, when we silently ignore bad trees, this will cause us to try\n 4:  53c8e1cd7c =  5:  26909daca4 attr: fix BUG() when parsing attrs outside of repo\n 5:  32a429fb60 =  6:  0b99184f50 remote-curl: fix parsing of detached SHA256 heads\n 6:  9cb7baa50c =  7:  ccfda3c2d2 builtin/rev-parse: allow shortening to more than 40 hex characters\n 7:  e189a4ad15 =  8:  1813e7eb5c builtin/blame: don't access potentially unitialized `the_hash_algo`\n 8:  bc4bda3508 =  9:  31182a1fc6 builtin/bundle: abort \"verify\" early when there is no repository\n 9:  39e56dab62 ! 10:  78e19d0a1b builtin/diff: explicitly set hash algo when there is no repo\n    @@ Commit message\n         hashing the files that we are diffing so that we can print the \"index\"\n         line:\n     \n    -    ```\n    -    diff --git a/a b/b\n    -    index 7898192..6178079 100644\n    -    --- a/a\n    -    +++ b/b\n    -    @@ -1 +1 @@\n    -    -a\n    -    +b\n    -    ```\n    +        ```\n    +        diff --git a/a b/b\n    +        index 7898192..6178079 100644\n    +        --- a/a\n    +        +++ b/b\n    +        @@ -1 +1 @@\n    +        -a\n    +        +b\n    +        ```\n     \n         We implicitly use SHA1 to calculate the hash here, which is because\n         `the_repository` gets initialized with SHA1 during the startup routine.\n10:  508e28ed1e ! 11:  51bcddbc31 builtin/shortlog: don't set up revisions without repo\n    @@ Commit message\n         repository in that context, it is thus unsupported to pass any revisions\n         as arguments.\n     \n    -    Reghardless of that we still end up calling `setup_revisions()`. While\n    +    Regardless of that we still end up calling `setup_revisions()`. While\n         that works alright, it is somewhat strange. Furthermore, this is about\n         to cause problems when we unset the default object hash.\n     \n11:  f86a6ff3ba = 12:  e8126371e1 repository: stop setting SHA1 as the default object hash\n\nbase-commit: 21306a098c3f174ad4c2a5cddb9069ee27a548b0\n-- \n2.45.0-rc0\n\n"},{"id":"493340","messageId":"a986b464d3e0250fc01f82ea8f8b91625eceeffe.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 01/12] path: harden validation of HEAD with non-standard hashes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:26Z","receivedAt":"2024-04-23T05:07:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `validate_headref()` function takes a path to a supposed \"HEAD\" file\nand checks whether its format is something that we understand. It is\nused as part of our repository discovery to check whether a specific\ndirectory is a Git directory or not.\n\nPart of the validation is a check for a detached HEAD that contains a\nplain object ID. To do this validation we use `get_oid_hex()`, which\nrelies on `the_hash_algo`. At this point in time the hash algo cannot\nyet be initialized though because we didn't yet read the Git config.\nConsequently, it will always be the SHA1 hash algorithm.\n\nIn practice this works alright because `get_oid_hex()` only ends up\nchecking whether the prefix of the buffer is a valid object ID. And\nbecause SHA1 is shorter than SHA256, the function will successfully\nparse SHA256 object IDs, as well.\n\nIt is somewhat fragile though and not really the intent to only check\nfor SHA1. With this in mind, harden the code to use `get_oid_hex_any()`\nto check whether the \"HEAD\" file parses as any known hash.\n\nOne might be hard pressed to tighten the check even further and fully\nvalidate the file contents, not only the prefix. In practice though that\nwouldn't make a lot of sense as it could be that the repository uses a\nhash function that produces longer hashes than SHA256, but which the\ncurrent version of Git doesn't understand yet. We'd still want to detect\nthe repository as proper Git repository in that case, and we will fail\neventually with a proper error message that the hash isn't understood\nwhen trying to set up the repository format.\n\nIt follows that we could just leave the current code intact, as in\npractice the code change doesn't have any user visible impact. But it\nalso prepares us for `the_hash_algo` being unset when there is no\nrepository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n path.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/path.c b/path.c\nindex 67229edb9c..cc02165530 100644\n--- a/path.c\n+++ b/path.c\n@@ -693,7 +693,7 @@ int validate_headref(const char *path)\n \t/*\n \t * Is this a detached HEAD?\n \t */\n-\tif (!get_oid_hex(buffer, &oid))\n+\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n \t\treturn 0;\n \n \treturn -1;\n-- \n2.45.0-rc0\n\n"},{"id":"493341","messageId":"a347c7e6cae257e35c84ace191a8079f1dddc0d4.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 02/12] path: move `validate_headref()` to its only user","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:32Z","receivedAt":"2024-04-23T05:07:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While `validate_headref()` is only called from `is_git_directory()` in\n\"setup.c\", it is currently implemented in \"path.c\". Move it over such\nthat it becomes clear that it is only really used during setup in order\nto discover repositories.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n path.c  | 53 -----------------------------------------------------\n path.h  |  1 -\n setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 53 insertions(+), 54 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex cc02165530..bd6e25245d 100644\n--- a/path.c\n+++ b/path.c\n@@ -5,7 +5,6 @@\n #include \"abspath.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n-#include \"hex.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n@@ -647,58 +646,6 @@ void strbuf_git_common_path(struct strbuf *sb,\n \tva_end(args);\n }\n \n-int validate_headref(const char *path)\n-{\n-\tstruct stat st;\n-\tchar buffer[256];\n-\tconst char *refname;\n-\tstruct object_id oid;\n-\tint fd;\n-\tssize_t len;\n-\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\n-\t/* Make sure it is a \"refs/..\" symlink */\n-\tif (S_ISLNK(st.st_mode)) {\n-\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n-\t\t\treturn 0;\n-\t\treturn -1;\n-\t}\n-\n-\t/*\n-\t * Anything else, just open it and try to see if it is a symbolic ref.\n-\t */\n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn -1;\n-\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n-\tclose(fd);\n-\n-\tif (len < 0)\n-\t\treturn -1;\n-\tbuffer[len] = '\\0';\n-\n-\t/*\n-\t * Is it a symbolic ref?\n-\t */\n-\tif (skip_prefix(buffer, \"ref:\", &refname)) {\n-\t\twhile (isspace(*refname))\n-\t\t\trefname++;\n-\t\tif (starts_with(refname, \"refs/\"))\n-\t\t\treturn 0;\n-\t}\n-\n-\t/*\n-\t * Is this a detached HEAD?\n-\t */\n-\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n-\t\treturn 0;\n-\n-\treturn -1;\n-}\n-\n static struct passwd *getpw_str(const char *username, size_t len)\n {\n \tstruct passwd *pw;\ndiff --git a/path.h b/path.h\nindex ea96487b29..c3bc8617bd 100644\n--- a/path.h\n+++ b/path.h\n@@ -173,7 +173,6 @@ const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \n int ends_with_path_components(const char *path, const char *components);\n-int validate_headref(const char *ref);\n \n int calc_shared_perm(int mode);\n int adjust_shared_perm(const char *path);\ndiff --git a/setup.c b/setup.c\nindex f4b32f76e3..7c996659bd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -4,6 +4,7 @@\n #include \"environment.h\"\n #include \"exec-cmd.h\"\n #include \"gettext.h\"\n+#include \"hex.h\"\n #include \"object-name.h\"\n #include \"refs.h\"\n #include \"repository.h\"\n@@ -341,6 +342,58 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n \treturn ret;\n }\n \n+static int validate_headref(const char *path)\n+{\n+\tstruct stat st;\n+\tchar buffer[256];\n+\tconst char *refname;\n+\tstruct object_id oid;\n+\tint fd;\n+\tssize_t len;\n+\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\n+\t/* Make sure it is a \"refs/..\" symlink */\n+\tif (S_ISLNK(st.st_mode)) {\n+\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n+\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n+\t\t\treturn 0;\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Anything else, just open it and try to see if it is a symbolic ref.\n+\t */\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn -1;\n+\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n+\tclose(fd);\n+\n+\tif (len < 0)\n+\t\treturn -1;\n+\tbuffer[len] = '\\0';\n+\n+\t/*\n+\t * Is it a symbolic ref?\n+\t */\n+\tif (skip_prefix(buffer, \"ref:\", &refname)) {\n+\t\twhile (isspace(*refname))\n+\t\t\trefname++;\n+\t\tif (starts_with(refname, \"refs/\"))\n+\t\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * Is this a detached HEAD?\n+\t */\n+\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n+\t\treturn 0;\n+\n+\treturn -1;\n+}\n+\n /*\n  * Test if it looks like we're at a git directory.\n  * We want to see:\n-- \n2.45.0-rc0\n\n"},{"id":"493342","messageId":"c0a15b2fa68396c763ccd8745a54e16c2941f6a0.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 03/12] parse-options-cb: only abbreviate hashes when hash algo is known","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:37Z","receivedAt":"2024-04-23T05:07:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPT__ABBREV()` option can be used to add an option that abbreviates\nobject IDs. When given a length longer than `the_hash_algo->hexsz`, then\nit will instead set the length to that maximum length.\n\nIt may not always be guaranteed that we have `the_hash_algo` initialized\nproperly as the hash algorithm can only be set up after we have set up\n`the_repository`. In that case, the hash would always be truncated to\nthe hex length of SHA1, which may not be what the user desires.\n\nIn practice it's not a problem as all commands that use `OPT__ABBREV()`\nalso have `RUN_SETUP` set and thus cannot work without a repository.\nConsequently, both `the_repository` and `the_hash_algo` would be\nproperly set up.\n\nRegardless of that, harden the code to not truncate the length when we\ndidn't set up a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n parse-options-cb.c       |  3 ++-\n t/t0040-parse-options.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex bdc7fae497..d99d688d3c 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -7,6 +7,7 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"setup.h\"\n #include \"string-list.h\"\n #include \"strvec.h\"\n #include \"oid-array.h\"\n@@ -29,7 +30,7 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \t\t\t\t     opt->long_name);\n \t\tif (v && v < MINIMUM_ABBREV)\n \t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (v > the_hash_algo->hexsz)\n+\t\telse if (startup_info->have_repository && v > the_hash_algo->hexsz)\n \t\t\tv = the_hash_algo->hexsz;\n \t}\n \t*(int *)(opt->value) = v;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 8bb2a8b453..45a773642f 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -176,6 +176,23 @@ test_expect_success 'long options' '\n \ttest_cmp expect output\n '\n \n+test_expect_success 'abbreviate to something longer than SHA1 length' '\n+\tcat >expect <<-EOF &&\n+\tboolean: 0\n+\tinteger: 0\n+\tmagnitude: 0\n+\ttimestamp: 0\n+\tstring: (not set)\n+\tabbrev: 100\n+\tverbose: -1\n+\tquiet: 0\n+\tdry run: no\n+\tfile: (not set)\n+\tEOF\n+\ttest-tool parse-options --abbrev=100 >output &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'missing required value' '\n \tcat >expect <<-\\EOF &&\n \terror: switch `s'\\'' requires a value\n-- \n2.45.0-rc0\n\n"},{"id":"493343","messageId":"1b5f904eedfd1bd3bc5d79cd43baf5e7a83b96aa.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 04/12] attr: don't recompute default attribute source","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:42Z","receivedAt":"2024-04-23T05:07:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `default_attr_source()` function lazily computes the attr source\nsupposedly once, only. This is done via a static variable `attr_source`\nthat contains the resolved object ID of the attr source's tree. If the\nvariable is the null object ID then we try to look up the attr source,\notherwise we skip over it.\n\nThis approach is flawed though: the variable will never be set to\nanything else but the null object ID in case there is no attr source.\nConsequently, we re-compute the information on every call. And in the\nworst case, when we silently ignore bad trees, this will cause us to try\nand look up the treeish every single time.\n\nImprove this by introducing a separate variable `has_attr_source` to\ntrack whether we already computed the attr source and, if so, whether we\nhave an attr source or not.\n\nThis also allows us to convert the `ignore_bad_attr_tree` to not be\nstatic anymore as the code will only be executed once anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 679e42258c..9d911aeb31 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1206,15 +1206,16 @@ static void collect_some_attrs(struct index_state *istate,\n }\n \n static const char *default_attr_source_tree_object_name;\n-static int ignore_bad_attr_tree;\n \n void set_git_attr_source(const char *tree_object_name)\n {\n \tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n }\n \n-static void compute_default_attr_source(struct object_id *attr_source)\n+static int compute_default_attr_source(struct object_id *attr_source)\n {\n+\tint ignore_bad_attr_tree = 0;\n+\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n@@ -1230,22 +1231,28 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \t\tignore_bad_attr_tree = 1;\n \t}\n \n-\tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n-\t\treturn;\n+\tif (!default_attr_source_tree_object_name)\n+\t\treturn 0;\n \n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n-\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n-\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\t\t\t attr_source)) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n }\n \n static struct object_id *default_attr_source(void)\n {\n \tstatic struct object_id attr_source;\n+\tstatic int has_attr_source = -1;\n \n-\tif (is_null_oid(&attr_source))\n-\t\tcompute_default_attr_source(&attr_source);\n-\tif (is_null_oid(&attr_source))\n+\tif (has_attr_source < 0)\n+\t\thas_attr_source = compute_default_attr_source(&attr_source);\n+\tif (!has_attr_source)\n \t\treturn NULL;\n \treturn &attr_source;\n }\n-- \n2.45.0-rc0\n\n"},{"id":"493344","messageId":"26909daca4ad416b1f3d0738751facf2d66d1aba.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 05/12] attr: fix BUG() when parsing attrs outside of repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:47Z","receivedAt":"2024-04-23T05:07:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are\nset, then `compute_default_attr_source()` will try to look up the value\nas a treeish. It is possible to hit that function while outside of a Git\nrepository though, for example when using `git grep --no-index`. In that\ncase, Git will hit a bug because we try to look up the main ref store\noutside of a repository.\n\nHandle the case gracefully and detect when we try to look up an attr\nsource without a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c                |  6 ++++++\n t/t0003-attributes.sh | 15 +++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/attr.c b/attr.c\nindex 9d911aeb31..4bd09bcb4b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1234,6 +1234,12 @@ static int compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\treturn 0;\n \n+\tif (!startup_info->have_repository) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"cannot use --attr-source or GIT_ATTR_SOURCE without repo\"));\n+\t\treturn 0;\n+\t}\n+\n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n \t\t\t\t attr_source)) {\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 774b52c298..3efdec54dd 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -428,6 +428,21 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre\n \t)\n '\n \n+test_expect_success 'diff without repository with attr source' '\n+\tmkdir -p \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t(\n+\t\tcd \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\ttouch file &&\n+\t\tcat >expect <<-EOF &&\n+\t\tfatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo\n+\t\tEOF\n+\t\ttest_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n-- \n2.45.0-rc0\n\n"},{"id":"493345","messageId":"0b99184f504bdab56b6d2d49f609b74a3e5c9c40.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 06/12] remote-curl: fix parsing of detached SHA256 heads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:52Z","receivedAt":"2024-04-23T05:07:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The dumb HTTP transport tries to read the remote HEAD reference by\ndownloading the \"HEAD\" file and then parsing it via `http_fetch_ref()`.\nThis function will either parse the file as an object ID in case it is\nexactly `the_hash_algo->hexsz` long, or otherwise it will check whether\nthe reference starts with \"ref :\" and parse it as a symbolic ref.\n\nThis is broken when parsing detached HEADs of a remote SHA256 repository\nbecause we never update `the_hash_algo` to the discovered remote object\nhash. Consequently, `the_hash_algo` will always be the fallback SHA1\nhash algorithm, which will cause us to fail parsing HEAD altogteher when\nit contains a SHA256 object ID.\n\nFix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`.\nWhile at it, let's make the expected SHA1 fallback explicit in our code,\nwhich also addresses an upcoming issue where we are going to remove the\nSHA1 fallback for `the_hash_algo`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n remote-curl.c              | 19 ++++++++++++++++++-\n t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0b6d7815fd..004b707fdf 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -266,12 +266,23 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \treturn list;\n }\n \n+/*\n+ * Try to detect the hash algorithm used by the remote repository when using\n+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash\n+ * directly have to derive it from the advertised ref lengths.\n+ */\n static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)\n {\n \tconst char *p = memchr(heads->buf, '\\t', heads->len);\n \tint algo;\n+\n+\t/*\n+\t * In case the remote has no refs we have no way to reliably determine\n+\t * the object hash used by that repository. In that case we simply fall\n+\t * back to SHA1, which may or may not be correct.\n+\t */\n \tif (!p)\n-\t\treturn the_hash_algo;\n+\t\treturn &hash_algos[GIT_HASH_SHA1];\n \n \talgo = hash_algo_by_length((p - heads->buf) / 2);\n \tif (algo == GIT_HASH_UNKNOWN)\n@@ -295,6 +306,12 @@ static struct ref *parse_info_refs(struct discovery *heads)\n \t\t    \"is this a git repository?\",\n \t\t    transport_anonymize_url(url.buf));\n \n+\t/*\n+\t * Set the repository's hash algo to whatever we have just detected.\n+\t * This ensures that we can correctly parse the remote references.\n+\t */\n+\trepo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));\n+\n \tdata = heads->buf;\n \tstart = NULL;\n \tmid = data;\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 4c3b32785d..5f16cbc58d 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -55,6 +55,21 @@ test_expect_success 'list refs from outside any repository' '\n \ttest_cmp expect actual\n '\n \n+\n+test_expect_success 'list detached HEAD from outside any repository' '\n+\tgit clone --mirror \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" \\\n+\t\tupdate-ref --no-deref HEAD refs/heads/main &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" update-server-info &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\tHEAD\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tnongit git ls-remote \"$HTTPD_URL/dumb/repo-detached.git\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create password-protected repository' '\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\n \tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n-- \n2.45.0-rc0\n\n"},{"id":"493346","messageId":"ccfda3c2d2bafd9803ac26936f872b59d14ee1b9.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 07/12] builtin/rev-parse: allow shortening to more than 40 hex characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:07:57Z","receivedAt":"2024-04-23T05:08:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--short=` option for git-rev-parse(1) allows the user to specify\nto how many characters object IDs should be shortened to. The option is\nbroken though for SHA256 repositories because we set the maximum allowed\nhash size to `the_hash_algo->hexsz` before we have even set up the repo.\nConsequently, `the_hash_algo` will always be SHA1 and thus we truncate\nevery hash after at most 40 characters.\n\nFix this by accessing `the_hash_algo` only after we have set up the\nrepo.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/rev-parse.c  | 5 ++---\n t/t1500-rev-parse.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 624182e507..d7b87c605e 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -687,7 +687,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tconst char *name = NULL;\n \tstruct object_context unused;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tint seen_end_of_options = 0;\n \tenum format_type format = FORMAT_DEFAULT;\n \n@@ -863,8 +862,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tabbrev = strtoul(arg, NULL, 10);\n \t\t\t\tif (abbrev < MINIMUM_ABBREV)\n \t\t\t\t\tabbrev = MINIMUM_ABBREV;\n-\t\t\t\telse if (hexsz <= abbrev)\n-\t\t\t\t\tabbrev = hexsz;\n+\t\t\t\telse if ((int)the_hash_algo->hexsz <= abbrev)\n+\t\t\t\t\tabbrev = the_hash_algo->hexsz;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--sq\")) {\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex a669e592f1..30c31918fd 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -304,4 +304,10 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--short= truncates to the actual hash length' '\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse --short=100 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.45.0-rc0\n\n"},{"id":"493347","messageId":"1813e7eb5c2a9e136f828a74c53843be2f13f90c.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 08/12] builtin/blame: don't access potentially unitialized `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:08:02Z","receivedAt":"2024-04-23T05:08:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We access `the_hash_algo` in git-blame(1) before we have executed\n`parse_options_start()`, which may not be properly set up in case we\nhave no repository. This is fine for most of the part because all the\ncall paths that lead to it (git-blame(1), git-annotate(1) as well as\ngit-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.\n\nThere is one exception though, namely when passing `-h` to print the\nhelp. Here we will access `the_hash_algo` even if there is no repo.\nThis works fine right now because `the_hash_algo` gets sets up to point\nto the SHA1 algorithm via `initialize_repository()`. But we're about to\nstop doing this, and thus the code would lead to a `NULL` pointer\nexception.\n\nPrepare the code for this and only access `the_hash_algo` after we are\nsure that there is a proper repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/blame.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 9aa74680a3..e325825936 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -915,7 +915,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct range_set ranges;\n \tunsigned int range_i;\n \tlong anchor;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tlong num_lines = 0;\n \tconst char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;\n \tconst char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;\n@@ -973,11 +972,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t} else if (show_progress < 0)\n \t\tshow_progress = isatty(2);\n \n-\tif (0 < abbrev && abbrev < hexsz)\n+\tif (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\n \telse if (!abbrev)\n-\t\tabbrev = hexsz;\n+\t\tabbrev = the_hash_algo->hexsz;\n \n \tif (revs_file && read_ancestry(revs_file))\n \t\tdie_errno(\"reading graft file '%s' failed\", revs_file);\n-- \n2.45.0-rc0\n\n"},{"id":"493348","messageId":"31182a1fc6a0123dd94fbfc3dce2bc49d7ff6086.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 09/12] builtin/bundle: abort \"verify\" early when there is no repository","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:08:07Z","receivedAt":"2024-04-23T05:08:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Verifying a bundle requires us to have a repository. This is encoded in\n`verify_bundle()`, which will return an error if there is no repository.\nWe call `open_bundle()` before we call `verify_bundle()` though, which\nalready performs some verifications even though we may ultimately abort\ndue to a missing repository.\n\nThis is problematic because `open_bundle()` already reads the bundle\nheader and verifies that it contains a properly formatted hash. When\nthere is no repository we have no clue what hash function to expect\nthough, so we always end up assuming SHA1 here, which may or may not be\ncorrect. Furthermore, we are about to stop initializing `the_hash_algo`\nwhen there is no repository, which will lead to segfaults.\n\nCheck early on whether we have a repository to fix this issue.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/bundle.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 3ad11dc5d0..d5d41a8f67 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -140,6 +140,11 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n+\tif (!startup_info->have_repository) {\n+\t\tret = error(_(\"need a repository to verify a bundle\"));\n+\t\tgoto cleanup;\n+\t}\n+\n \tif ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n-- \n2.45.0-rc0\n\n"},{"id":"493349","messageId":"78e19d0a1b09ed36bfd73fe128d2f78addcd1134.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 10/12] builtin/diff: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:08:11Z","receivedAt":"2024-04-23T05:08:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-diff(1) command can be used outside repositories to diff two\nfiles with each other. But even if there is no repository we will end up\nhashing the files that we are diffing so that we can print the \"index\"\nline:\n\n    ```\n    diff --git a/a b/b\n    index 7898192..6178079 100644\n    --- a/a\n    +++ b/b\n    @@ -1 +1 @@\n    -a\n    +b\n    ```\n\nWe implicitly use SHA1 to calculate the hash here, which is because\n`the_repository` gets initialized with SHA1 during the startup routine.\nWe are about to stop doing this though such that `the_repository` only\never has a hash function when it was properly initialized via a repo's\nconfiguration.\n\nTo give full control to our users, we would ideally add a new switch to\ngit-diff(1) that allows them to specify the hash function when executed\noutside of a repository. But for now, we only convert the code to make\nthis explicit such that we can stop setting the default hash algorithm\nfor `the_repository`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/diff.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 6e196e0c7d..58ec7e5da2 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \t}\n \n+\t/*\n+\t * When operating outside of a Git repository we need to have a hash\n+\t * algorithm at hand so that we can generate the blob hashes. We\n+\t * default to SHA1 here, but may eventually want to change this to be\n+\t * configurable via a command line option.\n+\t */\n+\tif (nongit)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tinit_diff_ui_defaults();\n \tgit_config(git_diff_ui_config, NULL);\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n-- \n2.45.0-rc0\n\n"},{"id":"493350","messageId":"51bcddbc3154addac208ba15866ff70becf90480.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 11/12] builtin/shortlog: don't set up revisions without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:08:16Z","receivedAt":"2024-04-23T05:08:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is possible to run git-shortlog(1) outside of a repository by passing\nit output from git-log(1) via standard input. Obviously, as there is no\nrepository in that context, it is thus unsupported to pass any revisions\nas arguments.\n\nRegardless of that we still end up calling `setup_revisions()`. While\nthat works alright, it is somewhat strange. Furthermore, this is about\nto cause problems when we unset the default object hash.\n\nRefactor the code to only call `setup_revisions()` when we have a\nrepository. This is safe to do as we already verify that there are no\narguments when running outside of a repository anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3c7cd2d6ef..d4daf31e22 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -435,7 +435,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n \n-\tif (setup_revisions(argc, argv, &rev, NULL) != 1) {\n+\tif (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {\n \t\terror(_(\"unrecognized argument: %s\"), argv[1]);\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n-- \n2.45.0-rc0\n\n"},{"id":"493351","messageId":"e8126371e1b8c4292d5e65a6a7822b56fb8e6fc5.1713848619.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"[PATCH v2 12/12] repository: stop setting SHA1 as the default object hash","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-23T05:08:21Z","receivedAt":"2024-04-23T05:08:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During the startup of Git, we call `initialize_the_repository()` to set\nup `the_repository` as well as `the_index`. Part of this setup is also\nto set the default object hash of the repository to SHA1. This has the\neffect that `the_hash_algo` is getting initialized to SHA1, as well.\nThis default hash algorithm eventually gets overridden by most Git\ncommands via `setup_git_directory()`, which also detects the actual hash\nalgorithm used by the repository.\n\nThere are some commands though that don't access a repository at all, or\nat a later point only, and thus retain the default hash function for\nsome amount of time. As some of the the preceding commits demonstrate,\nthis can lead to subtle issues when we access `the_hash_algo` when no\nrepository has been set up.\n\nAddress this issue by dropping the set up of the default hash algorithm\ncompletely. The effect of this is that `the_hash_algo` will map to a\n`NULL` pointer and thus cause Git to crash when something tries to\naccess the hash algorithm without it being properly initialized. It thus\nforces all Git commands to explicitly set up the hash algorithm in case\nthere is no repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repository.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/repository.c b/repository.c\nindex e15b416944..b65b1a8c8b 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -35,8 +35,6 @@ void initialize_the_repository(void)\n \tthe_repo.parsed_objects = parsed_object_pool_new();\n \n \tindex_state_init(&the_index, the_repository);\n-\n-\trepo_set_hash_algo(&the_repo, GIT_HASH_SHA1);\n }\n \n static void expand_base_dir(char **out, const char *in,\n-- \n2.45.0-rc0\n\n"},{"id":"493360","messageId":"xmqqttjsow8n.fsf@gitster.g","threadId":"61337","inReplyTo":"Zic-B7oSlv1PwvOL@tanuki","subject":"Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-23T16:54:32Z","receivedAt":"2024-04-23T16:54:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> All makes sense, and given the above, I strongly suspect that we\n>> would want to make the validate_headref() function a file-scope\n>> static in setup.c to make sure it is only called/callable from the\n>> repository discovery codepath.  Especially that if somebody calls\n>> this function again after we find out that the repository uses\n>> SHA-256, we would want to let the caller know that the detached HEAD\n>> records SHA-1 and we are in an inconsistent state.\n>\n> Fair enough, we can definitely do so. It only has a single callsite\n> anyway.\n\nI was wondering if I was missing a future plans to call it from\nother code paths that are triggered after the set-up was done.  If\nthat is not the case, we should do so.  It matters more for the\nfuture callers than the current ones.  They _all_ have to be aware\nof the deliberate looseness of the check here.\n\nThanks.  \n"},{"id":"493554","messageId":"xmqqwmoi31aw.fsf@gitster.g","threadId":"61337","inReplyTo":"cover.1713848619.git.ps@pks.im","subject":"Re: [PATCH v2 00/12] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-27T22:09:43Z","receivedAt":"2024-04-27T22:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n[cc: added Josh as the commit-graph fuzzer was his creation].\n\n> this is the second version of my patch series that causes us to stop\n> relying on the SHA1 default hash.\n\nWith this topic merged, 'seen' fails \"fuzz smoke test\"; I think this\n\n    https://github.com/git/git/actions/runs/8807729398/job/24175445340\n\nis the first merge of this topic into 'seen' where \"fuzz smoke test\"\nstarted failing.\n\nWith the merge of the topic from 'seen' reverted tentatively,\n\n    https://github.com/git/git/actions/runs/8862811497/job/24336185541\n\nthe same test seems happy.\n"},{"id":"493586","messageId":"Zi84rykU_qxE-aq6@tanuki","threadId":"61337","inReplyTo":"xmqqwmoi31aw.fsf@gitster.g","subject":"Re: [PATCH v2 00/12] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:05:46Z","receivedAt":"2024-04-29T06:05:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Apr 27, 2024 at 03:09:43PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> [cc: added Josh as the commit-graph fuzzer was his creation].\n> \n> > this is the second version of my patch series that causes us to stop\n> > relying on the SHA1 default hash.\n> \n> With this topic merged, 'seen' fails \"fuzz smoke test\"; I think this\n> \n>     https://github.com/git/git/actions/runs/8807729398/job/24175445340\n> \n> is the first merge of this topic into 'seen' where \"fuzz smoke test\"\n> started failing.\n> \n> With the merge of the topic from 'seen' reverted tentatively,\n> \n>     https://github.com/git/git/actions/runs/8862811497/job/24336185541\n> \n> the same test seems happy.\n\nIndeed, thanks for the heads up! Another test gap that we have in the\nGitLab CI setup. I'll add a separate patch series to plug this gap and\nadd the job to GitLab CI, too.\n\nThe fix for the failure is easy:\n\n    diff --git a/oss-fuzz/fuzz-commit-graph.c b/oss-fuzz/fuzz-commit-graph.c\n    index 2992079dd9..94ecbb9242 100644\n    --- a/oss-fuzz/fuzz-commit-graph.c\n    +++ b/oss-fuzz/fuzz-commit-graph.c\n    @@ -18,6 +18,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n         * touching the disk to keep the individual fuzz-test cases as fast as\n         * possible.\n         */\n    +\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n        the_repository->settings.commit_graph_generation_version = 2;\n        the_repository->settings.commit_graph_read_changed_paths = 1;\n        g = parse_commit_graph(&the_repository->settings, (void *)data, size);\n\nI'll send a v3.\n\nThanks!\n\nPatrick\n"},{"id":"493590","messageId":"cover.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH v3 00/13] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:09Z","receivedAt":"2024-04-29T06:34:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series that stops relying on the\nSHA1 fallback configured for `the_hash_algo`.\n\nThere's only a single change compared to v2, which is a new patch that\nfixes a segfault in the commit-graph fuzzer.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (13):\n  path: harden validation of HEAD with non-standard hashes\n  path: move `validate_headref()` to its only user\n  parse-options-cb: only abbreviate hashes when hash algo is known\n  attr: don't recompute default attribute source\n  attr: fix BUG() when parsing attrs outside of repo\n  remote-curl: fix parsing of detached SHA256 heads\n  builtin/rev-parse: allow shortening to more than 40 hex characters\n  builtin/blame: don't access potentially unitialized `the_hash_algo`\n  builtin/bundle: abort \"verify\" early when there is no repository\n  builtin/diff: explicitly set hash algo when there is no repo\n  builtin/shortlog: don't set up revisions without repo\n  oss-fuzz/commit-graph: set up hash algorithm\n  repository: stop setting SHA1 as the default object hash\n\n attr.c                       | 31 +++++++++++++++------\n builtin/blame.c              |  5 ++--\n builtin/bundle.c             |  5 ++++\n builtin/diff.c               |  9 ++++++\n builtin/rev-parse.c          |  5 ++--\n builtin/shortlog.c           |  2 +-\n oss-fuzz/fuzz-commit-graph.c |  1 +\n parse-options-cb.c           |  3 +-\n path.c                       | 53 ------------------------------------\n path.h                       |  1 -\n remote-curl.c                | 19 ++++++++++++-\n repository.c                 |  2 --\n setup.c                      | 53 ++++++++++++++++++++++++++++++++++++\n t/t0003-attributes.sh        | 15 ++++++++++\n t/t0040-parse-options.sh     | 17 ++++++++++++\n t/t1500-rev-parse.sh         |  6 ++++\n t/t5550-http-fetch-dumb.sh   | 15 ++++++++++\n 17 files changed, 168 insertions(+), 74 deletions(-)\n\nRange-diff against v2:\n 1:  a986b464d3 =  1:  5134f35cda path: harden validation of HEAD with non-standard hashes\n 2:  a347c7e6ca =  2:  589b6a99ef path: move `validate_headref()` to its only user\n 3:  c0a15b2fa6 =  3:  9a63c445d2 parse-options-cb: only abbreviate hashes when hash algo is known\n 4:  1b5f904eed =  4:  929bacbfce attr: don't recompute default attribute source\n 5:  26909daca4 =  5:  8f20aec1ee attr: fix BUG() when parsing attrs outside of repo\n 6:  0b99184f50 =  6:  53439067a1 remote-curl: fix parsing of detached SHA256 heads\n 7:  ccfda3c2d2 =  7:  1f74960760 builtin/rev-parse: allow shortening to more than 40 hex characters\n 8:  1813e7eb5c =  8:  2d985abca1 builtin/blame: don't access potentially unitialized `the_hash_algo`\n 9:  31182a1fc6 =  9:  f3b23d28aa builtin/bundle: abort \"verify\" early when there is no repository\n10:  78e19d0a1b = 10:  7577b6b96c builtin/diff: explicitly set hash algo when there is no repo\n11:  51bcddbc31 = 11:  509c79d1d3 builtin/shortlog: don't set up revisions without repo\n -:  ---------- > 12:  660f976129 oss-fuzz/commit-graph: set up hash algorithm\n12:  e8126371e1 = 13:  95909c2da5 repository: stop setting SHA1 as the default object hash\n-- \n2.45.0-rc1\n\n"},{"id":"493591","messageId":"5134f35cda5ab47429411293dbc0a987a306c9d1.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 01/13] path: harden validation of HEAD with non-standard hashes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:14Z","receivedAt":"2024-04-29T06:34:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `validate_headref()` function takes a path to a supposed \"HEAD\" file\nand checks whether its format is something that we understand. It is\nused as part of our repository discovery to check whether a specific\ndirectory is a Git directory or not.\n\nPart of the validation is a check for a detached HEAD that contains a\nplain object ID. To do this validation we use `get_oid_hex()`, which\nrelies on `the_hash_algo`. At this point in time the hash algo cannot\nyet be initialized though because we didn't yet read the Git config.\nConsequently, it will always be the SHA1 hash algorithm.\n\nIn practice this works alright because `get_oid_hex()` only ends up\nchecking whether the prefix of the buffer is a valid object ID. And\nbecause SHA1 is shorter than SHA256, the function will successfully\nparse SHA256 object IDs, as well.\n\nIt is somewhat fragile though and not really the intent to only check\nfor SHA1. With this in mind, harden the code to use `get_oid_hex_any()`\nto check whether the \"HEAD\" file parses as any known hash.\n\nOne might be hard pressed to tighten the check even further and fully\nvalidate the file contents, not only the prefix. In practice though that\nwouldn't make a lot of sense as it could be that the repository uses a\nhash function that produces longer hashes than SHA256, but which the\ncurrent version of Git doesn't understand yet. We'd still want to detect\nthe repository as proper Git repository in that case, and we will fail\neventually with a proper error message that the hash isn't understood\nwhen trying to set up the repository format.\n\nIt follows that we could just leave the current code intact, as in\npractice the code change doesn't have any user visible impact. But it\nalso prepares us for `the_hash_algo` being unset when there is no\nrepository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n path.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/path.c b/path.c\nindex 67229edb9c..cc02165530 100644\n--- a/path.c\n+++ b/path.c\n@@ -693,7 +693,7 @@ int validate_headref(const char *path)\n \t/*\n \t * Is this a detached HEAD?\n \t */\n-\tif (!get_oid_hex(buffer, &oid))\n+\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n \t\treturn 0;\n \n \treturn -1;\n-- \n2.45.0-rc1\n\n"},{"id":"493592","messageId":"589b6a99efffc9d3f5800cc3dea8ce3367d1e9fe.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 02/13] path: move `validate_headref()` to its only user","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:19Z","receivedAt":"2024-04-29T06:34:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While `validate_headref()` is only called from `is_git_directory()` in\n\"setup.c\", it is currently implemented in \"path.c\". Move it over such\nthat it becomes clear that it is only really used during setup in order\nto discover repositories.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n path.c  | 53 -----------------------------------------------------\n path.h  |  1 -\n setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 53 insertions(+), 54 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex cc02165530..bd6e25245d 100644\n--- a/path.c\n+++ b/path.c\n@@ -5,7 +5,6 @@\n #include \"abspath.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n-#include \"hex.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n@@ -647,58 +646,6 @@ void strbuf_git_common_path(struct strbuf *sb,\n \tva_end(args);\n }\n \n-int validate_headref(const char *path)\n-{\n-\tstruct stat st;\n-\tchar buffer[256];\n-\tconst char *refname;\n-\tstruct object_id oid;\n-\tint fd;\n-\tssize_t len;\n-\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\n-\t/* Make sure it is a \"refs/..\" symlink */\n-\tif (S_ISLNK(st.st_mode)) {\n-\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n-\t\t\treturn 0;\n-\t\treturn -1;\n-\t}\n-\n-\t/*\n-\t * Anything else, just open it and try to see if it is a symbolic ref.\n-\t */\n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn -1;\n-\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n-\tclose(fd);\n-\n-\tif (len < 0)\n-\t\treturn -1;\n-\tbuffer[len] = '\\0';\n-\n-\t/*\n-\t * Is it a symbolic ref?\n-\t */\n-\tif (skip_prefix(buffer, \"ref:\", &refname)) {\n-\t\twhile (isspace(*refname))\n-\t\t\trefname++;\n-\t\tif (starts_with(refname, \"refs/\"))\n-\t\t\treturn 0;\n-\t}\n-\n-\t/*\n-\t * Is this a detached HEAD?\n-\t */\n-\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n-\t\treturn 0;\n-\n-\treturn -1;\n-}\n-\n static struct passwd *getpw_str(const char *username, size_t len)\n {\n \tstruct passwd *pw;\ndiff --git a/path.h b/path.h\nindex ea96487b29..c3bc8617bd 100644\n--- a/path.h\n+++ b/path.h\n@@ -173,7 +173,6 @@ const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \n int ends_with_path_components(const char *path, const char *components);\n-int validate_headref(const char *ref);\n \n int calc_shared_perm(int mode);\n int adjust_shared_perm(const char *path);\ndiff --git a/setup.c b/setup.c\nindex f4b32f76e3..7c996659bd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -4,6 +4,7 @@\n #include \"environment.h\"\n #include \"exec-cmd.h\"\n #include \"gettext.h\"\n+#include \"hex.h\"\n #include \"object-name.h\"\n #include \"refs.h\"\n #include \"repository.h\"\n@@ -341,6 +342,58 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n \treturn ret;\n }\n \n+static int validate_headref(const char *path)\n+{\n+\tstruct stat st;\n+\tchar buffer[256];\n+\tconst char *refname;\n+\tstruct object_id oid;\n+\tint fd;\n+\tssize_t len;\n+\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\n+\t/* Make sure it is a \"refs/..\" symlink */\n+\tif (S_ISLNK(st.st_mode)) {\n+\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n+\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n+\t\t\treturn 0;\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Anything else, just open it and try to see if it is a symbolic ref.\n+\t */\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn -1;\n+\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n+\tclose(fd);\n+\n+\tif (len < 0)\n+\t\treturn -1;\n+\tbuffer[len] = '\\0';\n+\n+\t/*\n+\t * Is it a symbolic ref?\n+\t */\n+\tif (skip_prefix(buffer, \"ref:\", &refname)) {\n+\t\twhile (isspace(*refname))\n+\t\t\trefname++;\n+\t\tif (starts_with(refname, \"refs/\"))\n+\t\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * Is this a detached HEAD?\n+\t */\n+\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n+\t\treturn 0;\n+\n+\treturn -1;\n+}\n+\n /*\n  * Test if it looks like we're at a git directory.\n  * We want to see:\n-- \n2.45.0-rc1\n\n"},{"id":"493593","messageId":"9a63c445d21fb6d9179c6fb8a96e0c221ddf16a4.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 03/13] parse-options-cb: only abbreviate hashes when hash algo is known","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:23Z","receivedAt":"2024-04-29T06:34:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPT__ABBREV()` option can be used to add an option that abbreviates\nobject IDs. When given a length longer than `the_hash_algo->hexsz`, then\nit will instead set the length to that maximum length.\n\nIt may not always be guaranteed that we have `the_hash_algo` initialized\nproperly as the hash algorithm can only be set up after we have set up\n`the_repository`. In that case, the hash would always be truncated to\nthe hex length of SHA1, which may not be what the user desires.\n\nIn practice it's not a problem as all commands that use `OPT__ABBREV()`\nalso have `RUN_SETUP` set and thus cannot work without a repository.\nConsequently, both `the_repository` and `the_hash_algo` would be\nproperly set up.\n\nRegardless of that, harden the code to not truncate the length when we\ndidn't set up a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n parse-options-cb.c       |  3 ++-\n t/t0040-parse-options.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex bdc7fae497..d99d688d3c 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -7,6 +7,7 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"setup.h\"\n #include \"string-list.h\"\n #include \"strvec.h\"\n #include \"oid-array.h\"\n@@ -29,7 +30,7 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \t\t\t\t     opt->long_name);\n \t\tif (v && v < MINIMUM_ABBREV)\n \t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (v > the_hash_algo->hexsz)\n+\t\telse if (startup_info->have_repository && v > the_hash_algo->hexsz)\n \t\t\tv = the_hash_algo->hexsz;\n \t}\n \t*(int *)(opt->value) = v;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 8bb2a8b453..45a773642f 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -176,6 +176,23 @@ test_expect_success 'long options' '\n \ttest_cmp expect output\n '\n \n+test_expect_success 'abbreviate to something longer than SHA1 length' '\n+\tcat >expect <<-EOF &&\n+\tboolean: 0\n+\tinteger: 0\n+\tmagnitude: 0\n+\ttimestamp: 0\n+\tstring: (not set)\n+\tabbrev: 100\n+\tverbose: -1\n+\tquiet: 0\n+\tdry run: no\n+\tfile: (not set)\n+\tEOF\n+\ttest-tool parse-options --abbrev=100 >output &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'missing required value' '\n \tcat >expect <<-\\EOF &&\n \terror: switch `s'\\'' requires a value\n-- \n2.45.0-rc1\n\n"},{"id":"493594","messageId":"929bacbfceeb1627b257f68d9f66155878a11e18.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 04/13] attr: don't recompute default attribute source","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:28Z","receivedAt":"2024-04-29T06:34:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `default_attr_source()` function lazily computes the attr source\nsupposedly once, only. This is done via a static variable `attr_source`\nthat contains the resolved object ID of the attr source's tree. If the\nvariable is the null object ID then we try to look up the attr source,\notherwise we skip over it.\n\nThis approach is flawed though: the variable will never be set to\nanything else but the null object ID in case there is no attr source.\nConsequently, we re-compute the information on every call. And in the\nworst case, when we silently ignore bad trees, this will cause us to try\nand look up the treeish every single time.\n\nImprove this by introducing a separate variable `has_attr_source` to\ntrack whether we already computed the attr source and, if so, whether we\nhave an attr source or not.\n\nThis also allows us to convert the `ignore_bad_attr_tree` to not be\nstatic anymore as the code will only be executed once anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 679e42258c..9d911aeb31 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1206,15 +1206,16 @@ static void collect_some_attrs(struct index_state *istate,\n }\n \n static const char *default_attr_source_tree_object_name;\n-static int ignore_bad_attr_tree;\n \n void set_git_attr_source(const char *tree_object_name)\n {\n \tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n }\n \n-static void compute_default_attr_source(struct object_id *attr_source)\n+static int compute_default_attr_source(struct object_id *attr_source)\n {\n+\tint ignore_bad_attr_tree = 0;\n+\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n@@ -1230,22 +1231,28 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \t\tignore_bad_attr_tree = 1;\n \t}\n \n-\tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n-\t\treturn;\n+\tif (!default_attr_source_tree_object_name)\n+\t\treturn 0;\n \n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n-\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n-\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\t\t\t attr_source)) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n }\n \n static struct object_id *default_attr_source(void)\n {\n \tstatic struct object_id attr_source;\n+\tstatic int has_attr_source = -1;\n \n-\tif (is_null_oid(&attr_source))\n-\t\tcompute_default_attr_source(&attr_source);\n-\tif (is_null_oid(&attr_source))\n+\tif (has_attr_source < 0)\n+\t\thas_attr_source = compute_default_attr_source(&attr_source);\n+\tif (!has_attr_source)\n \t\treturn NULL;\n \treturn &attr_source;\n }\n-- \n2.45.0-rc1\n\n"},{"id":"493595","messageId":"8f20aec1ee046ca5d21fe4ce2b19e24869545a42.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 05/13] attr: fix BUG() when parsing attrs outside of repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:33Z","receivedAt":"2024-04-29T06:34:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are\nset, then `compute_default_attr_source()` will try to look up the value\nas a treeish. It is possible to hit that function while outside of a Git\nrepository though, for example when using `git grep --no-index`. In that\ncase, Git will hit a bug because we try to look up the main ref store\noutside of a repository.\n\nHandle the case gracefully and detect when we try to look up an attr\nsource without a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c                |  6 ++++++\n t/t0003-attributes.sh | 15 +++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/attr.c b/attr.c\nindex 9d911aeb31..4bd09bcb4b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1234,6 +1234,12 @@ static int compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\treturn 0;\n \n+\tif (!startup_info->have_repository) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"cannot use --attr-source or GIT_ATTR_SOURCE without repo\"));\n+\t\treturn 0;\n+\t}\n+\n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n \t\t\t\t attr_source)) {\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 774b52c298..3efdec54dd 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -428,6 +428,21 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre\n \t)\n '\n \n+test_expect_success 'diff without repository with attr source' '\n+\tmkdir -p \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t(\n+\t\tcd \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\ttouch file &&\n+\t\tcat >expect <<-EOF &&\n+\t\tfatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo\n+\t\tEOF\n+\t\ttest_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n-- \n2.45.0-rc1\n\n"},{"id":"493596","messageId":"53439067a1e94075c3f432b6c2774b1cfb11e88c.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 06/13] remote-curl: fix parsing of detached SHA256 heads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:38Z","receivedAt":"2024-04-29T06:34:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The dumb HTTP transport tries to read the remote HEAD reference by\ndownloading the \"HEAD\" file and then parsing it via `http_fetch_ref()`.\nThis function will either parse the file as an object ID in case it is\nexactly `the_hash_algo->hexsz` long, or otherwise it will check whether\nthe reference starts with \"ref :\" and parse it as a symbolic ref.\n\nThis is broken when parsing detached HEADs of a remote SHA256 repository\nbecause we never update `the_hash_algo` to the discovered remote object\nhash. Consequently, `the_hash_algo` will always be the fallback SHA1\nhash algorithm, which will cause us to fail parsing HEAD altogteher when\nit contains a SHA256 object ID.\n\nFix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`.\nWhile at it, let's make the expected SHA1 fallback explicit in our code,\nwhich also addresses an upcoming issue where we are going to remove the\nSHA1 fallback for `the_hash_algo`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n remote-curl.c              | 19 ++++++++++++++++++-\n t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0b6d7815fd..004b707fdf 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -266,12 +266,23 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \treturn list;\n }\n \n+/*\n+ * Try to detect the hash algorithm used by the remote repository when using\n+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash\n+ * directly have to derive it from the advertised ref lengths.\n+ */\n static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)\n {\n \tconst char *p = memchr(heads->buf, '\\t', heads->len);\n \tint algo;\n+\n+\t/*\n+\t * In case the remote has no refs we have no way to reliably determine\n+\t * the object hash used by that repository. In that case we simply fall\n+\t * back to SHA1, which may or may not be correct.\n+\t */\n \tif (!p)\n-\t\treturn the_hash_algo;\n+\t\treturn &hash_algos[GIT_HASH_SHA1];\n \n \talgo = hash_algo_by_length((p - heads->buf) / 2);\n \tif (algo == GIT_HASH_UNKNOWN)\n@@ -295,6 +306,12 @@ static struct ref *parse_info_refs(struct discovery *heads)\n \t\t    \"is this a git repository?\",\n \t\t    transport_anonymize_url(url.buf));\n \n+\t/*\n+\t * Set the repository's hash algo to whatever we have just detected.\n+\t * This ensures that we can correctly parse the remote references.\n+\t */\n+\trepo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));\n+\n \tdata = heads->buf;\n \tstart = NULL;\n \tmid = data;\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 4c3b32785d..5f16cbc58d 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -55,6 +55,21 @@ test_expect_success 'list refs from outside any repository' '\n \ttest_cmp expect actual\n '\n \n+\n+test_expect_success 'list detached HEAD from outside any repository' '\n+\tgit clone --mirror \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" \\\n+\t\tupdate-ref --no-deref HEAD refs/heads/main &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" update-server-info &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\tHEAD\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tnongit git ls-remote \"$HTTPD_URL/dumb/repo-detached.git\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create password-protected repository' '\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\n \tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n-- \n2.45.0-rc1\n\n"},{"id":"493597","messageId":"1f749607604b80517b60b3b696b22287aca0ab54.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 07/13] builtin/rev-parse: allow shortening to more than 40 hex characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:43Z","receivedAt":"2024-04-29T06:34:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--short=` option for git-rev-parse(1) allows the user to specify\nto how many characters object IDs should be shortened to. The option is\nbroken though for SHA256 repositories because we set the maximum allowed\nhash size to `the_hash_algo->hexsz` before we have even set up the repo.\nConsequently, `the_hash_algo` will always be SHA1 and thus we truncate\nevery hash after at most 40 characters.\n\nFix this by accessing `the_hash_algo` only after we have set up the\nrepo.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/rev-parse.c  | 5 ++---\n t/t1500-rev-parse.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 624182e507..d7b87c605e 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -687,7 +687,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tconst char *name = NULL;\n \tstruct object_context unused;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tint seen_end_of_options = 0;\n \tenum format_type format = FORMAT_DEFAULT;\n \n@@ -863,8 +862,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tabbrev = strtoul(arg, NULL, 10);\n \t\t\t\tif (abbrev < MINIMUM_ABBREV)\n \t\t\t\t\tabbrev = MINIMUM_ABBREV;\n-\t\t\t\telse if (hexsz <= abbrev)\n-\t\t\t\t\tabbrev = hexsz;\n+\t\t\t\telse if ((int)the_hash_algo->hexsz <= abbrev)\n+\t\t\t\t\tabbrev = the_hash_algo->hexsz;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--sq\")) {\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex a669e592f1..30c31918fd 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -304,4 +304,10 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--short= truncates to the actual hash length' '\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse --short=100 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.45.0-rc1\n\n"},{"id":"493598","messageId":"2d985abca1c8f99d888816965dc87caf70774a86.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 08/13] builtin/blame: don't access potentially unitialized `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:47Z","receivedAt":"2024-04-29T06:34:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We access `the_hash_algo` in git-blame(1) before we have executed\n`parse_options_start()`, which may not be properly set up in case we\nhave no repository. This is fine for most of the part because all the\ncall paths that lead to it (git-blame(1), git-annotate(1) as well as\ngit-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.\n\nThere is one exception though, namely when passing `-h` to print the\nhelp. Here we will access `the_hash_algo` even if there is no repo.\nThis works fine right now because `the_hash_algo` gets sets up to point\nto the SHA1 algorithm via `initialize_repository()`. But we're about to\nstop doing this, and thus the code would lead to a `NULL` pointer\nexception.\n\nPrepare the code for this and only access `the_hash_algo` after we are\nsure that there is a proper repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/blame.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 9aa74680a3..e325825936 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -915,7 +915,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct range_set ranges;\n \tunsigned int range_i;\n \tlong anchor;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tlong num_lines = 0;\n \tconst char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;\n \tconst char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;\n@@ -973,11 +972,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t} else if (show_progress < 0)\n \t\tshow_progress = isatty(2);\n \n-\tif (0 < abbrev && abbrev < hexsz)\n+\tif (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\n \telse if (!abbrev)\n-\t\tabbrev = hexsz;\n+\t\tabbrev = the_hash_algo->hexsz;\n \n \tif (revs_file && read_ancestry(revs_file))\n \t\tdie_errno(\"reading graft file '%s' failed\", revs_file);\n-- \n2.45.0-rc1\n\n"},{"id":"493599","messageId":"f3b23d28aa7443052eed80c1fbc90c994c2c910b.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 09/13] builtin/bundle: abort \"verify\" early when there is no repository","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:52Z","receivedAt":"2024-04-29T06:34:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Verifying a bundle requires us to have a repository. This is encoded in\n`verify_bundle()`, which will return an error if there is no repository.\nWe call `open_bundle()` before we call `verify_bundle()` though, which\nalready performs some verifications even though we may ultimately abort\ndue to a missing repository.\n\nThis is problematic because `open_bundle()` already reads the bundle\nheader and verifies that it contains a properly formatted hash. When\nthere is no repository we have no clue what hash function to expect\nthough, so we always end up assuming SHA1 here, which may or may not be\ncorrect. Furthermore, we are about to stop initializing `the_hash_algo`\nwhen there is no repository, which will lead to segfaults.\n\nCheck early on whether we have a repository to fix this issue.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/bundle.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 3ad11dc5d0..d5d41a8f67 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -140,6 +140,11 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n+\tif (!startup_info->have_repository) {\n+\t\tret = error(_(\"need a repository to verify a bundle\"));\n+\t\tgoto cleanup;\n+\t}\n+\n \tif ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n-- \n2.45.0-rc1\n\n"},{"id":"493600","messageId":"7577b6b96ca2d1d7070ff92e13fccd14d2e5f86e.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 10/13] builtin/diff: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:34:57Z","receivedAt":"2024-04-29T06:35:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-diff(1) command can be used outside repositories to diff two\nfiles with each other. But even if there is no repository we will end up\nhashing the files that we are diffing so that we can print the \"index\"\nline:\n\n    ```\n    diff --git a/a b/b\n    index 7898192..6178079 100644\n    --- a/a\n    +++ b/b\n    @@ -1 +1 @@\n    -a\n    +b\n    ```\n\nWe implicitly use SHA1 to calculate the hash here, which is because\n`the_repository` gets initialized with SHA1 during the startup routine.\nWe are about to stop doing this though such that `the_repository` only\never has a hash function when it was properly initialized via a repo's\nconfiguration.\n\nTo give full control to our users, we would ideally add a new switch to\ngit-diff(1) that allows them to specify the hash function when executed\noutside of a repository. But for now, we only convert the code to make\nthis explicit such that we can stop setting the default hash algorithm\nfor `the_repository`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/diff.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 6e196e0c7d..58ec7e5da2 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \t}\n \n+\t/*\n+\t * When operating outside of a Git repository we need to have a hash\n+\t * algorithm at hand so that we can generate the blob hashes. We\n+\t * default to SHA1 here, but may eventually want to change this to be\n+\t * configurable via a command line option.\n+\t */\n+\tif (nongit)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tinit_diff_ui_defaults();\n \tgit_config(git_diff_ui_config, NULL);\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n-- \n2.45.0-rc1\n\n"},{"id":"493601","messageId":"509c79d1d33a69367726549fcf87f932f83db864.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 11/13] builtin/shortlog: don't set up revisions without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:35:02Z","receivedAt":"2024-04-29T06:35:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is possible to run git-shortlog(1) outside of a repository by passing\nit output from git-log(1) via standard input. Obviously, as there is no\nrepository in that context, it is thus unsupported to pass any revisions\nas arguments.\n\nRegardless of that we still end up calling `setup_revisions()`. While\nthat works alright, it is somewhat strange. Furthermore, this is about\nto cause problems when we unset the default object hash.\n\nRefactor the code to only call `setup_revisions()` when we have a\nrepository. This is safe to do as we already verify that there are no\narguments when running outside of a repository anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3c7cd2d6ef..d4daf31e22 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -435,7 +435,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n \n-\tif (setup_revisions(argc, argv, &rev, NULL) != 1) {\n+\tif (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {\n \t\terror(_(\"unrecognized argument: %s\"), argv[1]);\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n-- \n2.45.0-rc1\n\n"},{"id":"493602","messageId":"660f976129ece3398465053884fbac62a6341bee.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 12/13] oss-fuzz/commit-graph: set up hash algorithm","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:35:06Z","receivedAt":"2024-04-29T06:35:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Our fuzzing setups don't work in a proper repository, but only use the\nin-memory configured `the_repository`. Consequently, we never go through\nthe full repository setup procedures and thus do not set up the hash\nalgo used by the repository.\n\nThe commit-graph fuzzer does rely on a properly initialized hash algo\nthough. Initialize it explicitly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n oss-fuzz/fuzz-commit-graph.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/oss-fuzz/fuzz-commit-graph.c b/oss-fuzz/fuzz-commit-graph.c\nindex 2992079dd9..94ecbb9242 100644\n--- a/oss-fuzz/fuzz-commit-graph.c\n+++ b/oss-fuzz/fuzz-commit-graph.c\n@@ -18,6 +18,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \t * touching the disk to keep the individual fuzz-test cases as fast as\n \t * possible.\n \t */\n+\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n \tthe_repository->settings.commit_graph_generation_version = 2;\n \tthe_repository->settings.commit_graph_read_changed_paths = 1;\n \tg = parse_commit_graph(&the_repository->settings, (void *)data, size);\n-- \n2.45.0-rc1\n\n"},{"id":"493603","messageId":"95909c2da57f6d9814561c94d75be4582195bb5f.1714371422.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1714371422.git.ps@pks.im","subject":"[PATCH v3 13/13] repository: stop setting SHA1 as the default object hash","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T06:35:11Z","receivedAt":"2024-04-29T06:35:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During the startup of Git, we call `initialize_the_repository()` to set\nup `the_repository` as well as `the_index`. Part of this setup is also\nto set the default object hash of the repository to SHA1. This has the\neffect that `the_hash_algo` is getting initialized to SHA1, as well.\nThis default hash algorithm eventually gets overridden by most Git\ncommands via `setup_git_directory()`, which also detects the actual hash\nalgorithm used by the repository.\n\nThere are some commands though that don't access a repository at all, or\nat a later point only, and thus retain the default hash function for\nsome amount of time. As some of the the preceding commits demonstrate,\nthis can lead to subtle issues when we access `the_hash_algo` when no\nrepository has been set up.\n\nAddress this issue by dropping the set up of the default hash algorithm\ncompletely. The effect of this is that `the_hash_algo` will map to a\n`NULL` pointer and thus cause Git to crash when something tries to\naccess the hash algorithm without it being properly initialized. It thus\nforces all Git commands to explicitly set up the hash algorithm in case\nthere is no repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repository.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/repository.c b/repository.c\nindex e15b416944..b65b1a8c8b 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -35,8 +35,6 @@ void initialize_the_repository(void)\n \tthe_repo.parsed_objects = parsed_object_pool_new();\n \n \tindex_state_init(&the_index, the_repository);\n-\n-\trepo_set_hash_algo(&the_repo, GIT_HASH_SHA1);\n }\n \n static void expand_base_dir(char **out, const char *in,\n-- \n2.45.0-rc1\n\n"},{"id":"494222","messageId":"cover.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1713519789.git.ps@pks.im","subject":"[PATCH v4 00/13] Stop relying on SHA1 fallback for `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:52:42Z","receivedAt":"2024-05-07T04:52:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series that stops relying on the\nSHA1 fallback configured for `the_hash_algo`. Instead, any callers that\naccess `the_hash_algo` without it being initialized will now crash hard.\nThis is designed to catch various subtle bugs going forward.\n\nThe only change compared to v3 of this patch series is a rebase on top\nof the following two topics:\n\n  - ps/the-index-is-no-more, which causes a conflict in the final patch.\n\n  - jc/no-default-attr-tree-in-bare, which causes a conflict in the\n    fourth patch.\n\nInterestingly, the range diff does not surface the second resolved\nconflict. This is because the context is actually the same, but when\napplying the patch in question you would get a conflict. To aid with\nthis I decided to generate patches with `--unified=4`, which surfaces\nthe previously-conflicting hunk in `compute_default_attr_source()`.\n\nPatrick\n\nPatrick Steinhardt (13):\n  path: harden validation of HEAD with non-standard hashes\n  path: move `validate_headref()` to its only user\n  parse-options-cb: only abbreviate hashes when hash algo is known\n  attr: don't recompute default attribute source\n  attr: fix BUG() when parsing attrs outside of repo\n  remote-curl: fix parsing of detached SHA256 heads\n  builtin/rev-parse: allow shortening to more than 40 hex characters\n  builtin/blame: don't access potentially unitialized `the_hash_algo`\n  builtin/bundle: abort \"verify\" early when there is no repository\n  builtin/diff: explicitly set hash algo when there is no repo\n  builtin/shortlog: don't set up revisions without repo\n  oss-fuzz/commit-graph: set up hash algorithm\n  repository: stop setting SHA1 as the default object hash\n\n attr.c                       | 31 +++++++++++++++------\n builtin/blame.c              |  5 ++--\n builtin/bundle.c             |  5 ++++\n builtin/diff.c               |  9 ++++++\n builtin/rev-parse.c          |  5 ++--\n builtin/shortlog.c           |  2 +-\n oss-fuzz/fuzz-commit-graph.c |  1 +\n parse-options-cb.c           |  3 +-\n path.c                       | 53 ------------------------------------\n path.h                       |  1 -\n remote-curl.c                | 19 ++++++++++++-\n repository.c                 | 20 --------------\n setup.c                      | 53 ++++++++++++++++++++++++++++++++++++\n t/t0003-attributes.sh        | 15 ++++++++++\n t/t0040-parse-options.sh     | 17 ++++++++++++\n t/t1500-rev-parse.sh         |  6 ++++\n t/t5550-http-fetch-dumb.sh   | 15 ++++++++++\n 17 files changed, 168 insertions(+), 92 deletions(-)\n\nRange-diff against v3:\n 1:  5134f35cda =  1:  5ee372c2d8 path: harden validation of HEAD with non-standard hashes\n 2:  589b6a99ef =  2:  ece0ab94a8 path: move `validate_headref()` to its only user\n 3:  9a63c445d2 =  3:  1a0859eaf1 parse-options-cb: only abbreviate hashes when hash algo is known\n 4:  929bacbfce =  4:  1204a34216 attr: don't recompute default attribute source\n 5:  8f20aec1ee =  5:  f098ce9690 attr: fix BUG() when parsing attrs outside of repo\n 6:  53439067a1 =  6:  e8876052ad remote-curl: fix parsing of detached SHA256 heads\n 7:  1f74960760 =  7:  6116030310 builtin/rev-parse: allow shortening to more than 40 hex characters\n 8:  2d985abca1 =  8:  872ded113e builtin/blame: don't access potentially unitialized `the_hash_algo`\n 9:  f3b23d28aa =  9:  5b4a21d2ce builtin/bundle: abort \"verify\" early when there is no repository\n10:  7577b6b96c = 10:  e7254ae757 builtin/diff: explicitly set hash algo when there is no repo\n11:  509c79d1d3 = 11:  c21b15ba17 builtin/shortlog: don't set up revisions without repo\n12:  660f976129 = 12:  f37beb0e89 oss-fuzz/commit-graph: set up hash algorithm\n13:  95909c2da5 ! 13:  950b08bc78 repository: stop setting SHA1 as the default object hash\n    @@ Commit message\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## repository.c ##\n    -@@ repository.c: void initialize_the_repository(void)\n    - \tthe_repo.parsed_objects = parsed_object_pool_new();\n    - \n    - \tindex_state_init(&the_index, the_repository);\n    +@@ repository.c: void initialize_repository(struct repository *repo)\n    + \trepo->parsed_objects = parsed_object_pool_new();\n    + \tALLOC_ARRAY(repo->index, 1);\n    + \tindex_state_init(repo->index, repo);\n     -\n    --\trepo_set_hash_algo(&the_repo, GIT_HASH_SHA1);\n    +-\t/*\n    +-\t * Unfortunately, we need to keep this hack around for the time being:\n    +-\t *\n    +-\t *   - Not setting up the hash algorithm for `the_repository` leads to\n    +-\t *     crashes because `the_hash_algo` is a macro that expands to\n    +-\t *     `the_repository->hash_algo`. So if Git commands try to access\n    +-\t *     `the_hash_algo` without a Git directory we crash.\n    +-\t *\n    +-\t *   - Setting up the hash algorithm to be SHA1 by default breaks other\n    +-\t *     commands when running with SHA256.\n    +-\t *\n    +-\t * This is another point in case why having global state is a bad idea.\n    +-\t * Eventually, we should remove this hack and stop setting the hash\n    +-\t * algorithm in this function altogether. Instead, it should only ever\n    +-\t * be set via our repository setup procedures. But that requires more\n    +-\t * work.\n    +-\t */\n    +-\tif (repo == the_repository)\n    +-\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n      }\n      \n      static void expand_base_dir(char **out, const char *in,\n-- \n2.45.0\n\n"},{"id":"494223","messageId":"5ee372c2d8f98b8cd8c0b51bda15ad5449339776.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 01/13] path: harden validation of HEAD with non-standard hashes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:52:47Z","receivedAt":"2024-05-07T04:52:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `validate_headref()` function takes a path to a supposed \"HEAD\" file\nand checks whether its format is something that we understand. It is\nused as part of our repository discovery to check whether a specific\ndirectory is a Git directory or not.\n\nPart of the validation is a check for a detached HEAD that contains a\nplain object ID. To do this validation we use `get_oid_hex()`, which\nrelies on `the_hash_algo`. At this point in time the hash algo cannot\nyet be initialized though because we didn't yet read the Git config.\nConsequently, it will always be the SHA1 hash algorithm.\n\nIn practice this works alright because `get_oid_hex()` only ends up\nchecking whether the prefix of the buffer is a valid object ID. And\nbecause SHA1 is shorter than SHA256, the function will successfully\nparse SHA256 object IDs, as well.\n\nIt is somewhat fragile though and not really the intent to only check\nfor SHA1. With this in mind, harden the code to use `get_oid_hex_any()`\nto check whether the \"HEAD\" file parses as any known hash.\n\nOne might be hard pressed to tighten the check even further and fully\nvalidate the file contents, not only the prefix. In practice though that\nwouldn't make a lot of sense as it could be that the repository uses a\nhash function that produces longer hashes than SHA256, but which the\ncurrent version of Git doesn't understand yet. We'd still want to detect\nthe repository as proper Git repository in that case, and we will fail\neventually with a proper error message that the hash isn't understood\nwhen trying to set up the repository format.\n\nIt follows that we could just leave the current code intact, as in\npractice the code change doesn't have any user visible impact. But it\nalso prepares us for `the_hash_algo` being unset when there is no\nrepository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n path.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/path.c b/path.c\nindex 67229edb9c..cc02165530 100644\n--- a/path.c\n+++ b/path.c\n@@ -692,9 +692,9 @@ int validate_headref(const char *path)\n \n \t/*\n \t * Is this a detached HEAD?\n \t */\n-\tif (!get_oid_hex(buffer, &oid))\n+\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n \t\treturn 0;\n \n \treturn -1;\n }\n-- \n2.45.0\n\n"},{"id":"494224","messageId":"ece0ab94a80a8f0d0541185ddfe584b43a468cb1.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 02/13] path: move `validate_headref()` to its only user","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:52:51Z","receivedAt":"2024-05-07T04:52:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While `validate_headref()` is only called from `is_git_directory()` in\n\"setup.c\", it is currently implemented in \"path.c\". Move it over such\nthat it becomes clear that it is only really used during setup in order\nto discover repositories.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n path.c  | 53 -----------------------------------------------------\n path.h  |  1 -\n setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 53 insertions(+), 54 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex cc02165530..bd6e25245d 100644\n--- a/path.c\n+++ b/path.c\n@@ -4,9 +4,8 @@\n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n-#include \"hex.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"dir.h\"\n@@ -646,60 +645,8 @@ void strbuf_git_common_path(struct strbuf *sb,\n \tdo_git_common_path(repo, sb, fmt, args);\n \tva_end(args);\n }\n \n-int validate_headref(const char *path)\n-{\n-\tstruct stat st;\n-\tchar buffer[256];\n-\tconst char *refname;\n-\tstruct object_id oid;\n-\tint fd;\n-\tssize_t len;\n-\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\n-\t/* Make sure it is a \"refs/..\" symlink */\n-\tif (S_ISLNK(st.st_mode)) {\n-\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n-\t\t\treturn 0;\n-\t\treturn -1;\n-\t}\n-\n-\t/*\n-\t * Anything else, just open it and try to see if it is a symbolic ref.\n-\t */\n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn -1;\n-\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n-\tclose(fd);\n-\n-\tif (len < 0)\n-\t\treturn -1;\n-\tbuffer[len] = '\\0';\n-\n-\t/*\n-\t * Is it a symbolic ref?\n-\t */\n-\tif (skip_prefix(buffer, \"ref:\", &refname)) {\n-\t\twhile (isspace(*refname))\n-\t\t\trefname++;\n-\t\tif (starts_with(refname, \"refs/\"))\n-\t\t\treturn 0;\n-\t}\n-\n-\t/*\n-\t * Is this a detached HEAD?\n-\t */\n-\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n-\t\treturn 0;\n-\n-\treturn -1;\n-}\n-\n static struct passwd *getpw_str(const char *username, size_t len)\n {\n \tstruct passwd *pw;\n \tchar *username_z = xmemdupz(username, len);\ndiff --git a/path.h b/path.h\nindex ea96487b29..c3bc8617bd 100644\n--- a/path.h\n+++ b/path.h\n@@ -172,9 +172,8 @@ const char *git_path_merge_head(struct repository *r);\n const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \n int ends_with_path_components(const char *path, const char *components);\n-int validate_headref(const char *ref);\n \n int calc_shared_perm(int mode);\n int adjust_shared_perm(const char *path);\n \ndiff --git a/setup.c b/setup.c\nindex f4b32f76e3..7c996659bd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -3,8 +3,9 @@\n #include \"copy.h\"\n #include \"environment.h\"\n #include \"exec-cmd.h\"\n #include \"gettext.h\"\n+#include \"hex.h\"\n #include \"object-name.h\"\n #include \"refs.h\"\n #include \"repository.h\"\n #include \"config.h\"\n@@ -340,8 +341,60 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n \tstrbuf_release(&path);\n \treturn ret;\n }\n \n+static int validate_headref(const char *path)\n+{\n+\tstruct stat st;\n+\tchar buffer[256];\n+\tconst char *refname;\n+\tstruct object_id oid;\n+\tint fd;\n+\tssize_t len;\n+\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\n+\t/* Make sure it is a \"refs/..\" symlink */\n+\tif (S_ISLNK(st.st_mode)) {\n+\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n+\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n+\t\t\treturn 0;\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Anything else, just open it and try to see if it is a symbolic ref.\n+\t */\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn -1;\n+\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n+\tclose(fd);\n+\n+\tif (len < 0)\n+\t\treturn -1;\n+\tbuffer[len] = '\\0';\n+\n+\t/*\n+\t * Is it a symbolic ref?\n+\t */\n+\tif (skip_prefix(buffer, \"ref:\", &refname)) {\n+\t\twhile (isspace(*refname))\n+\t\t\trefname++;\n+\t\tif (starts_with(refname, \"refs/\"))\n+\t\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * Is this a detached HEAD?\n+\t */\n+\tif (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)\n+\t\treturn 0;\n+\n+\treturn -1;\n+}\n+\n /*\n  * Test if it looks like we're at a git directory.\n  * We want to see:\n  *\n-- \n2.45.0\n\n"},{"id":"494225","messageId":"1a0859eaf118090b4d172792e63ee9a6ba752b71.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 03/13] parse-options-cb: only abbreviate hashes when hash algo is known","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:52:56Z","receivedAt":"2024-05-07T04:53:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPT__ABBREV()` option can be used to add an option that abbreviates\nobject IDs. When given a length longer than `the_hash_algo->hexsz`, then\nit will instead set the length to that maximum length.\n\nIt may not always be guaranteed that we have `the_hash_algo` initialized\nproperly as the hash algorithm can only be set up after we have set up\n`the_repository`. In that case, the hash would always be truncated to\nthe hex length of SHA1, which may not be what the user desires.\n\nIn practice it's not a problem as all commands that use `OPT__ABBREV()`\nalso have `RUN_SETUP` set and thus cannot work without a repository.\nConsequently, both `the_repository` and `the_hash_algo` would be\nproperly set up.\n\nRegardless of that, harden the code to not truncate the length when we\ndidn't set up a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n parse-options-cb.c       |  3 ++-\n t/t0040-parse-options.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex bdc7fae497..d99d688d3c 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -6,8 +6,9 @@\n #include \"date.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"setup.h\"\n #include \"string-list.h\"\n #include \"strvec.h\"\n #include \"oid-array.h\"\n \n@@ -28,9 +29,9 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \t\t\treturn error(_(\"option `%s' expects a numerical value\"),\n \t\t\t\t     opt->long_name);\n \t\tif (v && v < MINIMUM_ABBREV)\n \t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (v > the_hash_algo->hexsz)\n+\t\telse if (startup_info->have_repository && v > the_hash_algo->hexsz)\n \t\t\tv = the_hash_algo->hexsz;\n \t}\n \t*(int *)(opt->value) = v;\n \treturn 0;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 8bb2a8b453..45a773642f 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -175,8 +175,25 @@ test_expect_success 'long options' '\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n+test_expect_success 'abbreviate to something longer than SHA1 length' '\n+\tcat >expect <<-EOF &&\n+\tboolean: 0\n+\tinteger: 0\n+\tmagnitude: 0\n+\ttimestamp: 0\n+\tstring: (not set)\n+\tabbrev: 100\n+\tverbose: -1\n+\tquiet: 0\n+\tdry run: no\n+\tfile: (not set)\n+\tEOF\n+\ttest-tool parse-options --abbrev=100 >output &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'missing required value' '\n \tcat >expect <<-\\EOF &&\n \terror: switch `s'\\'' requires a value\n \tEOF\n-- \n2.45.0\n\n"},{"id":"494226","messageId":"1204a342161235c38273981874add59031cb554e.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 04/13] attr: don't recompute default attribute source","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:01Z","receivedAt":"2024-05-07T04:53:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `default_attr_source()` function lazily computes the attr source\nsupposedly once, only. This is done via a static variable `attr_source`\nthat contains the resolved object ID of the attr source's tree. If the\nvariable is the null object ID then we try to look up the attr source,\notherwise we skip over it.\n\nThis approach is flawed though: the variable will never be set to\nanything else but the null object ID in case there is no attr source.\nConsequently, we re-compute the information on every call. And in the\nworst case, when we silently ignore bad trees, this will cause us to try\nand look up the treeish every single time.\n\nImprove this by introducing a separate variable `has_attr_source` to\ntrack whether we already computed the attr source and, if so, whether we\nhave an attr source or not.\n\nThis also allows us to convert the `ignore_bad_attr_tree` to not be\nstatic anymore as the code will only be executed once anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 6af7151088..a5b717e4ce 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1205,41 +1205,48 @@ static void collect_some_attrs(struct index_state *istate,\n \tfill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n }\n \n static const char *default_attr_source_tree_object_name;\n-static int ignore_bad_attr_tree;\n \n void set_git_attr_source(const char *tree_object_name)\n {\n \tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n }\n \n-static void compute_default_attr_source(struct object_id *attr_source)\n+static int compute_default_attr_source(struct object_id *attr_source)\n {\n+\tint ignore_bad_attr_tree = 0;\n+\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n \tif (!default_attr_source_tree_object_name && git_attr_tree) {\n \t\tdefault_attr_source_tree_object_name = git_attr_tree;\n \t\tignore_bad_attr_tree = 1;\n \t}\n \n-\tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n-\t\treturn;\n+\tif (!default_attr_source_tree_object_name)\n+\t\treturn 0;\n \n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n-\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n-\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\t\t\t attr_source)) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n }\n \n static struct object_id *default_attr_source(void)\n {\n \tstatic struct object_id attr_source;\n+\tstatic int has_attr_source = -1;\n \n-\tif (is_null_oid(&attr_source))\n-\t\tcompute_default_attr_source(&attr_source);\n-\tif (is_null_oid(&attr_source))\n+\tif (has_attr_source < 0)\n+\t\thas_attr_source = compute_default_attr_source(&attr_source);\n+\tif (!has_attr_source)\n \t\treturn NULL;\n \treturn &attr_source;\n }\n \n-- \n2.45.0\n\n"},{"id":"494227","messageId":"f098ce9690f03bd42411b47c4cea23ecc8d96dee.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 05/13] attr: fix BUG() when parsing attrs outside of repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:05Z","receivedAt":"2024-05-07T04:53:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"If either the `--attr-source` option or the `GIT_ATTR_SOURCE` envvar are\nset, then `compute_default_attr_source()` will try to look up the value\nas a treeish. It is possible to hit that function while outside of a Git\nrepository though, for example when using `git grep --no-index`. In that\ncase, Git will hit a bug because we try to look up the main ref store\noutside of a repository.\n\nHandle the case gracefully and detect when we try to look up an attr\nsource without a repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n attr.c                |  6 ++++++\n t/t0003-attributes.sh | 15 +++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/attr.c b/attr.c\nindex a5b717e4ce..c89ab3478e 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1226,8 +1226,14 @@ static int compute_default_attr_source(struct object_id *attr_source)\n \n \tif (!default_attr_source_tree_object_name)\n \t\treturn 0;\n \n+\tif (!startup_info->have_repository) {\n+\t\tif (!ignore_bad_attr_tree)\n+\t\t\tdie(_(\"cannot use --attr-source or GIT_ATTR_SOURCE without repo\"));\n+\t\treturn 0;\n+\t}\n+\n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\n \t\t\t\t attr_source)) {\n \t\tif (!ignore_bad_attr_tree)\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex d755cc3c29..72fadca1e8 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -433,8 +433,23 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre\n \t\ttest_cmp expect actual\n \t)\n '\n \n+test_expect_success 'diff without repository with attr source' '\n+\tmkdir -p \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t(\n+\t\tcd \"$TRASH_DIRECTORY/outside/nongit\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\ttouch file &&\n+\t\tcat >expect <<-EOF &&\n+\t\tfatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo\n+\t\tEOF\n+\t\ttest_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n \t\tattr_check_source foo/bar/f f tag-1 &&\n-- \n2.45.0\n\n"},{"id":"494228","messageId":"e8876052ada5845e9a5ea4e910ff9da2c0837c07.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 06/13] remote-curl: fix parsing of detached SHA256 heads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:10Z","receivedAt":"2024-05-07T04:53:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The dumb HTTP transport tries to read the remote HEAD reference by\ndownloading the \"HEAD\" file and then parsing it via `http_fetch_ref()`.\nThis function will either parse the file as an object ID in case it is\nexactly `the_hash_algo->hexsz` long, or otherwise it will check whether\nthe reference starts with \"ref :\" and parse it as a symbolic ref.\n\nThis is broken when parsing detached HEADs of a remote SHA256 repository\nbecause we never update `the_hash_algo` to the discovered remote object\nhash. Consequently, `the_hash_algo` will always be the fallback SHA1\nhash algorithm, which will cause us to fail parsing HEAD altogteher when\nit contains a SHA256 object ID.\n\nFix this issue by setting up `the_hash_algo` via `repo_set_hash_algo()`.\nWhile at it, let's make the expected SHA1 fallback explicit in our code,\nwhich also addresses an upcoming issue where we are going to remove the\nSHA1 fallback for `the_hash_algo`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n remote-curl.c              | 19 ++++++++++++++++++-\n t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0b6d7815fd..004b707fdf 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -265,14 +265,25 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \n \treturn list;\n }\n \n+/*\n+ * Try to detect the hash algorithm used by the remote repository when using\n+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash\n+ * directly have to derive it from the advertised ref lengths.\n+ */\n static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)\n {\n \tconst char *p = memchr(heads->buf, '\\t', heads->len);\n \tint algo;\n+\n+\t/*\n+\t * In case the remote has no refs we have no way to reliably determine\n+\t * the object hash used by that repository. In that case we simply fall\n+\t * back to SHA1, which may or may not be correct.\n+\t */\n \tif (!p)\n-\t\treturn the_hash_algo;\n+\t\treturn &hash_algos[GIT_HASH_SHA1];\n \n \talgo = hash_algo_by_length((p - heads->buf) / 2);\n \tif (algo == GIT_HASH_UNKNOWN)\n \t\treturn NULL;\n@@ -294,8 +305,14 @@ static struct ref *parse_info_refs(struct discovery *heads)\n \t\tdie(\"%sinfo/refs not valid: could not determine hash algorithm; \"\n \t\t    \"is this a git repository?\",\n \t\t    transport_anonymize_url(url.buf));\n \n+\t/*\n+\t * Set the repository's hash algo to whatever we have just detected.\n+\t * This ensures that we can correctly parse the remote references.\n+\t */\n+\trepo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));\n+\n \tdata = heads->buf;\n \tstart = NULL;\n \tmid = data;\n \twhile (i < heads->len) {\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 4c3b32785d..5f16cbc58d 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -54,8 +54,23 @@ test_expect_success 'list refs from outside any repository' '\n \tnongit git ls-remote \"$HTTPD_URL/dumb/repo.git\" >actual &&\n \ttest_cmp expect actual\n '\n \n+\n+test_expect_success 'list detached HEAD from outside any repository' '\n+\tgit clone --mirror \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" \\\n+\t\tupdate-ref --no-deref HEAD refs/heads/main &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git\" update-server-info &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\tHEAD\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tnongit git ls-remote \"$HTTPD_URL/dumb/repo-detached.git\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create password-protected repository' '\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\n \tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n \t       \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git\"\n-- \n2.45.0\n\n"},{"id":"494229","messageId":"61160303108133f0225aae0f0776fa15c7ee07ec.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 07/13] builtin/rev-parse: allow shortening to more than 40 hex characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:15Z","receivedAt":"2024-05-07T04:53:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--short=` option for git-rev-parse(1) allows the user to specify\nto how many characters object IDs should be shortened to. The option is\nbroken though for SHA256 repositories because we set the maximum allowed\nhash size to `the_hash_algo->hexsz` before we have even set up the repo.\nConsequently, `the_hash_algo` will always be SHA1 and thus we truncate\nevery hash after at most 40 characters.\n\nFix this by accessing `the_hash_algo` only after we have set up the\nrepo.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/rev-parse.c  | 5 ++---\n t/t1500-rev-parse.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex af79538632..7d10ee33c4 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -686,9 +686,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tunsigned int flags = 0;\n \tconst char *name = NULL;\n \tstruct object_context unused;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tint seen_end_of_options = 0;\n \tenum format_type format = FORMAT_DEFAULT;\n \n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n@@ -862,10 +861,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tcontinue;\n \t\t\t\tabbrev = strtoul(arg, NULL, 10);\n \t\t\t\tif (abbrev < MINIMUM_ABBREV)\n \t\t\t\t\tabbrev = MINIMUM_ABBREV;\n-\t\t\t\telse if (hexsz <= abbrev)\n-\t\t\t\t\tabbrev = hexsz;\n+\t\t\t\telse if ((int)the_hash_algo->hexsz <= abbrev)\n+\t\t\t\t\tabbrev = the_hash_algo->hexsz;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--sq\")) {\n \t\t\t\toutput_sq = 1;\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex a669e592f1..30c31918fd 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -303,5 +303,11 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '\n \tgit rev-parse --symbolic-full-name --bisect >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--short= truncates to the actual hash length' '\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse --short=100 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.45.0\n\n"},{"id":"494230","messageId":"872ded113ef3cc97cfdaa7f199ffe6aca45c6067.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 08/13] builtin/blame: don't access potentially unitialized `the_hash_algo`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:20Z","receivedAt":"2024-05-07T04:53:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We access `the_hash_algo` in git-blame(1) before we have executed\n`parse_options_start()`, which may not be properly set up in case we\nhave no repository. This is fine for most of the part because all the\ncall paths that lead to it (git-blame(1), git-annotate(1) as well as\ngit-pick-axe(1)) specify `RUN_SETUP` and thus require a repository.\n\nThere is one exception though, namely when passing `-h` to print the\nhelp. Here we will access `the_hash_algo` even if there is no repo.\nThis works fine right now because `the_hash_algo` gets sets up to point\nto the SHA1 algorithm via `initialize_repository()`. But we're about to\nstop doing this, and thus the code would lead to a `NULL` pointer\nexception.\n\nPrepare the code for this and only access `the_hash_algo` after we are\nsure that there is a proper repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/blame.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 9aa74680a3..e325825936 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -914,9 +914,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tint cmd_is_annotate = !strcmp(argv[0], \"annotate\");\n \tstruct range_set ranges;\n \tunsigned int range_i;\n \tlong anchor;\n-\tconst int hexsz = the_hash_algo->hexsz;\n \tlong num_lines = 0;\n \tconst char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;\n \tconst char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;\n \n@@ -972,13 +971,13 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tshow_progress = 0;\n \t} else if (show_progress < 0)\n \t\tshow_progress = isatty(2);\n \n-\tif (0 < abbrev && abbrev < hexsz)\n+\tif (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\n \telse if (!abbrev)\n-\t\tabbrev = hexsz;\n+\t\tabbrev = the_hash_algo->hexsz;\n \n \tif (revs_file && read_ancestry(revs_file))\n \t\tdie_errno(\"reading graft file '%s' failed\", revs_file);\n \n-- \n2.45.0\n\n"},{"id":"494231","messageId":"5b4a21d2ce791a2ae45ce91ff3ae4f4a03efecb8.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 09/13] builtin/bundle: abort \"verify\" early when there is no repository","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:24Z","receivedAt":"2024-05-07T04:53:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Verifying a bundle requires us to have a repository. This is encoded in\n`verify_bundle()`, which will return an error if there is no repository.\nWe call `open_bundle()` before we call `verify_bundle()` though, which\nalready performs some verifications even though we may ultimately abort\ndue to a missing repository.\n\nThis is problematic because `open_bundle()` already reads the bundle\nheader and verifies that it contains a properly formatted hash. When\nthere is no repository we have no clue what hash function to expect\nthough, so we always end up assuming SHA1 here, which may or may not be\ncorrect. Furthermore, we are about to stop initializing `the_hash_algo`\nwhen there is no repository, which will lead to segfaults.\n\nCheck early on whether we have a repository to fix this issue.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/bundle.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 3ad11dc5d0..d5d41a8f67 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -139,8 +139,13 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n+\tif (!startup_info->have_repository) {\n+\t\tret = error(_(\"need a repository to verify a bundle\"));\n+\t\tgoto cleanup;\n+\t}\n+\n \tif ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n \t}\n-- \n2.45.0\n\n"},{"id":"494232","messageId":"e7254ae757b5522a78f4bc390c72af406ea08641.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 10/13] builtin/diff: explicitly set hash algo when there is no repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:29Z","receivedAt":"2024-05-07T04:53:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-diff(1) command can be used outside repositories to diff two\nfiles with each other. But even if there is no repository we will end up\nhashing the files that we are diffing so that we can print the \"index\"\nline:\n\n    ```\n    diff --git a/a b/b\n    index 7898192..6178079 100644\n    --- a/a\n    +++ b/b\n    @@ -1 +1 @@\n    -a\n    +b\n    ```\n\nWe implicitly use SHA1 to calculate the hash here, which is because\n`the_repository` gets initialized with SHA1 during the startup routine.\nWe are about to stop doing this though such that `the_repository` only\never has a hash function when it was properly initialized via a repo's\nconfiguration.\n\nTo give full control to our users, we would ideally add a new switch to\ngit-diff(1) that allows them to specify the hash function when executed\noutside of a repository. But for now, we only convert the code to make\nthis explicit such that we can stop setting the default hash algorithm\nfor `the_repository`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/diff.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex efc37483b3..9b6cdabe15 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -464,8 +464,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\t!path_inside_repo(prefix, argv[i + 1]))))\n \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \t}\n \n+\t/*\n+\t * When operating outside of a Git repository we need to have a hash\n+\t * algorithm at hand so that we can generate the blob hashes. We\n+\t * default to SHA1 here, but may eventually want to change this to be\n+\t * configurable via a command line option.\n+\t */\n+\tif (nongit)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tinit_diff_ui_defaults();\n \tgit_config(git_diff_ui_config, NULL);\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n \n-- \n2.45.0\n\n"},{"id":"494233","messageId":"c21b15ba1759a9dde7ba1d50817bcb44fe1c66da.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 11/13] builtin/shortlog: don't set up revisions without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:35Z","receivedAt":"2024-05-07T04:53:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is possible to run git-shortlog(1) outside of a repository by passing\nit output from git-log(1) via standard input. Obviously, as there is no\nrepository in that context, it is thus unsupported to pass any revisions\nas arguments.\n\nRegardless of that we still end up calling `setup_revisions()`. While\nthat works alright, it is somewhat strange. Furthermore, this is about\nto cause problems when we unset the default object hash.\n\nRefactor the code to only call `setup_revisions()` when we have a\nrepository. This is safe to do as we already verify that there are no\narguments when running outside of a repository anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3c7cd2d6ef..d4daf31e22 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -434,9 +434,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\terror(_(\"too many arguments given outside repository\"));\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n \n-\tif (setup_revisions(argc, argv, &rev, NULL) != 1) {\n+\tif (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {\n \t\terror(_(\"unrecognized argument: %s\"), argv[1]);\n \t\tusage_with_options(shortlog_usage, options);\n \t}\n \n-- \n2.45.0\n\n"},{"id":"494234","messageId":"f37beb0e8920ca2f57e74e8934596fb65ae935ac.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 12/13] oss-fuzz/commit-graph: set up hash algorithm","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:40Z","receivedAt":"2024-05-07T04:53:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Our fuzzing setups don't work in a proper repository, but only use the\nin-memory configured `the_repository`. Consequently, we never go through\nthe full repository setup procedures and thus do not set up the hash\nalgo used by the repository.\n\nThe commit-graph fuzzer does rely on a properly initialized hash algo\nthough. Initialize it explicitly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n oss-fuzz/fuzz-commit-graph.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/oss-fuzz/fuzz-commit-graph.c b/oss-fuzz/fuzz-commit-graph.c\nindex fe15e2c225..75e668a057 100644\n--- a/oss-fuzz/fuzz-commit-graph.c\n+++ b/oss-fuzz/fuzz-commit-graph.c\n@@ -18,8 +18,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \t * normally be read from the repository's gitdir. We want to avoid\n \t * touching the disk to keep the individual fuzz-test cases as fast as\n \t * possible.\n \t */\n+\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n \tthe_repository->settings.commit_graph_generation_version = 2;\n \tthe_repository->settings.commit_graph_read_changed_paths = 1;\n \tg = parse_commit_graph(&the_repository->settings, (void *)data, size);\n \trepo_clear(the_repository);\n-- \n2.45.0\n\n"},{"id":"494235","messageId":"950b08bc78a76a6f627698372306e2eccc0f3c39.1715057362.git.ps@pks.im","threadId":"61337","inReplyTo":"cover.1715057362.git.ps@pks.im","subject":"[PATCH v4 13/13] repository: stop setting SHA1 as the default object hash","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:53:44Z","receivedAt":"2024-05-07T04:53:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During the startup of Git, we call `initialize_the_repository()` to set\nup `the_repository` as well as `the_index`. Part of this setup is also\nto set the default object hash of the repository to SHA1. This has the\neffect that `the_hash_algo` is getting initialized to SHA1, as well.\nThis default hash algorithm eventually gets overridden by most Git\ncommands via `setup_git_directory()`, which also detects the actual hash\nalgorithm used by the repository.\n\nThere are some commands though that don't access a repository at all, or\nat a later point only, and thus retain the default hash function for\nsome amount of time. As some of the the preceding commits demonstrate,\nthis can lead to subtle issues when we access `the_hash_algo` when no\nrepository has been set up.\n\nAddress this issue by dropping the set up of the default hash algorithm\ncompletely. The effect of this is that `the_hash_algo` will map to a\n`NULL` pointer and thus cause Git to crash when something tries to\naccess the hash algorithm without it being properly initialized. It thus\nforces all Git commands to explicitly set up the hash algorithm in case\nthere is no repository.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repository.c | 20 --------------------\n 1 file changed, 20 deletions(-)\n\ndiff --git a/repository.c b/repository.c\nindex 2118f563e3..15c10015b0 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -25,28 +25,8 @@ void initialize_repository(struct repository *repo)\n \trepo->remote_state = remote_state_new();\n \trepo->parsed_objects = parsed_object_pool_new();\n \tALLOC_ARRAY(repo->index, 1);\n \tindex_state_init(repo->index, repo);\n-\n-\t/*\n-\t * Unfortunately, we need to keep this hack around for the time being:\n-\t *\n-\t *   - Not setting up the hash algorithm for `the_repository` leads to\n-\t *     crashes because `the_hash_algo` is a macro that expands to\n-\t *     `the_repository->hash_algo`. So if Git commands try to access\n-\t *     `the_hash_algo` without a Git directory we crash.\n-\t *\n-\t *   - Setting up the hash algorithm to be SHA1 by default breaks other\n-\t *     commands when running with SHA256.\n-\t *\n-\t * This is another point in case why having global state is a bad idea.\n-\t * Eventually, we should remove this hack and stop setting the hash\n-\t * algorithm in this function altogether. Instead, it should only ever\n-\t * be set via our repository setup procedures. But that requires more\n-\t * work.\n-\t */\n-\tif (repo == the_repository)\n-\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n }\n \n static void expand_base_dir(char **out, const char *in,\n \t\t\t    const char *base_dir, const char *def_in)\n-- \n2.45.0\n\n"}]}