[RFC 1/2] Portability: Convert strictly between function pointers

[RFC 1/2] Portability: Convert strictly between function pointers

[Michael Witten]

Currently, building git with:

  CFLAGS="-std=c99 -pedantic -Wall -Werror -g -02"

causes gcc 4.5.2 to fail with:

  ISO C forbids initialization between function pointer and 'void *'

The C99 standard isn't very explicit about this fact, but the
following paragraphs seem relevant:

  6.3.2.3.1:
  A pointer to void may be converted to or from a pointer to
  any incomplete or object type. A pointer to any incomplete
  or object type may be converted to a pointer to void and
  back again; the result shall compare equal to the original
  pointer.

  6.3.2.3.8:
  A pointer to a function of one type may be converted to a
  pointer to a function of another type and back again; the
  result shall compare equal to the original pointer. If a
  converted pointer is used to call a function whose type is
  not compatible with the pointed-to type, the behavior is
  undefined.

  6.5.4.3:
  Conversions that involve pointers, other than where
  permitted by the constraints of 6.5.16.1, shall be
  specified by means of an explicit cast.

Also, this website:

  http://www.safercode.com/blog/2008/11/25/generic-function-pointers-in-c-and-void.html

provides some inspiring comments:

  Why can't we use void* for a Generic Function Pointer?
  ------------------------------------------------------

  This is because a void* is a pointer to a generic
  "data" type. A void * is used to denote pointers to
  objects and in some systems, pointers to functions can
  be larger than pointers to objects. So, if you convert
  amongst them, you'll lose information and hence, the
  situation would be undefined and implementation dependent.
  ...

However, that seems like a hypothetical justification for the
implicitly undefined behavior of using a variable of type
pointer to void to carry around a value of type
pointer to function.

In any case, the solution is clear: Rather than converting
to and from a pointer to void, one can instead convert to
and from a pointer to any other function type, say:

  void (*)(void)

Is this silly? Probably, but at least it's standards conforming.

