{"thread":{"id":"56958","subject":"[PATCH 0/4] Inspect reflog data programmatically in more tests","startedAt":"2021-11-22T14:20:59Z","lastAt":"2021-12-02T18:36:05Z","messageCount":60,"participants":["Han-Wen Nienhuys via GitGitGadget","Ævar Arnfjörð Bjarmason","Han-Wen Nienhuys","Junio C Hamano","Bagas Sanjaya","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"441949","messageId":"pull.1145.git.git.1637590855.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":null,"subject":"[PATCH 0/4] Inspect reflog data programmatically in more tests","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-22T14:20:51Z","receivedAt":"2021-11-22T14:20:59Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"This helps for reftable support, and will help if we want to reconsider\nunder which conditions reflogs get created/updated.\n\nHan-Wen Nienhuys (4):\n  show-branch: show reflog message\n  refs: trim newline from reflog message\n  test-ref-store: tweaks to for-each-reflog-ent format\n  t1400: use test-helper ref-store to inspect reflog contents\n\n builtin/show-branch.c          |  7 ++-----\n reflog-walk.c                  |  6 ++----\n refs/files-backend.c           | 30 +++++++++++++++---------------\n t/helper/test-ref-store.c      |  5 ++---\n t/t1400-update-ref.sh          | 18 +++++++++++-------\n t/t1405-main-ref-store.sh      |  4 ++--\n t/t1406-submodule-ref-store.sh |  4 ++--\n t/t3202-show-branch.sh         | 15 +++++++++++++++\n 8 files changed, 51 insertions(+), 38 deletions(-)\n\n\nbase-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1145%2Fhanwen%2Freflog-prelims-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1145/hanwen/reflog-prelims-v1\nPull-Request: https://github.com/git/git/pull/1145\n-- \ngitgitgadget\n"},{"id":"441950","messageId":"fd2595d370a8a257c44693fdc98194cd8447e22a.1637590855.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.git.git.1637590855.gitgitgadget@gmail.com","subject":"[PATCH 1/4] show-branch: show reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-22T14:20:52Z","receivedAt":"2021-11-22T14:21:00Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nBefore, --reflog option would look for '\\t' in the reflog message. As refs.c\nalready parses the reflog line, the '\\t' was never found, and show-branch\n--reflog would always say \"(none)\" as reflog message\n\nAdd test.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n builtin/show-branch.c  | 12 +++++++-----\n t/t3202-show-branch.sh | 15 +++++++++++++++\n 2 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 082449293b5..f1e8318592c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -761,6 +761,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tchar *logmsg;\n \t\t\tchar *nth_desc;\n \t\t\tconst char *msg;\n+\t\t\tchar *end;\n \t\t\ttimestamp_t timestamp;\n \t\t\tint tz;\n \n@@ -770,11 +771,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\treflog = i;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tmsg = strchr(logmsg, '\\t');\n-\t\t\tif (!msg)\n-\t\t\t\tmsg = \"(none)\";\n-\t\t\telse\n-\t\t\t\tmsg++;\n+\n+\t\t\tend = strchr(logmsg, '\\n');\n+\t\t\tif (end)\n+\t\t\t\t*end = '\\0';\n+\n+\t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n \t\t\t\t\t\tshow_date(timestamp, tz,\n \t\t\t\t\t\t\t  DATE_MODE(RELATIVE)),\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ad9902a06b9..d4d64401e4b 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -4,6 +4,9 @@ test_description='test show-branch'\n \n . ./test-lib.sh\n \n+# arbitrary reference time: 2009-08-30 19:20:00\n+GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n+\n test_expect_success 'setup' '\n \ttest_commit initial &&\n \tfor i in $(test_seq 1 10)\n@@ -146,4 +149,16 @@ test_expect_success 'show branch --merge-base with N arguments' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show branch --reflog=2' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n+\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n+\t>\t--\n+\t>\t+  [refs/heads/branch10@{0}] branch10\n+\t>\t++ [refs/heads/branch10@{1}] initial\n+\tEOF\n+\tgit show-branch --reflog=2 >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"441951","messageId":"dfb639373234a6b8d5f9110380a66ffccbe0b1d6.1637590855.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.git.git.1637590855.gitgitgadget@gmail.com","subject":"[PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-22T14:20:53Z","receivedAt":"2021-11-22T14:21:01Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nCommit 523fa69c (\"reflog: cleanse messages in the refs.c layer\") standardizes\nhow write entries into the reflog. This commit standardizes how we get messages\nout of the reflog. Before, the files backend implicitly added '\\n' to the end of\nreflog message on reading, which creates a subtle incompatibility with alternate\nref storage backends, such as reftable.\n\nWe address this by stripping LF from the message before we pass it to the\nuser-provided callback.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n builtin/show-branch.c          |  5 -----\n reflog-walk.c                  |  6 ++----\n refs/files-backend.c           | 30 +++++++++++++++---------------\n t/t1405-main-ref-store.sh      |  5 ++---\n t/t1406-submodule-ref-store.sh |  4 ++--\n 5 files changed, 21 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex f1e8318592c..016ccdafd0f 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -761,7 +761,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tchar *logmsg;\n \t\t\tchar *nth_desc;\n \t\t\tconst char *msg;\n-\t\t\tchar *end;\n \t\t\ttimestamp_t timestamp;\n \t\t\tint tz;\n \n@@ -772,10 +771,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tend = strchr(logmsg, '\\n');\n-\t\t\tif (end)\n-\t\t\t\t*end = '\\0';\n-\n \t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n \t\t\t\t\t\tshow_date(timestamp, tz,\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 8ac4b284b6b..3ee59a98d2f 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -244,8 +244,6 @@ void get_reflog_message(struct strbuf *sb,\n \n \tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n \tlen = strlen(info->message);\n-\tif (len > 0)\n-\t\tlen--; /* strip away trailing newline */\n \tstrbuf_add(sb, info->message, len);\n }\n \n@@ -284,10 +282,10 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n \t\tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n \t\tget_reflog_selector(&selector, reflog_info, dmode, force_date, 0);\n \t\tif (oneline) {\n-\t\t\tprintf(\"%s: %s\", selector.buf, info->message);\n+\t\t\tprintf(\"%s: %s\\n\", selector.buf, info->message);\n \t\t}\n \t\telse {\n-\t\t\tprintf(\"Reflog: %s (%s)\\nReflog message: %s\",\n+\t\t\tprintf(\"Reflog: %s (%s)\\nReflog message: %s\\n\",\n \t\t\t       selector.buf, info->email, info->message);\n \t\t}\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 151b0056fe5..583bbc5f8b9 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1936,17 +1936,15 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n \tint tz;\n \tconst char *p = sb->buf;\n \n-\t/* old SP new SP name <email> SP time TAB msg LF */\n-\tif (!sb->len || sb->buf[sb->len - 1] != '\\n' ||\n-\t    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||\n+\t/* old SP new SP name <email> SP time TAB msg */\n+\tif (!sb->len || parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||\n \t    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||\n-\t    !(email_end = strchr(p, '>')) ||\n-\t    email_end[1] != ' ' ||\n+\t    !(email_end = strchr(p, '>')) || email_end[1] != ' ' ||\n \t    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||\n \t    !message || message[0] != ' ' ||\n-\t    (message[1] != '+' && message[1] != '-') ||\n-\t    !isdigit(message[2]) || !isdigit(message[3]) ||\n-\t    !isdigit(message[4]) || !isdigit(message[5]))\n+\t    (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) ||\n+\t    !isdigit(message[3]) || !isdigit(message[4]) ||\n+\t    !isdigit(message[5]))\n \t\treturn 0; /* corrupt? */\n \temail_end[1] = '\\0';\n \ttz = strtol(message + 1, NULL, 10);\n@@ -2038,6 +2036,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \t\t\t\tstrbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));\n \t\t\t\tscanp = bp;\n \t\t\t\tendp = bp + 1;\n+\t\t\t\tstrbuf_trim_trailing_newline(&sb);\n \t\t\t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n \t\t\t\tstrbuf_reset(&sb);\n \t\t\t\tif (ret)\n@@ -2050,6 +2049,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \t\t\t\t * Process it, and we can end the loop.\n \t\t\t\t */\n \t\t\t\tstrbuf_splice(&sb, 0, 0, buf, endp - buf);\n+\t\t\t\tstrbuf_trim_trailing_newline(&sb);\n \t\t\t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n \t\t\t\tstrbuf_reset(&sb);\n \t\t\t\tbreak;\n@@ -2099,7 +2099,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n \tif (!logfp)\n \t\treturn -1;\n \n-\twhile (!ret && !strbuf_getwholeline(&sb, logfp, '\\n'))\n+\twhile (!ret && !strbuf_getline(&sb, logfp))\n \t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n \tfclose(logfp);\n \tstrbuf_release(&sb);\n@@ -3059,18 +3059,18 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \tif ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,\n \t\t\t\t   message, policy_cb)) {\n \t\tif (!cb->newlog)\n-\t\t\tprintf(\"would prune %s\", message);\n+\t\t\tprintf(\"would prune %s\\n\", message);\n \t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n-\t\t\tprintf(\"prune %s\", message);\n+\t\t\tprintf(\"prune %s\\n\", message);\n \t} else {\n \t\tif (cb->newlog) {\n-\t\t\tfprintf(cb->newlog, \"%s %s %s %\"PRItime\" %+05d\\t%s\",\n-\t\t\t\toid_to_hex(ooid), oid_to_hex(noid),\n-\t\t\t\temail, timestamp, tz, message);\n+\t\t\tfprintf(cb->newlog, \"%s %s %s %\" PRItime \" %+05d\\t%s\\n\",\n+\t\t\t\toid_to_hex(ooid), oid_to_hex(noid), email,\n+\t\t\t\ttimestamp, tz, message);\n \t\t\toidcpy(&cb->last_kept_oid, noid);\n \t\t}\n \t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n-\t\t\tprintf(\"keep %s\", message);\n+\t\t\tprintf(\"keep %s\\n\", message);\n \t}\n \treturn 0;\n }\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 49718b7ea7f..a600bedf2cd 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -89,13 +89,12 @@ test_expect_success 'for_each_reflog()' '\n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n \thead -n1 actual | grep one &&\n-\ttail -n2 actual | head -n1 | grep recreate-main\n+\ttail -n1 actual | grep recreate-main\n '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n-\thead -n1 actual | grep recreate-main &&\n-\ttail -n2 actual | head -n1 | grep one\n+\ttail -n1 actual | grep one\n '\n \n test_expect_success 'reflog_exists(HEAD)' '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 0a87058971e..b0365c1fee0 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -74,13 +74,13 @@ test_expect_success 'for_each_reflog()' '\n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n \thead -n1 actual | grep first &&\n-\ttail -n2 actual | head -n1 | grep main.to.new\n+\ttail -n1 actual | grep main.to.new\n '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n \thead -n1 actual | grep main.to.new &&\n-\ttail -n2 actual | head -n1 | grep first\n+\ttail -n1 actual | grep first\n '\n \n test_expect_success 'reflog_exists(HEAD)' '\n-- \ngitgitgadget\n\n"},{"id":"441952","messageId":"8a1b094d54732b8b60eacb9892ab460a411bcec3.1637590855.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.git.git.1637590855.gitgitgadget@gmail.com","subject":"[PATCH 3/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-22T14:20:54Z","receivedAt":"2021-11-22T14:21:02Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nFollow the reflog format more closely, so it can be used for comparing\nreflogs in tests without using inspecting files under .git/logs/\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/helper/test-ref-store.c | 5 ++---\n t/t1405-main-ref-store.sh | 1 +\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex b314b81a45b..0fcad9b3812 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -151,9 +151,8 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n \t\t       const char *committer, timestamp_t timestamp,\n \t\t       int tz, const char *msg, void *cb_data)\n {\n-\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n-\t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n-\t       committer, timestamp, tz, msg);\n+\tprintf(\"%s %s %s %\" PRItime \" %+05d\\t%s\\n\", oid_to_hex(old_oid),\n+\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n \treturn 0;\n }\n \ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex a600bedf2cd..76b15458409 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n+\thead -n1 actual | grep recreate-main &&\n \ttail -n1 actual | grep one\n '\n \n-- \ngitgitgadget\n\n"},{"id":"441953","messageId":"4ba97a4e70aa437f9f710746fa7a8abad0732996.1637590855.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.git.git.1637590855.gitgitgadget@gmail.com","subject":"[PATCH 4/4] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-22T14:20:55Z","receivedAt":"2021-11-22T14:21:06Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nThis avoids inspecting the file system, which only works with the files ref\nbackend.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/t1400-update-ref.sh | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 0d4f73acaa8..f91432e7a25 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -10,6 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n Z=$ZERO_OID\n+TAB='\t'\n \n m=refs/heads/main\n n_dir=refs/heads/gu\n@@ -318,11 +319,12 @@ test_expect_success 'symref empty directory removal' '\n cat >expect <<EOF\n $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tInitial Creation\n $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB\n EOF\n test_expect_success \"verifying $m's log (logged by touch)\" '\n-\ttest_when_finished \"rm -rf .git/$m .git/logs expect\" &&\n-\ttest_cmp expect .git/logs/$m\n+\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n+\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n+\ttest_cmp actual expect\n '\n \n test_expect_success \"create $m (logged by config)\" '\n@@ -347,11 +349,12 @@ test_expect_success \"set $m (logged by config)\" '\n cat >expect <<EOF\n $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000\tInitial Creation\n $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000\n+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000$TAB\n EOF\n test_expect_success \"verifying $m's log (logged by config)\" '\n-\ttest_when_finished \"rm -f .git/$m .git/logs/$m expect\" &&\n-\ttest_cmp expect .git/logs/$m\n+\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n+\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n+\ttest_cmp actual expect\n '\n \n test_expect_success 'set up for querying the reflog' '\n@@ -467,7 +470,8 @@ $h_OTHER $h_FIXED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151040 +0000\tco\n $h_FIXED $h_MERGED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151100 +0000\tcommit (merge): Merged initial commit and a later commit.\n EOF\n test_expect_success 'git commit logged updates' '\n-\ttest_cmp expect .git/logs/$m\n+\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n+\ttest_cmp expect actual\n '\n unset h_TEST h_OTHER h_FIXED h_MERGED\n \n-- \ngitgitgadget\n"},{"id":"441955","messageId":"211122.86lf1gz05q.gmgdl@evledraar.gmail.com","threadId":"56958","inReplyTo":"4ba97a4e70aa437f9f710746fa7a8abad0732996.1637590855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-22T15:20:18Z","receivedAt":"2021-11-22T15:21:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n\n>  Z=$ZERO_OID\n> +TAB='\t'\n>  \n>  m=refs/heads/main\n>  n_dir=refs/heads/gu\n> @@ -318,11 +319,12 @@ test_expect_success 'symref empty directory removal' '\n>  cat >expect <<EOF\n>  $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tInitial Creation\n>  $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n> -$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n> +$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB\n\nSo preceding commits added a trailing tab, or was that always the case\nwith the alternate test utility?\n"},{"id":"441973","messageId":"CAFQ2z_MGVh6Yb5tE7V2wSSUh4tS4aEaqELTr=XUCF3Cy55hczg@mail.gmail.com","threadId":"56958","inReplyTo":"211122.86lf1gz05q.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-22T17:07:00Z","receivedAt":"2021-11-22T17:07:14Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Mon, Nov 22, 2021 at 4:21 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n>\n> >  Z=$ZERO_OID\n> > +TAB='        '\n> >\n> >  m=refs/heads/main\n> >  n_dir=refs/heads/gu\n> > @@ -318,11 +319,12 @@ test_expect_success 'symref empty directory removal' '\n> >  cat >expect <<EOF\n> >  $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000    Initial Creation\n> >  $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000    Switch\n> > -$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n> > +$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB\n>\n> So preceding commits added a trailing tab, or was that always the case\n> with the alternate test utility?\n\nlog_ref_write_fd() only writes \\t into the log message if there is a\nnon-empty message, and previously this was checking the log file\ndirectly. The test helper tweaked in one of the previous commits\nunconditionally prints a \\t, which we now have to deal with.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442033","messageId":"xmqqlf1fde4n.fsf@gitster.g","threadId":"56958","inReplyTo":"4ba97a4e70aa437f9f710746fa7a8abad0732996.1637590855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T22:22:48Z","receivedAt":"2021-11-22T22:22:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> This avoids inspecting the file system, which only works with the files ref\n> backend.\n\nNice.\n\nBetween always leaving HT after the timezone and mimicking the\nfiles-backend storage more closely by using HT as a separator\nonly when there is actually a message, I have no strong preference,\nbut as a \"test-helper\", being uniform would be more useful than\nbeing less ugly, so I think I am OK with this change (and the fact\nthat test-helper unconditonally puts HT in hits output).\n\n\n\n>  Z=$ZERO_OID\n> +TAB='\t'\n>  \n>  m=refs/heads/main\n>  n_dir=refs/heads/gu\n> @@ -318,11 +319,12 @@ test_expect_success 'symref empty directory removal' '\n>  cat >expect <<EOF\n>  $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tInitial Creation\n>  $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n> -$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n> +$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB\n>  EOF\n>  test_expect_success \"verifying $m's log (logged by touch)\" '\n> -\ttest_when_finished \"rm -rf .git/$m .git/logs expect\" &&\n> -\ttest_cmp expect .git/logs/$m\n> +\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n> +\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n> +\ttest_cmp actual expect\n>  '\n>  \n>  test_expect_success \"create $m (logged by config)\" '\n> @@ -347,11 +349,12 @@ test_expect_success \"set $m (logged by config)\" '\n>  cat >expect <<EOF\n>  $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000\tInitial Creation\n>  $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n> -$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000\n> +$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000$TAB\n>  EOF\n>  test_expect_success \"verifying $m's log (logged by config)\" '\n> -\ttest_when_finished \"rm -f .git/$m .git/logs/$m expect\" &&\n> -\ttest_cmp expect .git/logs/$m\n> +\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n> +\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n> +\ttest_cmp actual expect\n>  '\n>  \n>  test_expect_success 'set up for querying the reflog' '\n> @@ -467,7 +470,8 @@ $h_OTHER $h_FIXED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151040 +0000\tco\n>  $h_FIXED $h_MERGED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151100 +0000\tcommit (merge): Merged initial commit and a later commit.\n>  EOF\n>  test_expect_success 'git commit logged updates' '\n> -\ttest_cmp expect .git/logs/$m\n> +\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n> +\ttest_cmp expect actual\n>  '\n>  unset h_TEST h_OTHER h_FIXED h_MERGED\n"},{"id":"442034","messageId":"xmqqee77de4c.fsf@gitster.g","threadId":"56958","inReplyTo":"fd2595d370a8a257c44693fdc98194cd8447e22a.1637590855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] show-branch: show reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T22:22:59Z","receivedAt":"2021-11-22T22:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> Before, --reflog option would look for '\\t' in the reflog message. As refs.c\n> already parses the reflog line, the '\\t' was never found, and show-branch\n> --reflog would always say \"(none)\" as reflog message\n\nWell spotted.  It may show that nobody pays attention to output from\n\"show-branch -g\" (or nobody runs it in the first place), but it is\ngood to fix it anyway.\n\nThanks.\n\n>\n> Add test.\n>\n> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n> ---\n>  builtin/show-branch.c  | 12 +++++++-----\n>  t/t3202-show-branch.sh | 15 +++++++++++++++\n>  2 files changed, 22 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 082449293b5..f1e8318592c 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -761,6 +761,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \t\t\tchar *logmsg;\n>  \t\t\tchar *nth_desc;\n>  \t\t\tconst char *msg;\n> +\t\t\tchar *end;\n>  \t\t\ttimestamp_t timestamp;\n>  \t\t\tint tz;\n>  \n> @@ -770,11 +771,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \t\t\t\treflog = i;\n>  \t\t\t\tbreak;\n>  \t\t\t}\n> -\t\t\tmsg = strchr(logmsg, '\\t');\n> -\t\t\tif (!msg)\n> -\t\t\t\tmsg = \"(none)\";\n> -\t\t\telse\n> -\t\t\t\tmsg++;\n> +\n> +\t\t\tend = strchr(logmsg, '\\n');\n> +\t\t\tif (end)\n> +\t\t\t\t*end = '\\0';\n> +\n> +\t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n>  \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n>  \t\t\t\t\t\tshow_date(timestamp, tz,\n>  \t\t\t\t\t\t\t  DATE_MODE(RELATIVE)),\n> diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\n> index ad9902a06b9..d4d64401e4b 100755\n> --- a/t/t3202-show-branch.sh\n> +++ b/t/t3202-show-branch.sh\n> @@ -4,6 +4,9 @@ test_description='test show-branch'\n>  \n>  . ./test-lib.sh\n>  \n> +# arbitrary reference time: 2009-08-30 19:20:00\n> +GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n> +\n>  test_expect_success 'setup' '\n>  \ttest_commit initial &&\n>  \tfor i in $(test_seq 1 10)\n> @@ -146,4 +149,16 @@ test_expect_success 'show branch --merge-base with N arguments' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'show branch --reflog=2' '\n> +\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n> +\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n> +\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n> +\t>\t--\n> +\t>\t+  [refs/heads/branch10@{0}] branch10\n> +\t>\t++ [refs/heads/branch10@{1}] initial\n> +\tEOF\n> +\tgit show-branch --reflog=2 >actual &&\n> +\ttest_cmp actual expect\n> +'\n> +\n>  test_done\n"},{"id":"442037","messageId":"xmqq35nnddw7.fsf@gitster.g","threadId":"56958","inReplyTo":"dfb639373234a6b8d5f9110380a66ffccbe0b1d6.1637590855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T22:27:52Z","receivedAt":"2021-11-22T22:28:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> Commit 523fa69c (\"reflog: cleanse messages in the refs.c layer\") standardizes\n> how write entries into the reflog. This commit standardizes how we get messages\n> out of the reflog. Before, the files backend implicitly added '\\n' to the end of\n> reflog message on reading, which creates a subtle incompatibility with alternate\n> ref storage backends, such as reftable.\n>\n> We address this by stripping LF from the message before we pass it to the\n> user-provided callback.\n\nIf this were truly \"user-provided\", then I'd argue that all backends\nshould follow whatever the files backend has been doing forever---if\nthe files added LF implicitly, others should, too, because that is\npretty much what these \"user-provided\" callbacks have been expecting\nto see.\n\nIn other words, it would be a bug for newer backends to behave\ndifferently.\n\nBut I _think_ these callbacks are all under our control, and if that\nis the case, I am fine either way, even though I would have a strong\npreference not to have to change the API without a good reason, even\nif it is a purely internal one.\n\nSo, let's go and see if we can find a good reason in the changes we\ncan make to the callback functions ;-)\n\n> -\t\t\tend = strchr(logmsg, '\\n');\n> -\t\t\tif (end)\n> -\t\t\t\t*end = '\\0';\n> -\n\nWe could argue that the lack of LF at the end from the API output\nmade this caller simpler, which may be a plus.\n\n> diff --git a/reflog-walk.c b/reflog-walk.c\n> index 8ac4b284b6b..3ee59a98d2f 100644\n> --- a/reflog-walk.c\n> +++ b/reflog-walk.c\n> @@ -244,8 +244,6 @@ void get_reflog_message(struct strbuf *sb,\n>  \n>  \tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n>  \tlen = strlen(info->message);\n> -\tif (len > 0)\n> -\t\tlen--; /* strip away trailing newline */\n\nLikewise.\n\n> @@ -284,10 +282,10 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n>  \t\tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n>  \t\tget_reflog_selector(&selector, reflog_info, dmode, force_date, 0);\n>  \t\tif (oneline) {\n> -\t\t\tprintf(\"%s: %s\", selector.buf, info->message);\n> +\t\t\tprintf(\"%s: %s\\n\", selector.buf, info->message);\n>  \t\t}\n>  \t\telse {\n> -\t\t\tprintf(\"Reflog: %s (%s)\\nReflog message: %s\",\n> +\t\t\tprintf(\"Reflog: %s (%s)\\nReflog message: %s\\n\",\n>  \t\t\t       selector.buf, info->email, info->message);\n>  \t\t}\n\nThis is Meh either way.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 151b0056fe5..583bbc5f8b9 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1936,17 +1936,15 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n>  \tint tz;\n>  \tconst char *p = sb->buf;\n>  \n> -\t/* old SP new SP name <email> SP time TAB msg LF */\n> -\tif (!sb->len || sb->buf[sb->len - 1] != '\\n' ||\n> -\t    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||\n> +\t/* old SP new SP name <email> SP time TAB msg */\n> +\tif (!sb->len || parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||\n>  \t    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||\n> -\t    !(email_end = strchr(p, '>')) ||\n> -\t    email_end[1] != ' ' ||\n> +\t    !(email_end = strchr(p, '>')) || email_end[1] != ' ' ||\n>  \t    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||\n>  \t    !message || message[0] != ' ' ||\n> -\t    (message[1] != '+' && message[1] != '-') ||\n> -\t    !isdigit(message[2]) || !isdigit(message[3]) ||\n> -\t    !isdigit(message[4]) || !isdigit(message[5]))\n> +\t    (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) ||\n> +\t    !isdigit(message[3]) || !isdigit(message[4]) ||\n> +\t    !isdigit(message[5]))\n>  \t\treturn 0; /* corrupt? */\n>  \temail_end[1] = '\\0';\n>  \ttz = strtol(message + 1, NULL, 10);\n> @@ -2038,6 +2036,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n>  \t\t\t\tstrbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));\n>  \t\t\t\tscanp = bp;\n>  \t\t\t\tendp = bp + 1;\n> +\t\t\t\tstrbuf_trim_trailing_newline(&sb);\n>  \t\t\t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n>  \t\t\t\tstrbuf_reset(&sb);\n>  \t\t\t\tif (ret)\n> @@ -2050,6 +2049,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n>  \t\t\t\t * Process it, and we can end the loop.\n>  \t\t\t\t */\n>  \t\t\t\tstrbuf_splice(&sb, 0, 0, buf, endp - buf);\n> +\t\t\t\tstrbuf_trim_trailing_newline(&sb);\n>  \t\t\t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n>  \t\t\t\tstrbuf_reset(&sb);\n>  \t\t\t\tbreak;\n> @@ -2099,7 +2099,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n>  \tif (!logfp)\n>  \t\treturn -1;\n>  \n> -\twhile (!ret && !strbuf_getwholeline(&sb, logfp, '\\n'))\n> +\twhile (!ret && !strbuf_getline(&sb, logfp))\n>  \t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n>  \tfclose(logfp);\n>  \tstrbuf_release(&sb);\n> @@ -3059,18 +3059,18 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n>  \tif ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,\n>  \t\t\t\t   message, policy_cb)) {\n>  \t\tif (!cb->newlog)\n> -\t\t\tprintf(\"would prune %s\", message);\n> +\t\t\tprintf(\"would prune %s\\n\", message);\n>  \t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> -\t\t\tprintf(\"prune %s\", message);\n> +\t\t\tprintf(\"prune %s\\n\", message);\n>  \t} else {\n>  \t\tif (cb->newlog) {\n> -\t\t\tfprintf(cb->newlog, \"%s %s %s %\"PRItime\" %+05d\\t%s\",\n> -\t\t\t\toid_to_hex(ooid), oid_to_hex(noid),\n> -\t\t\t\temail, timestamp, tz, message);\n> +\t\t\tfprintf(cb->newlog, \"%s %s %s %\" PRItime \" %+05d\\t%s\\n\",\n> +\t\t\t\toid_to_hex(ooid), oid_to_hex(noid), email,\n> +\t\t\t\ttimestamp, tz, message);\n>  \t\t\toidcpy(&cb->last_kept_oid, noid);\n>  \t\t}\n>  \t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> -\t\t\tprintf(\"keep %s\", message);\n> +\t\t\tprintf(\"keep %s\\n\", message);\n>  \t}\n>  \treturn 0;\n>  }\n\nHmmmm.  I'll defer my judgment to the end.\n\n> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\n> index 49718b7ea7f..a600bedf2cd 100755\n> --- a/t/t1405-main-ref-store.sh\n> +++ b/t/t1405-main-ref-store.sh\n> @@ -89,13 +89,12 @@ test_expect_success 'for_each_reflog()' '\n>  test_expect_success 'for_each_reflog_ent()' '\n>  \t$RUN for-each-reflog-ent HEAD >actual &&\n>  \thead -n1 actual | grep one &&\n> -\ttail -n2 actual | head -n1 | grep recreate-main\n> +\ttail -n1 actual | grep recreate-main\n>  '\n>  \n>  test_expect_success 'for_each_reflog_ent_reverse()' '\n>  \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n> -\thead -n1 actual | grep recreate-main &&\n> -\ttail -n2 actual | head -n1 | grep one\n> +\ttail -n1 actual | grep one\n>  '\n>  \n>  test_expect_success 'reflog_exists(HEAD)' '\n> diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\n> index 0a87058971e..b0365c1fee0 100755\n> --- a/t/t1406-submodule-ref-store.sh\n> +++ b/t/t1406-submodule-ref-store.sh\n> @@ -74,13 +74,13 @@ test_expect_success 'for_each_reflog()' '\n>  test_expect_success 'for_each_reflog_ent()' '\n>  \t$RUN for-each-reflog-ent HEAD >actual &&\n>  \thead -n1 actual | grep first &&\n> -\ttail -n2 actual | head -n1 | grep main.to.new\n> +\ttail -n1 actual | grep main.to.new\n>  '\n>  \n>  test_expect_success 'for_each_reflog_ent_reverse()' '\n>  \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n>  \thead -n1 actual | grep main.to.new &&\n> -\ttail -n2 actual | head -n1 | grep first\n> +\ttail -n1 actual | grep first\n>  '\n>  \n>  test_expect_success 'reflog_exists(HEAD)' '\n\nI got an impression from the proposed log message and the changes to\nthe code (except for the refs/files-backend.c, which I only skimmed)\nthat the idea is that the refs API stops adding LF at the end, and\nthe callers got a matching change to compensate for the (now)\nmissing LF.  If that is the idea behind the change, why do we need\nto change any existing test?  The only way any tests need to be\nmodified due to such a change I can think of is when we forget to or\nfailed to make compensating change to the callers of the API.\n\nPuzzled...\n\nAh, the $RUN is hiding what is really going on; it is running the\n\"test-tool ref-store\" helper, and we did not adjust that helper.  So\nif we make a compensating change to the test-tool then we do not\nhave to have these changes at all?  But that point may be moot.\n\nIn any case, in order to lose 5 lines from show-branch.c, and 2\nlines from reflog-walk.c, I see that we had to touch 30+ lines in\nrefs/files-backend.c.  I find it a bit hard to sell this as an\nimprovement to the API, to be honest.\n\nLuckily, it looks to me that this step is mostly unreleated to the\nmain thrust of these patches in the series, which is \"reading\n.git/logs/ in the test would work only when testing files backend;\nuse for-each-ref test helper to recreate what would have been read\nby such tests from the files backend's files and inspect that\ninstead, and that would allow us test other backends for free\".\n\nSo I suspect that this step can be safely dropped?\n\nThanks.\n\n"},{"id":"442041","messageId":"xmqqr1b7bz5v.fsf@gitster.g","threadId":"56958","inReplyTo":"8a1b094d54732b8b60eacb9892ab460a411bcec3.1637590855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T22:31:24Z","receivedAt":"2021-11-22T22:31:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> Follow the reflog format more closely, so it can be used for comparing\n\nThere is no v$n designator on the title line, but I have this\nfeeling that I've seen this patch before.  More importantly, I\nremember that I found it unclear what you exactly mean \"the\" reflog\nformat.  Is that what the files backend stores on one line in its\nfile?  The reason I suspect that may be the answer is because I do\nnot recall documenting \"the\" reflog format in Documentation/ and\nwhatever we have historically been writing would be the most\ncanonical and/or authoritative format.\n\n> reflogs in tests without using inspecting files under .git/logs/\n\nI agree 100% with the goal.  \n\nIt seems that one line of .git/logs/HEAD looks like\n\n<new> SP <old> SP <user> SP '<' <email> '>' SP <time> SP <zone> HT <oneline> LF\n\nand being able to extract a line like that for given reflog entry\nout of any backend in a consistent way is valuable when testing\ndifferent backends.\n\nIt seems that is what the new code is writing, so perhaps the first\nparagraph can be clarified to indicate as such.\n\n    We have some tests that read from files in .git/logs/ hierarchy\n    when checking if correct reflog entries are created, but that is\n    too specific to the files backend.  Other backends like reftable\n    may not store its reflog entries in such a \"one line per entry\"\n    format.\n\n    Update for-each-reflog-ent test helper to produce output that\n    would be identical to lines in a reflog file files backend uses.\n    That way, (1) the current tests can be updated to use the test\n    helper to read the reflog entries instead of (parts of) reflog\n    files, and perform the same inspection for correctness, and (2)\n    when the ref backend is swapped to another backend, the updated\n    test can be used as-is to check the correctness.\n\nor something along the line?\n\n> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n> ---\n>  t/helper/test-ref-store.c | 5 ++---\n>  t/t1405-main-ref-store.sh | 1 +\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\n> index b314b81a45b..0fcad9b3812 100644\n> --- a/t/helper/test-ref-store.c\n> +++ b/t/helper/test-ref-store.c\n> @@ -151,9 +151,8 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n>  \t\t       const char *committer, timestamp_t timestamp,\n>  \t\t       int tz, const char *msg, void *cb_data)\n>  {\n> -\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n> -\t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n> -\t       committer, timestamp, tz, msg);\n> +\tprintf(\"%s %s %s %\" PRItime \" %+05d\\t%s\\n\", oid_to_hex(old_oid),\n> +\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n\nLooks good to me.  We might want to make the printf format\nconditional to add \\t%s only when msg is not empty, though.\nHopefully such a change would follow the reflog format even more\nclosely to make 4/4 unnecessary?\n\n>  \treturn 0;\n>  }\n>  \n> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\n> index a600bedf2cd..76b15458409 100755\n> --- a/t/t1405-main-ref-store.sh\n> +++ b/t/t1405-main-ref-store.sh\n> @@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '\n>  \n>  test_expect_success 'for_each_reflog_ent_reverse()' '\n>  \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n> +\thead -n1 actual | grep recreate-main &&\n\nI am not sure how this new test helps validate the change to the\ncode.\n\nWhat was different in the old output was that the timezone was not\nin %+05d format, and the field separator before the log message was\nnot HT.  So if this grepped for HT or +0000 to make sure we are\nusing the format that is close to what actually is stored in the\nfiles, I would understand this change, but it is unclear what it\nproves to make sure that the oldest entry has recreate-main.\n\nIn fact, with the code part of the patch reverted, this new test\nseems to pass.\n\n>  \ttail -n1 actual | grep one\n>  '\n"},{"id":"442095","messageId":"3ff61d01-ddd1-6b67-d8cb-4bd22736738e@gmail.com","threadId":"56958","inReplyTo":"fd2595d370a8a257c44693fdc98194cd8447e22a.1637590855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] show-branch: show reflog message","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-11-23T07:40:56Z","receivedAt":"2021-11-23T07:41:03Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 22/11/21 21.20, Han-Wen Nienhuys via GitGitGadget wrote:\n> From: Han-Wen Nienhuys <hanwen@google.com>\n> \n> Before, --reflog option would look for '\\t' in the reflog message. As refs.c\n> already parses the reflog line, the '\\t' was never found, and show-branch\n> --reflog would always say \"(none)\" as reflog message\n> \n> Add test.\n> \n\nAdd what the test?\n\n> +# arbitrary reference time: 2009-08-30 19:20:00\n> +GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n> +\n\nWhy don't you use oneliner `export GIT_TEST_DATE_NOW=1251660000`?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"442099","messageId":"CABPp-BF+C1_XS+9FKwhwur4s5j0dUqs1VP8PDdZzw-edbfhRjQ@mail.gmail.com","threadId":"56958","inReplyTo":"3ff61d01-ddd1-6b67-d8cb-4bd22736738e@gmail.com","subject":"Re: [PATCH 1/4] show-branch: show reflog message","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-11-23T08:03:44Z","receivedAt":"2021-11-23T08:03:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 22, 2021 at 11:44 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n>\n> On 22/11/21 21.20, Han-Wen Nienhuys via GitGitGadget wrote:\n> > From: Han-Wen Nienhuys <hanwen@google.com>\n> >\n> > Before, --reflog option would look for '\\t' in the reflog message. As refs.c\n> > already parses the reflog line, the '\\t' was never found, and show-branch\n> > --reflog would always say \"(none)\" as reflog message\n> >\n> > Add test.\n> >\n>\n> Add what the test?\n>\n> > +# arbitrary reference time: 2009-08-30 19:20:00\n> > +GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n> > +\n>\n> Why don't you use oneliner `export GIT_TEST_DATE_NOW=1251660000`?\n\nt/check-non-portable-shell.pl will flag this construct and throw an\nerror if you put that in your test script.\n"},{"id":"442102","messageId":"211123.864k83w3y4.gmgdl@evledraar.gmail.com","threadId":"56958","inReplyTo":"dfb639373234a6b8d5f9110380a66ffccbe0b1d6.1637590855.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-23T10:24:12Z","receivedAt":"2021-11-23T10:40:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n> [...]\n> @@ -3059,18 +3059,18 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n>  \tif ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,\n>  \t\t\t\t   message, policy_cb)) {\n>  \t\tif (!cb->newlog)\n> -\t\t\tprintf(\"would prune %s\", message);\n> +\t\t\tprintf(\"would prune %s\\n\", message);\n>  \t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> -\t\t\tprintf(\"prune %s\", message);\n> +\t\t\tprintf(\"prune %s\\n\", message);\n>  \t} else {\n>  \t\tif (cb->newlog) {\n> -\t\t\tfprintf(cb->newlog, \"%s %s %s %\"PRItime\" %+05d\\t%s\",\n> -\t\t\t\toid_to_hex(ooid), oid_to_hex(noid),\n> -\t\t\t\temail, timestamp, tz, message);\n> +\t\t\tfprintf(cb->newlog, \"%s %s %s %\" PRItime \" %+05d\\t%s\\n\",\n> +\t\t\t\toid_to_hex(ooid), oid_to_hex(noid), email,\n> +\t\t\t\ttimestamp, tz, message);\n>  \t\t\toidcpy(&cb->last_kept_oid, noid);\n>  \t\t}\n>  \t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> -\t\t\tprintf(\"keep %s\", message);\n> +\t\t\tprintf(\"keep %s\\n\", message);\n>  \t}\n>  \treturn 0;\n>  }\n\nI haven't looked deeply into this topic as a whole, but FWIW this\nconflicts with a topic I've got locally and was going to submit sometime\nafter the current batch of my own ref fixes in \"next\".\n\nThat we've got verbose output at all in file-backend.c is a wart, and it\nshould just be moved out if it entirely, this commit (stacked on top of\nvarious other fixes I've got in the area) does that:\nhttps://github.com/avar/git/commit/eff40d2d81b\n\nWith that change the reftable code will need to handle far less of this,\nas it'll be handled in builtin/reflog.c, it just needs to behave\nproperly in the appropriate \"expire reflog?\" callback. I.e. the backend\ntells us \"does this expire?\", the builtin/reflog.c code consumes that\nand does any verbose or non-verbose (or progress etc.) output.\n\nNow, the merge conflict between that and this looks rather trivial, but\nI can't help but wonder if we're going in the wrong direction here\nAPI-wise, or maybe \"wrong direction\" is too strong, but \"sticking with\nthe wrong patterN?\".\n\nI think your cleanup works, but wouldn't a better thing be to move this\nto callbacks rather than tweaking the fprintf formats?\n\nI.e. in my version the whole body of this function has become:\n\t\n\tstatic int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n\t\t\t\t     const char *email, timestamp_t timestamp, int tz,\n\t\t\t\t     const char *message, void *cb_data)\n\t{\n\t\tstruct expire_reflog_cb *cb = cb_data;\n\t\treflog_expiry_should_prune_fn *fn = cb->should_prune_fn;\n\t\n\t\tif (cb->rewrite)\n\t\t\tooid = &cb->last_kept_oid;\n\t\n\t\tif (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))\n\t\t\treturn 0;\n\t\n\t\tif (cb->dry_run)\n\t\t\treturn 0; /* --dry-run */\n\t\n\t\tfprintf(cb->newlog, \"%s %s %s %\"PRItime\" %+05d\\t%s\", oid_to_hex(ooid),\n\t\t\toid_to_hex(noid), email, timestamp, tz, message);\n\t\toidcpy(&cb->last_kept_oid, noid);\n\t\n\t\treturn 0;\n\t}\n\nThe only file-backend specific part of it is that fprintf(). If we moved\ntowards making that part of it be:\n\n\twrite_reflog_entry_cb(oid_to_hex(ooid), oid_to_hex(noid), email, timestamp, tz, message);\n\nThen we could lift the whole of this API to a level that makes more\nsense for a backed to implement.\n\nThe refs/files-backend.c shouldn't need to have one function calll the\n\"should_prune_fn\" *and* write out the data. Instead some code common to\nall backends should call the \"should prune?\", and then call the\nbackend's \"here's an entry for you to write\" callback.\n\nBut maybe I'm overthinking this whole thing. I'm just wondering if we\nhave say a sqlite reflog backend as opposed to the file/reftable backend\nthat wants to store this data in a schema. Such a a backend would need\nto unpack the data, as we're sprintf() formatting function parameters\nbefore it gets passed to the backends.\n"},{"id":"442170","messageId":"CAFQ2z_PE7TMj=qfQVroK_gRfZk-xF9PKhk2yxqF-bB+2aA7eoQ@mail.gmail.com","threadId":"56958","inReplyTo":"xmqq35nnddw7.fsf@gitster.g","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-23T16:35:52Z","receivedAt":"2021-11-23T16:36:06Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Han-Wen Nienhuys <hanwen@google.com>\n> >\n> > Commit 523fa69c (\"reflog: cleanse messages in the refs.c layer\") standardizes\n> > how write entries into the reflog. This commit standardizes how we get messages\n> > out of the reflog. Before, the files backend implicitly added '\\n' to the end of\n> > reflog message on reading, which creates a subtle incompatibility with alternate\n> > ref storage backends, such as reftable.\n> >\n> > We address this by stripping LF from the message before we pass it to the\n> > user-provided callback.\n>\n> If this were truly \"user-provided\", then I'd argue that all backends\n> should follow whatever the files backend has been doing forever---if\n> the files added LF implicitly, others should, too, because that is\n> pretty much what these \"user-provided\" callbacks have been expecting\n> to see.\n\nI think it's just wrong. If you pass `msg` to a storage API, you\nshould get `msg` when you read it back, not (msg + \"\\n\").\n\n> Ah, the $RUN is hiding what is really going on; it is running the\n> \"test-tool ref-store\" helper, and we did not adjust that helper.  So\n> if we make a compensating change to the test-tool then we do not\n> have to have these changes at all?  But that point may be moot.\n>\n> In any case, in order to lose 5 lines from show-branch.c, and 2\n> lines from reflog-walk.c, I see that we had to touch 30+ lines in\n> refs/files-backend.c.  I find it a bit hard to sell this as an\n> improvement to the API, to be honest.\n\nThe test-tool ref-store adds its own '\\n', so you always get a blank\nline in the output. That serves no purpose, and leads to the\n\n  tail-n2  | head -n1\n\nin order to read the last log line. I think it's silly, and should be dropped.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442172","messageId":"CAFQ2z_PSS9zOzR6nGYZ8DBK+6oOQzkJsEy_7y+NprwJ1OHNs7w@mail.gmail.com","threadId":"56958","inReplyTo":"211123.864k83w3y4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-23T16:44:43Z","receivedAt":"2021-11-23T16:45:44Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Nov 23, 2021 at 11:40 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> I think your cleanup works, but wouldn't a better thing be to move this\n> to callbacks rather than tweaking the fprintf formats?\n\nsure. In the reftable glue, I have\n\n        if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n                /* XXX - skip writing records that were not changed. */\n                err = reftable_addition_commit(add);\n        } else {\n                /* XXX - print something */\n        }\n\nletting the callbacks do the printing means less work for reftable.\n\n> The refs/files-backend.c shouldn't need to have one function calll the\n> \"should_prune_fn\" *and* write out the data. Instead some code common to\n> all backends should call the \"should prune?\", and then call the\n> backend's \"here's an entry for you to write\" callback.\n\nnot sure if that will work. For reftable, you have to write something\n(a tombstone) if you _do_ want to prune the entry.\n\n> But maybe I'm overthinking this whole thing. I'm just wondering if we\n> have say a sqlite reflog backend as opposed to the file/reftable backend\n> that wants to store this data in a schema. Such a a backend would need\n\nreftable also stores this in a schema: there are separate fields for\ne-mail, timezone, timestamp etc.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442173","messageId":"CAFQ2z_PhazkdC8JOqDW-=VD4iLq_==x23+fN7T-Vp9M2DoW=qw@mail.gmail.com","threadId":"56958","inReplyTo":"xmqqr1b7bz5v.fsf@gitster.g","subject":"Re: [PATCH 3/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-23T17:06:37Z","receivedAt":"2021-11-23T17:06:52Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Mon, Nov 22, 2021 at 11:31 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Han-Wen Nienhuys <hanwen@google.com>\n> >\n> > Follow the reflog format more closely, so it can be used for comparing\n>\n> There is no v$n designator on the title line, but I have this\n> feeling that I've seen this patch before.  More importantly, I\n\nyou have, as part of a RFC to drop \"update reflog if reflog exists\".\nSince we didn't get consensus there, I'm offering up its predecessors\nseparately.\n\n> or something along the line?\n\nsure. I'll update the message\n\n> > @@ -151,9 +151,8 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n> >                      const char *committer, timestamp_t timestamp,\n> >                      int tz, const char *msg, void *cb_data)\n> >  {\n> > -     printf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n> > -            oid_to_hex(old_oid), oid_to_hex(new_oid),\n> > -            committer, timestamp, tz, msg);\n> > +     printf(\"%s %s %s %\" PRItime \" %+05d\\t%s\\n\", oid_to_hex(old_oid),\n> > +            oid_to_hex(new_oid), committer, timestamp, tz, msg);\n>\n> Looks good to me.  We might want to make the printf format\n> conditional to add \\t%s only when msg is not empty, though.\n> Hopefully such a change would follow the reflog format even more\n> closely to make 4/4 unnecessary?\n\nI think the conditional formatting of \\t is impractical. It makes things like\n\n  (metadata, msg) = line.split('\\t')\n\nin Python require special casing in case msg is empty.\n\n> > diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\n> > index a600bedf2cd..76b15458409 100755\n> > --- a/t/t1405-main-ref-store.sh\n> > +++ b/t/t1405-main-ref-store.sh\n> > @@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '\n> >\n> >  test_expect_success 'for_each_reflog_ent_reverse()' '\n> >       $RUN for-each-reflog-ent-reverse HEAD >actual &&\n> > +     head -n1 actual | grep recreate-main &&\n>\n> I am not sure how this new test helps validate the change to the\n> code.\n\nIt's for consistency with the preceding test. I can make a separate commit.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442174","messageId":"xmqqczmqajdk.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_PE7TMj=qfQVroK_gRfZk-xF9PKhk2yxqF-bB+2aA7eoQ@mail.gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-23T17:09:59Z","receivedAt":"2021-11-23T17:10:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> If this were truly \"user-provided\", then I'd argue that all backends\n>> should follow whatever the files backend has been doing forever---if\n>> the files added LF implicitly, others should, too, because that is\n>> pretty much what these \"user-provided\" callbacks have been expecting\n>> to see.\n>\n> I think it's just wrong. If you pass `msg` to a storage API, you\n> should get `msg` when you read it back, not (msg + \"\\n\").\n\nIf you give a log message \"git commit -m 'single line'\", you get LF\nat the end of the commit message for free.  This is no different.\nAnd you know that this is not a \"storage API\" that stores the input\nin verbatim after looking at refs.c::copy_reflog_msg().\n\n>> Ah, the $RUN is hiding what is really going on; it is running the\n>> \"test-tool ref-store\" helper, and we did not adjust that helper.  So\n>> if we make a compensating change to the test-tool then we do not\n>> have to have these changes at all?  But that point may be moot.\n>>\n>> In any case, in order to lose 5 lines from show-branch.c, and 2\n>> lines from reflog-walk.c, I see that we had to touch 30+ lines in\n>> refs/files-backend.c.  I find it a bit hard to sell this as an\n>> improvement to the API, to be honest.\n>\n> The test-tool ref-store adds its own '\\n', so you always get a blank\n> line in the output. That serves no purpose, and leads to the\n\nBut that is only because test-tool is wrong, no?  If we know that\nthe API gives the trailing LF, what purpose does it serve to add\nanother one?\n\n>   tail-n2  | head -n1\n>\n> in order to read the last log line. I think it's silly, and should be dropped.\n\nYes, it is silly and should be dropped, but if you drop it on the\ntool side, then it may become even easier to do the \"instead of\nreading from .git/refs/logs files, have the tool dump and inspect\nthat\" step, no?\n\nThis being test-tool, I do not mind losing backward compatibility\nthat forces us a silly \"tail -n 2 | head -n 1\" pipeline at all.  But\nif silliness comes from the test-tool thta does not work well with\nthe internal API, that is what should be fixed.  Surely you can\nchange the API and its current callers to compensate for its silliness,\nbut I do not think that is a good direction to go in.\n\nThanks.\n\n"},{"id":"442178","messageId":"CAFQ2z_Mct+KBZ3vO6udwqeiHYA8od8CGH_w5BO5LaidP-AYDsg@mail.gmail.com","threadId":"56958","inReplyTo":"xmqqczmqajdk.fsf@gitster.g","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-23T17:28:24Z","receivedAt":"2021-11-23T17:28:39Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Nov 23, 2021 at 6:10 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n> > On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >>\n> >> If this were truly \"user-provided\", then I'd argue that all backends\n> >> should follow whatever the files backend has been doing forever---if\n> >> the files added LF implicitly, others should, too, because that is\n> >> pretty much what these \"user-provided\" callbacks have been expecting\n> >> to see.\n> >\n> > I think it's just wrong. If you pass `msg` to a storage API, you\n> > should get `msg` when you read it back, not (msg + \"\\n\").\n>\n> If you give a log message \"git commit -m 'single line'\", you get LF\n> at the end of the commit message for free.  This is no different.\n> And you know that this is not a \"storage API\" that stores the input\n> in verbatim after looking at refs.c::copy_reflog_msg().\n\nI'm talking about refs/refs-internal.h. It seems you want to add something like\n\n/* The ref backend should add a '\\n' relative to the message supplied\nto the delete/symref/update functions. */\ntypedef int for_each_reflog_ent_fn(struct ref_store *ref_store,\n                                   const char *refname,\n                                   each_reflog_ent_fn fn,\n                                   void *cb_data);\n\n?\n\n--\nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442184","messageId":"xmqqfsrm9122.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_PhazkdC8JOqDW-=VD4iLq_==x23+fN7T-Vp9M2DoW=qw@mail.gmail.com","subject":"Re: [PATCH 3/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-23T18:31:01Z","receivedAt":"2021-11-23T18:31:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n>> > +     printf(\"%s %s %s %\" PRItime \" %+05d\\t%s\\n\", oid_to_hex(old_oid),\n>> > +            oid_to_hex(new_oid), committer, timestamp, tz, msg);\n>>\n>> Looks good to me.  We might want to make the printf format\n>> conditional to add \\t%s only when msg is not empty, though.\n>> Hopefully such a change would follow the reflog format even more\n>> closely to make 4/4 unnecessary?\n>\n> I think the conditional formatting of \\t is impractical. It makes things like\n>\n>   (metadata, msg) = line.split('\\t')\n>\n> in Python require special casing in case msg is empty.\n\nDoesn't it however make it cumobersome (as we saw in 4/4 and Ævar's\nreaction to it) to write tests to add trailing whitespace like this,\nI am afraid?\n\nWithout trailing HT, a self-test of this data dumper would become\ntrivial---just run it and compare its output with the real file in\n.git/refs/logs/ directory, no?\n\nAs this is only test-helper, I do not mind the deviation from the\nformat, even though the log message claims to make it closer, to\nalways show HT.  And because the consumers of this data are only\ntest scripts, I do not mind they are sloppier than the real-world\ncode.\n\nBut if this were a pair of real world data producer/consumer, the\nconsumer would be prepared to see and deal with a line that ought to\nhave but lacks HT anyway, so I suspect that the amount of code to\nparse conditionally added HT is not that large.\n\n>> > diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\n>> > index a600bedf2cd..76b15458409 100755\n>> > --- a/t/t1405-main-ref-store.sh\n>> > +++ b/t/t1405-main-ref-store.sh\n>> > @@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '\n>> >\n>> >  test_expect_success 'for_each_reflog_ent_reverse()' '\n>> >       $RUN for-each-reflog-ent-reverse HEAD >actual &&\n>> > +     head -n1 actual | grep recreate-main &&\n>>\n>> I am not sure how this new test helps validate the change to the\n>> code.\n>\n> It's for consistency with the preceding test. I can make a separate commit.\n\nMeaning this is an unrelated clean-up of the existing test before\nthis series started?  Sure, just like the show-branch bugfix, it\nwould be nicer to have a separate commit for it.\n\nThanks.\n"},{"id":"442200","messageId":"xmqqzgpu7grv.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_Mct+KBZ3vO6udwqeiHYA8od8CGH_w5BO5LaidP-AYDsg@mail.gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-23T20:34:28Z","receivedAt":"2021-11-23T20:34:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> I'm talking about refs/refs-internal.h. It seems you want to add something like\n>\n> /* The ref backend should add a '\\n' relative to the message supplied\n> to the delete/symref/update functions. */\n> typedef int for_each_reflog_ent_fn(struct ref_store *ref_store,\n>                                    const char *refname,\n>                                    each_reflog_ent_fn fn,\n>                                    void *cb_data);\n>\n> ?\n\nSorry, I do not follow.  Doesn't the ref backend already ensure that\nthe message is not an incomplete line?  If you feed a single\ncomplete line when updating, I do not think the backend should add\nany extra LF relative to the given message:\n\n    $ git update-ref -m 'creating a new branch manually\n    ' refs/heads/newtest master\n    $ git update-ref -m 'updating a new branch manually\n    ' refs/heads/newtest master~1\n    $ git reflog refs/heads/newtest\n    4150a1677b refs/heads/newtest@{0}: updating a new branch manually\n    5f439a0ecf refs/heads/newtest@{1}: updating the reference\n\nI think what deserves such a comment more is the prototype for\neach_reflog_ent_fn describing what the parameters to that callback\nfunction, to help the callers of the iterator what to expect.  That\nis the end-user facing part.\n\n/*\n * Callback to process a reflog entry found by the iteration functions (see\n * below)\n */\ntypedef int each_reflog_ent_fn(\n\t\tstruct object_id *old_oid, struct object_id *new_oid,\n\t\tconst char *committer, timestamp_t timestamp,\n\t\tint tz, const char *msg, void *cb_data);\n\nCurrently it only says \"Callback (see below)\" but \"below\" has only\ncomments about the difference between refs_for_each_reflog_ent() and\nrefs_for_each_reflog_entreverse() functions, and does not talk about\nwhat \"committer\" looks like (i.e. does it give user.name equivalent,\nuser.name plus <user.email>, or something else?), and what \"msg\"\nlooks like (i.e. if a multi-line message was given when a ref was\nupdated or created, do we get these lines intact? if it gets\nmangled, how? if the original was a single-liner incomplete line, do\nwe lack the final LF?), and how tz is encoded.\n\nI think the rule for \"msg\" is that:\n\n   a multi-line message, or a message on a single incomplete-line,\n   are normalized into a single complete line, and callback gets a\n   single complete line.\n\nOnce these rules get specified tightly and fully, it is up to the\nref(log) backends how to implement the interface.  If files-backend\nwants to keep LF at the end of the message when storing (so that it\ncan easily use it as record delimiter), it can do so and reuse the\nLF at the end of the message as part of the msg parameter to the\ncallback.  If another backend wants to drop LF at the end of the\nmessage when storing (to save space), it can do so as long as the\ncallback function gets the LF just like the other backend.\n\n"},{"id":"442236","messageId":"CAFQ2z_Mb+NzQfXfMhdQoPs09p2j1AWRAdXcX3JSznn+H9+T0Bw@mail.gmail.com","threadId":"56958","inReplyTo":"xmqqzgpu7grv.fsf@gitster.g","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-24T11:17:04Z","receivedAt":"2021-11-24T11:17:18Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Nov 23, 2021 at 9:34 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n> > I'm talking about refs/refs-internal.h. It seems you want to add something like\n> >\n> > /* The ref backend should add a '\\n' relative to the message supplied\n> > to the delete/symref/update functions. */\n> > typedef int for_each_reflog_ent_fn(struct ref_store *ref_store,\n> >                                    const char *refname,\n> >                                    each_reflog_ent_fn fn,\n> >                                    void *cb_data);\n> >\n> > ?\n>\n> Sorry, I do not follow.  Doesn't the ref backend already ensure that\n> the message is not an incomplete line?  If you feed a single\n> complete line when updating, I do not think the backend should add\n> any extra LF relative to the given message:\n\nBut it does, currently. As of\n\n  commit 523fa69c36744ae6779e38614cb9bfb2be552923\n  Author: Junio C Hamano <gitster@pobox.com>\n  ..\n  - We expect that a reflog message consists of a single line.  The\n       file format used by the files backend may add a LF after the\n\nit is the job of the files ref backend to add a LF, and the input to\nthe ref backend is a string without trailing LF. But the files backend\nthen produces that message with a LF, when reading back the data, eg.\n\n$ git --version\ngit version 2.34.0.rc2.393.gf8c9666880-goog\n\n$ GIT_TRACE_REFS=\"1\" git branch -m bla blub\n..\n12:03:59.408705 refs/debug.c:162        rename_ref: refs/heads/bla ->\nrefs/heads/blub \"Branch: renamed refs/heads/bla to refs/heads/blub\": 0\n\n$ GIT_TRACE_REFS=1 git reflog show refs/heads/blub\n12:04:23.277805 refs/debug.c:294        reflog_ent refs/heads/blub\n(ret 0): cd3e606211bb1cf8bc57f7d76bab98cc17a150bc ->\ncd3e606211bb1cf8bc57f7d76bab98cc17a150bc, Han-Wen Nienhuys\n<hanwen@google.com> 1637751839 \"Branch: renamed refs/heads/bla to\nrefs/heads/blub\n\"\n\n> I think the rule for \"msg\" is that:\n>\n>    a multi-line message, or a message on a single incomplete-line,\n>    are normalized into a single complete line, and callback gets a\n>    single complete line.\n>\n\nThat is not how it works today. The files backend verbatimly dumps the\nmessage supplied to it. (Maybe it should crash if there is a '\\n' in\nthe message).\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442258","messageId":"xmqqpmqp2xmz.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_Mb+NzQfXfMhdQoPs09p2j1AWRAdXcX3JSznn+H9+T0Bw@mail.gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-24T18:53:40Z","receivedAt":"2021-11-24T18:53:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n>> ...  Doesn't the ref backend already ensure that\n>> the message is not an incomplete line?  If you feed a single\n>> complete line when updating, I do not think the backend should add\n>> any extra LF relative to the given message:\n>\n> But it does, currently. As of\n>\n>   commit 523fa69c36744ae6779e38614cb9bfb2be552923\n>   Author: Junio C Hamano <gitster@pobox.com>\n>   ..\n>   - We expect that a reflog message consists of a single line.  The\n>        file format used by the files backend may add a LF after the\n>\n> it is the job of the files ref backend to add a LF, and the input to\n> the ref backend is a string without trailing LF. But the files backend\n> then produces that message with a LF, when reading back the data, eg.\n\nAh, perhaps our confusion comes from the fact that I view the ref\nAPI as a whole and do not draw a fine line of distinction between\nthe \"ref API implementation common across backends\" vs \"what ref\nfiles backend does\".  But as the implementor of one backend, you\nobviously have to care.\n\nIn any case, when I did that commit, I clearly meant that the\nnormalization implemented by the patch belong to the common part to\nbe used by all backends for uniform handling of refs.  If a call to\nrefs API in a repository that is configured to use reftable backend\nbypasses the normalization function introduced in that commit, that\nis a bug.\n\nSo, yes \"ref API ensures that the message is not an incomplete line\nto turn 0, 1, or multiple lines input into a single line\", which is\nwhy the experiments you omitted from your quote (reproduced here)\n\n    $ git update-ref -m 'creating a new branch manually\n    ' refs/heads/newtest master\n    $ git update-ref -m 'updating a new branch manually\n    ' refs/heads/newtest master~1\n    $ git reflog refs/heads/newtest\n    4150a1677b refs/heads/newtest@{0}: updating a new branch manually\n    5f439a0ecf refs/heads/newtest@{1}: updating the reference\n\nto give a complete line when recording the reflog entries does not\nresult in an extra LF shown on the output.\n\n>> I think the rule for \"msg\" is that:\n>>\n>>    a multi-line message, or a message on a single incomplete-line,\n>>    are normalized into a single complete line, and callback gets a\n>>    single complete line.\n>\n> That is not how it works today. The files backend verbatimly dumps the\n> message supplied to it. (Maybe it should crash if there is a '\\n' in\n> the message).\n\nAs I said, if parts of refs API implementation forgets to call\nnormalization, it is a bug.  Is there different codepath other than\nthe one that is exercised with the \"git update -m ''\" experiment\nabove and you found such a bug there?  It needs to be fixed.\n\nThanks.\n"},{"id":"442259","messageId":"CAFQ2z_PqgwYy+RxQm7d7WDQYvQQ2vVE0cXHdDKNwQJm3Xc+NFw@mail.gmail.com","threadId":"56958","inReplyTo":"xmqqpmqp2xmz.fsf@gitster.g","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-24T19:06:14Z","receivedAt":"2021-11-24T19:06:28Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Nov 24, 2021 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n> >> ...  Doesn't the ref backend already ensure that\n> >> the message is not an incomplete line?  If you feed a single\n> >> complete line when updating, I do not think the backend should add\n> >> any extra LF relative to the given message:\n> >\n> > But it does, currently. As of\n> >\n> >   commit 523fa69c36744ae6779e38614cb9bfb2be552923\n> >   Author: Junio C Hamano <gitster@pobox.com>\n> >   ..\n> >   - We expect that a reflog message consists of a single line.  The\n> >        file format used by the files backend may add a LF after the\n> >\n> > it is the job of the files ref backend to add a LF, and the input to\n> > the ref backend is a string without trailing LF. But the files backend\n> > then produces that message with a LF, when reading back the data, eg.\n>\n> Ah, perhaps our confusion comes from the fact that I view the ref\n> API as a whole and do not draw a fine line of distinction between\n> the \"ref API implementation common across backends\" vs \"what ref\n> files backend does\".  But as the implementor of one backend, you\n> obviously have to care.\n\nHaving the read function return something different than what the\nwrite gets still seems odd to me.\nHow about having copy_reflog_msg() trim trailing space and then always add LF?\n\nThe files backend can assert that the string ends in LF, and doesn't\nneed to add LF itself.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442260","messageId":"xmqq1r352w3g.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_Mb+NzQfXfMhdQoPs09p2j1AWRAdXcX3JSznn+H9+T0Bw@mail.gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-24T19:26:59Z","receivedAt":"2021-11-24T19:27:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> $ GIT_TRACE_REFS=\"1\" git branch -m bla blub\n> ..\n> 12:03:59.408705 refs/debug.c:162        rename_ref: refs/heads/bla ->\n> refs/heads/blub \"Branch: renamed refs/heads/bla to refs/heads/blub\": 0\n>\n> $ GIT_TRACE_REFS=1 git reflog show refs/heads/blub\n> 12:04:23.277805 refs/debug.c:294        reflog_ent refs/heads/blub\n> (ret 0): cd3e606211bb1cf8bc57f7d76bab98cc17a150bc ->\n> cd3e606211bb1cf8bc57f7d76bab98cc17a150bc, Han-Wen Nienhuys\n> <hanwen@google.com> 1637751839 \"Branch: renamed refs/heads/bla to\n> refs/heads/blub\n> \"\n>\n>> I think the rule for \"msg\" is that:\n>>\n>>    a multi-line message, or a message on a single incomplete-line,\n>>    are normalized into a single complete line, and callback gets a\n>>    single complete line.\n>>\n>\n> That is not how it works today. The files backend verbatimly dumps the\n> message supplied to it. (Maybe it should crash if there is a '\\n' in\n> the message).\n\nI still am puzzled what you wanted to illustrate with the \"git\nbranch -m bla\" trace.  The call graph into the refs API looks like\nthis:\n\nbuiltin/branch.c::cmd_branch()\n -> branch.c::create_branch()\n    -> refs.c::ref_transaction_update()\n       -> refs.c::ref_transaction_add_update()\n    -> refs.c::ref_transaction_commit()\n\nand the message the user gave is passed through in \"msg\" variable\nwithout modification, when calling ref_transaction_update(), which\nin turn makes a call to ref_transaction_add_update().  It does this:\n\n    struct ref_update *ref_transaction_add_update(\n                    struct ref_transaction *transaction,\n                    const char *refname, unsigned int flags,\n                    const struct object_id *new_oid,\n                    const struct object_id *old_oid,\n                    const char *msg)\n    {\n            struct ref_update *update;\n\n            if (transaction->state != REF_TRANSACTION_OPEN)\n                    BUG(\"update called for transaction that is not open\");\n\n            FLEX_ALLOC_STR(update, refname, refname);\n            ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n            transaction->updates[transaction->nr++] = update;\n\n            update->flags = flags;\n\n            if (flags & REF_HAVE_NEW)\n                    oidcpy(&update->new_oid, new_oid);\n            if (flags & REF_HAVE_OLD)\n                    oidcpy(&update->old_oid, old_oid);\n            update->msg = normalize_reflog_message(msg);\n            return update;\n    }\n\nAnd normalize_reflog_message() calls copy_reflog_msg() to squash\nruns of isspace() bytes (note: that class includes LF) into a single\nspace, and runs rtrim(), so update->msg will get a single incomplete\nline.  As I suspected in a separate message, I think my notion of\nwhat happens in the ref API implementation common to all backends\nand what happens in each backend, and hence my statements about the\ndistinction, were much fuzzier than yours, so I should say that I\nconsider that all of the above is happening in the common part\nacross all backends.  If normalize happens in ref-files, it should\nhappen in reftable backend, too, automatically.\n\nThe files-backend has no chance to even see an embedded LF in the\nmessage when the transaction gets committed and the backend is\ncalled, does it?  So I am not sure why we should crash upon seeing a\nLF in the message.\n\nIn any case, it seems that as a comment to clarify the end-user\nfacing each_reflog_ent_fn() parameters, what you quoted above from\nmy message seems correct to me, after following the callgraph like\nthe above.  A 0-line (i.e. incomplete, like your 'bla' given to \"git\nbranch\"), 1-line (i.e. a single complete line, like the message I\ngave to \"update-ref -m\" in my earlier illustration), or multi-line\nmessage given when a reflog entry is created, is normalized and when\nthe each_reflog_ent_fn() callback is called, it is shown to the\ncallback function as a single complete line, with a LF at the end.\n\nPhrased without the explanation specific to this discussion, but\nwith a bit more detail:\n\n  The message given when a reflog entry was created, is normalized\n  into a single line by turning each LF into a space, squeezing a\n  run of multiple whitespaces into one space, and removing trailing\n  whitespaces. The callback gets a single complete line in the 'msg'\n  parameter.\n\nperhaps?\n"},{"id":"442262","messageId":"CAFQ2z_P9XkXruBeEPd77cMF7BanzgmMF5CocxNj_JMmXVMeqaw@mail.gmail.com","threadId":"56958","inReplyTo":"xmqq1r352w3g.fsf@gitster.g","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-24T19:39:35Z","receivedAt":"2021-11-24T19:39:50Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Nov 24, 2021 at 8:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n> > $ GIT_TRACE_REFS=\"1\" git branch -m bla blub\n> > ..\n> > 12:03:59.408705 refs/debug.c:162        rename_ref: refs/heads/bla ->\n> > refs/heads/blub \"Branch: renamed refs/heads/bla to refs/heads/blub\": 0\n> >\n> > $ GIT_TRACE_REFS=1 git reflog show refs/heads/blub\n> > 12:04:23.277805 refs/debug.c:294        reflog_ent refs/heads/blub\n> > (ret 0): cd3e606211bb1cf8bc57f7d76bab98cc17a150bc ->\n> > cd3e606211bb1cf8bc57f7d76bab98cc17a150bc, Han-Wen Nienhuys\n> > <hanwen@google.com> 1637751839 \"Branch: renamed refs/heads/bla to\n> > refs/heads/blub\n> > \"\n> >\n> >> I think the rule for \"msg\" is that:\n> >>\n> >>    a multi-line message, or a message on a single incomplete-line,\n> >>    are normalized into a single complete line, and callback gets a\n> >>    single complete line.\n> >>\n> >\n> > That is not how it works today. The files backend verbatimly dumps the\n> > message supplied to it. (Maybe it should crash if there is a '\\n' in\n> > the message).\n>\n> I still am puzzled what you wanted to illustrate with the \"git\n> branch -m bla\" trace.\n\nI'm trying to illustrate that (from the perspective of the ref backend\nAPI) one call inserts something without a '\\n', but the call that\nreads the info back, gets the same data back with a '\\n'. It looks\nconfusing and inconsistent to me.\n\nIt seems fine to decide that the message should always end in a LF,\nbut then why not do that in the normalization routine, so it is shared\nacross all backends?\n\nFor the purpose of the debug support (GIT_TRACE_REFS=1), I would\nrather prefer if the message was always without LF, because the LF\nruins the visual of the debug output, but that is a minor concern.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442271","messageId":"xmqqtug1z32j.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_PqgwYy+RxQm7d7WDQYvQQ2vVE0cXHdDKNwQJm3Xc+NFw@mail.gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-24T20:55:16Z","receivedAt":"2021-11-24T20:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n>> Ah, perhaps our confusion comes from the fact that I view the ref\n>> API as a whole and do not draw a fine line of distinction between\n>> the \"ref API implementation common across backends\" vs \"what ref\n>> files backend does\".  But as the implementor of one backend, you\n>> obviously have to care.\n>\n> Having the read function return something different than what the\n> write gets still seems odd to me.\n> How about having copy_reflog_msg() trim trailing space and then always add LF?\n>\n> The files backend can assert that the string ends in LF, and doesn't\n> need to add LF itself.\n\nEhh, let's step back a bit.\n\nIs there anything in the common part and files backend in ref API\nthat needs to be changed to fix some bug?  I do not see anything\nbroken that needs to be fixed by \"assert that the string ends in LF\nand doesn't need to add LF itself\" or by \"always add LF\".\n"},{"id":"442315","messageId":"0319503045ba8ab93eb09434ed4bada0050c5f8a.1637855872.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] t1405: check for_each_reflog_ent_reverse() more thoroughly","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-25T15:57:50Z","receivedAt":"2021-11-25T16:10:17Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nIf we are checking for a certain ordering, we should check that there are two\nentries. Do this by mirroring the preceding test.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/t1405-main-ref-store.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex a600bedf2cd..76b15458409 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n+\thead -n1 actual | grep recreate-main &&\n \ttail -n1 actual | grep one\n '\n \n-- \ngitgitgadget\n\n"},{"id":"442316","messageId":"0288e743eb2e96e2effd6b0b90c6f885009bf337.1637855872.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-25T15:57:52Z","receivedAt":"2021-11-25T16:10:21Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nOn iteration, the reflog message is always terminated by a newline. Trim it to\navoid clobbering the console with is this extra newline.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/debug.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 8667c640237..2631210795b 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -284,15 +284,21 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n \tint ret;\n \tchar o[GIT_MAX_HEXSZ + 1] = \"null\";\n \tchar n[GIT_MAX_HEXSZ + 1] = \"null\";\n+\tstruct strbuf trimmed = STRBUF_INIT;\n \tif (old_oid)\n \t\toid_to_hex_r(o, old_oid);\n \tif (new_oid)\n \t\toid_to_hex_r(n, new_oid);\n \n+\tstrbuf_addstr(&trimmed, msg);\n \tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n \t\t      dbg->cb_data);\n-\ttrace_printf_key(&trace_refs, \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n-\t\tdbg->refname, ret, o, n, committer, (long int)timestamp, msg);\n+\tstrbuf_trim_trailing_newline(&trimmed);\n+\ttrace_printf_key(&trace_refs,\n+\t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n+\t\t\t dbg->refname, ret, o, n, committer,\n+\t\t\t (long int)timestamp, trimmed.buf);\n+\tstrbuf_release(&trimmed);\n \treturn ret;\n }\n \n-- \ngitgitgadget\n"},{"id":"442317","messageId":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.git.git.1637590855.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Inspect reflog data programmatically in more tests","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-25T15:57:47Z","receivedAt":"2021-11-25T16:10:28Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"This helps for reftable support, and will help if we want to reconsider\nunder which conditions reflogs get created/updated.\n\nHan-Wen Nienhuys (5):\n  show-branch: show reflog message\n  test-ref-store: don't add newline to reflog message\n  t1405: check for_each_reflog_ent_reverse() more thoroughly\n  test-ref-store: tweaks to for-each-reflog-ent format\n  refs/debug: trim trailing LF from reflog message\n\n builtin/show-branch.c          | 12 +++++++-----\n refs/debug.c                   | 10 ++++++++--\n t/helper/test-ref-store.c      |  6 +++---\n t/t1400-update-ref.sh          | 13 ++++++++-----\n t/t1405-main-ref-store.sh      |  4 ++--\n t/t1406-submodule-ref-store.sh |  4 ++--\n t/t3202-show-branch.sh         | 15 +++++++++++++++\n 7 files changed, 45 insertions(+), 19 deletions(-)\n\n\nbase-commit: 35151cf0720460a897cde9b8039af364743240e7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1145%2Fhanwen%2Freflog-prelims-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1145/hanwen/reflog-prelims-v2\nPull-Request: https://github.com/git/git/pull/1145\n\nRange-diff vs v1:\n\n 1:  fd2595d370a = 1:  9d8394d8c76 show-branch: show reflog message\n 2:  dfb63937323 ! 2:  4a86d212589 refs: trim newline from reflog message\n     @@ Metadata\n      Author: Han-Wen Nienhuys <hanwen@google.com>\n      \n       ## Commit message ##\n     -    refs: trim newline from reflog message\n     +    test-ref-store: don't add newline to reflog message\n      \n     -    Commit 523fa69c (\"reflog: cleanse messages in the refs.c layer\") standardizes\n     -    how write entries into the reflog. This commit standardizes how we get messages\n     -    out of the reflog. Before, the files backend implicitly added '\\n' to the end of\n     -    reflog message on reading, which creates a subtle incompatibility with alternate\n     -    ref storage backends, such as reftable.\n     -\n     -    We address this by stripping LF from the message before we pass it to the\n     -    user-provided callback.\n     +    The files backend produces a newline for messages automatically, so before we\n     +    would print blank lines between entries.\n      \n          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n      \n     - ## builtin/show-branch.c ##\n     -@@ builtin/show-branch.c: int cmd_show_branch(int ac, const char **av, const char *prefix)\n     - \t\t\tchar *logmsg;\n     - \t\t\tchar *nth_desc;\n     - \t\t\tconst char *msg;\n     --\t\t\tchar *end;\n     - \t\t\ttimestamp_t timestamp;\n     - \t\t\tint tz;\n     - \n     -@@ builtin/show-branch.c: int cmd_show_branch(int ac, const char **av, const char *prefix)\n     - \t\t\t\tbreak;\n     - \t\t\t}\n     - \n     --\t\t\tend = strchr(logmsg, '\\n');\n     --\t\t\tif (end)\n     --\t\t\t\t*end = '\\0';\n     --\n     - \t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n     - \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n     - \t\t\t\t\t\tshow_date(timestamp, tz,\n     -\n     - ## reflog-walk.c ##\n     -@@ reflog-walk.c: void get_reflog_message(struct strbuf *sb,\n     - \n     - \tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n     - \tlen = strlen(info->message);\n     --\tif (len > 0)\n     --\t\tlen--; /* strip away trailing newline */\n     - \tstrbuf_add(sb, info->message, len);\n     - }\n     - \n     -@@ reflog-walk.c: void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n     - \t\tinfo = &commit_reflog->reflogs->items[commit_reflog->recno+1];\n     - \t\tget_reflog_selector(&selector, reflog_info, dmode, force_date, 0);\n     - \t\tif (oneline) {\n     --\t\t\tprintf(\"%s: %s\", selector.buf, info->message);\n     -+\t\t\tprintf(\"%s: %s\\n\", selector.buf, info->message);\n     - \t\t}\n     - \t\telse {\n     --\t\t\tprintf(\"Reflog: %s (%s)\\nReflog message: %s\",\n     -+\t\t\tprintf(\"Reflog: %s (%s)\\nReflog message: %s\\n\",\n     - \t\t\t       selector.buf, info->email, info->message);\n     - \t\t}\n     - \n     -\n     - ## refs/files-backend.c ##\n     -@@ refs/files-backend.c: static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n     - \tint tz;\n     - \tconst char *p = sb->buf;\n     - \n     --\t/* old SP new SP name <email> SP time TAB msg LF */\n     --\tif (!sb->len || sb->buf[sb->len - 1] != '\\n' ||\n     --\t    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||\n     -+\t/* old SP new SP name <email> SP time TAB msg */\n     -+\tif (!sb->len || parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||\n     - \t    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||\n     --\t    !(email_end = strchr(p, '>')) ||\n     --\t    email_end[1] != ' ' ||\n     -+\t    !(email_end = strchr(p, '>')) || email_end[1] != ' ' ||\n     - \t    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||\n     - \t    !message || message[0] != ' ' ||\n     --\t    (message[1] != '+' && message[1] != '-') ||\n     --\t    !isdigit(message[2]) || !isdigit(message[3]) ||\n     --\t    !isdigit(message[4]) || !isdigit(message[5]))\n     -+\t    (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) ||\n     -+\t    !isdigit(message[3]) || !isdigit(message[4]) ||\n     -+\t    !isdigit(message[5]))\n     - \t\treturn 0; /* corrupt? */\n     - \temail_end[1] = '\\0';\n     - \ttz = strtol(message + 1, NULL, 10);\n     -@@ refs/files-backend.c: static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n     - \t\t\t\tstrbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));\n     - \t\t\t\tscanp = bp;\n     - \t\t\t\tendp = bp + 1;\n     -+\t\t\t\tstrbuf_trim_trailing_newline(&sb);\n     - \t\t\t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n     - \t\t\t\tstrbuf_reset(&sb);\n     - \t\t\t\tif (ret)\n     -@@ refs/files-backend.c: static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n     - \t\t\t\t * Process it, and we can end the loop.\n     - \t\t\t\t */\n     - \t\t\t\tstrbuf_splice(&sb, 0, 0, buf, endp - buf);\n     -+\t\t\t\tstrbuf_trim_trailing_newline(&sb);\n     - \t\t\t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n     - \t\t\t\tstrbuf_reset(&sb);\n     - \t\t\t\tbreak;\n     -@@ refs/files-backend.c: static int files_for_each_reflog_ent(struct ref_store *ref_store,\n     - \tif (!logfp)\n     - \t\treturn -1;\n     - \n     --\twhile (!ret && !strbuf_getwholeline(&sb, logfp, '\\n'))\n     -+\twhile (!ret && !strbuf_getline(&sb, logfp))\n     - \t\tret = show_one_reflog_ent(&sb, fn, cb_data);\n     - \tfclose(logfp);\n     - \tstrbuf_release(&sb);\n     -@@ refs/files-backend.c: static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n     - \tif ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,\n     - \t\t\t\t   message, policy_cb)) {\n     - \t\tif (!cb->newlog)\n     --\t\t\tprintf(\"would prune %s\", message);\n     -+\t\t\tprintf(\"would prune %s\\n\", message);\n     - \t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n     --\t\t\tprintf(\"prune %s\", message);\n     -+\t\t\tprintf(\"prune %s\\n\", message);\n     - \t} else {\n     - \t\tif (cb->newlog) {\n     --\t\t\tfprintf(cb->newlog, \"%s %s %s %\"PRItime\" %+05d\\t%s\",\n     --\t\t\t\toid_to_hex(ooid), oid_to_hex(noid),\n     --\t\t\t\temail, timestamp, tz, message);\n     -+\t\t\tfprintf(cb->newlog, \"%s %s %s %\" PRItime \" %+05d\\t%s\\n\",\n     -+\t\t\t\toid_to_hex(ooid), oid_to_hex(noid), email,\n     -+\t\t\t\ttimestamp, tz, message);\n     - \t\t\toidcpy(&cb->last_kept_oid, noid);\n     - \t\t}\n     - \t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n     --\t\t\tprintf(\"keep %s\", message);\n     -+\t\t\tprintf(\"keep %s\\n\", message);\n     - \t}\n     + ## t/helper/test-ref-store.c ##\n     +@@ t/helper/test-ref-store.c: static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n     + \t\t       const char *committer, timestamp_t timestamp,\n     + \t\t       int tz, const char *msg, void *cb_data)\n     + {\n     +-\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n     +-\t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n     +-\t       committer, timestamp, tz, msg);\n     ++\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n     ++\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n       \treturn 0;\n       }\n     + \n      \n       ## t/t1405-main-ref-store.sh ##\n      @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog()' '\n 3:  8a1b094d547 ! 3:  0319503045b test-ref-store: tweaks to for-each-reflog-ent format\n     @@ Metadata\n      Author: Han-Wen Nienhuys <hanwen@google.com>\n      \n       ## Commit message ##\n     -    test-ref-store: tweaks to for-each-reflog-ent format\n     +    t1405: check for_each_reflog_ent_reverse() more thoroughly\n      \n     -    Follow the reflog format more closely, so it can be used for comparing\n     -    reflogs in tests without using inspecting files under .git/logs/\n     +    If we are checking for a certain ordering, we should check that there are two\n     +    entries. Do this by mirroring the preceding test.\n      \n          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n      \n     - ## t/helper/test-ref-store.c ##\n     -@@ t/helper/test-ref-store.c: static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n     - \t\t       const char *committer, timestamp_t timestamp,\n     - \t\t       int tz, const char *msg, void *cb_data)\n     - {\n     --\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n     --\t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n     --\t       committer, timestamp, tz, msg);\n     -+\tprintf(\"%s %s %s %\" PRItime \" %+05d\\t%s\\n\", oid_to_hex(old_oid),\n     -+\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n     - \treturn 0;\n     - }\n     - \n     -\n       ## t/t1405-main-ref-store.sh ##\n      @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog_ent()' '\n       \n 4:  4ba97a4e70a ! 4:  62f5cb8a824 t1400: use test-helper ref-store to inspect reflog contents\n     @@ Metadata\n      Author: Han-Wen Nienhuys <hanwen@google.com>\n      \n       ## Commit message ##\n     -    t1400: use test-helper ref-store to inspect reflog contents\n     +    test-ref-store: tweaks to for-each-reflog-ent format\n      \n     -    This avoids inspecting the file system, which only works with the files ref\n     -    backend.\n     +    We have some tests that read from files in .git/logs/ hierarchy\n     +    when checking if correct reflog entries are created, but that is\n     +    too specific to the files backend.  Other backends like reftable\n     +    may not store its reflog entries in such a \"one line per entry\"\n     +    format.\n     +\n     +    Update for-each-reflog-ent test helper to produce output that\n     +    is identical to lines in a reflog file files backend uses.\n     +    That way, (1) the current tests can be updated to use the test\n     +    helper to read the reflog entries instead of (parts of) reflog\n     +    files, and perform the same inspection for correctness, and (2)\n     +    when the ref backend is swapped to another backend, the updated\n     +    test can be used as-is to check the correctness.\n     +\n     +    Adapt t1400 to use the for-each-reflog-ent test helper.\n      \n          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n      \n     - ## t/t1400-update-ref.sh ##\n     -@@ t/t1400-update-ref.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n     - . ./test-lib.sh\n     + ## t/helper/test-ref-store.c ##\n     +@@ t/helper/test-ref-store.c: static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n     + \t\t       const char *committer, timestamp_t timestamp,\n     + \t\t       int tz, const char *msg, void *cb_data)\n     + {\n     +-\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n     +-\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n     ++\tprintf(\"%s %s %s %\" PRItime \" %+05d%s%s\", oid_to_hex(old_oid),\n     ++\t       oid_to_hex(new_oid), committer, timestamp, tz,\n     ++\t       *msg == '\\n' ? \"\" : \"\\t\", msg);\n     + \treturn 0;\n     + }\n       \n     - Z=$ZERO_OID\n     -+TAB='\t'\n     - \n     - m=refs/heads/main\n     - n_dir=refs/heads/gu\n     -@@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '\n     - cat >expect <<EOF\n     - $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tInitial Creation\n     - $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n     --$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n     -+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB\n     +\n     + ## t/t1400-update-ref.sh ##\n     +@@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n     + $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n       EOF\n       test_expect_success \"verifying $m's log (logged by touch)\" '\n      -\ttest_when_finished \"rm -rf .git/$m .git/logs expect\" &&\n     @@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '\n       '\n       \n       test_expect_success \"create $m (logged by config)\" '\n     -@@ t/t1400-update-ref.sh: test_expect_success \"set $m (logged by config)\" '\n     - cat >expect <<EOF\n     - $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000\tInitial Creation\n     - $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n     --$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000\n     -+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000$TAB\n     +@@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n     + $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000\n       EOF\n       test_expect_success \"verifying $m's log (logged by config)\" '\n      -\ttest_when_finished \"rm -f .git/$m .git/logs/$m expect\" &&\n -:  ----------- > 5:  0288e743eb2 refs/debug: trim trailing LF from reflog message\n\n-- \ngitgitgadget\n"},{"id":"442318","messageId":"9d8394d8c7666b13403394332d826a3729a1a3e0.1637855872.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] show-branch: show reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-25T15:57:48Z","receivedAt":"2021-11-25T16:10:38Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nBefore, --reflog option would look for '\\t' in the reflog message. As refs.c\nalready parses the reflog line, the '\\t' was never found, and show-branch\n--reflog would always say \"(none)\" as reflog message\n\nAdd test.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n builtin/show-branch.c  | 12 +++++++-----\n t/t3202-show-branch.sh | 15 +++++++++++++++\n 2 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 082449293b5..f1e8318592c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -761,6 +761,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tchar *logmsg;\n \t\t\tchar *nth_desc;\n \t\t\tconst char *msg;\n+\t\t\tchar *end;\n \t\t\ttimestamp_t timestamp;\n \t\t\tint tz;\n \n@@ -770,11 +771,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\treflog = i;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tmsg = strchr(logmsg, '\\t');\n-\t\t\tif (!msg)\n-\t\t\t\tmsg = \"(none)\";\n-\t\t\telse\n-\t\t\t\tmsg++;\n+\n+\t\t\tend = strchr(logmsg, '\\n');\n+\t\t\tif (end)\n+\t\t\t\t*end = '\\0';\n+\n+\t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n \t\t\t\t\t\tshow_date(timestamp, tz,\n \t\t\t\t\t\t\t  DATE_MODE(RELATIVE)),\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ad9902a06b9..d4d64401e4b 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -4,6 +4,9 @@ test_description='test show-branch'\n \n . ./test-lib.sh\n \n+# arbitrary reference time: 2009-08-30 19:20:00\n+GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n+\n test_expect_success 'setup' '\n \ttest_commit initial &&\n \tfor i in $(test_seq 1 10)\n@@ -146,4 +149,16 @@ test_expect_success 'show branch --merge-base with N arguments' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show branch --reflog=2' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n+\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n+\t>\t--\n+\t>\t+  [refs/heads/branch10@{0}] branch10\n+\t>\t++ [refs/heads/branch10@{1}] initial\n+\tEOF\n+\tgit show-branch --reflog=2 >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"442319","messageId":"4a86d21258918ea8124caef1cd2e435b465f67c5.1637855872.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] test-ref-store: don't add newline to reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-25T15:57:49Z","receivedAt":"2021-11-25T16:10:40Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nThe files backend produces a newline for messages automatically, so before we\nwould print blank lines between entries.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/helper/test-ref-store.c      | 5 ++---\n t/t1405-main-ref-store.sh      | 5 ++---\n t/t1406-submodule-ref-store.sh | 4 ++--\n 3 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex b314b81a45b..ef244aa6b27 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -151,9 +151,8 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n \t\t       const char *committer, timestamp_t timestamp,\n \t\t       int tz, const char *msg, void *cb_data)\n {\n-\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n-\t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n-\t       committer, timestamp, tz, msg);\n+\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n+\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n \treturn 0;\n }\n \ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 49718b7ea7f..a600bedf2cd 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -89,13 +89,12 @@ test_expect_success 'for_each_reflog()' '\n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n \thead -n1 actual | grep one &&\n-\ttail -n2 actual | head -n1 | grep recreate-main\n+\ttail -n1 actual | grep recreate-main\n '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n-\thead -n1 actual | grep recreate-main &&\n-\ttail -n2 actual | head -n1 | grep one\n+\ttail -n1 actual | grep one\n '\n \n test_expect_success 'reflog_exists(HEAD)' '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 0a87058971e..b0365c1fee0 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -74,13 +74,13 @@ test_expect_success 'for_each_reflog()' '\n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n \thead -n1 actual | grep first &&\n-\ttail -n2 actual | head -n1 | grep main.to.new\n+\ttail -n1 actual | grep main.to.new\n '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n \thead -n1 actual | grep main.to.new &&\n-\ttail -n2 actual | head -n1 | grep first\n+\ttail -n1 actual | grep first\n '\n \n test_expect_success 'reflog_exists(HEAD)' '\n-- \ngitgitgadget\n\n"},{"id":"442320","messageId":"62f5cb8a8243f9e5c2af5c148ed586a2504c8cee.1637855872.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-25T15:57:51Z","receivedAt":"2021-11-25T16:10:43Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nWe have some tests that read from files in .git/logs/ hierarchy\nwhen checking if correct reflog entries are created, but that is\ntoo specific to the files backend.  Other backends like reftable\nmay not store its reflog entries in such a \"one line per entry\"\nformat.\n\nUpdate for-each-reflog-ent test helper to produce output that\nis identical to lines in a reflog file files backend uses.\nThat way, (1) the current tests can be updated to use the test\nhelper to read the reflog entries instead of (parts of) reflog\nfiles, and perform the same inspection for correctness, and (2)\nwhen the ref backend is swapped to another backend, the updated\ntest can be used as-is to check the correctness.\n\nAdapt t1400 to use the for-each-reflog-ent test helper.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/helper/test-ref-store.c |  5 +++--\n t/t1400-update-ref.sh     | 13 ++++++++-----\n 2 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex ef244aa6b27..2b13d09a0be 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -151,8 +151,9 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n \t\t       const char *committer, timestamp_t timestamp,\n \t\t       int tz, const char *msg, void *cb_data)\n {\n-\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n-\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n+\tprintf(\"%s %s %s %\" PRItime \" %+05d%s%s\", oid_to_hex(old_oid),\n+\t       oid_to_hex(new_oid), committer, timestamp, tz,\n+\t       *msg == '\\n' ? \"\" : \"\\t\", msg);\n \treturn 0;\n }\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 0d4f73acaa8..0d46a488032 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -321,8 +321,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n EOF\n test_expect_success \"verifying $m's log (logged by touch)\" '\n-\ttest_when_finished \"rm -rf .git/$m .git/logs expect\" &&\n-\ttest_cmp expect .git/logs/$m\n+\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n+\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n+\ttest_cmp actual expect\n '\n \n test_expect_success \"create $m (logged by config)\" '\n@@ -350,8 +351,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000\n EOF\n test_expect_success \"verifying $m's log (logged by config)\" '\n-\ttest_when_finished \"rm -f .git/$m .git/logs/$m expect\" &&\n-\ttest_cmp expect .git/logs/$m\n+\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n+\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n+\ttest_cmp actual expect\n '\n \n test_expect_success 'set up for querying the reflog' '\n@@ -467,7 +469,8 @@ $h_OTHER $h_FIXED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151040 +0000\tco\n $h_FIXED $h_MERGED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151100 +0000\tcommit (merge): Merged initial commit and a later commit.\n EOF\n test_expect_success 'git commit logged updates' '\n-\ttest_cmp expect .git/logs/$m\n+\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n+\ttest_cmp expect actual\n '\n unset h_TEST h_OTHER h_FIXED h_MERGED\n \n-- \ngitgitgadget\n\n"},{"id":"442321","messageId":"CAFQ2z_N-XY6MrvYjKvkH-vNz1824p12aNeiHj8m8VPzPD0JxEA@mail.gmail.com","threadId":"56958","inReplyTo":"xmqqtug1z32j.fsf@gitster.g","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-25T16:00:20Z","receivedAt":"2021-11-25T16:14:14Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Nov 24, 2021 at 9:55 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Ehh, let's step back a bit.\n>\n> Is there anything in the common part and files backend in ref API\n> that needs to be changed to fix some bug?  I do not see anything\n> broken that needs to be fixed by \"assert that the string ends in LF\n> and doesn't need to add LF itself\" or by \"always add LF\".\n\nClearly, you and I disagree about what is logical behavior here.\n\nI've reworked the series to fit with your opinion on the matter. PTAL.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442350","messageId":"xmqqy25b73kt.fsf@gitster.g","threadId":"56958","inReplyTo":"4a86d21258918ea8124caef1cd2e435b465f67c5.1637855872.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/5] test-ref-store: don't add newline to reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-26T07:56:18Z","receivedAt":"2021-11-26T07:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> The files backend produces a newline for messages automatically, so before we\n> would print blank lines between entries.\n\nIt is misleading to say \"The files backend produces\" here.  The\npromise the API makes to the callback functions is that msg will be\na single complete line, regardless of what ref backend is used, no?\n\nUnless we plan to break the promise to the callback, forcing them to\ncompensate the differences of behaviour between backends, by making\nthe reftable backend behave differently.\n\n    The callback for each-reflog-ent iterator functions receives a\n    single complete line terminated with a newline at the end in the\n    'msg' parameter; by having a newline in the printf() format\n    string, we have been emitting a blank line between entries.\n\n    Fix it.\n\nor something, I think.\n\n> -\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n> -\t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n> -\t       committer, timestamp, tz, msg);\n> +\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n> +\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n>  \treturn 0;\n>  }\n>  \n> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\n> index 49718b7ea7f..a600bedf2cd 100755\n> --- a/t/t1405-main-ref-store.sh\n> +++ b/t/t1405-main-ref-store.sh\n> @@ -89,13 +89,12 @@ test_expect_success 'for_each_reflog()' '\n>  test_expect_success 'for_each_reflog_ent()' '\n>  \t$RUN for-each-reflog-ent HEAD >actual &&\n>  \thead -n1 actual | grep one &&\n> -\ttail -n2 actual | head -n1 | grep recreate-main\n> +\ttail -n1 actual | grep recreate-main\n\nAnd I do agree this makes it much more pleasant than before (not\njust this one but all the changes to the tests).\n\nThanks.\n\n"},{"id":"442351","messageId":"xmqqsfvj73ap.fsf@gitster.g","threadId":"56958","inReplyTo":"62f5cb8a8243f9e5c2af5c148ed586a2504c8cee.1637855872.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-26T08:02:22Z","receivedAt":"2021-11-26T08:04:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> -\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n> -\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n> +\tprintf(\"%s %s %s %\" PRItime \" %+05d%s%s\", oid_to_hex(old_oid),\n> +\t       oid_to_hex(new_oid), committer, timestamp, tz,\n> +\t       *msg == '\\n' ? \"\" : \"\\t\", msg);\n\nSo, we used to show the timezone as an integer casually, but now we\nmake sure we use the +/- prefix, with 0 filled 4 digits, that is\nconsistent with how we record the tz in different places (like the\nlow-level object header, or the user-visible header lines in commit\nand tag objects).\n\n>  \treturn 0;\n>  }\n>  \n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 0d4f73acaa8..0d46a488032 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -321,8 +321,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n>  $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n>  EOF\n>  test_expect_success \"verifying $m's log (logged by touch)\" '\n> -\ttest_when_finished \"rm -rf .git/$m .git/logs expect\" &&\n> -\ttest_cmp expect .git/logs/$m\n> +\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n> +\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n\nmicronit: lose SP between redirection '>' and its target 'actual'.\n\n> +\ttest_cmp actual expect\n\nAnd the +0000 will be checked by comparing with \"expect\" file.  Nice.\n\n>  '\n>  \n>  test_expect_success \"create $m (logged by config)\" '\n> @@ -350,8 +351,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n>  $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000\n>  EOF\n>  test_expect_success \"verifying $m's log (logged by config)\" '\n> -\ttest_when_finished \"rm -f .git/$m .git/logs/$m expect\" &&\n> -\ttest_cmp expect .git/logs/$m\n> +\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n> +\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n\nDitto.\n\n> +\ttest_cmp actual expect\n>  '\n>  \n>  test_expect_success 'set up for querying the reflog' '\n> @@ -467,7 +469,8 @@ $h_OTHER $h_FIXED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151040 +0000\tco\n>  $h_FIXED $h_MERGED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151100 +0000\tcommit (merge): Merged initial commit and a later commit.\n>  EOF\n>  test_expect_success 'git commit logged updates' '\n> -\ttest_cmp expect .git/logs/$m\n> +\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n\nThis one is good.\n\n> +\ttest_cmp expect actual\n>  '\n>  unset h_TEST h_OTHER h_FIXED h_MERGED\n\nThanks.\n"},{"id":"442352","messageId":"xmqqilwf72nf.fsf@gitster.g","threadId":"56958","inReplyTo":"0288e743eb2e96e2effd6b0b90c6f885009bf337.1637855872.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-26T08:16:20Z","receivedAt":"2021-11-26T08:18:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> On iteration, the reflog message is always terminated by a newline. Trim it to\n> avoid clobbering the console with is this extra newline.\n>\n> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n> ---\n>  refs/debug.c | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs/debug.c b/refs/debug.c\n> index 8667c640237..2631210795b 100644\n> --- a/refs/debug.c\n> +++ b/refs/debug.c\n> @@ -284,15 +284,21 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n>  \tint ret;\n>  \tchar o[GIT_MAX_HEXSZ + 1] = \"null\";\n>  \tchar n[GIT_MAX_HEXSZ + 1] = \"null\";\n> +\tstruct strbuf trimmed = STRBUF_INIT;\n>  \tif (old_oid)\n>  \t\toid_to_hex_r(o, old_oid);\n>  \tif (new_oid)\n>  \t\toid_to_hex_r(n, new_oid);\n>  \n> +\tstrbuf_addstr(&trimmed, msg);\n>  \tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n>  \t\t      dbg->cb_data);\n> -\ttrace_printf_key(&trace_refs, \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n> -\t\tdbg->refname, ret, o, n, committer, (long int)timestamp, msg);\n> +\tstrbuf_trim_trailing_newline(&trimmed);\n\nThe API promises to have only LF, not CRLF, at the end, so\nstrbuf_trim_trailing_newline() is a bit overkill (and if payload\nhappened to end with CR, we would lose it).\n\n> +\ttrace_printf_key(&trace_refs,\n> +\t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n> +\t\t\t dbg->refname, ret, o, n, committer,\n> +\t\t\t (long int)timestamp, trimmed.buf);\n> +\tstrbuf_release(&trimmed);\n>  \treturn ret;\n>  }\n\nCan we use counted bytes in trace_printf()?  If we can, it would be\nsimpler to just scan \"msg\" for LF and then show only the span\nbetween the beginning of the string and the found LF using \"%.*s\",\nperhaps like this?\n\n refs/debug.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git c/refs/debug.c w/refs/debug.c\nindex 8667c64023..c97eeb5740 100644\n--- c/refs/debug.c\n+++ w/refs/debug.c\n@@ -284,15 +284,20 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n \tint ret;\n \tchar o[GIT_MAX_HEXSZ + 1] = \"null\";\n \tchar n[GIT_MAX_HEXSZ + 1] = \"null\";\n+\tint msglen;\n+\n \tif (old_oid)\n \t\toid_to_hex_r(o, old_oid);\n \tif (new_oid)\n \t\toid_to_hex_r(n, new_oid);\n \n+\tmsglen = strchrnul(msg, '\\n') - msg;\n \tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n \t\t      dbg->cb_data);\n-\ttrace_printf_key(&trace_refs, \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n-\t\tdbg->refname, ret, o, n, committer, (long int)timestamp, msg);\n+\ttrace_printf_key(&trace_refs,\n+\t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%.*s\\\"\\n\",\n+\t\t\t dbg->refname, ret, o, n, committer, (long int)timestamp,\n+\t\t\t (int)msglen, msg);\n \treturn ret;\n }\n \n"},{"id":"442353","messageId":"xmqqczmn71ru.fsf_-_@gitster.g","threadId":"56958","inReplyTo":"xmqqzgpu7grv.fsf@gitster.g","subject":"Re* [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-26T08:35:17Z","receivedAt":"2021-11-26T08:37:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think what deserves such a comment more is the prototype for\n> each_reflog_ent_fn describing what the parameters to that callback\n> function, to help the callers of the iterator what to expect.  That\n> is the end-user facing part.\n\nPerhaps something along this line.\n\n--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---\nSubject: [PATCH] refs: document callback for reflog-ent iterators\n\nrefs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()\nfunctions take a callback function that gets called with the details\nof each reflog entry.  Its parameters were not documented beyond\ntheir names.  Elaborate a bit on each of them.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n refs.h | 22 +++++++++++++++++++++-\n 1 file changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git c/refs.h w/refs.h\nindex 48970dfc7e..4fa97d77cf 100644\n--- c/refs.h\n+++ w/refs.h\n@@ -462,7 +462,27 @@ int delete_reflog(const char *refname);\n \n /*\n  * Callback to process a reflog entry found by the iteration functions (see\n- * below)\n+ * below).\n+ *\n+ * The committer parameter is a single string, in the form\n+ * \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" (without double quotes).\n+ *\n+ * The timestamp parameter gives the seconds since epoch when the reflog\n+ * entry was created.\n+ *\n+ * The tz parameter is an up to 4 digits integer, whose absolute value\n+ * gives the hour and minute offset from GMT (the lower 2 digits are\n+ * minutes, the higher 2 digits are hours).  A negative tz means west of,\n+ * and a positive tz means east of GMT.\n+ *\n+ * The msg parameter is a single complete line; a reflog message given\n+ * to refs_delete_ref, refs_update_ref, etc. is returned to the\n+ * callback normalized---each run of whitespaces are squashed into a\n+ * single whitespace, trailing whitespace, if exists, is trimmed, and\n+ * then a single LF is added at the end.\n+ *\n+ * The cb_data is a caller-supplied pointer given to the iterator\n+ * functions.\n  */\n typedef int each_reflog_ent_fn(\n \t\tstruct object_id *old_oid, struct object_id *new_oid,\n"},{"id":"442419","messageId":"211128.86r1b0gnv6.gmgdl@evledraar.gmail.com","threadId":"56958","inReplyTo":"xmqqczmn71ru.fsf_-_@gitster.g","subject":"Re: Re* [PATCH 2/4] refs: trim newline from reflog message","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-28T17:50:15Z","receivedAt":"2021-11-28T18:08:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 26 2021, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> I think what deserves such a comment more is the prototype for\n>> each_reflog_ent_fn describing what the parameters to that callback\n>> function, to help the callers of the iterator what to expect.  That\n>> is the end-user facing part.\n>\n> Perhaps something along this line.\n>\n> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---\n> Subject: [PATCH] refs: document callback for reflog-ent iterators\n>\n> refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()\n> functions take a callback function that gets called with the details\n> of each reflog entry.  Its parameters were not documented beyond\n> their names.  Elaborate a bit on each of them.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  refs.h | 22 +++++++++++++++++++++-\n>  1 file changed, 21 insertions(+), 1 deletion(-)\n>\n> diff --git c/refs.h w/refs.h\n> index 48970dfc7e..4fa97d77cf 100644\n> --- c/refs.h\n> +++ w/refs.h\n> @@ -462,7 +462,27 @@ int delete_reflog(const char *refname);\n>  \n>  /*\n>   * Callback to process a reflog entry found by the iteration functions (see\n> - * below)\n> + * below).\n> + *\n> + * The committer parameter is a single string, in the form\n> + * \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" (without double quotes).\n> + *\n> + * The timestamp parameter gives the seconds since epoch when the reflog\n> + * entry was created.\n> + *\n> + * The tz parameter is an up to 4 digits integer, whose absolute value\n> + * gives the hour and minute offset from GMT (the lower 2 digits are\n> + * minutes, the higher 2 digits are hours).  A negative tz means west of,\n> + * and a positive tz means east of GMT.\n\nThe Line and Attu islands beg to differ :)\n\nNitpicking aside, perhaps these two pargaraphs would be better as simply\nreplaced by:\n\n    Consult \"Git internal format\" in git-commit-tree(1) for\n    details about the \"<unix timestamp> <time zone offset>\" format, and\n    see show_one_reflog_ent() for the parsing function.\n"},{"id":"442420","messageId":"xmqqpmqk5co6.fsf@gitster.g","threadId":"56958","inReplyTo":"211128.86r1b0gnv6.gmgdl@evledraar.gmail.com","subject":"Re: Re* [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-28T18:59:37Z","receivedAt":"2021-11-28T19:01:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Nitpicking aside, perhaps these two pargaraphs would be better as simply\n> replaced by:\n>\n>     Consult \"Git internal format\" in git-commit-tree(1) for\n>     details about the \"<unix timestamp> <time zone offset>\" format, and\n>     see show_one_reflog_ent() for the parsing function.\n\nMuch nicer; this is developer facing and reducing risk of\ninconsistency by incorrectly duplicating is much more important than\nhaving the info available in a single place.\n\nThanks.\n"},{"id":"442421","messageId":"xmqqk0gs5bgw.fsf@gitster.g","threadId":"56958","inReplyTo":"xmqqpmqk5co6.fsf@gitster.g","subject":"Re* [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-28T19:25:35Z","receivedAt":"2021-11-28T19:27:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> Nitpicking aside, perhaps these two pargaraphs would be better as simply\n>> replaced by:\n>>\n>>     Consult \"Git internal format\" in git-commit-tree(1) for\n>>     details about the \"<unix timestamp> <time zone offset>\" format, and\n>>     see show_one_reflog_ent() for the parsing function.\n>\n> Much nicer; this is developer facing and reducing risk of\n> inconsistency by incorrectly duplicating is much more important than\n> having the info available in a single place.\n\nHaving actually read the description, I do not think\n\"Documentation/date-formats.txt::Git internal format\" is a good fit.\nWe are describing the format of a single string there (e.g. <unix\ntimestamp> and <time zone offset> form a single string with one SP\nin between), but the parameters are integral types.\n\nSpecifically because the \"Git internal format\" is textual, an\nexample \"+0100\" given there makes it sufficiently clear that the\noffset is a \"a sign followed by 4 digits\" string, but those who need\nto use the value in \"int tz\" must know that is represented as a\npositive one hundred, not sixty, which does not have to be captured\nthere for the intended audience of date-formats.txt pagelet.\n\nHere is what I came up with\n\n---- >8 -------- >8 -------- >8 -------- >8 -------- >8 ----\nSubject: [PATCH v2] refs: document callback for reflog-ent iterators\n\nrefs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()\nfunctions take a callback function that gets called with the details\nof each reflog entry.  Its parameters were not documented beyond\ntheir names.  Elaborate a bit on each of them.\n\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * rewrite timestamp and tz, adjusting the internal format's\n   description from Documentation/date-formats.txt pagelet, as\n   pointed out by Ævar.\n\n refs.h | 24 +++++++++++++++++++++++-\n 1 file changed, 23 insertions(+), 1 deletion(-)\n\n The indented patch shows the part that was changed from the\n previous one.\n\n    diff --git i/refs.h w/refs.h\n    index 4fa97d77cf..f6fed5c7d8 100644\n    --- i/refs.h\n    +++ w/refs.h\n    @@ -467,13 +467,15 @@ int delete_reflog(const char *refname);\n      * The committer parameter is a single string, in the form\n      * \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" (without double quotes).\n      *\n    - * The timestamp parameter gives the seconds since epoch when the reflog\n    - * entry was created.\n    - *\n    - * The tz parameter is an up to 4 digits integer, whose absolute value\n    - * gives the hour and minute offset from GMT (the lower 2 digits are\n    - * minutes, the higher 2 digits are hours).  A negative tz means west of,\n    - * and a positive tz means east of GMT.\n    + * The timestamp parameter gives the time when entry was created as the number\n    + * of seconds since the UNIX epoch.\n    + *\n    + * The tz parameter gives the timezone offset for the user who created\n    + * the reflog entry, and its value gives a positive or negative offset\n    + * from UTC.  Its absolute value is formed by multiplying the hour\n    + * part by 100 and adding the minute part.  For example, 1 hour ahead\n    + * of UTC, CET == \"+0100\", is represented as positive one hundred (not\n    + * postiive sixty).\n      *\n      * The msg parameter is a single complete line; a reflog message given\n      * to refs_delete_ref, refs_update_ref, etc. is returned to the\n\ndiff --git c/refs.h w/refs.h\nindex 48970dfc7e..f6fed5c7d8 100644\n--- c/refs.h\n+++ w/refs.h\n@@ -462,7 +462,29 @@ int delete_reflog(const char *refname);\n \n /*\n  * Callback to process a reflog entry found by the iteration functions (see\n- * below)\n+ * below).\n+ *\n+ * The committer parameter is a single string, in the form\n+ * \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" (without double quotes).\n+ *\n+ * The timestamp parameter gives the time when entry was created as the number\n+ * of seconds since the UNIX epoch.\n+ *\n+ * The tz parameter gives the timezone offset for the user who created\n+ * the reflog entry, and its value gives a positive or negative offset\n+ * from UTC.  Its absolute value is formed by multiplying the hour\n+ * part by 100 and adding the minute part.  For example, 1 hour ahead\n+ * of UTC, CET == \"+0100\", is represented as positive one hundred (not\n+ * postiive sixty).\n+ *\n+ * The msg parameter is a single complete line; a reflog message given\n+ * to refs_delete_ref, refs_update_ref, etc. is returned to the\n+ * callback normalized---each run of whitespaces are squashed into a\n+ * single whitespace, trailing whitespace, if exists, is trimmed, and\n+ * then a single LF is added at the end.\n+ *\n+ * The cb_data is a caller-supplied pointer given to the iterator\n+ * functions.\n  */\n typedef int each_reflog_ent_fn(\n \t\tstruct object_id *old_oid, struct object_id *new_oid,\n"},{"id":"442429","messageId":"xmqqa6hn4rsj.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_N-XY6MrvYjKvkH-vNz1824p12aNeiHj8m8VPzPD0JxEA@mail.gmail.com","subject":"Re: [PATCH 2/4] refs: trim newline from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-29T02:30:36Z","receivedAt":"2021-11-29T02:32:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> On Wed, Nov 24, 2021 at 9:55 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> Ehh, let's step back a bit.\n>>\n>> Is there anything in the common part and files backend in ref API\n>> that needs to be changed to fix some bug?  I do not see anything\n>> broken that needs to be fixed by \"assert that the string ends in LF\n>> and doesn't need to add LF itself\" or by \"always add LF\".\n>\n> Clearly, you and I disagree about what is logical behavior here.\n>\n> I've reworked the series to fit with your opinion on the matter. PTAL.\n\nThanks.\n\nI find both are logical and internally consistent, so I prefer a\nchoice that requires fewer changes to what is already working.\n"},{"id":"442433","messageId":"211129.86h7bvgvqt.gmgdl@evledraar.gmail.com","threadId":"56958","inReplyTo":"xmqqk0gs5bgw.fsf@gitster.g","subject":"Re: Re* [PATCH 2/4] refs: trim newline from reflog message","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-29T08:39:58Z","receivedAt":"2021-11-29T09:42:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 28 2021, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>>\n>>> Nitpicking aside, perhaps these two pargaraphs would be better as simply\n>>> replaced by:\n>>>\n>>>     Consult \"Git internal format\" in git-commit-tree(1) for\n>>>     details about the \"<unix timestamp> <time zone offset>\" format, and\n>>>     see show_one_reflog_ent() for the parsing function.\n>>\n>> Much nicer; this is developer facing and reducing risk of\n>> inconsistency by incorrectly duplicating is much more important than\n>> having the info available in a single place.\n>\n> Having actually read the description, I do not think\n> \"Documentation/date-formats.txt::Git internal format\" is a good fit.\n> We are describing the format of a single string there (e.g. <unix\n> timestamp> and <time zone offset> form a single string with one SP\n> in between), but the parameters are integral types.\n>\n> Specifically because the \"Git internal format\" is textual, an\n> example \"+0100\" given there makes it sufficiently clear that the\n> offset is a \"a sign followed by 4 digits\" string, but those who need\n> to use the value in \"int tz\" must know that is represented as a\n> positive one hundred, not sixty, which does not have to be captured\n> there for the intended audience of date-formats.txt pagelet.\n>\n> Here is what I came up with\n>\n> ---- >8 -------- >8 -------- >8 -------- >8 -------- >8 ----\n> Subject: [PATCH v2] refs: document callback for reflog-ent iterators\n>\n> refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()\n> functions take a callback function that gets called with the details\n> of each reflog entry.  Its parameters were not documented beyond\n> their names.  Elaborate a bit on each of them.\n>\n> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * rewrite timestamp and tz, adjusting the internal format's\n>    description from Documentation/date-formats.txt pagelet, as\n>    pointed out by Ævar.\n>\n>  refs.h | 24 +++++++++++++++++++++++-\n>  1 file changed, 23 insertions(+), 1 deletion(-)\n>\n>  The indented patch shows the part that was changed from the\n>  previous one.\n>\n>     diff --git i/refs.h w/refs.h\n>     index 4fa97d77cf..f6fed5c7d8 100644\n>     --- i/refs.h\n>     +++ w/refs.h\n>     @@ -467,13 +467,15 @@ int delete_reflog(const char *refname);\n>       * The committer parameter is a single string, in the form\n>       * \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" (without double quotes).\n>       *\n>     - * The timestamp parameter gives the seconds since epoch when the reflog\n>     - * entry was created.\n>     - *\n>     - * The tz parameter is an up to 4 digits integer, whose absolute value\n>     - * gives the hour and minute offset from GMT (the lower 2 digits are\n>     - * minutes, the higher 2 digits are hours).  A negative tz means west of,\n>     - * and a positive tz means east of GMT.\n>     + * The timestamp parameter gives the time when entry was created as the number\n>     + * of seconds since the UNIX epoch.\n>     + *\n>     + * The tz parameter gives the timezone offset for the user who created\n>     + * the reflog entry, and its value gives a positive or negative offset\n>     + * from UTC.  Its absolute value is formed by multiplying the hour\n>     + * part by 100 and adding the minute part.  For example, 1 hour ahead\n>     + * of UTC, CET == \"+0100\", is represented as positive one hundred (not\n>     + * postiive sixty).\n>       *\n>       * The msg parameter is a single complete line; a reflog message given\n>       * to refs_delete_ref, refs_update_ref, etc. is returned to the\n\nThanks, this version of these docs looks good to me!\n"},{"id":"442436","messageId":"211129.86czmjgtfd.gmgdl@evledraar.gmail.com","threadId":"56958","inReplyTo":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-29T09:50:33Z","receivedAt":"2021-11-29T10:59:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 25 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> This helps for reftable support, and will help if we want to reconsider\n> under which conditions reflogs get created/updated.\n\nHaving looked at this in a bit more detail than last time\n(https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)\nI applaud the goals, but to be blunt the specific approach just seems a\nbit backwards to me.\n\nAs noted in that message I have patches to tweak the \"verbose\" mode to\nbe backend-independent, which as we see from your series is one thing in\nthe files backend that consumes the \"message\" and assumes things about\nnewlines.\n\nSimilarly from peeking at the reflog code I see it stores name/email\nseparately in a struct, but the refs.[ch] API wants to pass \"name\n<email>\" as one string.\n\nThe below working patch shows that we have three consumers of that\nspecific format, so we can just xstrfmt() that instead of xstrdup()-ing\nit in one case, and tweak the printf in the other two, and then we pass\nit as two parameters.\n\nSo it's truly backend-independent now, i.e. a \"real DB\" like reftable,\nSQL or whatever would store those in two separate fields.\n\nSo on the \"seems backwards\" above: If you apply this (and it passes all\ntests) and look at what \"message\" is used for you're guaranteed to find\nall the callers, and only a handful more of them are using that for\nanything.\n\nSo I'd really expect a series like this to just change the\nstrbuf_getwholeline() in files_for_each_reflog_ent() to be a\nstrbuf_getline(). Then flip everything downstream that expects a '\\n' to\neither not, or to add if needed.\n\nE.g. the test helper that's now adding a newline would be correct, and\nwe'd just need to tweak the file backend specific stuff that's now\nskipping the addition of newlines to add them instead.\n\nAnyway, changing the callbacks is quite verbose, and I just did this\nident->name/email change expecting that it would be more difficult.\n\nBut likewise the big benefit is that we *are* forced to tweak all\ncallers, so we're more likely to catch any subtle regressions, and it's\nthus easier to review once we get past the verbosity.\n\nI also wonder if for name/email/message a bit more tweaking of this to\nmake them all const char */size_t pairs wouldn't result in a much better\nend-state. I.e. now we read into a strbuf in a loop in files-backend.c\nand xstrdup() it.\n\nPerhaps reftable is capable of just handing the underlying code pointers\ninto the mmap()'d file, so we could even skip all (re)allocations? Or if\nnot, that certainly seems like a sensible thing to anticipate in a\nbackend-independent interface. We could do that in the file backend if\nwe were a bit smarter and used mmap() instead of the\nfopen()/read-in-a-loop pattern.\n\nJust my 0.02, if you're interested in running with the below assume my\nSOB.\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 27b9e78094d..2ccb20ce057 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -487,8 +487,9 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,\n }\n \n static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,\n-\t\tconst char *email, timestamp_t timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+\t\t\t\t  const char *committer_name, const char *committer_email,\n+\t\t\t\t  timestamp_t timestamp, int tz,\n+\t\t\t\t  const char *message, void *cb_data)\n {\n \tconst char *refname = cb_data;\n \ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 175c83e7cc2..0cf7d484613 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -295,7 +295,8 @@ static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit\n  * Return true iff the specified reflog entry should be expired.\n  */\n static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n-\t\t\t\t    const char *email, timestamp_t timestamp, int tz,\n+\t\t\t\t    const char *committer_name, const char *committer_email,\n+\t\t\t\t    timestamp_t timestamp, int tz,\n \t\t\t\t    const char *message, void *cb_data)\n {\n \tstruct expire_reflog_policy_cb *cb = cb_data;\n@@ -659,8 +660,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n }\n \n static int count_reflog_ent(struct object_id *ooid, struct object_id *noid,\n-\t\tconst char *email, timestamp_t timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+\t\t\t    const char *committer_name, const char *committer_email,\n+\t\t\t    timestamp_t timestamp, int tz,\n+\t\t\t    const char *message, void *cb_data)\n {\n \tstruct expire_reflog_policy_cb *cb = cb_data;\n \tif (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex a0ccc8654df..b6963050b6d 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -620,7 +620,8 @@ static int apply_stash(int argc, const char **argv, const char *prefix)\n }\n \n static int reject_reflog_ent(struct object_id *ooid, struct object_id *noid,\n-\t\t\t     const char *email, timestamp_t timestamp, int tz,\n+\t\t\t     const char *committer_name, const char *committer_email,\n+\t\t\t     timestamp_t timestamp, int tz,\n \t\t\t     const char *message, void *cb_data)\n {\n \treturn 1;\ndiff --git a/commit.c b/commit.c\nindex 551de4903c1..ea97f639cc4 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -937,8 +937,9 @@ static void add_one_commit(struct object_id *oid, struct rev_collect *revs)\n }\n \n static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n-\t\t\t\t  const char *ident, timestamp_t timestamp,\n-\t\t\t\t  int tz, const char *message, void *cbdata)\n+\t\t\t\t  const char *author, const char *email,\n+\t\t\t\t  timestamp_t timestamp, int tz,\n+\t\t\t\t  const char *message, void *cbdata)\n {\n \tstruct rev_collect *revs = cbdata;\n \ndiff --git a/object-name.c b/object-name.c\nindex fdff4601b2c..56b131fb4a7 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1287,7 +1287,8 @@ struct grab_nth_branch_switch_cbdata {\n };\n \n static int grab_nth_branch_switch(struct object_id *ooid, struct object_id *noid,\n-\t\t\t\t  const char *email, timestamp_t timestamp, int tz,\n+\t\t\t\t  const char *author, const char *email,\n+\t\t\t\t  timestamp_t timestamp, int tz,\n \t\t\t\t  const char *message, void *cb_data)\n {\n \tstruct grab_nth_branch_switch_cbdata *cb = cb_data;\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 8ac4b284b6b..12865f4cb84 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -20,8 +20,9 @@ struct complete_reflogs {\n };\n \n static int read_one_reflog(struct object_id *ooid, struct object_id *noid,\n-\t\tconst char *email, timestamp_t timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+\t\t\t   const char *author, const char *email,\n+\t\t\t   timestamp_t timestamp, int tz,\n+\t\t\t   const char *message, void *cb_data)\n {\n \tstruct complete_reflogs *array = cb_data;\n \tstruct reflog_info *item;\n@@ -30,7 +31,7 @@ static int read_one_reflog(struct object_id *ooid, struct object_id *noid,\n \titem = array->items + array->nr;\n \toidcpy(&item->ooid, ooid);\n \toidcpy(&item->noid, noid);\n-\titem->email = xstrdup(email);\n+\titem->email = xstrfmt(\"%s <%s>\", author, email);\n \titem->timestamp = timestamp;\n \titem->tz = tz;\n \titem->message = xstrdup(message);\ndiff --git a/refs.c b/refs.c\nindex d7cc0a23a3b..8d95fd2a6bb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -894,8 +894,9 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,\n }\n \n static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n-\t\tconst char *email, timestamp_t timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+\t\t\t   const char *committer_name, const char *committer_email,\n+\t\t\t   timestamp_t timestamp, int tz,\n+\t\t\t   const char *message, void *cb_data)\n {\n \tstruct read_ref_at_cb *cb = cb_data;\n \tint reached_count;\n@@ -936,8 +937,9 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n }\n \n static int read_ref_at_ent_newest(struct object_id *ooid, struct object_id *noid,\n-\t\t\t\t  const char *email, timestamp_t timestamp,\n-\t\t\t\t  int tz, const char *message, void *cb_data)\n+\t\t\t\t  const char *author, const char *committer_email,\n+\t\t\t\t  timestamp_t timestamp, int tz,\n+\t\t\t\t  const char *message, void *cb_data)\n {\n \tstruct read_ref_at_cb *cb = cb_data;\n \n@@ -948,8 +950,9 @@ static int read_ref_at_ent_newest(struct object_id *ooid, struct object_id *noid\n }\n \n static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,\n-\t\t\t\t  const char *email, timestamp_t timestamp,\n-\t\t\t\t  int tz, const char *message, void *cb_data)\n+\t\t\t\t  const char *committer_name, const char *committer_email,\n+\t\t\t\t  timestamp_t timestamp, int tz,\n+\t\t\t\t  const char *message, void *cb_data)\n {\n \tstruct read_ref_at_cb *cb = cb_data;\n \ndiff --git a/refs.h b/refs.h\nindex d5099d4984e..a34e98a4123 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -463,8 +463,9 @@ int delete_reflog(const char *refname);\n  */\n typedef int each_reflog_ent_fn(\n \t\tstruct object_id *old_oid, struct object_id *new_oid,\n-\t\tconst char *committer, timestamp_t timestamp,\n-\t\tint tz, const char *msg, void *cb_data);\n+\t\tconst char *committer_name, const char *committer_email,\n+\t\ttimestamp_t timestamp, int tz,\n+\t\tconst char *msg, void *cb_data);\n \n /* Iterate over reflog entries in the log for `refname`. */\n \n@@ -807,7 +808,7 @@ typedef void reflog_expiry_prepare_fn(const char *refname,\n \t\t\t\t      void *cb_data);\n typedef int reflog_expiry_should_prune_fn(struct object_id *ooid,\n \t\t\t\t\t  struct object_id *noid,\n-\t\t\t\t\t  const char *email,\n+\t\t\t\t\t  const char *author, const char *email,\n \t\t\t\t\t  timestamp_t timestamp, int tz,\n \t\t\t\t\t  const char *message, void *cb_data);\n typedef void reflog_expiry_cleanup_fn(void *cb_data);\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 8667c640237..dc4b417abeb 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -277,8 +277,9 @@ struct debug_reflog {\n \n static int debug_print_reflog_ent(struct object_id *old_oid,\n \t\t\t\t  struct object_id *new_oid,\n-\t\t\t\t  const char *committer, timestamp_t timestamp,\n-\t\t\t\t  int tz, const char *msg, void *cb_data)\n+\t\t\t\t  const char *committer, const char *committer_email,\n+\t\t\t\t  timestamp_t timestamp, int tz,\n+\t\t\t\t  const char *msg, void *cb_data)\n {\n \tstruct debug_reflog *dbg = (struct debug_reflog *)cb_data;\n \tint ret;\n@@ -289,10 +290,11 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n \tif (new_oid)\n \t\toid_to_hex_r(n, new_oid);\n \n-\tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n-\t\t      dbg->cb_data);\n-\ttrace_printf_key(&trace_refs, \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n-\t\tdbg->refname, ret, o, n, committer, (long int)timestamp, msg);\n+\tret = dbg->fn(old_oid, new_oid, committer, committer_email,\n+\t\t      timestamp, tz, msg, dbg->cb_data);\n+\ttrace_printf_key(&trace_refs, \"reflog_ent %s (ret %d): %s -> %s, %s <%s> %ld \\\"%s\\\"\\n\",\n+\t\tdbg->refname, ret, o, n, committer, committer_email,\n+\t\t\t (long int)timestamp, msg);\n \treturn ret;\n }\n \n@@ -374,12 +376,13 @@ static void debug_reflog_expiry_prepare(const char *refname,\n \n static int debug_reflog_expiry_should_prune_fn(struct object_id *ooid,\n \t\t\t\t\t       struct object_id *noid,\n-\t\t\t\t\t       const char *email,\n+\t\t\t\t\t       const char *committer_name, const char *committer_email,\n \t\t\t\t\t       timestamp_t timestamp, int tz,\n \t\t\t\t\t       const char *message, void *cb_data) {\n \tstruct debug_reflog_expiry_should_prune *prune = cb_data;\n \n-\tint result = prune->should_prune(ooid, noid, email, timestamp, tz, message, prune->cb_data);\n+\tint result = prune->should_prune(ooid, noid, committer_name, committer_email,\n+\t\t\t\t\t timestamp, tz, message, prune->cb_data);\n \ttrace_printf_key(&trace_refs, \"reflog_expire_should_prune: %s %ld: %d\\n\", message, (long int) timestamp, result);\n \treturn result;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 151b0056fe5..d15f3342c31 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1931,7 +1931,10 @@ static int files_delete_reflog(struct ref_store *ref_store,\n static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)\n {\n \tstruct object_id ooid, noid;\n-\tchar *email_end, *message;\n+\tconst char *name;\n+\tchar *email;\n+\tchar *email_end;\n+\tchar *message;\n \ttimestamp_t timestamp;\n \tint tz;\n \tconst char *p = sb->buf;\n@@ -1940,6 +1943,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n \tif (!sb->len || sb->buf[sb->len - 1] != '\\n' ||\n \t    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||\n \t    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||\n+\t    !(name = p) ||\n+\t    !(email = strchr(p, '<')) ||\n \t    !(email_end = strchr(p, '>')) ||\n \t    email_end[1] != ' ' ||\n \t    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||\n@@ -1948,13 +1953,20 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n \t    !isdigit(message[2]) || !isdigit(message[3]) ||\n \t    !isdigit(message[4]) || !isdigit(message[5]))\n \t\treturn 0; /* corrupt? */\n-\temail_end[1] = '\\0';\n+\t/* \\0 the SP before \"<\", marks end of \"name\"... */\n+\t*(email - 1) = '\\0';\n+\t/* ... and advance past the opening... \"<\" */\n+\temail++;\n+\t/* ...and stop at the \">\" */\n+\temail_end[0] = '\\0';\n+\n \ttz = strtol(message + 1, NULL, 10);\n \tif (message[6] != '\\t')\n \t\tmessage += 6;\n \telse\n \t\tmessage += 7;\n-\treturn fn(&ooid, &noid, p, timestamp, tz, message, cb_data);\n+\treturn fn(&ooid, &noid, name, email, timestamp, tz, message,\n+\t\t  cb_data);\n }\n \n static char *find_beginning_of_line(char *bob, char *scan)\n@@ -3047,7 +3059,8 @@ struct expire_reflog_cb {\n };\n \n static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n-\t\t\t     const char *email, timestamp_t timestamp, int tz,\n+\t\t\t     const char *author, const char *email,\n+\t\t\t     timestamp_t timestamp, int tz,\n \t\t\t     const char *message, void *cb_data)\n {\n \tstruct expire_reflog_cb *cb = cb_data;\n@@ -3056,7 +3069,8 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \tif (cb->flags & EXPIRE_REFLOGS_REWRITE)\n \t\tooid = &cb->last_kept_oid;\n \n-\tif ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,\n+\tif ((*cb->should_prune_fn)(ooid, noid, author, email,\n+\t\t\t\t   timestamp, tz,\n \t\t\t\t   message, policy_cb)) {\n \t\tif (!cb->newlog)\n \t\t\tprintf(\"would prune %s\", message);\n@@ -3064,9 +3078,9 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \t\t\tprintf(\"prune %s\", message);\n \t} else {\n \t\tif (cb->newlog) {\n-\t\t\tfprintf(cb->newlog, \"%s %s %s %\"PRItime\" %+05d\\t%s\",\n+\t\t\tfprintf(cb->newlog, \"%s %s %s <%s> %\"PRItime\" %+05d\\t%s\",\n \t\t\t\toid_to_hex(ooid), oid_to_hex(noid),\n-\t\t\t\temail, timestamp, tz, message);\n+\t\t\t\tauthor, email, timestamp, tz, message);\n \t\t\toidcpy(&cb->last_kept_oid, noid);\n \t\t}\n \t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\ndiff --git a/remote.c b/remote.c\nindex f958543d707..3cd0cdce879 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2428,8 +2428,9 @@ struct check_and_collect_until_cb_data {\n \n /* Get the timestamp of the latest entry. */\n static int peek_reflog(struct object_id *o_oid, struct object_id *n_oid,\n-\t\t       const char *ident, timestamp_t timestamp,\n-\t\t       int tz, const char *message, void *cb_data)\n+\t\t       const char *committer_name, const char *committer_email,\n+\t\t       timestamp_t timestamp, int tz,\n+\t\t       const char *message, void *cb_data)\n {\n \ttimestamp_t *ts = cb_data;\n \t*ts = timestamp;\n@@ -2438,8 +2439,9 @@ static int peek_reflog(struct object_id *o_oid, struct object_id *n_oid,\n \n static int check_and_collect_until(struct object_id *o_oid,\n \t\t\t\t   struct object_id *n_oid,\n-\t\t\t\t   const char *ident, timestamp_t timestamp,\n-\t\t\t\t   int tz, const char *message, void *cb_data)\n+\t\t\t\t   const char *committer_name, const char *committer_email,\n+\t\t\t\t   timestamp_t timestamp, int tz,\n+\t\t\t\t   const char *message, void *cb_data)\n {\n \tstruct commit *commit;\n \tstruct check_and_collect_until_cb_data *cb = cb_data;\ndiff --git a/revision.c b/revision.c\nindex 1981a0859f0..2979f456cb6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1597,8 +1597,9 @@ static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)\n }\n \n static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n-\t\tconst char *email, timestamp_t timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+\t\t\t\t const char *author, const char *email,\n+\t\t\t\t timestamp_t timestamp, int tz,\n+\t\t\t\t const char *message, void *cb_data)\n {\n \thandle_one_reflog_commit(ooid, cb_data);\n \thandle_one_reflog_commit(noid, cb_data);\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex b314b81a45b..727ac008ad8 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -148,12 +148,13 @@ static int cmd_for_each_reflog(struct ref_store *refs, const char **argv)\n }\n \n static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n-\t\t       const char *committer, timestamp_t timestamp,\n-\t\t       int tz, const char *msg, void *cb_data)\n+\t\t       const char *committer, const char *committer_email,\n+\t\t       timestamp_t timestamp, int tz,\n+\t\t       const char *msg, void *cb_data)\n {\n-\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n+\tprintf(\"%s %s %s <%s> %\"PRItime\" %d %s\\n\",\n \t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n-\t       committer, timestamp, tz, msg);\n+\t       committer, committer_email, timestamp, tz, msg);\n \treturn 0;\n }\n \ndiff --git a/wt-status.c b/wt-status.c\nindex e4f29b2b4c9..a8526a4a100 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -940,7 +940,8 @@ static void wt_longstatus_print_changed(struct wt_status *s)\n }\n \n static int stash_count_refs(struct object_id *ooid, struct object_id *noid,\n-\t\t\t    const char *email, timestamp_t timestamp, int tz,\n+\t\t\t    const char *committer_name, const char *committer_email,\n+\t\t\t    timestamp_t timestamp, int tz,\n \t\t\t    const char *message, void *cb_data)\n {\n \tint *c = cb_data;\n@@ -1592,7 +1593,8 @@ struct grab_1st_switch_cbdata {\n };\n \n static int grab_1st_switch(struct object_id *ooid, struct object_id *noid,\n-\t\t\t   const char *email, timestamp_t timestamp, int tz,\n+\t\t\t   const char *committer_name , const char *committer_email,\n+\t\t\t   timestamp_t timestamp, int tz,\n \t\t\t   const char *message, void *cb_data)\n {\n \tstruct grab_1st_switch_cbdata *cb = cb_data;\n"},{"id":"442471","messageId":"xmqq1r2y7ork.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_N20ESyzkPLdGbS9q8HEHGB7_gmaX8FUBR=jGqXLGcL1Q@mail.gmail.com","subject":"Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-29T19:19:59Z","receivedAt":"2021-11-29T19:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> The API promises to have only LF, not CRLF, at the end, so\n>> strbuf_trim_trailing_newline() is a bit overkill (and if payload\n>> happened to end with CR, we would lose it).\n>\n> it would be best if there was a way to escape characters (ie. \"\\n\" =>\n> \"\\\\n\"). Do we have a function for that?\n\nMere escaping would not work in a backward compatible way, without a\ntrick.  It was envisioned that we probably could encode *and* signal\nthe fact that the message is encoded by appending a trailing SP at\nthe end of the message.  See the log message of 523fa69c (reflog:\ncleanse messages in the refs.c layer, 2020-07-10) for details.\n\nHaving said that, that is about introducing a whole new reflog\nmessage format (whose use is signalled by the trailing SP), and I\nwould prefer it to happen\n\n (1) after we integrate with reftable, and\n\n (2) implemented as an option in the normalize_reflog_message()\n     function, so that no ref backends has to worry about it.\n\noutside this topic.\n\n> I beg to differ - despite this being fewer lines of code, I think\n> pointer arithmetic is best avoided if possible.\n\nI think repeated allocation and deallocation is best avoided, and\nthat is why I recommended it.  I do not see anything to fear in\npoiter arithmetic, as long as it is done clearly (e.g. in a narrow\nscope) and correctly.\n\nIf trace_printf() does not allow counted bytes \"%.*s\", the whole\ndiscussion is moot; I didn't go back to check.\n"},{"id":"442483","messageId":"xmqqwnkq69hq.fsf@gitster.g","threadId":"56958","inReplyTo":"xmqq1r2y7ork.fsf@gitster.g","subject":"Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-29T19:35:13Z","receivedAt":"2021-11-29T19:37:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n>> On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>>\n>>> The API promises to have only LF, not CRLF, at the end, so\n>>> strbuf_trim_trailing_newline() is a bit overkill (and if payload\n>>> happened to end with CR, we would lose it).\n>>\n>> it would be best if there was a way to escape characters (ie. \"\\n\" =>\n>> \"\\\\n\"). Do we have a function for that?\n>\n> Mere escaping would not work in a backward compatible way, without a\n> trick.  It was envisioned that we probably could encode *and* signal\n> the fact that the message is encoded by appending a trailing SP at\n> the end of the message.  See the log message of 523fa69c (reflog:\n> cleanse messages in the refs.c layer, 2020-07-10) for details.\n>\n> Having said that, that is about introducing a whole new reflog\n> message format (whose use is signalled by the trailing SP), and I\n> would prefer it to happen\n>\n>  (1) after we integrate with reftable, and\n>\n>  (2) implemented as an option in the normalize_reflog_message()\n>      function, so that no ref backends has to worry about it.\n>\n> outside this topic.\n\nSorry, I seem to have forgotten to give an answer to your question,\nand having warned sufficiently clearly that now is not the time to\ndo it, I can safely do so.  If we wanted to go the C-quote route,\nthen the function we would use would be quote.c::quote_c_style().\n\nEven though 523fa69c (reflog: cleanse messages in the refs.c layer,\n2020-07-10) muses on use of urlencode, it shouldn't be taken as a\nsign that I prefer it over c-quoting, or vice versa for that matter.\n\nIt was merely an example encoding picked in the context of\nexplaining that (1) I wasn't interested in keeping a multi-line\nmessage and spewing back as-is, and (2) but it is possible to loosen\nit in the future.\n\n"},{"id":"442492","messageId":"CAFQ2z_OioNmOP+_VvP71DTDqkaVLZg01yx5QVNo+mVGXxsUJ9Q@mail.gmail.com","threadId":"56958","inReplyTo":"211129.86czmjgtfd.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-29T18:24:28Z","receivedAt":"2021-11-29T21:12:27Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> > This helps for reftable support, and will help if we want to reconsider\n> > under which conditions reflogs get created/updated.\n>\n> Having looked at this in a bit more detail than last time\n> (https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)\n> I applaud the goals, but to be blunt the specific approach just seems a\n> bit backwards to me.\n>\n> As noted in that message I have patches to tweak the \"verbose\" mode to\n> be backend-independent, which as we see from your series is one thing in\n> the files backend that consumes the \"message\" and assumes things about\n> newlines.\n\nIn v2, I went with Jun's suggestion, and left the newlines alone, just\ntrimming them in refs/debug.c .  I think that makes most of your mail\nirrelevant?\n\n> Perhaps reftable is capable of just handing the underlying code pointers\n> into the mmap()'d file, so we could even skip all (re)allocations? Or if\n> not, that certainly seems like a sensible thing to anticipate in a\n> backend-independent interface. We could do that in the file backend if\n> we were a bit smarter and used mmap() instead of the\n> fopen()/read-in-a-loop pattern.\n\nIt sounds like premature optimization. Reading reflogs is not usually\na performance sensitive operation. Also, if you hand out mmap'd\npointers, how would the reftable storage code know when it is safe to\nclose and munmap the file?\n\n>\n> Just my 0.02, if you're interested in running with the below assume my\n> SOB.\n\nWhat is SOB in this context?\n\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442536","messageId":"xmqq35ne61d0.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_OioNmOP+_VvP71DTDqkaVLZg01yx5QVNo+mVGXxsUJ9Q@mail.gmail.com","subject":"Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-29T22:30:51Z","receivedAt":"2021-11-29T22:31:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\nJust this part...\n\n>> Just my 0.02, if you're interested in running with the below assume my\n>> SOB.\n>\n> What is SOB in this context?\n\n\"Signed-off-by:\"\n"},{"id":"442547","messageId":"211129.86v90aekug.gmgdl@evledraar.gmail.com","threadId":"56958","inReplyTo":"CAFQ2z_N20ESyzkPLdGbS9q8HEHGB7_gmaX8FUBR=jGqXLGcL1Q@mail.gmail.com","subject":"Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-29T20:59:52Z","receivedAt":"2021-11-29T22:48:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 29 2021, Han-Wen Nienhuys wrote:\n\n> On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> The API promises to have only LF, not CRLF, at the end, so\n>> strbuf_trim_trailing_newline() is a bit overkill (and if payload\n>> happened to end with CR, we would lose it).\n>\n> it would be best if there was a way to escape characters (ie. \"\\n\" =>\n> \"\\\\n\"). Do we have a function for that?\n>\n>> > +     trace_printf_key(&trace_refs,\n>> > +                      \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n>> > +                      dbg->refname, ret, o, n, committer,\n>> > +                      (long int)timestamp, trimmed.buf);\n>> > +     strbuf_release(&trimmed);\n>> >       return ret;\n>> >  }\n>>\n>> Can we use counted bytes in trace_printf()?  If we can, it would be\n>> simpler to just scan \"msg\" for LF and then show only the span\n>> between the beginning of the string and the found LF using \"%.*s\",\n>> perhaps like this?\n>\n> I beg to differ - despite this being fewer lines of code, I think\n> pointer arithmetic is best avoided if possible.\n\nWe usually do this with pointer arithmetic, but the %.*s format doesn't\nrequire that, just code like:\n\n    const char *str = \"foobar\";\n    size_t len = strlen(str);\n    len -= 1; /* give me less! */\n    printf(\"%.*s\", (int)len, str);\n\nSo you can also feed it (len - 1) or whatever if you know it to end with\na character you don't want.\n\nIt's (more simply done as) pointer arithmetic if you're finding that end\nmarker with strstr() or whatever, but you can also bend over backwards\nand get a \"len\" instead through other means, and in either case I think\nit beats reallocating the whole thing (more for readability than any\noptimization reasons).\n"},{"id":"442548","messageId":"CAFQ2z_N20ESyzkPLdGbS9q8HEHGB7_gmaX8FUBR=jGqXLGcL1Q@mail.gmail.com","threadId":"56958","inReplyTo":"xmqqilwf72nf.fsf@gitster.g","subject":"Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-29T18:29:18Z","receivedAt":"2021-11-29T22:48:46Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> The API promises to have only LF, not CRLF, at the end, so\n> strbuf_trim_trailing_newline() is a bit overkill (and if payload\n> happened to end with CR, we would lose it).\n\nit would be best if there was a way to escape characters (ie. \"\\n\" =>\n\"\\\\n\"). Do we have a function for that?\n\n> > +     trace_printf_key(&trace_refs,\n> > +                      \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n> > +                      dbg->refname, ret, o, n, committer,\n> > +                      (long int)timestamp, trimmed.buf);\n> > +     strbuf_release(&trimmed);\n> >       return ret;\n> >  }\n>\n> Can we use counted bytes in trace_printf()?  If we can, it would be\n> simpler to just scan \"msg\" for LF and then show only the span\n> between the beginning of the string and the found LF using \"%.*s\",\n> perhaps like this?\n\nI beg to differ - despite this being fewer lines of code, I think\npointer arithmetic is best avoided if possible.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442583","messageId":"211130.86r1ayedhn.gmgdl@evledraar.gmail.com","threadId":"56958","inReplyTo":"CAFQ2z_OioNmOP+_VvP71DTDqkaVLZg01yx5QVNo+mVGXxsUJ9Q@mail.gmail.com","subject":"Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-29T23:28:39Z","receivedAt":"2021-11-29T23:41:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 29 2021, Han-Wen Nienhuys wrote:\n\n> On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> > This helps for reftable support, and will help if we want to reconsider\n>> > under which conditions reflogs get created/updated.\n>>\n>> Having looked at this in a bit more detail than last time\n>> (https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)\n>> I applaud the goals, but to be blunt the specific approach just seems a\n>> bit backwards to me.\n>>\n>> As noted in that message I have patches to tweak the \"verbose\" mode to\n>> be backend-independent, which as we see from your series is one thing in\n>> the files backend that consumes the \"message\" and assumes things about\n>> newlines.\n>\n> In v2, I went with Jun's suggestion, and left the newlines alone, just\n> trimming them in refs/debug.c .  I think that makes most of your mail\n> irrelevant?\n\nTo whatever immediate problem you're trying to solve? Probably. I'm\nmainly pointing out that we can make some of these APIs nicer & a bit\nmore abstract from the details of the file backend.\n\nOr, as is the case with \"ident\" v.s. \"committer_name/committer_email\" in\nthese reflog callbacks no existing caller actually cares about that\nformat, so if/when we get to that reftable integration (which seems to\nhave the two as seperate fields) splitting those up in refs.[ch]\nprobably makes sense.\n\n>> Perhaps reftable is capable of just handing the underlying code pointers\n>> into the mmap()'d file, so we could even skip all (re)allocations? Or if\n>> not, that certainly seems like a sensible thing to anticipate in a\n>> backend-independent interface. We could do that in the file backend if\n>> we were a bit smarter and used mmap() instead of the\n>> fopen()/read-in-a-loop pattern.\n>\n> It sounds like premature optimization. Reading reflogs is not usually\n> a performance sensitive operation. Also, if you hand out mmap'd\n> pointers, how would the reftable storage code know when it is safe to\n> close and munmap the file?\n\nThe same way we do the fopen/fclose lifetime now, i.e. you could rely on\nthem for the iteration, which seems to be a common pattern in code that\nneeds this.\n\nI was aiming more for the lack of premature optimization there,\ni.e. instead of byte-twiddling things by injecting \\0s we could just\nhave char */size_t offsets (as noted elsewhere), which would also nicely\nallow binary storage formats, if those formats happen to have an\nembedded string. Is that true of reftable?\n"},{"id":"442877","messageId":"CAFQ2z_MwL2=c1DZV3jvUau35ArseqxkvjTQo0DjYhJobE_zxtg@mail.gmail.com","threadId":"56958","inReplyTo":"211130.86r1ayedhn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-12-02T16:11:49Z","receivedAt":"2021-12-02T16:12:04Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Nov 30, 2021 at 12:41 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> I was aiming more for the lack of premature optimization there,\n> i.e. instead of byte-twiddling things by injecting \\0s we could just\n> have char */size_t offsets (as noted elsewhere), which would also nicely\n> allow binary storage formats, if those formats happen to have an\n> embedded string. Is that true of reftable?\n\nreftable zlib compresses the reflog, so you can't mmap strings from the storage.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442878","messageId":"CAFQ2z_NYgsiLM1Ffw3O9gAFHVuq68iAfrRbgodHdUPRo5Pm0UA@mail.gmail.com","threadId":"56958","inReplyTo":"xmqq1r2y7ork.fsf@gitster.g","subject":"Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-12-02T16:24:28Z","receivedAt":"2021-12-02T16:24:42Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Mon, Nov 29, 2021 at 8:20 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n> > On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >>\n> >> The API promises to have only LF, not CRLF, at the end, so\n> >> strbuf_trim_trailing_newline() is a bit overkill (and if payload\n> >> happened to end with CR, we would lose it).\n> >\n> > it would be best if there was a way to escape characters (ie. \"\\n\" =>\n> > \"\\\\n\"). Do we have a function for that?\n>\n> Mere escaping would not work in a backward compatible way, without a\n> trick.  It was envisioned that we probably could encode *and* signal\n\nI'm talking about the debug output, which isn't subject to\ncompatibility guarantees.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"442887","messageId":"pull.1145.v3.git.git.1638466593.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v2.git.git.1637855872.gitgitgadget@gmail.com","subject":"[PATCH v3 0/5] Inspect reflog data programmatically in more tests","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-02T17:36:28Z","receivedAt":"2021-12-02T17:36:38Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"This helps for reftable support, and will help if we want to reconsider\nunder which conditions reflogs get created/updated.\n\nv3: pointer arithmetic for trimming newline.\n\nHan-Wen Nienhuys (5):\n  show-branch: show reflog message\n  test-ref-store: don't add newline to reflog message\n  t1405: check for_each_reflog_ent_reverse() more thoroughly\n  test-ref-store: tweaks to for-each-reflog-ent format\n  refs/debug: trim trailing LF from reflog message\n\n builtin/show-branch.c          | 12 +++++++-----\n refs/debug.c                   |  7 +++++--\n t/helper/test-ref-store.c      |  6 +++---\n t/t1400-update-ref.sh          | 13 ++++++++-----\n t/t1405-main-ref-store.sh      |  4 ++--\n t/t1406-submodule-ref-store.sh |  4 ++--\n t/t3202-show-branch.sh         | 15 +++++++++++++++\n 7 files changed, 42 insertions(+), 19 deletions(-)\n\n\nbase-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1145%2Fhanwen%2Freflog-prelims-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1145/hanwen/reflog-prelims-v3\nPull-Request: https://github.com/git/git/pull/1145\n\nRange-diff vs v2:\n\n 1:  9d8394d8c76 = 1:  6e94a6fbe05 show-branch: show reflog message\n 2:  4a86d212589 ! 2:  062481ffece test-ref-store: don't add newline to reflog message\n     @@ Metadata\n       ## Commit message ##\n          test-ref-store: don't add newline to reflog message\n      \n     -    The files backend produces a newline for messages automatically, so before we\n     -    would print blank lines between entries.\n     +    By convention, reflog messages always end in '\\n', so\n     +    before we would print blank lines between entries.\n      \n          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n      \n 3:  0319503045b = 3:  46a8e13a5e6 t1405: check for_each_reflog_ent_reverse() more thoroughly\n 4:  62f5cb8a824 ! 4:  40ba92dbf0d test-ref-store: tweaks to for-each-reflog-ent format\n     @@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 11171502\n      -\ttest_when_finished \"rm -rf .git/$m .git/logs expect\" &&\n      -\ttest_cmp expect .git/logs/$m\n      +\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n     -+\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n     ++\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n      +\ttest_cmp actual expect\n       '\n       \n     @@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 11171503\n      -\ttest_when_finished \"rm -f .git/$m .git/logs/$m expect\" &&\n      -\ttest_cmp expect .git/logs/$m\n      +\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n     -+\ttest-tool ref-store main for-each-reflog-ent $m > actual &&\n     ++\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n      +\ttest_cmp actual expect\n       '\n       \n 5:  0288e743eb2 ! 5:  751f713a025 refs/debug: trim trailing LF from reflog message\n     @@ refs/debug.c: static int debug_print_reflog_ent(struct object_id *old_oid,\n       \tint ret;\n       \tchar o[GIT_MAX_HEXSZ + 1] = \"null\";\n       \tchar n[GIT_MAX_HEXSZ + 1] = \"null\";\n     -+\tstruct strbuf trimmed = STRBUF_INIT;\n     ++\tchar *msgend = strchrnul(msg, '\\n');\n       \tif (old_oid)\n       \t\toid_to_hex_r(o, old_oid);\n       \tif (new_oid)\n     - \t\toid_to_hex_r(n, new_oid);\n     +@@ refs/debug.c: static int debug_print_reflog_ent(struct object_id *old_oid,\n       \n     -+\tstrbuf_addstr(&trimmed, msg);\n       \tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n       \t\t      dbg->cb_data);\n      -\ttrace_printf_key(&trace_refs, \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n      -\t\tdbg->refname, ret, o, n, committer, (long int)timestamp, msg);\n     -+\tstrbuf_trim_trailing_newline(&trimmed);\n      +\ttrace_printf_key(&trace_refs,\n     -+\t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n     ++\t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%.*s\\\"\\n\",\n      +\t\t\t dbg->refname, ret, o, n, committer,\n     -+\t\t\t (long int)timestamp, trimmed.buf);\n     -+\tstrbuf_release(&trimmed);\n     ++\t\t\t (long int)timestamp, (int)(msgend - msg), msg);\n       \treturn ret;\n       }\n       \n\n-- \ngitgitgadget\n"},{"id":"442888","messageId":"6e94a6fbe05e38c849c1311b18b0a95030af21ba.1638466593.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v3.git.git.1638466593.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] show-branch: show reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-02T17:36:29Z","receivedAt":"2021-12-02T17:36:47Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nBefore, --reflog option would look for '\\t' in the reflog message. As refs.c\nalready parses the reflog line, the '\\t' was never found, and show-branch\n--reflog would always say \"(none)\" as reflog message\n\nAdd test.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n builtin/show-branch.c  | 12 +++++++-----\n t/t3202-show-branch.sh | 15 +++++++++++++++\n 2 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 082449293b5..f1e8318592c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -761,6 +761,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tchar *logmsg;\n \t\t\tchar *nth_desc;\n \t\t\tconst char *msg;\n+\t\t\tchar *end;\n \t\t\ttimestamp_t timestamp;\n \t\t\tint tz;\n \n@@ -770,11 +771,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\treflog = i;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tmsg = strchr(logmsg, '\\t');\n-\t\t\tif (!msg)\n-\t\t\t\tmsg = \"(none)\";\n-\t\t\telse\n-\t\t\t\tmsg++;\n+\n+\t\t\tend = strchr(logmsg, '\\n');\n+\t\t\tif (end)\n+\t\t\t\t*end = '\\0';\n+\n+\t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n \t\t\t\t\t\tshow_date(timestamp, tz,\n \t\t\t\t\t\t\t  DATE_MODE(RELATIVE)),\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ad9902a06b9..d4d64401e4b 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -4,6 +4,9 @@ test_description='test show-branch'\n \n . ./test-lib.sh\n \n+# arbitrary reference time: 2009-08-30 19:20:00\n+GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n+\n test_expect_success 'setup' '\n \ttest_commit initial &&\n \tfor i in $(test_seq 1 10)\n@@ -146,4 +149,16 @@ test_expect_success 'show branch --merge-base with N arguments' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show branch --reflog=2' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n+\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n+\t>\t--\n+\t>\t+  [refs/heads/branch10@{0}] branch10\n+\t>\t++ [refs/heads/branch10@{1}] initial\n+\tEOF\n+\tgit show-branch --reflog=2 >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"442889","messageId":"751f713a0257126f45d9dc8f86bf38cfa2f923e6.1638466594.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v3.git.git.1638466593.gitgitgadget@gmail.com","subject":"[PATCH v3 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-02T17:36:33Z","receivedAt":"2021-12-02T17:36:48Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nOn iteration, the reflog message is always terminated by a newline. Trim it to\navoid clobbering the console with is this extra newline.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/debug.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 8667c640237..2defaa9c52e 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -284,6 +284,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n \tint ret;\n \tchar o[GIT_MAX_HEXSZ + 1] = \"null\";\n \tchar n[GIT_MAX_HEXSZ + 1] = \"null\";\n+\tchar *msgend = strchrnul(msg, '\\n');\n \tif (old_oid)\n \t\toid_to_hex_r(o, old_oid);\n \tif (new_oid)\n@@ -291,8 +292,10 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n \n \tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n \t\t      dbg->cb_data);\n-\ttrace_printf_key(&trace_refs, \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%s\\\"\\n\",\n-\t\tdbg->refname, ret, o, n, committer, (long int)timestamp, msg);\n+\ttrace_printf_key(&trace_refs,\n+\t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%.*s\\\"\\n\",\n+\t\t\t dbg->refname, ret, o, n, committer,\n+\t\t\t (long int)timestamp, (int)(msgend - msg), msg);\n \treturn ret;\n }\n \n-- \ngitgitgadget\n"},{"id":"442890","messageId":"40ba92dbf0d0a772534d8c00f86b375a08d25a12.1638466593.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v3.git.git.1638466593.gitgitgadget@gmail.com","subject":"[PATCH v3 4/5] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-02T17:36:32Z","receivedAt":"2021-12-02T17:36:49Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nWe have some tests that read from files in .git/logs/ hierarchy\nwhen checking if correct reflog entries are created, but that is\ntoo specific to the files backend.  Other backends like reftable\nmay not store its reflog entries in such a \"one line per entry\"\nformat.\n\nUpdate for-each-reflog-ent test helper to produce output that\nis identical to lines in a reflog file files backend uses.\nThat way, (1) the current tests can be updated to use the test\nhelper to read the reflog entries instead of (parts of) reflog\nfiles, and perform the same inspection for correctness, and (2)\nwhen the ref backend is swapped to another backend, the updated\ntest can be used as-is to check the correctness.\n\nAdapt t1400 to use the for-each-reflog-ent test helper.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/helper/test-ref-store.c |  5 +++--\n t/t1400-update-ref.sh     | 13 ++++++++-----\n 2 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 5ac33dfb598..775e5da5b95 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -152,8 +152,9 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n \t\t       const char *committer, timestamp_t timestamp,\n \t\t       int tz, const char *msg, void *cb_data)\n {\n-\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n-\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n+\tprintf(\"%s %s %s %\" PRItime \" %+05d%s%s\", oid_to_hex(old_oid),\n+\t       oid_to_hex(new_oid), committer, timestamp, tz,\n+\t       *msg == '\\n' ? \"\" : \"\\t\", msg);\n \treturn 0;\n }\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 0d4f73acaa8..ef8fdcef466 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -321,8 +321,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000\n EOF\n test_expect_success \"verifying $m's log (logged by touch)\" '\n-\ttest_when_finished \"rm -rf .git/$m .git/logs expect\" &&\n-\ttest_cmp expect .git/logs/$m\n+\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n+\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n+\ttest_cmp actual expect\n '\n \n test_expect_success \"create $m (logged by config)\" '\n@@ -350,8 +351,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000\n EOF\n test_expect_success \"verifying $m's log (logged by config)\" '\n-\ttest_when_finished \"rm -f .git/$m .git/logs/$m expect\" &&\n-\ttest_cmp expect .git/logs/$m\n+\ttest_when_finished \"git update-ref -d $m && rm -rf .git/logs actual expect\" &&\n+\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n+\ttest_cmp actual expect\n '\n \n test_expect_success 'set up for querying the reflog' '\n@@ -467,7 +469,8 @@ $h_OTHER $h_FIXED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151040 +0000\tco\n $h_FIXED $h_MERGED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151100 +0000\tcommit (merge): Merged initial commit and a later commit.\n EOF\n test_expect_success 'git commit logged updates' '\n-\ttest_cmp expect .git/logs/$m\n+\ttest-tool ref-store main for-each-reflog-ent $m >actual &&\n+\ttest_cmp expect actual\n '\n unset h_TEST h_OTHER h_FIXED h_MERGED\n \n-- \ngitgitgadget\n\n"},{"id":"442891","messageId":"46a8e13a5e6d04b2265b3bd4f06e81408e63fe98.1638466593.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v3.git.git.1638466593.gitgitgadget@gmail.com","subject":"[PATCH v3 3/5] t1405: check for_each_reflog_ent_reverse() more thoroughly","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-02T17:36:31Z","receivedAt":"2021-12-02T17:36:51Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nIf we are checking for a certain ordering, we should check that there are two\nentries. Do this by mirroring the preceding test.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/t1405-main-ref-store.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex a600bedf2cd..76b15458409 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n+\thead -n1 actual | grep recreate-main &&\n \ttail -n1 actual | grep one\n '\n \n-- \ngitgitgadget\n\n"},{"id":"442892","messageId":"062481ffece8415260c84fb4fd3a68a566c15135.1638466593.git.gitgitgadget@gmail.com","threadId":"56958","inReplyTo":"pull.1145.v3.git.git.1638466593.gitgitgadget@gmail.com","subject":"[PATCH v3 2/5] test-ref-store: don't add newline to reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-02T17:36:30Z","receivedAt":"2021-12-02T17:36:56Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nBy convention, reflog messages always end in '\\n', so\nbefore we would print blank lines between entries.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n t/helper/test-ref-store.c      | 5 ++---\n t/t1405-main-ref-store.sh      | 5 ++---\n t/t1406-submodule-ref-store.sh | 4 ++--\n 3 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 3986665037a..5ac33dfb598 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -152,9 +152,8 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n \t\t       const char *committer, timestamp_t timestamp,\n \t\t       int tz, const char *msg, void *cb_data)\n {\n-\tprintf(\"%s %s %s %\"PRItime\" %d %s\\n\",\n-\t       oid_to_hex(old_oid), oid_to_hex(new_oid),\n-\t       committer, timestamp, tz, msg);\n+\tprintf(\"%s %s %s %\" PRItime \" %d %s\", oid_to_hex(old_oid),\n+\t       oid_to_hex(new_oid), committer, timestamp, tz, msg);\n \treturn 0;\n }\n \ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 49718b7ea7f..a600bedf2cd 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -89,13 +89,12 @@ test_expect_success 'for_each_reflog()' '\n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n \thead -n1 actual | grep one &&\n-\ttail -n2 actual | head -n1 | grep recreate-main\n+\ttail -n1 actual | grep recreate-main\n '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n-\thead -n1 actual | grep recreate-main &&\n-\ttail -n2 actual | head -n1 | grep one\n+\ttail -n1 actual | grep one\n '\n \n test_expect_success 'reflog_exists(HEAD)' '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 0a87058971e..b0365c1fee0 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -74,13 +74,13 @@ test_expect_success 'for_each_reflog()' '\n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n \thead -n1 actual | grep first &&\n-\ttail -n2 actual | head -n1 | grep main.to.new\n+\ttail -n1 actual | grep main.to.new\n '\n \n test_expect_success 'for_each_reflog_ent_reverse()' '\n \t$RUN for-each-reflog-ent-reverse HEAD >actual &&\n \thead -n1 actual | grep main.to.new &&\n-\ttail -n2 actual | head -n1 | grep first\n+\ttail -n1 actual | grep first\n '\n \n test_expect_success 'reflog_exists(HEAD)' '\n-- \ngitgitgadget\n\n"},{"id":"442898","messageId":"xmqq7dcmvoq6.fsf@gitster.g","threadId":"56958","inReplyTo":"CAFQ2z_NYgsiLM1Ffw3O9gAFHVuq68iAfrRbgodHdUPRo5Pm0UA@mail.gmail.com","subject":"Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-02T18:36:01Z","receivedAt":"2021-12-02T18:36:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> On Mon, Nov 29, 2021 at 8:20 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Han-Wen Nienhuys <hanwen@google.com> writes:\n>>\n>> > On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> >>\n>> >> The API promises to have only LF, not CRLF, at the end, so\n>> >> strbuf_trim_trailing_newline() is a bit overkill (and if payload\n>> >> happened to end with CR, we would lose it).\n>> >\n>> > it would be best if there was a way to escape characters (ie. \"\\n\" =>\n>> > \"\\\\n\"). Do we have a function for that?\n>>\n>> Mere escaping would not work in a backward compatible way, without a\n>> trick.  It was envisioned that we probably could encode *and* signal\n>\n> I'm talking about the debug output, which isn't subject to\n> compatibility guarantees.\n\nThere is no need to show \"\\n\" in the debug output in the first\nplace, no?  The message part stored in the reflog database is\nalready normalized to have each runs of whitespaces (including CR\nand LF) turned and squashed into a single SP, so the LF at the end\nis constant that the debug output can unconditionally strip without\nlosing information.\n\nOr are we talking about logging the _input_ to be recorded as the\nmessage for a new reflog entry?  Then I agree we can do whatever,\nand if you prefer C-style quoting, the helpers in quote.c are your\nfriends.\n\nThanks.\n\n\n"}]}