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

Han-Wen Nienhuys [off-list ref] writes:

On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano [off-list ref] wrote:

quoted

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

quoted

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.

On Tue, Nov 23, 2021 at 6:10 PM Junio C Hamano [off-list ref] wrote:

Han-Wen Nienhuys [off-list ref] writes:

quoted

On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano [off-list ref] wrote:

quoted

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

I'm talking about refs/refs-internal.h. It seems you want to add something like

/* The ref backend should add a '\n' relative to the message supplied
to the delete/symref/update functions. */
typedef int for_each_reflog_ent_fn(struct ref_store *ref_store,
                                   const char *refname,
                                   each_reflog_ent_fn fn,
                                   void *cb_data);

?

--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado