From: Michael Haggerty <mhagger@alum.mit.edu>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
Stefan Beller <sbeller@google.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Ramsay Jones <ramsay@ramsayjones.plus.com>
Subject: Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog
Date: Sat, 22 Apr 2017 10:05:02 +0200 [thread overview]
Message-ID: <d3428e5e-9ac2-0426-31fd-92d29a414b3d@alum.mit.edu> (raw)
In-Reply-To: <20170419110145.5086-11-pclouds@gmail.com>
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@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
next prev parent reply other threads:[~2017-04-22 8:05 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-19 11:01 [PATCH v3 00/12] Fix git-gc losing objects in multi worktree Nguyễn Thái Ngọc Duy
2017-04-19 11:01 ` [PATCH v3 01/12] revision.h: new flag in struct rev_info wrt. worktree-related refs Nguyễn Thái Ngọc Duy
2017-04-19 11:01 ` [PATCH v3 02/12] revision.c: refactor add_index_objects_to_pending() Nguyễn Thái Ngọc Duy
2017-04-19 11:01 ` [PATCH v3 03/12] revision.c: --indexed-objects add objects from all worktrees Nguyễn Thái Ngọc Duy
2017-04-19 11:01 ` [PATCH v3 04/12] refs.c: refactor get_submodule_ref_store(), share common free block Nguyễn Thái Ngọc Duy
2017-04-22 5:13 ` Michael Haggerty
2017-04-19 11:01 ` [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store Nguyễn Thái Ngọc Duy
2017-04-19 22:02 ` Johannes Sixt
2017-04-20 2:01 ` Duy Nguyen
2017-04-20 11:56 ` Duy Nguyen
2017-04-20 16:30 ` Johannes Sixt
2017-04-22 5:27 ` Michael Haggerty
2017-04-24 18:12 ` Stefan Beller
2017-04-19 11:01 ` [PATCH v3 06/12] refs: add refs_head_ref() Nguyễn Thái Ngọc Duy
2017-04-22 6:37 ` Michael Haggerty
2017-04-22 8:15 ` Duy Nguyen
2017-04-19 11:01 ` [PATCH v3 07/12] revision.c: use refs_for_each*() instead of for_each_*_submodule() Nguyễn Thái Ngọc Duy
2017-04-19 11:01 ` [PATCH v3 08/12] refs: remove dead for_each_*_submodule() Nguyễn Thái Ngọc Duy
2017-04-19 16:07 ` Ramsay Jones
2017-04-20 3:08 ` Junio C Hamano
2017-04-22 5:35 ` Michael Haggerty
2017-04-19 11:01 ` [PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees Nguyễn Thái Ngọc Duy
2017-04-22 5:48 ` Michael Haggerty
2017-04-19 11:01 ` [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog Nguyễn Thái Ngọc Duy
2017-04-22 8:05 ` Michael Haggerty [this message]
2017-04-23 4:44 ` Duy Nguyen
2017-05-17 13:59 ` Michael Haggerty
2017-04-19 11:01 ` [PATCH v3 11/12] revision.c: --reflog add HEAD reflog from all worktrees Nguyễn Thái Ngọc Duy
2017-04-19 11:01 ` [PATCH v3 12/12] rev-list: expose and document --single-worktree Nguyễn Thái Ngọc Duy
2017-04-22 8:14 ` [PATCH v3 00/12] Fix git-gc losing objects in multi worktree Michael Haggerty
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d3428e5e-9ac2-0426-31fd-92d29a414b3d@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=ramsay@ramsayjones.plus.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).