On Mon, Mar 28 2022, Derrick Stolee wrote:

On 3/26/2022 1:24 AM, Junio C Hamano wrote:

quoted

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

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.

You can say

	struct something *foo = NULL;

	if (some condition)
		foo = alloc_and_init_foo();

	...

	free_and_destruct(foo);

and it is correct that "initialize with NULL" alone would not let
you use the thing as full fledged 'foo', but you can still safely
pass it to free_and_destruct() (or if "something" does not hold
external resources, it could just be free()).  A flow like this:

	struct rev_info revs = REV_INFO_INIT;

	if (!some condition)
		goto leave;
	init(&revs);
	... use revs ...
leave:
        release(&revs);

would exactly be the same thing.

In other words, you say "I do not see how it makes any sense" but to
me it looks more like what does not make sense is your argument
against what was suggested.

[re-arranging a bit]

However, now that we are intending to free rev_info structs,
we need them to be initialized with NULL pointers because
otherwise the release_revisions() method won't know which
portions were legitimately initialized and which ones were
not.

Maybe this NULL assignment happens as part of
repo_init_revisions(), but that also assumes that there is no
code path that would jump to a "leave" or "cleanup" tag before
running that initialization method (which is the broken case
that Junio mentions above).

Maybe there are tools that would identify that Junio's example
would be bad, but it is also likely that a compiler doesn't
catch that issue and tests don't cover that error condition.

It's my preference to initialize things to all-zeroes whenever
there is any chance of complexity, which is why this topic has
come to my attention on multiple threads.

Mine too, IOW I really don't like the version in my own v3 here, but
wanted to just use an initialization to { 0 } as in the v2.

But I read Junio's reply to
https://lore.kernel.org/git/220324.868rszmga6.gmgdl@evledraar.gmail.com/ (local)
as objecting to that general assumption, as we do e.g. in some of this
code:

    git grep 'struct.*=.*\{ 0 \}'

So the v3 rewrite of those was seeking a way around that which didn't
require implementing a full init-from-macro of REV_INFO_INIT.

Junio, would you be OK/prefer to basically have the v2 verison, with
just a dummy macro like this in revision.h?:

    #define REV_INFO_INIT { 0 }

Which we'd then do something more useful with down the line?

Ævar has stated in multiple threads that he prefers to not
initialize data so that static analysis tools can detect a
use-before-initialization of specific members.

Just to be clear, I prefer not doing initialization in cases where the
compiler is guaranteed to help you by skipping them, i.e. init to NULL
within a function where say 2 branchesh both init the value, but if you
were to change that the init to NULL would hide the bug.

(And I'm sure it's not something I came up with, but got pointed out to
me repeatedly in review (by Jeff King or Junio?), until I got it...)

But skipping zero-ing out a struct like "struct rev_info" is not such a
case where the compiler can really help, and we often use that struct at
a far distance from where it was init'd. So I'd really like to see us
not do that as a habit, even if say valgrind can sometimes catch it (but
only if we have 100% test coverage!).

You're probably thinking more of the case of [1]. I consider that a
pretty clear case of running with scissors, I just figured that one was
narrow enough to be OK, but it's good to have [2] instead.

1. https://lore.kernel.org/git/patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com/ (local)
2. https://lore.kernel.org/git/patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com/ (local)