Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format

"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:

From: Han-Wen Nienhuys <redacted>

Follow the reflog format more closely, so it can be used for comparing
reflogs in tests without using inspecting files under .git/logs/

I find the above written in clear language that conveys what it
wants to say unclearly X-<.

I am guessing, from the patch text, that the callback function to
refs_for_each_reflog_ent() and its _reverse() variant takes a "msg"
parameter, which is expected to include a terminating LF at its end,
but some backends omits the LF, and this change is to work around it
by conditionally adding (or refraining from adding) LF in the
each_reflog() function that is used for testing.

Is that what is going on?  Or is it "even within a single backend,
sometimes we get message with terminating LF and sometimes without"?

In any case, it changes the output.  We used to spend two output
lines per record in tests for these two functions, but now we assume
we get one each line per one reflog entry.  Which may be OK as far
as these tests are concerned, but does this leave problems with the
real world callbacks?  Do the real callers to for_each_reflog_ent()
and for_each_reflog_ent_reverse() also now need to cater to this "we
sometimes get LF at the end, we sometimes don't" differences by
having a similar logic as we see in the updated each_reflog() in
this patch?

Shouldn't we be instead making sure that the callback functions get
the msg argument in a consistent way, whether it is with or without
terminating LF, instead of forcing the callers to cope with the
differences like this each_reflog() function does in this patch?

Thanks.

On Mon, Aug 30, 2021 at 01:51:56PM -0700, Junio C Hamano wrote:

"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:

quoted

From: Han-Wen Nienhuys <redacted>

Follow the reflog format more closely, so it can be used for comparing
reflogs in tests without using inspecting files under .git/logs/

I find the above written in clear language that conveys what it
wants to say unclearly X-<.

I agree; there is unfortunately more than meets the eye for this patch,
so untangling it all took me a little while.

[...]
Is that what is going on?  Or is it "even within a single backend,
sometimes we get message with terminating LF and sometimes without"?

Is exactly what I was wondering about in [1]. And...

Shouldn't we be instead making sure that the callback functions get
the msg argument in a consistent way, whether it is with or without
terminating LF, instead of forcing the callers to cope with the
differences like this each_reflog() function does in this patch?

...that is a much more sensible resolution than what I saw in this
patch. (At the very least, strchr is confusing since we don't expect LF
characters anywhere in the reflog message, and this is really dealing
with how the callback is invoked, not the actual bytes-on-disk).

I would probably prefer that the callback be standardized to either
always or never give a trailing LF.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/YS0+MHk3svGr7d4n@nand.local/