patch, 4 partsInspect reflog data programmatically in more tests

This helps for reftable support, and will help if we want to reconsider under which conditions reflogs get created/updated.

Han-Wen Nienhuys (4):
  show-branch: show reflog message
  refs: trim newline from reflog message
  test-ref-store: tweaks to for-each-reflog-ent format
  t1400: use test-helper ref-store to inspect reflog contents

 builtin/show-branch.c          |  7 ++-----
 reflog-walk.c                  |  6 ++----
 refs/files-backend.c           | 30 +++++++++++++++---------------
 t/helper/test-ref-store.c      |  5 ++---
 t/t1400-update-ref.sh          | 18 +++++++++++-------
 t/t1405-main-ref-store.sh      |  4 ++--
 t/t1406-submodule-ref-store.sh |  4 ++--
 t/t3202-show-branch.sh         | 15 +++++++++++++++
 8 files changed, 51 insertions(+), 38 deletions(-)

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1145%2Fhanwen%2Freflog-prelims-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1145/hanwen/reflog-prelims-v1
Pull-Request: https://github.com/git/git/pull/1145

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

Before, --reflog option would look for '\t' in the reflog message. As refs.c already parses the reflog line, the '\t' was never found, and show-branch --reflog would always say "(none)" as reflog message

Add test.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 builtin/show-branch.c  | 12 +++++++-----
 t/t3202-show-branch.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

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

Well spotted. It may show that nobody pays attention to output from "show-branch -g" (or nobody runs it in the first place), but it is good to fix it anyway.

Thanks.

On 22/11/21 21.20, Han-Wen Nienhuys via GitGitGadget wrote:

Add what the test?

Why don't you use oneliner `export GIT_TEST_DATE_NOW=1251660000`?

On Mon, Nov 22, 2021 at 11:44 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:

t/check-non-portable-shell.pl will flag this construct and throw an error if you put that in your test script.

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

Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes how write entries into the reflog. This commit standardizes how we get messages out of the reflog. Before, the files backend implicitly added '\n' to the end of reflog message on reading, which creates a subtle incompatibility with alternate ref storage backends, such as reftable.

We address this by stripping LF from the message before we pass it to the user-provided callback.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 builtin/show-branch.c          |  5 -----
 reflog-walk.c                  |  6 ++----
 refs/files-backend.c           | 30 +++++++++++++++---------------
 t/t1405-main-ref-store.sh      |  5 ++---
 t/t1406-submodule-ref-store.sh |  4 ++--
 5 files changed, 21 insertions(+), 29 deletions(-)

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

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.

In other words, it would be a bug for newer backends to behave differently.

But I _think_ these callbacks are all under our control, and if that is the case, I am fine either way, even though I would have a strong preference not to have to change the API without a good reason, even if it is a purely internal one.

So, let's go and see if we can find a good reason in the changes we can make to the callback functions ;-)

We could argue that the lack of LF at the end from the API output made this caller simpler, which may be a plus.

Likewise.

This is Meh either way.

Hmmmm.  I'll defer my judgment to the end.

I got an impression from the proposed log message and the changes to the code (except for the refs/files-backend.c, which I only skimmed) that the idea is that the refs API stops adding LF at the end, and the callers got a matching change to compensate for the (now) missing LF. If that is the idea behind the change, why do we need to change any existing test? The only way any tests need to be modified due to such a change I can think of is when we forget to or failed to make compensating change to the callers of the API.

Puzzled...

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.

Luckily, it looks to me that this step is mostly unreleated to the main thrust of these patches in the series, which is "reading .git/logs/ in the test would work only when testing files backend; use for-each-ref test helper to recreate what would have been read by such tests from the files backend's files and inspect that instead, and that would allow us test other backends for free".

So I suspect that this step can be safely dropped?

Thanks.

On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:

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

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

  tail-n2  | head -n1

in order to read the last log line. I think it's silly, and should be dropped.

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

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

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?

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 <gitster@pobox.com> wrote:

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

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

Sorry, I do not follow. Doesn't the ref backend already ensure that the message is not an incomplete line? If you feed a single complete line when updating, I do not think the backend should add any extra LF relative to the given message:

    $ git update-ref -m 'creating a new branch manually
    ' refs/heads/newtest master
    $ git update-ref -m 'updating a new branch manually
    ' refs/heads/newtest master~1
    $ git reflog refs/heads/newtest
    4150a1677b refs/heads/newtest@{0}: updating a new branch manually
    5f439a0ecf refs/heads/newtest@{1}: updating the reference

I think what deserves such a comment more is the prototype for each_reflog_ent_fn describing what the parameters to that callback function, to help the callers of the iterator what to expect. That is the end-user facing part.

/*
 * Callback to process a reflog entry found by the iteration functions (see
 * below)
 */
typedef int each_reflog_ent_fn(
		struct object_id *old_oid, struct object_id *new_oid,
		const char *committer, timestamp_t timestamp,
		int tz, const char *msg, void *cb_data);

Currently it only says "Callback (see below)" but "below" has only comments about the difference between refs_for_each_reflog_ent() and refs_for_each_reflog_entreverse() functions, and does not talk about what "committer" looks like (i.e. does it give user.name equivalent, user.name plus <user.email>, or something else?), and what "msg" looks like (i.e. if a multi-line message was given when a ref was updated or created, do we get these lines intact? if it gets mangled, how? if the original was a single-liner incomplete line, do we lack the final LF?), and how tz is encoded.

I think the rule for "msg" is that:

   a multi-line message, or a message on a single incomplete-line,
   are normalized into a single complete line, and callback gets a
   single complete line.

Once these rules get specified tightly and fully, it is up to the ref(log) backends how to implement the interface. If files-backend wants to keep LF at the end of the message when storing (so that it can easily use it as record delimiter), it can do so and reuse the LF at the end of the message as part of the msg parameter to the callback. If another backend wants to drop LF at the end of the message when storing (to save space), it can do so as long as the callback function gets the LF just like the other backend.

On Tue, Nov 23, 2021 at 9:34 PM Junio C Hamano <gitster@pobox.com> wrote:

But it does, currently. As of

  commit 523fa69c36744ae6779e38614cb9bfb2be552923
  Author: Junio C Hamano <gitster@pobox.com>
  ..
  - We expect that a reflog message consists of a single line.  The
       file format used by the files backend may add a LF after the

