[PATCH] tag: add tag.createReflog option

Subsystems: documentation, the rest

STALE3475d

13 messages, 3 authors, 2017-01-26 · open the first message on its own page

[PATCH] tag: add tag.createReflog option

From: <hidden>
Date: 2017-01-25 00:19:43 

From: Cornelius Weig <redacted>

Git does not create a history for tags, in contrast to common
expectation to simply version everything. This can be changed by using
the `--create-reflog` flag when creating the tag. However, a config
option to enable this behavior by default is missing.

This commit adds the configuration variable `tag.createReflog` which
enables reflogs for new tags by default.

Signed-off-by: Cornelius Weig <redacted>
---
 Documentation/config.txt  |  5 +++++
 Documentation/git-tag.txt |  8 +++++---
 builtin/tag.c             |  6 +++++-
 t/t7004-tag.sh            | 14 ++++++++++++++
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4c..9e5f6f6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2945,6 +2945,11 @@ submodule.alternateErrorStrategy
 	as computed via `submodule.alternateLocation`. Possible values are
 	`ignore`, `info`, `die`. Default is `die`.
 
+tag.createReflog::
+	A boolean to specify whether newly created tags should have a reflog.
+	If `--[no-]create-reflog` is specified on the command line, it takes
+	precedence. Defaults to `false`.
+
 tag.forceSignAnnotated::
 	A boolean to specify whether annotated tags created should be GPG signed.
 	If `--annotate` is specified on the command line, it takes
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5055a96..f2ed370 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
-	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
+	[--column[=<options>] | --no-column] [--[no-]create-reflog] [--sort=<key>]
 	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
 'git tag' -v <tagname>...
 
@@ -149,8 +149,10 @@ This option is only applicable when listing tags without annotation lines.
 	all, 'whitespace' removes just leading/trailing whitespace lines and
 	'strip' removes both whitespace and commentary.
 
---create-reflog::
-	Create a reflog for the tag.
+--[no-]create-reflog::
+	Force to create a reflog for the tag, or no reflog if `--no-create-reflog`
+	is used. Unless the `tag.createReflog` config variable is set to true, no
+	reflog is created by default. See linkgit:git-config[1].
 
 <tagname>::
 	The name of the tag to create, delete, or describe.
diff --git a/builtin/tag.c b/builtin/tag.c
index 73df728..1f13e4d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,6 +30,7 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 static int force_sign_annotate;
+static int create_reflog;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -165,6 +166,10 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 		force_sign_annotate = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "tag.createreflog")) {
+		create_reflog = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "tag", &colopts);
@@ -325,7 +330,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
-	int create_reflog = 0;
 	int annotate = 0, force = 0;
 	int cmdmode = 0, create_tag_object = 0;
 	const char *msgfile = NULL, *keyid = NULL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 1cfa8a2..67b39ec 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -90,6 +90,20 @@ test_expect_success '--create-reflog does not create reflog on failure' '
 	test_must_fail git reflog exists refs/tags/mytag
 '
 
+test_expect_success 'option tag.createreflog creates reflog by default' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git config tag.createReflog true &&
+	git tag tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog
+'
+
+test_expect_success 'option tag.createreflog overridden by command line' '
+	test_when_finished "git tag -d tag_without_reflog" &&
+	git config tag.createReflog true &&
+	git tag --no-create-reflog tag_without_reflog &&
+	test_must_fail git reflog exists refs/tags/tag_without_reflog
+'
+
 test_expect_success 'listing all tags if one exists should succeed' '
 	git tag -l &&
 	git tag
-- 
2.10.2

Re: [PATCH] tag: add tag.createReflog option

From: Pranit Bauva <hidden>
Date: 2017-01-25 05:06:31 

Hey Cornelius,

On Wed, Jan 25, 2017 at 5:49 AM,  [off-list ref] wrote:
From: Cornelius Weig <redacted>

Git does not create a history for tags, in contrast to common
expectation to simply version everything. This can be changed by using
the `--create-reflog` flag when creating the tag. However, a config
option to enable this behavior by default is missing.

This commit adds the configuration variable `tag.createReflog` which
enables reflogs for new tags by default.

Signed-off-by: Cornelius Weig <redacted>
You have also added the option --no-create-reflog so would it be worth
to mention it in the commit message?
quoted hunk
---
 Documentation/config.txt  |  5 +++++
 Documentation/git-tag.txt |  8 +++++---
 builtin/tag.c             |  6 +++++-
 t/t7004-tag.sh            | 14 ++++++++++++++
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4c..9e5f6f6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2945,6 +2945,11 @@ submodule.alternateErrorStrategy
        as computed via `submodule.alternateLocation`. Possible values are
        `ignore`, `info`, `die`. Default is `die`.

+tag.createReflog::
+       A boolean to specify whether newly created tags should have a reflog.
+       If `--[no-]create-reflog` is specified on the command line, it takes
+       precedence. Defaults to `false`.
This follows the convention, good! :)
quoted hunk
 tag.forceSignAnnotated::
        A boolean to specify whether annotated tags created should be GPG signed.
        If `--annotate` is specified on the command line, it takes
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5055a96..f2ed370 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,7 @@ SYNOPSIS
        <tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
