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

[Junio C Hamano <gitster@pobox.com>]

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

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

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