{"thread":{"id":"64092","subject":"[PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter","startedAt":"2025-09-04T17:58:33Z","lastAt":"2025-09-08T04:17:35Z","messageCount":4,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"525555","messageId":"e0bc9a67-faa9-4218-a55a-c7d53c15cfce@web.de","threadId":"64092","inReplyTo":null,"subject":"[PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-09-04T17:58:25Z","receivedAt":"2025-09-04T17:58:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Expose the expected type of the second parameter of extend_abbrev_len()\ninstead of casting a void pointer internally.  Just a single caller\npasses in a void pointer, the rest pass the correct type.  Let the\ncompiler help keeping it that way.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n object-name.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 732056ff5e..1e0118e8a6 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -696,10 +696,9 @@ static inline char get_hex_char_from_oid(const struct object_id *oid,\n \t\treturn hex[oid->hash[pos >> 1] & 0xf];\n }\n \n-static int extend_abbrev_len(const struct object_id *oid, void *cb_data)\n+static int extend_abbrev_len(const struct object_id *oid,\n+\t\t\t     struct min_abbrev_data *mad)\n {\n-\tstruct min_abbrev_data *mad = cb_data;\n-\n \tunsigned int i = mad->init_len;\n \twhile (mad->hex[i] && mad->hex[i] == get_hex_char_from_oid(oid, i))\n \t\ti++;\n-- \n2.51.0\n"},{"id":"525561","messageId":"xmqqv7ly6kup.fsf@gitster.g","threadId":"64092","inReplyTo":"e0bc9a67-faa9-4218-a55a-c7d53c15cfce@web.de","subject":"Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T20:09:18Z","receivedAt":"2025-09-04T20:09:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Expose the expected type of the second parameter of extend_abbrev_len()\n> instead of casting a void pointer internally.  Just a single caller\n> passes in a void pointer, the rest pass the correct type.  Let the\n> compiler help keeping it that way.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  object-name.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n\nWe obviously do *not* have to, but I have to wonder if we want to go\none step further to have that single caller explicitly cast it down\nto make the intent more clear, i.e.e.g.,\n\n object-name.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git c/object-name.c w/object-name.c\nindex 11aa0e6afc..8335d0239e 100644\n--- c/object-name.c\n+++ w/object-name.c\n@@ -714,7 +714,9 @@ static int repo_extend_abbrev_len(struct repository *r UNUSED,\n \t\t\t\t  const struct object_id *oid,\n \t\t\t\t  void *cb_data)\n {\n-\treturn extend_abbrev_len(oid, cb_data);\n+\tstruct min_abbrev_data *mad = cb_data;\n+\n+\treturn extend_abbrev_len(oid, mad);\n }\n \n static void find_abbrev_len_for_midx(struct multi_pack_index *m,\n\n\n\n"},{"id":"525711","messageId":"81d10b44-5b3a-42af-acf4-ae76f2fee298@web.de","threadId":"64092","inReplyTo":"xmqqv7ly6kup.fsf@gitster.g","subject":"Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-09-07T16:22:18Z","receivedAt":"2025-09-07T16:22:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 9/4/25 10:09 PM, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> Expose the expected type of the second parameter of extend_abbrev_len()\n>> instead of casting a void pointer internally.  Just a single caller\n>> passes in a void pointer, the rest pass the correct type.  Let the\n>> compiler help keeping it that way.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  object-name.c | 5 ++---\n>>  1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> We obviously do *not* have to, but I have to wonder if we want to go\n> one step further to have that single caller explicitly cast it down\n> to make the intent more clear, i.e.e.g.,\n> \n>  object-name.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git c/object-name.c w/object-name.c\n> index 11aa0e6afc..8335d0239e 100644\n> --- c/object-name.c\n> +++ w/object-name.c\n> @@ -714,7 +714,9 @@ static int repo_extend_abbrev_len(struct repository *r UNUSED,\n>  \t\t\t\t  const struct object_id *oid,\n>  \t\t\t\t  void *cb_data)\n>  {\n> -\treturn extend_abbrev_len(oid, cb_data);\n> +\tstruct min_abbrev_data *mad = cb_data;\n> +\n> +\treturn extend_abbrev_len(oid, mad);\n>  }\n>  \n>  static void find_abbrev_len_for_midx(struct multi_pack_index *m,\n\nI can see the appeal, even though (or because) it's kinda half a step\nback as it keeps the original local variable, in a better place.\n\nWe could _lunge_ forward and add type checks to allow the compiler to\ntell us whether the pointers' journey through the void is safe.  The\ntrick below is simple enough, but requires bespoke macros AFAICS.\n\nRené\n\n\n---\n object-name.c | 34 ++++++++++++++++++++++++++--------\n 1 file changed, 26 insertions(+), 8 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 1e0118e8a6..c37a3826d0 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -52,6 +52,22 @@ struct disambiguate_state {\n \tunsigned always_call_fn:1;\n };\n \n+#define DEFINE_DISAMBIGUATE_HINT_CB(scope, fn_name) \\\n+scope int fn_name##__void(struct repository *r, const struct object_id *oid, \\\n+\t\t\t  void *cb_data) \\\n+{ \\\n+\treturn fn_name(r, oid, cb_data); \\\n+} \\\n+scope int fn_name##__void(struct repository *, const struct object_id *, void *)\n+\n+#define SET_DISAMBIGUATE_HINT_CB_DATA(ds, fn_name, data) do { \\\n+\tstruct disambiguate_state *dsp = (ds); \\\n+\tif (0) \\\n+\t\tfn_name(NULL, NULL, (data)); \\\n+\tdsp->fn = fn_name##__void; \\\n+\tdsp->cb_data = (data); \\\n+} while (0)\n+\n static void update_candidates(struct disambiguate_state *ds, const struct object_id *current)\n {\n \t/* The hash algorithm of current has already been filtered */\n@@ -510,11 +526,13 @@ static int collect_ambiguous(const struct object_id *oid, void *data)\n \n static int repo_collect_ambiguous(struct repository *r UNUSED,\n \t\t\t\t  const struct object_id *oid,\n-\t\t\t\t  void *data)\n+\t\t\t\t  struct oid_array *collect)\n {\n-\treturn collect_ambiguous(oid, data);\n+\treturn collect_ambiguous(oid, collect);\n }\n \n+DEFINE_DISAMBIGUATE_HINT_CB(static, repo_collect_ambiguous);\n+\n static int sort_ambiguous(const void *va, const void *vb, void *ctx)\n {\n \tstruct repository *sort_ambiguous_repo = ctx;\n@@ -654,8 +672,7 @@ int repo_for_each_abbrev(struct repository *r, const char *prefix,\n \t\treturn -1;\n \n \tds.always_call_fn = 1;\n-\tds.fn = repo_collect_ambiguous;\n-\tds.cb_data = &collect;\n+\tSET_DISAMBIGUATE_HINT_CB_DATA(&ds, repo_collect_ambiguous, &collect);\n \tfind_short_object_filename(&ds);\n \tfind_short_packed_object(&ds);\n \n@@ -711,11 +728,13 @@ static int extend_abbrev_len(const struct object_id *oid,\n \n static int repo_extend_abbrev_len(struct repository *r UNUSED,\n \t\t\t\t  const struct object_id *oid,\n-\t\t\t\t  void *cb_data)\n+\t\t\t\t  struct min_abbrev_data *mad)\n {\n-\treturn extend_abbrev_len(oid, cb_data);\n+\treturn extend_abbrev_len(oid, mad);\n }\n \n+DEFINE_DISAMBIGUATE_HINT_CB(static, repo_extend_abbrev_len);\n+\n static void find_abbrev_len_for_midx(struct multi_pack_index *m,\n \t\t\t\t     struct min_abbrev_data *mad)\n {\n@@ -871,9 +890,8 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex,\n \tif (init_object_disambiguation(r, hex, mad.cur_len, algo, &ds) < 0)\n \t\treturn -1;\n \n-\tds.fn = repo_extend_abbrev_len;\n \tds.always_call_fn = 1;\n-\tds.cb_data = (void *)&mad;\n+\tSET_DISAMBIGUATE_HINT_CB_DATA(&ds, repo_extend_abbrev_len, &mad);\n \n \tfind_short_object_filename(&ds);\n \t(void)finish_object_disambiguation(&ds, &oid_ret);\n-- \n2.51.0\n\n"},{"id":"525749","messageId":"xmqqh5xdzig2.fsf@gitster.g","threadId":"64092","inReplyTo":"81d10b44-5b3a-42af-acf4-ae76f2fee298@web.de","subject":"Re: [PATCH] object-name: declare pointer type of extend_abbrev_len()'s 2nd parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-08T04:17:33Z","receivedAt":"2025-09-08T04:17:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> I can see the appeal, even though (or because) it's kinda half a step\n> back as it keeps the original local variable, in a better place.\n>\n> We could _lunge_ forward and add type checks to allow the compiler to\n> tell us whether the pointers' journey through the void is safe.  The\n> trick below is simple enough, but requires bespoke macros AFAICS.\n\nOnce there is even a single step of callback interface where the\ncallback parameter has to be a generic \"void *\", we have to cast\ndown to the concrete \"struct min_abbrev_data *\" either explicitly\nor implicitly anyway, so it does not really make that much of a\ndifference (and that is why I said \"we obviously do not have to\").\n\nThanks.\n"}]}