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

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
[off-list ref] wrote:

quoted

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 <redacted>

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:

quoted

@@ -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