-       [--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
+       [--column[=<options>] | --no-column] [--[no-]create-reflog] [--sort=<key>]
        [--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
 'git tag' -v <tagname>...

@@ -149,8 +149,10 @@ This option is only applicable when listing tags without annotation lines.
        all, 'whitespace' removes just leading/trailing whitespace lines and
        'strip' removes both whitespace and commentary.

---create-reflog::
-       Create a reflog for the tag.
+--[no-]create-reflog::
+       Force to create a reflog for the tag, or no reflog if `--no-create-reflog`
+       is used. Unless the `tag.createReflog` config variable is set to true, no
+       reflog is created by default. See linkgit:git-config[1].

 <tagname>::
        The name of the tag to create, delete, or describe.
diff --git a/builtin/tag.c b/builtin/tag.c
index 73df728..1f13e4d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,6 +30,7 @@ static const char * const git_tag_usage[] = {

 static unsigned int colopts;
 static int force_sign_annotate;
+static int create_reflog;

 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -165,6 +166,10 @@ static int git_tag_config(const char *var, const char *value, void *cb)
                force_sign_annotate = git_config_bool(var, value);
                return 0;
        }
+       if (!strcmp(var, "tag.createreflog")) {
+               create_reflog = git_config_bool(var, value);
+               return 0;
+       }

        if (starts_with(var, "column."))
                return git_column_config(var, value, "tag", &colopts);
@@ -325,7 +330,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
        const char *object_ref, *tag;
        struct create_tag_options opt;
        char *cleanup_arg = NULL;
-       int create_reflog = 0;
        int annotate = 0, force = 0;
        int cmdmode = 0, create_tag_object = 0;
        const char *msgfile = NULL, *keyid = NULL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 1cfa8a2..67b39ec 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -90,6 +90,20 @@ test_expect_success '--create-reflog does not create reflog on failure' '
        test_must_fail git reflog exists refs/tags/mytag
 '

+test_expect_success 'option tag.createreflog creates reflog by default' '
+       test_when_finished "git tag -d tag_with_reflog" &&
+       git config tag.createReflog true &&
+       git tag tag_with_reflog &&
+       git reflog exists refs/tags/tag_with_reflog
+'
+
+test_expect_success 'option tag.createreflog overridden by command line' '
+       test_when_finished "git tag -d tag_without_reflog" &&
+       git config tag.createReflog true &&
+       git tag --no-create-reflog tag_without_reflog &&
+       test_must_fail git reflog exists refs/tags/tag_without_reflog
+'
+
 test_expect_success 'listing all tags if one exists should succeed' '
        git tag -l &&
        git tag
--
2.10.2

Re: [PATCH] tag: add tag.createReflog option

From: Jeff King <hidden>
Date: 2017-01-25 18:01:01 

On Wed, Jan 25, 2017 at 01:19:06AM +0100, cornelius.weig@tngtech.com wrote:

From: Cornelius Weig <redacted>

Git does not create a history for tags, in contrast to common
expectation to simply version everything. This can be changed by using
the `--create-reflog` flag when creating the tag. However, a config
option to enable this behavior by default is missing.
Hmm, I didn't even know we had "tag --create-reflog". Looks like it was
added by 144c76fa39 (update-ref and tag: add --create-reflog arg,
2015-07-21).

IMHO it is a mistake. The "update-ref --create-reflog" variant makes
sense to me as a plumbing operation. But are there end users who want to
create a reflog for just _one_ tag?

As your patch shows, the more likely variant is "I want reflogs for all
tags". But that raises two questions with your patch:

  - yours isn't "reflogs for all tags". It's "reflogs for tags I created
    with git-tag". What about other operations that create tags, like
    fetching (or even just a script that uses update-ref under the
    hood).

    IOW, instead of tag.createReflog, should this be tweaing
    core.logallrefupdates to have a mode that includes tags?

  - Is that the end of it, or is the desire really "I want reflogs for
    _everything_"? That seems like a sane thing to want.

    If so, then the update to core.logallrefupdates should turn it into
    a tri-state:

      - false; no reflogs

      - true; reflogs for branches, remotes, notes, as now

      - always; reflogs for all refs under "refs/"

I made a lot of suppositions about your desires there, so maybe you
really do want just tag.createReflog. But "core.logallrefupdates =
always" sounds a lot more useful to me.

-Peff

Re: [PATCH] tag: add tag.createReflog option

From: Cornelius Weig <hidden>
Date: 2017-01-25 21:21:57 

On 01/25/2017 07:00 PM, Jeff King wrote:

  - Is that the end of it, or is the desire really "I want reflogs for
    _everything_"? That seems like a sane thing to want.

    If so, then the update to core.logallrefupdates should turn it into
    a tri-state:

      - false; no reflogs

      - true; reflogs for branches, remotes, notes, as now

      - always; reflogs for all refs under "refs/"
I think you nailed it. This is much more useful than what I suggested.
I'll see if I can code it up.

Re: [PATCH] tag: add tag.createReflog option

From: Jeff King <hidden>
Date: 2017-01-25 21:33:41 

On Wed, Jan 25, 2017 at 10:21:48PM +0100, Cornelius Weig wrote:

On 01/25/2017 07:00 PM, Jeff King wrote:
quoted
  - Is that the end of it, or is the desire really "I want reflogs for
    _everything_"? That seems like a sane thing to want.

    If so, then the update to core.logallrefupdates should turn it into
    a tri-state:

      - false; no reflogs

      - true; reflogs for branches, remotes, notes, as now

      - always; reflogs for all refs under "refs/"
I think you nailed it. This is much more useful than what I suggested.
I'll see if I can code it up.
I cheated a little. I actually wrote the "always" patch 3 years ago as
part of another thing I was working on. But in the end I didn't need it,
and never submitted it.

The patch is below for reference. I have no idea whether it even applies
now, let alone runs and does the right thing. But perhaps you can
salvage bits of it (but feel free to ignore it if it makes things
harder).

-- >8 --
From: Jeff King <redacted>
Date: Mon, 15 Apr 2013 23:31:05 -0400
Subject: [PATCH] teach core.logallrefupdates an "always" mode

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
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).

Signed-off-by: Jeff King <redacted>
---
 Documentation/config.txt |  8 +++++---
 branch.c                 |  2 +-
 builtin/checkout.c       |  2 +-
 builtin/init-db.c        |  2 +-
 cache.h                  |  9 ++++++++-
 config.c                 |  7 ++++++-
 environment.c            |  2 +-
 refs.c                   | 23 +++++++++++++++++------
 t/t1400-update-ref.sh    | 32 ++++++++++++++++++++++++++++++++
 9 files changed, 72 insertions(+), 15 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e37ba94a72..cb72e559ec 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -390,10 +390,12 @@ core.logAllRefUpdates::
 	"$GIT_DIR/logs/<ref>", by appending the new and old
 	SHA1, the date/time and the reason of the update, but
 	only when the file exists.  If this configuration
-	variable is set to true, missing "$GIT_DIR/logs/<ref>"
+	variable is set to `true`, a missing "$GIT_DIR/logs/<ref>"
 	file is automatically created for branch heads (i.e. under
-	refs/heads/), remote refs (i.e. under refs/remotes/),
-	note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+	`refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+	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/`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
diff --git a/branch.c b/branch.c
index 2bef1e7e71..c11880b181 100644
--- a/branch.c
+++ b/branch.c
@@ -259,7 +259,7 @@ void create_branch(const char *head,
 	}
 
 	if (reflog)
-		log_all_ref_updates = 1;
+		log_all_ref_updates = LOG_REFS_NORMAL;
 
 	if (forcing)
 		snprintf(msg, sizeof msg, "branch: Reset to %s",
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a9c1b5a95f..00e231d83b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -564,7 +564,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
 
 				temp = log_all_ref_updates;
-				log_all_ref_updates = 1;
+				log_all_ref_updates = LOG_REFS_NORMAL;
 				if (log_ref_setup(ref_name, log_file, sizeof(log_file))) {
 					fprintf(stderr, _("Can not do reflog for '%s'\n"),
 					    opts->new_orphan_branch);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 78aa3872dd..0ebad0b37d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -264,7 +264,7 @@ static int create_default_files(const char *template_path)
 		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
-		if (log_all_ref_updates == -1)
+		if (log_all_ref_updates == LOG_REFS_UNSET)
 		    git_config_set("core.logallrefupdates", "true");
 		if (prefixcmp(git_dir, work_tree) ||
 		    strcmp(git_dir + strlen(work_tree), "/.git")) {
diff --git a/cache.h b/cache.h
index 2b192d24ac..d2bfabc67f 100644
--- a/cache.h
+++ b/cache.h
@@ -536,7 +536,6 @@ extern int minimum_abbrev, default_abbrev;
 extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
-extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
@@ -556,6 +555,14 @@ extern int core_preload_index;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
 
+enum log_refs_config {
+	LOG_REFS_UNSET = -1,
+	LOG_REFS_NONE = 0,
+	LOG_REFS_NORMAL, /* see should_create_reflog for rules */
+	LOG_REFS_ALWAYS
+};
+extern enum log_refs_config log_all_ref_updates;
+
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,
diff --git a/config.c b/config.c
index b5696354fa..ffb892c0a0 100644
--- a/config.c
+++ b/config.c
@@ -601,7 +601,12 @@ static int git_default_core_config(const char *var, const char *value)
 	}
 
 	if (!strcmp(var, "core.logallrefupdates")) {
-		log_all_ref_updates = git_config_bool(var, value);
+		if (value && !strcasecmp(value, "always"))
+			log_all_ref_updates = LOG_REFS_ALWAYS;
+		else if (git_config_bool(var, value))
+			log_all_ref_updates = LOG_REFS_NORMAL;
+		else
+			log_all_ref_updates = LOG_REFS_NONE;
 		return 0;
 	}
 
diff --git a/environment.c b/environment.c
index 85edd7f95a..1867d31d75 100644
--- a/environment.c
+++ b/environment.c
@@ -19,7 +19,7 @@ int ignore_case;
 int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
-int log_all_ref_updates = -1; /* unspecified */
+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
 int warn_ambiguous_refs = 1;
 int repository_format_version;
 const char *git_commit_encoding;
diff --git a/refs.c b/refs.c
index 541fec2065..3cd203ef87 100644
--- a/refs.c
+++ b/refs.c
@@ -1896,7 +1896,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	lock->force_write = 1;
 	flag = log_all_ref_updates;
-	log_all_ref_updates = 0;
+	log_all_ref_updates = LOG_REFS_NONE;
 	if (write_ref_sha1(lock, orig_sha1, NULL))
 		error("unable to write current sha1 into %s", oldrefname);
 	log_all_ref_updates = flag;
@@ -1965,16 +1965,27 @@ static int copy_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
+int should_create_reflog(const char *refname)
+{
+	switch (log_all_ref_updates) {
+	case LOG_REFS_ALWAYS:
+		return 1;
+	case LOG_REFS_NORMAL:
+		return !prefixcmp(refname, "refs/heads/") ||
+		       !prefixcmp(refname, "refs/remotes/") ||
+		       !prefixcmp(refname, "refs/notes/") ||
+		       !strcmp(refname, "HEAD");
+	default:
+		return 0;
+	}
+}
+
 int log_ref_setup(const char *refname, char *logfile, int bufsize)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
 
 	git_snpath(logfile, bufsize, "logs/%s", refname);
-	if (log_all_ref_updates &&
-	    (!prefixcmp(refname, "refs/heads/") ||
-	     !prefixcmp(refname, "refs/remotes/") ||
-	     !prefixcmp(refname, "refs/notes/") ||
-	     !strcmp(refname, "HEAD"))) {
+	if (should_create_reflog(refname)) {
 		if (safe_create_leading_directories(logfile) < 0)
 			return error("unable to create directory for %s",
 				     logfile);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0bbf..f60196d294 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,36 @@ test_expect_success \
 	'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
 	'test OTHER = $(git cat-file blob "master@{2005-05-26 23:42}:F")'
 
+test_expect_success 'core.logAllRefUpdates=true does not log refs/foo/' '
+	test_config core.logAllRefUpdates true &&
+	test_commit log-true &&
+	git update-ref -m reflog-message refs/heads/logme HEAD &&
+	git update-ref -m reflog-message refs/foo/logme HEAD &&
+	{
+		echo "refs/heads/logme@{0} reflog-message"
+	} >expect &&
+	{
+		git log -g -1 --format="%gD %gs" refs/heads/logme &&
+		git log -g -1 --format="%gD %gs" refs/foo/logme
+	} >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'core.logAllRefUpdates=always logs refs/foo/' '
+	test_config core.logAllRefUpdates always &&
+	test_commit log-always &&
+	git update-ref -m reflog-message refs/heads/logme HEAD &&
+	git update-ref -m reflog-message refs/foo/logme HEAD &&
+	{
+		echo "refs/heads/logme@{0} reflog-message"
+		echo "refs/foo/logme@{0} reflog-message"
+	} >expect &&
+	{
+		git log -g -1 --format="%gD %gs" refs/heads/logme &&
+		git log -g -1 --format="%gD %gs" refs/foo/logme
+	} >actual &&
+	test_cmp expect actual
+'
+
+
 test_done
-- 
2.11.0.840.gd37c5973a

[PATCH] refs: add option core.logAllRefUpdates = always

From: <hidden>
Date: 2017-01-26 01:17:32 

Hi peff,

 you made it easy for me. Most of your patch still applied, only the tests
didn't quite fit. Maybe you can have a look if I've overlooked something, since
you know the changes best?

Thanks for supporting this with your patch!

[PATCH] refs: add option core.logAllRefUpdates = always

From: <hidden>
Date: 2017-01-26 01:17:35 

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
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).

Based-on-patch-by: Jeff King [off-list ref]
Signed-off-by: Cornelius Weig <redacted>
---
 Documentation/config.txt  |  7 +++++--
 Documentation/git-tag.txt |  3 ++-
 branch.c                  |  2 +-
 builtin/init-db.c         |  2 +-
 cache.h                   |  9 +++++++-
 config.c                  |  7 ++++++-
 environment.c             |  2 +-
 refs.c                    | 15 +++++++++-----
 refs/files-backend.c      |  6 +++---
 t/t1400-update-ref.sh     | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4c..2117616 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -517,10 +517,13 @@ core.logAllRefUpdates::
 	"`$GIT_DIR/logs/<ref>`", by appending the new and old
 	SHA-1, the date/time and the reason of the update, but
 	only when the file exists.  If this configuration
-	variable is set to true, missing "`$GIT_DIR/logs/<ref>`"
+	variable is set to `true`, missing "`$GIT_DIR/logs/<ref>`"
 	file is automatically created for branch heads (i.e. under
 	refs/heads/), remote refs (i.e. under refs/remotes/),
-	note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+	`refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+	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/`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
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].
 
 <tagname>::
 	The name of the tag to create, delete, or describe.
diff --git a/branch.c b/branch.c
index c431cbf..b955d4f 100644
--- a/branch.c
+++ b/branch.c
@@ -298,7 +298,7 @@ void create_branch(const char *name, const char *start_name,
 			 start_name);
 
 	if (reflog)
-		log_all_ref_updates = 1;
+		log_all_ref_updates = LOG_REFS_NORMAL;
 
 	if (!dont_change_ref) {
 		struct ref_transaction *transaction;
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 76d68fa..1d4d6a0 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,
 		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
-		if (log_all_ref_updates == -1)
+		if (log_all_ref_updates == LOG_REFS_UNSET)
 			git_config_set("core.logallrefupdates", "true");
 		if (needs_work_tree_config(original_git_dir, work_tree))
 			git_config_set("core.worktree", work_tree);
diff --git a/cache.h b/cache.h
index 00a029a..96eeaaf 100644
--- a/cache.h
+++ b/cache.h
@@ -660,7 +660,6 @@ extern int minimum_abbrev, default_abbrev;
 extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
-extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
 extern const char *apply_default_whitespace;
@@ -728,6 +727,14 @@ enum hide_dotfiles_type {
 };
 extern enum hide_dotfiles_type hide_dotfiles;
 
+enum log_refs_config {
+	LOG_REFS_UNSET = -1,
+	LOG_REFS_NONE = 0,
+	LOG_REFS_NORMAL,
+	LOG_REFS_ALWAYS
+};
+extern enum log_refs_config log_all_ref_updates;
+
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,
diff --git a/config.c b/config.c
index b680f79..c6b874a 100644
--- a/config.c
+++ b/config.c
@@ -826,7 +826,12 @@ static int git_default_core_config(const char *var, const char *value)
 	}
 
 	if (!strcmp(var, "core.logallrefupdates")) {
-		log_all_ref_updates = git_config_bool(var, value);
+		if (value && !strcasecmp(value, "always"))
+			log_all_ref_updates = LOG_REFS_ALWAYS;
+		else if (git_config_bool(var, value))
+			log_all_ref_updates = LOG_REFS_NORMAL;
+		else
+			log_all_ref_updates = LOG_REFS_NONE;
 		return 0;
 	}
 
diff --git a/environment.c b/environment.c
index 8a83101..c07fb17 100644
--- a/environment.c
+++ b/environment.c
@@ -21,7 +21,6 @@ int ignore_case;
 int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
-int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
 int ref_paranoia = -1;
@@ -64,6 +63,7 @@ int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 unsigned long pack_size_limit_cfg;
 enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
diff --git a/refs.c b/refs.c
index 9bd0bc1..cd36b64 100644
--- a/refs.c
+++ b/refs.c
@@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)
 
 int should_autocreate_reflog(const char *refname)
 {
-	if (!log_all_ref_updates)
+	switch (log_all_ref_updates) {
+	case LOG_REFS_ALWAYS:
+		return 1;
+	case LOG_REFS_NORMAL:
+		return starts_with(refname, "refs/heads/") ||
+			starts_with(refname, "refs/remotes/") ||
+			starts_with(refname, "refs/notes/") ||
+			!strcmp(refname, "HEAD");
+	default:
 		return 0;
-	return starts_with(refname, "refs/heads/") ||
-		starts_with(refname, "refs/remotes/") ||
-		starts_with(refname, "refs/notes/") ||
-		!strcmp(refname, "HEAD");
+	}
 }
 
 int is_branch(const char *refname)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f902393..14b17a6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2682,7 +2682,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 	}
 
 	flag = log_all_ref_updates;
-	log_all_ref_updates = 0;
+	log_all_ref_updates = LOG_REFS_NONE;
 	if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
 	    commit_ref_update(refs, lock, orig_sha1, NULL, &err)) {
 		error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
@@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
 
-	if (log_all_ref_updates < 0)
-		log_all_ref_updates = !is_bare_repository();
+	if (log_all_ref_updates == LOG_REFS_UNSET)
+		log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
 
 	result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d4fb977..a920559 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -93,6 +93,36 @@ 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
+'
+
+test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '
+	test_config core.logAllRefUpdates true &&
+	test_when_finished "git update-ref -d $outside" &&
+	git update-ref --no-create-reflog $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 \
 	"create $m (by HEAD)" \
 	"git update-ref HEAD $A &&
@@ -501,6 +531,7 @@ test_expect_success 'stdin does not create reflogs by default' '
 '
 
 test_expect_success 'stdin creates reflogs with --create-reflog' '
+	test_when_finished "git update-ref -d $outside" &&
 	echo "create $outside $m" >stdin &&
 	git update-ref --create-reflog --stdin <stdin &&
 	git rev-parse $m >expect &&
@@ -509,6 +540,28 @@ test_expect_success 'stdin creates reflogs with --create-reflog' '
 	git reflog exists $outside
 '
 
+test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '
+	test_config core.logAllRefUpdates true &&
+	test_when_finished "git update-ref -d $outside" &&
+	echo "create $outside $m" >stdin &&
+	git update-ref --stdin <stdin &&
+	git rev-parse $m >expect &&
+	git rev-parse $outside >actual &&
+	test_cmp expect actual &&
+	test_must_fail git reflog exists $outside
+'
+
+test_expect_success 'stdin creates reflog when core.logAllRefUpdates=always' '
+	test_config core.logAllRefUpdates always &&
+	test_when_finished "git update-ref -d $outside" &&
+	echo "create $outside $m" >stdin &&
+	git update-ref --stdin <stdin &&
+	git rev-parse $m >expect &&
+	git rev-parse $outside >actual &&
+	test_cmp expect actual &&
+	git reflog exists $outside
+'
+
 test_expect_success 'stdin succeeds with quoted argument' '
 	git update-ref -d $a &&
 	echo "create $a \"$m\"" >stdin &&
-- 
2.10.2

Re: [PATCH] refs: add option core.logAllRefUpdates = always

From: Jeff King <hidden>
Date: 2017-01-26 03:35:55 

On Thu, Jan 26, 2017 at 02:16:54AM +0100, cornelius.weig@tngtech.com wrote:

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
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).
I tried to read this patch with fresh eyes. But given the history, you
may take my review with a grain of salt. :)

