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

Commit Message

Matheus Tavares March 22, 2019, 11:22 p.m. UTC

From: Ævar Arnfjörð Bjarmason <[e-mail]>

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]
for an earlier and simpler version that hardcoded a SHA-1s.

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/[e-mail]/

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Signed-off-by: Matheus Tavares <[e-mail]>
Helped-by: Matheus Tavares <[e-mail]>
---
 t/t5604-clone-reference.sh | 116 +++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

Comments

Matheus Tavares March 24, 2019, 6:09 p.m. UTC | #1

On Fri, Mar 22, 2019 at 8:22 PM Matheus Tavares
<[e-mail]> wrote:
>
> From: Ævar Arnfjörð Bjarmason <[e-mail]>
>
> 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]
> for an earlier and simpler version that hardcoded a SHA-1s.
>
> 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/[e-mail]/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> Signed-off-by: Matheus Tavares <[e-mail]>
> Helped-by: Matheus Tavares <[e-mail]>
> ---
>  t/t5604-clone-reference.sh | 116 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>
> diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
> index 4320082b1b..708b1a2c66 100755
> --- a/t/t5604-clone-reference.sh
> +++ b/t/t5604-clone-reference.sh
> @@ -221,4 +221,120 @@ test_expect_success 'clone, dissociate from alternates' '
>         ( cd C && git fsck )
>  '
>
> +test_expect_success 'setup repo with garbage in objects/*' '
> +       git init S &&
> +       (
> +               cd S &&
> +               test_commit A &&
> +
> +               cd .git/objects &&
> +               >.some-hidden-file &&
> +               >some-file &&
> +               mkdir .some-hidden-dir &&
> +               >.some-hidden-dir/some-file &&
> +               >.some-hidden-dir/.some-dot-file &&
> +               mkdir some-dir &&
> +               >some-dir/some-file &&
> +               >some-dir/.some-dot-file
> +       )
> +'
> +
> +test_expect_success 'clone a repo with garbage in objects/*' '
> +       for option in --local --no-hardlinks --shared --dissociate
> +       do
> +               git clone $option S S$option || return 1 &&
> +               git -C S$option fsck || return 1
> +       done &&
> +       find S-* -name "*some*" | sort >actual &&
> +       cat >expected <<-EOF &&
> +       S--dissociate/.git/objects/.some-hidden-file
> +       S--dissociate/.git/objects/some-dir
> +       S--dissociate/.git/objects/some-dir/.some-dot-file
> +       S--dissociate/.git/objects/some-dir/some-file
> +       S--dissociate/.git/objects/some-file
> +       S--local/.git/objects/.some-hidden-file
> +       S--local/.git/objects/some-dir
> +       S--local/.git/objects/some-dir/.some-dot-file
> +       S--local/.git/objects/some-dir/some-file
> +       S--local/.git/objects/some-file
> +       S--no-hardlinks/.git/objects/.some-hidden-file
> +       S--no-hardlinks/.git/objects/some-dir
> +       S--no-hardlinks/.git/objects/some-dir/.some-dot-file
> +       S--no-hardlinks/.git/objects/some-dir/some-file
> +       S--no-hardlinks/.git/objects/some-file
> +       EOF
> +       test_cmp expected actual
> +'
> +
> +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
> +               )
> +       ) &&
> +       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
> +               )
> +       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

Ævar, maybe I'm missing something here, but do we really need the
first sed command ("s!/..\$!/X!") ?


> +       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
> +       test_cmp expected-files T--shared.objects-files.raw
> +'
> +
>  test_done
> --
> 2.20.1
>

SZEDER Gábor March 24, 2019, 8:56 p.m. UTC | #2

On Fri, Mar 22, 2019 at 08:22:31PM -0300, Matheus Tavares wrote:
> From: Ævar Arnfjörð Bjarmason <[e-mail]>
> 
> 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/

> 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.

> 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/[e-mail]/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> Signed-off-by: Matheus Tavares <[e-mail]>
> Helped-by: Matheus Tavares <[e-mail]>


> +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.

> +	) &&
> +	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 '../../../'.

