| Message ID | 8a1b094d54732b8b60eacb9892ab460a411bcec3.1637590855.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Inspect reflog data programmatically in more tests | expand |
"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 > '
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 <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.
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); 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 && tail -n1 actual | grep one '