Overall it looks OK to me. A few comments below.

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).
I don't think my original had tests for this, but it might be worth
adding a test for this last bit (i.e., that an update of ORIG_HEAD does
not write a reflog when logallrefupdates is set to "always").
quoted hunk
diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4c..2117616 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -517,10 +517,13 @@ core.logAllRefUpdates::
 	"`$GIT_DIR/logs/<ref>`", by appending the new and old
 	SHA-1, the date/time and the reason of the update, but
 	only when the file exists.  If this configuration
-	variable is set to true, missing "`$GIT_DIR/logs/<ref>`"
+	variable is set to `true`, missing "`$GIT_DIR/logs/<ref>`"
 	file is automatically created for branch heads (i.e. under
 	refs/heads/), remote refs (i.e. under refs/remotes/),
-	note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+	`refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+	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/`.
I guess the backtick fixups came from my original. It might be easier to
see the change if they were pulled into their own patch, but it's
probably not that big a deal.
quoted hunk
--- 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].
This documentation tweak makes sense to me.
quoted hunk
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 76d68fa..1d4d6a0 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,
 		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
-		if (log_all_ref_updates == -1)
+		if (log_all_ref_updates == LOG_REFS_UNSET)
 			git_config_set("core.logallrefupdates", "true");
 		if (needs_work_tree_config(original_git_dir, work_tree))
 			git_config_set("core.worktree", work_tree);
