Re: [PATCH 2/4] refs: trim newline from reflog message
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 22, 2021, 22:27 UTC
- Message-ID
- <xmqq35nnddw7.fsf@gitster.g>
- In-Reply-To
- <dfb639373234a6b8d5f9110380a66ffccbe0b1d6.1637590855.git.gitgitgadget@gmail.com>
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 10 quoted lines
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes
> how write entries into the reflog. This commit standardizes how we get messages
> out of the reflog. Before, the files backend implicitly added '\n' to the end of
> reflog message on reading, which creates a subtle incompatibility with alternate
> ref storage backends, such as reftable.
>
> We address this by stripping LF from the message before we pass it to the
> user-provided callback.If this were truly "user-provided", then I'd argue that all backends should follow whatever the files backend has been doing forever---if the files added LF implicitly, others should, too, because that is pretty much what these "user-provided" callbacks have been expecting to see.
In other words, it would be a bug for newer backends to behave differently.
But I _think_ these callbacks are all under our control, and if that is the case, I am fine either way, even though I would have a strong preference not to have to change the API without a good reason, even if it is a purely internal one.
So, let's go and see if we can find a good reason in the changes we can make to the callback functions ;-)
> - end = strchr(logmsg, '\n'); > - if (end) > - *end = '\0'; > -
We could argue that the lack of LF at the end from the API output made this caller simpler, which may be a plus.
Show 10 quoted lines
> diff --git a/reflog-walk.c b/reflog-walk.c > index 8ac4b284b6b..3ee59a98d2f 100644 > --- a/reflog-walk.c > +++ b/reflog-walk.c > @@ -244,8 +244,6 @@ void get_reflog_message(struct strbuf *sb, > > info = &commit_reflog->reflogs->items[commit_reflog->recno+1]; > len = strlen(info->message); > - if (len > 0) > - len--; /* strip away trailing newline */
Likewise.
Show 12 quoted lines
> @@ -284,10 +282,10 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
> info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
> get_reflog_selector(&selector, reflog_info, dmode, force_date, 0);
> if (oneline) {
> - printf("%s: %s", selector.buf, info->message);
> + printf("%s: %s\n", selector.buf, info->message);
> }
> else {
> - printf("Reflog: %s (%s)\nReflog message: %s",
> + printf("Reflog: %s (%s)\nReflog message: %s\n",
> selector.buf, info->email, info->message);
> }This is Meh either way.
Show 78 quoted lines
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 151b0056fe5..583bbc5f8b9 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1936,17 +1936,15 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
> int tz;
> const char *p = sb->buf;
>
> - /* old SP new SP name <email> SP time TAB msg LF */
> - if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
> - parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
> + /* old SP new SP name <email> SP time TAB msg */
> + if (!sb->len || parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
> parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
> - !(email_end = strchr(p, '>')) ||
> - email_end[1] != ' ' ||
> + !(email_end = strchr(p, '>')) || email_end[1] != ' ' ||
> !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||
> !message || message[0] != ' ' ||
> - (message[1] != '+' && message[1] != '-') ||
> - !isdigit(message[2]) || !isdigit(message[3]) ||
> - !isdigit(message[4]) || !isdigit(message[5]))
> + (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) ||
> + !isdigit(message[3]) || !isdigit(message[4]) ||
> + !isdigit(message[5]))
> return 0; /* corrupt? */
> email_end[1] = '\0';
> tz = strtol(message + 1, NULL, 10);
> @@ -2038,6 +2036,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
> strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
> scanp = bp;
> endp = bp + 1;
> + strbuf_trim_trailing_newline(&sb);
> ret = show_one_reflog_ent(&sb, fn, cb_data);
> strbuf_reset(&sb);
> if (ret)
> @@ -2050,6 +2049,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
> * Process it, and we can end the loop.
> */
> strbuf_splice(&sb, 0, 0, buf, endp - buf);
> + strbuf_trim_trailing_newline(&sb);
> ret = show_one_reflog_ent(&sb, fn, cb_data);
> strbuf_reset(&sb);
> break;
> @@ -2099,7 +2099,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
> if (!logfp)
> return -1;
>
> - while (!ret && !strbuf_getwholeline(&sb, logfp, '\n'))
> + while (!ret && !strbuf_getline(&sb, logfp))
> ret = show_one_reflog_ent(&sb, fn, cb_data);
> fclose(logfp);
> strbuf_release(&sb);
> @@ -3059,18 +3059,18 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
> if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
> message, policy_cb)) {
> if (!cb->newlog)
> - printf("would prune %s", message);
> + printf("would prune %s\n", message);
> else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
> - printf("prune %s", message);
> + printf("prune %s\n", message);
> } else {
> if (cb->newlog) {
> - fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
> - oid_to_hex(ooid), oid_to_hex(noid),
> - email, timestamp, tz, message);
> + fprintf(cb->newlog, "%s %s %s %" PRItime " %+05d\t%s\n",
> + oid_to_hex(ooid), oid_to_hex(noid), email,
> + timestamp, tz, message);
> oidcpy(&cb->last_kept_oid, noid);
> }
> if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
> - printf("keep %s", message);
> + printf("keep %s\n", message);
> }
> return 0;
> }Hmmmm. I'll defer my judgment to the end.
Show 40 quoted lines
> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh > index 49718b7ea7f..a600bedf2cd 100755 > --- a/t/t1405-main-ref-store.sh > +++ b/t/t1405-main-ref-store.sh > @@ -89,13 +89,12 @@ test_expect_success 'for_each_reflog()' ' > test_expect_success 'for_each_reflog_ent()' ' > $RUN for-each-reflog-ent HEAD >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)' '
I got an impression from the proposed log message and the changes to the code (except for the refs/files-backend.c, which I only skimmed) that the idea is that the refs API stops adding LF at the end, and the callers got a matching change to compensate for the (now) missing LF. If that is the idea behind the change, why do we need to change any existing test? The only way any tests need to be modified due to such a change I can think of is when we forget to or failed to make compensating change to the callers of the API.
Puzzled...
Ah, the $RUN is hiding what is really going on; it is running the "test-tool ref-store" helper, and we did not adjust that helper. So if we make a compensating change to the test-tool then we do not have to have these changes at all? But that point may be moot.
In any case, in order to lose 5 lines from show-branch.c, and 2 lines from reflog-walk.c, I see that we had to touch 30+ lines in refs/files-backend.c. I find it a bit hard to sell this as an improvement to the API, to be honest.
Luckily, it looks to me that this step is mostly unreleated to the main thrust of these patches in the series, which is "reading .git/logs/ in the test would work only when testing files backend; use for-each-ref test helper to recreate what would have been read by such tests from the files backend's files and inspect that instead, and that would allow us test other backends for free".
So I suspect that this step can be safely dropped?
Thanks.