Re: [PATCH v3 1/3] refs: keep track of unresolved reference value in iterators

[Junio C Hamano <gitster@pobox.com>]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index aa52d9be7c7..5ed69c23f74 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -245,9 +245,11 @@ static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
>  {
>  	struct object_id oid;
>  	int flag;
> -
> -	if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,

Here, we had a nice blank line that separated the decls and the
first statement.

> -				     &oid, &flag)) {
> +	const char *referent = refs_resolve_ref_unsafe(&refs->base,
> +						       refname,
> +						       RESOLVE_REF_READING,
> +						       &oid, &flag);
> +	if (!referent) {

We lost it here, though.

>  		oidclr(&oid, the_repository->hash_algo);
>  		flag |= REF_ISBROKEN;
>  	} else if (is_null_oid(&oid)) {
> @@ -268,7 +270,11 @@ static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
>  		oidclr(&oid, the_repository->hash_algo);
>  		flag |= REF_BAD_NAME | REF_ISBROKEN;
>  	}
> -	add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
> +
> +	if (!(flag & REF_ISSYMREF))
> +		referent = NULL;

OK, this is new in this round.  The idea is that everybody else can
rely on the invariant that the referent being NULL is equivalent to
REF_ISSYMREF bit in flag word being off from here on.

> +	add_entry_to_dir(dir, create_ref_entry(refname, referent, &oid, flag));
>  }
>  
>  /*
> @@ -886,6 +892,11 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)
>  		iter->base.refname = iter->iter0->refname;
>  		iter->base.oid = iter->iter0->oid;
>  		iter->base.flags = iter->iter0->flags;
> +		if (iter->iter0->flags & REF_ISSYMREF)
> +			iter->base.referent = iter->iter0->referent;
> +		else
> +			iter->base.referent = NULL;
>  		return ITER_OK;
>  	}

Hmph, why not an unconditional

		iter->base.referent = iter->iter0->referent;

instead?  This code is making sure (iter->base.flags & REF_ISSYMREF)
is directly linked to non-NULL-ness or iter->base.referent, and we
want to make everybody take it as invariant.  Shouldn't this code
also rely on the same invariant?  If iter-iter0->referent is NULL,
iter->iter0->flag has REF_ISSYMREF bit off, and vice versa.