Re: check-ref-format: include refs/ in the argument or to strip it?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 25, 2014 at 11:26:36AM -0700, Jonathan Nieder wrote:

> It's still very puzzling to me.  The comment came at the same time as
> the behavior, in v0.99.9~120 (git-check-ref-format: reject funny ref
> names, 2005-10-13).  Before that, the behavior was even stranger ---
> it checked that there was exactly one slash in the argument.
> 
> I'm willing to believe we might not want that check any more, though.

Yeah, given that among experiences gits, nobody can figure out a
motivation or a history for the feature (and given that it causes
problems), I do not see any problem with loosening it.

> That way, when tools internally use other refs (e.g., FETCH_HEAD),
> git doesn't have to automatically incur the cost of maintaining the
> reflog for those.  What other refs should there be reflogs for?  I
> haven't thought carefully about this.

I think you'd in theory want a reflog for anything. The refs in
refs/tags are not meant to change, but if they do (e.g., somebody
force-pushes a tag to a server) it is nice to have a log of what
happened.  I think the same argument could apply to anything in refs/.

I think more ephemeral things (like MERGE_HEAD) tend to be in the root,
outside of refs. Reflogging those _could_ be useful, but probably isn't
(and in the case of something like FETCH_HEAD, would not record all of
the information anyway).

I wrote the patch below over a year ago and very nearly submitted it. At
GitHub we use reflogs frequently for auditing and forensics, and I
wanted to have such an audit trail for everything. However, we ended up
doing something a little more invasive that I do not think would be that
interesting to upstream (though I am happy to submit a patch if people
think otherwise): we maintain a separate "audit log" for all refs that
is never pruned, and lives on even when refs are deleted.

-- >8 --
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 <peff@xxxxxxxx>
---
Looking over the code, I am not sure that it actually works as
advertised with respect to ORIG_HEAD, etc. That would be easy enough to
fix, though.

 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    | 31 +++++++++++++++++++++++++++++++
 9 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c55c22a..27629df 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -405,10 +405,12 @@ 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`, 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 46e8aa8..1d140b7 100644
--- a/branch.c
+++ b/branch.c
@@ -292,7 +292,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 f71e745..65bc066 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -586,7 +586,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 56f85e2..ab0651f 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 (!starts_with(git_dir, work_tree) ||
 		    strcmp(git_dir + strlen(work_tree), "/.git")) {
diff --git a/cache.h b/cache.h
index fcb511d..79c5ae1 100644
--- a/cache.h
+++ b/cache.h
@@ -603,7 +603,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 int shared_repository;
@@ -634,6 +633,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;
+
 /*
  * The character that begins a commented line in user-editable file
  * that is subject to stripspace.
diff --git a/config.c b/config.c
index 058505c..7706fdf 100644
--- a/config.c
+++ b/config.c
@@ -703,7 +703,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 565f652..3e77237 100644
--- a/environment.c
+++ b/environment.c
@@ -21,7 +21,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 warn_on_object_refname_ambiguity = 1;
 int repository_format_version;
diff --git a/refs.c b/refs.c
index 27927f2..3b71e01 100644
--- a/refs.c
+++ b/refs.c
@@ -2707,7 +2707,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;
@@ -2776,17 +2776,28 @@ static int copy_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
+static int should_create_reflog(const char *refname)
+{
+	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;
+	}
+}
+
 /* This function must set a meaningful errno on failure */
 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 &&
-	    (starts_with(refname, "refs/heads/") ||
-	     starts_with(refname, "refs/remotes/") ||
-	     starts_with(refname, "refs/notes/") ||
-	     !strcmp(refname, "HEAD"))) {
+	if (should_create_reflog(refname)) {
 		if (safe_create_leading_directories(logfile) < 0) {
 			int save_errno = errno;
 			error("unable to create directory for %s", logfile);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 0218e96..7669917 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -983,4 +983,35 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
 	test_must_fail git rev-parse --verify -q $c
 '
 
+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.1.0.346.ga0367b9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]