Re: [PATCH 5/6] t5526: break test submodule differently

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

On Tue, Jan 09, 2024 at 02:23:55PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > In 10f5c52656 (submodule: avoid auto-discovery in
> > prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
> > recursive fetch with submodule in the case where the submodule is broken
> > due to whatever reason. The test to exercise that the fix works breaks
> > the submodule by deleting its `HEAD` reference, which will cause us to
> > not detect the directory as a Git repository.
> >
> > While this is perfectly fine in theory, this way of breaking the repo
> > becomes problematic with the current efforts to introduce another refdb
> > backend into Git. The new reftable backend has a stub HEAD file that
> > always contains "ref: refs/heads/.invalid" so that tools continue to be
> > able to detect such a repository. But as the reftable backend will never
> > delete this file even when asked to delete `HEAD` the current way to
> > delete the `HEAD` reference will stop working.
> 
> This patch is not the appropriate place to bikeshed (but since this is
> the first time I've read or paid attention to it), if I saw "ref:
> refs/heads/.invalid", the word ".invalid" would make me think
> something was broken in my repository. Would it make sense to use some
> less alarming word, such as perhaps ".placeholder", ".stand-in",
> ".synthesized" or even the name of the non-file-based backend in use?

Well, something _is_ broken in your repository in case Git ever tries to
read the "HEAD" placeholder in a reftable-enabled repository. But I
guess you rather come from the angle of using `cat HEAD` as a user. I do
agree that using a better hint could help users, but this detail has
already been recorded as such in "Documentation/technical/reftable.txt".

We can of course change this to be "ref: refs/heads/.reftable", but as
you already say this is something that should be discussed in a separate
thread.

> > Adapt the code to instead delete the objects database. Going back with
> > this new way to cuase breakage confirms that it triggers the infinite
> > recursion just the same, and there are no equivalent ongoing efforts to
> > replace the object database with an alternate backend.
> 
> s/cuase/cause/
> 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
> >         # Break the receiving submodule
> > -       test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
> > +       rm -r dst/sub/.git/objects &&
> 
> If there is no guarantee that .git/objects will exist when any
> particular backend is in use, would it be more robust to use -f here,
> as well?
> 
>     rm -rf dst/sub/.git/objects &&

`.git/objects` always exists in a healthy repository. If it doesn't,
then `is_git_directory()` would return a false-ish value and we wouldn't
recognize the repository as such. Or are you saying that this could
potentially change in the future if there was ever to be an alternate
ODB format? If so that is a valid remark, but the test would break
regardless of whether we use `-f` or not: if a missing ".git/objects"
directory does not lead to a corrupted repository then the whole premise
of the test is broken.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]