> +	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

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

Matheus Tavares March 26, 2019, 7:43 p.m. UTC | #3

On Sun, Mar 24, 2019 at 5:56 PM SZEDER Gábor <[e-mail]> wrote:
>
> On Fri, Mar 22, 2019 at 08:22:31PM -0300, Matheus Tavares wrote:
> > From: Ævar Arnfjörð Bjarmason <[e-mail]>
> >
> > 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/[e-mail]/
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> > Signed-off-by: Matheus Tavares <[e-mail]>
> > Helped-by: Matheus Tavares <[e-mail]>
>
>
> > +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
> >

Thomas Gummerer March 28, 2019, 9:49 p.m. UTC | #4

On 03/22, Matheus Tavares wrote:
> From: Ævar Arnfjörð Bjarmason <[e-mail]>
> 
> 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]
> for an earlier and simpler version that hardcoded a SHA-1s.
> 
> 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/[e-mail]/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> Signed-off-by: Matheus Tavares <[e-mail]>
> Helped-by: Matheus Tavares <[e-mail]>

The trailers should be in the order things have happened usually.  So
having Ævar's S-o-b first makes sense, but the Helped-by should come
before your S-o-b, as you made the changes first before sending out
the patch series.

When sending someone elses patch in a slightly modified version, it
may also be useful to add which parts you changed, as it was done in
e8dfcace31 ("poll: use GetTickCount64() to avoid wrap-around issues",
2018-10-31) for example.

Iirc, the test that is added in this patch does not work on some
platforms, notably MacOS.  That would mean that we would break
bisectability at this patch on some platforms if we were to introduce
it here.  Therefore I think it would be better to squash this patch
into the next one which fixes these inconsistencies.

Note that I can't test this at the moment, so this concern is only
based on previous discussions that I remember.  If that's already
addressed somehow, all the better!

Matheus Tavares March 29, 2019, 2:06 p.m. UTC | #5

On Thu, Mar 28, 2019 at 6:49 PM Thomas Gummerer <[e-mail]> wrote:
>
> On 03/22, Matheus Tavares wrote:
> > From: Ævar Arnfjörð Bjarmason <[e-mail]>
> >
> > 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]
> > for an earlier and simpler version that hardcoded a SHA-1s.
> >
> > 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/[e-mail]/
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> > Signed-off-by: Matheus Tavares <[e-mail]>
> > Helped-by: Matheus Tavares <[e-mail]>
>
> The trailers should be in the order things have happened usually.  So
> having Ævar's S-o-b first makes sense, but the Helped-by should come
> before your S-o-b, as you made the changes first before sending out
> the patch series.

Ok, thanks for letting me know. I'll fix it.

> When sending someone elses patch in a slightly modified version, it
> may also be useful to add which parts you changed, as it was done in
> e8dfcace31 ("poll: use GetTickCount64() to avoid wrap-around issues",
> 2018-10-31) for example.

Thanks, I didn't know about that! I searched the log and didn't see
many of this on patches with 'Helped-by' tags, is there a particular
case to use it or not?

> Iirc, the test that is added in this patch does not work on some
> platforms, notably MacOS.  That would mean that we would break
> bisectability at this patch on some platforms if we were to introduce
> it here.  Therefore I think it would be better to squash this patch
> into the next one which fixes these inconsistencies.
> Note that I can't test this at the moment, so this concern is only
> based on previous discussions that I remember.  If that's already
> addressed somehow, all the better!

Yes, it is already addressed :) The section of these tests that used
to break on some platforms is now moved to the next patch which also
fixes the platform inconsistencies. Now both patches (this and the
next) work on macOS, NetBSD and GNU/Linux. Also every test and job is
passing at travis-ci, except by the job named "Documentation"[1]. But,
it's weird since these patches don't even touch Documentation/... And
master is failing the same job at my fork as well [2]... Any thoughts
on that?

[1] https://travis-ci.org/MatheusBernardino/git/builds/512713775
[2] https://travis-ci.org/MatheusBernardino/git/builds/513028692

