[PATCH 0/2] Cleanup refs API [WAS: Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API]

2018-07-30 Thread Stefan Beller 
> > I anticipate that we need to have a lot of back pointers to the repository
> > in question, hence I think we should have the repository pointer promoted
> > to not just a back pointer.
>
> I will probably need more time to study that commit and maybe the mail
> archive for the history of this series. But if I remember correctly
> some of these for_each_ api is quite a pain (perhaps it's the for_each
> version of reflog?) and it's probably better to redesign it (again
> talking without real understanding of the problem).

I stepped back a bit and reconsidered the point made above, and I do not
think that the repository argument is any special. If you need a repository
(for e.g. lookup_commit or friends), you'll have to pass it through the
callback cookie, whether directly or as part of a struct tailored to
your purpose.

Instead we should strive to make the refs API smaller and cleaner,
omitting the repository argument at all, and instead should be focussing
on a ref_store argument instead.

This series applies on master; when we decide to go this direction
we can drop origin/sb/refs-in-repo.

Thanks,
Stefan

Derrick Stolee (1):
  replace-objects: use arbitrary repositories

Stefan Beller (1):
  refs: switch for_each_replace_ref back to use a ref_store

 builtin/replace.c | 4 +---
 refs.c| 4 ++--
 refs.h| 2 +-
 replace-object.c  | 5 +++--
 4 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.18.0.132.g195c49a2227

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

2018-07-27 Thread Duy Nguyen 
On Fri, Jul 27, 2018 at 7:31 PM Stefan Beller  wrote:
>
> On Fri, Jul 27, 2018 at 10:19 AM Brandon Williams  wrote:
> >
> > On 07/27, Duy Nguyen wrote:
> > > On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller  wrote:
> > > >
> > > > Currently the refs API takes a 'ref_store' as an argument to specify
> > > > which ref store to iterate over; however it is more useful to specify
> > > > the repository instead (or later a specific worktree of a repository).
> > >
> > > There is no 'later'. worktrees.c already passes a worktree specific
> > > ref store. If you make this move you have to also design a way to give
> > > a specific ref store now.
> > >
> > > Frankly I still dislike the decision to pass repo everywhere,
> > > especially when refs code already has a nice ref-store abstraction.
> > > Some people frown upon back pointers. But I think adding a back
> > > pointer in ref-store, pointing back to the repository is the right
> > > move.
> >
> > I don't quite understand why the refs code would need a whole repository
> > and not just the ref-store it self.  I thought the refs code was self
> > contained enough that all its state was based on the passed in
> > ref-store.  If its not, then we've done a terrible job at avoiding
> > layering violations (well actually we're really really bad at this in
> > general, and I *think* we're trying to make this better though the
> > object store/index refactoring).
> >
> > If anything I would expect that the actual ref-store code would remain
> > untouched by any refactoring and that instead the higher-level API that
> > hasn't already been converted to explicitly use a ref-store (and instead
> > just calls the underlying impl with get_main_ref_store()).  Am I missing
> > something here?
>
> Then I think we might want to go with the original in Stolees proposal
> https://github.com/gitgitgadget/git/pull/11/commits/300db80140dacc927db0d46c804ca0ef4dcc1be1
> but there the call to for_each_replace_ref just looks ugly, as it takes the
> repository as both the repository where to obtain the ref store from
> as well as the back pointer.
>
> I anticipate that we need to have a lot of back pointers to the repository
> in question, hence I think we should have the repository pointer promoted
> to not just a back pointer.

I will probably need more time to study that commit and maybe the mail
archive for the history of this series. But if I remember correctly
some of these for_each_ api is quite a pain (perhaps it's the for_each
version of reflog?) and it's probably better to redesign it (again
talking without real understanding of the problem).
-- 
Duy

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

