{"thread":{"id":"56404","subject":"[PATCH 0/4] Gets rid of \"if reflog exists, append to it regardless of config settings\"","startedAt":"2021-08-30T14:48:53Z","lastAt":"2021-11-11T14:45:41Z","messageCount":40,"participants":["Han-Wen Nienhuys via GitGitGadget","Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"434085","messageId":"pull.1067.git.git.1630334929.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":null,"subject":"[PATCH 0/4] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-30T14:48:44Z","receivedAt":"2021-08-30T14:48:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"As discussed in\nCAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com\n\nHan-Wen Nienhuys (4):\n test-ref-store: tweaks to for-each-reflog-ent format\n t1400: use test-helper ref-store to inspect reflog contents\n refs: drop force_create argument of create_reflog API\n RFC: refs: reflog entries aren't written based on reflog existence.\n\n builtin/checkout.c | 2 +-\n refs.c | 9 ++++-----\n refs.h | 4 ++--\n refs/debug.c | 5 ++---\n refs/files-backend.c | 25 +++----------------------\n refs/packed-backend.c | 3 +--\n refs/refs-internal.h | 2 +-\n t/helper/test-ref-store.c | 9 ++++-----\n t/t1400-update-ref.sh | 21 ++++++++++++---------\n t/t1405-main-ref-store.sh | 7 ++++---\n t/t1406-submodule-ref-store.sh | 6 +++---\n 11 files changed, 37 insertions(+), 56 deletions(-)\n\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1067%2Fhanwen%2Freflog-touch-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1067/hanwen/reflog-touch-v1\nPull-Request: https://github.com/git/git/pull/1067\n-- \ngitgitgadget\n"},{"id":"434086","messageId":"d48207d6858502f04fd501a24ff7c2a80062dfbe.1630334929.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.git.git.1630334929.gitgitgadget@gmail.com","subject":"[PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-30T14:48:45Z","receivedAt":"2021-08-30T14:48:54Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \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 \n---\n t/helper/test-ref-store.c | 6 +++---\n t/t1405-main-ref-store.sh | 5 +++--\n t/t1406-submodule-ref-store.sh | 4 ++--\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex b314b81a45b..d7bbb20e614 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -151,9 +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\\n\",\n-\t oid_to_hex(old_oid), oid_to_hex(new_oid),\n-\t committer, timestamp, tz, msg);\n+\tconst char *newline = strchr(msg, '\\n') ? \"\" : \"\\n\";\n+\tprintf(\"%s %s %s %\" PRItime \" %+05d\\t%s%s\", oid_to_hex(old_oid),\n+\t oid_to_hex(new_oid), committer, timestamp, tz, msg, newline);\n \treturn 0;\n }\n \ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 49718b7ea7f..16a99382770 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -88,14 +88,15 @@ test_expect_success 'for_each_reflog()' '\n \n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n+\tcat 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":"434087","messageId":"dab8e115faf35529b086f102662ff86ca57a9748.1630334929.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.git.git.1630334929.gitgitgadget@gmail.com","subject":"[PATCH 2/4] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-30T14:48:46Z","receivedAt":"2021-08-30T14:48:57Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nSigned-off-by: Han-Wen Nienhuys \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 4506cd435b0..de0cf678f8e 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -315,14 +315,16 @@ test_expect_success 'symref empty directory removal' '\n \ttest_path_is_file .git/logs/HEAD\n '\n \n+TAB='\t'\n cat >expect < 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 < 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\n"},{"id":"434088","messageId":"048e2d17077f89049221e6e4a16b7e4143bc244c.1630334929.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.git.git.1630334929.gitgitgadget@gmail.com","subject":"[PATCH 3/4] refs: drop force_create argument of create_reflog API","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-30T14:48:47Z","receivedAt":"2021-08-30T14:49:01Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nThere is only one caller, builtin/checkout.c, and it hardcodes\nforce_create=1.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n builtin/checkout.c | 2 +-\n refs.c | 9 ++++-----\n refs.h | 4 ++--\n refs/debug.c | 5 ++---\n refs/files-backend.c | 5 ++---\n refs/packed-backend.c | 3 +--\n refs/refs-internal.h | 2 +-\n t/helper/test-ref-store.c | 3 +--\n t/t1405-main-ref-store.sh | 2 +-\n t/t1406-submodule-ref-store.sh | 2 +-\n 10 files changed, 16 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b5d477919a7..3c6506e0595 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -876,7 +876,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tint ret;\n \t\t\t\tstruct strbuf err = STRBUF_INIT;\n \n-\t\t\t\tret = safe_create_reflog(refname, 1, &err);\n+\t\t\t\tret = safe_create_reflog(refname, &err);\n \t\t\t\tif (ret) {\n \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n \t\t\t\t\t\topts->new_orphan_branch, err.buf);\ndiff --git a/refs.c b/refs.c\nindex 8b9f7c3a80a..ca59a7cc652 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2347,16 +2347,15 @@ int reflog_exists(const char *refname)\n }\n \n int refs_create_reflog(struct ref_store *refs, const char *refname,\n-\t\t int force_create, struct strbuf *err)\n+\t\t struct strbuf *err)\n {\n-\treturn refs->be->create_reflog(refs, refname, force_create, err);\n+\treturn refs->be->create_reflog(refs, refname, err);\n }\n \n-int safe_create_reflog(const char *refname, int force_create,\n-\t\t struct strbuf *err)\n+int safe_create_reflog(const char *refname, struct strbuf *err)\n {\n \treturn refs_create_reflog(get_main_ref_store(the_repository), refname,\n-\t\t\t\t force_create, err);\n+\t\t\t\t err);\n }\n \n int refs_delete_reflog(struct ref_store *refs, const char *refname)\ndiff --git a/refs.h b/refs.h\nindex 48970dfc7e0..3c457bc19c8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -416,8 +416,8 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n * Setup reflog before using. Fill in err and return -1 on failure.\n */\n int refs_create_reflog(struct ref_store *refs, const char *refname,\n-\t\t int force_create, struct strbuf *err);\n-int safe_create_reflog(const char *refname, int force_create, struct strbuf *err);\n+\t\t struct strbuf *err);\n+int safe_create_reflog(const char *refname, struct strbuf *err);\n \n /** Reads log for the value of ref during at_time. **/\n int read_ref_at(struct ref_store *refs,\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 1a7a9e11cfa..f6b01b1eba0 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -340,11 +340,10 @@ static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)\n }\n \n static int debug_create_reflog(struct ref_store *ref_store, const char *refname,\n-\t\t\t int force_create, struct strbuf *err)\n+\t\t\t struct strbuf *err)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->create_reflog(drefs->refs, refname,\n-\t\t\t\t\t\t force_create, err);\n+\tint res = drefs->refs->be->create_reflog(drefs->refs, refname, err);\n \ttrace_printf_key(&trace_refs, \"create_reflog: %s: %d\\n\", refname, res);\n \treturn res;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 677b7e4cdd2..e05ada9286d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1598,15 +1598,14 @@ error:\n \treturn -1;\n }\n \n-static int files_create_reflog(struct ref_store *ref_store,\n-\t\t\t const char *refname, int force_create,\n+static int files_create_reflog(struct ref_store *ref_store, const char *refname,\n \t\t\t struct strbuf *err)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"create_reflog\");\n \tint fd;\n \n-\tif (log_ref_setup(refs, refname, force_create, &fd, err))\n+\tif (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))\n \t\treturn -1;\n \n \tif (fd >= 0)\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex f8aa97d7998..af7038de42d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1618,8 +1618,7 @@ static int packed_reflog_exists(struct ref_store *ref_store,\n }\n \n static int packed_create_reflog(struct ref_store *ref_store,\n-\t\t\t const char *refname, int force_create,\n-\t\t\t struct strbuf *err)\n+\t\t\t\tconst char *refname, struct strbuf *err)\n {\n \tBUG(\"packed reference store does not support reflogs\");\n }\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 3155708345f..cc0e56e8c82 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -590,7 +590,7 @@ typedef int for_each_reflog_ent_reverse_fn(struct ref_store *ref_store,\n \t\t\t\t\t void *cb_data);\n typedef int reflog_exists_fn(struct ref_store *ref_store, const char *refname);\n typedef int create_reflog_fn(struct ref_store *ref_store, const char *refname,\n-\t\t\t int force_create, struct strbuf *err);\n+\t\t\t struct strbuf *err);\n typedef int delete_reflog_fn(struct ref_store *ref_store, const char *refname);\n typedef int reflog_expire_fn(struct ref_store *ref_store,\n \t\t\t const char *refname, const struct object_id *oid,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex d7bbb20e614..58d5438a290 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -181,11 +181,10 @@ static int cmd_reflog_exists(struct ref_store *refs, const char **argv)\n static int cmd_create_reflog(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n-\tint force_create = arg_flags(*argv++, \"force-create\");\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n-\tret = refs_create_reflog(refs, refname, force_create, &err);\n+\tret = refs_create_reflog(refs, refname, &err);\n \tif (err.len)\n \t\tputs(err.buf);\n \treturn ret;\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 16a99382770..4ee95fceccd 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -109,7 +109,7 @@ test_expect_success 'delete_reflog(HEAD)' '\n '\n \n test_expect_success 'create-reflog(HEAD)' '\n-\t$RUN create-reflog HEAD 1 &&\n+\t$RUN create-reflog HEAD &&\n \tgit reflog exists HEAD\n '\n \ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex b0365c1fee0..78d40bdcd8b 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -92,7 +92,7 @@ test_expect_success 'delete_reflog() not allowed' '\n '\n \n test_expect_success 'create-reflog() not allowed' '\n-\ttest_must_fail $RUN create-reflog HEAD 1\n+\ttest_must_fail $RUN create-reflog HEAD\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"434089","messageId":"2f11fd7718005d1e94b3139f086134896da202f1.1630334929.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.git.git.1630334929.gitgitgadget@gmail.com","subject":"[PATCH 4/4] RFC: refs: reflog entries aren't written based on reflog existence.","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-30T14:48:48Z","receivedAt":"2021-08-30T14:49:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nBefore, if we aren't supposed to update reflogs (eg.\ncore.logallrefupdates=NONE), we would still write reflog entries if the\nreflog file (.git/logs/REFNAME) existed.\n\nThe reftable storage backend cannot distinguish between a non-existing\nreflog, and an empty one. Therefore it cannot mimick this functionality.\n\nIn CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,\nwe came to the conclusion that this feature is probably a remnant from\nthe time that reflogs weren't enabled by default, and it does not need\nto be kept.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n refs/files-backend.c | 20 +-------------------\n t/t1400-update-ref.sh | 7 +++----\n 2 files changed, 4 insertions(+), 23 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex e05ada9286d..25f5a4ce777 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1551,6 +1551,7 @@ static int log_ref_setup(struct files_ref_store *refs,\n \tstruct strbuf logfile_sb = STRBUF_INIT;\n \tchar *logfile;\n \n+\t*logfd = -1;\n \tfiles_reflog_path(refs, &logfile_sb, refname);\n \tlogfile = strbuf_detach(&logfile_sb, NULL);\n \n@@ -1565,26 +1566,8 @@ static int log_ref_setup(struct files_ref_store *refs,\n \t\t\telse\n \t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n \t\t\t\t\t logfile, strerror(errno));\n-\n \t\t\tgoto error;\n \t\t}\n-\t} else {\n-\t\t*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);\n-\t\tif (*logfd < 0) {\n-\t\t\tif (errno == ENOENT || errno == EISDIR) {\n-\t\t\t\t/*\n-\t\t\t\t * The logfile doesn't already exist,\n-\t\t\t\t * but that is not an error; it only\n-\t\t\t\t * means that we won't write log\n-\t\t\t\t * entries to it.\n-\t\t\t\t */\n-\t\t\t\t;\n-\t\t\t} else {\n-\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t\t logfile, strerror(errno));\n-\t\t\t\tgoto error;\n-\t\t\t}\n-\t\t}\n \t}\n \n \tif (*logfd >= 0)\n@@ -1592,7 +1575,6 @@ static int log_ref_setup(struct files_ref_store *refs,\n \n \tfree(logfile);\n \treturn 0;\n-\n error:\n \tfree(logfile);\n \treturn -1;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex de0cf678f8e..52433f6d0d2 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -269,7 +269,7 @@ test_expect_success \"(not) changed .git/$m\" '\n '\n \n rm -f .git/logs/refs/heads/main\n-test_expect_success \"create $m (logged by touch)\" '\n+test_expect_success \"create $m\" '\n \ttest_config core.logAllRefUpdates false &&\n \tGIT_COMMITTER_DATE=\"2005-05-26 23:30\" \\\n \tgit update-ref --create-reflog HEAD $A -m \"Initial Creation\" &&\n@@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' '\n \ttest_path_is_file .git/logs/HEAD\n '\n \n-TAB='\t'\n cat >expect < 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$TAB\n EOF\n+\n test_expect_success \"verifying $m's log (logged by touch)\" '\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@@ -346,6 +344,7 @@ test_expect_success \"set $m (logged by config)\" '\n \ttest $A = $(git show-ref -s --verify $m)\n '\n \n+TAB='\t'\n cat >expect < 1117150320 +0000\tInitial Creation\n $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n-- \ngitgitgadget\n"},{"id":"434151","messageId":"YS04HFTC3SczuWS1@nand.local","threadId":"56404","inReplyTo":"d48207d6858502f04fd501a24ff7c2a80062dfbe.1630334929.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T19:57:16Z","receivedAt":"2021-08-30T19:57:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:\n> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\n> index 49718b7ea7f..16a99382770 100755\n> --- a/t/t1405-main-ref-store.sh\n> +++ b/t/t1405-main-ref-store.sh\n> @@ -88,14 +88,15 @@ test_expect_success 'for_each_reflog()' '\n>\n> test_expect_success 'for_each_reflog_ent()' '\n> \t$RUN for-each-reflog-ent HEAD >actual &&\n> +\tcat actual &&\n\nNit; stray debugging.\n\nThanks,\nTaylor\n"},{"id":"434157","messageId":"YS0+MHk3svGr7d4n@nand.local","threadId":"56404","inReplyTo":"d48207d6858502f04fd501a24ff7c2a80062dfbe.1630334929.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T20:23:12Z","receivedAt":"2021-08-30T20:23:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:\n> From: Han-Wen Nienhuys \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>\n> Signed-off-by: Han-Wen Nienhuys \n> ---\n> t/helper/test-ref-store.c | 6 +++---\n> t/t1405-main-ref-store.sh | 5 +++--\n> t/t1406-submodule-ref-store.sh | 4 ++--\n> 3 files changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\n> index b314b81a45b..d7bbb20e614 100644\n> --- a/t/helper/test-ref-store.c\n> +++ b/t/helper/test-ref-store.c\n> @@ -151,9 +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\\n\",\n> -\t oid_to_hex(old_oid), oid_to_hex(new_oid),\n> -\t committer, timestamp, tz, msg);\n> +\tconst char *newline = strchr(msg, '\\n') ? \"\" : \"\\n\";\n> +\tprintf(\"%s %s %s %\" PRItime \" %+05d\\t%s%s\", oid_to_hex(old_oid),\n> +\t oid_to_hex(new_oid), committer, timestamp, tz, msg, newline);\n\nHaving read the rest of the series, I did scratch my head quite a bit\nhere, but I think the change is actually quite simple. In the files\nbackend, show_one_reflog_ent is parsing line-wise, and each line ends\nwith the LF.\n\nOf course, we don't expect the reflog to have messages that actually\ncontain a newline because we cleanse them with copy_reflog_message()\nbefore writing.\n\nSo really it seems like the files-backend should be calling rstrip on\nthe message before handing it to the callback. Either that, or we could\ncall rstrip ourselves (since the generic strchr() makes me think that\nthe LF could appear anywhere in the string, at least on first read).\n\nThanks,\nTaylor\n"},{"id":"434165","messageId":"xmqqpmtu3bbn.fsf@gitster.g","threadId":"56404","inReplyTo":"d48207d6858502f04fd501a24ff7c2a80062dfbe.1630334929.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T20:51:56Z","receivedAt":"2021-08-30T20:51:59Z","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> 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\nI find the above written in clear language that conveys what it\nwants to say unclearly X-<.\n\nI am guessing, from the patch text, that the callback function to\nrefs_for_each_reflog_ent() and its _reverse() variant takes a \"msg\"\nparameter, which is expected to include a terminating LF at its end,\nbut some backends omits the LF, and this change is to work around it\nby conditionally adding (or refraining from adding) LF in the\neach_reflog() function that is used for testing.\n\nIs that what is going on? Or is it \"even within a single backend,\nsometimes we get message with terminating LF and sometimes without\"?\n\nIn any case, it changes the output. We used to spend two output\nlines per record in tests for these two functions, but now we assume\nwe get one each line per one reflog entry. Which may be OK as far\nas these tests are concerned, but does this leave problems with the\nreal world callbacks? Do the real callers to for_each_reflog_ent()\nand for_each_reflog_ent_reverse() also now need to cater to this \"we\nsometimes get LF at the end, we sometimes don't\" differences by\nhaving a similar logic as we see in the updated each_reflog() in\nthis patch?\n\nShouldn't we be instead making sure that the callback functions get\nthe msg argument in a consistent way, whether it is with or without\nterminating LF, instead of forcing the callers to cope with the\ndifferences like this each_reflog() function does in this patch?\n\nThanks.\n"},{"id":"434166","messageId":"xmqqlf4i3b5v.fsf@gitster.g","threadId":"56404","inReplyTo":"dab8e115faf35529b086f102662ff86ca57a9748.1630334929.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T20:55:24Z","receivedAt":"2021-08-30T20:55:32Z","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> +TAB='\t'\n> cat >expect < $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\nNice. So the file backend may omit the trailing tab for an empty\nmessage at the storage level, but the test-tool consistently gives\nthe same format for entries with empty and non-empty messages, and\nthat is shown in the above. Makes sense.\n\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 < $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\nDitto.\n\nLooks good to me.\n\n"},{"id":"434167","messageId":"YS1GXNoPgjIjO3B7@nand.local","threadId":"56404","inReplyTo":"xmqqpmtu3bbn.fsf@gitster.g","subject":"Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T20:58:04Z","receivedAt":"2021-08-30T20:58:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 30, 2021 at 01:51:56PM -0700, Junio C Hamano wrote:\n> \"Han-Wen Nienhuys via GitGitGadget\" writes:\n>\n> > From: Han-Wen Nienhuys \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>\n> I find the above written in clear language that conveys what it\n> wants to say unclearly X-<.\n\nI agree; there is unfortunately more than meets the eye for this patch,\nso untangling it all took me a little while.\n\n> [...]\n> Is that what is going on? Or is it \"even within a single backend,\n> sometimes we get message with terminating LF and sometimes without\"?\n\nIs exactly what I was wondering about in [1]. And...\n\n> Shouldn't we be instead making sure that the callback functions get\n> the msg argument in a consistent way, whether it is with or without\n> terminating LF, instead of forcing the callers to cope with the\n> differences like this each_reflog() function does in this patch?\n\n...that is a much more sensible resolution than what I saw in this\npatch. (At the very least, strchr is confusing since we don't expect LF\ncharacters anywhere in the reflog message, and this is really dealing\nwith how the callback is invoked, not the actual bytes-on-disk).\n\nI would probably prefer that the callback be standardized to either\nalways or never give a trailing LF.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/YS0+MHk3svGr7d4n@nand.local/\n"},{"id":"434170","messageId":"xmqqh7f63at2.fsf@gitster.g","threadId":"56404","inReplyTo":"048e2d17077f89049221e6e4a16b7e4143bc244c.1630334929.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] refs: drop force_create argument of create_reflog API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T21:03:05Z","receivedAt":"2021-08-30T21:03:12Z","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> There is only one caller, builtin/checkout.c, and it hardcodes\n> force_create=1.\n\nThis dates back to abd0cd3a (refs: new public ref function:\nsafe_create_reflog, 2015-07-21) and it seems that we never acquired\nthe second caller to this helper.\n\nMakes sense.\n"},{"id":"434172","messageId":"YS1JTuBY0Lq34yP/@nand.local","threadId":"56404","inReplyTo":"2f11fd7718005d1e94b3139f086134896da202f1.1630334929.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] RFC: refs: reflog entries aren't written based on reflog existence.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T21:10:38Z","receivedAt":"2021-08-30T21:10:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 30, 2021 at 02:48:48PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:\n> From: Han-Wen Nienhuys \n>\n> Before, if we aren't supposed to update reflogs (eg.\n> core.logallrefupdates=NONE), we would still write reflog entries if the\n> reflog file (.git/logs/REFNAME) existed.\n>\n> The reftable storage backend cannot distinguish between a non-existing\n> reflog, and an empty one. Therefore it cannot mimick this functionality.\n>\n> In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,\n\nThis looks like a faithful implementation of that discussion, so I don't\nsee any problems with the change.\n\n> we came to the conclusion that this feature is probably a remnant from\n> the time that reflogs weren't enabled by default, and it does not need\n> to be kept.\n>\n> Signed-off-by: Han-Wen Nienhuys \n> ---\n> refs/files-backend.c | 20 +-------------------\n> t/t1400-update-ref.sh | 7 +++----\n> 2 files changed, 4 insertions(+), 23 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index e05ada9286d..25f5a4ce777 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1551,6 +1551,7 @@ static int log_ref_setup(struct files_ref_store *refs,\n> \tstruct strbuf logfile_sb = STRBUF_INIT;\n> \tchar *logfile;\n>\n> +\t*logfd = -1;\n> \tfiles_reflog_path(refs, &logfile_sb, refname);\n> \tlogfile = strbuf_detach(&logfile_sb, NULL);\n>\n> @@ -1565,26 +1566,8 @@ static int log_ref_setup(struct files_ref_store *refs,\n> \t\t\telse\n> \t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> \t\t\t\t\t logfile, strerror(errno));\n> -\n> \t\t\tgoto error;\n> \t\t}\n> -\t} else {\n> -\t\t*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);\n> -\t\tif (*logfd < 0) {\n> -\t\t\tif (errno == ENOENT || errno == EISDIR) {\n> -\t\t\t\t/*\n> -\t\t\t\t * The logfile doesn't already exist,\n> -\t\t\t\t * but that is not an error; it only\n> -\t\t\t\t * means that we won't write log\n> -\t\t\t\t * entries to it.\n> -\t\t\t\t */\n> -\t\t\t\t;\n> -\t\t\t} else {\n> -\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> -\t\t\t\t\t logfile, strerror(errno));\n> -\t\t\t\tgoto error;\n> -\t\t\t}\n> -\t\t}\n> \t}\n>\n> \tif (*logfd >= 0)\n\nI'm nit-picking, but I think that this conditional could probably be\nmoved into the if-statement above, since we no longer set logfd anywhere\nelse. It might read more clearly, and if you're going to resubmit this\nseries anyway, it would be helpful to pick that up.\n> @@ -1592,7 +1575,6 @@ static int log_ref_setup(struct files_ref_store *refs,\n>\n> \tfree(logfile);\n> \treturn 0;\n> -\n> error:\n> \tfree(logfile);\n> \treturn -1;\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index de0cf678f8e..52433f6d0d2 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -269,7 +269,7 @@ test_expect_success \"(not) changed .git/$m\" '\n> '\n>\n> rm -f .git/logs/refs/heads/main\n> -test_expect_success \"create $m (logged by touch)\" '\n> +test_expect_success \"create $m\" '\n> \ttest_config core.logAllRefUpdates false &&\n> \tGIT_COMMITTER_DATE=\"2005-05-26 23:30\" \\\n> \tgit update-ref --create-reflog HEAD $A -m \"Initial Creation\" &&\n> @@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' '\n> \ttest_path_is_file .git/logs/HEAD\n> '\n>\n> -TAB='\t'\n> cat >expect < $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tInitial Creation\n\nOK, so \"Initial Creation\" happened in a state where\n\"core.logAllRefUpdates=false\", but we passed \"--create-reflog\" to force\nthe behavior anyway.\n\nThen during \"Switch\", we had a reflog, but again \"core.logAllRefUpdates\"\nis false, and we didn't pass \"--create-reflog\", so no more updates are\nwritten to the reflog.\n\nThat's exactly the test that I would have expected to exist for this\nchange, and so this LGTM.\n\nThanks,\nTaylor\n"},{"id":"434173","messageId":"xmqqbl5e3a9h.fsf@gitster.g","threadId":"56404","inReplyTo":"2f11fd7718005d1e94b3139f086134896da202f1.1630334929.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] RFC: refs: reflog entries aren't written based on reflog existence.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T21:14:50Z","receivedAt":"2021-08-30T21:14:53Z","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> Before, if we aren't supposed to update reflogs (eg.\n> core.logallrefupdates=NONE), we would still write reflog entries if the\n> reflog file (.git/logs/REFNAME) existed.\n>\n> The reftable storage backend cannot distinguish between a non-existing\n> reflog, and an empty one. Therefore it cannot mimick this functionality.\n>\n> In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,\n> we came to the conclusion that this feature is probably a remnant from\n> the time that reflogs weren't enabled by default, and it does not need\n> to be kept.\n>\n> Signed-off-by: Han-Wen Nienhuys \n> ---\n> refs/files-backend.c | 20 +-------------------\n> t/t1400-update-ref.sh | 7 +++----\n> 2 files changed, 4 insertions(+), 23 deletions(-)\n\nWhile I like the loss of lines and reduced complexity...\n\nOne potential harm this change will bring to us is what happens to\npeople who disable core.logAllRefUpdates manually after using the\nrepository for a while. Their @{4} will point at the same commit no\nmatter how many operations are done on the current branch after they\ndo so. I wouldn't mind if \"git reflog disable\" command is given to\nthe users prominently and core.logAllRefUpdates becomes a mere\nimplementation detail nobody has to care about---in such a world, we\ncould set the configuration and drop the existing reflog records at\nthe same time and nobody will be hurt.\n\nIf we keep the current behaviour, what are we harming instead?\nGrowth of diskspace usage is an obvious one, but disks are cheaper\ncompared to human brainwave cycles cost.\n\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index de0cf678f8e..52433f6d0d2 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -269,7 +269,7 @@ test_expect_success \"(not) changed .git/$m\" '\n> '\n> \n> rm -f .git/logs/refs/heads/main\n> -test_expect_success \"create $m (logged by touch)\" '\n> +test_expect_success \"create $m\" '\n> \ttest_config core.logAllRefUpdates false &&\n> \tGIT_COMMITTER_DATE=\"2005-05-26 23:30\" \\\n> \tgit update-ref --create-reflog HEAD $A -m \"Initial Creation\" &&\n> @@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' '\n> \ttest_path_is_file .git/logs/HEAD\n> '\n> \n> -TAB='\t'\n> cat >expect < $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$TAB\n> EOF\n> +\n> test_expect_success \"verifying $m's log (logged by touch)\" '\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> @@ -346,6 +344,7 @@ test_expect_success \"set $m (logged by config)\" '\n> \ttest $A = $(git show-ref -s --verify $m)\n> '\n> \n> +TAB='\t'\n> cat >expect < $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\nMoving the definition of TAB= around in the test is not all that\nwelcome. If anything, I'd suggest doing so nearer to the beginning\nof the test sequence, before the first test that uses the ref-store\ntest-tool, because it is just like $GIT_COMMITTER_EMAIL and $RUN\nthat define constants used throughout the remainder of the test\nscript.\n\nTHanks.\n"},{"id":"434198","messageId":"xmqqv93m1tnf.fsf@gitster.g","threadId":"56404","inReplyTo":"YS1GXNoPgjIjO3B7@nand.local","subject":"Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T21:59:00Z","receivedAt":"2021-08-30T21:59:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> Shouldn't we be instead making sure that the callback functions get\n>> the msg argument in a consistent way, whether it is with or without\n>> terminating LF, instead of forcing the callers to cope with the\n>> differences like this each_reflog() function does in this patch?\n>\n> ...that is a much more sensible resolution than what I saw in this\n> patch. (At the very least, strchr is confusing since we don't expect LF\n> characters anywhere in the reflog message, and this is really dealing\n> with how the callback is invoked, not the actual bytes-on-disk).\n>\n> I would probably prefer that the callback be standardized to either\n> always or never give a trailing LF.\n\nTrue, and I agree that the use of LF by on-disk reflog storage\nformat is just as the record terminator, so I do not mind if we\ndecide to standardize on a single incomplete line without LF.\n\nThanks.\n\n"},{"id":"434791","messageId":"pull.1067.v2.git.git.1630947142.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.git.git.1630334929.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-06T16:52:17Z","receivedAt":"2021-09-06T16:52:27Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"As discussed in\nCAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com\n\nv2 addresses all outstanding comments.\n\nThere is one open question: three is a line marked \"XXX\" in commit 995d450d\n(\"refs: trim newline from reflog message\"). I'd think it would print a\ndouble newline before (which seems wrong), but I'm unsure how to get the\ncodepath to run.\n\nHan-Wen Nienhuys (5):\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 refs: drop force_create argument of create_reflog API\n RFC: refs: reflog entries aren't written based on reflog existence.\n\n builtin/checkout.c | 2 +-\n builtin/show-branch.c | 4 +-\n reflog-walk.c | 6 +--\n refs.c | 9 ++--\n refs.h | 4 +-\n refs/debug.c | 5 +-\n refs/files-backend.c | 88 +++++++++++++---------------------\n refs/packed-backend.c | 3 +-\n refs/refs-internal.h | 2 +-\n t/helper/test-ref-store.c | 8 ++--\n t/t1400-update-ref.sh | 21 ++++----\n t/t1405-main-ref-store.sh | 6 +--\n t/t1406-submodule-ref-store.sh | 6 +--\n 13 files changed, 71 insertions(+), 93 deletions(-)\n\n\nbase-commit: e0a2f5cbc585657e757385ad918f167f519cfb96\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1067%2Fhanwen%2Freflog-touch-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1067/hanwen/reflog-touch-v2\nPull-Request: https://github.com/git/git/pull/1067\n\nRange-diff vs v1:\n\n 1: d48207d6858 ! 1: 995d450da42 test-ref-store: tweaks to for-each-reflog-ent format\n @@ Metadata\n Author: Han-Wen Nienhuys \n \n ## Commit message ##\n - test-ref-store: tweaks to for-each-reflog-ent format\n + refs: trim newline from reflog message\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 + 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 Signed-off-by: Han-Wen Nienhuys \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 -+\tconst char *newline = strchr(msg, '\\n') ? \"\" : \"\\n\";\n -+\tprintf(\"%s %s %s %\" PRItime \" %+05d\\t%s%s\", oid_to_hex(old_oid),\n -+\t oid_to_hex(new_oid), committer, timestamp, tz, msg, newline);\n - \treturn 0;\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\t\tshow_one_commit(rev[i], 1);\n + \t\t\t}\n + \t\t\telse\n +-\t\t\t\tputs(reflog_msg[i]);\n ++\t\t\t\tputs(reflog_msg[i]); /* XXX - this puts a\n ++\t\t\t\t\t\t\tnewline. Did we put two\n ++\t\t\t\t\t\t\tnewlines beforehand? */\n + \n + \t\t\tif (is_head)\n + \t\t\t\thead_at = i;\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 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 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 + \treturn 0;\n + }\n \n ## t/t1405-main-ref-store.sh ##\n @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog()' '\n - \n test_expect_success 'for_each_reflog_ent()' '\n \t$RUN for-each-reflog-ent HEAD >actual &&\n -+\tcat actual &&\n \thead -n1 actual | grep one &&\n -\ttail -n2 actual | head -n1 | grep recreate-main\n +\ttail -n1 actual | grep recreate-main\n @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog()' '\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 +-\thead -n1 actual | grep recreate-main &&\n -\ttail -n2 actual | head -n1 | grep one\n +\ttail -n1 actual | grep one\n '\n -: ----------- > 2: 11b296a55e9 test-ref-store: tweaks to for-each-reflog-ent format\n 2: dab8e115faf ! 3: 9ec09cc64cd t1400: use test-helper ref-store to inspect reflog contents\n @@ Commit message\n Signed-off-by: Han-Wen Nienhuys \n \n ## t/t1400-update-ref.sh ##\n -@@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '\n - \ttest_path_is_file .git/logs/HEAD\n - '\n +@@ t/t1400-update-ref.sh: 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 +@@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '\n cat >expect < 1117150200 +0000\tInitial Creation\n $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n 3: 048e2d17077 = 4: aa25fd9b7de refs: drop force_create argument of create_reflog API\n 4: 2f11fd77180 ! 5: f6a7c5ad56e RFC: refs: reflog entries aren't written based on reflog existence.\n @@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,\n \tchar *logfile;\n \n +\t*logfd = -1;\n ++\tif (!force_create && !should_autocreate_reflog(refname))\n ++\t\treturn 0;\n ++\n \tfiles_reflog_path(refs, &logfile_sb, refname);\n \tlogfile = strbuf_detach(&logfile_sb, NULL);\n \n -@@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,\n - \t\t\telse\n - \t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n - \t\t\t\t\t logfile, strerror(errno));\n +-\tif (force_create || should_autocreate_reflog(refname)) {\n +-\t\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n +-\t\t\tif (errno == ENOENT)\n +-\t\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n +-\t\t\t\t\t \"%s\", logfile, strerror(errno));\n +-\t\t\telse if (errno == EISDIR)\n +-\t\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n +-\t\t\t\t\t logfile);\n +-\t\t\telse\n +-\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n +-\t\t\t\t\t logfile, strerror(errno));\n -\n - \t\t\tgoto error;\n - \t\t}\n +-\t\t\tgoto error;\n +-\t\t}\n -\t} else {\n -\t\t*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);\n -\t\tif (*logfd < 0) {\n @@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,\n -\t\t\t\tgoto error;\n -\t\t\t}\n -\t\t}\n ++\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n ++\t\tif (errno == ENOENT)\n ++\t\t\tstrbuf_addf(err,\n ++\t\t\t\t \"unable to create directory for '%s': \"\n ++\t\t\t\t \"%s\",\n ++\t\t\t\t logfile, strerror(errno));\n ++\t\telse if (errno == EISDIR)\n ++\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n ++\t\t\t\t logfile);\n ++\t\telse\n ++\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n ++\t\t\t\t logfile, strerror(errno));\n \t}\n \n \tif (*logfd >= 0)\n -@@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,\n + \t\tadjust_shared_perm(logfile);\n \n \tfree(logfile);\n - \treturn 0;\n +-\treturn 0;\n -\n - error:\n - \tfree(logfile);\n - \treturn -1;\n +-error:\n +-\tfree(logfile);\n +-\treturn -1;\n ++\treturn (*logfd < 0) ? -1 : 0;\n + }\n + \n + static int files_create_reflog(struct ref_store *ref_store, const char *refname,\n \n ## t/t1400-update-ref.sh ##\n @@ t/t1400-update-ref.sh: test_expect_success \"(not) changed .git/$m\" '\n @@ t/t1400-update-ref.sh: test_expect_success \"(not) changed .git/$m\" '\n \tGIT_COMMITTER_DATE=\"2005-05-26 23:30\" \\\n \tgit update-ref --create-reflog HEAD $A -m \"Initial Creation\" &&\n @@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '\n - \ttest_path_is_file .git/logs/HEAD\n - '\n \n --TAB='\t'\n cat >expect < 1117150200 +0000\tInitial Creation\n -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n @@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '\n test_expect_success \"verifying $m's log (logged by touch)\" '\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 -@@ t/t1400-update-ref.sh: test_expect_success \"set $m (logged by config)\" '\n - \ttest $A = $(git show-ref -s --verify $m)\n - '\n - \n -+TAB='\t'\n - cat >expect < 1117150320 +0000\tInitial Creation\n - $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000\tSwitch\n\n-- \ngitgitgadget\n"},{"id":"434792","messageId":"995d450da42f35b4cecdd699fa1b1a74518f481e.1630947142.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v2.git.git.1630947142.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-06T16:52:18Z","receivedAt":"2021-09-06T16:52:28Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \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 \n---\n builtin/show-branch.c | 4 +++-\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, 24 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d77ce7aeb38..acf710313a0 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -888,7 +888,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\tshow_one_commit(rev[i], 1);\n \t\t\t}\n \t\t\telse\n-\t\t\t\tputs(reflog_msg[i]);\n+\t\t\t\tputs(reflog_msg[i]); /* XXX - this puts a\n+\t\t\t\t\t\t\tnewline. Did we put two\n+\t\t\t\t\t\t\tnewlines beforehand? */\n \n \t\t\tif (is_head)\n \t\t\t\thead_at = i;\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex e9cd3283694..944cce929c8 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -245,8 +245,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@@ -285,10 +283,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 677b7e4cdd2..206c9f8b932 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1897,17 +1897,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 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 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@@ -1999,6 +1997,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@@ -2011,6 +2010,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@@ -2060,7 +2060,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@@ -3020,18 +3020,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":"434793","messageId":"11b296a55e9e450575c64ade1a2cb93a1886b9da.1630947142.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v2.git.git.1630947142.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-06T16:52:19Z","receivedAt":"2021-09-06T16:52:30Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \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 \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":"434794","messageId":"9ec09cc64cd6f227e96fb9752c23ce3e54e5dd2f.1630947142.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v2.git.git.1630947142.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-06T16:52:20Z","receivedAt":"2021-09-06T16:52:32Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nSigned-off-by: Han-Wen Nienhuys \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 4506cd435b0..8ced98e0fe8 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 < 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 < 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\n"},{"id":"434795","messageId":"aa25fd9b7de02c3b5c620def8fae4d5b6ce456c5.1630947142.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v2.git.git.1630947142.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] refs: drop force_create argument of create_reflog API","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-06T16:52:21Z","receivedAt":"2021-09-06T16:52:33Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nThere is only one caller, builtin/checkout.c, and it hardcodes\nforce_create=1.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n builtin/checkout.c | 2 +-\n refs.c | 9 ++++-----\n refs.h | 4 ++--\n refs/debug.c | 5 ++---\n refs/files-backend.c | 5 ++---\n refs/packed-backend.c | 3 +--\n refs/refs-internal.h | 2 +-\n t/helper/test-ref-store.c | 3 +--\n t/t1405-main-ref-store.sh | 2 +-\n t/t1406-submodule-ref-store.sh | 2 +-\n 10 files changed, 16 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b5d477919a7..3c6506e0595 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -876,7 +876,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tint ret;\n \t\t\t\tstruct strbuf err = STRBUF_INIT;\n \n-\t\t\t\tret = safe_create_reflog(refname, 1, &err);\n+\t\t\t\tret = safe_create_reflog(refname, &err);\n \t\t\t\tif (ret) {\n \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n \t\t\t\t\t\topts->new_orphan_branch, err.buf);\ndiff --git a/refs.c b/refs.c\nindex 8b9f7c3a80a..ca59a7cc652 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2347,16 +2347,15 @@ int reflog_exists(const char *refname)\n }\n \n int refs_create_reflog(struct ref_store *refs, const char *refname,\n-\t\t int force_create, struct strbuf *err)\n+\t\t struct strbuf *err)\n {\n-\treturn refs->be->create_reflog(refs, refname, force_create, err);\n+\treturn refs->be->create_reflog(refs, refname, err);\n }\n \n-int safe_create_reflog(const char *refname, int force_create,\n-\t\t struct strbuf *err)\n+int safe_create_reflog(const char *refname, struct strbuf *err)\n {\n \treturn refs_create_reflog(get_main_ref_store(the_repository), refname,\n-\t\t\t\t force_create, err);\n+\t\t\t\t err);\n }\n \n int refs_delete_reflog(struct ref_store *refs, const char *refname)\ndiff --git a/refs.h b/refs.h\nindex 48970dfc7e0..3c457bc19c8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -416,8 +416,8 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n * Setup reflog before using. Fill in err and return -1 on failure.\n */\n int refs_create_reflog(struct ref_store *refs, const char *refname,\n-\t\t int force_create, struct strbuf *err);\n-int safe_create_reflog(const char *refname, int force_create, struct strbuf *err);\n+\t\t struct strbuf *err);\n+int safe_create_reflog(const char *refname, struct strbuf *err);\n \n /** Reads log for the value of ref during at_time. **/\n int read_ref_at(struct ref_store *refs,\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 1a7a9e11cfa..f6b01b1eba0 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -340,11 +340,10 @@ static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)\n }\n \n static int debug_create_reflog(struct ref_store *ref_store, const char *refname,\n-\t\t\t int force_create, struct strbuf *err)\n+\t\t\t struct strbuf *err)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->create_reflog(drefs->refs, refname,\n-\t\t\t\t\t\t force_create, err);\n+\tint res = drefs->refs->be->create_reflog(drefs->refs, refname, err);\n \ttrace_printf_key(&trace_refs, \"create_reflog: %s: %d\\n\", refname, res);\n \treturn res;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 206c9f8b932..b710d43be16 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1598,15 +1598,14 @@ error:\n \treturn -1;\n }\n \n-static int files_create_reflog(struct ref_store *ref_store,\n-\t\t\t const char *refname, int force_create,\n+static int files_create_reflog(struct ref_store *ref_store, const char *refname,\n \t\t\t struct strbuf *err)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"create_reflog\");\n \tint fd;\n \n-\tif (log_ref_setup(refs, refname, force_create, &fd, err))\n+\tif (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))\n \t\treturn -1;\n \n \tif (fd >= 0)\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex f8aa97d7998..af7038de42d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1618,8 +1618,7 @@ static int packed_reflog_exists(struct ref_store *ref_store,\n }\n \n static int packed_create_reflog(struct ref_store *ref_store,\n-\t\t\t const char *refname, int force_create,\n-\t\t\t struct strbuf *err)\n+\t\t\t\tconst char *refname, struct strbuf *err)\n {\n \tBUG(\"packed reference store does not support reflogs\");\n }\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 3155708345f..cc0e56e8c82 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -590,7 +590,7 @@ typedef int for_each_reflog_ent_reverse_fn(struct ref_store *ref_store,\n \t\t\t\t\t void *cb_data);\n typedef int reflog_exists_fn(struct ref_store *ref_store, const char *refname);\n typedef int create_reflog_fn(struct ref_store *ref_store, const char *refname,\n-\t\t\t int force_create, struct strbuf *err);\n+\t\t\t struct strbuf *err);\n typedef int delete_reflog_fn(struct ref_store *ref_store, const char *refname);\n typedef int reflog_expire_fn(struct ref_store *ref_store,\n \t\t\t const char *refname, const struct object_id *oid,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 0fcad9b3812..a65fda66ddc 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -180,11 +180,10 @@ static int cmd_reflog_exists(struct ref_store *refs, const char **argv)\n static int cmd_create_reflog(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n-\tint force_create = arg_flags(*argv++, \"force-create\");\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n-\tret = refs_create_reflog(refs, refname, force_create, &err);\n+\tret = refs_create_reflog(refs, refname, &err);\n \tif (err.len)\n \t\tputs(err.buf);\n \treturn ret;\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 76b15458409..3cb5e23d6db 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -108,7 +108,7 @@ test_expect_success 'delete_reflog(HEAD)' '\n '\n \n test_expect_success 'create-reflog(HEAD)' '\n-\t$RUN create-reflog HEAD 1 &&\n+\t$RUN create-reflog HEAD &&\n \tgit reflog exists HEAD\n '\n \ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex b0365c1fee0..78d40bdcd8b 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -92,7 +92,7 @@ test_expect_success 'delete_reflog() not allowed' '\n '\n \n test_expect_success 'create-reflog() not allowed' '\n-\ttest_must_fail $RUN create-reflog HEAD 1\n+\ttest_must_fail $RUN create-reflog HEAD\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"434796","messageId":"f6a7c5ad56efceef9c11226beb854b806ef54687.1630947142.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v2.git.git.1630947142.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] RFC: refs: reflog entries aren't written based on reflog existence.","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-06T16:52:22Z","receivedAt":"2021-09-06T16:52:34Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nBefore, if we aren't supposed to update reflogs (eg.\ncore.logallrefupdates=NONE), we would still write reflog entries if the\nreflog file (.git/logs/REFNAME) existed.\n\nThe reftable storage backend cannot distinguish between a non-existing\nreflog, and an empty one. Therefore it cannot mimick this functionality.\n\nIn CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,\nwe came to the conclusion that this feature is probably a remnant from\nthe time that reflogs weren't enabled by default, and it does not need\nto be kept.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n refs/files-backend.c | 53 ++++++++++++++-----------------------------\n t/t1400-update-ref.sh | 5 ++--\n 2 files changed, 19 insertions(+), 39 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b710d43be16..5ba68584aba 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1551,51 +1551,32 @@ static int log_ref_setup(struct files_ref_store *refs,\n \tstruct strbuf logfile_sb = STRBUF_INIT;\n \tchar *logfile;\n \n+\t*logfd = -1;\n+\tif (!force_create && !should_autocreate_reflog(refname))\n+\t\treturn 0;\n+\n \tfiles_reflog_path(refs, &logfile_sb, refname);\n \tlogfile = strbuf_detach(&logfile_sb, NULL);\n \n-\tif (force_create || should_autocreate_reflog(refname)) {\n-\t\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n-\t\t\tif (errno == ENOENT)\n-\t\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n-\t\t\t\t\t \"%s\", logfile, strerror(errno));\n-\t\t\telse if (errno == EISDIR)\n-\t\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n-\t\t\t\t\t logfile);\n-\t\t\telse\n-\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t\t logfile, strerror(errno));\n-\n-\t\t\tgoto error;\n-\t\t}\n-\t} else {\n-\t\t*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);\n-\t\tif (*logfd < 0) {\n-\t\t\tif (errno == ENOENT || errno == EISDIR) {\n-\t\t\t\t/*\n-\t\t\t\t * The logfile doesn't already exist,\n-\t\t\t\t * but that is not an error; it only\n-\t\t\t\t * means that we won't write log\n-\t\t\t\t * entries to it.\n-\t\t\t\t */\n-\t\t\t\t;\n-\t\t\t} else {\n-\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t\t logfile, strerror(errno));\n-\t\t\t\tgoto error;\n-\t\t\t}\n-\t\t}\n+\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n+\t\tif (errno == ENOENT)\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t \"unable to create directory for '%s': \"\n+\t\t\t\t \"%s\",\n+\t\t\t\t logfile, strerror(errno));\n+\t\telse if (errno == EISDIR)\n+\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n+\t\t\t\t logfile);\n+\t\telse\n+\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n+\t\t\t\t logfile, strerror(errno));\n \t}\n \n \tif (*logfd >= 0)\n \t\tadjust_shared_perm(logfile);\n \n \tfree(logfile);\n-\treturn 0;\n-\n-error:\n-\tfree(logfile);\n-\treturn -1;\n+\treturn (*logfd < 0) ? -1 : 0;\n }\n \n static int files_create_reflog(struct ref_store *ref_store, const char *refname,\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 8ced98e0fe8..446b568cef3 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -270,7 +270,7 @@ test_expect_success \"(not) changed .git/$m\" '\n '\n \n rm -f .git/logs/refs/heads/main\n-test_expect_success \"create $m (logged by touch)\" '\n+test_expect_success \"create $m\" '\n \ttest_config core.logAllRefUpdates false &&\n \tGIT_COMMITTER_DATE=\"2005-05-26 23:30\" \\\n \tgit update-ref --create-reflog HEAD $A -m \"Initial Creation\" &&\n@@ -318,9 +318,8 @@ test_expect_success 'symref empty directory removal' '\n \n cat >expect < 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$TAB\n EOF\n+\n test_expect_success \"verifying $m's log (logged by touch)\" '\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-- \ngitgitgadget\n"},{"id":"434807","messageId":"878s09nxh1.fsf@evledraar.gmail.com","threadId":"56404","inReplyTo":"11b296a55e9e450575c64ade1a2cb93a1886b9da.1630947142.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-06T22:34:43Z","receivedAt":"2021-09-06T22:35:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> From: Han-Wen Nienhuys \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>\n> Signed-off-by: Han-Wen Nienhuys \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\nNit: Would be a more readable diff if this wasn't a\nline-wrap-while-at-it change in addition to changing the format string.\n\nI.e. the last 4x parameters aren't changed, so leaving them on their own\nline & just changing the string & the two oid_to_hex()...\n"},{"id":"434808","messageId":"874kaxnxb0.fsf@evledraar.gmail.com","threadId":"56404","inReplyTo":"995d450da42f35b4cecdd699fa1b1a74518f481e.1630947142.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/5] refs: trim newline from reflog message","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-06T22:38:05Z","receivedAt":"2021-09-06T22:41:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> From: Han-Wen Nienhuys \n> [...]\n> -\t\t\t\tputs(reflog_msg[i]);\n> +\t\t\t\tputs(reflog_msg[i]); /* XXX - this puts a\n> +\t\t\t\t\t\t\tnewline. Did we put two\n> +\t\t\t\t\t\t\tnewlines beforehand? */\n\nI recently added some tests for show-branch, see the tip of\nab/show-branch-tests. This seems like it would be clearer to both you &\nreviewers if we first checked & test_cmp'd the reflog behavior of \"git\nshow-branch\" in some \"here's how it works now test\", then made whatever\nnon-changes to the format done here.\n\nPresumably this newline mystery would become clear once we'd have\ncoverage of its reflog codepaths.\n\n"},{"id":"434809","messageId":"87zgspmidx.fsf@evledraar.gmail.com","threadId":"56404","inReplyTo":"aa25fd9b7de02c3b5c620def8fae4d5b6ce456c5.1630947142.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] refs: drop force_create argument of create_reflog API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-06T22:42:26Z","receivedAt":"2021-09-06T22:47:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> From: Han-Wen Nienhuys \n>\n> There is only one caller, builtin/checkout.c, and it hardcodes\n> force_create=1.\n\nWas it ever needed? A glance at abd0cd3a301 (refs: new public ref\nfunction: safe_create_reflog, 2015-07-21) suggests probably not, but\nthen there's 0f2a71d9923 (refs: add REF_FORCE_CREATE_REFLOG flag,\n2015-07-21) ...\n \n> [...]\n> -static int files_create_reflog(struct ref_store *ref_store,\n> -\t\t\t const char *refname, int force_create,\n> +static int files_create_reflog(struct ref_store *ref_store, const char *refname,\n> \t\t\t struct strbuf *err)\n> {\n> \tstruct files_ref_store *refs =\n> \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"create_reflog\");\n> \tint fd;\n> \n> -\tif (log_ref_setup(refs, refname, force_create, &fd, err))\n> +\tif (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))\n\nWe can lose the inline comment here & let the function definition speak\nfor itself, we usually don't inline comment boolean flags.\n\nIn any case, having not dug (but presumably you have) some overview of\nhow we ended up having this not-required flag would be nice. I.e. was it\nalways this dead-end, or did we replace it with REF_FORCE_CREATE_REFLOG\nat some point etc?\n"},{"id":"434810","messageId":"87v93dmh2n.fsf@evledraar.gmail.com","threadId":"56404","inReplyTo":"f6a7c5ad56efceef9c11226beb854b806ef54687.1630947142.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] RFC: refs: reflog entries aren't written based on reflog existence.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-06T22:50:48Z","receivedAt":"2021-09-06T23:15:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,\n\nNit: use for quoting, not message-id.\n\n> we came to the conclusion that this feature is probably a remnant from\n> the time that reflogs weren't enabled by default, and it does not need\n> to be kept.\n\nMaybe some summary of the flexibily either Jeff King or Junio mentioned\nwe were losing (i.e. we can't selectively enable per-ref now), but that\nwe think it's OK because...\n\nFor the implementation:\n\n> +\t*logfd = -1;\n\nWeird, more on this later...\n\n\n> +\tif (!force_create && !should_autocreate_reflog(refname))\n> +\t\treturn 0;\n\nOK, so we can early abort.\n\n> \tfiles_reflog_path(refs, &logfile_sb, refname);\n> \tlogfile = strbuf_detach(&logfile_sb, NULL);\n> \n> -\tif (force_create || should_autocreate_reflog(refname)) {\n> -\t\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n> -\t\t\tif (errno == ENOENT)\n> -\t\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n> -\t\t\t\t\t \"%s\", logfile, strerror(errno));\n\nHere we use one indent/wrapping style...\n\n> -\t\t\telse if (errno == EISDIR)\n> -\t\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n> -\t\t\t\t\t logfile);\n> -\t\t\telse\n> -\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> -\t\t\t\t\t logfile, strerror(errno));\n> -\n> -\t\t\tgoto error;\n> -\t\t}\n> -\t} else {\n> -\t\t*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);\n> -\t\tif (*logfd < 0) {\n> -\t\t\tif (errno == ENOENT || errno == EISDIR) {\n> -\t\t\t\t/*\n> -\t\t\t\t * The logfile doesn't already exist,\n> -\t\t\t\t * but that is not an error; it only\n> -\t\t\t\t * means that we won't write log\n> -\t\t\t\t * entries to it.\n> -\t\t\t\t */\n> -\t\t\t\t;\n> -\t\t\t} else {\n> -\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> -\t\t\t\t\t logfile, strerror(errno));\n> -\t\t\t\tgoto error;\n> -\t\t\t}\n> -\t\t}\n> +\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n> +\t\tif (errno == ENOENT)\n> +\t\t\tstrbuf_addf(err,\n> +\t\t\t\t \"unable to create directory for '%s': \"\n> +\t\t\t\t \"%s\",\n> +\t\t\t\t logfile, strerror(errno));\n\n...but here it's changed while we're at it, this patch would be easier\nto follow IMO if we just left the formatting alone (or did it as another\nstep). I'm aware that it ends us at over 79 columns, but that was the\ncase before...\n\n> +\t\telse if (errno == EISDIR)\n> +\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n> +\t\t\t\t logfile);\n> +\t\telse\n> +\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> +\t\t\t\t logfile, strerror(errno));\n> \t}\n> \n> \tif (*logfd >= 0)\n> \t\tadjust_shared_perm(logfile);\n> \n> \tfree(logfile);\n> -\treturn 0;\n> -\n> -error:\n> -\tfree(logfile);\n> -\treturn -1;\n> +\treturn (*logfd < 0) ? -1 : 0;\n\nOn \"more on this later\": Since we just return -1, 0 or a valid fd now,\ncan't we just return the \"fd\" here and let the callers sort out -1, 0\nand >0?\n"},{"id":"434874","messageId":"CAFQ2z_Oen0Q5hfpbZdzWgJ8D=EetBNGx7sQ3ZxnJSXC-tSKGYw@mail.gmail.com","threadId":"56404","inReplyTo":"878s09nxh1.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-09-07T13:33:30Z","receivedAt":"2021-09-07T13:33:44Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Sep 7, 2021 at 12:35 AM Ævar Arnfjörð Bjarmason\n wrote:\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> Nit: Would be a more readable diff if this wasn't a\n> line-wrap-while-at-it change in addition to changing the format string.\n>\n> I.e. the last 4x parameters aren't changed, so leaving them on their own\n> line & just changing the string & the two oid_to_hex()...\n\nThis is clang-format's output, and the new code takes up less lines vertically.\n\nIf you think this is really, really important, I can change it, but I\nthink it's a better use of everyone's time to leave mechanical tasks\n(like formatting) to the machines.\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":"434875","messageId":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v2.git.git.1630947142.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:41Z","receivedAt":"2021-09-07T13:37:00Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":" 1: e158882812f show-branch: show reflog message\n 1: 995d450da42 ! 2: d16d94164c1 refs: trim newline from reflog message\n @@ Commit message\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\t\tshow_one_commit(rev[i], 1);\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 - \t\t\telse\n --\t\t\t\tputs(reflog_msg[i]);\n -+\t\t\t\tputs(reflog_msg[i]); /* XXX - this puts a\n -+\t\t\t\t\t\t\tnewline. Did we put two\n -+\t\t\t\t\t\t\tnewlines beforehand? */\n \n - \t\t\tif (is_head)\n - \t\t\t\thead_at = i;\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 2: 11b296a55e9 = 3: e273963216c test-ref-store: tweaks to for-each-reflog-ent format\n 3: 9ec09cc64cd = 4: 52093fce57c t1400: use test-helper ref-store to inspect reflog contents\n 4: aa25fd9b7de ! 5: ce0047028dd refs: drop force_create argument of create_reflog API\n @@ Commit message\n There is only one caller, builtin/checkout.c, and it hardcodes\n force_create=1.\n \n + This argument was introduced in abd0cd3a301 (refs: new public ref function:\n + safe_create_reflog, 2015-07-21), which promised to immediately use it in a\n + follow-on commit, but that never happened.\n +\n Signed-off-by: Han-Wen Nienhuys \n \n ## builtin/checkout.c ##\n @@ refs/files-backend.c: error:\n \tint fd;\n \n -\tif (log_ref_setup(refs, refname, force_create, &fd, err))\n -+\tif (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))\n ++\tif (log_ref_setup(refs, refname, 1, &fd, err))\n \t\treturn -1;\n \n \tif (fd >= 0)\n 5: f6a7c5ad56e ! 6: 7a030cfd3e2 RFC: refs: reflog entries aren't written based on reflog existence.\n @@ Commit message\n The reftable storage backend cannot distinguish between a non-existing\n reflog, and an empty one. Therefore it cannot mimick this functionality.\n \n - In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,\n - we came to the conclusion that this feature is probably a remnant from\n - the time that reflogs weren't enabled by default, and it does not need\n - to be kept.\n + With this feature, it is possible to mark only specific branches as subject to\n + reflog updates. When introduced, it presumably served as a cheap substitute for\n + introducing branch.$NAME.logRefUpdate configuration setting.\n +\n + Reflogs are small and don't impact the runtime of normal operations, so this\n + flexibility is not very useful. Since it incurs complexity for alternate ref\n + backends, we remove it.\n +\n + Further background to this change is in\n + .\n \n Signed-off-by: Han-Wen Nienhuys \n \n -: ----------- > 7: 1124dbad594 refs: change log_ref_setup calling convention\n\n-- \ngitgitgadget\n"},{"id":"434876","messageId":"e158882812fef0c6a69fb0bc1ff5d46f38906426.1631021808.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] show-branch: show reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:42Z","receivedAt":"2021-09-07T13:37:01Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \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 \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 d77ce7aeb38..902a0d99850 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -762,6 +762,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@@ -771,11 +772,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":"434877","messageId":"ce0047028dd4941c7b91d3299107651238b1431f.1631021808.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] refs: drop force_create argument of create_reflog API","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:46Z","receivedAt":"2021-09-07T13:37:02Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nThere is only one caller, builtin/checkout.c, and it hardcodes\nforce_create=1.\n\nThis argument was introduced in abd0cd3a301 (refs: new public ref function:\nsafe_create_reflog, 2015-07-21), which promised to immediately use it in a\nfollow-on commit, but that never happened.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n builtin/checkout.c | 2 +-\n refs.c | 9 ++++-----\n refs.h | 4 ++--\n refs/debug.c | 5 ++---\n refs/files-backend.c | 5 ++---\n refs/packed-backend.c | 3 +--\n refs/refs-internal.h | 2 +-\n t/helper/test-ref-store.c | 3 +--\n t/t1405-main-ref-store.sh | 2 +-\n t/t1406-submodule-ref-store.sh | 2 +-\n 10 files changed, 16 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b5d477919a7..3c6506e0595 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -876,7 +876,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tint ret;\n \t\t\t\tstruct strbuf err = STRBUF_INIT;\n \n-\t\t\t\tret = safe_create_reflog(refname, 1, &err);\n+\t\t\t\tret = safe_create_reflog(refname, &err);\n \t\t\t\tif (ret) {\n \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n \t\t\t\t\t\topts->new_orphan_branch, err.buf);\ndiff --git a/refs.c b/refs.c\nindex 8b9f7c3a80a..ca59a7cc652 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2347,16 +2347,15 @@ int reflog_exists(const char *refname)\n }\n \n int refs_create_reflog(struct ref_store *refs, const char *refname,\n-\t\t int force_create, struct strbuf *err)\n+\t\t struct strbuf *err)\n {\n-\treturn refs->be->create_reflog(refs, refname, force_create, err);\n+\treturn refs->be->create_reflog(refs, refname, err);\n }\n \n-int safe_create_reflog(const char *refname, int force_create,\n-\t\t struct strbuf *err)\n+int safe_create_reflog(const char *refname, struct strbuf *err)\n {\n \treturn refs_create_reflog(get_main_ref_store(the_repository), refname,\n-\t\t\t\t force_create, err);\n+\t\t\t\t err);\n }\n \n int refs_delete_reflog(struct ref_store *refs, const char *refname)\ndiff --git a/refs.h b/refs.h\nindex 48970dfc7e0..3c457bc19c8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -416,8 +416,8 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n * Setup reflog before using. Fill in err and return -1 on failure.\n */\n int refs_create_reflog(struct ref_store *refs, const char *refname,\n-\t\t int force_create, struct strbuf *err);\n-int safe_create_reflog(const char *refname, int force_create, struct strbuf *err);\n+\t\t struct strbuf *err);\n+int safe_create_reflog(const char *refname, struct strbuf *err);\n \n /** Reads log for the value of ref during at_time. **/\n int read_ref_at(struct ref_store *refs,\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 1a7a9e11cfa..f6b01b1eba0 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -340,11 +340,10 @@ static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)\n }\n \n static int debug_create_reflog(struct ref_store *ref_store, const char *refname,\n-\t\t\t int force_create, struct strbuf *err)\n+\t\t\t struct strbuf *err)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->create_reflog(drefs->refs, refname,\n-\t\t\t\t\t\t force_create, err);\n+\tint res = drefs->refs->be->create_reflog(drefs->refs, refname, err);\n \ttrace_printf_key(&trace_refs, \"create_reflog: %s: %d\\n\", refname, res);\n \treturn res;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 206c9f8b932..04d73b89c9c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1598,15 +1598,14 @@ error:\n \treturn -1;\n }\n \n-static int files_create_reflog(struct ref_store *ref_store,\n-\t\t\t const char *refname, int force_create,\n+static int files_create_reflog(struct ref_store *ref_store, const char *refname,\n \t\t\t struct strbuf *err)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"create_reflog\");\n \tint fd;\n \n-\tif (log_ref_setup(refs, refname, force_create, &fd, err))\n+\tif (log_ref_setup(refs, refname, 1, &fd, err))\n \t\treturn -1;\n \n \tif (fd >= 0)\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex f8aa97d7998..af7038de42d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1618,8 +1618,7 @@ static int packed_reflog_exists(struct ref_store *ref_store,\n }\n \n static int packed_create_reflog(struct ref_store *ref_store,\n-\t\t\t const char *refname, int force_create,\n-\t\t\t struct strbuf *err)\n+\t\t\t\tconst char *refname, struct strbuf *err)\n {\n \tBUG(\"packed reference store does not support reflogs\");\n }\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 3155708345f..cc0e56e8c82 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -590,7 +590,7 @@ typedef int for_each_reflog_ent_reverse_fn(struct ref_store *ref_store,\n \t\t\t\t\t void *cb_data);\n typedef int reflog_exists_fn(struct ref_store *ref_store, const char *refname);\n typedef int create_reflog_fn(struct ref_store *ref_store, const char *refname,\n-\t\t\t int force_create, struct strbuf *err);\n+\t\t\t struct strbuf *err);\n typedef int delete_reflog_fn(struct ref_store *ref_store, const char *refname);\n typedef int reflog_expire_fn(struct ref_store *ref_store,\n \t\t\t const char *refname, const struct object_id *oid,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 0fcad9b3812..a65fda66ddc 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -180,11 +180,10 @@ static int cmd_reflog_exists(struct ref_store *refs, const char **argv)\n static int cmd_create_reflog(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n-\tint force_create = arg_flags(*argv++, \"force-create\");\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n-\tret = refs_create_reflog(refs, refname, force_create, &err);\n+\tret = refs_create_reflog(refs, refname, &err);\n \tif (err.len)\n \t\tputs(err.buf);\n \treturn ret;\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 76b15458409..3cb5e23d6db 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -108,7 +108,7 @@ test_expect_success 'delete_reflog(HEAD)' '\n '\n \n test_expect_success 'create-reflog(HEAD)' '\n-\t$RUN create-reflog HEAD 1 &&\n+\t$RUN create-reflog HEAD &&\n \tgit reflog exists HEAD\n '\n \ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex b0365c1fee0..78d40bdcd8b 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -92,7 +92,7 @@ test_expect_success 'delete_reflog() not allowed' '\n '\n \n test_expect_success 'create-reflog() not allowed' '\n-\ttest_must_fail $RUN create-reflog HEAD 1\n+\ttest_must_fail $RUN create-reflog HEAD\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"434878","messageId":"d16d94164c141ded2a918aabf7044e360543fb61.1631021808.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] refs: trim newline from reflog message","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:43Z","receivedAt":"2021-09-07T13:37:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \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 \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 902a0d99850..58a894e81e2 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -762,7 +762,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@@ -773,10 +772,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 e9cd3283694..944cce929c8 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -245,8 +245,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@@ -285,10 +283,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 677b7e4cdd2..206c9f8b932 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1897,17 +1897,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 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 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@@ -1999,6 +1997,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@@ -2011,6 +2010,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@@ -2060,7 +2060,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@@ -3020,18 +3020,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":"434879","messageId":"e273963216cdaa3c08eee6032da0677b546ecd9d.1631021808.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:44Z","receivedAt":"2021-09-07T13:37:04Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \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 \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":"434880","messageId":"7a030cfd3e2521f08562ff7ecc00743b786c0e32.1631021808.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] RFC: refs: reflog entries aren't written based on reflog existence.","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:47Z","receivedAt":"2021-09-07T13:37:08Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nBefore, if we aren't supposed to update reflogs (eg.\ncore.logallrefupdates=NONE), we would still write reflog entries if the\nreflog file (.git/logs/REFNAME) existed.\n\nThe reftable storage backend cannot distinguish between a non-existing\nreflog, and an empty one. Therefore it cannot mimick this functionality.\n\nWith this feature, it is possible to mark only specific branches as subject to\nreflog updates. When introduced, it presumably served as a cheap substitute for\nintroducing branch.$NAME.logRefUpdate configuration setting.\n\nReflogs are small and don't impact the runtime of normal operations, so this\nflexibility is not very useful. Since it incurs complexity for alternate ref\nbackends, we remove it.\n\nFurther background to this change is in\n.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n refs/files-backend.c | 53 ++++++++++++++-----------------------------\n t/t1400-update-ref.sh | 5 ++--\n 2 files changed, 19 insertions(+), 39 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 04d73b89c9c..4aa4d2bbba1 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1551,51 +1551,32 @@ static int log_ref_setup(struct files_ref_store *refs,\n \tstruct strbuf logfile_sb = STRBUF_INIT;\n \tchar *logfile;\n \n+\t*logfd = -1;\n+\tif (!force_create && !should_autocreate_reflog(refname))\n+\t\treturn 0;\n+\n \tfiles_reflog_path(refs, &logfile_sb, refname);\n \tlogfile = strbuf_detach(&logfile_sb, NULL);\n \n-\tif (force_create || should_autocreate_reflog(refname)) {\n-\t\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n-\t\t\tif (errno == ENOENT)\n-\t\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n-\t\t\t\t\t \"%s\", logfile, strerror(errno));\n-\t\t\telse if (errno == EISDIR)\n-\t\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n-\t\t\t\t\t logfile);\n-\t\t\telse\n-\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t\t logfile, strerror(errno));\n-\n-\t\t\tgoto error;\n-\t\t}\n-\t} else {\n-\t\t*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);\n-\t\tif (*logfd < 0) {\n-\t\t\tif (errno == ENOENT || errno == EISDIR) {\n-\t\t\t\t/*\n-\t\t\t\t * The logfile doesn't already exist,\n-\t\t\t\t * but that is not an error; it only\n-\t\t\t\t * means that we won't write log\n-\t\t\t\t * entries to it.\n-\t\t\t\t */\n-\t\t\t\t;\n-\t\t\t} else {\n-\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t\t logfile, strerror(errno));\n-\t\t\t\tgoto error;\n-\t\t\t}\n-\t\t}\n+\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n+\t\tif (errno == ENOENT)\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t \"unable to create directory for '%s': \"\n+\t\t\t\t \"%s\",\n+\t\t\t\t logfile, strerror(errno));\n+\t\telse if (errno == EISDIR)\n+\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n+\t\t\t\t logfile);\n+\t\telse\n+\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n+\t\t\t\t logfile, strerror(errno));\n \t}\n \n \tif (*logfd >= 0)\n \t\tadjust_shared_perm(logfile);\n \n \tfree(logfile);\n-\treturn 0;\n-\n-error:\n-\tfree(logfile);\n-\treturn -1;\n+\treturn (*logfd < 0) ? -1 : 0;\n }\n \n static int files_create_reflog(struct ref_store *ref_store, const char *refname,\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 8ced98e0fe8..446b568cef3 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -270,7 +270,7 @@ test_expect_success \"(not) changed .git/$m\" '\n '\n \n rm -f .git/logs/refs/heads/main\n-test_expect_success \"create $m (logged by touch)\" '\n+test_expect_success \"create $m\" '\n \ttest_config core.logAllRefUpdates false &&\n \tGIT_COMMITTER_DATE=\"2005-05-26 23:30\" \\\n \tgit update-ref --create-reflog HEAD $A -m \"Initial Creation\" &&\n@@ -318,9 +318,8 @@ test_expect_success 'symref empty directory removal' '\n \n cat >expect < 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$TAB\n EOF\n+\n test_expect_success \"verifying $m's log (logged by touch)\" '\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-- \ngitgitgadget\n\n"},{"id":"434881","messageId":"52093fce57c7ef91b6d730f50d6b7ffa6cadc03f.1631021808.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] t1400: use test-helper ref-store to inspect reflog contents","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:45Z","receivedAt":"2021-09-07T13:37:10Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nSigned-off-by: Han-Wen Nienhuys \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 4506cd435b0..8ced98e0fe8 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 < 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 < 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\n"},{"id":"434882","messageId":"1124dbad594ed24d4b20d5afc213b57c6f253603.1631021808.git.gitgitgadget@gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] refs: change log_ref_setup calling convention","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-07T13:36:48Z","receivedAt":"2021-09-07T13:37:12Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nSigned-off-by: Han-Wen Nienhuys \n---\n refs/files-backend.c | 48 ++++++++++++++++++--------------------------\n 1 file changed, 20 insertions(+), 28 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4aa4d2bbba1..114d01a9e3c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1535,30 +1535,26 @@ static int open_or_create_logfile(const char *path, void *cb)\n }\n \n /*\n- * Create a reflog for a ref. If force_create = 0, only create the\n- * reflog for certain refs (those for which should_autocreate_reflog\n- * returns non-zero). Otherwise, create it regardless of the reference\n- * name. If the logfile already existed or was created, return 0 and\n- * set *logfd to the file descriptor opened for appending to the file.\n- * If no logfile exists and we decided not to create one, return 0 and\n- * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and\n- * return -1.\n+ * Create a reflog for a ref. If force_create = 0, only create the reflog for\n+ * certain refs (those for which should_autocreate_reflog returns non-zero).\n+ * Otherwise, create it regardless of the reference name. On success, return the\n+ * filedescriptor. If the log should not be written, return 0 On error, return\n+ * -1 and fill in *err.\n */\n-static int log_ref_setup(struct files_ref_store *refs,\n-\t\t\t const char *refname, int force_create,\n-\t\t\t int *logfd, struct strbuf *err)\n+static int log_ref_setup(struct files_ref_store *refs, const char *refname,\n+\t\t\t int force_create, struct strbuf *err)\n {\n \tstruct strbuf logfile_sb = STRBUF_INIT;\n \tchar *logfile;\n \n-\t*logfd = -1;\n+\tint logfd = -1;\n \tif (!force_create && !should_autocreate_reflog(refname))\n \t\treturn 0;\n \n \tfiles_reflog_path(refs, &logfile_sb, refname);\n \tlogfile = strbuf_detach(&logfile_sb, NULL);\n \n-\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n+\tif (raceproof_create_file(logfile, open_or_create_logfile, &logfd)) {\n \t\tif (errno == ENOENT)\n \t\t\tstrbuf_addf(err,\n \t\t\t\t \"unable to create directory for '%s': \"\n@@ -1572,11 +1568,11 @@ static int log_ref_setup(struct files_ref_store *refs,\n \t\t\t\t logfile, strerror(errno));\n \t}\n \n-\tif (*logfd >= 0)\n+\tif (logfd > 0)\n \t\tadjust_shared_perm(logfile);\n \n \tfree(logfile);\n-\treturn (*logfd < 0) ? -1 : 0;\n+\treturn logfd;\n }\n \n static int files_create_reflog(struct ref_store *ref_store, const char *refname,\n@@ -1584,15 +1580,14 @@ static int files_create_reflog(struct ref_store *ref_store, const char *refname,\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"create_reflog\");\n-\tint fd;\n-\n-\tif (log_ref_setup(refs, refname, 1, &fd, err))\n-\t\treturn -1;\n+\tint fd = log_ref_setup(refs, refname, 1, err);\n \n-\tif (fd >= 0)\n+\tif (fd > 0) {\n \t\tclose(fd);\n+\t\treturn 0;\n+\t}\n \n-\treturn 0;\n+\treturn fd;\n }\n \n static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n@@ -1624,15 +1619,12 @@ static int files_log_ref_write(struct files_ref_store *refs,\n \tif (log_all_ref_updates == LOG_REFS_UNSET)\n \t\tlog_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n \n-\tresult = log_ref_setup(refs, refname,\n-\t\t\t flags & REF_FORCE_CREATE_REFLOG,\n-\t\t\t &logfd, err);\n+\tlogfd = log_ref_setup(refs, refname, flags & REF_FORCE_CREATE_REFLOG,\n+\t\t\t err);\n \n-\tif (result)\n-\t\treturn result;\n+\tif (logfd <= 0)\n+\t\treturn logfd;\n \n-\tif (logfd < 0)\n-\t\treturn 0;\n \tresult = log_ref_write_fd(logfd, old_oid, new_oid,\n \t\t\t\t git_committer_info(0), msg);\n \tif (result) {\n-- \ngitgitgadget\n"},{"id":"434891","messageId":"87fsugl6en.fsf@evledraar.gmail.com","threadId":"56404","inReplyTo":"CAFQ2z_Oen0Q5hfpbZdzWgJ8D=EetBNGx7sQ3ZxnJSXC-tSKGYw@mail.gmail.com","subject":"Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-07T15:53:07Z","receivedAt":"2021-09-07T16:05:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 07 2021, Han-Wen Nienhuys wrote:\n\n> On Tue, Sep 7, 2021 at 12:35 AM Ævar Arnfjörð Bjarmason\n> wrote:\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>> Nit: Would be a more readable diff if this wasn't a\n>> line-wrap-while-at-it change in addition to changing the format string.\n>>\n>> I.e. the last 4x parameters aren't changed, so leaving them on their own\n>> line & just changing the string & the two oid_to_hex()...\n>\n> This is clang-format's output, and the new code takes up less lines vertically.\n>\n> If you think this is really, really important, I can change it, but I\n> think it's a better use of everyone's time to leave mechanical tasks\n> (like formatting) to the machines.\n\nIt's not \"really, really important\" or even \"really important\", just\nnotes while reading the series.\n\nPatches are read N times, including during review. Since humans read\nformatting changes, it's in general are not something we can leave to\nmachines. We're after all looking at the patch on-list, not diffing two\nversions of the generated machine code, or of clang-format's output.\n\nI don't think the intent of the clang-format target is to run it before\npatch submission, but to serve as input on suggested formatting changes.\n\nIf on master you run:\n\n git ls-files '*.[ch]' -z | xargs -n 1 -0 clang-format -i\n\nYou'll get:\n\n 631 files changed, 48246 insertions(+), 45061 deletions(-)\n\nWhich I think speaks for itself in the likelyhood that\n\"git-clang-format\" is going to inject unnecessary churn into submitted\npatches.\n\nI think this case is quite small and not worth a re-roll.\n"},{"id":"437978","messageId":"CAFQ2z_PAs3fG1h31KWVXJ=02t1i9UvWPuD4tCK-ZWfewkPQJEw@mail.gmail.com","threadId":"56404","inReplyTo":"pull.1067.v3.git.git.1631021808.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/7] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-10-05T15:54:14Z","receivedAt":"2021-10-05T15:54:27Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Sep 7, 2021 at 3:37 PM Han-Wen Nienhuys via GitGitGadget\n wrote:\n>\n> CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com\n>\n> v3:\n>\n> * fix show-branch\n> * add some more context to commit messages\n> * change calling convention for log_ref_setup; could fold into predecessor\n> if needed too.\n\nHi Junio,\n\nI had the impression that I addressed all outstanding comments (but\nnot sure). Are you waiting for me to do something before this can go\ninto 'seen' ?\n\nThere is a merge conflict against master, so I'll send a v4 shortly.\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":"438083","messageId":"xmqqk0iqrp4q.fsf@gitster.g","threadId":"56404","inReplyTo":"CAFQ2z_PAs3fG1h31KWVXJ=02t1i9UvWPuD4tCK-ZWfewkPQJEw@mail.gmail.com","subject":"Re: [PATCH v3 0/7] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-06T18:20:53Z","receivedAt":"2021-10-06T18:20:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> I had the impression that I addressed all outstanding comments (but\n> not sure). Are you waiting for me to do something before this can go\n> into 'seen' ?\n>\n> There is a merge conflict against master, so I'll send a v4 shortly.\n\nSorry. I seem to have looked at and commented on the precursor RFC\nof this topic, but nobody other than Ævar seems to have commented on\nthe second iteration and the topic was completely under my radar,\nand I do not remember what it was about.\n\nIt would be good to have an update for others to see.\n\nThanks.\n\n\n"},{"id":"438084","messageId":"xmqqfsterotr.fsf@gitster.g","threadId":"56404","inReplyTo":"xmqqk0iqrp4q.fsf@gitster.g","subject":"Re: [PATCH v3 0/7] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-06T18:27:28Z","receivedAt":"2021-10-06T18:27:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Han-Wen Nienhuys writes:\n>\n>> I had the impression that I addressed all outstanding comments (but\n>> not sure). Are you waiting for me to do something before this can go\n>> into 'seen' ?\n>>\n>> There is a merge conflict against master, so I'll send a v4 shortly.\n>\n> Sorry. I seem to have looked at and commented on the precursor RFC\n> of this topic, but nobody other than Ævar seems to have commented on\n> the second iteration and the topic was completely under my radar,\n> and I do not remember what it was about.\n>\n> It would be good to have an update for others to see.\n>\n> Thanks.\n\nNot really. I think the comment on the RFC still stands, and I do\nnot recall seeing a response to the point.\n\n One potential harm this change will bring to us is what happens to\n people who disable core.logAllRefUpdates manually after using the\n repository for a while. Their @{4} will point at the same commit no\n matter how many operations are done on the current branch after they\n do so. I wouldn't mind if \"git reflog disable\" command is given to\n the users prominently and core.logAllRefUpdates becomes a mere\n implementation detail nobody has to care about---in such a world, we\n could set the configuration and drop the existing reflog records at\n the same time and nobody will be hurt.\n\n If we keep the current behaviour, what are we harming instead?\n Growth of diskspace usage is an obvious one, but disks are cheaper\n compared to human brainwave cycles cost.\n\nAs it is about the basic design of the \"feature\" (or misfeature), no\nmatter how improved the internal implementation details get, I am\nskeptical how it is solved (other than \"immediately when we notice\ncore.logAllRefUpdates is disabled, remove all the existing reflog\nentries to avoid confusion, in addition to stop appending any more\nreflog entries\", that is).\n"},{"id":"438182","messageId":"xmqqbl40razw.fsf@gitster.g","threadId":"56404","inReplyTo":"xmqqfsterotr.fsf@gitster.g","subject":"Re: [PATCH v3 0/7] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-07T17:38:27Z","receivedAt":"2021-10-07T17:38:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Not really. I think the comment on the RFC still stands, and I do\n> not recall seeing a response to the point.\n>\n> One potential harm this change will bring to us is what happens to\n> people who disable core.logAllRefUpdates manually after using the\n> repository for a while. Their @{4} will point at the same commit no\n> matter how many operations are done on the current branch after they\n> do so. I wouldn't mind if \"git reflog disable\" command is given to\n> the users prominently and core.logAllRefUpdates becomes a mere\n> implementation detail nobody has to care about---in such a world, we\n> could set the configuration and drop the existing reflog records at\n> the same time and nobody will be hurt.\n>\n> If we keep the current behaviour, what are we harming instead?\n> Growth of diskspace usage is an obvious one, but disks are cheaper\n> compared to human brainwave cycles cost.\n\nIIRC, the only reason why reftable implementation may want to change\nthe behaviour we have to avoid getting blamed for breaking is\nbecause it cannot implement \"a reflog exists, and we need to record\nfurther ref movements by appending to it, no matter what the\nconfiguration says\" when the existing reflog is empty, because its\ndata structure lacks support for expressing \"exists but empty\".\n\nI think the behaviour change described in the title of this message\ncan be limited in the scope to hurt users a lot less, and can still\nsatisfy the goal of helping reftable not getting blamed for\nbreakage, perhaps by making the behaviour for an empty but existing\nreflog unspecified or implementation defined per backend.\n\nThat would allow us to avoid situation where a user can say foo@{1}\nbut it does not refer to the point where the branch's tip pointed\njust before the most recent update to it. As an empty but existing\nreflog would not give foo@{$n} for any value of $n, there is much\nless risk of confusing users if we did not append new entries to it,\nI would hope.\n\n"},{"id":"440906","messageId":"CAFQ2z_NASfhdHdZrY3gK29LRK_8Guj0LZ=GgCr84k7XX2L+Dow@mail.gmail.com","threadId":"56404","inReplyTo":"xmqqbl40razw.fsf@gitster.g","subject":"Re: [PATCH v3 0/7] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-11-11T11:46:12Z","receivedAt":"2021-11-11T11:46:28Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Oct 7, 2021 at 7:38 PM Junio C Hamano wrote:\n> > Not really. I think the comment on the RFC still stands, and I do\n> > not recall seeing a response to the point.\n> >\n> > One potential harm this change will bring to us is what happens to\n> > people who disable core.logAllRefUpdates manually after using the\n> > repository for a while. Their @{4} will point at the same commit no\n> > matter how many operations are done on the current branch after they\n> > do so. I wouldn't mind if \"git reflog disable\" command is given to\n> > the users prominently and core.logAllRefUpdates becomes a mere\n> > implementation detail nobody has to care about---in such a world, we\n> > could set the configuration and drop the existing reflog records at\n> > the same time and nobody will be hurt.\n\nA git 'reflog disable' command would address your concerns, but it is\na destructive operation, so the cure might be worse than the solution.\n\n> IIRC, the only reason why reftable implementation may want to change\n> the behaviour we have to avoid getting blamed for breaking is\n> because it cannot implement \"a reflog exists, and we need to record\n> further ref movements by appending to it, no matter what the\n> configuration says\" when the existing reflog is empty, because its\n> data structure lacks support for expressing \"exists but empty\".\n>\n> I think the behaviour change described in the title of this message\n> can be limited in the scope to hurt users a lot less, and can still\n> satisfy the goal of helping reftable not getting blamed for\n> breakage, perhaps by making the behaviour for an empty but existing\n> reflog unspecified or implementation defined per backend.\n\nIf we accept implementation-dependent features, we could just leave\nthe whole feature as is. I had expected more breakage, but there is\nonly one test case in t1400 that needs addressing. If the test\ncoverage reflects the popularity of the feature, it should be fine to\nleave this divergence in, and mark the test with REFFILES.\n\nThe commits prior to the RFC should be OK for committing. In\nparticular, there is a bugfix for the show-branch command. Should I\nresend those separately?\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":"440912","messageId":"211111.86k0hevjhs.gmgdl@evledraar.gmail.com","threadId":"56404","inReplyTo":"CAFQ2z_NASfhdHdZrY3gK29LRK_8Guj0LZ=GgCr84k7XX2L+Dow@mail.gmail.com","subject":"Re: [PATCH v3 0/7] Gets rid of \"if reflog exists, append to it regardless of config settings\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-11T14:38:56Z","receivedAt":"2021-11-11T14:45:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 11 2021, Han-Wen Nienhuys wrote:\n\n> On Thu, Oct 7, 2021 at 7:38 PM Junio C Hamano wrote:\n>> > Not really. I think the comment on the RFC still stands, and I do\n>> > not recall seeing a response to the point.\n>> >\n>> > One potential harm this change will bring to us is what happens to\n>> > people who disable core.logAllRefUpdates manually after using the\n>> > repository for a while. Their @{4} will point at the same commit no\n>> > matter how many operations are done on the current branch after they\n>> > do so. I wouldn't mind if \"git reflog disable\" command is given to\n>> > the users prominently and core.logAllRefUpdates becomes a mere\n>> > implementation detail nobody has to care about---in such a world, we\n>> > could set the configuration and drop the existing reflog records at\n>> > the same time and nobody will be hurt.\n>\n> A git 'reflog disable' command would address your concerns, but it is\n> a destructive operation, so the cure might be worse than the solution.\n>\n>> IIRC, the only reason why reftable implementation may want to change\n>> the behaviour we have to avoid getting blamed for breaking is\n>> because it cannot implement \"a reflog exists, and we need to record\n>> further ref movements by appending to it, no matter what the\n>> configuration says\" when the existing reflog is empty, because its\n>> data structure lacks support for expressing \"exists but empty\".\n>>\n>> I think the behaviour change described in the title of this message\n>> can be limited in the scope to hurt users a lot less, and can still\n>> satisfy the goal of helping reftable not getting blamed for\n>> breakage, perhaps by making the behaviour for an empty but existing\n>> reflog unspecified or implementation defined per backend.\n>\n> If we accept implementation-dependent features, we could just leave\n> the whole feature as is. I had expected more breakage, but there is\n> only one test case in t1400 that needs addressing. If the test\n> coverage reflects the popularity of the feature, it should be fine to\n> leave this divergence in, and mark the test with REFFILES.\n>\n> The commits prior to the RFC should be OK for committing. In\n> particular, there is a bugfix for the show-branch command. Should I\n> resend those separately?\n\nI've got some follow-up patches to what's sitting in \"next\" already that\nhoist some reffiles-specific stuff into builtin/reflog.c, I haven't\ntested but I expect that the behavior change is silent now in the\nreftable backend, i.e. it doesn't implement progress/verbose the same\nway, presumably.\n\nBetween that and 5ac15ad2509 (reflog tests: add --updateref tests,\n2021-10-16) & 52106430dc8 (refs/files: remove \"name exist?\" check in\nlock_ref_oid_basic(), 2021-10-16) I wouldn't put too much faith in those\nreflog tests.\n\nNone of that should be a blocker for your series landing, just say'n. I\ndon't trust those tests.\n\nIMO the only meaningful way to be confident in testing these sorts of\nthings with reftable is more of the chaos monkey approach of the\nGIT_TEST_* modes, i.e. we now have a WIP mode to do that for reftable\nthat has known breakages.\n\nWe could similarly instrument the test suite to do \"git reflog expire\"\nfor each ref at the end of tests, a bunch of things would break, but we\ncould log the complete -V run and see if what breaks is different under\nthe two backends.\n\nI've got some WIP patches to add a similar chaos mode using \"git gc\n--auto\", and it turned up some interesting stuff. It's what I used\ninitially to test what's now landed in ae35e16cd43 (reflog expire: don't\nlock reflogs using previously seen OID, 2021-08-23).\n"}]}