{"thread":{"id":"53955","subject":"[PATCH] refs: add \\t to reflog in the files backend","startedAt":"2020-07-31T11:36:18Z","lastAt":"2020-07-31T17:36:51Z","messageCount":3,"participants":["Han-Wen Nienhuys via GitGitGadget","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402524","messageId":"pull.688.git.1596195370757.gitgitgadget@gmail.com","threadId":"53955","inReplyTo":null,"subject":"[PATCH] refs: add \\t to reflog in the files backend","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-31T11:36:10Z","receivedAt":"2020-07-31T11:36:18Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\ncommit 523fa69c3 (Jul 10, 2020) \"reflog: cleanse messages in the\nrefs.c layer\" centralized reflog normalizaton. However, the\nnormalizaton added a leading \"\\t\" to the message. This is an artifact\nof the reflog storage format in the files backend, so it should be\nadded there.\n\nRoutines that parse back the reflog (such as grab_nth_branch_switch)\nexpect the \"\\t\" to not be in the message, so without this fix, git\nwith reftable cannot process the \"@{-1}\" syntax.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n refs: add \\t to reflog in the files backend\n \n commit 523fa69c3 (Jul 10, 2020) \"reflog: cleanse messages in the refs.c\n layer\" centralized reflog normalizaton. However, the normalizaton added\n a leading \"\\t\" to the message. This is an artifact of the reflog storage\n format in the files backend, so it should be added there.\n \n Routines that parse back the reflog (such as grab_nth_branch_switch)\n expect the \"\\t\" to not be in the message, so without this fix, git with\n reftable cannot process the \"@{-1}\" syntax.\n \n Signed-off-by: Han-Wen Nienhuys hanwen@google.com [hanwen@google.com]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-688%2Fhanwen%2Ftab-reflog-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-688/hanwen/tab-reflog-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/688\n\n refs.c | 1 -\n refs/files-backend.c | 4 +++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 89814c7be4..2dd851fe81 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -907,7 +907,6 @@ static void copy_reflog_msg(struct strbuf *sb, const char *msg)\n \tchar c;\n \tint wasspace = 1;\n \n-\tstrbuf_addch(sb, '\\t');\n \twhile ((c = *msg++)) {\n \t\tif (wasspace && isspace(c))\n \t\t\tcontinue;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex e0aba23eb2..985631f33e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1628,8 +1628,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n \tint ret = 0;\n \n \tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n-\tif (msg && *msg)\n+\tif (msg && *msg) {\n+\t\tstrbuf_addch(&sb, '\\t');\n \t\tstrbuf_addstr(&sb, msg);\n+\t}\n \tstrbuf_addch(&sb, '\\n');\n \tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\tret = -1;\n\nbase-commit: e8ab941b671da6890181aea5b5755d1d9eea24ec\n-- \ngitgitgadget\n"},{"id":"402561","messageId":"xmqqd04b4nak.fsf@gitster.c.googlers.com","threadId":"53955","inReplyTo":"pull.688.git.1596195370757.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: add \\t to reflog in the files backend","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-31T17:17:07Z","receivedAt":"2020-07-31T17:17:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" writes:\n\n> From: Han-Wen Nienhuys \n>\n> commit 523fa69c3 (Jul 10, 2020) \"reflog: cleanse messages in the\n> refs.c layer\" centralized reflog normalizaton. However, the\n> normalizaton added a leading \"\\t\" to the message. This is an artifact\n> of the reflog storage format in the files backend, so it should be\n> added there.\n\nI agree with you that it makes sense to move the logic to add \"\\t\"\nbefore the log message from generic layer to the backend. Thanks\nfor spotting and correcting.\n\nWill queue.\n"},{"id":"402563","messageId":"20200731173649.GA843002@coredump.intra.peff.net","threadId":"53955","inReplyTo":"pull.688.git.1596195370757.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: add \\t to reflog in the files backend","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T17:36:49Z","receivedAt":"2020-07-31T17:36:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 11:36:10AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> diff --git a/refs.c b/refs.c\n> index 89814c7be4..2dd851fe81 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -907,7 +907,6 @@ static void copy_reflog_msg(struct strbuf *sb, const char *msg)\n> \tchar c;\n> \tint wasspace = 1;\n> \n> -\tstrbuf_addch(sb, '\\t');\n> \twhile ((c = *msg++)) {\n> \t\tif (wasspace && isspace(c))\n> \t\t\tcontinue;\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index e0aba23eb2..985631f33e 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1628,8 +1628,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n> \tint ret = 0;\n> \n> \tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n> -\tif (msg && *msg)\n> +\tif (msg && *msg) {\n> +\t\tstrbuf_addch(&sb, '\\t');\n> \t\tstrbuf_addstr(&sb, msg);\n> +\t}\n\nI wondered here whether the existing code would always write the tab,\neven for an entry with no message, since we seem to unconditionally add\nthe tab in the hunk above.\n\nBut it looks like we only call copy_reflog_msg() if it's non-empty:\n\n static char *normalize_reflog_message(const char *msg)\n {\n struct strbuf sb = STRBUF_INIT;\n \n if (msg && *msg)\n copy_reflog_msg(&sb, msg);\n return strbuf_detach(&sb, NULL);\n }\n\nso this is retaining that behavior (and indeed, doing a simple \"git\nupdate-ref\" confirms that we currently omit the tab in this case).\n\nSo the patch looks good (and the intent seems obviously good to me, as\nwell).\n\n-Peff\n"}]}