2018-07-27 Thread Stefan Beller 
On Fri, Jul 27, 2018 at 10:19 AM Brandon Williams  wrote:
>
> On 07/27, Duy Nguyen wrote:
> > On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller  wrote:
> > >
> > > Currently the refs API takes a 'ref_store' as an argument to specify
> > > which ref store to iterate over; however it is more useful to specify
> > > the repository instead (or later a specific worktree of a repository).
> >
> > There is no 'later'. worktrees.c already passes a worktree specific
> > ref store. If you make this move you have to also design a way to give
> > a specific ref store now.
> >
> > Frankly I still dislike the decision to pass repo everywhere,
> > especially when refs code already has a nice ref-store abstraction.
> > Some people frown upon back pointers. But I think adding a back
> > pointer in ref-store, pointing back to the repository is the right
> > move.
>
> I don't quite understand why the refs code would need a whole repository
> and not just the ref-store it self.  I thought the refs code was self
> contained enough that all its state was based on the passed in
> ref-store.  If its not, then we've done a terrible job at avoiding
> layering violations (well actually we're really really bad at this in
> general, and I *think* we're trying to make this better though the
> object store/index refactoring).
>
> If anything I would expect that the actual ref-store code would remain
> untouched by any refactoring and that instead the higher-level API that
> hasn't already been converted to explicitly use a ref-store (and instead
> just calls the underlying impl with get_main_ref_store()).  Am I missing
> something here?

Then I think we might want to go with the original in Stolees proposal
https://github.com/gitgitgadget/git/pull/11/commits/300db80140dacc927db0d46c804ca0ef4dcc1be1
but there the call to for_each_replace_ref just looks ugly, as it takes the
repository as both the repository where to obtain the ref store from
as well as the back pointer.

I anticipate that we need to have a lot of back pointers to the repository
in question, hence I think we should have the repository pointer promoted
to not just a back pointer.

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

2018-07-27 Thread Brandon Williams 
On 07/27, Duy Nguyen wrote:
> On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller  wrote:
> >
> > Currently the refs API takes a 'ref_store' as an argument to specify
> > which ref store to iterate over; however it is more useful to specify
> > the repository instead (or later a specific worktree of a repository).
> 
> There is no 'later'. worktrees.c already passes a worktree specific
> ref store. If you make this move you have to also design a way to give
> a specific ref store now.
> 
> Frankly I still dislike the decision to pass repo everywhere,
> especially when refs code already has a nice ref-store abstraction.
> Some people frown upon back pointers. But I think adding a back
> pointer in ref-store, pointing back to the repository is the right
> move.

I don't quite understand why the refs code would need a whole repository
and not just the ref-store it self.  I thought the refs code was self
contained enough that all its state was based on the passed in
ref-store.  If its not, then we've done a terrible job at avoiding
layering violations (well actually we're really really bad at this in
general, and I *think* we're trying to make this better though the
object store/index refactoring).

If anything I would expect that the actual ref-store code would remain
untouched by any refactoring and that instead the higher-level API that
hasn't already been converted to explicitly use a ref-store (and instead
just calls the underlying impl with get_main_ref_store()).  Am I missing
something here?

-- 
Brandon Williams

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

2018-07-27 Thread Duy Nguyen 
On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller  wrote:
>
> Currently the refs API takes a 'ref_store' as an argument to specify
> which ref store to iterate over; however it is more useful to specify
> the repository instead (or later a specific worktree of a repository).

There is no 'later'. worktrees.c already passes a worktree specific
ref store. If you make this move you have to also design a way to give
a specific ref store now.

Frankly I still dislike the decision to pass repo everywhere,
especially when refs code already has a nice ref-store abstraction.
Some people frown upon back pointers. But I think adding a back
pointer in ref-store, pointing back to the repository is the right
move.
-- 
Duy

[PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

2018-07-26 Thread Stefan Beller 
Currently the refs API takes a 'ref_store' as an argument to specify
which ref store to iterate over; however it is more useful to specify
the repository instead (or later a specific worktree of a repository).

Introduce a new API, that takes a repository struct instead of a ref store;
the repository argument is also passed through to the callback, which is
of type 'each_repo_ref_fn' that is introduced in a previous patch and is
an extension of the 'each_ref_fn' type with the additional repository
argument.

We wrap the old API as in a very shallow way around the new API,
by wrapping the callback and the callback data into a new callback
to translate between the 'each_ref_fn' and 'each_repo_ref_fn' type.

The wrapping implementation could be done either in refs.c or as presented
in this patch as a 'static inline' in the header file itself. This has the
advantage that the line of the old API is changed (and not just its
implementation in refs.c), such that it will show up in git-blame.

The new API is not perfect yet, as some of them take both a 'repository'
and 'ref_store' argument. This is done for an easy migration:
If the ref_store argument is non-NULL, prefer it over the repository
to compute which refs to iterate over. That way we can ensure that this
step of API migration doesn't confuse which ref store to work on.

Once all callers have migrated to this newly introduced API, we can
get rid of the old API; a second migration step in the future will remove
the then useless ref_store argument

Signed-off-by: Stefan Beller 
---
 refs.c | 158 +++---
 refs.h | 352 +++--
 2 files changed, 407 insertions(+), 103 deletions(-)

diff --git a/refs.c b/refs.c
index 2513f77acb3..27e3772fca9 100644
--- a/refs.c
+++ b/refs.c
@@ -217,7 +217,7 @@ char *resolve_refdup(const char *refname, int resolve_flags,
 /* The argument to filter_refs */
 struct ref_filter {
const char *pattern;
-   each_ref_fn *fn;
+   each_repo_ref_fn *fn;
void *cb_data;
 };
 
@@ -289,14 +289,15 @@ int ref_filter_match(const char *refname,
return 1;
 }
 
-static int filter_refs(const char *refname, const struct object_id *oid,
-  int flags, void *data)
+static int filter_refs(struct repository *r,
+  const char *refname, const struct object_id *oid,
+  int flags, void *data)
 {
struct ref_filter *filter = (struct ref_filter *)data;
 
if (wildmatch(filter->pattern, refname, 0))
return 0;
-   return filter->fn(refname, oid, flags, filter->cb_data);
+   return filter->fn(r, refname, oid, flags, filter->cb_data);
 }
 
 enum peel_status peel_object(const struct object_id *name, struct object_id 
*oid)
@@ -371,46 +372,50 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, 
const struct string_li
for_each_rawref(warn_if_dangling_symref, );
 }
 
-int refs_for_each_tag_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+int refs_for_each_tag_repo_ref(struct repository *r, each_repo_ref_fn fn, void 
*cb_data)
 {
-   return refs_for_each_ref_in(refs, "refs/tags/", fn, cb_data);
+   return refs_for_each_repo_ref_in(r, NULL, "refs/tags/", fn, cb_data);
 }
 
-int for_each_tag_ref(each_ref_fn fn, void *cb_data)
+int for_each_tag_repo_ref(struct repository *r, each_repo_ref_fn fn, void 
*cb_data)
 {
-   return refs_for_each_tag_ref(get_main_ref_store(the_repository), fn, 
cb_data);
+   return refs_for_each_tag_repo_ref(r, fn, cb_data);
 }
 
-int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+int refs_for_each_branch_repo_ref(struct repository *r, struct ref_store *refs,
+ each_repo_ref_fn fn, void *cb_data)
 {
-   return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data);
+   return refs_for_each_repo_ref_in(r, refs, "refs/heads/", fn, cb_data);
 }
 
-int for_each_branch_ref(each_ref_fn fn, void *cb_data)
+int for_each_branch_repo_ref(struct repository *r, each_repo_ref_fn fn, void 
*cb_data)
 {
-   return refs_for_each_branch_ref(get_main_ref_store(the_repository), fn, 
cb_data);
+   return refs_for_each_branch_repo_ref(r, NULL, fn, cb_data);
 }
 
-int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+int refs_for_each_remote_repo_ref(struct repository *r, struct ref_store *refs,
+ each_repo_ref_fn fn, void *cb_data)
 {
-   return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
+   return refs_for_each_repo_ref_in(r, refs, "refs/remotes/", fn, cb_data);
 }
 
-int for_each_remote_ref(each_ref_fn fn, void *cb_data)
+int for_each_remote_repo_ref(struct repository *r, each_repo_ref_fn fn, void 
*cb_data)
 {
-   return refs_for_each_remote_ref(get_main_ref_store(the_repository), fn, 
cb_data);
+   return