Re: [GSoC][PATCH v4 1/7] clone: test for our behavior on odd objects/* content

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Sun, Mar 24, 2019 at 5:56 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Fri, Mar 22, 2019 at 08:22:31PM -0300, Matheus Tavares wrote:
> > From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >
> > Add tests for what happens when we perform a local clone on a repo
> > containing odd files at .git/object directory, such as symlinks to other
> > dirs, or unknown files.
> >
> > I'm bending over backwards here to avoid a SHA1 dependency. See [1]
>
> s/SHA1/SHA-1/
>

Thanks, nice catch.

> > for an earlier and simpler version that hardcoded a SHA-1s.
>
> s/SHA-1s/SHA-1/ or s/a SHA-1s/SHA-1s/, depending on what you consider
> multiple occurrances of the same SHA-1.
>

Yes, I think it should be just "SHA-1s". Thanks.

> > This behavior has been the same for a *long* time, but hasn't been
> > tested for.
> >
> > There's a good post-hoc argument to be made for copying over unknown
> > things, e.g. I'd like a git version that doesn't know about the
> > commit-graph to copy it under "clone --local" so a newer git version
> > can make use of it.
> >
> > In follow-up commits we'll look at changing some of this behavior, but
> > for now let's just assert it as-is so we'll notice what we'll change
> > later.
> >
> > 1. https://public-inbox.org/git/20190226002625.13022-5-avarab@gmail.com/
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > Helped-by: Matheus Tavares <matheus.bernardino@usp.br>
>
>
> > +test_expect_success SYMLINKS 'setup repo with manually symlinked dirs and unknown files at objects/' '
> > +     git init T &&
> > +     (
> > +             cd T &&
> > +             test_commit A &&
> > +             git gc &&
> > +             (
> > +                     cd .git/objects &&
> > +                     mv pack packs &&
> > +                     ln -s packs pack
> > +             ) &&
> > +             test_commit B &&
> > +             (
> > +                     cd .git/objects &&
> > +                     find ?? -type d >loose-dirs &&
> > +                     last_loose=$(tail -n 1 loose-dirs) &&
> > +                     rm -f loose-dirs &&
> > +                     mv $last_loose a-loose-dir &&
> > +                     ln -s a-loose-dir $last_loose &&
> > +                     find . -type f | sort >../../../T.objects-files.raw &&
> > +                     echo unknown_content> unknown_file
> > +             )
>
> Please drop these inner subshells.  They are unnecessary, because the
> outer subshell alone is sufficient to ensure that the test script
> returns to the original directory if one of the commands were to fail.

Ok!

> > +     ) &&
> > +     git -C T fsck &&
> > +     git -C T rev-list --all --objects >T.objects
> > +'
> > +
> > +
> > +test_expect_success SYMLINKS 'clone repo with symlinked dirs and unknown files at objects/' '
> > +     for option in --local --no-hardlinks --shared --dissociate
> > +     do
> > +             git clone $option T T$option || return 1 &&
> > +             git -C T$option fsck || return 1 &&
> > +             git -C T$option rev-list --all --objects >T$option.objects &&
> > +             test_cmp T.objects T$option.objects &&
> > +             (
> > +                     cd T$option/.git/objects &&
> > +                     find . -type f | sort >../../../T$option.objects-files.raw
> > +             )
>
> Nit: this might be a bit easier on the eyes when written as
>
>   (
>         cd T$option/.git/objects &&
>         find . -type f
>   ) | sort >T$option.objects-files.raw
>
> because it would avoid that '../../../'.

Sounds good, but in the next patch of this series, another 'find'
statement will be added inside this subshell, so I think that change
is not really possible, unfortunately.

> > +     done &&
> > +
> > +     for raw in $(ls T*.raw)
> > +     do
> > +             sed -e "s!/..\$!/X!; s!/../!/Y/!; s![0-9a-f]\{38,\}!Z!" \
> > +                 -e "/multi-pack-index/d" -e "/commit-graph/d" <$raw >$raw.de-sha || return 1
> > +     done &&
> > +
> > +     cat >expected-files <<-EOF &&
> > +     ./Y/Z
> > +     ./Y/Z
> > +     ./a-loose-dir/Z
> > +     ./Y/Z
> > +     ./info/packs
> > +     ./pack/pack-Z.idx
> > +     ./pack/pack-Z.pack
> > +     ./packs/pack-Z.idx
> > +     ./packs/pack-Z.pack
> > +     ./unknown_file
> > +     EOF
> > +
> > +     for option in --local --dissociate --no-hardlinks
> > +     do
> > +             test_cmp expected-files T$option.objects-files.raw.de-sha || return 1
> > +     done &&
> > +
> > +     cat >expected-files <<-EOF &&
> > +     ./info/alternates
> > +     EOF
>
> Perhaps
>
>   echo ./info/alternates >expected-files

Indeed, much simpler. Thanks.

> > +     test_cmp expected-files T--shared.objects-files.raw
> > +'
> > +
> >  test_done
> > --
> > 2.20.1
> >