Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests

On Mon, Nov 29 2021, Han-Wen Nienhuys wrote:

On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

quoted

This helps for reftable support, and will help if we want to reconsider
under which conditions reflogs get created/updated.

Having looked at this in a bit more detail than last time
(https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/ (local))
I applaud the goals, but to be blunt the specific approach just seems a
bit backwards to me.

As noted in that message I have patches to tweak the "verbose" mode to
be backend-independent, which as we see from your series is one thing in
the files backend that consumes the "message" and assumes things about
newlines.

In v2, I went with Jun's suggestion, and left the newlines alone, just
trimming them in refs/debug.c .  I think that makes most of your mail
irrelevant?

To whatever immediate problem you're trying to solve? Probably. I'm
mainly pointing out that we can make some of these APIs nicer & a bit
more abstract from the details of the file backend.

Or, as is the case with "ident" v.s. "committer_name/committer_email" in
these reflog callbacks no existing caller actually cares about that
format, so if/when we get to that reftable integration (which seems to
have the two as seperate fields) splitting those up in refs.[ch]
probably makes sense.

quoted

Perhaps reftable is capable of just handing the underlying code pointers
into the mmap()'d file, so we could even skip all (re)allocations? Or if
not, that certainly seems like a sensible thing to anticipate in a
backend-independent interface. We could do that in the file backend if
we were a bit smarter and used mmap() instead of the
fopen()/read-in-a-loop pattern.

It sounds like premature optimization. Reading reflogs is not usually
a performance sensitive operation. Also, if you hand out mmap'd
pointers, how would the reftable storage code know when it is safe to
close and munmap the file?

The same way we do the fopen/fclose lifetime now, i.e. you could rely on
them for the iteration, which seems to be a common pattern in code that
needs this.

I was aiming more for the lack of premature optimization there,
i.e. instead of byte-twiddling things by injecting \0s we could just
have char */size_t offsets (as noted elsewhere), which would also nicely
allow binary storage formats, if those formats happen to have an
embedded string. Is that true of reftable?