Unfortunately (or fortunately?), this requires more
explicit casting, as per 6.5.4.3.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 builtin/fetch-pack.c   |    2 +-
 builtin/receive-pack.c |    2 +-
 cache.h                |    5 +++--
 sha1_file.c            |    2 +-
 transport.c            |    4 ++--
 transport.h            |    4 ++--
 6 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 1724b76..ded1784 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -225,7 +225,7 @@ static void insert_one_alternate_ref(const struct ref *ref, void *unused)
 
 static void insert_alternate_refs(void)
 {
-	foreach_alt_odb(refs_from_alternate_cb, insert_one_alternate_ref);
+	foreach_alt_odb(refs_from_alternate_cb, (alt_odb_fn_cb)insert_one_alternate_ref);
 }
 
 #define INITIAL_FLUSH 16
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 27050e7..4ce9241 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -738,7 +738,7 @@ static void add_one_alternate_ref(const struct ref *ref, void *unused)
 
 static void add_alternate_refs(void)
 {
-	foreach_alt_odb(refs_from_alternate_cb, add_one_alternate_ref);
+	foreach_alt_odb(refs_from_alternate_cb, (alt_odb_fn_cb)add_one_alternate_ref);
 }
 
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
diff --git a/cache.h b/cache.h
index 9f06d21..ab3407a 100644
--- a/cache.h
+++ b/cache.h
@@ -892,8 +892,9 @@ extern struct alternate_object_database {
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
 extern void add_to_alternates_file(const char *reference);
+typedef void (*alt_odb_fn_cb)(void);
-typedef int alt_odb_fn(struct alternate_object_database *, void *);
+typedef int (*alt_odb_fn)(struct alternate_object_database *, alt_odb_fn_cb);
-extern void foreach_alt_odb(alt_odb_fn, void*);
+extern void foreach_alt_odb(alt_odb_fn, alt_odb_fn_cb);
 
 struct pack_window {
 	struct pack_window *next;
diff --git a/sha1_file.c b/sha1_file.c
index df0edba..8555dbe 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -388,7 +388,7 @@ void add_to_alternates_file(const char *reference)
 		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
 }
 
-void foreach_alt_odb(alt_odb_fn fn, void *cb)
+void foreach_alt_odb(alt_odb_fn fn, alt_odb_fn_cb cb)
 {
 	struct alternate_object_database *ent;
 
diff --git a/transport.c b/transport.c
index a02f79a..f572fa8 100644
--- a/transport.c
+++ b/transport.c
@@ -1190,14 +1190,14 @@ literal_copy:
 	return xstrdup(url);
 }
 
-int refs_from_alternate_cb(struct alternate_object_database *e, void *cb)
+int refs_from_alternate_cb(struct alternate_object_database *e, alt_odb_fn_cb cb)
 {
 	char *other;
 	size_t len;
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *extra;
-	alternate_ref_fn *ref_fn = cb;
+	alternate_ref_fn ref_fn = (alternate_ref_fn)cb;
 
 	e->name[-1] = '\0';
 	other = xstrdup(real_path(e->base));
diff --git a/transport.h b/transport.h
index efb1968..47ea41a 100644
--- a/transport.h
+++ b/transport.h
@@ -166,7 +166,7 @@ int transport_refs_pushed(struct ref *ref);
 void transport_print_push_status(const char *dest, struct ref *refs,
 		  int verbose, int porcelain, int *nonfastforward);
 
-typedef void alternate_ref_fn(const struct ref *, void *);
+typedef void (*alternate_ref_fn)(const struct ref *, void *);
-extern int refs_from_alternate_cb(struct alternate_object_database *e, void *cb);
+extern int refs_from_alternate_cb(struct alternate_object_database *e, alt_odb_fn_cb cb);
 
 #endif
-- 
1.7.4.2.417.g32d76d

[RFC 2/2] Future Proofing: Pass around pointers to either functions or data

[Michael Witten]

The seemingly unnecessary generality of the alternate object
database callback infrastructure suggests that there are plans
for passing around not only callbacks to callbacks, but also
arbitrary data to callbacks. Unfortunately, as discussed in the
previous commit's message, C[99] does not allow for values of type
pointer to function to be stored in variables of type pointer to
void.

The only unified solution, then, is to pass around a value of
type union of both pointer to function and pointer to void; this
solution is voiced here as well:

  http://www.safercode.com/blog/2008/11/25/generic-function-pointers-in-c-and-void.html

That article states:

  How can I use the same variable for object / data
  -------------------------------------------------
  and function pointers inter-changeably?
  ---------------------------------------

  Simple, use a union of a void * and the function pointer
  (like we described above) and you need not worry about
  the size because the compiler will automatically choose
  the largest size for this union which can hold a function
  pointer.

This commit introduces such a union and a couple of macros
for using it effectively.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 builtin/fetch-pack.c   |    3 ++-
 builtin/receive-pack.c |    3 ++-
 cache.h                |   16 ++++++++++++++--
 sha1_file.c            |    4 ++--
 transport.c            |    4 ++--
 transport.h            |    2 +-
 6 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index ded1784..737b382 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -225,7 +225,8 @@ static void insert_one_alternate_ref(const struct ref *ref, void *unused)
 
 static void insert_alternate_refs(void)
 {
+	ALT_ODB_FN_INFO_FUNC(info,insert_one_alternate_ref);
-	foreach_alt_odb(refs_from_alternate_cb, (alt_odb_fn_cb)insert_one_alternate_ref);
+	foreach_alt_odb(refs_from_alternate_cb, info);
 }
 
 #define INITIAL_FLUSH 16
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4ce9241..51e66d2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -738,7 +738,8 @@ static void add_one_alternate_ref(const struct ref *ref, void *unused)
 
 static void add_alternate_refs(void)
 {
+	ALT_ODB_FN_INFO_FUNC(info,add_one_alternate_ref);
-	foreach_alt_odb(refs_from_alternate_cb, (alt_odb_fn_cb)add_one_alternate_ref);
+	foreach_alt_odb(refs_from_alternate_cb, info);
 }
 
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
diff --git a/cache.h b/cache.h
index ab3407a..ca8574a 100644
--- a/cache.h
+++ b/cache.h
@@ -893,8 +893,20 @@ extern struct alternate_object_database {
 extern void prepare_alt_odb(void);
 extern void add_to_alternates_file(const char *reference);
 typedef void (*alt_odb_fn_cb)(void);
+union alt_odb_fn_info {
+	alt_odb_fn_cb func;
+	void *data;
+};
+#define ALT_ODB_FN_INFO_FUNC(variable, function) \
+	union alt_odb_fn_info variable = { \
+		.func = (alt_odb_fn_cb)function \
+	}
+#define ALT_ODB_FN_INFO_DATA(variable, data_) \
+	union alt_odb_fn_info variable = { \
+		.data = data_ \
+	}
-typedef int (*alt_odb_fn)(struct alternate_object_database *, alt_odb_fn_cb);
+typedef int (*alt_odb_fn)(struct alternate_object_database *, union alt_odb_fn_info);
-extern void foreach_alt_odb(alt_odb_fn, alt_odb_fn_cb);
+extern void foreach_alt_odb(alt_odb_fn, union alt_odb_fn_info);
 
 struct pack_window {
 	struct pack_window *next;
diff --git a/sha1_file.c b/sha1_file.c
index 8555dbe..2709fea 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -388,13 +388,13 @@ void add_to_alternates_file(const char *reference)
 		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
 }
 
-void foreach_alt_odb(alt_odb_fn fn, alt_odb_fn_cb cb)
+void foreach_alt_odb(alt_odb_fn fn, union alt_odb_fn_info info)
 {
 	struct alternate_object_database *ent;
 
 	prepare_alt_odb();
 	for (ent = alt_odb_list; ent; ent = ent->next)
-		if (fn(ent, cb))
+		if (fn(ent, info))
 			return;
 }
 
diff --git a/transport.c b/transport.c
index f572fa8..3b44b27 100644
--- a/transport.c
+++ b/transport.c
@@ -1190,14 +1190,14 @@ literal_copy:
 	return xstrdup(url);
 }
 
-int refs_from_alternate_cb(struct alternate_object_database *e, alt_odb_fn_cb cb)
+int refs_from_alternate_cb(struct alternate_object_database *e, union alt_odb_fn_info info)
 {
 	char *other;
 	size_t len;
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *extra;
-	alternate_ref_fn ref_fn = (alternate_ref_fn)cb;
+	alternate_ref_fn ref_fn = (alternate_ref_fn)info.func;
 
 	e->name[-1] = '\0';
 	other = xstrdup(real_path(e->base));
diff --git a/transport.h b/transport.h
index 47ea41a..d00e547 100644
--- a/transport.h
+++ b/transport.h
@@ -167,6 +167,6 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		  int verbose, int porcelain, int *nonfastforward);
 
 typedef void (*alternate_ref_fn)(const struct ref *, void *);
-extern int refs_from_alternate_cb(struct alternate_object_database *e, alt_odb_fn_cb cb);
+extern int refs_from_alternate_cb(struct alternate_object_database *e, union alt_odb_fn_info info);
 
 #endif
-- 
1.7.4.2.417.g32d76d

Re: [RFC 2/2] Future Proofing: Pass around pointers to either functions or data

[Erik Faye-Lund]

On Tue, Mar 29, 2011 at 5:28 PM, Michael Witten <mfwitten@gmail.com> wrote:
> +#define ALT_ODB_FN_INFO_FUNC(variable, function) \
> +       union alt_odb_fn_info variable = { \
> +               .func = (alt_odb_fn_cb)function \
> +       }
> +#define ALT_ODB_FN_INFO_DATA(variable, data_) \
> +       union alt_odb_fn_info variable = { \
> +               .data = data_ \
> +       }

We try to stay away from C99 features like this, as it doesn't work on
all compilers we target.

Re: [RFC 2/2] Future Proofing: Pass around pointers to either functions or data

[Michael Witten]

On Wed, 30 Mar 2011 12:09:46 +0200, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Tue, Mar 29, 2011 at 5:28 PM, Michael Witten <mfwitten@gmail.com> wrote:
>> +#define ALT_ODB_FN_INFO_FUNC(variable, function) \
>> +       union alt_odb_fn_info variable = { \
>> +               .func = (alt_odb_fn_cb)function \
>> +       }
>> +#define ALT_ODB_FN_INFO_DATA(variable, data_) \
>> +       union alt_odb_fn_info variable = { \
>> +               .data = data_ \
>> +       }
>
> We try to stay away from C99 features like this, as it doesn't work on
> all compilers we target.

How about changing it to this:

#define ALT_ODB_FN_INFO_FUNC(variable, function) \
	union alt_odb_fn_info variable; do { \
		variable.func = (alt_odb_fn_cb)function; \
	} while (0)
#define ALT_ODB_FN_INFO_DATA(variable, data_) \
	union alt_odb_fn_info variable; do { \
		variable.data = data_ \
	} while (0)

Re: [RFC 2/2] Future Proofing: Pass around pointers to either functions or data

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> On Tue, Mar 29, 2011 at 5:28 PM, Michael Witten <mfwitten@gmail.com> wrote:
>> +#define ALT_ODB_FN_INFO_FUNC(variable, function) \
>> +       union alt_odb_fn_info variable = { \
>> +               .func = (alt_odb_fn_cb)function \
>> +       }
>> +#define ALT_ODB_FN_INFO_DATA(variable, data_) \
>> +       union alt_odb_fn_info variable = { \
>> +               .data = data_ \
>> +       }
>
> We try to stay away from C99 features like this, as it doesn't work on
> all compilers we target.

I had an impression that 1/2 is a good fix but 2/2 is going overboard with
a theoretical exercise without real gain.

[RFC 1/3] Alternates API: Untangle the interface

[Michael Witten]

These 2 functions:

      cache.h:896:    foreach_alt_odb
  transport.h:170:    refs_from_alternate_cb

were unnecessarily coupled; the callback used by:

  refs_from_alternate_cb

was being passed to the higher-level:

  foreach_alt_odb

which necessitated some casting ugliness that has
actually been invoking undefined behavior[0].

This commit decouples this relationship, resulting
in a simpler (albeit slightly more verbose) usage.
As a bonus, the undefined behavior is automatically
resolved.

References
----------

[0] Variables of type pointer to void were being used
to pass around values of type pointer to function; see:

  [RFC 1/2] Portability: Convert strictly between function pointers
  Message-ID: <3c6b883f-8860-4da2-b328-d912019a4145-mfwitten@gmail.com>

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 builtin/fetch-pack.c   |    7 ++++++-
 builtin/receive-pack.c |    7 ++++++-
 cache.h                |    4 ++--
 sha1_file.c            |    4 ++--
 transport.c            |    3 +--
 transport.h            |    4 ++--
 6 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 9f06d21..71cfcef 100644
--- a/cache.h
+++ b/cache.h
@@ -892,8 +892,8 @@ extern struct alternate_object_database {
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
 extern void add_to_alternates_file(const char *reference);
-typedef int alt_odb_fn(struct alternate_object_database *, void *);
+typedef int (*alt_odb_fn)(struct alternate_object_database *);
-extern void foreach_alt_odb(alt_odb_fn, void*);
+extern void foreach_alt_odb(alt_odb_fn);
 
 struct pack_window {
 	struct pack_window *next;
diff --git a/sha1_file.c b/sha1_file.c
index df0edba..e55a496 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -388,13 +388,13 @@ void add_to_alternates_file(const char *reference)
 		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
 }
 
-void foreach_alt_odb(alt_odb_fn fn, void *cb)
+void foreach_alt_odb(alt_odb_fn fn)
 {
 	struct alternate_object_database *ent;
 
 	prepare_alt_odb();
 	for (ent = alt_odb_list; ent; ent = ent->next)
-		if (fn(ent, cb))
+		if (fn(ent))
 			return;
 }
 
diff --git a/transport.h b/transport.h
index efb1968..72a692f 100644
--- a/transport.h
+++ b/transport.h
@@ -166,7 +166,7 @@ int transport_refs_pushed(struct ref *ref);
 void transport_print_push_status(const char *dest, struct ref *refs,
 		  int verbose, int porcelain, int *nonfastforward);
 
-typedef void alternate_ref_fn(const struct ref *, void *);
+typedef void (*alternate_ref_fn)(const struct ref *, void *);
-extern int refs_from_alternate_cb(struct alternate_object_database *e, void *cb);
+extern int refs_from_alternate_cb(struct alternate_object_database *, alternate_ref_fn);
 
 #endif
diff --git a/transport.c b/transport.c
index a02f79a..c61723f 100644
--- a/transport.c
+++ b/transport.c
@@ -1190,14 +1190,13 @@ literal_copy:
 	return xstrdup(url);
 }
 
-int refs_from_alternate_cb(struct alternate_object_database *e, void *cb)
+int refs_from_alternate_cb(struct alternate_object_database *e, alternate_ref_fn ref_fn)
 {
 	char *other;
 	size_t len;
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *extra;
-	alternate_ref_fn *ref_fn = cb;
 
 	e->name[-1] = '\0';
 	other = xstrdup(real_path(e->base));
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 85aff02..62ebdb4 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -224,9 +224,14 @@ static void insert_one_alternate_ref(const struct ref *ref, void *unused)
 	rev_list_insert_ref(NULL, ref->old_sha1, 0, NULL);
 }
 
+static int alt_odb_callback(struct alternate_object_database *d)
+{
+	return refs_from_alternate_cb(d,insert_one_alternate_ref);
+}
+
 static void insert_alternate_refs(void)
 {
-	foreach_alt_odb(refs_from_alternate_cb, insert_one_alternate_ref);
+	foreach_alt_odb(alt_odb_callback);
 }
 
 #define INITIAL_FLUSH 16
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 27050e7..8ef6301 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -736,9 +736,14 @@ static void add_one_alternate_ref(const struct ref *ref, void *unused)
 	add_extra_ref(".have", ref->old_sha1, 0);
 }
 
+static int alt_odb_callback(struct alternate_object_database *d)
+{
+	return refs_from_alternate_cb(d,add_one_alternate_ref);
+}
+
 static void add_alternate_refs(void)
 {
-	foreach_alt_odb(refs_from_alternate_cb, add_one_alternate_ref);
+	foreach_alt_odb(alt_odb_callback);
 }
 
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
-- 
1.7.4.18.g68fe8

[RFC 2/3] Alternates API: Improve naming

[Michael Witten]

This should make the API a bit more cohesive.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 builtin/fetch-pack.c   |    2 +-
 builtin/receive-pack.c |    2 +-
 sha1_file.c            |    6 +++---
 transport.c            |    8 ++++----
 transport.h            |    4 ++--
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/transport.h b/transport.h
index 72a692f..88fa484 100644
--- a/transport.h
+++ b/transport.h
@@ -166,7 +166,7 @@ int transport_refs_pushed(struct ref *ref);
 void transport_print_push_status(const char *dest, struct ref *refs,
 		  int verbose, int porcelain, int *nonfastforward);
 
-typedef void (*alternate_ref_fn)(const struct ref *, void *);
+typedef void (*transport_alt_refs_fn)(const struct ref *, void *);
-extern int refs_from_alternate_cb(struct alternate_object_database *, alternate_ref_fn);
+extern int transport_alt_refs(struct alternate_object_database *, transport_alt_refs_fn);
 
 #endif
diff --git a/transport.c b/transport.c
index c61723f..53d37a9 100644
--- a/transport.c
+++ b/transport.c
@@ -1190,7 +1190,7 @@ literal_copy:
 	return xstrdup(url);
 }
 