I expected that this hunk would need tweaked due to refactoring around
init-db that happened earlier this year. But it seems fine.
quoted hunk
diff --git a/refs.c b/refs.c
index 9bd0bc1..cd36b64 100644
--- a/refs.c
+++ b/refs.c
@@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)
 
 int should_autocreate_reflog(const char *refname)
 {
-	if (!log_all_ref_updates)
+	switch (log_all_ref_updates) {
+	case LOG_REFS_ALWAYS:
+		return 1;
+	case LOG_REFS_NORMAL:
+		return starts_with(refname, "refs/heads/") ||
+			starts_with(refname, "refs/remotes/") ||
+			starts_with(refname, "refs/notes/") ||
+			!strcmp(refname, "HEAD");
+	default:
 		return 0;
-	return starts_with(refname, "refs/heads/") ||
-		starts_with(refname, "refs/remotes/") ||
-		starts_with(refname, "refs/notes/") ||
-		!strcmp(refname, "HEAD");
+	}
 }
And this function got broken out already by David in an earlier patch.
Looks good.
quoted hunk
@@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
 
-	if (log_all_ref_updates < 0)
-		log_all_ref_updates = !is_bare_repository();
+	if (log_all_ref_updates == LOG_REFS_UNSET)
+		log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
This hunk is new, I think. The enum values are set in such a way that
the original code would have continued to work, but I think using the
symbolic names is an improvement.

