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

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

[Jonathan Nieder]

Hi,

Michael Haggerty wrote[1]:
> Jonathan Nieder wrote:

>> The check-ref-format documentation is pretty unclear, but the
>> intent is that it would be used like
>>
>>	git check-ref-format heads/master
>>
>> (see the surviving examples in contrib/examples/). That way, it can
>> enforce the rule (from git-check-ref-format(1))
>>
>>	2. They must contain at least one /. This enforces the presence
>>	of a category like heads/, tags/ etc. but the actual names
>>	are not restricted.
[...]
> Thanks for the explanation and the pointer.
>
> I suppose that was a usage that was more popular in the past than
> now. I can hardly remember anyone talking about references like
> "heads/master" or "tags/v1". It seems to me that when somebody wants
> to be unambiguous they usually use the whole reference name
> "refs/heads/master" or "refs/tags/v1". When they want to be succinct
> they usually use just "master" or "v1".
>
> To me it seems incongruous to have the "heads/master" syntax
> supported at this deep level of plumbing. In most cases, the caller
> could get the same effect by prepending "refs/" to the string and
> then calling check_refname_format with a new
> REFNAME_REQUIRE_CATEGORY option that requires both a "refs/" prefix
> and a total of at least *three* levels.

I agree that this piece of UI is pretty weird.  Worse, it's
underdocumented.

I wonder if it would make sense to have the check-ref-format commandline
utility require two slashes when its argument begins with "refs/" (still
requiring only one slash when it doesn't, for backward compatibility)
and to start encouraging people to pass refnames with refs/ to it.

The alternative would be something like the following, which doesn't
seem too appealing.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
[1] https://code-review.googlesource.com/1017

 Documentation/git-check-ref-format.txt | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index fc02959..447e9fb 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -15,8 +15,8 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-Checks if a given 'refname' is acceptable, and exits with a non-zero
-status if it is not.
+Checks if refs/<refname> is an acceptable ref name, and exits
+with a non-zero status if it is not.
 
 A reference is used in Git to specify branches and tags.  A
 branch head is stored in the `refs/heads` hierarchy, while
@@ -91,14 +91,14 @@ OPTIONS
 	components).  The default is `--no-allow-onelevel`.
 
 --refspec-pattern::
-	Interpret <refname> as a reference name pattern for a refspec
-	(as used with remote repositories).  If this option is
-	enabled, <refname> is allowed to contain a single `*`
-	in place of a one full pathname component (e.g.,
-	`foo/*/bar` but not `foo/bar*`).
+	Interpret refs/<refname> as a reference name pattern
+	for a refspec (as used with remote repositories).
+	If this option is enabled, <refname> is allowed
+	to contain a single `*` in place of a one full pathname
+	component (e.g., `foo/*/bar` but not `foo/bar*`).
 
 --normalize::
-	Normalize 'refname' by removing any leading slash (`/`)
+	Normalize <refname> by removing any leading slash (`/`)
 	characters and collapsing runs of adjacent slashes between
 	name components into a single slash.  Iff the normalized
 	refname is valid then print it to standard output and exit
@@ -118,7 +118,7 @@ $ git check-ref-format --branch @{-1}
 * Determine the reference name to use for a new branch:
 +
 ------------
-$ ref=$(git check-ref-format --normalize "refs/heads/$newbranch") ||
+$ ref=$(git check-ref-format --normalize "heads/$newbranch") ||
 die "we do not like '$newbranch' as a branch name."
 ------------
 
-- 

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Michael Haggerty wrote[1]:
>> Jonathan Nieder wrote:
>
>>> The check-ref-format documentation is pretty unclear, but the
>>> intent is that it would be used like
>>>
>>>	git check-ref-format heads/master
>>>
>>> (see the surviving examples in contrib/examples/). That way, it can
>>> enforce the rule (from git-check-ref-format(1))
>>>
>>>	2. They must contain at least one /. This enforces the presence
>>>	of a category like heads/, tags/ etc. but the actual names
>>>	are not restricted.
> [...]
>> Thanks for the explanation and the pointer.

I wanted to follow this discussion, especially the ellided [...]
"pointer", but had a hart time finding what "pointer" was.

Anyway, the true origin of ONELEVEL as far as I recall was to give
us a way to say "in this code path, we also expect to be fed HEAD,
ORIG_HEAD, etc., so please do not subject us to the 'at least one
slash' rule.", implication of which is that the 'at least one slash'
rule was to expect things are 'refs/<anything>' so there will be at
least one.  Even back then, that <anything> alone had at least one
slash (e.g. heads/master), but the intention was *never* that we
would forbid <anything> that does not have a slash by feeding
<anything> part alone to check-ref-format, i.e. things like
"refs/stash" were designed to be allowed.

