Re: [PATCH v4 5/6] core.fsyncobjectfiles: tests for batch mode

[Neeraj Singh <nksingh85@gmail.com>]

On Tue, Sep 21, 2021 at 4:58 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
>
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > Add test cases to exercise batch mode for 'git add'
> > and 'git stash'. These tests ensure that the added
> > data winds up in the object database.
> >
> > I verified the tests by introducing an incorrect rename
> > in do_sync_and_rename.
> >
> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> > ---
> >  t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
> >  t/t3700-add.sh        | 11 +++++++++++
> >  t/t3903-stash.sh      | 14 ++++++++++++++
> >  3 files changed, 59 insertions(+)
> >  create mode 100644 t/lib-unique-files.sh
> >
> > diff --git a/t/lib-unique-files.sh b/t/lib-unique-files.sh
> > new file mode 100644
> > index 00000000000..a8a25eba61d
> > --- /dev/null
> > +++ b/t/lib-unique-files.sh
> > @@ -0,0 +1,34 @@
> > +# Helper to create files with unique contents
> > +
> > +test_create_unique_files_base__=$(date -u)
> > +test_create_unique_files_counter__=0
> > +
> > +# Create multiple files with unique contents. Takes the number of
> > +# directories, the number of files in each directory, and the base
> > +# directory.
> > +#
> > +# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files
> > +#                                each in the specified directory, all
> > +#                                with unique contents.
> > +
> > +test_create_unique_files() {
> > +     test "$#" -ne 3 && BUG "3 param"
> > +
> > +     local dirs=$1
> > +     local files=$2
> > +     local basedir=$3
> > +
> > +     rm -rf $basedir >/dev/null
>
> Why the >/dev/null? It's not a "-rfv", and any errors would go to
> stderr.

Will fix. Clearly I don't know UNIX very well.

>
> > +             mkdir -p "$dir" > /dev/null
>
> Ditto.

Will fix.

>
> > +             for j in $(test_seq $files)
> > +             do
> > +                     test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
> > +                     echo "$test_create_unique_files_base__.$test_create_unique_files_counter__"  >"$dir/file$j.txt"
>
> Would be much more readable if we these variables were shorter.
>
> But actually, why are we trying to create files as a function of "date
> -u" at all? This is all in the trash directory, which is rm -rf'd beween
> runs, why aren't names created with test_seq or whatever OK? I.e. just
> 1.txt, 2.txt....
>

The uniqueness is in the contents of the file.  I wanted to make sure that
we are really creating new objects and not reusing old ones.  Is the scope
of the "trash repo" small enough that I can be guaranteed that a new one
is created before my test since the last time I tried adding something to
the ODB?

> > +test_expect_success 'stash with core.fsyncobjectfiles=batch' "
> > +     test_create_unique_files 2 4 fsync-files &&
> > +     git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ &&
> > +     rm -f fsynced_files &&
> > +
> > +     # The files were untracked, so use the third parent,
> > +     # which contains the untracked files
> > +     git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
> > +     test_line_count = 8 fsynced_files &&
> > +     cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
> > +"
> > +
> > +
> >  test_expect_success 'stash -c stash.useBuiltin=false warning ' '
> >       expected="stash.useBuiltin support has been removed" &&
>
> We really prefer our tests to create the same data each time if
> possible, but as noted with the "date -u" comment above you're
> explicitly bypassing that, but I still can't see why...

I'm trying to make sure we get new object contents. Is there a better
way to achieve what I want without the risk of finding that the contents
are already in the database from a previous test run?

Thanks again for the thorough review,
-Neeraj