From: "René Scharfe" <l.s.r@web•de>
To: Junio C Hamano <gitster@pobox•com>
Cc: Git List <git@vger•kernel.org>
Subject: Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter
Date: Sun, 7 Sep 2025 18:22:18 +0200 [thread overview]
Message-ID: <81d10b44-5b3a-42af-acf4-ae76f2fee298@web.de> (raw)
In-Reply-To: <xmqqv7ly6kup.fsf@gitster.g>
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
next prev parent reply other threads:[~2025-09-07 16:22 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-04 17:58 [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter René Scharfe
2025-09-04 20:09 ` Junio C Hamano
2025-09-07 16:22 ` René Scharfe [this message]
2025-09-08 4:17 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=81d10b44-5b3a-42af-acf4-ae76f2fee298@web.de \
--to=l.s.r@web$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox