From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 22 Nov 2021 22:31:24 GMT
Subject: Re: [PATCH 3/4] test-ref-store: tweaks to for-each-reflog-ent format
Message-ID: <xmqqr1b7bz5v.fsf@gitster.g>
In-Reply-To: <8a1b094d54732b8b60eacb9892ab460a411bcec3.1637590855.git.gitgitgadget@gmail.com>

"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

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.

> reflogs in tests without using inspecting files under .git/logs/

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?

> 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(-)
>
> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> index b314b81a45b..0fcad9b3812 100644
> --- a/t/helper/test-ref-store.c
> +++ b/t/helper/test-ref-store.c
> @@ -151,9 +151,8 @@ 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);

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?

>  	return 0;
>  }
>  
> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
> index a600bedf2cd..76b15458409 100755
> --- a/t/t1405-main-ref-store.sh
> +++ b/t/t1405-main-ref-store.sh
> @@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '
>  
>  test_expect_success 'for_each_reflog_ent_reverse()' '
>  	$RUN for-each-reflog-ent-reverse HEAD >actual &&
> +	head -n1 actual | grep recreate-main &&

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.

>  	tail -n1 actual | grep one
>  '