Re: [PATCH v3 20/25] refs: RFC: Reftable support for git-core

On Mon, Aug 23, 2021 at 12:12 PM Ævar Arnfjörð Bjarmason
[off-list ref] 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.

quoted

[...]
+     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/ (local)

The XXX comments are directed at reviewers. I don't know if these
places relate to test failures.

quoted

[...]
+     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/ (local)
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.

quoted

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

quoted

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

quoted

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

quoted

+     echo hoi >> file.t &&

Nit: >>file.t, not ">> file.t".

Dnoe.

quoted

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

quoted

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

quoted

[...]
+     rm -rf .git &&
+     (GIT_TEST_REFTABLE=1 git_init --object-format=sha256) &&
+     mv .git/hooks .git/hooks-disabled &&

ditto.

done.

quoted

+     ! git update-ref -d refs/tags/file $INVALID_SHA1 &&

Always "test_must_fail git", not "! git".

done.

quoted

[...]
+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?

quoted

[...]
+     git show-ref | cut -f2 -d" " >actual &&

Git on LHS of a pipe again.

quoted

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

quoted

[...]
+. "$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/ (local) 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