From: Lucas Seiki Oshiro Date: Tue, 18 Nov 2025 20:16:09 GMT Subject: Re: [PATCH v4 2/2] repo: add --all to git-repo-info Message-ID: In-Reply-To: >> + for (unsigned long i = 0; i < ARRAY_SIZE(repo_info_fields); i++) { >> > I am not sure if "unsigned long i" is the type you want here. I do > not mind, and actually I prefer, a simple platform natural "int i" > for something simple like this [*], but I know other people prefer to > use "size_t" to work with ARRAY_SIZE() these days. Yeah, I also thought it an unsigned long feels out of place, but I was only following ARRAY_SIZE. Actually, I was trying to avoid a warning. In this case we have very few `repo_info_field`s and any int type would work here... I'll replace it by size_t, then. > Side note: The reason they insist using size_t here is that > "-Wsign-compare" makes the compiler complain. But I would > say that it only shows what a misguided feature > -Wsign-compare warning is, especially given that the > compiler perfectly well knows how big repo_info_fields[] > array is and the iteration cannot do any harm if done with a > signed integer smaller than size_t Perhaps if ARRAY_SIZE(repo_info_fields) is bigger than the maximum limit of the integer type, which would overflow and this for would loop forever. But, obviously this wouldn't happen here. > This would work, but the symmetry between a list of keys vs the > "--all" option is lost. > > I'd rather see something like the following after a #leftoverbits > clean-up commit: > > if (all_keys && argc) > die(_("--all and cannot be used together")); > > if (all_keys) > return print_all_fields(repo, format); > else > return print_fields(argc, argv, repo, format); I'll change it in v5. > Again, this would work for now, but maybe "git repo info --keys" > that emits these would be easier to manage. This can be left to > #leftoverbits of course. I can't see a use for it other than these tests. What about writing a helper inside t/helpers for that? > But then we have seem to have seen too many #leftoverbits material, > you might want to handle some or all of them in this series in a > reroll? I am starting to become undecided. I agree with all of them, but I think they were too much for this series... I also think that after git-repo-structure being added to repo.c I think that it deserves a patchset only for refactoring. But I'll send a v5 containing the changes directly related to this series. Thanks again. I'll send a v5 soon.