it is the job of the files ref backend to add a LF, and the input to the ref backend is a string without trailing LF. But the files backend then produces that message with a LF, when reading back the data, eg.

$ git --version git version 2.34.0.rc2.393.gf8c9666880-goog

$ GIT_TRACE_REFS="1" git branch -m bla blub .. 12:03:59.408705 refs/debug.c:162 rename_ref: refs/heads/bla -> refs/heads/blub "Branch: renamed refs/heads/bla to refs/heads/blub": 0

$ GIT_TRACE_REFS=1 git reflog show refs/heads/blub 12:04:23.277805 refs/debug.c:294 reflog_ent refs/heads/blub (ret 0): cd3e606211bb1cf8bc57f7d76bab98cc17a150bc -> cd3e606211bb1cf8bc57f7d76bab98cc17a150bc, Han-Wen Nienhuys <hanwen@google.com> 1637751839 "Branch: renamed refs/heads/bla to refs/heads/blub "

That is not how it works today. The files backend verbatimly dumps the message supplied to it. (Maybe it should crash if there is a '\n' in the message).

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

Ah, perhaps our confusion comes from the fact that I view the ref API as a whole and do not draw a fine line of distinction between the "ref API implementation common across backends" vs "what ref files backend does". But as the implementor of one backend, you obviously have to care.

In any case, when I did that commit, I clearly meant that the normalization implemented by the patch belong to the common part to be used by all backends for uniform handling of refs. If a call to refs API in a repository that is configured to use reftable backend bypasses the normalization function introduced in that commit, that is a bug.

So, yes "ref API ensures that the message is not an incomplete line to turn 0, 1, or multiple lines input into a single line", which is why the experiments you omitted from your quote (reproduced here)

    $ git update-ref -m 'creating a new branch manually
    ' refs/heads/newtest master
    $ git update-ref -m 'updating a new branch manually
    ' refs/heads/newtest master~1
    $ git reflog refs/heads/newtest
    4150a1677b refs/heads/newtest@{0}: updating a new branch manually
    5f439a0ecf refs/heads/newtest@{1}: updating the reference

to give a complete line when recording the reflog entries does not result in an extra LF shown on the output.

As I said, if parts of refs API implementation forgets to call normalization, it is a bug. Is there different codepath other than the one that is exercised with the "git update -m ''" experiment above and you found such a bug there? It needs to be fixed.

Thanks.

On Wed, Nov 24, 2021 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote:

Having the read function return something different than what the write gets still seems odd to me. How about having copy_reflog_msg() trim trailing space and then always add LF?

The files backend can assert that the string ends in LF, and doesn't need to add LF itself.

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

Ehh, let's step back a bit.

Is there anything in the common part and files backend in ref API that needs to be changed to fix some bug? I do not see anything broken that needs to be fixed by "assert that the string ends in LF and doesn't need to add LF itself" or by "always add LF".

On Wed, Nov 24, 2021 at 9:55 PM Junio C Hamano <gitster@pobox.com> wrote:

Clearly, you and I disagree about what is logical behavior here.

I've reworked the series to fit with your opinion on the matter. PTAL.

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

Thanks.

I find both are logical and internally consistent, so I prefer a choice that requires fewer changes to what is already working.

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

I still am puzzled what you wanted to illustrate with the "git branch -m bla" trace. The call graph into the refs API looks like this:

builtin/branch.c::cmd_branch()
 -> branch.c::create_branch()
    -> refs.c::ref_transaction_update()
       -> refs.c::ref_transaction_add_update()
    -> refs.c::ref_transaction_commit()

and the message the user gave is passed through in "msg" variable without modification, when calling ref_transaction_update(), which in turn makes a call to ref_transaction_add_update(). It does this:

    struct ref_update *ref_transaction_add_update(
                    struct ref_transaction *transaction,
                    const char *refname, unsigned int flags,
                    const struct object_id *new_oid,
                    const struct object_id *old_oid,
                    const char *msg)
    {
            struct ref_update *update;

            if (transaction->state != REF_TRANSACTION_OPEN)
                    BUG("update called for transaction that is not open");

            FLEX_ALLOC_STR(update, refname, refname);
            ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
            transaction->updates[transaction->nr++] = update;

            update->flags = flags;

            if (flags & REF_HAVE_NEW)
                    oidcpy(&update->new_oid, new_oid);
            if (flags & REF_HAVE_OLD)
                    oidcpy(&update->old_oid, old_oid);
            update->msg = normalize_reflog_message(msg);
            return update;
    }

And normalize_reflog_message() calls copy_reflog_msg() to squash runs of isspace() bytes (note: that class includes LF) into a single space, and runs rtrim(), so update->msg will get a single incomplete line. As I suspected in a separate message, I think my notion of what happens in the ref API implementation common to all backends and what happens in each backend, and hence my statements about the distinction, were much fuzzier than yours, so I should say that I consider that all of the above is happening in the common part across all backends. If normalize happens in ref-files, it should happen in reftable backend, too, automatically.

The files-backend has no chance to even see an embedded LF in the message when the transaction gets committed and the backend is called, does it? So I am not sure why we should crash upon seeing a LF in the message.

In any case, it seems that as a comment to clarify the end-user facing each_reflog_ent_fn() parameters, what you quoted above from my message seems correct to me, after following the callgraph like the above. A 0-line (i.e. incomplete, like your 'bla' given to "git branch"), 1-line (i.e. a single complete line, like the message I gave to "update-ref -m" in my earlier illustration), or multi-line message given when a reflog entry is created, is normalized and when the each_reflog_ent_fn() callback is called, it is shown to the callback function as a single complete line, with a LF at the end.

Phrased without the explanation specific to this discussion, but with a bit more detail:

  The message given when a reflog entry was created, is normalized
  into a single line by turning each LF into a space, squeezing a
  run of multiple whitespaces into one space, and removing trailing
  whitespaces. The callback gets a single complete line in the 'msg'
  parameter.

perhaps?

On Wed, Nov 24, 2021 at 8:27 PM Junio C Hamano <gitster@pobox.com> wrote:

