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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 23, 2021, 09:50 UTC
Message-ID: <871r6kh3m8.fsf@evledraar.gmail.com>
In-Reply-To: <3d57f7c443082fd2a7f01aee003a9cd3ca2dd910.1629207607.git.gitgitgadget@gmail.com>

On Tue, Aug 17 2021, Han-Wen Nienhuys via GitGitGadget wrote:

Show 15 quoted lines

> [...]
> +		if (len == cap) {
> +			cap = 2 * cap + 1;
> +			logs = realloc(logs, cap * sizeof(*logs));
> +		}
> +
> +		logs[len++] = log;
> [...]
> +		if (logs_len >= logs_cap) {
> +			int new_cap = logs_cap * 2 + 1;
> +			logs = realloc(logs, new_cap * sizeof(*logs));
> +			logs_cap = new_cap;
> +		}
> +		logs[logs_len++] = log;
> +	}

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?

Show 7 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/

Show 6 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?

> [...]
> +git_init () {
> +	git init -b primary "$@"
> +}

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?

> +initialize ()  {
> +	rm -rf .git &&

Should instead set up a test_when_finished "rm -rf .git" ?

> +	(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) &&

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

> +	echo hoi >> file.t &&

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

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

Show 5 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?

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

ditto.

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

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

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

Show 15 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

> [...]
> +if test_have_prereq !REFFILES
> +then
> +  skip_all='skipping pack-refs tests; need files backend'
> +  test_done
> +fi

Indent with spaces?

Show 6 quoted lines

> +if test_have_prereq !REFFILES
> +then
> +  skip_all='skipping tests; incompatible with reftable'
> +  test_done
> +fi
> +

Ditto.

(In general for issues noted above, I saw many more of some of them, including but not limited to this space formatting issue, but elided the patch after the first occurrences).