I assume you grepped for log_all_ref_updates to find this. I see only
one spot that now doesn't use the symbolic names. In builtin/checkout.c,
update_refs_for_switch() checks:

  if (opts->new_branch_log && !log_all_ref_updates)

That looks buggy, as it would treat LOG_REFS_NORMAL and LOG_REFS_UNSET
the same, and I do not see us resolving the UNSET case to a true/false
value. But I don't think the bug is new in your patch; the default value
was "-1" already.

I doubt it can be triggered in practice, because either:

  - the config value is set in the config file, and we pick up that
    value, whether it's "true" or "false"

  - it's unset, in which case our default would be to enable reflogs in
    a non-bare repo. And since git-checkout would refuse to run in a
    bare repo, we must be non-bare, and thus enabling reflogs does the
    right thing.

But it works quite by accident. I wonder if we should this
"is_bare_repository" check into a function that can be called instead of
accessing log_all_ref_updates() directly.
quoted hunk
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -93,6 +93,36 @@ 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
+'
Adding the tests to the existing --create-reflog tests is a good choice.

+test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '
This test title is _really_ long, and will wrap in the output on
reasonable-sized terminals. Maybe '--no-create-reflog overrides
core.logAllRefUpdates=always' would be shorter?

 test_expect_success 'stdin creates reflogs with --create-reflog' '