Thomas Gummerer March 29, 2019, 7:31 p.m. UTC | #6

On 03/29, Matheus Tavares Bernardino wrote:
> On Thu, Mar 28, 2019 at 6:49 PM Thomas Gummerer <[e-mail]> wrote:
> > When sending someone elses patch in a slightly modified version, it
> > may also be useful to add which parts you changed, as it was done in
> > e8dfcace31 ("poll: use GetTickCount64() to avoid wrap-around issues",
> > 2018-10-31) for example.
> 
> Thanks, I didn't know about that! I searched the log and didn't see
> many of this on patches with 'Helped-by' tags, is there a particular
> case to use it or not?

Helped-by tags are usually used when you want to give someone credit
for help you got on a patch that you originally authored.  It's up to
you at which point of involvement you actually want to add the tag, I
tend to add them whenever someones input significantly
changes/improves the patch.  I think adding it here might be okay,
it's just less common when sending a patch that someone else authored
originally.

> > Iirc, the test that is added in this patch does not work on some
> > platforms, notably MacOS.  That would mean that we would break
> > bisectability at this patch on some platforms if we were to introduce
> > it here.  Therefore I think it would be better to squash this patch
> > into the next one which fixes these inconsistencies.
> > Note that I can't test this at the moment, so this concern is only
> > based on previous discussions that I remember.  If that's already
> > addressed somehow, all the better!
> 
> Yes, it is already addressed :) The section of these tests that used
> to break on some platforms is now moved to the next patch which also
> fixes the platform inconsistencies. Now both patches (this and the
> next) work on macOS, NetBSD and GNU/Linux.

Great!

>                                             Also every test and job is
> passing at travis-ci, except by the job named "Documentation"[1]. But,
> it's weird since these patches don't even touch Documentation/... And
> master is failing the same job at my fork as well [2]... Any thoughts
> on that?

Yeah, this error seems to have nothing to do with your patch series.
Since the last run of travis on master [*1*] at least the asciidoc
package doesn't seem to have changed, so from a first look I don't
quite understand what's going on there.  In any case, I don't think
you need to worry about that for now, as it hasn't been triggered by
your changes (I won't discourage you from looking at why it is failing
and to try and fix that, but I think your time is probably better
spent looking at this patch series and the proposal for GSoC for
now).

*1*: https://travis-ci.org/git/git/builds/508784487

> [1] https://travis-ci.org/MatheusBernardino/git/builds/512713775
> [2] https://travis-ci.org/MatheusBernardino/git/builds/513028692

SZEDER Gábor March 29, 2019, 7:42 p.m. UTC | #7

On Fri, Mar 29, 2019 at 07:31:58PM +0000, Thomas Gummerer wrote:
> >                                             Also every test and job is
> > passing at travis-ci, except by the job named "Documentation"[1]. But,
> > it's weird since these patches don't even touch Documentation/... And
> > master is failing the same job at my fork as well [2]... Any thoughts
> > on that?
> 
> Yeah, this error seems to have nothing to do with your patch series.
> Since the last run of travis on master [*1*] at least the asciidoc
> package doesn't seem to have changed, so from a first look I don't
> quite understand what's going on there.

https://public-inbox.org/git/[e-mail]/

Matheus Tavares March 30, 2019, 2:49 a.m. UTC | #8

On Fri, Mar 29, 2019 at 4:32 PM Thomas Gummerer <[e-mail]> wrote:
>
> On 03/29, Matheus Tavares Bernardino wrote:
> > On Thu, Mar 28, 2019 at 6:49 PM Thomas Gummerer <[e-mail]> wrote:
> > > When sending someone elses patch in a slightly modified version, it
> > > may also be useful to add which parts you changed, as it was done in
> > > e8dfcace31 ("poll: use GetTickCount64() to avoid wrap-around issues",
> > > 2018-10-31) for example.
> >
> > Thanks, I didn't know about that! I searched the log and didn't see
> > many of this on patches with 'Helped-by' tags, is there a particular
> > case to use it or not?
>
> Helped-by tags are usually used when you want to give someone credit
> for help you got on a patch that you originally authored.  It's up to
> you at which point of involvement you actually want to add the tag, I
> tend to add them whenever someones input significantly
> changes/improves the patch.  I think adding it here might be okay,
> it's just less common when sending a patch that someone else authored
> originally.
>