I'm trying to illustrate that (from the perspective of the ref backend API) one call inserts something without a '\n', but the call that reads the info back, gets the same data back with a '\n'. It looks confusing and inconsistent to me.

It seems fine to decide that the message should always end in a LF, but then why not do that in the normalization routine, so it is shared across all backends?

For the purpose of the debug support (GIT_TRACE_REFS=1), I would rather prefer if the message was always without LF, because the LF ruins the visual of the debug output, but that is a minor concern.

Junio C Hamano <gitster@pobox.com> writes:

Perhaps something along this line.

--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
Subject: [PATCH] refs: document callback for reflog-ent iterators

refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse() functions take a callback function that gets called with the details of each reflog entry. Its parameters were not documented beyond their names. Elaborate a bit on each of them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

On Fri, Nov 26 2021, Junio C Hamano wrote:

The Line and Attu islands beg to differ :)

Nitpicking aside, perhaps these two pargaraphs would be better as simply replaced by:

    Consult "Git internal format" in git-commit-tree(1) for
    details about the "<unix timestamp> <time zone offset>" format, and
    see show_one_reflog_ent() for the parsing function.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Much nicer; this is developer facing and reducing risk of inconsistency by incorrectly duplicating is much more important than having the info available in a single place.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Having actually read the description, I do not think "Documentation/date-formats.txt::Git internal format" is a good fit. We are describing the format of a single string there (e.g. <unix timestamp> and <time zone offset> form a single string with one SP in between), but the parameters are integral types.

Specifically because the "Git internal format" is textual, an example "+0100" given there makes it sufficiently clear that the offset is a "a sign followed by 4 digits" string, but those who need to use the value in "int tz" must know that is represented as a positive one hundred, not sixty, which does not have to be captured there for the intended audience of date-formats.txt pagelet.

Here is what I came up with

---- >8 -------- >8 -------- >8 -------- >8 -------- >8 ----
Subject: [PATCH v2] refs: document callback for reflog-ent iterators

refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse() functions take a callback function that gets called with the details of each reflog entry. Its parameters were not documented beyond their names. Elaborate a bit on each of them.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * rewrite timestamp and tz, adjusting the internal format's
   description from Documentation/date-formats.txt pagelet, as
   pointed out by Ævar.

 refs.h | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

 The indented patch shows the part that was changed from the
 previous one.

    diff --git i/refs.h w/refs.h
    index 4fa97d77cf..f6fed5c7d8 100644
    --- i/refs.h
    +++ w/refs.h
    @@ -467,13 +467,15 @@ int delete_reflog(const char *refname);
      * The committer parameter is a single string, in the form
      * "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" (without double quotes).
      *
    - * The timestamp parameter gives the seconds since epoch when the reflog
    - * entry was created.
    - *
    - * The tz parameter is an up to 4 digits integer, whose absolute value
    - * gives the hour and minute offset from GMT (the lower 2 digits are
    - * minutes, the higher 2 digits are hours).  A negative tz means west of,
    - * and a positive tz means east of GMT.
    + * The timestamp parameter gives the time when entry was created as the number
    + * of seconds since the UNIX epoch.
    + *
    + * The tz parameter gives the timezone offset for the user who created
    + * the reflog entry, and its value gives a positive or negative offset
    + * from UTC.  Its absolute value is formed by multiplying the hour
    + * part by 100 and adding the minute part.  For example, 1 hour ahead
    + * of UTC, CET == "+0100", is represented as positive one hundred (not
    + * postiive sixty).
      *
      * The msg parameter is a single complete line; a reflog message given
      * to refs_delete_ref, refs_update_ref, etc. is returned to the

On Sun, Nov 28 2021, Junio C Hamano wrote:

Thanks, this version of these docs looks good to me!

On Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:

I haven't looked deeply into this topic as a whole, but FWIW this conflicts with a topic I've got locally and was going to submit sometime after the current batch of my own ref fixes in "next".

That we've got verbose output at all in file-backend.c is a wart, and it should just be moved out if it entirely, this commit (stacked on top of various other fixes I've got in the area) does that: https://github.com/avar/git/commit/eff40d2d81b

With that change the reftable code will need to handle far less of this, as it'll be handled in builtin/reflog.c, it just needs to behave properly in the appropriate "expire reflog?" callback. I.e. the backend tells us "does this expire?", the builtin/reflog.c code consumes that and does any verbose or non-verbose (or progress etc.) output.

Now, the merge conflict between that and this looks rather trivial, but I can't help but wonder if we're going in the wrong direction here API-wise, or maybe "wrong direction" is too strong, but "sticking with the wrong patterN?".

I think your cleanup works, but wouldn't a better thing be to move this to callbacks rather than tweaking the fprintf formats?

I.e. in my version the whole body of this function has become:
	
	static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
				     const char *email, timestamp_t timestamp, int tz,
				     const char *message, void *cb_data)
	{
		struct expire_reflog_cb *cb = cb_data;
		reflog_expiry_should_prune_fn *fn = cb->should_prune_fn;
	
		if (cb->rewrite)
			ooid = &cb->last_kept_oid;
	
		if (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))
			return 0;
	
		if (cb->dry_run)
			return 0; /* --dry-run */
	
		fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s", oid_to_hex(ooid),
			oid_to_hex(noid), email, timestamp, tz, message);
		oidcpy(&cb->last_kept_oid, noid);
	
		return 0;
	}

The only file-backend specific part of it is that fprintf(). If we moved towards making that part of it be:

	write_reflog_entry_cb(oid_to_hex(ooid), oid_to_hex(noid), email, timestamp, tz, message);

Then we could lift the whole of this API to a level that makes more sense for a backed to implement.

The refs/files-backend.c shouldn't need to have one function calll the "should_prune_fn" *and* write out the data. Instead some code common to all backends should call the "should prune?", and then call the backend's "here's an entry for you to write" callback.

But maybe I'm overthinking this whole thing. I'm just wondering if we have say a sqlite reflog backend as opposed to the file/reftable backend that wants to store this data in a schema. Such a a backend would need to unpack the data, as we're sprintf() formatting function parameters before it gets passed to the backends.

On Tue, Nov 23, 2021 at 11:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

sure. In the reftable glue, I have

        if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
                /* XXX - skip writing records that were not changed. */
                err = reftable_addition_commit(add);
        } else {
                /* XXX - print something */
        }

