Re: [PATCH v2 2/3] refs: add option core.logAllRefUpdates = always
From: Junio C Hamano <hidden>
Date: 2017-01-26 23:41:17
cornelius.weig@tngtech.com writes:
From: Cornelius Weig <redacted> When core.logallrefupdates is true, we only create a new reflog for refs that are under certain well-known hierarchies. The reason is that we know that some hierarchies (like refs/tags) do not typically change, and
s/do not typically/are not meant to/;
that unknown hierarchies might not want reflogs at all (e.g., a hypothetical refs/foo might be meant to change often and drop old history immediately). However, sometimes it is useful to override this decision and simply log for all refs, because the safety and audit trail is more important than the performance implications of keeping the log around. This patch introduces a new "always" mode for the core.logallrefupdates option which will log updates to everything under refs/, regardless where in the hierarchy it is (we still will not log things like ORIG_HEAD and FETCH_HEAD, which are known to be transient).
OK.
quoted hunk
diff --git a/Documentation/config.txt b/Documentation/config.txt index 3cd8030..2117616 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt@@ -522,6 +522,8 @@ core.logAllRefUpdates:: refs/heads/), remote refs (i.e. under refs/remotes/), `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
Ahh, the answer to my question on 1/3 is "no, the commit that the patch was taken out of was already wrong, still having the old line in front of its rewrite".
note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`. + If it is set to `always`, then a missing reflog is automatically + created for any ref under `refs/`. +
OK.
quoted hunk
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index 5055a96..2ac25a9 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt@@ -150,7 +150,8 @@ This option is only applicable when listing tags without annotation lines. 'strip' removes both whitespace and commentary. --create-reflog:: - Create a reflog for the tag. + Create a reflog for the tag. To globally enable reflogs for tags, see + `core.logAllRefUpdates` in linkgit:git-config[1].
OK.
quoted hunk
diff --git a/builtin/checkout.c b/builtin/checkout.c index bfe685c..1db0b44 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c@@ -612,7 +612,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts, const char *old_desc, *reflog_msg; if (opts->new_branch) { if (opts->new_orphan_branch) { - if (opts->new_branch_log && !log_all_ref_updates) { + if (opts->new_branch_log && should_autocreate_reflog("refs/heads/")) {
This is inviting a maintenance nightmare. The helper function is
defined to take the final refname, not a leading directory name.
That is why you named the parameter "refname" in your patch like
this:
--- a/refs.h
+++ b/refs.h
@@ -64,6 +64,8 @@ int read_ref(const char *refname, unsigned char *sha1);
int ref_exists(const char *refname);
+int should_autocreate_reflog(const char *refname);
+
int is_branch(const char *refname);
The callers are not supposed to know that its current implementation
happens to only use the leading prefix. When the definition of this
helper function is changed (e.g. imagine a future where this
"log.allrefupdate" is further enhanced to take glob patterns to
match the refname against), this may break and nobody would notice
for a few weeks, and we will get a regression report after a release
is made.
Don't we have the refname for the branch already in this codepath?
quoted hunk
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index d4fb977..b9084ca 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh@@ -93,6 +93,42 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' ' git reflog exists $outside ' +test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' ' + test_config core.logAllRefUpdates true && + test_when_finished "git update-ref -d $outside" && + git update-ref $outside $A && + git rev-parse $A >expect && + git rev-parse $outside >actual && + test_cmp expect actual && + test_must_fail git reflog exists $outside +' + +test_expect_success 'core.logAllRefUpdates=always creates reflog by default' ' + test_config core.logAllRefUpdates always && + test_when_finished "git update-ref -d $outside" && + git update-ref $outside $A && + git rev-parse $A >expect && + git rev-parse $outside >actual && + test_cmp expect actual && + git reflog exists $outside +'
You might want to add two tests for your original motivation, i.e. test_config core.logAllRefUpdates always && git tag a-tag && git reflog exists refs/tags/a-tag and the other one that does not give reflog for a tag. Other than that, looks good to me.