Ok, got it, thanks!

> > > Iirc, the test that is added in this patch does not work on some
> > > platforms, notably MacOS.  That would mean that we would break
> > > bisectability at this patch on some platforms if we were to introduce
> > > it here.  Therefore I think it would be better to squash this patch
> > > into the next one which fixes these inconsistencies.
> > > Note that I can't test this at the moment, so this concern is only
> > > based on previous discussions that I remember.  If that's already
> > > addressed somehow, all the better!
> >
> > Yes, it is already addressed :) The section of these tests that used
> > to break on some platforms is now moved to the next patch which also
> > fixes the platform inconsistencies. Now both patches (this and the
> > next) work on macOS, NetBSD and GNU/Linux.
>
> Great!
>
> >                                             Also every test and job is
> > passing at travis-ci, except by the job named "Documentation"[1]. But,
> > it's weird since these patches don't even touch Documentation/... And
> > master is failing the same job at my fork as well [2]... Any thoughts
> > on that?
>
> Yeah, this error seems to have nothing to do with your patch series.
> Since the last run of travis on master [*1*] at least the asciidoc
> package doesn't seem to have changed, so from a first look I don't
> quite understand what's going on there.  In any case, I don't think
> you need to worry about that for now, as it hasn't been triggered by
> your changes (I won't discourage you from looking at why it is failing
> and to try and fix that, but I think your time is probably better
> spent looking at this patch series and the proposal for GSoC for
> now).
>

Ok, thanks again.

> *1*: https://travis-ci.org/git/git/builds/508784487
>
> > [1] https://travis-ci.org/MatheusBernardino/git/builds/512713775
> > [2] https://travis-ci.org/MatheusBernardino/git/builds/513028692

Patch

diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index 4320082b1b..708b1a2c66 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -221,4 +221,120 @@  test_expect_success 'clone, dissociate from alternates' '
 	( cd C && git fsck )
 '
 
+test_expect_success 'setup repo with garbage in objects/*' '
+	git init S &&
+	(
+		cd S &&
+		test_commit A &&
+
+		cd .git/objects &&
+		>.some-hidden-file &&
+		>some-file &&
+		mkdir .some-hidden-dir &&
+		>.some-hidden-dir/some-file &&
+		>.some-hidden-dir/.some-dot-file &&
+		mkdir some-dir &&
+		>some-dir/some-file &&
+		>some-dir/.some-dot-file
+	)
+'
+
+test_expect_success 'clone a repo with garbage in objects/*' '
+	for option in --local --no-hardlinks --shared --dissociate
+	do
+		git clone $option S S$option || return 1 &&
+		git -C S$option fsck || return 1
+	done &&
+	find S-* -name "*some*" | sort >actual &&
+	cat >expected <<-EOF &&
+	S--dissociate/.git/objects/.some-hidden-file
+	S--dissociate/.git/objects/some-dir
+	S--dissociate/.git/objects/some-dir/.some-dot-file
+	S--dissociate/.git/objects/some-dir/some-file
+	S--dissociate/.git/objects/some-file
+	S--local/.git/objects/.some-hidden-file
+	S--local/.git/objects/some-dir
+	S--local/.git/objects/some-dir/.some-dot-file
+	S--local/.git/objects/some-dir/some-file
+	S--local/.git/objects/some-file
+	S--no-hardlinks/.git/objects/.some-hidden-file
+	S--no-hardlinks/.git/objects/some-dir
+	S--no-hardlinks/.git/objects/some-dir/.some-dot-file
+	S--no-hardlinks/.git/objects/some-dir/some-file
+	S--no-hardlinks/.git/objects/some-file
+	EOF
+	test_cmp expected actual
+'
+
+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
+		)
+	) &&
+	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
+		)
+	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
+	test_cmp expected-files T--shared.objects-files.raw
+'
+
 test_done