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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2021, 17:09 UTC
Message-ID: <xmqqczmqajdk.fsf@gitster.g>
In-Reply-To: <CAFQ2z_PE7TMj=qfQVroK_gRfZk-xF9PKhk2yxqF-bB+2aA7eoQ@mail.gmail.com>

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

Show 10 quoted lines

> 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().

Show 12 quoted lines

>> 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.