That the function does not reject what does not begin with "refs/"
when ONELEVEL is not in effect is just being loose.

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

[Jonathan Nieder]

Junio C Hamano wrote:

>                implication of which is that the 'at least one slash'
> rule was to expect things are 'refs/<anything>' so there will be at
> least one.  Even back then, that <anything> alone had at least one
> slash (e.g. heads/master), but the intention was *never* that we
> would forbid <anything> that does not have a slash by feeding
> <anything> part alone to check-ref-format, i.e. things like
> "refs/stash" were designed to be allowed.

Now I'm more confused.  Until 5f7b202a (2008-01-01), there was a
comment

		if (level < 2)
			return -2; /* at least of form "heads/blah" */

and that behavior has been preserved since the beginning.

Why do most old callers pass a string that doesn't start with refs/
(e.g., see the callers in 03feddd6, 2005-10-13)?  Has the intent been
to relax the requirement since then?

Jonathan

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

[Jeff King]

On Fri, Aug 22, 2014 at 11:45:15AM -0700, Jonathan Nieder wrote:

> Junio C Hamano wrote:
> 
> >                implication of which is that the 'at least one slash'
> > rule was to expect things are 'refs/<anything>' so there will be at
> > least one.  Even back then, that <anything> alone had at least one
> > slash (e.g. heads/master), but the intention was *never* that we
> > would forbid <anything> that does not have a slash by feeding
> > <anything> part alone to check-ref-format, i.e. things like
> > "refs/stash" were designed to be allowed.
> 
> Now I'm more confused.  Until 5f7b202a (2008-01-01), there was a
> comment
> 
> 		if (level < 2)
> 			return -2; /* at least of form "heads/blah" */
> 
> and that behavior has been preserved since the beginning.
> 
> Why do most old callers pass a string that doesn't start with refs/
> (e.g., see the callers in 03feddd6, 2005-10-13)?  Has the intent been
> to relax the requirement since then?

Yeah, this weird "do not allow refs/foo" behavior has continually
confused me. Coincidentally I just noticed a case today where
"pack-refs" treats "refs/foo" specially for no good reason:

  http://thread.gmane.org/gmane.comp.version-control.git/255729

After much head scratching over the years, I am of the opinion that
nobody every really _meant_ to prevent "refs/foo", and that code
comments like the one you quote above were an attempt to document
existing buggy behavior that was really trying to differentiate "HEAD"
from "refs/*". That's just my opinion, though. :) I'd be happy if all of
the special-treatment of "refs/foo" went away and check_refname_format
always got the full ref.

-Peff

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

[Junio C Hamano]

On Fri, Aug 22, 2014 at 10:46 PM, Jeff King <peff@peff.net> wrote:
>
> After much head scratching over the years, I am of the opinion that
> nobody every really _meant_ to prevent "refs/foo"...

Yup, that matches my understanding.

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

[Ronnie Sahlberg]

On Fri, Aug 22, 2014 at 10:46 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Aug 22, 2014 at 11:45:15AM -0700, Jonathan Nieder wrote:
>
>> Junio C Hamano wrote:
>>
>> >                implication of which is that the 'at least one slash'
>> > rule was to expect things are 'refs/<anything>' so there will be at
>> > least one.  Even back then, that <anything> alone had at least one
>> > slash (e.g. heads/master), but the intention was *never* that we
>> > would forbid <anything> that does not have a slash by feeding
>> > <anything> part alone to check-ref-format, i.e. things like
>> > "refs/stash" were designed to be allowed.
>>
>> Now I'm more confused.  Until 5f7b202a (2008-01-01), there was a
>> comment
>>
>>               if (level < 2)
>>                       return -2; /* at least of form "heads/blah" */
>>
>> and that behavior has been preserved since the beginning.
>>
>> Why do most old callers pass a string that doesn't start with refs/
>> (e.g., see the callers in 03feddd6, 2005-10-13)?  Has the intent been
>> to relax the requirement since then?
>
> Yeah, this weird "do not allow refs/foo" behavior has continually
> confused me. Coincidentally I just noticed a case today where
> "pack-refs" treats "refs/foo" specially for no good reason:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/255729
>
> After much head scratching over the years, I am of the opinion that
> nobody every really _meant_ to prevent "refs/foo", and that code
> comments like the one you quote above were an attempt to document
> existing buggy behavior that was really trying to differentiate "HEAD"
> from "refs/*". That's just my opinion, though. :) I'd be happy if all of
> the special-treatment of "refs/foo" went away and check_refname_format
> always got the full ref.
>

