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

[Han-Wen Nienhuys <hanwen@google.com>]

On Mon, Nov 22, 2021 at 11:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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

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.

> or something along the line?

sure. I'll update the message

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

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.

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

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