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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2021, 20:51 UTC
Message-ID: <xmqqpmtu3bbn.fsf@gitster.g>
In-Reply-To: <d48207d6858502f04fd501a24ff7c2a80062dfbe.1630334929.git.gitgitgadget@gmail.com>

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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.