Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
Duy Nguyen writes: > Long term, I think that's what Mike wants. Though the filter_ref_store > might return an (opaque) iterator instead of a ref store and > for_each_refstore() becomes a thin wrapper around the iterator > interface. Ah, yes, an iterator would be a lot more suitable abstraction there. Thanks.
Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
On Wed, Feb 15, 2017 at 1:24 AM, Junio C Hamano wrote:
> Duy Nguyen writes:
>
>> Direct call sites are just middle men though, e.g.
>> for_each_ref_submodule(). I'll attempt to clean this up at some point
>> in future (probably at the same time I attempt to kill *_submodule ref
>> api). But I think for now I'll just put a TODO or FIXME comment here.
>
> So we'd have get_*_ref_store() for various cases and then will have
> a unifying for_each_ref_in_refstore() that takes a ref-store, which
> supersedes all the ad-hoc iterators like for_each_ref_submodule(),
> for_each_namespaced_ref(), etc?
>
> That's a very sensible longer-term goal, methinks.
Ah I forgot about ref namespace. I'll need to try something out in that area.
> I am wondering what we should do to for_each_{tag,branch,...}_ref().
> Would that become
>
> ref_store = get_ref_main_store();
> tag_ref_store = filter_ref_store(ref_store, "refs/tags/");
> for_each_ref_in_refstore(tag_ref_store, ...);
>
> or do you plan to have some other pattern?
Long term, I think that's what Mike wants. Though the filter_ref_store
might return an (opaque) iterator instead of a ref store and
for_each_refstore() becomes a thin wrapper around the iterator
interface.
--
Duy
Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
On Tue, Feb 14, 2017 at 2:04 AM, Duy Nguyen wrote: > On Tue, Feb 14, 2017 at 6:55 AM, Stefan Beller wrote: >>> >>> +/* >>> + * Return the ref_store instance for the specified submodule. For the >>> + * main repository, use submodule==NULL; such a call cannot fail. >> >> So now we have both a get_main as well as a get_submodule function, >> but the submodule function can return the main as well? >> >> I'd rather see this as a BUG; or asking another way: >> What is the difference between get_submodule_ref_store(NULL) >> and get_main_ref_store() ? > > Technical debts :) In order to do that, we need to make sure > _submodule() never takes NULL as main repo. But current code does. On > example is revision.c where submodule==NULL is the main repo. In the > end, I agree that get_submodule_ref_store(NULL) should be a bug. > >> As you went through all call sites (by renaming the function), we'd >> be able to tell that there is no caller with NULL, or is it? > > Direct call sites are just middle men though, e.g. > for_each_ref_submodule(). I'll attempt to clean this up at some point > in future (probably at the same time I attempt to kill *_submodule ref > api). But I think for now I'll just put a TODO or FIXME comment here. ok, fine with me. My line of thinking when originally responding was to put the FIXME comment at the caller site, and there we'd differentiate between the two cases and call the appropriate function. > -- > Duy
Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
Duy Nguyen writes:
> Direct call sites are just middle men though, e.g.
> for_each_ref_submodule(). I'll attempt to clean this up at some point
> in future (probably at the same time I attempt to kill *_submodule ref
> api). But I think for now I'll just put a TODO or FIXME comment here.
So we'd have get_*_ref_store() for various cases and then will have
a unifying for_each_ref_in_refstore() that takes a ref-store, which
supersedes all the ad-hoc iterators like for_each_ref_submodule(),
for_each_namespaced_ref(), etc?
That's a very sensible longer-term goal, methinks.
I am wondering what we should do to for_each_{tag,branch,...}_ref().
Would that become
ref_store = get_ref_main_store();
tag_ref_store = filter_ref_store(ref_store, "refs/tags/");
for_each_ref_in_refstore(tag_ref_store, ...);
or do you plan to have some other pattern?
Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
On Tue, Feb 14, 2017 at 6:55 AM, Stefan Beller wrote: >> >> +/* >> + * Return the ref_store instance for the specified submodule. For the >> + * main repository, use submodule==NULL; such a call cannot fail. > > So now we have both a get_main as well as a get_submodule function, > but the submodule function can return the main as well? > > I'd rather see this as a BUG; or asking another way: > What is the difference between get_submodule_ref_store(NULL) > and get_main_ref_store() ? Technical debts :) In order to do that, we need to make sure _submodule() never takes NULL as main repo. But current code does. On example is revision.c where submodule==NULL is the main repo. In the end, I agree that get_submodule_ref_store(NULL) should be a bug. > As you went through all call sites (by renaming the function), we'd > be able to tell that there is no caller with NULL, or is it? Direct call sites are just middle men though, e.g. for_each_ref_submodule(). I'll attempt to clean this up at some point in future (probably at the same time I attempt to kill *_submodule ref api). But I think for now I'll just put a TODO or FIXME comment here. -- Duy
Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
> > +/* > + * Return the ref_store instance for the specified submodule. For the > + * main repository, use submodule==NULL; such a call cannot fail. So now we have both a get_main as well as a get_submodule function, but the submodule function can return the main as well? I'd rather see this as a BUG; or asking another way: What is the difference between get_submodule_ref_store(NULL) and get_main_ref_store() ? As you went through all call sites (by renaming the function), we'd be able to tell that there is no caller with NULL, or is it? Stefan > For > + * a submodule, the submodule must exist and be a nonbare repository, > + * otherwise return NULL. If the requested reference store has not yet > + * been initialized, initialize it first. > + * > + * For backwards compatibility, submodule=="" is treated the same as > + * submodule==NULL. > + */ > +struct ref_store *get_submodule_ref_store(const char *submodule); > +struct ref_store *get_main_ref_store(void);
[PATCH 11/11] refs: split and make get_*_ref_store() public API
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 59 ++--
refs.h | 13
refs/files-backend.c | 2 +-
refs/refs-internal.h | 12 ---
4 files changed, 48 insertions(+), 38 deletions(-)
diff --git a/refs.c b/refs.c
index 9ac194945..48350da87 100644
--- a/refs.c
+++ b/refs.c
@@ -1160,7 +1160,7 @@ int head_ref(each_ref_fn fn, void *cb_data)
static int do_for_each_ref(const char *submodule, const char *prefix,
each_ref_fn fn, int trim, int flags, void *cb_data)
{
- struct ref_store *refs = get_ref_store(submodule);
+ struct ref_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
if (!refs)
@@ -1304,7 +1304,7 @@ const char *resolve_ref_recursively(struct ref_store
*refs,
/* backend functions */
int refs_init_db(struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->init_db(refs, err);
}
@@ -1312,7 +1312,7 @@ int refs_init_db(struct strbuf *err)
const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
unsigned char *sha1, int *flags)
{
- return resolve_ref_recursively(get_ref_store(NULL), refname,
+ return resolve_ref_recursively(get_main_ref_store(), refname,
resolve_flags, sha1, flags);
}
@@ -1333,10 +1333,10 @@ int resolve_gitlink_ref(const char *submodule, const
char *refname,
/* We need to strip off one or more trailing slashes */
char *stripped = xmemdupz(submodule, len);
- refs = get_ref_store(stripped);
+ refs = get_submodule_ref_store(stripped);
free(stripped);
} else {
- refs = get_ref_store(submodule);
+ refs = get_submodule_ref_store(submodule);
}
if (!refs)
@@ -1491,15 +1491,24 @@ static struct ref_store *init_submodule_ref_store(const
char *path)
return refs;
}
-struct ref_store *get_ref_store(const char *submodule)
+struct ref_store *get_main_ref_store(void)
{
struct ref_store *refs;
- if (!submodule || !*submodule) {
- refs = lookup_ref_store(NULL);
+ refs = lookup_ref_store(NULL);
- if (!refs)
- refs = ref_store_init(NULL, NULL);
+ if (!refs)
+ refs = ref_store_init(NULL, NULL);
+
+ return refs;
+}
+
+struct ref_store *get_submodule_ref_store(const char *submodule)
+{
+ struct ref_store *refs;
+
+ if (!submodule || !*submodule) {
+ return get_main_ref_store();
} else {
refs = lookup_ref_store(submodule);
@@ -1519,14 +1528,14 @@ void base_ref_store_init(struct ref_store *refs,
/* backend functions */
int pack_refs(unsigned int flags)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->pack_refs(refs, flags);
}
int peel_ref(const char *refname, unsigned char *sha1)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->peel_ref(refs, refname, sha1);
}
@@ -1534,7 +1543,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
int create_symref(const char *ref_target, const char *refs_heads_master,
const char *logmsg)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->create_symref(refs, ref_target, refs_heads_master,
logmsg);
@@ -1543,7 +1552,7 @@ int create_symref(const char *ref_target, const char
*refs_heads_master,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->transaction_commit(refs, transaction, err);
}
@@ -1553,14 +1562,14 @@ int verify_refname_available(const char *refname,
const struct string_list *skip,
struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->verify_refname_available(refs, refname, extra, skip,
err);
}
int for_each_reflog(each_ref_fn fn, void *cb_data)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
struct ref_iterator *iter;
iter = refs->be->reflog_iterator_begin(refs);
@@ -1571,7 +1580,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