letting the callbacks do the printing means less work for reftable.

not sure if that will work. For reftable, you have to write something (a tombstone) if you _do_ want to prune the entry.

reftable also stores this in a schema: there are separate fields for e-mail, timezone, timestamp etc.

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/

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/helper/test-ref-store.c | 5 ++---
 t/t1405-main-ref-store.sh | 1 +
 2 files changed, 3 insertions(+), 3 deletions(-)

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

There is no v$n designator on the title line, but I have this feeling that I've seen this patch before. More importantly, I remember that I found it unclear what you exactly mean "the" reflog format. Is that what the files backend stores on one line in its file? The reason I suspect that may be the answer is because I do not recall documenting "the" reflog format in Documentation/ and whatever we have historically been writing would be the most canonical and/or authoritative format.

I agree 100% with the goal.  

It seems that one line of .git/logs/HEAD looks like

<new> SP <old> SP <user> SP '<' <email> '>' SP <time> SP <zone> HT <oneline> LF

and being able to extract a line like that for given reflog entry out of any backend in a consistent way is valuable when testing different backends.

It seems that is what the new code is writing, so perhaps the first paragraph can be clarified to indicate as such.

    We have some tests that read from files in .git/logs/ hierarchy
    when checking if correct reflog entries are created, but that is
    too specific to the files backend.  Other backends like reftable
    may not store its reflog entries in such a "one line per entry"
    format.

    Update for-each-reflog-ent test helper to produce output that
    would be identical to lines in a reflog file files backend uses.
    That way, (1) the current tests can be updated to use the test
    helper to read the reflog entries instead of (parts of) reflog
    files, and perform the same inspection for correctness, and (2)
    when the ref backend is swapped to another backend, the updated
    test can be used as-is to check the correctness.

or something along the line?

Looks good to me. We might want to make the printf format conditional to add \t%s only when msg is not empty, though. Hopefully such a change would follow the reflog format even more closely to make 4/4 unnecessary?

I am not sure how this new test helps validate the change to the code.

What was different in the old output was that the timezone was not in %+05d format, and the field separator before the log message was not HT. So if this grepped for HT or +0000 to make sure we are using the format that is close to what actually is stored in the files, I would understand this change, but it is unclear what it proves to make sure that the oldest entry has recreate-main.

In fact, with the code part of the patch reverted, this new test seems to pass.

On Mon, Nov 22, 2021 at 11:31 PM Junio C Hamano <gitster@pobox.com> wrote:

you have, as part of a RFC to drop "update reflog if reflog exists". Since we didn't get consensus there, I'm offering up its predecessors separately.

sure. I'll update the message

I think the conditional formatting of \t is impractical. It makes things like

  (metadata, msg) = line.split('\t')

in Python require special casing in case msg is empty.

It's for consistency with the preceding test. I can make a separate commit.

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

Doesn't it however make it cumobersome (as we saw in 4/4 and Ævar's reaction to it) to write tests to add trailing whitespace like this, I am afraid?

Without trailing HT, a self-test of this data dumper would become trivial---just run it and compare its output with the real file in .git/refs/logs/ directory, no?

As this is only test-helper, I do not mind the deviation from the format, even though the log message claims to make it closer, to always show HT. And because the consumers of this data are only test scripts, I do not mind they are sloppier than the real-world code.

But if this were a pair of real world data producer/consumer, the consumer would be prepared to see and deal with a line that ought to have but lacks HT anyway, so I suspect that the amount of code to parse conditionally added HT is not that large.

Meaning this is an unrelated clean-up of the existing test before this series started? Sure, just like the show-branch bugfix, it would be nicer to have a separate commit for it.

Thanks.

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

This avoids inspecting the file system, which only works with the files ref backend.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/t1400-update-ref.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

On Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:

So preceding commits added a trailing tab, or was that always the case with the alternate test utility?

On Mon, Nov 22, 2021 at 4:21 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

log_ref_write_fd() only writes \t into the log message if there is a non-empty message, and previously this was checking the log file directly. The test helper tweaked in one of the previous commits unconditionally prints a \t, which we now have to deal with.

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

Nice.

Between always leaving HT after the timezone and mimicking the files-backend storage more closely by using HT as a separator only when there is actually a message, I have no strong preference, but as a "test-helper", being uniform would be more useful than being less ugly, so I think I am OK with this change (and the fact that test-helper unconditonally puts HT in hits output).

This helps for reftable support, and will help if we want to reconsider under which conditions reflogs get created/updated.

Han-Wen Nienhuys (5):
  show-branch: show reflog message
  test-ref-store: don't add newline to reflog message
  t1405: check for_each_reflog_ent_reverse() more thoroughly
  test-ref-store: tweaks to for-each-reflog-ent format
  refs/debug: trim trailing LF from reflog message

 builtin/show-branch.c          | 12 +++++++-----
 refs/debug.c                   | 10 ++++++++--
 t/helper/test-ref-store.c      |  6 +++---
 t/t1400-update-ref.sh          | 13 ++++++++-----
 t/t1405-main-ref-store.sh      |  4 ++--
 t/t1406-submodule-ref-store.sh |  4 ++--
 t/t3202-show-branch.sh         | 15 +++++++++++++++
 7 files changed, 45 insertions(+), 19 deletions(-)

base-commit: 35151cf0720460a897cde9b8039af364743240e7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1145%2Fhanwen%2Freflog-prelims-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1145/hanwen/reflog-prelims-v2
Pull-Request: https://github.com/git/git/pull/1145

