Re: [PATCH v4 09/27] revisions API users: use release_revisions() needing REV_INFO_INIT

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 1, 2022, 10:31 UTC
Message-ID: <220401.86h77dnkpw.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq1qyh25a5.fsf@gitster.g>

On Thu, Mar 31 2022, Junio C Hamano wrote:

Show 26 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Use release_revisions() to various users of "struct rev_list" which
>> need to have their "struct rev_info" zero-initialized before we can
>> start using it.
>>
>> To do this add a stub "REV_INFO_INIT" macro, ideally macro would be
>> able to fully initialize a "struct rev_info", but all it does is the
>> equivalent of assigning "{ 0 }" to the struct, the API user will still
>> need to use repo_init_revisions(). In some future follow-up work we'll
>> hopefully make REV_INFO_INIT be a "stand-alone" init likke STRBUF_INIT
>> and other similar macros.
>
> I do not think we want to leave such a misleading paragraph to
> future developers.
>
> Yes, We may want to move some of what init_revisions() does to
> REV_INFO_INIT(), and for that, it helps to start using greppable
> string REV_INFO_INT early rather than { 0 } to ease such transition,
> and that is what we should be stressing, instead of ranting "it does
> not do anything, so why are we stupidly introducing a name, instead
> of writing { 0 }, which is what amounts to it anyway?" without
> explicitly saying so but hinting with words like "stub", "all it
> does is", and "will still need to".
>
> Is that some kind of passive-aggressive thing?

No, I'm not trying to be a dick. I'm just confused, sorry :)

I.e. I didn't grok what you really wanted from that REV_INFO_INIT pattern in the beginning, and I largely draft my commit messages as a way to adequately explain things to myself.

The "stub" and "not sufficient (yet!)" part of the docs is a (probably too leaky) artifact of having (after the whole thread about REV_INFO_INIT started) gotten that mostly working as a "real" initializer.

Which I figured I'd submit at some point after this lands. The grep.c case was quite tricky, but the rest look pretty easy, I just didn't finish them.

I can re-roll & omit this entirely from the commit message, and the API docs.

I just thought, and still think, that it's worth drawing the API user's attention to the fact that unlike most stand-alone *_INIT macros this one isn't sufficient to be up & running with a "struct rev_info".

Which isn't the case with {STRBUF,STRVEC}_INIT, CHILD_PROCESS_INIT, STRING_LIST_INIT_* etc. etc., i.e. if you've used any of these:

    git grep -h -o '\S+_INIT;' | sort | uniq -c | sort -nr

You're likely to expect them to "flow" a certain way when it comes to initializing the struct on the stack, but this one will be a bit of an odd case while repo_init_revisions() is still required, which seems to me to be worth explaining.

But not enough to argue about it, so whatever you'd prefer...

Show 5 quoted lines

> You cannot use get_revision() even after calling init_revisions(),
> and still need to use setup_revisions() before hand, but that does
> not mean init_revisions() is not doing its job.  It may be
> implemented as the zero-initialization right now, but it misses the
> point to put a stress on the fact that it doesn't do much now.

I don't think it's broken, having gotten most of the way towards rewriting it to be macro-init'd there are some tricky edge cases where we might always need to call a function to fully initialize it.

But per the above I thought it made sense to explain that this particular *_INIT is a bit of an odd case if you've gotten used to other such macros.