From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 411BF1F8C8 for ; Wed, 22 Sep 2021 17:56:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236959AbhIVR5g (ORCPT ); Wed, 22 Sep 2021 13:57:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49272 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236837AbhIVR5f (ORCPT ); Wed, 22 Sep 2021 13:57:35 -0400 Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 47CFBC061574 for ; Wed, 22 Sep 2021 10:56:05 -0700 (PDT) Received: by mail-lf1-x131.google.com with SMTP id x27so15217399lfu.5 for ; Wed, 22 Sep 2021 10:56:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=XDKjIvKef5F9MmQ0PKTG+MiOfneXIFYFtTp67YDEp58=; b=cJVn/dhLE50HajeDhkEbcj/v4Ld9IwturM9ETvPS83Vzb2F/qwKEHcUjItxYqi9X1Q z2cYCu+1/pJmoIB/bwlC0X0c+PcllRdFVMNhEV2zdPvJAI2NC6vf1a2+tpGW6Wmh5ShP pkd9JzZFcvYBAFfi8kf5kRutOaWgIIkHhziMdm4c3bGmyFq6mB/jFVLzT9AvNXK83B5B yOFMLrhY3bvbZcnvaZMNWfOku6IfsxAziryCFWdkOvkqZiIPby/6c/VQ6LZpWSAM7gty kyzpl8yaKwIq+w5r8t2CnLduFaeVy+aVEZYb2CBJUA2zs6d6h/AlK/ZmUf7EF2XTUPMZ rMzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=XDKjIvKef5F9MmQ0PKTG+MiOfneXIFYFtTp67YDEp58=; b=5jfRWE5pwI1KneaRlZjgde4RiZCuIDqkWm07qdBTdP1QNmHJcrDaNIZbpFuZCq86xm 6EmLNrphxHTCQfAVcoK+1L0ZkpUPUNEpvv4BTita8qB5h9rKengZGJX1BBj1/pWZSMTR ZSxn0fLcbVt6h3zxS3aXvP5DiRsf3cIaWIw+OU+grKFEKqo4fIgP+np8EY5cuVIlHadb R+3o61mXYbpUgjq6q+66TZjVmkYPyjpsHUYuBJoKfIh2ywLglBlr+CJTsvER+nYUi5Db sI6MvFAuLEw5Wim0vj4qG3uYivd8AS0n4GAozD5Fhk+uGOFQsGJT+rA3yVPI6OrjhvI9 Fqug== X-Gm-Message-State: AOAM533mtZ/AUp/2MkBcv0Vsf1gVJfmVoDWjm2MggF3qsWnMbZ+HSMst o2axZjWVNYw54WM2/4yQTPZQTsBDx4zl3WqYBMM= X-Google-Smtp-Source: ABdhPJxja9Ck14a/lXo9/3IMgsYekZx3N6i/WCjiUCmoUBCeUUGO+3eLwgWrqNK6YI04UrLcceXSco/M1AWmyHP0PiU= X-Received: by 2002:a2e:9296:: with SMTP id d22mr547281ljh.413.1632333363537; Wed, 22 Sep 2021 10:56:03 -0700 (PDT) MIME-Version: 1.0 References: <87a6k58or9.fsf@evledraar.gmail.com> <87wnn974gx.fsf@evledraar.gmail.com> In-Reply-To: <87wnn974gx.fsf@evledraar.gmail.com> From: Neeraj Singh Date: Wed, 22 Sep 2021 10:55:52 -0700 Message-ID: Subject: Re: [PATCH v4 5/6] core.fsyncobjectfiles: tests for batch mode To: =?UTF-8?B?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Cc: Neeraj Singh via GitGitGadget , Git List , Johannes Schindelin , Jeff King , Jeff Hostetler , Christoph Hellwig , "Randall S. Becker" , Bagas Sanjaya , Neeraj Singh Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Sep 21, 2021 at 7:02 PM =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote: > > > On Tue, Sep 21 2021, Neeraj Singh wrote: > > > On Tue, Sep 21, 2021 at 4:58 PM =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason > > wrote: > >> > >> > >> On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote: > >> > >> > From: Neeraj Singh > >> > > >> > 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 > >> > --- > >> > 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__=3D$(date -u) > >> > +test_create_unique_files_counter__=3D0 > >> > + > >> > +# 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 fi= les > >> > +# each in the specified directory, a= ll > >> > +# with unique contents. > >> > + > >> > +test_create_unique_files() { > >> > + test "$#" -ne 3 && BUG "3 param" > >> > + > >> > + local dirs=3D$1 > >> > + local files=3D$2 > >> > + local basedir=3D$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__=3D$((test_c= reate_unique_files_counter__ + 1)) > >> > + echo "$test_create_unique_files_base__.$test_c= reate_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 bewe= en > >> 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 t= hat > > we are really creating new objects and not reusing old ones. Is the sc= ope > > of the "trash repo" small enough that I can be guaranteed that a new on= e > > is created before my test since the last time I tried adding something = to > > the ODB? > > > >> > +test_expect_success 'stash with core.fsyncobjectfiles=3Dbatch' " > >> > + test_create_unique_files 2 4 fsync-files && > >> > + git -c core.fsyncobjectfiles=3Dbatch 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 =3D 8 fsynced_files && > >> > + cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file= -e > >> > +" > >> > + > >> > + > >> > test_expect_success 'stash -c stash.useBuiltin=3Dfalse warning ' ' > >> > expected=3D"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 content= s > > are already in the database from a previous test run? > > You can just do something like: > > test_expect_success 'setup data' ' > test_commit A && > test_commit B > ' > > Which will create files A.t, B.t etc, or create them via: > > obj=3D$(echo foo | git hash-object -w --stdin) > > etc. > > I.e. the uniqueness you're doing here seems to assume that tests are > re-using the same object store across runs, but we create a new trash > directory for each one, if you run the test with "-d" you can see it > being left behind for inspection. This is already ensured for the test. > > The only potential caveat I can imagine is that some filesystem like say > btrfs-like that does some COW or object de-duplication would behave > differently, but other than that... It looks like the same repo is reused for each test_expect_success line in the top-level t*.sh script. So for test_create_unique_files to be maximally useful, it should have some state that is different for each invocation. How about I use the test_tick mechanism to produce this uniqueness? It wouldn't be globally unique like the date method, but it should be good enough if the repo is recycled every time test-lib is reinitialized. I'm changing lib-unique-files to use test_tick and to be a little more readable as you suggested. Please let me know if you have any other suggestions.