There are also a lot of places where we assume that a refs will start
with "refs/heads/" and not just "refs/"
for_each_branch_ref(), log_ref_setup() (so no reflogs) is_branch() to
name a few.

This makes the api a bit confusing and hard to predict. Which
functions allow refs/foo and which will ignore it?
Are there any compelling reasons why refs/foo should be allowed?

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

[Jonathan Nieder]

Ronnie Sahlberg wrote:
> On Fri, Aug 22, 2014 at 10:46 PM, Jeff King <peff@peff.net> wrote:

>> Yeah, this weird "do not allow refs/foo" behavior has continually
>> confused me. Coincidentally I just noticed a case today where
>> "pack-refs" treats "refs/foo" specially for no good reason:
>>
>>   http://thread.gmane.org/gmane.comp.version-control.git/255729
>>
>> After much head scratching over the years, I am of the opinion that
>> nobody every really _meant_ to prevent "refs/foo", and that code
>> comments like the one you quote above were an attempt to document
>> existing buggy behavior that was really trying to differentiate "HEAD"
>> from "refs/*". That's just my opinion, though. :)

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.

[...]
> There are also a lot of places where we assume that a refs will start
> with "refs/heads/" and not just "refs/"
> for_each_branch_ref(), log_ref_setup() (so no reflogs) is_branch() to
> name a few.

for_each_branch_ref is for iterating over local branches, which are
defined as refs that start with refs/heads/*.  Likewise, the only
point of is_branch is to check whether a ref is under refs/heads/*.
That's not an assumption about all refs.

log_ref_setup implements the policy that there are only reflogs for:

 * refs where the reflog was explicitly created ("git branch
   --create-reflog" does this, but for some reason there's no
   corresponding "git update-ref --create-reflog" so people have
   to use mkdir directly for other refs), plus

 * if the '[core] logallrefupdates' configuration is enabled (and it
   is by default for non-bare repositories), then HEAD, refs/heads/*,
   refs/notes/*, and refs/remotes/*.

This is documented in git-config(1) --- see core.logAllRefUpdates.

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.

It definitely isn't an assumption that *all* refs will match that
pattern.  But it might be worth changing for other reasons.

Jonathan

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

[Jeff King]

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@peff.net>
---
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

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

[Michael Haggerty]

On 08/25/2014 09:09 PM, Jeff King wrote:
> [...]
> 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 like the idea. I would use this setting on all my repos (information
packrat that I am).

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> There are also a lot of places where we assume that a refs will start
> with "refs/heads/" and not just "refs/"
> for_each_branch_ref(), log_ref_setup() (so no reflogs) is_branch() to
> name a few.

for-each-BRANCH-ref and is-BRANCH are explicitly about branches and
it is natural that they will work only on refs that start with
refs/heads/, no?  I am not sure about log-ref-setup ("git grep pu"
does not find it).

> This makes the api a bit confusing and hard to predict. Which
> functions allow refs/foo and which will ignore it?  Are there any
> compelling reasons why refs/foo should be allowed?

The only one I can think of offhand that we internally use is the
stash, but that does not give us any assurance that no third-party
tool is using refs/frotz for its own use X-<.

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

[Jonathan Nieder]

Junio C Hamano wrote:
>> Michael Haggerty wrote[1]:
>>> Jonathan Nieder wrote:

>>>> The check-ref-format documentation is pretty unclear, but the
>>>> intent is that it would be used like
>>>>
>>>>	git check-ref-format heads/master
>>>>
>>>> (see the surviving examples in contrib/examples/). That way, it can
>>>> enforce the rule (from git-check-ref-format(1))
[...]
>>> Thanks for the explanation and the pointer.
>
> I wanted to follow this discussion, especially the ellided [...]
> "pointer", but had a hart time finding what "pointer" was.

I missed this question before.

The discussion happened at https://code-review.googlesource.com/1017.
It's easier to see after clicking the 'Expand All' button, but even
then it's hard to see the signal in the 'Patch Set <n> was rebased'
noise.

The pointer Michael mentioned was to the git-check-ref-format(1)
manpage and old 'check-ref-format' callers that can be found in
contrib/examples/.  Sorry for the lack of context.

Jonathan