Re: [PATCH 3/4] test-ref-store: tweaks to for-each-reflog-ent format

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2021, 22:31 UTC
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?

Show 19 quoted lines

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

Show 12 quoted lines

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