Re: [PATCH v6 03/12] fsck: support refs pointing to promisor objects

[Brandon Williams <bmwill@google.com>]

On 12/05, Jeff Hostetler wrote:
> From: Jonathan Tan <jonathantanmy@google.com>
> 
> Teach fsck to not treat refs referring to missing promisor objects as an
> error when extensions.partialclone is set.
> 
> For the purposes of warning about no default refs, such refs are still
> treated as legitimate refs.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/fsck.c           |  8 ++++++++
>  t/t0410-partial-clone.sh | 24 ++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 2934299..ee937bb 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -434,6 +434,14 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,
>  
>  	obj = parse_object(oid);
>  	if (!obj) {
> +		if (is_promisor_object(oid)) {
> +			/*
> +			 * Increment default_refs anyway, because this is a
> +			 * valid ref.
> +			 */
> +			 default_refs++;
> +			 return 0;
> +		}
>  		error("%s: invalid sha1 pointer %s", refname, oid_to_hex(oid));
>  		errors_found |= ERROR_REACHABLE;
>  		/* We'll continue with the rest despite the error.. */
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 3ddb3b9..bf75162 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -13,6 +13,14 @@ pack_as_from_promisor () {
>  	>repo/.git/objects/pack/pack-$HASH.promisor
>  }
>  
> +promise_and_delete () {
> +	HASH=$(git -C repo rev-parse "$1") &&
> +	git -C repo tag -a -m message my_annotated_tag "$HASH" &&
> +	git -C repo rev-parse my_annotated_tag | pack_as_from_promisor &&
> +	git -C repo tag -d my_annotated_tag &&
> +	delete_object repo "$HASH"
> +}
> +
>  test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '
>  	test_create_repo repo &&
>  	test_commit -C repo my_commit &&
> @@ -78,4 +86,20 @@ test_expect_success 'missing reflog object alone fails fsck, even with extension
>  	test_must_fail git -C repo fsck
>  '
>  
> +test_expect_success 'missing ref object, but promised, passes fsck' '
> +	rm -rf repo &&

Instead of requiring that every test first removes 'repo', maybe you
want to have each test do its own cleanup by adding in
'test_when_finished' lines to do the removals?  Just a thought.

> +	test_create_repo repo &&
> +	test_commit -C repo my_commit &&
> +
> +	A=$(git -C repo commit-tree -m a HEAD^{tree}) &&
> +
> +	# Reference $A only from ref
> +	git -C repo branch my_branch "$A" &&
> +	promise_and_delete "$A" &&
> +
> +	git -C repo config core.repositoryformatversion 1 &&
> +	git -C repo config extensions.partialclone "arbitrary string" &&
> +	git -C repo fsck
> +'
> +
>  test_done
> -- 
> 2.9.3
> 

-- 
Brandon Williams