Re: [PATCH 2/4] refs: trim newline from reflog message

[Junio C Hamano <gitster@pobox.com>]

Han-Wen Nienhuys <hanwen@google.com> writes:

> On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> If this were truly "user-provided", then I'd argue that all backends
>> should follow whatever the files backend has been doing forever---if
>> the files added LF implicitly, others should, too, because that is
>> pretty much what these "user-provided" callbacks have been expecting
>> to see.
>
> I think it's just wrong. If you pass `msg` to a storage API, you
> should get `msg` when you read it back, not (msg + "\n").

If you give a log message "git commit -m 'single line'", you get LF
at the end of the commit message for free.  This is no different.
And you know that this is not a "storage API" that stores the input
in verbatim after looking at refs.c::copy_reflog_msg().

>> Ah, the $RUN is hiding what is really going on; it is running the
>> "test-tool ref-store" helper, and we did not adjust that helper.  So
>> if we make a compensating change to the test-tool then we do not
>> have to have these changes at all?  But that point may be moot.
>>
>> In any case, in order to lose 5 lines from show-branch.c, and 2
>> lines from reflog-walk.c, I see that we had to touch 30+ lines in
>> refs/files-backend.c.  I find it a bit hard to sell this as an
>> improvement to the API, to be honest.
>
> The test-tool ref-store adds its own '\n', so you always get a blank
> line in the output. That serves no purpose, and leads to the

But that is only because test-tool is wrong, no?  If we know that
the API gives the trailing LF, what purpose does it serve to add
another one?

>   tail-n2  | head -n1
>
> in order to read the last log line. I think it's silly, and should be dropped.

Yes, it is silly and should be dropped, but if you drop it on the
tool side, then it may become even easier to do the "instead of
reading from .git/refs/logs files, have the tool dump and inspect
that" step, no?

This being test-tool, I do not mind losing backward compatibility
that forces us a silly "tail -n 2 | head -n 1" pipeline at all.  But
if silliness comes from the test-tool thta does not work well with
the internal API, that is what should be fixed.  Surely you can
change the API and its current callers to compensate for its silliness,
but I do not think that is a good direction to go in.

Thanks.