-int refs_from_alternate_cb(struct alternate_object_database *e, alternate_ref_fn ref_fn)
+int transport_alt_refs(struct alternate_object_database *d, transport_alt_refs_fn ref_fn)
 {
 	char *other;
 	size_t len;
@@ -1198,9 +1198,9 @@ int refs_from_alternate_cb(struct alternate_object_database *e, alternate_ref_fn
 	struct transport *transport;
 	const struct ref *extra;
 
-	e->name[-1] = '\0';
+	d->name[-1] = '\0';
-	other = xstrdup(real_path(e->base));
+	other = xstrdup(real_path(d->base));
-	e->name[-1] = '/';
+	d->name[-1] = '/';
 	len = strlen(other);
 
 	while (other[len-1] == '/')
diff --git a/sha1_file.c b/sha1_file.c
index e55a496..a6c1f1f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -390,11 +390,11 @@ void add_to_alternates_file(const char *reference)
 
 void foreach_alt_odb(alt_odb_fn fn)
 {
-	struct alternate_object_database *ent;
+	struct alternate_object_database *d;
 
 	prepare_alt_odb();
-	for (ent = alt_odb_list; ent; ent = ent->next)
+	for (d = alt_odb_list; d; d = d->next)
-		if (fn(ent))
+		if (fn(d))
 			return;
 }
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 62ebdb4..b3a9f74 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -226,7 +226,7 @@ static void insert_one_alternate_ref(const struct ref *ref, void *unused)
 
 static int alt_odb_callback(struct alternate_object_database *d)
 {
-	return refs_from_alternate_cb(d,insert_one_alternate_ref);
+	return transport_alt_refs(d,insert_one_alternate_ref);
 }
 
 static void insert_alternate_refs(void)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8ef6301..a761332 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -738,7 +738,7 @@ static void add_one_alternate_ref(const struct ref *ref, void *unused)
 
 static int alt_odb_callback(struct alternate_object_database *d)
 {
-	return refs_from_alternate_cb(d,add_one_alternate_ref);
+	return transport_alt_refs(d,add_one_alternate_ref);
 }
 
 static void add_alternate_refs(void)
-- 
1.7.4.18.g68fe8

[RFC 3/3] Alternates API: Remove unused parameter

[Michael Witten]

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 builtin/fetch-pack.c   |    2 +-
 builtin/receive-pack.c |    2 +-
 transport.c            |    2 +-
 transport.h            |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/transport.h b/transport.h
index 88fa484..cf6fda4 100644
--- a/transport.h
+++ b/transport.h
@@ -166,7 +166,7 @@ int transport_refs_pushed(struct ref *ref);
 void transport_print_push_status(const char *dest, struct ref *refs,
 		  int verbose, int porcelain, int *nonfastforward);
 
-typedef void (*transport_alt_refs_fn)(const struct ref *, void *);
+typedef void (*transport_alt_refs_fn)(const struct ref *);
 extern int transport_alt_refs(struct alternate_object_database *, transport_alt_refs_fn);
 
 #endif
diff --git a/transport.c b/transport.c
index 53d37a9..165f69e 100644
--- a/transport.c
+++ b/transport.c
@@ -1217,7 +1217,7 @@ int transport_alt_refs(struct alternate_object_database *d, transport_alt_refs_f
 	for (extra = transport_get_remote_refs(transport);
 	     extra;
 	     extra = extra->next)
-		ref_fn(extra, NULL);
+		ref_fn(extra);
 	transport_disconnect(transport);
 	free(other);
 	return 0;
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index b3a9f74..d2e0111 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -219,7 +219,7 @@ static void send_request(int fd, struct strbuf *buf)
 		safe_write(fd, buf->buf, buf->len);
 }
 
-static void insert_one_alternate_ref(const struct ref *ref, void *unused)
+static void insert_one_alternate_ref(const struct ref *ref)
 {
 	rev_list_insert_ref(NULL, ref->old_sha1, 0, NULL);
 }
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a761332..47a5aaa 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -731,7 +731,7 @@ static int delete_only(struct command *commands)
 	return 1;
 }
 
-static void add_one_alternate_ref(const struct ref *ref, void *unused)
+static void add_one_alternate_ref(const struct ref *ref)
 {
 	add_extra_ref(".have", ref->old_sha1, 0);
 }
-- 
1.7.4.18.g68fe8

[RFC 0/3] Alternates API

[Michael Witten]

Michael Witten (3):
  Alternates API: Untangle the interface
  Alternates API: Improve naming
  Alternates API: Remove unused parameter

 builtin/fetch-pack.c   |    9 +++++++--
 builtin/receive-pack.c |    9 +++++++--
 cache.h                |    4 ++--
 sha1_file.c            |    8 ++++----
 transport.c            |   11 +++++------
 transport.h            |    4 ++--
 6 files changed, 27 insertions(+), 18 deletions(-)

-- 
1.7.4.18.g68fe8

Re: [RFC 1/3] Alternates API: Untangle the interface

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> diff --git a/transport.c b/transport.c
> index a02f79a..c61723f 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1190,14 +1190,13 @@ literal_copy:
>  	return xstrdup(url);
>  }
>  
> -int refs_from_alternate_cb(struct alternate_object_database *e, void *cb)
> +int refs_from_alternate_cb(struct alternate_object_database *e, alternate_ref_fn ref_fn)
>  {

Adding an extra "fn" is Ok, but I'd rather not to see removal of generic
callback data pointer if the _only_ reason you are doing so is "currently
nobody uses it".

Besides, I see "a function pointer and a data pointer cannot be held in a
single variable" as an academic mental mastu^wexercise that is useless in
real life.  Are there real platforms that matter where (void *) cannot
hold a pointer to a function?

Re: [RFC 2/3] Alternates API: Improve naming

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> diff --git a/sha1_file.c b/sha1_file.c
> index e55a496..a6c1f1f 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -390,11 +390,11 @@ void add_to_alternates_file(const char *reference)
>  
>  void foreach_alt_odb(alt_odb_fn fn)
>  {
> -	struct alternate_object_database *ent;
> +	struct alternate_object_database *d;
>  
>  	prepare_alt_odb();
> -	for (ent = alt_odb_list; ent; ent = ent->next)
> +	for (d = alt_odb_list; d; d = d->next)
> -		if (fn(ent))
> +		if (fn(d))

I see this as a naming regression; that pointer variable points at an
entry in a list, and the callback is very well aware of that too (so the
s/e/d/g in an earlier hunk also is a naming regression).

Re: [RFC 3/3] Alternates API: Remove unused parameter

[Junio C Hamano]

Curious; why are your recent patch series all have screwed-up threading?

Here is an earlier one:

 [ 120: Michael Witten         ] [PATCH 4/4] Clean: Remove useless parameters from both get_commit_info() functions
  [  16: Michael Witten         ] [PATCH 0/4] Miscellaneous Improvements
  [  51: Michael Witten         ] [PATCH 1/4] Typos: t/README
  [  56: Michael Witten         ] [PATCH 2/4] Clean: Remove superfluous strbuf 'docs'
  [  21: Michael Witten         ] [PATCH 3/4] Clean: Remove unnecessary `\' (line continuation)

and this series looks like this:

 [  63: Michael Witten         ] [RFC 3/3] Alternates API: Remove unused parameter
  [  16: Michael Witten         ] [RFC 0/3] Alternates API
  [ 152: Michael Witten         ] [RFC 1/3] Alternates API: Untangle the interface
  [  99: Michael Witten         ] [RFC 2/3] Alternates API: Improve naming

Did we break git-send-email recently?  I would personally be worried more
about that than the alternates callbacks.

Having said all that, I didn't like refs_from_alternate_cb interface
myself.  I should probably have done for_each_ref_in_alternates() that
takes two callback functions, one that decides if it wants to deal with an
alternate repository given the path to it, and the other that is fed the
refs from the alternate.  Both callback functions should take the usual
"void *cb_data" in addition, of course.

IOW, something like...

	typedef int (*skip_alternate_fn)(const char *, void *);

	int for_each_ref_in_alternates(each_ref_fn ref_fn,
        			skip_alternate_fn skip_fn,
                                void *cb_data);