+	test_when_finished "git update-ref -d $outside" &&
 	echo "create $outside $m" >stdin &&
 	git update-ref --create-reflog --stdin <stdin &&
 	git rev-parse $m >expect &&
Adding missing cleanup. Good.

+test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '
I don't mind these extra stdin tests, but IMHO they are just redundant.
The "--stdin --create-reflog" one makes sure the option is propagated
down via the --stdin machinery. But we know the config option is handled
at a low level anyway.

I guess it depends on how black-box we want the testing to be. It just
seems unlikely for a regression to be found here and not in the tests
above.

-Peff

Re: [PATCH] refs: add option core.logAllRefUpdates = always

From: Cornelius Weig <hidden>
Date: 2017-01-26 14:06:52 

Hi Peff,

 thanks for your thoughts.

I tried to read this patch with fresh eyes. But given the history, you
may take my review with a grain of salt. :)
Does it mean another reviewer is needed?

I don't think my original had tests for this, but it might be worth
adding a test for this last bit (i.e., that an update of ORIG_HEAD does
not write a reflog when logallrefupdates is set to "always").
Good point. I blindly copied your commit message without thinking too
much about it.

I guess the backtick fixups came from my original. It might be easier to
see the change if they were pulled into their own patch, but it's
probably not that big a deal.
If it's best practice to break out such changes, I'll revise it.
quoted
@@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
 
-	if (log_all_ref_updates < 0)
-		log_all_ref_updates = !is_bare_repository();
+	if (log_all_ref_updates == LOG_REFS_UNSET)
+		log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
This hunk is new, I think. The enum values are set in such a way that
the original code would have continued to work, but I think using the
symbolic names is an improvement.
Yes it's new.

I assume you grepped for log_all_ref_updates to find this. I see only
one spot that now doesn't use the symbolic names. In builtin/checkout.c,
update_refs_for_switch() checks:

  if (opts->new_branch_log && !log_all_ref_updates)

That looks buggy, as it would treat LOG_REFS_NORMAL and LOG_REFS_UNSET
the same, and I do not see us resolving the UNSET case to a true/false
value. But I don't think the bug is new in your patch; the default value
was "-1" already.

I doubt it can be triggered in practice, because either:

  - the config value is set in the config file, and we pick up that
    value, whether it's "true" or "false"

  - it's unset, in which case our default would be to enable reflogs in
    a non-bare repo. And since git-checkout would refuse to run in a
    bare repo, we must be non-bare, and thus enabling reflogs does the
    right thing.
That far I can follow.

But it works quite by accident. I wonder if we should this
"is_bare_repository" check into a function that can be called instead of
accessing log_all_ref_updates() directly.
Are you saying that we should move the `!log_all_ref_updates` check into
its own function where we should also check `is_bare_repository`? I
don't see that this would win much, because as you said: checkouts in a
bare repo are forbidden anyway.

Other than that, I guess it should better read `log_all_ref_update !=
LOG_REFS_NONE` instead of `!log_all_ref_updates`.
quoted
+test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '
This test title is _really_ long, and will wrap in the output on
reasonable-sized terminals. Maybe '--no-create-reflog overrides
core.logAllRefUpdates=always' would be shorter?
Yes, I agree.
quoted
+test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '
I don't mind these extra stdin tests, but IMHO they are just redundant.
The "--stdin --create-reflog" one makes sure the option is propagated
down via the --stdin machinery. But we know the config option is handled
at a low level anyway.

I guess it depends on how black-box we want the testing to be. It just
seems unlikely for a regression to be found here and not in the tests
above.
Since these other stdin tests were around, I added this variant. But
you're right: this test breaks along with the other and doesn't add add
more safety. I'll remove it.

However, I realized that I have not written tests about ref updates in a
bare repository. Do you think it's worthwile?

Cheers,
  Cornelius

Re: [PATCH] refs: add option core.logAllRefUpdates = always

From: Jeff King <hidden>
Date: 2017-01-26 14:46:17 

On Thu, Jan 26, 2017 at 03:06:40PM +0100, Cornelius Weig wrote:
quoted
But it works quite by accident. I wonder if we should this
"is_bare_repository" check into a function that can be called instead of
accessing log_all_ref_updates() directly.
Are you saying that we should move the `!log_all_ref_updates` check into
its own function where we should also check `is_bare_repository`? I
don't see that this would win much, because as you said: checkouts in a
bare repo are forbidden anyway.
Yes, I'm suggesting making something like the should_autocreate_reflog()
function public.

