Re: [PATCH v4 2/2] repo: add --all to git-repo-info

[Junio C Hamano <gitster@pobox.com>]

Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> writes:

>>> + 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.

Yes.  We can tell, and a compiler should be able to figure out, that
inside the loop nothing other than increment by one per iteration is
done to "i", and the ARRAY_SIZE(repo_info_fields) is a compile-time
constant that comfortably fits in platform natural "int", so we
know, and a compiler should know, that there is nothing to complain
about if "int i" is used there.

But the quality of implementation of -Wsign-compare may not be good
enough to figure it out.

As ARRAY_SIZE() essentially is a size_t divided by another size_t,
use of size_t is the safest solution that does not require any
braincycle to pick.

>> 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?

If you use "git repo info" only occasionally, wouldn't "git repo
info --keys", if supported, be a useful way to get a more focused
help than "git repo --help" where you have to scan the entire
document and try to find the list of keys that are supported from
there?