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

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 30, 2021, 20:58 UTC
Message-ID: <YS1GXNoPgjIjO3B7@nand.local>
In-Reply-To: <xmqqpmtu3bbn.fsf@gitster.g>

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

Show 9 quoted lines

> "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 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/