Re: [PATCH v3 08/27] revisions API users: add "goto cleanup" for "rev_info" early exit

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Mar 25 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> It would be a lot cleaner to be able to initialize "struct rev_info"
>> with "{ 0 }" here, or if a "REV_INFO_INIT" existed, we'll hopefully
>> get around to making the initialization easier in the future (now it
>> can't be done via a macro).
>
> If "struct rev_info" can be initialized with "{ 0 }" here, i.e.
>
> -	struct rev_info rev;
> +	struct rev_info rev = { 0 };
>
> to give us a valid solution, why wouldn't you be able to do
>
> +#define REV_INFO_INIT { 0 }
>
> elsewhere in a common *.h file, and then
>
> -	struct rev_info rev;
> +	struct rev_info rev = REV_INFO_INIT;
>
> to make the fact that "rev" is initialized (and ready to be handed
> to the releaser) even more explicit?  It's like arguing against
> fixing a code like this:
>
> 	struct char *pointer;
> 	...
> 	if (condition)
> 		pointer = malloc(...);
> 	...
>         /* pointer leaks */
>  
> by initializing
>
> 	struct char *pointer = NULL;
> 	...
> 	if (condition)
> 		pointer = malloc(...);
> 	...
>         free(pointer);
>
> because for some reason you are against the macro NULL but you are
> willing to spell it out as "0" (without double-quotes)?

I was fine with having it be { 0 } and have "release" functions
everywhere assume that memset-ing structures to zero makes them safe to
pass to release(), but my reading of your v2 feedback was that you
didn't like making that assumption, so I changed it in the v3 so we
wouldn't assume that.

I then took that REV_INFO_INIT suggestion to mean that this series
could/should be predicated on some cross-codebase refactoring to change
all of the "struct rev_info" initialization, which would be a rather
large digression.

Because I don't see how it makes any sense to have a REV_INFO_INIT if it
doesn't actually give you an init'd "struct rev_info" that's ready for
use. I.e. if you still need to call repo_init_revisions() it's just a
misleading interface.

I also think the cosmetic issues of initialization here really aren't
important at all, fixing these leaks is. So whatever idioms you're OK
with us using here are fine by me, as long as I can be made to clearly
understand what you want for a re-roll :)

Very preferably something that doesn't require refactoring the entirety
of "struct rev_info" init across the codebase...