Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 22, 2017, 08:05 UTC
Message-ID: <d3428e5e-9ac2-0426-31fd-92d29a414b3d@alum.mit.edu>
In-Reply-To: <20170419110145.5086-11-pclouds@gmail.com>

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:

Show 79 quoted lines

> refs/bisect is unfortunately per-worktree, so we need to look in
> per-worktree logs/refs/bisect in addition to per-repo logs/refs. The
> current iterator only goes through per-repo logs/refs.
> 
> Ideally we should have something like merge_ref_iterator_begin (and
> maybe with a predicate), but for dir_iterator. Since there's only one
> use case for this pattern, let's not add a bunch more code for
> merge_dir_iterator_begin just yet.
> 
> PS. Note the unsorted order of for_each_reflog in the test. This is
> supposed to be OK, for now. If we enforce order on for_each_reflog()
> then some more work will be required.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs/files-backend.c          | 46 ++++++++++++++++++++++++++++++++-----------
>  t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 11 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4149943a6e..fce380679c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1171,15 +1171,6 @@ static void files_reflog_path(struct files_ref_store *refs,
>  			      struct strbuf *sb,
>  			      const char *refname)
>  {
> -	if (!refname) {
> -		/*
> -		 * FIXME: of course this is wrong in multi worktree
> -		 * setting. To be fixed real soon.
> -		 */
> -		strbuf_addf(sb, "%s/logs", refs->gitcommondir);
> -		return;
> -	}
> -
>  	switch (ref_type(refname)) {
>  	case REF_TYPE_PER_WORKTREE:
>  	case REF_TYPE_PSEUDOREF:
> @@ -3368,6 +3359,7 @@ struct files_reflog_iterator {
>  
>  	struct ref_store *ref_store;
>  	struct dir_iterator *dir_iterator;
> +	struct dir_iterator *worktree_dir_iterator;
>  	struct object_id oid;
>  };
>  
> @@ -3388,6 +3380,21 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
>  		if (ends_with(diter->basename, ".lock"))
>  			continue;
>  
> +		if (iter->worktree_dir_iterator) {
> +			const char *refname = diter->relative_path;
> +
> +			switch (ref_type(refname)) {
> +			case REF_TYPE_PER_WORKTREE:
> +			case REF_TYPE_PSEUDOREF:
> +				continue;
> +			case REF_TYPE_NORMAL:
> +				break;
> +			default:
> +				die("BUG: unknown ref type %d of ref %s",
> +				    ref_type(refname), refname);
> +			}
> +		}
> +
>  		if (refs_read_ref_full(iter->ref_store,
>  				       diter->relative_path, 0,
>  				       iter->oid.hash, &flags)) {
> @@ -3401,7 +3408,11 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
>  		return ITER_OK;
>  	}
>  
> -	iter->dir_iterator = NULL;
> +	iter->dir_iterator = iter->worktree_dir_iterator;
> +	if (iter->worktree_dir_iterator) {
> +		iter->worktree_dir_iterator = NULL;
> +		return files_reflog_iterator_advance(ref_iterator);
> +	}

I find this implementation confusing:

* `if (iter->worktree_dir_iterator)` sounds like it should mean
  that we are iterating over worktree references but it really means
  that we are iterating over the common references in a repository
  that is a linked worktree.
* `files_reflog_iterator_advance()` is called recursively, but only
  for the first worktree reference.
* `iter->worktree_dir_iterator` is moved over to `iter->dir_iterator`
  when the common refs are exhausted.

Do you find it more readable as follows?:

static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
{
	struct files_reflog_iterator *iter =
		(struct files_reflog_iterator *)ref_iterator;
	int ok;

	while (1) {
		struct dir_iterator **diter;
		int normal_only, flags;

		if (iter->dir_iterator) {
			diter = &iter->dir_iterator;
			/*
			 * If we are in a worktree, then we only
			 * include "normal" common references:
			 */
			normal_only = !!iter->worktree_dir_iterator;
		} else if (iter->worktree_dir_iterator) {
			diter = &iter->worktree_dir_iterator;
			normal_only = 0;
		} else {
			ok = ITER_DONE;
			break;
		}

		ok = dir_iterator_advance(*diter);
		if (ok == ITER_ERROR) {
			*diter = NULL;
			break;
		} else if (ok == ITER_DONE) {
			*diter = NULL;
			/* There might still be worktree refs left: */
			continue;
		}

		if (!S_ISREG((*diter)->st.st_mode))
			continue;
		if ((*diter)->basename[0] == '.')
			continue;
		if (ends_with((*diter)->basename, ".lock"))
			continue;

		iter->base.refname = (*diter)->relative_path;

		if (normal_only &&
		    ref_type(iter->base.refname) != REF_TYPE_NORMAL)
			continue;

		if (refs_read_ref_full(iter->ref_store,
				       iter->base.refname, 0,
				       iter->oid.hash, &flags)) {
			error("bad ref for %s", (*diter)->path.buf);
			continue;
		}

		iter->base.oid = &iter->oid;
		iter->base.flags = flags;
		return ITER_OK;
	}

	if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
		return ITER_ERROR;

	return ok;
}

>  	if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
>  		ok = ITER_ERROR;
>  	return ok;
> [...]

Michael