On Thu, Jan 21 2021, Han-Wen Nienhuys wrote:
> On Thu, Jan 21, 2021 at 5:14 PM Han-Wen Nienhuys <hanwen@xxxxxxxxxx> wrote:
>>
>> On Thu, Jan 21, 2021 at 4:55 PM Ævar Arnfjörð Bjarmason
>> <avarab@xxxxxxxxx> wrote:
>> > I must say I do find it a bit more concerning in the case of the
>> > reftable series. I.e. the git_reftable_read_raw_ref() function seems
>> > like a faithful copy of the general logic (at least this part) of the
>> > refs_resolve_ref_unsafe() function in refs.c.
>> >
>> > That I can remove error checking/handling from this place means existing
>> > general logic was faithfully copied without checking that we have a test
>> > for it, or adding one. Or perhaps given the history of this codebase
>> > there's out-of-tree tests for it somewhere?
>>
>> One of the earlier iterations of this code didn't have the errno
>> handling, and it caused a bug that I tracked down to the errno
>> handling. I'm fairly sure of this, because I remember being perplexed
>> and flabbergasted at using errno as an out-of-band API mechanism. The
>> first iteration of reftable code was about a year ago, so if you
>> checkout the Git source code from that time, and remove the errno
>> assignments, you would be able to see which test exercised this code
>> path.
>>
>> Or you could check where errno is read-back (I believe ENOENT is the
>> case that has special meaning)
>
> In refs.c, there is behavior that depends on errno,
>
> if (refs_read_raw_ref(refs, refname,
> oid, &sb_refname, &read_flags)) {
> ..
> /*
> * Otherwise a missing ref is OK. But the files backend
> * may show errors besides ENOENT if there are
> * similarly-named refs.
> */
> if (errno != ENOENT &&
> errno != EISDIR &&
> errno != ENOTDIR)
> return NULL;
>
> blame tells me it has something to do with dir/file conflicts and
> commit a1c1d8170db.
I'm aware mostly of what the code is doing. Yes some of it's used, some
of it's not.
My upthread [1] wasn't clear enough, the intended message was "hey this
new major refactoring/replacement of a core git format still has zero
coverage on at least some of the code it seeks to replace" not "hey
what's this thing in refs.c doing?".
I.e. lack of test coverage on refactored code as part of a big patch
series is a lot scarier than the same lack of coverage on existing
long-established code we'll either not update, or in small incremental
updates.
So it was a gentle prod to maybe look into what the state of that in the
reftable series as a whole is and fix the coverage gaps. I'd like the
feature in git.git, but this series has been stalled for a while. I for
one would feel a lot more confident in reviewing it if I knew I'd just
discovered an outlier case.
But in any case, when I wrote[1] I didn't know about / had forgotten
"make cover". As noted in t/README that can even give pretty HTML
output. It takes ages to run so I haven't run it on your series, but I
ran it on master a couple of days ago and it flagged those (and other)
parts of refs.c.
So perhaps running it on the reftables series will find some useful
cases to make up the test difference / potential bugs.
Of course 100% line coverage isn't practical (hitting every BUG(...)),
and even 100% line coverage != 100% test coverage. But it's a very good
first stop to see glaring omissions. E.g. the refs.c cases I mentioned
where we'resetting flag "xyz" in a() and then acting on "xyz/!xyz" in
b().
1. <871ree8eto.fsf@xxxxxxxxxxxxxxxxxxx>
