{"thread":{"id":"60534","subject":"[PATCH] setup: recognize bare repositories with packed-refs","startedAt":"2023-11-17T20:33:11Z","lastAt":"2023-12-12T01:22:21Z","messageCount":23,"participants":["Adam Majer","Junio C Hamano","Glen Choo","Josh Steadmon","Jeff King","Patrick Steinhardt","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484995","messageId":"20231117203253.21143-1-adamm@zombino.com","threadId":"60534","inReplyTo":"20231117202513.20604-1-adamm@zombino.com","subject":"[PATCH] setup: recognize bare repositories with packed-refs","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-11-17T20:32:53Z","receivedAt":"2023-11-17T20:33:11Z","isPatch":true,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"In a garbage collected bare git repository, the refs/ subdirectory is\nempty. In use-cases when such a repository is directly added into\nanother repository, it no longer is detected as valid. Git doesn't\npreserve empty paths so refs/ subdirectory is not present. Simply\ncreating an empty refs/ subdirectory fixes this problem.\n\nLooking more carefully, there are two backends to handle various refs in\ngit -- the files backend that uses refs/ subdirectory and the\npacked-refs backend that uses packed-refs file. If references are not\nfound in refs/ subdirectory (or directory doesn't exist), the\npacked-refs directory will be consulted. Garbage collected repository\nwill have all its references in packed-refs file.\n\nTo allow the use-case when packed-refs is the only source of refs and\nrefs/ subdirectory is simply not present, augment 'is_git_directory()'\nsetup function to look for packed-refs file as an alternative to refs/\nsubdirectory.\n\nSigned-off-by: Adam Majer \n---\n setup.c | 10 +++++++---\n t/t6500-gc.sh | 8 ++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex fc592dc6dd..2a6dda6ae9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -348,7 +348,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n *\n * - either an objects/ directory _or_ the proper\n * GIT_OBJECT_DIRECTORY environment variable\n- * - a refs/ directory\n+ * - a refs/ directory or packed-refs file\n * - either a HEAD symlink or a HEAD file that is formatted as\n * a proper \"ref:\", or a regular file HEAD that has a properly\n * formatted sha1 object name.\n@@ -384,8 +384,12 @@ int is_git_directory(const char *suspect)\n \n \tstrbuf_setlen(&path, len);\n \tstrbuf_addstr(&path, \"/refs\");\n-\tif (access(path.buf, X_OK))\n-\t\tgoto done;\n+\tif (access(path.buf, X_OK)) {\n+\t\tstrbuf_setlen(&path, len);\n+\t\tstrbuf_addstr(&path, \"/packed-refs\");\n+\t\tif (access(path.buf, R_OK))\n+\t\t\tgoto done;\n+\t}\n \n \tret = 1;\n done:\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 18fe1c25e6..e81eb7d2ab 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -214,6 +214,14 @@ test_expect_success 'gc.repackFilter launches repack with a filter' '\n \tgrep -E \"^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*\" trace.out\n '\n \n+test_expect_success 'remove empty refs/ subdirectory' '\n+\tgit -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 &&\n+\ttest_dir_is_empty bare.git/refs/heads &&\n+\ttest_dir_is_empty bare.git/refs/tags &&\n+\trm -r bare.git/refs &&\n+\tGIT_DIR=\"$PWD\"/bare.git git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814\n+'\n+\n test_expect_success 'gc.repackFilterTo store filtered out objects' '\n \ttest_when_finished \"rm -rf bare.git filtered.git\" &&\n \n-- \n2.42.0\n\n"},{"id":"484996","messageId":"20231117202513.20604-1-adamm@zombino.com","threadId":"60534","inReplyTo":null,"subject":"[PATCH] setup: recognize bare repositories with packed-refs","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-11-17T20:25:13Z","receivedAt":"2023-11-17T20:33:34Z","isPatch":true,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"When a garbage collected bare git repository is added to another git\nrepository, the refs/ subdirectory is empty. In case-cases when such a\nrepository is added into another repository directly, it no longer is\ndetected as a valid. Git doesn't preserve empty paths so refs/\nsubdirectory is not present in parent git. Simply creating an empty\nrefs/ subdirectory fixes this problem.\n\nLooking more carefully, there are two backends to handle various refs in\ngit -- the files backend that uses refs/ subdirectory and the\npacked-refs backend that uses packed-refs file. If references are not\nfound in refs/ subdirectory (or directory doesn't exist), the\npacked-refs directory will be consulted. Garbage collected repository\nwill have all its references in packed-refs file.\n\nTo allow the use-case when packed-refs is the only source of refs and\nrefs/ subdirectory is simply not present, augment 'is_git_directory()'\nsetup function to look for packed-refs file as an alternative to refs/\nsubdirectory.\n\nSigned-off-by: Adam Majer \n---\n setup.c | 10 +++++++---\n t/t6500-gc.sh | 8 ++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex fc592dc6dd..2a6dda6ae9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -348,7 +348,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n *\n * - either an objects/ directory _or_ the proper\n * GIT_OBJECT_DIRECTORY environment variable\n- * - a refs/ directory\n+ * - a refs/ directory or packed-refs file\n * - either a HEAD symlink or a HEAD file that is formatted as\n * a proper \"ref:\", or a regular file HEAD that has a properly\n * formatted sha1 object name.\n@@ -384,8 +384,12 @@ int is_git_directory(const char *suspect)\n \n \tstrbuf_setlen(&path, len);\n \tstrbuf_addstr(&path, \"/refs\");\n-\tif (access(path.buf, X_OK))\n-\t\tgoto done;\n+\tif (access(path.buf, X_OK)) {\n+\t\tstrbuf_setlen(&path, len);\n+\t\tstrbuf_addstr(&path, \"/packed-refs\");\n+\t\tif (access(path.buf, R_OK))\n+\t\t\tgoto done;\n+\t}\n \n \tret = 1;\n done:\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 18fe1c25e6..e81eb7d2ab 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -214,6 +214,14 @@ test_expect_success 'gc.repackFilter launches repack with a filter' '\n \tgrep -E \"^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*\" trace.out\n '\n \n+test_expect_success 'remove empty refs/ subdirectory' '\n+\tgit -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 &&\n+\ttest_dir_is_empty bare.git/refs/heads &&\n+\ttest_dir_is_empty bare.git/refs/tags &&\n+\trm -r bare.git/refs &&\n+\tGIT_DIR=\"$PWD\"/bare.git git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814\n+'\n+\n test_expect_success 'gc.repackFilterTo store filtered out objects' '\n \ttest_when_finished \"rm -rf bare.git filtered.git\" &&\n \n-- \n2.42.0\n\n"},{"id":"484997","messageId":"830936c7-bcbb-480a-a5b1-7029a626d58b@zombino.com","threadId":"60534","inReplyTo":"20231117203253.21143-1-adamm@zombino.com","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-11-17T20:44:29Z","receivedAt":"2023-11-17T20:44:32Z","isPatch":true,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"On 2023-11-17 21:32, Adam Majer wrote:\n> +test_expect_success 'remove empty refs/ subdirectory' '\n> +\tgit -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 &&\n> +\ttest_dir_is_empty bare.git/refs/heads &&\n> +\ttest_dir_is_empty bare.git/refs/tags &&\n> +\trm -r bare.git/refs &&\n> +\tGIT_DIR=\"$PWD\"/bare.git git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814\n\nIn the test, I've first tried to use GIT_CEILING_DIRECTORIES=\"$PWD\" here \ninstead, but git went up into the parent directory anyway. I'm not sure \nif this is a bug, or feature, or my misunderstanding.\n\n- Adam\n\n"},{"id":"485008","messageId":"xmqqbkbppbrd.fsf@gitster.g","threadId":"60534","inReplyTo":"20231117203253.21143-1-adamm@zombino.com","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-19T23:24:38Z","receivedAt":"2023-11-19T23:24:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Majer writes:\n\n> In a garbage collected bare git repository, the refs/ subdirectory is\n> empty. In use-cases when such a repository is directly added into\n> another repository, it no longer is detected as valid.\n\nJosh & Glen [*], isn't this a layout that we explicitly discourage and\neventually plan to forbid anyway?\n\n*1* who worked on e35f202b (setup: trace bare repository setups, 2023-05-01)\n"},{"id":"485013","messageId":"CA+we5Gqp2nhoXyqJKJiXQMDx-ZXcnNGHu3wnwERWREng2XHHuQ@mail.gmail.com","threadId":"60534","inReplyTo":"xmqqbkbppbrd.fsf@gitster.g","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Glen Choo","fromEmail":"glencbz@gmail.com","sentAt":"2023-11-20T09:31:29Z","receivedAt":"2023-11-20T09:31:41Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"On Mon, Nov 20, 2023 at 7:24 AM Junio C Hamano wrote:\n>\n> Adam Majer writes:\n>\n> > In a garbage collected bare git repository, the refs/ subdirectory is\n> > empty. In use-cases when such a repository is directly added into\n> > another repository, it no longer is detected as valid.\n>\n> Josh & Glen [*], isn't this a layout that we explicitly discourage and\n> eventually plan to forbid anyway?\n\nIf my recollection of [1] serves me correctly, we didn't come to a\nstrong conclusion on whether or not to forbid bare repositories in the\nworking tree, particularly because it would leave existing repos (like\nGit LFS) high and dry. Though personally, I'd be happy to see a\nversion of Git that forbade bare repositories in the working tree.\n\nI don't really recall the bare repo tracing bits, so I'll leave that to Josh.\n\n[1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n"},{"id":"485014","messageId":"ab332078-dd49-41cf-a3d1-6a67a507b122@zombino.com","threadId":"60534","inReplyTo":"xmqqbkbppbrd.fsf@gitster.g","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-11-20T09:43:28Z","receivedAt":"2023-11-20T09:43:31Z","isPatch":true,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"On 11/20/23 00:24, Junio C Hamano wrote:\n> Adam Majer writes:\n> \n>> In a garbage collected bare git repository, the refs/ subdirectory is\n>> empty. In use-cases when such a repository is directly added into\n>> another repository, it no longer is detected as valid.\n> \n> Josh & Glen [*], isn't this a layout that we explicitly discourage and\n> eventually plan to forbid anyway?\n> \n> *1* who worked on e35f202b (setup: trace bare repository setups, 2023-05-01)\n\nThis is fair enough. Completely removing embedded git repos would cause \nsome pain in test suites as it was discussed in the thread for that \ncommit[1]. Gitea (for example) has a few dozen embedded bare \nrepositories for tests.\n\nIn either case, running `git gc` on a bare repository makes it no longer \ndetectable as a git repository after checkout, GIT_DIR or not. This \nseems to be unintentional and not linked to the other discussion.\n\n- Adam\n"},{"id":"485177","messageId":"ZWTxNBcF1wLufJPj@google.com","threadId":"60534","inReplyTo":"CA+we5Gqp2nhoXyqJKJiXQMDx-ZXcnNGHu3wnwERWREng2XHHuQ@mail.gmail.com","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-11-27T19:42:44Z","receivedAt":"2023-11-27T19:42:50Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.11.20 17:31, Glen Choo wrote:\n> On Mon, Nov 20, 2023 at 7:24 AM Junio C Hamano wrote:\n> >\n> > Adam Majer writes:\n> >\n> > > In a garbage collected bare git repository, the refs/ subdirectory is\n> > > empty. In use-cases when such a repository is directly added into\n> > > another repository, it no longer is detected as valid.\n> >\n> > Josh & Glen [*], isn't this a layout that we explicitly discourage and\n> > eventually plan to forbid anyway?\n> \n> If my recollection of [1] serves me correctly, we didn't come to a\n> strong conclusion on whether or not to forbid bare repositories in the\n> working tree, particularly because it would leave existing repos (like\n> Git LFS) high and dry. Though personally, I'd be happy to see a\n> version of Git that forbade bare repositories in the working tree.\n> \n> I don't really recall the bare repo tracing bits, so I'll leave that to Josh.\n> \n> [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nYeah, my understanding was that we don't want to forbid bare\nrepositories outright, which is why we have the config option to let\nend-users choose what to do with them.\n"},{"id":"485178","messageId":"ZWTxuBV1DGieo6n2@google.com","threadId":"60534","inReplyTo":"20231117203253.21143-1-adamm@zombino.com","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-11-27T19:44:56Z","receivedAt":"2023-11-27T19:45:02Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.11.17 21:32, Adam Majer wrote:\n> In a garbage collected bare git repository, the refs/ subdirectory is\n> empty. In use-cases when such a repository is directly added into\n> another repository, it no longer is detected as valid. Git doesn't\n> preserve empty paths so refs/ subdirectory is not present. Simply\n> creating an empty refs/ subdirectory fixes this problem.\n> \n> Looking more carefully, there are two backends to handle various refs in\n> git -- the files backend that uses refs/ subdirectory and the\n> packed-refs backend that uses packed-refs file. If references are not\n> found in refs/ subdirectory (or directory doesn't exist), the\n> packed-refs directory will be consulted. Garbage collected repository\n> will have all its references in packed-refs file.\n> \n> To allow the use-case when packed-refs is the only source of refs and\n> refs/ subdirectory is simply not present, augment 'is_git_directory()'\n> setup function to look for packed-refs file as an alternative to refs/\n> subdirectory.\n> \n> Signed-off-by: Adam Majer \n> ---\n> setup.c | 10 +++++++---\n> t/t6500-gc.sh | 8 ++++++++\n> 2 files changed, 15 insertions(+), 3 deletions(-)\n> \n> diff --git a/setup.c b/setup.c\n> index fc592dc6dd..2a6dda6ae9 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -348,7 +348,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> *\n> * - either an objects/ directory _or_ the proper\n> * GIT_OBJECT_DIRECTORY environment variable\n> - * - a refs/ directory\n> + * - a refs/ directory or packed-refs file\n> * - either a HEAD symlink or a HEAD file that is formatted as\n> * a proper \"ref:\", or a regular file HEAD that has a properly\n> * formatted sha1 object name.\n> @@ -384,8 +384,12 @@ int is_git_directory(const char *suspect)\n> \n> \tstrbuf_setlen(&path, len);\n> \tstrbuf_addstr(&path, \"/refs\");\n> -\tif (access(path.buf, X_OK))\n> -\t\tgoto done;\n> +\tif (access(path.buf, X_OK)) {\n> +\t\tstrbuf_setlen(&path, len);\n> +\t\tstrbuf_addstr(&path, \"/packed-refs\");\n> +\t\tif (access(path.buf, R_OK))\n> +\t\t\tgoto done;\n> +\t}\n> \n> \tret = 1;\n> done:\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 18fe1c25e6..e81eb7d2ab 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -214,6 +214,14 @@ test_expect_success 'gc.repackFilter launches repack with a filter' '\n> \tgrep -E \"^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*\" trace.out\n> '\n> \n> +test_expect_success 'remove empty refs/ subdirectory' '\n> +\tgit -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 &&\n> +\ttest_dir_is_empty bare.git/refs/heads &&\n> +\ttest_dir_is_empty bare.git/refs/tags &&\n> +\trm -r bare.git/refs &&\n> +\tGIT_DIR=\"$PWD\"/bare.git git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814\n> +'\n> +\n\nTwo suggestions for the test here:\n1) Can you give the test a more descriptive name, such as \"GCed bare\nrepos still recognized\"?\n2) Can you add a check that bare.git/packed-refs exists?\n\nOther than that, looks good to me.\n\nReviewed-by: Josh Steadmon \n"},{"id":"485193","messageId":"2e5e3225-6e94-43ff-ab7b-18aed2048372@zombino.com","threadId":"60534","inReplyTo":"ZWTxuBV1DGieo6n2@google.com","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-11-28T14:14:58Z","receivedAt":"2023-11-28T14:15:01Z","isPatch":true,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"On 11/27/23 20:44, Josh Steadmon wrote:> Two suggestions for the test here:\n> 1) Can you give the test a more descriptive name, such as \"GCed bare \n> repos still recognized\"?\nThanks, adjusted. I've also added that empty refs/ directory is not there.\n\n> 2) Can you add a check that bare.git/packed-refs exists?\nDone.\n\nI've also removed the -C parameter since we actually need GIT_DIR= in\nall cases to prevent git from going up directory tree. -C is then\nsuperflous. In addition, I've changed the hardcoded object id to master\nbranch to make it less magical looking.\n\n- Adam\n"},{"id":"485194","messageId":"20231128142845.11523-1-adamm@zombino.com","threadId":"60534","inReplyTo":"20231117203253.21143-1-adamm@zombino.com","subject":"[PATCH] setup: recognize bare repositories with packed-refs","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-11-28T14:28:45Z","receivedAt":"2023-11-28T14:28:48Z","isPatch":true,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"In a garbage collected bare git repository, the refs/ subdirectory is\nempty. In use-cases when such a repository is directly added into\nanother repository, it no longer is detected as valid. Git doesn't\npreserve empty paths so refs/ subdirectory is not present. Simply\ncreating an empty refs/ subdirectory fixes this problem.\n\nLooking more carefully, there are two backends to handle various refs in\ngit -- the files backend that uses refs/ subdirectory and the\npacked-refs backend that uses packed-refs file. If references are not\nfound in refs/ subdirectory (or directory doesn't exist), the\npacked-refs directory will be consulted. Garbage collected repository\nwill have all its references in packed-refs file.\n\nTo allow the use-case when packed-refs is the only source of refs and\nrefs/ subdirectory is simply not present, augment 'is_git_directory()'\nsetup function to look for packed-refs file as an alternative to refs/\nsubdirectory.\n\nSigned-off-by: Adam Majer \n---\n setup.c | 10 +++++++---\n t/t6500-gc.sh | 9 +++++++++\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex fc592dc6dd..2a6dda6ae9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -348,7 +348,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n *\n * - either an objects/ directory _or_ the proper\n * GIT_OBJECT_DIRECTORY environment variable\n- * - a refs/ directory\n+ * - a refs/ directory or packed-refs file\n * - either a HEAD symlink or a HEAD file that is formatted as\n * a proper \"ref:\", or a regular file HEAD that has a properly\n * formatted sha1 object name.\n@@ -384,8 +384,12 @@ int is_git_directory(const char *suspect)\n \n \tstrbuf_setlen(&path, len);\n \tstrbuf_addstr(&path, \"/refs\");\n-\tif (access(path.buf, X_OK))\n-\t\tgoto done;\n+\tif (access(path.buf, X_OK)) {\n+\t\tstrbuf_setlen(&path, len);\n+\t\tstrbuf_addstr(&path, \"/packed-refs\");\n+\t\tif (access(path.buf, R_OK))\n+\t\t\tgoto done;\n+\t}\n \n \tret = 1;\n done:\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 18fe1c25e6..4ad1690817 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -214,6 +214,15 @@ test_expect_success 'gc.repackFilter launches repack with a filter' '\n \tgrep -E \"^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*\" trace.out\n '\n \n+test_expect_success 'GCed bare repos without empty refs/ still recognized' '\n+\tGIT_DIR=\"$PWD\"/bare.git git cat-file -e master &&\n+\ttest_dir_is_empty bare.git/refs/heads &&\n+\ttest_dir_is_empty bare.git/refs/tags &&\n+\ttest_file_not_empty bare.git/packed-refs &&\n+\trm -r bare.git/refs &&\n+\tGIT_DIR=\"$PWD\"/bare.git git cat-file -e master\n+'\n+\n test_expect_success 'gc.repackFilterTo store filtered out objects' '\n \ttest_when_finished \"rm -rf bare.git filtered.git\" &&\n \n-- \n2.43.0.1.g67290e5b65\n\n"},{"id":"485196","messageId":"ZWY1WSUL11Y6990U@google.com","threadId":"60534","inReplyTo":"20231128142845.11523-1-adamm@zombino.com","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-11-28T18:45:45Z","receivedAt":"2023-11-28T18:45:50Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.11.28 15:28, Adam Majer wrote:\n> In a garbage collected bare git repository, the refs/ subdirectory is\n> empty. In use-cases when such a repository is directly added into\n> another repository, it no longer is detected as valid. Git doesn't\n> preserve empty paths so refs/ subdirectory is not present. Simply\n> creating an empty refs/ subdirectory fixes this problem.\n> \n> Looking more carefully, there are two backends to handle various refs in\n> git -- the files backend that uses refs/ subdirectory and the\n> packed-refs backend that uses packed-refs file. If references are not\n> found in refs/ subdirectory (or directory doesn't exist), the\n> packed-refs directory will be consulted. Garbage collected repository\n> will have all its references in packed-refs file.\n> \n> To allow the use-case when packed-refs is the only source of refs and\n> refs/ subdirectory is simply not present, augment 'is_git_directory()'\n> setup function to look for packed-refs file as an alternative to refs/\n> subdirectory.\n> \n> Signed-off-by: Adam Majer \n> ---\n> setup.c | 10 +++++++---\n> t/t6500-gc.sh | 9 +++++++++\n> 2 files changed, 16 insertions(+), 3 deletions(-)\n> \n> diff --git a/setup.c b/setup.c\n> index fc592dc6dd..2a6dda6ae9 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -348,7 +348,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> *\n> * - either an objects/ directory _or_ the proper\n> * GIT_OBJECT_DIRECTORY environment variable\n> - * - a refs/ directory\n> + * - a refs/ directory or packed-refs file\n> * - either a HEAD symlink or a HEAD file that is formatted as\n> * a proper \"ref:\", or a regular file HEAD that has a properly\n> * formatted sha1 object name.\n> @@ -384,8 +384,12 @@ int is_git_directory(const char *suspect)\n> \n> \tstrbuf_setlen(&path, len);\n> \tstrbuf_addstr(&path, \"/refs\");\n> -\tif (access(path.buf, X_OK))\n> -\t\tgoto done;\n> +\tif (access(path.buf, X_OK)) {\n> +\t\tstrbuf_setlen(&path, len);\n> +\t\tstrbuf_addstr(&path, \"/packed-refs\");\n> +\t\tif (access(path.buf, R_OK))\n> +\t\t\tgoto done;\n> +\t}\n> \n> \tret = 1;\n> done:\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 18fe1c25e6..4ad1690817 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -214,6 +214,15 @@ test_expect_success 'gc.repackFilter launches repack with a filter' '\n> \tgrep -E \"^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*\" trace.out\n> '\n> \n> +test_expect_success 'GCed bare repos without empty refs/ still recognized' '\n> +\tGIT_DIR=\"$PWD\"/bare.git git cat-file -e master &&\n> +\ttest_dir_is_empty bare.git/refs/heads &&\n> +\ttest_dir_is_empty bare.git/refs/tags &&\n> +\ttest_file_not_empty bare.git/packed-refs &&\n> +\trm -r bare.git/refs &&\n> +\tGIT_DIR=\"$PWD\"/bare.git git cat-file -e master\n> +'\n> +\n> test_expect_success 'gc.repackFilterTo store filtered out objects' '\n> \ttest_when_finished \"rm -rf bare.git filtered.git\" &&\n> \n> -- \n> 2.43.0.1.g67290e5b65\n\nThanks for the fixes. This looks good to me. BTW, in the future please\nadd a version number when you send updated patches (e.g. add \"-v 2\" to\nyour command-line if you're using git-format-patch).\n\nReviewed-by: Josh Steadmon \n"},{"id":"485197","messageId":"20231128190446.GA10477@coredump.intra.peff.net","threadId":"60534","inReplyTo":"20231128142845.11523-1-adamm@zombino.com","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-28T19:04:46Z","receivedAt":"2023-11-28T19:04:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 28, 2023 at 03:28:45PM +0100, Adam Majer wrote:\n\n> In a garbage collected bare git repository, the refs/ subdirectory is\n> empty. In use-cases when such a repository is directly added into\n> another repository, it no longer is detected as valid. Git doesn't\n> preserve empty paths so refs/ subdirectory is not present. Simply\n> creating an empty refs/ subdirectory fixes this problem.\n\nI understand your use case, but I still have a vague feeling that this\nis bending some assumptions in a way that may create problems or\nconfusion later. In particular:\n\n> Looking more carefully, there are two backends to handle various refs in\n> git -- the files backend that uses refs/ subdirectory and the\n> packed-refs backend that uses packed-refs file. If references are not\n> found in refs/ subdirectory (or directory doesn't exist), the\n> packed-refs directory will be consulted. Garbage collected repository\n> will have all its references in packed-refs file.\n\nThis second paragraph doesn't seem totally accurate to me. There are not\nreally two backends that Git can use. For production use, there is just\none, the \"files\" backend, which happens to also use packed-refs under\nthe hood (and a convenient way for the code to structure this was a\nsubordinate backend). But it has never been possible to have a repo that\njust uses packed-refs.\n\nThere is also the experimental reftable, of course. And there we have\nnot yet loosened is_git_directory(), and it has to create an unused\n\"refs/\" directory (there has been some discussion about allowing it to\nbe an empty file, though no patches have been merged).\n\nSo with regards to the loosening in your patch, my questions would be:\n\n - if we are going to change the rules for repository detection, is\n this where we want to end up? We haven't changed them (yet) for\n reftables. If we are going to do so, should we have a scheme that\n will work for that transition, too? The \"refs is an empty file\"\n scheme would fix your use case, too (though see below).\n\n - is the rest of Git ready to handle a missing \"refs/\" directory? It\n looks like making a ref will auto-create it (since we may have to\n make refs/foo/bar/... anyway).\n\n - what about other implementations? Your embedded repos will\n presumably not work with libgit2, jgit, etc, until they also get\n similar patches.\n\n - what about empty repositories? In that case there will be no \"refs/\"\n file and no \"packed-refs\" file (such a repository is less likely, of\n course, but it may contain objects but no refs, or the point may be\n to have an empty repo as a test vector). Likewise, it is possible\n for a repository to have an empty \"objects\" directory (even with a\n non-empty refs directory, if there are only symrefs), and your patch\n doesn't address that.\n\n> To allow the use-case when packed-refs is the only source of refs and\n> refs/ subdirectory is simply not present, augment 'is_git_directory()'\n> setup function to look for packed-refs file as an alternative to refs/\n> subdirectory.\n\nGetting back to your use case, I'd suggest one of:\n\n - do the usual \"touch refs/.gitignore\" trick to explicitly track the\n empty directory. It looks like the ref code will ignore this (we\n don't allow ref names to start with \".\" in a path component)\n\n - whatever is consuming the embedded repos could \"mkdir -p refs\n objects\" as needed. This is a minor pain, but I think in the long\n term we are moving to a world where you have to explicitly do\n \"GIT_DIR=$PWD/embedded.git\" to access an embedded bare repo. So\n they're already special and require some setup; adding an extra step\n may not be so bad.\n\nNow it may be that neither of those solutions is acceptable for various\nreasons. But it is probably worth detailing those reasons in your commit\nmessage.\n\n-Peff\n"},{"id":"485242","messageId":"ZWcOvjGPVS_CMUAk@tanuki","threadId":"60534","inReplyTo":"20231128190446.GA10477@coredump.intra.peff.net","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-29T10:13:18Z","receivedAt":"2023-11-29T10:13:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:\n> On Tue, Nov 28, 2023 at 03:28:45PM +0100, Adam Majer wrote:\n> \n> > In a garbage collected bare git repository, the refs/ subdirectory is\n> > empty. In use-cases when such a repository is directly added into\n> > another repository, it no longer is detected as valid. Git doesn't\n> > preserve empty paths so refs/ subdirectory is not present. Simply\n> > creating an empty refs/ subdirectory fixes this problem.\n> \n> I understand your use case, but I still have a vague feeling that this\n> is bending some assumptions in a way that may create problems or\n> confusion later. In particular:\n> \n> > Looking more carefully, there are two backends to handle various refs in\n> > git -- the files backend that uses refs/ subdirectory and the\n> > packed-refs backend that uses packed-refs file. If references are not\n> > found in refs/ subdirectory (or directory doesn't exist), the\n> > packed-refs directory will be consulted. Garbage collected repository\n> > will have all its references in packed-refs file.\n> \n> This second paragraph doesn't seem totally accurate to me. There are not\n> really two backends that Git can use. For production use, there is just\n> one, the \"files\" backend, which happens to also use packed-refs under\n> the hood (and a convenient way for the code to structure this was a\n> subordinate backend). But it has never been possible to have a repo that\n> just uses packed-refs.\n> \n> There is also the experimental reftable, of course. And there we have\n> not yet loosened is_git_directory(), and it has to create an unused\n> \"refs/\" directory (there has been some discussion about allowing it to\n> be an empty file, though no patches have been merged).\n\nAs I'm currently working on the reftable backend this thought has also\ncrossed my mind. The reftable backend doesn't only create \"refs/\", but\nit also creates \"HEAD\" with contents \"ref: refs/heads/.invalid\" so that\nGit commands recognize the Git directory properly. Longer-term I would\nreally love to see us doing a better job of detecting Git repositories\nso that we don't have to carry this legacy baggage around.\n\nI can see different ways for how to do this:\n\n - Either we iterate through all known reference backends, asking\n each of them whether they recognize the directory as something\n they understand.\n\n - Or we start parsing the gitconfig of the repository so that we can\n learn about which reference backend to expect, and then ask that\n specific backend whether it thinks that the directory indeed looks\n like something it can handle.\n\nI'd personally prefer the latter, but I'm not sure whether we really\nwant to try and parse any file that happens to be called \"config\".\n\n> So with regards to the loosening in your patch, my questions would be:\n> \n> - if we are going to change the rules for repository detection, is\n> this where we want to end up? We haven't changed them (yet) for\n> reftables. If we are going to do so, should we have a scheme that\n> will work for that transition, too? The \"refs is an empty file\"\n> scheme would fix your use case, too (though see below).\n> \n> - is the rest of Git ready to handle a missing \"refs/\" directory? It\n> looks like making a ref will auto-create it (since we may have to\n> make refs/foo/bar/... anyway).\n> \n> - what about other implementations? Your embedded repos will\n> presumably not work with libgit2, jgit, etc, until they also get\n> similar patches.\n> \n> - what about empty repositories? In that case there will be no \"refs/\"\n> file and no \"packed-refs\" file (such a repository is less likely, of\n> course, but it may contain objects but no refs, or the point may be\n> to have an empty repo as a test vector). Likewise, it is possible\n> for a repository to have an empty \"objects\" directory (even with a\n> non-empty refs directory, if there are only symrefs), and your patch\n> doesn't address that.\n\nJust throwing this out there, but we could use this as an excuse to\nintroduce \"extensions.refFormat\". If it's explicitly configured to be\n\"reffiles\" then we accept repositories even if they don't have the\n\"refs/\" directory or a \"packed-refs\" file. This would still require work\nin alternative implementations of Git, but this work will need to happen\nanyway when the reftable backend lands.\n\nI'd personally love for this extension to be introduced before I'm\nsending the reftable backend upstream so that we can have discussions\naround it beforehand.\n\nPatrick\n\n> > To allow the use-case when packed-refs is the only source of refs and\n> > refs/ subdirectory is simply not present, augment 'is_git_directory()'\n> > setup function to look for packed-refs file as an alternative to refs/\n> > subdirectory.\n> \n> Getting back to your use case, I'd suggest one of:\n> \n> - do the usual \"touch refs/.gitignore\" trick to explicitly track the\n> empty directory. It looks like the ref code will ignore this (we\n> don't allow ref names to start with \".\" in a path component)\n> \n> - whatever is consuming the embedded repos could \"mkdir -p refs\n> objects\" as needed. This is a minor pain, but I think in the long\n> term we are moving to a world where you have to explicitly do\n> \"GIT_DIR=$PWD/embedded.git\" to access an embedded bare repo. So\n> they're already special and require some setup; adding an extra step\n> may not be so bad.\n> \n> Now it may be that neither of those solutions is acceptable for various\n> reasons. But it is probably worth detailing those reasons in your commit\n> message.\n> \n> -Peff\n> \n"},{"id":"485244","messageId":"ZWethlRRtuQLDRlJ@nand.local","threadId":"60534","inReplyTo":"20231128190446.GA10477@coredump.intra.peff.net","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-29T21:30:46Z","receivedAt":"2023-11-29T21:30:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:\n> - whatever is consuming the embedded repos could \"mkdir -p refs\n> objects\" as needed. This is a minor pain, but I think in the long\n> term we are moving to a world where you have to explicitly do\n> \"GIT_DIR=$PWD/embedded.git\" to access an embedded bare repo. So\n> they're already special and require some setup; adding an extra step\n> may not be so bad.\n\nI hope not. I suppose that using embedded bare repositories in a test\nrequires additional setup at least to \"cd\" into the directory (if they\nare not using `$GIT_DIR` or `--git-dir` already). But I fear that\nimposing even a small change like this is too tall an order for how many\nmillions of these exist in the wild across all sorts of projects.\n\nThanks,\nTaylor\n"},{"id":"485395","messageId":"20231206201048.GE103708@coredump.intra.peff.net","threadId":"60534","inReplyTo":"ZWcOvjGPVS_CMUAk@tanuki","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-06T20:10:48Z","receivedAt":"2023-12-06T20:10:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 29, 2023 at 11:13:18AM +0100, Patrick Steinhardt wrote:\n\n> As I'm currently working on the reftable backend this thought has also\n> crossed my mind. The reftable backend doesn't only create \"refs/\", but\n> it also creates \"HEAD\" with contents \"ref: refs/heads/.invalid\" so that\n> Git commands recognize the Git directory properly. Longer-term I would\n> really love to see us doing a better job of detecting Git repositories\n> so that we don't have to carry this legacy baggage around.\n> \n> I can see different ways for how to do this:\n> \n> - Either we iterate through all known reference backends, asking\n> each of them whether they recognize the directory as something\n> they understand.\n> \n> - Or we start parsing the gitconfig of the repository so that we can\n> learn about which reference backend to expect, and then ask that\n> specific backend whether it thinks that the directory indeed looks\n> like something it can handle.\n> \n> I'd personally prefer the latter, but I'm not sure whether we really\n> want to try and parse any file that happens to be called \"config\".\n\nWe do eventually parse the config file to pick up repositoryFormatVersion.\nBut there's sort of a chicken-and-egg here where we only do so after\ngaining some confidence that it's a repo directory. :)\n\nI actually think the \"ask each backend if it looks plausible\" is\nreasonable, at least for an implementation that knows about all\nbackends. Though what gives me pause is how older versions of Git will\nbehave with a new-format repository that does not have a \"refs\"\ndirectory.\n\nThere are really two compatibility checks. In is_git_directory(), we\nwant to say \"is this a repo or not\". And then later we parse the config,\nmake sure the repository format is OK, and that we support all\nextensions. So right now, an older version of Git that encounters a\nreftable-formatted repo (that has a vestigial \"refs/\" directory) says\n\"ah, that is a repo, but I don't understand it\" (the latter because\npresumably the repo version/extensions in .git/config are values it\ndoesn't know about). But if we get rid of \"refs/\", then older versions\nof Git will stop even considering it as a repo at all, and will keep\nsearching up to the ceiling directory. So either:\n\n 1. They'll find nothing, and you'll get \"you're not in a git repo\",\n rather than \"you're in a git repo, but I don't understand it\".\n Which is slightly worse.\n\n 2. They'll find some _other_ containing repo. Which could be quite\n confusing.\n\nSo forgetting at all about how we structure the code, it seems to me\nthat the problem is not new code, but all of the existing code which\nlooks for access(\"refs\", X_OK).\n\nI dunno. Maybe that is being too paranoid about backwards compatibility.\nPeople will have to turn on reftable manually, at least for a while, and\nwould hopefully know what they are signing up for, and that old versions\nmight not work as well. And by the time a new format becomes the\ndefault, it's possible that those older versions would have become quite\nrare.\n\n> Just throwing this out there, but we could use this as an excuse to\n> introduce \"extensions.refFormat\". If it's explicitly configured to be\n> \"reffiles\" then we accept repositories even if they don't have the\n> \"refs/\" directory or a \"packed-refs\" file. This would still require work\n> in alternative implementations of Git, but this work will need to happen\n> anyway when the reftable backend lands.\n> \n> I'd personally love for this extension to be introduced before I'm\n> sending the reftable backend upstream so that we can have discussions\n> around it beforehand.\n\nWe already have an extension config option to specify that we're using\nreftable, don't we? But anything in config has the same chicken-and-egg\nproblems as above, I think.\n\n-Peff\n"},{"id":"485396","messageId":"20231206210836.GA106480@coredump.intra.peff.net","threadId":"60534","inReplyTo":"ZWethlRRtuQLDRlJ@nand.local","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-06T21:08:36Z","receivedAt":"2023-12-06T21:08:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 29, 2023 at 04:30:46PM -0500, Taylor Blau wrote:\n\n> On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:\n> > - whatever is consuming the embedded repos could \"mkdir -p refs\n> > objects\" as needed. This is a minor pain, but I think in the long\n> > term we are moving to a world where you have to explicitly do\n> > \"GIT_DIR=$PWD/embedded.git\" to access an embedded bare repo. So\n> > they're already special and require some setup; adding an extra step\n> > may not be so bad.\n> \n> I hope not. I suppose that using embedded bare repositories in a test\n> requires additional setup at least to \"cd\" into the directory (if they\n> are not using `$GIT_DIR` or `--git-dir` already). But I fear that\n> imposing even a small change like this is too tall an order for how many\n> millions of these exist in the wild across all sorts of projects.\n\nI dunno. I am skeptical that there are millions of these. Who really\nwants to embed bare git repos except for projects related to Git itself,\nwhich want test vectors? Is there a use case I'm missing?\n\n-Peff\n"},{"id":"485414","messageId":"ZXFt1foDuHKBmFwk@tanuki","threadId":"60534","inReplyTo":"20231206201048.GE103708@coredump.intra.peff.net","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T07:01:41Z","receivedAt":"2023-12-07T07:01:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 03:10:48PM -0500, Jeff King wrote:\n> On Wed, Nov 29, 2023 at 11:13:18AM +0100, Patrick Steinhardt wrote:\n> \n> > As I'm currently working on the reftable backend this thought has also\n> > crossed my mind. The reftable backend doesn't only create \"refs/\", but\n> > it also creates \"HEAD\" with contents \"ref: refs/heads/.invalid\" so that\n> > Git commands recognize the Git directory properly. Longer-term I would\n> > really love to see us doing a better job of detecting Git repositories\n> > so that we don't have to carry this legacy baggage around.\n> > \n> > I can see different ways for how to do this:\n> > \n> > - Either we iterate through all known reference backends, asking\n> > each of them whether they recognize the directory as something\n> > they understand.\n> > \n> > - Or we start parsing the gitconfig of the repository so that we can\n> > learn about which reference backend to expect, and then ask that\n> > specific backend whether it thinks that the directory indeed looks\n> > like something it can handle.\n> > \n> > I'd personally prefer the latter, but I'm not sure whether we really\n> > want to try and parse any file that happens to be called \"config\".\n> \n> We do eventually parse the config file to pick up repositoryFormatVersion.\n> But there's sort of a chicken-and-egg here where we only do so after\n> gaining some confidence that it's a repo directory. :)\n> \n> I actually think the \"ask each backend if it looks plausible\" is\n> reasonable, at least for an implementation that knows about all\n> backends. Though what gives me pause is how older versions of Git will\n> behave with a new-format repository that does not have a \"refs\"\n> directory.\n> \n> There are really two compatibility checks. In is_git_directory(), we\n> want to say \"is this a repo or not\". And then later we parse the config,\n> make sure the repository format is OK, and that we support all\n> extensions. So right now, an older version of Git that encounters a\n> reftable-formatted repo (that has a vestigial \"refs/\" directory) says\n> \"ah, that is a repo, but I don't understand it\" (the latter because\n> presumably the repo version/extensions in .git/config are values it\n> doesn't know about). But if we get rid of \"refs/\", then older versions\n> of Git will stop even considering it as a repo at all, and will keep\n> searching up to the ceiling directory. So either:\n> \n> 1. They'll find nothing, and you'll get \"you're not in a git repo\",\n> rather than \"you're in a git repo, but I don't understand it\".\n> Which is slightly worse.\n> \n> 2. They'll find some _other_ containing repo. Which could be quite\n> confusing.\n> \n> So forgetting at all about how we structure the code, it seems to me\n> that the problem is not new code, but all of the existing code which\n> looks for access(\"refs\", X_OK).\n\nTrue. The question is of course how much value there is in an old tool\nto be able to discover a new repository that it wouldn't be able to read\nin the first place due to it not understanding the reference format. So\nI'd very much like to see that eventually, we're able to get rid of\n\"legacy\" cruft that doesn't serve any purpose anymore.\n\nThe question is whether we can do a better job of this going forward so\nthat at least we don't have to pose the same question in the future.\nRight now, we'll face the same problem whenever any part of the current\non-disk repository data structures changes.\n\nI wonder whether it would make sense to introduce something like a\nfilesystem-level hint, e.g. in the form of a new \".is-git-repository\"\nfile. If Git discovers that file then it assumes the directory to be a\nGit repository -- and everything else is set up by parsing the config\nand thus the repository's configured format.\n\n> I dunno. Maybe that is being too paranoid about backwards compatibility.\n> People will have to turn on reftable manually, at least for a while, and\n> would hopefully know what they are signing up for, and that old versions\n> might not work as well. And by the time a new format becomes the\n> default, it's possible that those older versions would have become quite\n> rare.\n> \n> > Just throwing this out there, but we could use this as an excuse to\n> > introduce \"extensions.refFormat\". If it's explicitly configured to be\n> > \"reffiles\" then we accept repositories even if they don't have the\n> > \"refs/\" directory or a \"packed-refs\" file. This would still require work\n> > in alternative implementations of Git, but this work will need to happen\n> > anyway when the reftable backend lands.\n> > \n> > I'd personally love for this extension to be introduced before I'm\n> > sending the reftable backend upstream so that we can have discussions\n> > around it beforehand.\n> \n> We already have an extension config option to specify that we're using\n> reftable, don't we? But anything in config has the same chicken-and-egg\n> problems as above, I think.\n\nNot yet, no. I plan to submit the new \"extensions.refFormat\" extension\nsoonish though, probably next week.\n\nPatrick\n"},{"id":"485436","messageId":"20231207073451.GA1278091@coredump.intra.peff.net","threadId":"60534","inReplyTo":"ZXFt1foDuHKBmFwk@tanuki","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:34:51Z","receivedAt":"2023-12-07T07:34:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2023 at 08:01:41AM +0100, Patrick Steinhardt wrote:\n\n> > So forgetting at all about how we structure the code, it seems to me\n> > that the problem is not new code, but all of the existing code which\n> > looks for access(\"refs\", X_OK).\n> \n> True. The question is of course how much value there is in an old tool\n> to be able to discover a new repository that it wouldn't be able to read\n> in the first place due to it not understanding the reference format. So\n> I'd very much like to see that eventually, we're able to get rid of\n> \"legacy\" cruft that doesn't serve any purpose anymore.\n\nRight, nobody is going to be mad about not being able to use the\nrepository with old code. My concern is that by skipping it in the\ndiscovery phase, though, the user may see unexpected behavior (like\ncontinuing and finding some _other_ repo). I admit it's a pretty narrow\ncase, though.\n\n> The question is whether we can do a better job of this going forward so\n> that at least we don't have to pose the same question in the future.\n> Right now, we'll face the same problem whenever any part of the current\n> on-disk repository data structures changes.\n> \n> I wonder whether it would make sense to introduce something like a\n> filesystem-level hint, e.g. in the form of a new \".is-git-repository\"\n> file. If Git discovers that file then it assumes the directory to be a\n> Git repository -- and everything else is set up by parsing the config\n> and thus the repository's configured format.\n\nI kind of think the presence of a well-formed HEAD is a good indicator\nthat it is a Git directory. IOW, I don't have any real problem with\nsimply loosening is_git_directory() to remove the \"refs/\" check\nentirely. But again, that can only help us going forward; older versions\nwill still get confused if we truly ditch \"refs/\" for other formats.\n\nOf course some ref formats may want to avoid the \"HEAD\" file itself, so\nyour .is-git-repository would be cleaner. I'm just not sure if it's\nworth the headache to try to switch things now.\n\n> > We already have an extension config option to specify that we're using\n> > reftable, don't we? But anything in config has the same chicken-and-egg\n> > problems as above, I think.\n> \n> Not yet, no. I plan to submit the new \"extensions.refFormat\" extension\n> soonish though, probably next week.\n\nAh, OK. I remember talking about it with Han-Wen long ago, but I admit I\nhave not paid much attention to reftable work recently. :) So I am happy\nyou are picking it up.\n\n-Peff\n"},{"id":"485442","messageId":"d47f6496-0328-480b-8296-da9e812f6e1b@zombino.com","threadId":"60534","inReplyTo":"20231206210836.GA106480@coredump.intra.peff.net","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-12-07T08:20:44Z","receivedAt":"2023-12-07T08:20:46Z","isPatch":true,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"\n\nOn 12/6/23 22:08, Jeff King wrote:\n> On Wed, Nov 29, 2023 at 04:30:46PM -0500, Taylor Blau wrote:\n> \n>> On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:\n>>> - whatever is consuming the embedded repos could \"mkdir -p refs\n>>> objects\" as needed. This is a minor pain, but I think in the long\n>>> term we are moving to a world where you have to explicitly do\n>>> \"GIT_DIR=$PWD/embedded.git\" to access an embedded bare repo. So\n>>> they're already special and require some setup; adding an extra step\n>>> may not be so bad.\n>>\n>> I hope not. I suppose that using embedded bare repositories in a test\n>> requires additional setup at least to \"cd\" into the directory (if they\n>> are not using `$GIT_DIR` or `--git-dir` already). But I fear that\n>> imposing even a small change like this is too tall an order for how many\n>> millions of these exist in the wild across all sorts of projects.\n> \n> I dunno. I am skeptical that there are millions of these. Who really\n> wants to embed bare git repos except for projects related to Git itself,\n> which want test vectors? Is there a use case I'm missing?\nWell, it's an \"easy\" thing to do, instead of recreating these test cases \nfrom sources like it's done here. It seems this is what happens in \nprojects like Gitea.\n\nAs to the original questions you've raised earlier in the thread, I \nthought about it, and I don't really have a compelling reason to try to \nforce this patch into Git. At least, I do not feel it necessary to try \nto argue the points you've raised. If that means the patch is ignored, \nI'm ok with that.\n\nThe reasons I put it here is simply I found that it fixes an issue I \ncame across and that \"everything else\" worked. I don't know the \nintricacies of current or future git plans and I would rather delegate \nsuch discussion to the experts.\n\nBest regards,\nAdam\n"},{"id":"485443","messageId":"ZXGERbcSiM8PDQbw@tanuki","threadId":"60534","inReplyTo":"20231207073451.GA1278091@coredump.intra.peff.net","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:37:25Z","receivedAt":"2023-12-07T08:37:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:34:51AM -0500, Jeff King wrote:\n> On Thu, Dec 07, 2023 at 08:01:41AM +0100, Patrick Steinhardt wrote:\n> \n> > > So forgetting at all about how we structure the code, it seems to me\n> > > that the problem is not new code, but all of the existing code which\n> > > looks for access(\"refs\", X_OK).\n> > \n> > True. The question is of course how much value there is in an old tool\n> > to be able to discover a new repository that it wouldn't be able to read\n> > in the first place due to it not understanding the reference format. So\n> > I'd very much like to see that eventually, we're able to get rid of\n> > \"legacy\" cruft that doesn't serve any purpose anymore.\n> \n> Right, nobody is going to be mad about not being able to use the\n> repository with old code. My concern is that by skipping it in the\n> discovery phase, though, the user may see unexpected behavior (like\n> continuing and finding some _other_ repo). I admit it's a pretty narrow\n> case, though.\n\nAgreed, that's also an angle I brought up in a separate thread [1]. The\nsecond benefit is that the user would get a proper error message stating\nthat the \"extensions.refFormat\" is not understood compared to Git just\nskipping over the repository completely.\n\n> > The question is whether we can do a better job of this going forward so\n> > that at least we don't have to pose the same question in the future.\n> > Right now, we'll face the same problem whenever any part of the current\n> > on-disk repository data structures changes.\n> > \n> > I wonder whether it would make sense to introduce something like a\n> > filesystem-level hint, e.g. in the form of a new \".is-git-repository\"\n> > file. If Git discovers that file then it assumes the directory to be a\n> > Git repository -- and everything else is set up by parsing the config\n> > and thus the repository's configured format.\n> \n> I kind of think the presence of a well-formed HEAD is a good indicator\n> that it is a Git directory. IOW, I don't have any real problem with\n> simply loosening is_git_directory() to remove the \"refs/\" check\n> entirely. But again, that can only help us going forward; older versions\n> will still get confused if we truly ditch \"refs/\" for other formats.\n> \n> Of course some ref formats may want to avoid the \"HEAD\" file itself, so\n> your .is-git-repository would be cleaner. I'm just not sure if it's\n> worth the headache to try to switch things now.\n\nI think that both \"HEAD\" and \"refs/\" are in the same spirit and consider\nboth to be legacy cruft that ideally wouldn't exist with the reftable\nbackend. I think dropping just one of these requirements (\"refs/\") is\nthe least favorable option though:\n\n - We'd still have unneeded files that only exist to aid old clients.\n\n - At the same time, the old clients wouldn't be able to detect the\n repository anymore and need an update. So we could just as well drop\n both files and would have the same outcome.\n\n - This is not a long-term solution in case anything else in the\n on-disk format will ever change.\n\nWhether it's worth getting rid of them now... probably not, at least not\nfor the time being. But if we want to address this issue I'd rather want\nto aim for a proper solution that also works for future changes.\n\nPatrick\n\n[1]: \n"},{"id":"485475","messageId":"xmqqfs0c36fq.fsf@gitster.g","threadId":"60534","inReplyTo":"20231128190446.GA10477@coredump.intra.peff.net","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-08T18:17:13Z","receivedAt":"2023-12-08T18:17:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So with regards to the loosening in your patch, my questions would be:\n>\n> - if we are going to change the rules for repository detection, is\n> this where we want to end up? We haven't changed them (yet) for\n> reftables. If we are going to do so, should we have a scheme that\n> will work for that transition, too? The \"refs is an empty file\"\n> scheme would fix your use case, too (though see below).\n>\n> - is the rest of Git ready to handle a missing \"refs/\" directory? It\n> looks like making a ref will auto-create it (since we may have to\n> make refs/foo/bar/... anyway).\n>\n> - what about other implementations? Your embedded repos will\n> presumably not work with libgit2, jgit, etc, until they also get\n> similar patches.\n>\n> - what about empty repositories? In that case there will be no \"refs/\"\n> file and no \"packed-refs\" file (such a repository is less likely, of\n> course, but it may contain objects but no refs, or the point may be\n> to have an empty repo as a test vector). Likewise, it is possible\n> for a repository to have an empty \"objects\" directory (even with a\n> non-empty refs directory, if there are only symrefs), and your patch\n> doesn't address that.\n\nAll good points.\n\n> Getting back to your use case, I'd suggest one of:\n>\n> - do the usual \"touch refs/.gitignore\" trick to explicitly track the\n> empty directory. It looks like the ref code will ignore this (we\n> don't allow ref names to start with \".\" in a path component)\n>\n> - whatever is consuming the embedded repos could \"mkdir -p refs\n> objects\" as needed. This is a minor pain, but I think in the long\n> term we are moving to a world where you have to explicitly do\n> \"GIT_DIR=$PWD/embedded.git\" to access an embedded bare repo. So\n> they're already special and require some setup; adding an extra step\n> may not be so bad.\n\nYeah, it truly is caused by the combination of the fact that we do\nnot \"track\" empty directories and that skeleton Git repository\nstructure does rely on possibly empty directories. The above two\nare reasonable workarounds when you are dealing with any medium that\ndoes not allow empty directories, not just working tree managed by\nGit.\n"},{"id":"485480","messageId":"ZXOF75NwxI187QDQ@nand.local","threadId":"60534","inReplyTo":"20231206210836.GA106480@coredump.intra.peff.net","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T21:09:03Z","receivedAt":"2023-12-08T21:09:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Dec 06, 2023 at 04:08:36PM -0500, Jeff King wrote:\n> On Wed, Nov 29, 2023 at 04:30:46PM -0500, Taylor Blau wrote:\n>\n> > On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:\n> > > - whatever is consuming the embedded repos could \"mkdir -p refs\n> > > objects\" as needed. This is a minor pain, but I think in the long\n> > > term we are moving to a world where you have to explicitly do\n> > > \"GIT_DIR=$PWD/embedded.git\" to access an embedded bare repo. So\n> > > they're already special and require some setup; adding an extra step\n> > > may not be so bad.\n> >\n> > I hope not. I suppose that using embedded bare repositories in a test\n> > requires additional setup at least to \"cd\" into the directory (if they\n> > are not using `$GIT_DIR` or `--git-dir` already). But I fear that\n> > imposing even a small change like this is too tall an order for how many\n> > millions of these exist in the wild across all sorts of projects.\n>\n> I dunno. I am skeptical that there are millions of these. Who really\n> wants to embed bare git repos except for projects related to Git itself,\n> which want test vectors? Is there a use case I'm missing?\n\nJust picking on GitHub as an example, my copy has a fair number of\nembedded bare repositories:\n\n $ find . -mindepth 2 -type d -name '*.git' | wc -l\n 279\n\nThat might be an unfair example in general, since GitHub probably has a\ngreater need to embed bare repositories than most other projects. But I\nthink that we shouldn't make our decision here based on volume of\nembedded bare repositories, but rather on the number of projects which\nhave >1 embedded bare repository.\n\nIn other words, the cost of migrating a single project's embedded bare\nrepositories is roughly the same whether there are 1 or 279 of them. So\nthe effort scales with the number of projects, not repositories.\n\nPerhaps I'm over-estimating how difficult this transition would be to\nimpose on users. But it does make me very leery to make this kind of a\nchange without having a better sense of how many of them exist in the\nwild.\n\nSearching just on GitHub for `path:**/*.git/config` [^1], it looks like\nthere are ~1,400 results. That provides us an upper-bound on the number\nof projects which have embedded bare repositories, so perhaps I really\nam overestimating the burden we'd be imposing on other projects.\n\nI dunno :-).\n\nThanks,\nTaylor\n\n[^1]: Searching for \"path:**/*.git\" doesn't quite work, since GitHub's\n search doesn't match directories here. So the search I actually used\n isn't perfect, but it should give us a rough approximation.\n"},{"id":"485550","messageId":"20231212012220.GD376323@coredump.intra.peff.net","threadId":"60534","inReplyTo":"ZXOF75NwxI187QDQ@nand.local","subject":"Re: [PATCH] setup: recognize bare repositories with packed-refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T01:22:20Z","receivedAt":"2023-12-12T01:22:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2023 at 04:09:03PM -0500, Taylor Blau wrote:\n\n> > I dunno. I am skeptical that there are millions of these. Who really\n> > wants to embed bare git repos except for projects related to Git itself,\n> > which want test vectors? Is there a use case I'm missing?\n> \n> Just picking on GitHub as an example, my copy has a fair number of\n> embedded bare repositories:\n> \n> $ find . -mindepth 2 -type d -name '*.git' | wc -l\n> 279\n> \n> That might be an unfair example in general, since GitHub probably has a\n> greater need to embed bare repositories than most other projects. But I\n> think that we shouldn't make our decision here based on volume of\n> embedded bare repositories, but rather on the number of projects which\n> have >1 embedded bare repository.\n\nRight, I meant \"I am skeptical there are a lot of projects that have\nembedded repositories\". It is useful if your project is related to\nworking on Git itself and you store your test vectors that way. So\ngithub.git is not alone there (there is libgit2, other forges, and so\non). But I don't think it is representative in general.\n\n> Perhaps I'm over-estimating how difficult this transition would be to\n> impose on users. But it does make me very leery to make this kind of a\n> change without having a better sense of how many of them exist in the\n> wild.\n\nJust to be clear: I am not proposing any transition here. It is already\nthe case that your \"refs/\" directory is necessary for Git to recognize\nthe bare repo, and you risk committing a broken state if you have no\nloose refs in it.\n\nThere's been a proposal elsewhere to require extra steps to recognize an\nembedded bare repo. Which I agree will be a pain for folks who use them,\nbut may be worth it for the security benefit. But here I was only saying\nthat _if_ we do that other change, then adding extra steps might not be\ntoo bad on top. :)\n\n(BTW, I think libgit2 already faces this problem, because it wants\nnon-bare repos; so there is some magic where it stores \".gitted\"\ndirectories, and then renames them on the fly).\n\n> Searching just on GitHub for `path:**/*.git/config` [^1], it looks like\n> there are ~1,400 results. That provides us an upper-bound on the number\n> of projects which have embedded bare repositories, so perhaps I really\n> am overestimating the burden we'd be imposing on other projects.\n\nThanks, that's an interesting number, and matches my intuition.\n\nOf course it's not a true upper bound anyway. It wouldn't count private\nprojects (though maybe it hits github.git in your case), not to mention\nstuff that isn't hosted on GitHub.\n\n-Peff\n"}]}