{"thread":{"id":"63487","subject":"[PATCH] /: [FirstTimer] Remove DISABLE_SIGN_COMPARE_WARNINGS from file add-interactive.c","startedAt":"2025-05-20T19:19:10Z","lastAt":"2025-05-20T21:10:27Z","messageCount":2,"participants":["Lucas Eiji","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518523","messageId":"20250520191859.136798-1-lucaseiji54@gmail.com","threadId":"63487","inReplyTo":null,"subject":"[PATCH] /: [FirstTimer] Remove DISABLE_SIGN_COMPARE_WARNINGS from file add-interactive.c","fromName":"Lucas Eiji","fromEmail":"lucaseiji54@gmail.com","sentAt":"2025-05-20T19:18:59Z","receivedAt":"2025-05-20T19:19:10Z","isPatch":true,"sender":{"key":"lucaseiji54@gmail.com","avatar":null},"body":"From: Eiji Uchiyama <eijiuchiyama@github.com>\n\nThis is an initial contribution to git, based on the SoC 2025 ideas\nfor microprojects. It removes the DISABLE_SIGN_COMPARE_WARNINGS macro and\nsolves the warnings generated by running make DEVELOPER=1 -j4\n\nSigned-off-by: Lucas Eiji Uchiyama <eijiuchiyama@usp.br>\n---\n add-interactive.c | 24 +++++++++++-------------\n 1 file changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 97ff35b6f1..3a0c44c47f 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"add-interactive.h\"\n #include \"color.h\"\n@@ -213,10 +211,10 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)\n \telse if (index > 0 &&\n \t\t starts_with(list->sorted.items[index - 1].string, string))\n \t\treturn -1;\n-\telse if (index + 1 < list->sorted.nr &&\n+\telse if (index + 1 < (long int)(list->sorted.nr) &&\n \t\t starts_with(list->sorted.items[index + 1].string, string))\n \t\treturn -1;\n-\telse if (index < list->sorted.nr &&\n+\telse if (index < (long int)(list->sorted.nr) &&\n \t\t starts_with(list->sorted.items[index].string, string))\n \t\titem = list->sorted.items[index].util;\n \telse\n@@ -244,7 +242,7 @@ static void list(struct add_i_state *s, struct string_list *list, int *selected,\n \t\tcolor_fprintf_ln(stdout, s->header_color,\n \t\t\t\t \"%s\", opts->header);\n \n-\tfor (i = 0; i < list->nr; i++) {\n+\tfor (i = 0; i < (long int)(list->nr); i++) {\n \t\topts->print_item(i, selected ? selected[i] : 0, list->items + i,\n \t\t\t\t opts->print_item_data);\n \n@@ -385,7 +383,7 @@ static ssize_t list_and_choose(struct add_i_state *s,\n \t\t\t\t\tto = from + 1;\n \t\t\t}\n \n-\t\t\tif (from < 0 || from >= items->items.nr ||\n+\t\t\tif (from < 0 || from >= (long int)(items->items.nr) ||\n \t\t\t    (singleton && from + 1 != to)) {\n \t\t\t\tcolor_fprintf_ln(stderr, s->error_color,\n \t\t\t\t\t\t _(\"Huh (%s)?\"), p);\n@@ -395,7 +393,7 @@ static ssize_t list_and_choose(struct add_i_state *s,\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tif (to > items->items.nr)\n+\t\t\tif (to > (long int)(items->items.nr))\n \t\t\t\tto = items->items.nr;\n \n \t\t\tfor (; from < to; from++)\n@@ -859,7 +857,7 @@ static int get_untracked_files(struct repository *r,\n \tadd_pattern_list(&dir, EXC_CMDL, \"--exclude option\");\n \tfill_directory(&dir, r->index, ps);\n \n-\tfor (i = 0; i < dir.nr; i++) {\n+\tfor (i = 0; (long int)(i) < dir.nr; i++) {\n \t\tstruct dir_entry *ent = dir.entries[i];\n \n \t\tif (index_name_is_other(r->index, ent->name, ent->len)) {\n@@ -939,7 +937,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,\n \t\treturn -1;\n \n \tif (unmerged_count || binary_count) {\n-\t\tfor (i = j = 0; i < files->items.nr; i++) {\n+\t\tfor (i = j = 0; i < (long int)(files->items.nr); i++) {\n \t\t\tstruct file_item *item = files->items.items[i].util;\n \n \t\t\tif (item->index.binary || item->worktree.binary) {\n@@ -972,7 +970,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,\n \t\tstruct strvec args = STRVEC_INIT;\n \t\tstruct pathspec ps_selected = { 0 };\n \n-\t\tfor (i = 0; i < files->items.nr; i++)\n+\t\tfor (i = 0; i < (long int)(files->items.nr); i++)\n \t\t\tif (files->selected[i])\n \t\t\t\tstrvec_push(&args,\n \t\t\t\t\t    files->items.items[i].string);\n@@ -1018,7 +1016,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n \t\t\t     oid_to_hex(!is_initial ? &oid :\n \t\t\t\t\ts->r->hash_algo->empty_tree),\n \t\t\t     \"--\", NULL);\n-\t\tfor (i = 0; i < files->items.nr; i++)\n+\t\tfor (i = 0; i < (long int)(files->items.nr); i++)\n \t\t\tif (files->selected[i])\n \t\t\t\tstrvec_push(&cmd.args,\n \t\t\t\t\t    files->items.items[i].string);\n@@ -1146,7 +1144,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)\n \tssize_t i;\n \tint res = 0;\n \n-\tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n+\tfor (i = 0; i < (long int)(ARRAY_SIZE(command_list)); i++) {\n \t\tstruct command_item *util = xcalloc(1, sizeof(*util));\n \t\tutil->command = command_list[i].command;\n \t\tstring_list_append(&commands.items, command_list[i].string)\n@@ -1183,7 +1181,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)\n \t\tstruct command_item *util;\n \n \t\ti = list_and_choose(&s, &commands, &main_loop_opts);\n-\t\tif (i < 0 || i >= commands.items.nr)\n+\t\tif (i < 0 || i >= (long int)(commands.items.nr))\n \t\t\tutil = NULL;\n \t\telse\n \t\t\tutil = commands.items.items[i].util;\n-- \n2.34.1\n\n"},{"id":"518533","messageId":"xmqqplg3gf9b.fsf@gitster.g","threadId":"63487","inReplyTo":"20250520191859.136798-1-lucaseiji54@gmail.com","subject":"Re: [PATCH] /: [FirstTimer] Remove DISABLE_SIGN_COMPARE_WARNINGS from file add-interactive.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-20T21:10:24Z","receivedAt":"2025-05-20T21:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Eiji <lucaseiji54@gmail.com> writes:\n\n> From: Eiji Uchiyama <eijiuchiyama@github.com>\n>\n> This is an initial contribution to git, based on the SoC 2025 ideas\n> for microprojects. It removes the DISABLE_SIGN_COMPARE_WARNINGS macro and\n> solves the warnings generated by running make DEVELOPER=1 -j4\n\nYour first sentence is not something you want to carve in stone as\npart of the official history, and should not be part of the proposed\ncommit log message.  Yet, it is very nice of you to tell your\nreviewers that you are the first-time contributor and may want extra\nhelp by community members.  If you want to do so, do it below the\nthree-dash line that is between the proposed log message and the\ndiffstat.\n\nThe usual way to compose a log message of this project is to\n\n - Give an observation on how the current system works in the\n   present tense (so no need to say \"Currently X is Y\", or\n   \"Previously X was Y\" to describe the state before your change;\n   just \"X is Y\" is enough), and discuss what you perceive as a\n   problem in it.\n\n - Propose a solution (optional---often, problem description\n   trivially leads to an obvious solution in reader's minds).\n\n - Give commands to the codebase to \"become like so\".\n\nin this order.  I would expect to see something along the lines of ...\n\n    Subject: [PATCH] add-interactive.c: squelch -Wsign-compare warnings\n\n    A handful of functions in add-interactive.c compare .nr member\n    of a string_list structure (which is of type size_t) with a\n    local variable (which often is of type int), and triggers\n    compiler warnings due to -Wsign-compare being part of the\n    developer configuration.\n\n    Squelch them by DOING THIS AND THAT.\n\n... but I'll refrain from filling the \"DOING THIS AND THAT\" part.\n\n> -\telse if (index + 1 < list->sorted.nr &&\n> +\telse if (index + 1 < (long int)(list->sorted.nr) &&\n\nWell, by sprinkling casts all over the place, you can squelch almost\nany compiler warnings, but the real question you should ask is: is\nit making the code more correct, or at least not worse?\n\nFor example, what does the above code do on a platform where size_t\nis 64-bit unsigned integer, and \"long int\" is 32-bit signed integer?\nFor those who are reading from sidelines, \"index\" here is \"int\".\n\nVery locally on this line, I think the more correct fix may be to\ndeclare that \"index\" is of type \"size_t\" (not \"int\").  We may also\nhave to barf when \"index + 1\" overflows \"size_t\".\n\nBut do not go there yet.\n\nI think the real culprit is that string_list is misdesigned in that\nmost of the code there work with platform natural \"int\" type\n(e.g. get_entry_index() that looks for the location in the array for\na given string does bisection using \"int\", add_entry() that returns\nwhere in the array of strings it inserted the new one using \"int\",\nstring_list_find_insert_index() that finds an existing entry or the\nlocation a new entry should be inserted into uses \"int\"), yet it\ndeclares the size of the array of the string using \"size_t\".\n\nThose index-yielding API functions (and internal implementation\ndetails) in string_list should be using \"size_t\" to express where in\nthe array they want to point at, or \"int\" that may be a lot shorter\n(and has only half the range of \"unsigned\" on the positive side)\nwould never be adequate.  Or change the .nr member to \"int\" (of\ncourse, the code that grows the array must be careful not to\noverflow .nr and let it wraparound---but the code must be careful no\nmatter what type it is; declaring \"size_t nr;\" alone does not fix\nanything).\n\nThe string_list API must be fixed first before fixing the calling\nprograms like this one, I would think.\n\nI'll stop here, as all the other changes to this file were due to\nthe misdesign of the string_list API.\n\nPlease do not get discouraged by _our_ code being sloppy and GSoC\nmicroproject ideas page being under-specified.  Neither is your\nfault.\n\nAnd welcome to Git development community.\n\n\n\n\n[Footnote]\n\nQuite honestly, -Wsign-compare is mostly garbage [*] and I wish we\ndid not add it to the developer settings.  A more effective way to\nsquelch them is not by sprinkling the casts like this, but to remove\nit from config.mak.dev ;-)\n\nhttps://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/\n\n"}]}