I agree it is working correctly now. It's just that it's rather subtle
that it treats LOG_REFS_UNSET implicitly as LOG_REFS_NONE.

It would also possibly break if more values are added to the enum
(depending on what those values are).

However, I realized that I have not written tests about ref updates in a
bare repository. Do you think it's worthwile?
There should already be a test for logAllRefUpdates=true in a bare
repository (if there isn't, we should probably add one). Testing the
"always" case individually does not add much over testing it in a
non-bare repository. IMHO.

-Peff

[PATCH v2 1/3] config: add markup to core.logAllRefUpdates doc

From: <hidden>
Date: 2017-01-26 22:41:21 

From: Cornelius Weig <redacted>

Signed-off-by: Cornelius Weig <redacted>
---

Notes:
    As suggested, I moved the modification of the markup to its own commit.

 Documentation/config.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4c..3cd8030 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -517,10 +517,11 @@ core.logAllRefUpdates::
 	"`$GIT_DIR/logs/<ref>`", by appending the new and old
 	SHA-1, the date/time and the reason of the update, but
 	only when the file exists.  If this configuration
-	variable is set to true, missing "`$GIT_DIR/logs/<ref>`"
+	variable is set to `true`, missing "`$GIT_DIR/logs/<ref>`"
 	file is automatically created for branch heads (i.e. under
 	refs/heads/), remote refs (i.e. under refs/remotes/),
-	note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+	`refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+	note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
-- 
2.10.2

[PATCH v2 2/3] refs: add option core.logAllRefUpdates = always

From: <hidden>
Date: 2017-01-26 22:41:17 

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
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).

Based-on-patch-by: Jeff King [off-list ref]
Signed-off-by: Cornelius Weig <redacted>
Reviewed-by: Jeff King <redacted>
---

Notes:
    Changes with respect to the previous version:
    
     - add test that checks that no reflog is created for ORIG_HEAD if
       core.logAllRefUpdates=always
     - remove redundant tests that check reflog creation when update-ref is called
       with --stdin
     - make test description shorter
     - make the function should_autocreate_reflog() public and use it in
       update_refs_for_switch().
    
    The last item addresses Peff's concern that the previous version only works by
    accident and may break in the future (see
    20170126033547.7bszipvkpi2jb4ad@sigill.intra.peff.net). In particular, this
    concerns the following change:
    
    - if (opts->new_branch_log && !log_all_ref_updates) {
    + if (opts->new_branch_log && should_autocreate_reflog("refs/heads/")) {
    
    The function call to `should_autocreate_reflog()` answers exactly the question
    that the original test `!log_all_ref_updates` tried to resolve in the original
    version.

 Documentation/config.txt  |  2 ++
 Documentation/git-tag.txt |  3 ++-
 branch.c                  |  2 +-
 builtin/checkout.c        |  2 +-
 builtin/init-db.c         |  2 +-
 cache.h                   |  9 ++++++++-
 config.c                  |  7 ++++++-
 environment.c             |  2 +-
 refs.c                    | 15 ++++++++++-----
 refs.h                    |  2 ++
 refs/files-backend.c      |  6 +++---
 refs/refs-internal.h      |  2 --
 t/t1400-update-ref.sh     | 37 +++++++++++++++++++++++++++++++++++++
 13 files changed, 74 insertions(+), 17 deletions(-)

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/`),
 	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/`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
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].
 
 <tagname>::
 	The name of the tag to create, delete, or describe.
diff --git a/branch.c b/branch.c
index c431cbf..b955d4f 100644
--- a/branch.c
+++ b/branch.c
@@ -298,7 +298,7 @@ void create_branch(const char *name, const char *start_name,
 			 start_name);
 
 	if (reflog)
-		log_all_ref_updates = 1;
+		log_all_ref_updates = LOG_REFS_NORMAL;
 
 	if (!dont_change_ref) {
 		struct ref_transaction *transaction;
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/")) {
 				int ret;
 				char *refname;
 				struct strbuf err = STRBUF_INIT;
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 76d68fa..1d4d6a0 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,
 		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
-		if (log_all_ref_updates == -1)
+		if (log_all_ref_updates == LOG_REFS_UNSET)
 			git_config_set("core.logallrefupdates", "true");
 		if (needs_work_tree_config(original_git_dir, work_tree))
 			git_config_set("core.worktree", work_tree);
diff --git a/cache.h b/cache.h
index 00a029a..96eeaaf 100644
--- a/cache.h
+++ b/cache.h
@@ -660,7 +660,6 @@ extern int minimum_abbrev, default_abbrev;
 extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
-extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
 extern const char *apply_default_whitespace;
@@ -728,6 +727,14 @@ enum hide_dotfiles_type {
 };
 extern enum hide_dotfiles_type hide_dotfiles;
 
+enum log_refs_config {
+	LOG_REFS_UNSET = -1,
+	LOG_REFS_NONE = 0,
+	LOG_REFS_NORMAL,
+	LOG_REFS_ALWAYS
+};
+extern enum log_refs_config log_all_ref_updates;
+
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,
diff --git a/config.c b/config.c
index b680f79..c6b874a 100644
--- a/config.c
+++ b/config.c
@@ -826,7 +826,12 @@ static int git_default_core_config(const char *var, const char *value)
 	}
 
 	if (!strcmp(var, "core.logallrefupdates")) {
-		log_all_ref_updates = git_config_bool(var, value);
+		if (value && !strcasecmp(value, "always"))
+			log_all_ref_updates = LOG_REFS_ALWAYS;
+		else if (git_config_bool(var, value))
+			log_all_ref_updates = LOG_REFS_NORMAL;
+		else
+			log_all_ref_updates = LOG_REFS_NONE;
 		return 0;
 	}
 
diff --git a/environment.c b/environment.c
index 8a83101..c07fb17 100644
--- a/environment.c
+++ b/environment.c
@@ -21,7 +21,6 @@ int ignore_case;
 int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
-int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
 int ref_paranoia = -1;
@@ -64,6 +63,7 @@ int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 unsigned long pack_size_limit_cfg;
 enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
diff --git a/refs.c b/refs.c
index 9bd0bc1..cd36b64 100644
--- a/refs.c
+++ b/refs.c
@@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)
 
 int should_autocreate_reflog(const char *refname)
 {
-	if (!log_all_ref_updates)
+	switch (log_all_ref_updates) {
+	case LOG_REFS_ALWAYS:
+		return 1;
+	case LOG_REFS_NORMAL:
+		return starts_with(refname, "refs/heads/") ||
+			starts_with(refname, "refs/remotes/") ||
+			starts_with(refname, "refs/notes/") ||
+			!strcmp(refname, "HEAD");
+	default:
 		return 0;
-	return starts_with(refname, "refs/heads/") ||
-		starts_with(refname, "refs/remotes/") ||
-		starts_with(refname, "refs/notes/") ||
-		!strcmp(refname, "HEAD");
+	}
 }
 
 int is_branch(const char *refname)
diff --git a/refs.h b/refs.h
index 6947843..9fbff90 100644
--- 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);
 
 extern int refs_init_db(struct strbuf *err);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f902393..14b17a6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2682,7 +2682,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 	}
 
 	flag = log_all_ref_updates;
-	log_all_ref_updates = 0;
+	log_all_ref_updates = LOG_REFS_NONE;
 	if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
 	    commit_ref_update(refs, lock, orig_sha1, NULL, &err)) {
 		error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
@@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
 
-	if (log_all_ref_updates < 0)
-		log_all_ref_updates = !is_bare_repository();
+	if (log_all_ref_updates == LOG_REFS_UNSET)
+		log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
 
 	result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..25444cf 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -133,8 +133,6 @@ int verify_refname_available(const char *newname,
  */
 int copy_reflog_msg(char *buf, const char *msg);
 
-int should_autocreate_reflog(const char *refname);
-
 /**
  * Information needed for a single ref update. Set new_sha1 to the new
  * value or to null_sha1 to delete the ref. To check the old value
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
+'
+
+test_expect_success 'core.logAllRefUpdates=always creates no reflog for ORIG_HEAD' '
+	test_config core.logAllRefUpdates always &&
+	git update-ref ORIG_HEAD $A &&
+	test_must_fail git reflog exists ORIG_HEAD
+'
+
+test_expect_success '--no-create-reflog overrides core.logAllRefUpdates=always' '
+	test_config core.logAllRefUpdates true &&
+	test_when_finished "git update-ref -d $outside" &&
+	git update-ref --no-create-reflog $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 \
 	"create $m (by HEAD)" \
 	"git update-ref HEAD $A &&
@@ -501,6 +537,7 @@ test_expect_success 'stdin does not create reflogs by default' '
 '
 
 test_expect_success 'stdin creates reflogs with --create-reflog' '
+	test_when_finished "git update-ref -d $outside" &&
 	echo "create $outside $m" >stdin &&
 	git update-ref --create-reflog --stdin <stdin &&
 	git rev-parse $m >expect &&
-- 
2.10.2

[PATCH v2 3/3] update-ref: add test cases for bare repository

From: <hidden>
Date: 2017-01-26 22:41:20 

From: Cornelius Weig <redacted>

The default behavior of update-ref to create reflogs differs in
repositories with worktree and bare ones. The existing tests cover only
the behavior of repositories with worktree.

This commit adds tests that assert the correct behavior in bare
repositories for update-ref. Two cases are covered:

 - If core.logAllRefUpdates is not set, no reflogs should be created
 - If core.logAllRefUpdates is true, reflogs should be created

Signed-off-by: Cornelius Weig <redacted>
---
 t/t1400-update-ref.sh | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b9084ca..bad88c8 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -8,23 +8,33 @@ test_description='Test git update-ref and basic ref logging'
 
 Z=$_z40
 
-test_expect_success setup '
+m=refs/heads/master
+n_dir=refs/heads/gu
+n=$n_dir/fixes
+outside=refs/foo
+bare=bare-repo
 
+create_test_objects ()
+{
+	local T, sha1, prfx="$1"
 	for name in A B C D E F
 	do
 		test_tick &&
 		T=$(git write-tree) &&
 		sha1=$(echo $name | git commit-tree $T) &&
-		eval $name=$sha1
+		eval $prfx$name=$sha1
 	done
+}
 
+test_expect_success setup '
+	create_test_objects "" &&
+	mkdir $bare &&
+	cd $bare &&
+	git init --bare &&
+	create_test_objects "bare" &&
+	cd -
 '
 
-m=refs/heads/master
-n_dir=refs/heads/gu
-n=$n_dir/fixes
-outside=refs/foo
-
 test_expect_success \
 	"create $m" \
 	"git update-ref $m $A &&
@@ -93,6 +103,25 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '
 	git reflog exists $outside
 '
 
+test_expect_success 'creates no reflog in bare repository' '
+	git -C $bare update-ref $m $bareA &&
+	git -C $bare rev-parse $bareA >expect &&
+	git -C $bare rev-parse $m >actual &&
+	test_cmp expect actual &&
+	test_must_fail git -C $bare reflog exists $m
+'
+
+test_expect_success 'core.logAllRefUpdates=true creates reflog in bare repository' '
+	test_when_finished "git -C $bare config --unset core.logAllRefUpdates && \
+		rm $bare/logs/$m" &&
+	git -C $bare config core.logAllRefUpdates true &&
+	git -C $bare update-ref $m $bareB &&
+	git -C $bare rev-parse $bareB >expect &&
+	git -C $bare rev-parse $m >actual &&
+	test_cmp expect actual &&
+	git -C $bare reflog exists $m
+'
+
 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" &&
-- 
2.10.2
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help