Range-diff vs v1:

 1:  fd2595d370a = 1:  9d8394d8c76 show-branch: show reflog message
 2:  dfb63937323 ! 2:  4a86d212589 refs: trim newline from reflog message
     @@ Metadata
      Author: Han-Wen Nienhuys <hanwen@google.com>
      
       ## Commit message ##
     -    refs: trim newline from reflog message
     +    test-ref-store: don't add newline to reflog message
      
     -    Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes
     -    how write entries into the reflog. This commit standardizes how we get messages
     -    out of the reflog. Before, the files backend implicitly added '\n' to the end of
     -    reflog message on reading, which creates a subtle incompatibility with alternate
     -    ref storage backends, such as reftable.
     -
     -    We address this by stripping LF from the message before we pass it to the
     -    user-provided callback.
     +    The files backend produces a newline for messages automatically, so before we
     +    would print blank lines between entries.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     - ## builtin/show-branch.c ##
     -@@ builtin/show-branch.c: int cmd_show_branch(int ac, const char **av, const char *prefix)
     - 			char *logmsg;
     - 			char *nth_desc;
     - 			const char *msg;
     --			char *end;
     - 			timestamp_t timestamp;
     - 			int tz;
     - 
     -@@ builtin/show-branch.c: int cmd_show_branch(int ac, const char **av, const char *prefix)
     - 				break;
     - 			}
     - 
     --			end = strchr(logmsg, '\n');
     --			if (end)
     --				*end = '\0';
     --
     - 			msg = (*logmsg == '\0') ? "(none)" : logmsg;
     - 			reflog_msg[i] = xstrfmt("(%s) %s",
     - 						show_date(timestamp, tz,
     -
     - ## reflog-walk.c ##
     -@@ reflog-walk.c: void get_reflog_message(struct strbuf *sb,
     - 
     - 	info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
     - 	len = strlen(info->message);
     --	if (len > 0)
     --		len--; /* strip away trailing newline */
     - 	strbuf_add(sb, info->message, len);
     - }
     - 
     -@@ reflog-walk.c: void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
     - 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
     - 		get_reflog_selector(&selector, reflog_info, dmode, force_date, 0);
     - 		if (oneline) {
     --			printf("%s: %s", selector.buf, info->message);
     -+			printf("%s: %s\n", selector.buf, info->message);
     - 		}
     - 		else {
     --			printf("Reflog: %s (%s)\nReflog message: %s",
     -+			printf("Reflog: %s (%s)\nReflog message: %s\n",
     - 			       selector.buf, info->email, info->message);
     - 		}
     - 
     -
     - ## refs/files-backend.c ##
     -@@ refs/files-backend.c: static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
     - 	int tz;
     - 	const char *p = sb->buf;
     - 
     --	/* old SP new SP name <email> SP time TAB msg LF */
     --	if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
     --	    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
     -+	/* old SP new SP name <email> SP time TAB msg */
     -+	if (!sb->len || parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
     - 	    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
     --	    !(email_end = strchr(p, '>')) ||
     --	    email_end[1] != ' ' ||
     -+	    !(email_end = strchr(p, '>')) || email_end[1] != ' ' ||
     - 	    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||
     - 	    !message || message[0] != ' ' ||
     --	    (message[1] != '+' && message[1] != '-') ||
     --	    !isdigit(message[2]) || !isdigit(message[3]) ||
     --	    !isdigit(message[4]) || !isdigit(message[5]))
     -+	    (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) ||
     -+	    !isdigit(message[3]) || !isdigit(message[4]) ||
     -+	    !isdigit(message[5]))
     - 		return 0; /* corrupt? */
     - 	email_end[1] = '\0';
     - 	tz = strtol(message + 1, NULL, 10);
     -@@ refs/files-backend.c: static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
     - 				strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
     - 				scanp = bp;
     - 				endp = bp + 1;
     -+				strbuf_trim_trailing_newline(&sb);
     - 				ret = show_one_reflog_ent(&sb, fn, cb_data);
     - 				strbuf_reset(&sb);
     - 				if (ret)
     -@@ refs/files-backend.c: static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
     - 				 * Process it, and we can end the loop.
     - 				 */
     - 				strbuf_splice(&sb, 0, 0, buf, endp - buf);
     -+				strbuf_trim_trailing_newline(&sb);
     - 				ret = show_one_reflog_ent(&sb, fn, cb_data);
     - 				strbuf_reset(&sb);
     - 				break;
     -@@ refs/files-backend.c: static int files_for_each_reflog_ent(struct ref_store *ref_store,
     - 	if (!logfp)
     - 		return -1;
     - 
     --	while (!ret && !strbuf_getwholeline(&sb, logfp, '\n'))
     -+	while (!ret && !strbuf_getline(&sb, logfp))
     - 		ret = show_one_reflog_ent(&sb, fn, cb_data);
     - 	fclose(logfp);
     - 	strbuf_release(&sb);
     -@@ refs/files-backend.c: static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
     - 	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
     - 				   message, policy_cb)) {
     - 		if (!cb->newlog)
     --			printf("would prune %s", message);
     -+			printf("would prune %s\n", message);
     - 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
     --			printf("prune %s", message);
     -+			printf("prune %s\n", message);
     - 	} else {
     - 		if (cb->newlog) {
     --			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
     --				oid_to_hex(ooid), oid_to_hex(noid),
     --				email, timestamp, tz, message);
     -+			fprintf(cb->newlog, "%s %s %s %" PRItime " %+05d\t%s\n",
     -+				oid_to_hex(ooid), oid_to_hex(noid), email,
     -+				timestamp, tz, message);
     - 			oidcpy(&cb->last_kept_oid, noid);
     - 		}
     - 		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
     --			printf("keep %s", message);
     -+			printf("keep %s\n", message);
     - 	}
     + ## t/helper/test-ref-store.c ##
     +@@ t/helper/test-ref-store.c: static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
     + 		       const char *committer, timestamp_t timestamp,
     + 		       int tz, const char *msg, void *cb_data)
     + {
     +-	printf("%s %s %s %"PRItime" %d %s\n",
     +-	       oid_to_hex(old_oid), oid_to_hex(new_oid),
     +-	       committer, timestamp, tz, msg);
     ++	printf("%s %s %s %" PRItime " %d %s", oid_to_hex(old_oid),
     ++	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
       	return 0;
       }
     + 
      
       ## t/t1405-main-ref-store.sh ##
      @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog()' '
 3:  8a1b094d547 ! 3:  0319503045b test-ref-store: tweaks to for-each-reflog-ent format
     @@ Metadata
      Author: Han-Wen Nienhuys <hanwen@google.com>
      
       ## Commit message ##
     -    test-ref-store: tweaks to for-each-reflog-ent format
     +    t1405: check for_each_reflog_ent_reverse() more thoroughly
      
     -    Follow the reflog format more closely, so it can be used for comparing
     -    reflogs in tests without using inspecting files under .git/logs/
     +    If we are checking for a certain ordering, we should check that there are two
     +    entries. Do this by mirroring the preceding test.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     - ## t/helper/test-ref-store.c ##
     -@@ t/helper/test-ref-store.c: static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
     - 		       const char *committer, timestamp_t timestamp,
     - 		       int tz, const char *msg, void *cb_data)
     - {
     --	printf("%s %s %s %"PRItime" %d %s\n",
     --	       oid_to_hex(old_oid), oid_to_hex(new_oid),
     --	       committer, timestamp, tz, msg);
     -+	printf("%s %s %s %" PRItime " %+05d\t%s\n", oid_to_hex(old_oid),
     -+	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
     - 	return 0;
     - }
     - 
     -
       ## t/t1405-main-ref-store.sh ##
      @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog_ent()' '
       
 4:  4ba97a4e70a ! 4:  62f5cb8a824 t1400: use test-helper ref-store to inspect reflog contents
     @@ Metadata
      Author: Han-Wen Nienhuys <hanwen@google.com>
      
       ## Commit message ##
     -    t1400: use test-helper ref-store to inspect reflog contents
     +    test-ref-store: tweaks to for-each-reflog-ent format
      
     -    This avoids inspecting the file system, which only works with the files ref
     -    backend.
     +    We have some tests that read from files in .git/logs/ hierarchy
     +    when checking if correct reflog entries are created, but that is
     +    too specific to the files backend.  Other backends like reftable
     +    may not store its reflog entries in such a "one line per entry"
     +    format.
     +
     +    Update for-each-reflog-ent test helper to produce output that
     +    is identical to lines in a reflog file files backend uses.
     +    That way, (1) the current tests can be updated to use the test
     +    helper to read the reflog entries instead of (parts of) reflog
     +    files, and perform the same inspection for correctness, and (2)
     +    when the ref backend is swapped to another backend, the updated
     +    test can be used as-is to check the correctness.
     +
     +    Adapt t1400 to use the for-each-reflog-ent test helper.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     - ## t/t1400-update-ref.sh ##
     -@@ t/t1400-update-ref.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     - . ./test-lib.sh
     + ## t/helper/test-ref-store.c ##
     +@@ t/helper/test-ref-store.c: static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
     + 		       const char *committer, timestamp_t timestamp,
     + 		       int tz, const char *msg, void *cb_data)
     + {
     +-	printf("%s %s %s %" PRItime " %d %s", oid_to_hex(old_oid),
     +-	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
     ++	printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid),
     ++	       oid_to_hex(new_oid), committer, timestamp, tz,
     ++	       *msg == '\n' ? "" : "\t", msg);
     + 	return 0;
     + }
       
     - Z=$ZERO_OID
     -+TAB='	'
     - 
     - m=refs/heads/main
     - n_dir=refs/heads/gu
     -@@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '
     - cat >expect <<EOF
     - $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
     - $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
     --$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
     -+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB
     +
     + ## t/t1400-update-ref.sh ##
     +@@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
     + $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
       EOF
       test_expect_success "verifying $m's log (logged by touch)" '
      -	test_when_finished "rm -rf .git/$m .git/logs expect" &&
     @@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '
       '
       
       test_expect_success "create $m (logged by config)" '
     -@@ t/t1400-update-ref.sh: test_expect_success "set $m (logged by config)" '
     - cat >expect <<EOF
     - $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000	Initial Creation
     - $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
     --$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
     -+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000$TAB
     +@@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
     + $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
       EOF
       test_expect_success "verifying $m's log (logged by config)" '
      -	test_when_finished "rm -f .git/$m .git/logs/$m expect" &&
 -:  ----------- > 5:  0288e743eb2 refs/debug: trim trailing LF from reflog message

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

