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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2021, 18:31 UTC
Message-ID: <xmqqfsrm9122.fsf@gitster.g>
In-Reply-To: <CAFQ2z_PhazkdC8JOqDW-=VD4iLq_==x23+fN7T-Vp9M2DoW=qw@mail.gmail.com>

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

Show 13 quoted lines

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

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.

Show 14 quoted lines

>> > 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.
>
> It's for consistency with the preceding test. I can make a separate commit.

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.