Re: [PATCH v2 02/16] refs: convert for_each_tag_ref to struct object_id

[Jeff King <peff@peff.net>]

On Thu, Apr 23, 2015 at 11:13:32AM -0700, Stefan Beller wrote:

> On Wed, Apr 22, 2015 at 4:24 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > To allow piecemeal conversion of the for_each_*_ref functions, introduce
> > an additional typedef for a callback function that takes struct
> > object_id * instead of unsigned char *.  Provide an extra field in
> > struct ref_entry_cb for this callback and ensure at most one is set at a
> > time.  Temporarily suffix these new entries with _oid to distinguish
> > them.  Convert for_each_tag_ref and its callers to use the new _oid
> > functions, introducing temporary wrapper functions to avoid type
> > mismatches.
> >
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> 
> I am currently running this patch series via
> git rebase -i origin/next --exec=make --exec="make test"
> through the compilation and test suite one by one.
> (My current view of origin/next is (c8da2d582, Sync with 2.4.0-rc3)
> and this commit fails in t5312-prune-corruption.sh test 3 5 and 8

It's because of this hunk:

> > @@ -1950,6 +1956,21 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,
> >         data.trim = trim;
> >         data.flags = flags;
> >         data.fn = fn;
> > +       data.fn_oid = NULL;
> > +       data.cb_data = cb_data;
> > +
> > +       return do_for_each_entry(refs, base, do_one_ref, &data);
> > +}
> > +
> > +static int do_for_each_ref_oid(struct ref_cache *refs, const char *base,
> > +                          each_ref_fn_oid fn, int trim, int flags, void *cb_data)
> > +{
> > +       struct ref_entry_cb data;
> > +       data.base = base;
> > +       data.trim = trim;
> > +       data.flags = flags;
> > +       data.fn = NULL;
> > +       data.fn_oid = fn;
> >         data.cb_data = cb_data;
> >
> >         if (ref_paranoia < 0)

The ref_paranoia code gets pushed down into do_for_each_ref_oid, but it
needs called in both do_for_each_ref variants. This is probably an
artifact of rebasing the patches (the ref_paranoia stuff was added
recently).

I think it would make sense to pull the setup of the data struct into a
shared function rather than duplicate it. But we want to avoid having to
update do_for_each_ref callsites, so we'll have to provide a wrapper.

Like this:

diff --git a/refs.c b/refs.c
index 95863f2..ad39d74 100644
--- a/refs.c
+++ b/refs.c
@@ -1948,29 +1948,16 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base,
  * value, stop the iteration and return that value; otherwise, return
  * 0.
  */
-static int do_for_each_ref(struct ref_cache *refs, const char *base,
-			   each_ref_fn fn, int trim, int flags, void *cb_data)
+static int do_for_each_ref_generic(struct ref_cache *refs, const char *base,
+				   each_ref_fn fn, each_ref_fn_oid fn_oid,
+				   int trim, int flags, void *cb_data)
 {
 	struct ref_entry_cb data;
 	data.base = base;
 	data.trim = trim;
 	data.flags = flags;
 	data.fn = fn;
-	data.fn_oid = NULL;
-	data.cb_data = cb_data;
-
-	return do_for_each_entry(refs, base, do_one_ref, &data);
-}
-
-static int do_for_each_ref_oid(struct ref_cache *refs, const char *base,
-			   each_ref_fn_oid fn, int trim, int flags, void *cb_data)
-{
-	struct ref_entry_cb data;
-	data.base = base;
-	data.trim = trim;
-	data.flags = flags;
-	data.fn = NULL;
-	data.fn_oid = fn;
+	data.fn_oid = fn_oid;
 	data.cb_data = cb_data;
 
 	if (ref_paranoia < 0)
@@ -1981,6 +1968,18 @@ static int do_for_each_ref_oid(struct ref_cache *refs, const char *base,
 	return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 
+static int do_for_each_ref(struct ref_cache *refs, const char *base,
+			   each_ref_fn fn, int trim, int flags, void *cb_data)
+{
+	return do_for_each_ref_generic(refs, base, fn, NULL, trim, flags, cb_data);
+}
+
+static int do_for_each_ref_oid(struct ref_cache *refs, const char *base,
+			       each_ref_fn_oid fn, int trim, int flags, void *cb_data)
+{
+	return do_for_each_ref_generic(refs, base, NULL, fn, trim, flags, cb_data);
+}
+
 static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
 	unsigned char sha1[20];

You can even dispense with the _oid variant wrapper, and just call into
the generic version directly from the new callsites.

-Peff