If we are checking for a certain ordering, we should check that there are two entries. Do this by mirroring the preceding test.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/t1405-main-ref-store.sh | 1 +
 1 file changed, 1 insertion(+)

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

On iteration, the reflog message is always terminated by a newline. Trim it to avoid clobbering the console with is this extra newline.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/debug.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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

The API promises to have only LF, not CRLF, at the end, so strbuf_trim_trailing_newline() is a bit overkill (and if payload happened to end with CR, we would lose it).

Can we use counted bytes in trace_printf()? If we can, it would be simpler to just scan "msg" for LF and then show only the span between the beginning of the string and the found LF using "%.*s", perhaps like this?

 refs/debug.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:

it would be best if there was a way to escape characters (ie. "\n" => "\\n"). Do we have a function for that?

I beg to differ - despite this being fewer lines of code, I think pointer arithmetic is best avoided if possible.

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

Mere escaping would not work in a backward compatible way, without a trick. It was envisioned that we probably could encode *and* signal the fact that the message is encoded by appending a trailing SP at the end of the message. See the log message of 523fa69c (reflog: cleanse messages in the refs.c layer, 2020-07-10) for details.

Having said that, that is about introducing a whole new reflog message format (whose use is signalled by the trailing SP), and I would prefer it to happen

 (1) after we integrate with reftable, and

 (2) implemented as an option in the normalize_reflog_message()
     function, so that no ref backends has to worry about it.

outside this topic.

I think repeated allocation and deallocation is best avoided, and that is why I recommended it. I do not see anything to fear in poiter arithmetic, as long as it is done clearly (e.g. in a narrow scope) and correctly.

If trace_printf() does not allow counted bytes "%.*s", the whole discussion is moot; I didn't go back to check.

Junio C Hamano <gitster@pobox.com> writes:

Sorry, I seem to have forgotten to give an answer to your question, and having warned sufficiently clearly that now is not the time to do it, I can safely do so. If we wanted to go the C-quote route, then the function we would use would be quote.c::quote_c_style().

Even though 523fa69c (reflog: cleanse messages in the refs.c layer, 2020-07-10) muses on use of urlencode, it shouldn't be taken as a sign that I prefer it over c-quoting, or vice versa for that matter.

It was merely an example encoding picked in the context of explaining that (1) I wasn't interested in keeping a multi-line message and spewing back as-is, and (2) but it is possible to loosen it in the future.

On Mon, Nov 29, 2021 at 8:20 PM Junio C Hamano <gitster@pobox.com> wrote:

I'm talking about the debug output, which isn't subject to compatibility guarantees.

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

There is no need to show "\n" in the debug output in the first place, no? The message part stored in the reflog database is already normalized to have each runs of whitespaces (including CR and LF) turned and squashed into a single SP, so the LF at the end is constant that the debug output can unconditionally strip without losing information.

Or are we talking about logging the _input_ to be recorded as the message for a new reflog entry? Then I agree we can do whatever, and if you prefer C-style quoting, the helpers in quote.c are your friends.

Thanks.

On Mon, Nov 29 2021, Han-Wen Nienhuys wrote:

We usually do this with pointer arithmetic, but the %.*s format doesn't require that, just code like:

    const char *str = "foobar";
    size_t len = strlen(str);
    len -= 1; /* give me less! */
    printf("%.*s", (int)len, str);

So you can also feed it (len - 1) or whatever if you know it to end with a character you don't want.

It's (more simply done as) pointer arithmetic if you're finding that end marker with strstr() or whatever, but you can also bend over backwards and get a "len" instead through other means, and in either case I think it beats reallocating the whole thing (more for readability than any optimization reasons).

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

Before, --reflog option would look for '\t' in the reflog message. As refs.c already parses the reflog line, the '\t' was never found, and show-branch --reflog would always say "(none)" as reflog message

Add test.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 builtin/show-branch.c  | 12 +++++++-----
 t/t3202-show-branch.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

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

The files backend produces a newline for messages automatically, so before we would print blank lines between entries.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/helper/test-ref-store.c      | 5 ++---
 t/t1405-main-ref-store.sh      | 5 ++---
 t/t1406-submodule-ref-store.sh | 4 ++--
 3 files changed, 6 insertions(+), 8 deletions(-)

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

It is misleading to say "The files backend produces" here. The promise the API makes to the callback functions is that msg will be a single complete line, regardless of what ref backend is used, no?

Unless we plan to break the promise to the callback, forcing them to compensate the differences of behaviour between backends, by making the reftable backend behave differently.

    The callback for each-reflog-ent iterator functions receives a
    single complete line terminated with a newline at the end in the
    'msg' parameter; by having a newline in the printf() format
    string, we have been emitting a blank line between entries.

    Fix it.

or something, I think.

And I do agree this makes it much more pleasant than before (not just this one but all the changes to the tests).

Thanks.

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

We have some tests that read from files in .git/logs/ hierarchy when checking if correct reflog entries are created, but that is too specific to the files backend. Other backends like reftable may not store its reflog entries in such a "one line per entry" format.

Update for-each-reflog-ent test helper to produce output that is identical to lines in a reflog file files backend uses. That way, (1) the current tests can be updated to use the test helper to read the reflog entries instead of (parts of) reflog files, and perform the same inspection for correctness, and (2) when the ref backend is swapped to another backend, the updated test can be used as-is to check the correctness.

Adapt t1400 to use the for-each-reflog-ent test helper.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/helper/test-ref-store.c |  5 +++--
 t/t1400-update-ref.sh     | 13 ++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

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

So, we used to show the timezone as an integer casually, but now we make sure we use the +/- prefix, with 0 filled 4 digits, that is consistent with how we record the tz in different places (like the low-level object header, or the user-visible header lines in commit and tag objects).

micronit: lose SP between redirection '>' and its target 'actual'.

And the +0000 will be checked by comparing with "expect" file.  Nice.

Ditto.

This one is good.

Thanks.

On Thu, Nov 25 2021, Han-Wen Nienhuys via GitGitGadget wrote:

Having looked at this in a bit more detail than last time (https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/) I applaud the goals, but to be blunt the specific approach just seems a bit backwards to me.

As noted in that message I have patches to tweak the "verbose" mode to be backend-independent, which as we see from your series is one thing in the files backend that consumes the "message" and assumes things about newlines.

Similarly from peeking at the reflog code I see it stores name/email separately in a struct, but the refs.[ch] API wants to pass "name <email>" as one string.

The below working patch shows that we have three consumers of that specific format, so we can just xstrfmt() that instead of xstrdup()-ing it in one case, and tweak the printf in the other two, and then we pass it as two parameters.

So it's truly backend-independent now, i.e. a "real DB" like reftable, SQL or whatever would store those in two separate fields.

So on the "seems backwards" above: If you apply this (and it passes all tests) and look at what "message" is used for you're guaranteed to find all the callers, and only a handful more of them are using that for anything.

So I'd really expect a series like this to just change the strbuf_getwholeline() in files_for_each_reflog_ent() to be a strbuf_getline(). Then flip everything downstream that expects a '\n' to either not, or to add if needed.

E.g. the test helper that's now adding a newline would be correct, and we'd just need to tweak the file backend specific stuff that's now skipping the addition of newlines to add them instead.

Anyway, changing the callbacks is quite verbose, and I just did this ident->name/email change expecting that it would be more difficult.

But likewise the big benefit is that we *are* forced to tweak all callers, so we're more likely to catch any subtle regressions, and it's thus easier to review once we get past the verbosity.

I also wonder if for name/email/message a bit more tweaking of this to make them all const char */size_t pairs wouldn't result in a much better end-state. I.e. now we read into a strbuf in a loop in files-backend.c and xstrdup() it.

Perhaps reftable is capable of just handing the underlying code pointers into the mmap()'d file, so we could even skip all (re)allocations? Or if not, that certainly seems like a sensible thing to anticipate in a backend-independent interface. We could do that in the file backend if we were a bit smarter and used mmap() instead of the fopen()/read-in-a-loop pattern.

Just my 0.02, if you're interested in running with the below assume my SOB.

On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

In v2, I went with Jun's suggestion, and left the newlines alone, just trimming them in refs/debug.c . I think that makes most of your mail irrelevant?

It sounds like premature optimization. Reading reflogs is not usually a performance sensitive operation. Also, if you hand out mmap'd pointers, how would the reftable storage code know when it is safe to close and munmap the file?

What is SOB in this context?

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

Just this part...

"Signed-off-by:"

On Mon, Nov 29 2021, Han-Wen Nienhuys wrote:

To whatever immediate problem you're trying to solve? Probably. I'm mainly pointing out that we can make some of these APIs nicer & a bit more abstract from the details of the file backend.

