| Message ID | d48207d6858502f04fd501a24ff7c2a80062dfbe.1630334929.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Gets rid of "if reflog exists, append to it regardless of config settings" | expand |
On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh > index 49718b7ea7f..16a99382770 100755 > --- a/t/t1405-main-ref-store.sh > +++ b/t/t1405-main-ref-store.sh > @@ -88,14 +88,15 @@ test_expect_success 'for_each_reflog()' ' > > test_expect_success 'for_each_reflog_ent()' ' > $RUN for-each-reflog-ent HEAD >actual && > + cat actual && Nit; stray debugging. Thanks, Taylor
On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > Follow the reflog format more closely, so it can be used for comparing > reflogs in tests without using inspecting files under .git/logs/ > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > --- > t/helper/test-ref-store.c | 6 +++--- > t/t1405-main-ref-store.sh | 5 +++-- > t/t1406-submodule-ref-store.sh | 4 ++-- > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index b314b81a45b..d7bbb20e614 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -151,9 +151,9 @@ 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); > + const char *newline = strchr(msg, '\n') ? "" : "\n"; > + printf("%s %s %s %" PRItime " %+05d\t%s%s", oid_to_hex(old_oid), > + oid_to_hex(new_oid), committer, timestamp, tz, msg, newline); Having read the rest of the series, I did scratch my head quite a bit here, but I think the change is actually quite simple. In the files backend, show_one_reflog_ent is parsing line-wise, and each line ends with the LF. Of course, we don't expect the reflog to have messages that actually contain a newline because we cleanse them with copy_reflog_message() before writing. So really it seems like the files-backend should be calling rstrip on the message before handing it to the callback. Either that, or we could call rstrip ourselves (since the generic strchr() makes me think that the LF could appear anywhere in the string, at least on first read). Thanks, Taylor
"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 > reflogs in tests without using inspecting files under .git/logs/ I find the above written in clear language that conveys what it wants to say unclearly X-<. I am guessing, from the patch text, that the callback function to refs_for_each_reflog_ent() and its _reverse() variant takes a "msg" parameter, which is expected to include a terminating LF at its end, but some backends omits the LF, and this change is to work around it by conditionally adding (or refraining from adding) LF in the each_reflog() function that is used for testing. Is that what is going on? Or is it "even within a single backend, sometimes we get message with terminating LF and sometimes without"? In any case, it changes the output. We used to spend two output lines per record in tests for these two functions, but now we assume we get one each line per one reflog entry. Which may be OK as far as these tests are concerned, but does this leave problems with the real world callbacks? Do the real callers to for_each_reflog_ent() and for_each_reflog_ent_reverse() also now need to cater to this "we sometimes get LF at the end, we sometimes don't" differences by having a similar logic as we see in the updated each_reflog() in this patch? Shouldn't we be instead making sure that the callback functions get the msg argument in a consistent way, whether it is with or without terminating LF, instead of forcing the callers to cope with the differences like this each_reflog() function does in this patch? Thanks.
On Mon, Aug 30, 2021 at 01:51:56PM -0700, Junio C Hamano 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 > > reflogs in tests without using inspecting files under .git/logs/ > > I find the above written in clear language that conveys what it > wants to say unclearly X-<. I agree; there is unfortunately more than meets the eye for this patch, so untangling it all took me a little while. > [...] > Is that what is going on? Or is it "even within a single backend, > sometimes we get message with terminating LF and sometimes without"? Is exactly what I was wondering about in [1]. And... > Shouldn't we be instead making sure that the callback functions get > the msg argument in a consistent way, whether it is with or without > terminating LF, instead of forcing the callers to cope with the > differences like this each_reflog() function does in this patch? ...that is a much more sensible resolution than what I saw in this patch. (At the very least, strchr is confusing since we don't expect LF characters anywhere in the reflog message, and this is really dealing with how the callback is invoked, not the actual bytes-on-disk). I would probably prefer that the callback be standardized to either always or never give a trailing LF. Thanks, Taylor [1]: https://lore.kernel.org/git/YS0+MHk3svGr7d4n@nand.local/
Taylor Blau <me@ttaylorr.com> writes: >> Shouldn't we be instead making sure that the callback functions get >> the msg argument in a consistent way, whether it is with or without >> terminating LF, instead of forcing the callers to cope with the >> differences like this each_reflog() function does in this patch? > > ...that is a much more sensible resolution than what I saw in this > patch. (At the very least, strchr is confusing since we don't expect LF > characters anywhere in the reflog message, and this is really dealing > with how the callback is invoked, not the actual bytes-on-disk). > > I would probably prefer that the callback be standardized to either > always or never give a trailing LF. True, and I agree that the use of LF by on-disk reflog storage format is just as the record terminator, so I do not mind if we decide to standardize on a single incomplete line without LF. Thanks.
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index b314b81a45b..d7bbb20e614 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -151,9 +151,9 @@ 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); + const char *newline = strchr(msg, '\n') ? "" : "\n"; + printf("%s %s %s %" PRItime " %+05d\t%s%s", oid_to_hex(old_oid), + oid_to_hex(new_oid), committer, timestamp, tz, msg, newline); return 0; } diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh index 49718b7ea7f..16a99382770 100755 --- a/t/t1405-main-ref-store.sh +++ b/t/t1405-main-ref-store.sh @@ -88,14 +88,15 @@ test_expect_success 'for_each_reflog()' ' test_expect_success 'for_each_reflog_ent()' ' $RUN for-each-reflog-ent HEAD >actual && + cat actual && head -n1 actual | grep one && - tail -n2 actual | head -n1 | grep recreate-main + tail -n1 actual | grep recreate-main ' test_expect_success 'for_each_reflog_ent_reverse()' ' $RUN for-each-reflog-ent-reverse HEAD >actual && head -n1 actual | grep recreate-main && - tail -n2 actual | head -n1 | grep one + tail -n1 actual | grep one ' test_expect_success 'reflog_exists(HEAD)' ' diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh index 0a87058971e..b0365c1fee0 100755 --- a/t/t1406-submodule-ref-store.sh +++ b/t/t1406-submodule-ref-store.sh @@ -74,13 +74,13 @@ test_expect_success 'for_each_reflog()' ' test_expect_success 'for_each_reflog_ent()' ' $RUN for-each-reflog-ent HEAD >actual && head -n1 actual | grep first && - tail -n2 actual | head -n1 | grep main.to.new + tail -n1 actual | grep main.to.new ' test_expect_success 'for_each_reflog_ent_reverse()' ' $RUN for-each-reflog-ent-reverse HEAD >actual && head -n1 actual | grep main.to.new && - tail -n2 actual | head -n1 | grep first + tail -n1 actual | grep first ' test_expect_success 'reflog_exists(HEAD)' '