On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> 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@xxxxxxxxx>
> ---
> 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