Re: [PATCH v3 20/25] refs: RFC: Reftable support for git-core
- From
Han-Wen Nienhuys <hanwen@google.com>
- Date
- Aug 30, 2021, 13:31 UTC
- Message-ID
- <CAFQ2z_OTdXSvPcjPRi8fx6F_yjwiL3vRpPtnEOTyaeBxMB2wHQ@mail.gmail.com>
- In-Reply-To
- <871r6kh3m8.fsf@evledraar.gmail.com>
On Mon, Aug 23, 2021 at 12:12 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Elsewhere in this series we use the ARRAY_SIZE() macro from > git-compat-util.h, can't we also use REALLOC_ARRAY() from the same > header here?
Done.
Show 11 quoted lines
> > [...]
> > + if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> > + /* XXX - skip writing records that were not changed. */
> > + err = reftable_addition_commit(add);
> > + } else {
> > + /* XXX - print something */
> > + }
>
> Aren't these comments covered by some of the failing tests under
> GIT_TEST_REFTABLE=true? I.e. what I mentioned
> https://lore.kernel.org/git/877dgch4rn.fsf@evledraar.gmail.com/The XXX comments are directed at reviewers. I don't know if these places relate to test failures.
Show 13 quoted lines
> > [...]
> > + if (err < 0) {
> > + errno = reftable_error_to_errno(err);
> > + err = -1;
> > + goto done;
> > + }
>
> In your proposed fixup for the merger of our topics in
> https://lore.kernel.org/git/pull.1054.v3.git.git.1629207607.gitgitgadget@gmail.com/
> you have the call to reftable_error_to_errno() here deleted, so isn't
> this also redundant at this point (and then the
> reftable_error_to_errno() function can be deleted), or is this errno
> setting still needed with some of my changes?No; it's purely informational, but the errno codes aren't a good match, so OK to drop as well.
> Can't later tests just use "main" instead of primary with a > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main, then we can drop this > "git_init" wrapper?
Done.
> > +initialize () {
> > + rm -rf .git &&
>
> Should instead set up a test_when_finished "rm -rf .git" ?No. The test framework sets up a scratch repo, but the scratch repo doesn't use reftable, so it interferes with the tests.
Show 7 quoted lines
> > + (GIT_TEST_REFTABLE=1; export GIT_TEST_REFTABLE; git_init) && > > This export before calling git_init can surely go away if git_init goes, > but alo why export beforehand here, but in later tests in this file we > just do a plain: > > (GIT_TEST_REFTABLE=1 git_init) &&
Done.
Show 6 quoted lines
> > + mv .git/hooks .git/hooks-disabled > > Is this "mv" cargo-culted from what test_create_repo() used to do before > my-f0d4d398e28 (test-lib: split up and deprecate test_create_repo(), > 2021-05-10)? In any case templated hooks are disabled by default (named > *.sample), so I don't think this is needed.
Done.
> > + echo hoi >> file.t && > > Nit: >>file.t, not ">> file.t".
Dnoe.
Show 5 quoted lines
> > + git show-ref | sed s/before/after/g > expected && > > Don't have "git" on the LHS of a pipe, it'll hide a segfault. Should use > a temporary file. Also "s/ > />/g" like above (and in some places below, > will stop noting it...).
Done.
Show 13 quoted lines
> > +test_expect_success 'SHA256 support, env' ' > > + rm -rf .git && > > + GIT_DEFAULT_HASH=sha256 && export GIT_DEFAULT_HASH && > > + (GIT_TEST_REFTABLE=1 git_init) && > > + mv .git/hooks .git/hooks-disabled && > > Comments about this .git/hooks-disabled not being needed aside, this > seems to be duplicating the initialize() function. I.e. should we not > skip the "rm -rf" and "mv" here, and just set GIT_DEFAULT_HASH=sha256 > and call initialize? > > (Better yet, if we get rid of that "git init" wrapper as I noted above, > this can just be an argument to "git init", no?
Done.
Show 7 quoted lines
> > > [...] > > + rm -rf .git && > > + (GIT_TEST_REFTABLE=1 git_init --object-format=sha256) && > > + mv .git/hooks .git/hooks-disabled && > > ditto.
done.
> > + ! git update-ref -d refs/tags/file $INVALID_SHA1 && > > Always "test_must_fail git", not "! git".
done.
Show 42 quoted lines
>
> > [...]
> > +test_expect_success 'clone calls transaction_initial_commit' '
> > + test_commit message1 file1 &&
> > + git clone . cloned &&
> > + (test -f cloned/file1 || echo "Fixme.")
>
> So this test really tests nothing much, and we should skip the "Fixme"
> here and have this be test_expect_failure() or something instead?
>
> > [...]
> > + git show-ref | cut -f2 -d" " >actual &&
>
> Git on LHS of a pipe again.
>
> > +# This matches show-ref's output
> > +print_ref() {
> > + echo "$(git rev-parse "$1") $1"
> > +}
> > +
> > +test_expect_success 'peeled tags are stored' '
> > + initialize &&
> > + test_commit file &&
> > + git tag -m "annotated tag" test_tag HEAD &&
> > + {
> > + print_ref "refs/heads/primary" &&
> > + print_ref "refs/tags/file" &&
> > + print_ref "refs/tags/test_tag" &&
> > + print_ref "refs/tags/test_tag^{}"
> > + } >expect &&
>
> Maybe I'm missing something, but wouldn't this print_ref() helper be
> better as a "git for-each-ref --format" of some sort?
>
> > [...]
> > +. "$TEST_DIRECTORY"/lib-httpd.sh
> > +start_httpd
> > +
> > +REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
>
> Let's split these httpd-needing tests into another test file, see
> https://lore.kernel.org/git/87bl753i2p.fsf@evledraar.gmail.com/ for why.done.
-- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado