| Message ID | 20231117203253.21143-1-adamm@zombino.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | setup: recognize bare repositories with packed-refs | expand |
On 2023-11-17 21:32, Adam Majer wrote: > +test_expect_success 'remove empty refs/ subdirectory' ' > + git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 && > + test_dir_is_empty bare.git/refs/heads && > + test_dir_is_empty bare.git/refs/tags && > + rm -r bare.git/refs && > + GIT_DIR="$PWD"/bare.git git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 In the test, I've first tried to use GIT_CEILING_DIRECTORIES="$PWD" here instead, but git went up into the parent directory anyway. I'm not sure if this is a bug, or feature, or my misunderstanding. - Adam
Adam Majer <adamm@zombino.com> writes: > In a garbage collected bare git repository, the refs/ subdirectory is > empty. In use-cases when such a repository is directly added into > another repository, it no longer is detected as valid. Josh & Glen [*], isn't this a layout that we explicitly discourage and eventually plan to forbid anyway? *1* who worked on e35f202b (setup: trace bare repository setups, 2023-05-01)
On Mon, Nov 20, 2023 at 7:24 AM Junio C Hamano <gitster@pobox.com> wrote: > > Adam Majer <adamm@zombino.com> writes: > > > In a garbage collected bare git repository, the refs/ subdirectory is > > empty. In use-cases when such a repository is directly added into > > another repository, it no longer is detected as valid. > > Josh & Glen [*], isn't this a layout that we explicitly discourage and > eventually plan to forbid anyway? If my recollection of [1] serves me correctly, we didn't come to a strong conclusion on whether or not to forbid bare repositories in the working tree, particularly because it would leave existing repos (like Git LFS) high and dry. Though personally, I'd be happy to see a version of Git that forbade bare repositories in the working tree. I don't really recall the bare repo tracing bits, so I'll leave that to Josh. [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/
On 11/20/23 00:24, Junio C Hamano wrote: > Adam Majer <adamm@zombino.com> writes: > >> In a garbage collected bare git repository, the refs/ subdirectory is >> empty. In use-cases when such a repository is directly added into >> another repository, it no longer is detected as valid. > > Josh & Glen [*], isn't this a layout that we explicitly discourage and > eventually plan to forbid anyway? > > *1* who worked on e35f202b (setup: trace bare repository setups, 2023-05-01) This is fair enough. Completely removing embedded git repos would cause some pain in test suites as it was discussed in the thread for that commit[1]. Gitea (for example) has a few dozen embedded bare repositories for tests. In either case, running `git gc` on a bare repository makes it no longer detectable as a git repository after checkout, GIT_DIR or not. This seems to be unintentional and not linked to the other discussion. - Adam
On 2023.11.20 17:31, Glen Choo wrote: > On Mon, Nov 20, 2023 at 7:24 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Adam Majer <adamm@zombino.com> writes: > > > > > In a garbage collected bare git repository, the refs/ subdirectory is > > > empty. In use-cases when such a repository is directly added into > > > another repository, it no longer is detected as valid. > > > > Josh & Glen [*], isn't this a layout that we explicitly discourage and > > eventually plan to forbid anyway? > > If my recollection of [1] serves me correctly, we didn't come to a > strong conclusion on whether or not to forbid bare repositories in the > working tree, particularly because it would leave existing repos (like > Git LFS) high and dry. Though personally, I'd be happy to see a > version of Git that forbade bare repositories in the working tree. > > I don't really recall the bare repo tracing bits, so I'll leave that to Josh. > > [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/ Yeah, my understanding was that we don't want to forbid bare repositories outright, which is why we have the config option to let end-users choose what to do with them.
On 2023.11.17 21:32, Adam Majer wrote: > In a garbage collected bare git repository, the refs/ subdirectory is > empty. In use-cases when such a repository is directly added into > another repository, it no longer is detected as valid. Git doesn't > preserve empty paths so refs/ subdirectory is not present. Simply > creating an empty refs/ subdirectory fixes this problem. > > Looking more carefully, there are two backends to handle various refs in > git -- the files backend that uses refs/ subdirectory and the > packed-refs backend that uses packed-refs file. If references are not > found in refs/ subdirectory (or directory doesn't exist), the > packed-refs directory will be consulted. Garbage collected repository > will have all its references in packed-refs file. > > To allow the use-case when packed-refs is the only source of refs and > refs/ subdirectory is simply not present, augment 'is_git_directory()' > setup function to look for packed-refs file as an alternative to refs/ > subdirectory. > > Signed-off-by: Adam Majer <adamm@zombino.com> > --- > setup.c | 10 +++++++--- > t/t6500-gc.sh | 8 ++++++++ > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/setup.c b/setup.c > index fc592dc6dd..2a6dda6ae9 100644 > --- a/setup.c > +++ b/setup.c > @@ -348,7 +348,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) > * > * - either an objects/ directory _or_ the proper > * GIT_OBJECT_DIRECTORY environment variable > - * - a refs/ directory > + * - a refs/ directory or packed-refs file > * - either a HEAD symlink or a HEAD file that is formatted as > * a proper "ref:", or a regular file HEAD that has a properly > * formatted sha1 object name. > @@ -384,8 +384,12 @@ int is_git_directory(const char *suspect) > > strbuf_setlen(&path, len); > strbuf_addstr(&path, "/refs"); > - if (access(path.buf, X_OK)) > - goto done; > + if (access(path.buf, X_OK)) { > + strbuf_setlen(&path, len); > + strbuf_addstr(&path, "/packed-refs"); > + if (access(path.buf, R_OK)) > + goto done; > + } > > ret = 1; > done: > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index 18fe1c25e6..e81eb7d2ab 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -214,6 +214,14 @@ test_expect_success 'gc.repackFilter launches repack with a filter' ' > grep -E "^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*" trace.out > ' > > +test_expect_success 'remove empty refs/ subdirectory' ' > + git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 && > + test_dir_is_empty bare.git/refs/heads && > + test_dir_is_empty bare.git/refs/tags && > + rm -r bare.git/refs && > + GIT_DIR="$PWD"/bare.git git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 > +' > + Two suggestions for the test here: 1) Can you give the test a more descriptive name, such as "GCed bare repos still recognized"? 2) Can you add a check that bare.git/packed-refs exists? Other than that, looks good to me. Reviewed-by: Josh Steadmon <steadmon@google.com>
On 11/27/23 20:44, Josh Steadmon wrote:> Two suggestions for the test here: > 1) Can you give the test a more descriptive name, such as "GCed bare > repos still recognized"? Thanks, adjusted. I've also added that empty refs/ directory is not there. > 2) Can you add a check that bare.git/packed-refs exists? Done. I've also removed the -C parameter since we actually need GIT_DIR= in all cases to prevent git from going up directory tree. -C is then superflous. In addition, I've changed the hardcoded object id to master branch to make it less magical looking. - Adam
diff --git a/setup.c b/setup.c index fc592dc6dd..2a6dda6ae9 100644 --- a/setup.c +++ b/setup.c @@ -348,7 +348,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) * * - either an objects/ directory _or_ the proper * GIT_OBJECT_DIRECTORY environment variable - * - a refs/ directory + * - a refs/ directory or packed-refs file * - either a HEAD symlink or a HEAD file that is formatted as * a proper "ref:", or a regular file HEAD that has a properly * formatted sha1 object name. @@ -384,8 +384,12 @@ int is_git_directory(const char *suspect) strbuf_setlen(&path, len); strbuf_addstr(&path, "/refs"); - if (access(path.buf, X_OK)) - goto done; + if (access(path.buf, X_OK)) { + strbuf_setlen(&path, len); + strbuf_addstr(&path, "/packed-refs"); + if (access(path.buf, R_OK)) + goto done; + } ret = 1; done: diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 18fe1c25e6..e81eb7d2ab 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -214,6 +214,14 @@ test_expect_success 'gc.repackFilter launches repack with a filter' ' grep -E "^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*" trace.out ' +test_expect_success 'remove empty refs/ subdirectory' ' + git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 && + test_dir_is_empty bare.git/refs/heads && + test_dir_is_empty bare.git/refs/tags && + rm -r bare.git/refs && + GIT_DIR="$PWD"/bare.git git -C bare.git cat-file -e 60dd8ad7d8ed49005c18b7ce9f5b7a45c28df814 +' + test_expect_success 'gc.repackFilterTo store filtered out objects' ' test_when_finished "rm -rf bare.git filtered.git" &&
In a garbage collected bare git repository, the refs/ subdirectory is empty. In use-cases when such a repository is directly added into another repository, it no longer is detected as valid. Git doesn't preserve empty paths so refs/ subdirectory is not present. Simply creating an empty refs/ subdirectory fixes this problem. Looking more carefully, there are two backends to handle various refs in git -- the files backend that uses refs/ subdirectory and the packed-refs backend that uses packed-refs file. If references are not found in refs/ subdirectory (or directory doesn't exist), the packed-refs directory will be consulted. Garbage collected repository will have all its references in packed-refs file. To allow the use-case when packed-refs is the only source of refs and refs/ subdirectory is simply not present, augment 'is_git_directory()' setup function to look for packed-refs file as an alternative to refs/ subdirectory. Signed-off-by: Adam Majer <adamm@zombino.com> --- setup.c | 10 +++++++--- t/t6500-gc.sh | 8 ++++++++ 2 files changed, 15 insertions(+), 3 deletions(-)