[PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter

[PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter

[René Scharfe]

Expose the expected type of the second parameter of extend_abbrev_len()
instead of casting a void pointer internally.  Just a single caller
passes in a void pointer, the rest pass the correct type.  Let the
compiler help keeping it that way.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 object-name.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/object-name.c b/object-name.c
index 732056ff5e..1e0118e8a6 100644
--- a/object-name.c
+++ b/object-name.c
@@ -696,10 +696,9 @@ static inline char get_hex_char_from_oid(const struct object_id *oid,
 		return hex[oid->hash[pos >> 1] & 0xf];
 }
 
-static int extend_abbrev_len(const struct object_id *oid, void *cb_data)
+static int extend_abbrev_len(const struct object_id *oid,
+			     struct min_abbrev_data *mad)
 {
-	struct min_abbrev_data *mad = cb_data;
-
 	unsigned int i = mad->init_len;
 	while (mad->hex[i] && mad->hex[i] == get_hex_char_from_oid(oid, i))
 		i++;
-- 
2.51.0

Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Expose the expected type of the second parameter of extend_abbrev_len()
> instead of casting a void pointer internally.  Just a single caller
> passes in a void pointer, the rest pass the correct type.  Let the
> compiler help keeping it that way.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  object-name.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

We obviously do *not* have to, but I have to wonder if we want to go
one step further to have that single caller explicitly cast it down
to make the intent more clear, i.e.e.g.,

 object-name.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git c/object-name.c w/object-name.c
index 11aa0e6afc..8335d0239e 100644
--- c/object-name.c
+++ w/object-name.c
@@ -714,7 +714,9 @@ static int repo_extend_abbrev_len(struct repository *r UNUSED,
 				  const struct object_id *oid,
 				  void *cb_data)
 {
-	return extend_abbrev_len(oid, cb_data);
+	struct min_abbrev_data *mad = cb_data;
+
+	return extend_abbrev_len(oid, mad);
 }
 
 static void find_abbrev_len_for_midx(struct multi_pack_index *m,

Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter

[René Scharfe]

On 9/4/25 10:09 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Expose the expected type of the second parameter of extend_abbrev_len()
>> instead of casting a void pointer internally.  Just a single caller
>> passes in a void pointer, the rest pass the correct type.  Let the
>> compiler help keeping it that way.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  object-name.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> We obviously do *not* have to, but I have to wonder if we want to go
> one step further to have that single caller explicitly cast it down
> to make the intent more clear, i.e.e.g.,
> 
>  object-name.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git c/object-name.c w/object-name.c
> index 11aa0e6afc..8335d0239e 100644
> --- c/object-name.c
> +++ w/object-name.c
> @@ -714,7 +714,9 @@ static int repo_extend_abbrev_len(struct repository *r UNUSED,
>  				  const struct object_id *oid,
>  				  void *cb_data)
>  {
> -	return extend_abbrev_len(oid, cb_data);
> +	struct min_abbrev_data *mad = cb_data;
> +
> +	return extend_abbrev_len(oid, mad);
>  }
>  
>  static void find_abbrev_len_for_midx(struct multi_pack_index *m,

I can see the appeal, even though (or because) it's kinda half a step
back as it keeps the original local variable, in a better place.

We could _lunge_ forward and add type checks to allow the compiler to
tell us whether the pointers' journey through the void is safe.  The
trick below is simple enough, but requires bespoke macros AFAICS.

René


---
 object-name.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/object-name.c b/object-name.c
index 1e0118e8a6..c37a3826d0 100644
--- a/object-name.c
+++ b/object-name.c
@@ -52,6 +52,22 @@ struct disambiguate_state {
 	unsigned always_call_fn:1;
 };
 
+#define DEFINE_DISAMBIGUATE_HINT_CB(scope, fn_name) \
+scope int fn_name##__void(struct repository *r, const struct object_id *oid, \
+			  void *cb_data) \
+{ \
+	return fn_name(r, oid, cb_data); \
+} \
+scope int fn_name##__void(struct repository *, const struct object_id *, void *)
+
+#define SET_DISAMBIGUATE_HINT_CB_DATA(ds, fn_name, data) do { \
+	struct disambiguate_state *dsp = (ds); \
+	if (0) \
+		fn_name(NULL, NULL, (data)); \
+	dsp->fn = fn_name##__void; \
+	dsp->cb_data = (data); \
+} while (0)
+
 static void update_candidates(struct disambiguate_state *ds, const struct object_id *current)
 {
 	/* The hash algorithm of current has already been filtered */
@@ -510,11 +526,13 @@ static int collect_ambiguous(const struct object_id *oid, void *data)
 
 static int repo_collect_ambiguous(struct repository *r UNUSED,
 				  const struct object_id *oid,
-				  void *data)
+				  struct oid_array *collect)
 {
-	return collect_ambiguous(oid, data);
+	return collect_ambiguous(oid, collect);
 }
 
+DEFINE_DISAMBIGUATE_HINT_CB(static, repo_collect_ambiguous);
+
 static int sort_ambiguous(const void *va, const void *vb, void *ctx)
 {
 	struct repository *sort_ambiguous_repo = ctx;
@@ -654,8 +672,7 @@ int repo_for_each_abbrev(struct repository *r, const char *prefix,
 		return -1;
 
 	ds.always_call_fn = 1;
-	ds.fn = repo_collect_ambiguous;
-	ds.cb_data = &collect;
+	SET_DISAMBIGUATE_HINT_CB_DATA(&ds, repo_collect_ambiguous, &collect);
 	find_short_object_filename(&ds);
 	find_short_packed_object(&ds);
 
@@ -711,11 +728,13 @@ static int extend_abbrev_len(const struct object_id *oid,
 
 static int repo_extend_abbrev_len(struct repository *r UNUSED,
 				  const struct object_id *oid,
-				  void *cb_data)
+				  struct min_abbrev_data *mad)
 {
-	return extend_abbrev_len(oid, cb_data);
+	return extend_abbrev_len(oid, mad);
 }
 
+DEFINE_DISAMBIGUATE_HINT_CB(static, repo_extend_abbrev_len);
+
 static void find_abbrev_len_for_midx(struct multi_pack_index *m,
 				     struct min_abbrev_data *mad)
 {
@@ -871,9 +890,8 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex,
 	if (init_object_disambiguation(r, hex, mad.cur_len, algo, &ds) < 0)
 		return -1;
 
-	ds.fn = repo_extend_abbrev_len;
 	ds.always_call_fn = 1;
-	ds.cb_data = (void *)&mad;
+	SET_DISAMBIGUATE_HINT_CB_DATA(&ds, repo_extend_abbrev_len, &mad);
 
 	find_short_object_filename(&ds);
 	(void)finish_object_disambiguation(&ds, &oid_ret);
-- 
2.51.0

Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> I can see the appeal, even though (or because) it's kinda half a step
> back as it keeps the original local variable, in a better place.
>
> We could _lunge_ forward and add type checks to allow the compiler to
> tell us whether the pointers' journey through the void is safe.  The
> trick below is simple enough, but requires bespoke macros AFAICS.

Once there is even a single step of callback interface where the
callback parameter has to be a generic "void *", we have to cast
down to the concrete "struct min_abbrev_data *" either explicitly
or implicitly anyway, so it does not really make that much of a
difference (and that is why I said "we obviously do not have to").

Thanks.