Or, as is the case with "ident" v.s. "committer_name/committer_email" in these reflog callbacks no existing caller actually cares about that format, so if/when we get to that reftable integration (which seems to have the two as seperate fields) splitting those up in refs.[ch] probably makes sense.

The same way we do the fopen/fclose lifetime now, i.e. you could rely on them for the iteration, which seems to be a common pattern in code that needs this.

I was aiming more for the lack of premature optimization there, i.e. instead of byte-twiddling things by injecting \0s we could just have char */size_t offsets (as noted elsewhere), which would also nicely allow binary storage formats, if those formats happen to have an embedded string. Is that true of reftable?

On Tue, Nov 30, 2021 at 12:41 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

reftable zlib compresses the reflog, so you can't mmap strings from the storage.

This helps for reftable support, and will help if we want to reconsider under which conditions reflogs get created/updated.

v3: pointer arithmetic for trimming newline.

Han-Wen Nienhuys (5):
  show-branch: show reflog message
  test-ref-store: don't add newline to reflog message
  t1405: check for_each_reflog_ent_reverse() more thoroughly
  test-ref-store: tweaks to for-each-reflog-ent format
  refs/debug: trim trailing LF from reflog message

 builtin/show-branch.c          | 12 +++++++-----
 refs/debug.c                   |  7 +++++--
 t/helper/test-ref-store.c      |  6 +++---
 t/t1400-update-ref.sh          | 13 ++++++++-----
 t/t1405-main-ref-store.sh      |  4 ++--
 t/t1406-submodule-ref-store.sh |  4 ++--
 t/t3202-show-branch.sh         | 15 +++++++++++++++
 7 files changed, 42 insertions(+), 19 deletions(-)

base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1145%2Fhanwen%2Freflog-prelims-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1145/hanwen/reflog-prelims-v3
Pull-Request: https://github.com/git/git/pull/1145

Range-diff vs v2:

 1:  9d8394d8c76 = 1:  6e94a6fbe05 show-branch: show reflog message
 2:  4a86d212589 ! 2:  062481ffece test-ref-store: don't add newline to reflog message
     @@ Metadata
       ## Commit message ##
          test-ref-store: don't add newline to reflog message
      
     -    The files backend produces a newline for messages automatically, so before we
     -    would print blank lines between entries.
     +    By convention, reflog messages always end in '\n', so
     +    before we would print blank lines between entries.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
 3:  0319503045b = 3:  46a8e13a5e6 t1405: check for_each_reflog_ent_reverse() more thoroughly
 4:  62f5cb8a824 ! 4:  40ba92dbf0d test-ref-store: tweaks to for-each-reflog-ent format
     @@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 11171502
      -	test_when_finished "rm -rf .git/$m .git/logs expect" &&
      -	test_cmp expect .git/logs/$m
      +	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
     -+	test-tool ref-store main for-each-reflog-ent $m > actual &&
     ++	test-tool ref-store main for-each-reflog-ent $m >actual &&
      +	test_cmp actual expect
       '
       
     @@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 11171503
      -	test_when_finished "rm -f .git/$m .git/logs/$m expect" &&
      -	test_cmp expect .git/logs/$m
      +	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
     -+	test-tool ref-store main for-each-reflog-ent $m > actual &&
     ++	test-tool ref-store main for-each-reflog-ent $m >actual &&
      +	test_cmp actual expect
       '
       
 5:  0288e743eb2 ! 5:  751f713a025 refs/debug: trim trailing LF from reflog message
     @@ refs/debug.c: static int debug_print_reflog_ent(struct object_id *old_oid,
       	int ret;
       	char o[GIT_MAX_HEXSZ + 1] = "null";
       	char n[GIT_MAX_HEXSZ + 1] = "null";
     -+	struct strbuf trimmed = STRBUF_INIT;
     ++	char *msgend = strchrnul(msg, '\n');
       	if (old_oid)
       		oid_to_hex_r(o, old_oid);
       	if (new_oid)
     - 		oid_to_hex_r(n, new_oid);
     +@@ refs/debug.c: static int debug_print_reflog_ent(struct object_id *old_oid,
       
     -+	strbuf_addstr(&trimmed, msg);
       	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
       		      dbg->cb_data);
      -	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
      -		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
     -+	strbuf_trim_trailing_newline(&trimmed);
      +	trace_printf_key(&trace_refs,
     -+			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
     ++			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
      +			 dbg->refname, ret, o, n, committer,
     -+			 (long int)timestamp, trimmed.buf);
     -+	strbuf_release(&trimmed);
     ++			 (long int)timestamp, (int)(msgend - msg), msg);
       	return ret;
       }
       

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

Before, --reflog option would look for '\t' in the reflog message. As refs.c already parses the reflog line, the '\t' was never found, and show-branch --reflog would always say "(none)" as reflog message

Add test.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 builtin/show-branch.c  | 12 +++++++-----
 t/t3202-show-branch.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

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

On iteration, the reflog message is always terminated by a newline. Trim it to avoid clobbering the console with is this extra newline.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/debug.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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

We have some tests that read from files in .git/logs/ hierarchy when checking if correct reflog entries are created, but that is too specific to the files backend. Other backends like reftable may not store its reflog entries in such a "one line per entry" format.

Update for-each-reflog-ent test helper to produce output that is identical to lines in a reflog file files backend uses. That way, (1) the current tests can be updated to use the test helper to read the reflog entries instead of (parts of) reflog files, and perform the same inspection for correctness, and (2) when the ref backend is swapped to another backend, the updated test can be used as-is to check the correctness.

Adapt t1400 to use the for-each-reflog-ent test helper.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/helper/test-ref-store.c |  5 +++--
 t/t1400-update-ref.sh     | 13 ++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

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

If we are checking for a certain ordering, we should check that there are two entries. Do this by mirroring the preceding test.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/t1405-main-ref-store.sh | 1 +
 1 file changed, 1 insertion(+)

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

By convention, reflog messages always end in '\n', so before we would print blank lines between entries.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/helper/test-ref-store.c      | 5 ++---
 t/t1405-main-ref-store.sh      | 5 ++---
 t/t1406-submodule-ref-store.sh | 4 ++--
 3 files changed, 6 insertions(+), 8 deletions(-)