Giving priority to the reftable topic (was Re: What's cooking in git.git (Aug 2021, #06; Mon, 16))

Giving priority to the reftable topic (was Re: What's cooking in git.git (Aug 2021, #06; Mon, 16))

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> * hn/reftable (2021-08-16) 25 commits
>  - t1404: annotate test cases with REFFILES
>  ...
>  - hash.h: provide constants for the hash IDs
>
>  The "reftable" backend for the refs API.

As discussed in the thread that leads to [*1*], this topic has been
blocked by the "clean-up errno use in the refs subsystem" topic for
too long.  I think it deserves to have its own chance to be looked
at by more eyes.

I've reverted the three topics around "errno" out of 'next', while
rebasing them into a single strand of pearls, and queued them near
the tip of 'seen'.  The hn/reftable topic is merged into 'seen' 
earlier then these "errno" topics.

'seen' that has this topic, without merging known CI breakers (the
three "errno" topics are known to break when they are with the
hn/reftable topic, and the "builtin fsmonitor" also breaks CI),
passes the usual tests [*2*], except for the "pedantic" test we
recently added [*3*].

The breakage flagged by the compiler are all:

	char *fn = get_tmp_template(__FUNCTION__);

where the code expects that __FUNCTION__ is unconditionally
available.

With that problem fixed (which I would imagine should be easier than
brain surgery), we should be able to move the topic lower in 'seen',
hopefully touching 'next' soon to give it a wider exposure.

And when hn/reftable gets stable enough, the "errno clean-up" topic
can perhaps be rebased on top of it to work better together.

Thanks.


[Reference]

*1* https://lore.kernel.org/git/xmqqbl5syhiu.fsf@gitster.g/

*2* https://github.com/git/git/actions/runs/1148914175

*3* https://github.com/git/git/runs/3377289487?check_suite_focus=true#step:5:639

Re: Giving priority to the reftable topic (was Re: What's cooking in git.git (Aug 2021, #06; Mon, 16))

[Ramsay Jones]

On 20/08/2021 07:09, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> * hn/reftable (2021-08-16) 25 commits
>>  - t1404: annotate test cases with REFFILES
>>  ...
>>  - hash.h: provide constants for the hash IDs
>>
>>  The "reftable" backend for the refs API.
> 
> As discussed in the thread that leads to [*1*], this topic has been
> blocked by the "clean-up errno use in the refs subsystem" topic for
> too long.  I think it deserves to have its own chance to be looked
> at by more eyes.
> 
> I've reverted the three topics around "errno" out of 'next', while
> rebasing them into a single strand of pearls, and queued them near
> the tip of 'seen'.  The hn/reftable topic is merged into 'seen' 
> earlier then these "errno" topics.

Just a gentle reminder that this topic tickles my 'static-check.pl'
script, like so:

  $ diff nsc ssc
  ...
  88a91,98
  > reftable/generic.o	- reftable_table_seek_log
  > reftable/merged.o	- reftable_merged_table_hash_id
  > reftable/merged.o	- reftable_merged_table_min_update_index
  > reftable/merged.o	- reftable_merged_table_seek_log_at
  > reftable/publicbasics.o	- reftable_set_alloc
  > reftable/reader.o	- reader_seek
  > reftable/reader.o	- reftable_reader_seek_log_at
  > reftable/stack.o	- reftable_stack_auto_compact
  ...
  $ 

Which is to say, all of the above symbols are defined (and called) in
the '.c' file corresponding to the given object file, but not called
anywhere outside that file. I have not even looked at those functions,
but (with the possible exception of reftable_set_alloc()) they don't
strike me as 'public API functions'. So, maybe they should be marked
as 'static'?

ATB,
Ramsay Jones

Re: Giving priority to the reftable topic (was Re: What's cooking in git.git (Aug 2021, #06; Mon, 16))

[Ævar Arnfjörð Bjarmason]

References: <xmqqv945ng61.fsf@gitster.g> <xmqqtujkwsu0.fsf@gitster.g>
User-agent: Debian GNU/Linux 11 (bullseye); Emacs 27.1; mu4e 1.5.13
In-reply-to: <xmqqtujkwsu0.fsf@gitster.g>

On Thu, Aug 19 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> * hn/reftable (2021-08-16) 25 commits
>>  - t1404: annotate test cases with REFFILES
>>  ...
>>  - hash.h: provide constants for the hash IDs
>>
>>  The "reftable" backend for the refs API.
>
> As discussed in the thread that leads to [*1*], this topic has been
> blocked by the "clean-up errno use in the refs subsystem" topic for
> too long.  I think it deserves to have its own chance to be looked
> at by more eyes.
>
> I've reverted the three topics around "errno" out of 'next', while
> rebasing them into a single strand of pearls, and queued them near
> the tip of 'seen'.  The hn/reftable topic is merged into 'seen' 
> earlier then these "errno" topics.
>
> 'seen' that has this topic, without merging known CI breakers (the
> three "errno" topics are known to break when they are with the
> hn/reftable topic, and the "builtin fsmonitor" also breaks CI),
> passes the usual tests [*2*], except for the "pedantic" test we
> recently added [*3*].
>
> The breakage flagged by the compiler are all:
>
> 	char *fn = get_tmp_template(__FUNCTION__);
>
> where the code expects that __FUNCTION__ is unconditionally
> available.
>
> With that problem fixed (which I would imagine should be easier than
> brain surgery), we should be able to move the topic lower in 'seen',
> hopefully touching 'next' soon to give it a wider exposure.
>
> And when hn/reftable gets stable enough, the "errno clean-up" topic
> can perhaps be rebased on top of it to work better together.
>
> Thanks.
>
>
> [Reference]
>
> *1* https://lore.kernel.org/git/xmqqbl5syhiu.fsf@gitster.g/
>
> *2* https://github.com/git/git/actions/runs/1148914175
>
> *3* https://github.com/git/git/runs/3377289487?check_suite_focus=true#step:5:639

I sent the following yesterday in the GMail mobile app as
<CACBZZX41gwo=pv_4fn2VcLz0Qq7Av-CWsnNXfXSuzmwZnu-Q3w@mail.gmail.com>, I
did not have laptop access (vacation, back now), so it was rejected due
to inline HTML. Re-sending it here for the list's benefit:
    
    (In the Gmail app, so this'll probably not make the list, sorry, feel free
    to quote it)
    
    I've been on vacation for a couple of weeks, am back Monday.
    
    I haven't been able to look at these breakages in detail but it looked like
    there was a fix-up plus a logic error in reftable in combination with it
    with the now dead NULL parameter, perhaps something else I missed. I've
    only skimmed the list.
    
    I had a subsequent fixup topic ready on top to remove that parameter, can
    include in in a reroll. Then the segfault will be caught at compile time
    via a signature check.
    
    As I noted on list before we can do it with reftable first if you'd like,
    but I don't think the end result will be easier to review or should be fast
    tracked. We'll have the same questions about how reftable uses those ref
    APIs, but will need to review it against a basis that has more API action
    at a distance.
    
    So if you're willing to give me a few days I think d can sort it out to
    everyone's satisfaction with the refs API fixes first, if not I'll try to
    review the reftable topic again, but will probably use a local merger of it
    and my topics as the basis for that.

I hadn't seen c128427fd74 (Revert hn/refs-errno-cleanup and related
topics, 2021-08-19) yet, but in any case, I'll look at the breakages
etc. and see what can be done about them. Sorry for the
non-responsiveness for the last 2 weeks.

Re: Giving priority to the reftable topic (was Re: What's cooking in git.git (Aug 2021, #06; Mon, 16))

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 20 2021, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Aug 20, 2021, 08:09 Junio C Hamano <gitster@pobox.com> wrote:
>
>  Junio C Hamano <gitster@pobox.com> writes:
>
>  > * hn/reftable (2021-08-16) 25 commits
>  >  - t1404: annotate test cases with REFFILES
>  >  ...
>  >  - hash.h: provide constants for the hash IDs
>  >
>  >  The "reftable" backend for the refs API.
>
>  As discussed in the thread that leads to [*1*], this topic has been
>  blocked by the "clean-up errno use in the refs subsystem" topic for
>  too long.  I think it deserves to have its own chance to be looked
>  at by more eyes.
>
>  I've reverted the three topics around "errno" out of 'next', while
>  rebasing them into a single strand of pearls, and queued them near
>  the tip of 'seen'.  The hn/reftable topic is merged into 'seen' 
>  earlier then these "errno" topics.
>
>  'seen' that has this topic, without merging known CI breakers (the
>  three "errno" topics are known to break when they are with the
>  hn/reftable topic, and the "builtin fsmonitor" also breaks CI),
>  passes the usual tests [*2*], except for the "pedantic" test we
>  recently added [*3*].
>
>  The breakage flagged by the compiler are all:
>
>          char *fn = get_tmp_template(__FUNCTION__);
>
>  where the code expects that __FUNCTION__ is unconditionally
>  available.
>
>  With that problem fixed (which I would imagine should be easier than
>  brain surgery), we should be able to move the topic lower in 'seen',
>  hopefully touching 'next' soon to give it a wider exposure.
>
>  And when hn/reftable gets stable enough, the "errno clean-up" topic
>  can perhaps be rebased on top of it to work better together.
>
>  Thanks.
>
> (In the Gmail app, so this'll probably not make the list, sorry, feel free to quote it)
>
> I've been on vacation for a couple of weeks, am back Monday.
>
> I haven't been able to look at these breakages in detail but it looked like there was a fix-up plus a logic error in reftable in combination with it with
> the now dead NULL parameter, perhaps something else I missed. I've only skimmed the list.
>
> I had a subsequent fixup topic ready on top to remove that parameter, can include in in a reroll. Then the segfault will be caught at compile time via a
> signature check.
>
> As I noted on list before we can do it with reftable first if you'd like, but I don't think the end result will be easier to review or should be fast
> tracked. We'll have the same questions about how reftable uses those ref APIs, but will need to review it against a basis that has more API action at a
> distance.
>
> So if you're willing to give me a few days I think d can sort it out to everyone's satisfaction with the refs API fixes first, if not I'll try to review
> the reftable topic again, but will probably use a local merger of it and my topics as the basis for that.

I should have a re-roll of these topics ready soon, but aside from the
broken-ness of my "base" topics I don't see how the reftable topic is
anywhere near ready for "next" per my
https://lore.kernel.org/git/877dgch4rn.fsf@evledraar.gmail.com/; whether
it's based on my "base" topics or not.

>  [Reference]
>
>  *1* https://lore.kernel.org/git/xmqqbl5syhiu.fsf@gitster.g/
>
>  *2* https://github.com/git/git/actions/runs/1148914175
>
>  *3* https://github.com/git/git/runs/3377289487?check_suite_focus=true#step:5:639

Re: Giving priority to the reftable topic (was Re: What's cooking in git.git (Aug 2021, #06; Mon, 16))

[Han-Wen Nienhuys]

On Sat, Aug 21, 2021 at 12:08 AM Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
> Just a gentle reminder that this topic tickles my 'static-check.pl'
> script, like so:
>
>   $ diff nsc ssc
>   ...
>   88a91,98
>   > reftable/generic.o  - reftable_table_seek_log
>   > reftable/merged.o   - reftable_merged_table_hash_id
>   > reftable/merged.o   - reftable_merged_table_min_update_index
>   > reftable/merged.o   - reftable_merged_table_seek_log_at
>   > reftable/publicbasics.o     - reftable_set_alloc
>   > reftable/reader.o   - reader_seek
>   > reftable/reader.o   - reftable_reader_seek_log_at
>   > reftable/stack.o    - reftable_stack_auto_compact
>   ...
>   $
>
> Which is to say, all of the above symbols are defined (and called) in
> the '.c' file corresponding to the given object file, but not called
> anywhere outside that file. I have not even looked at those functions,
> but (with the possible exception of reftable_set_alloc()) they don't
> strike me as 'public API functions'. So, maybe they should be marked
> as 'static'?

They're all public except reader_seek().
I added some more coverage, in v4 of the library topic.

--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado