Junio C Hamano <gitster@xxxxxxxxx> writes:
> I was trying to summarize this topic for Release Notes.
>
> Possibly incompatible changes
> -----------------------------
>
> * Special refs such as "HEAD", "MERGE_HEAD", and "FETCH_HEAD" are now
> restricted to names with only uppercase letters and underscore. All
> other refs must live under refs/ namespace. Earlier, you could
> confuse git by storing an object name in $GIT_DIR/tmp/junk and say
> "git show tmp/junk", but this will no longer work.
>
> But noticed that "git update-ref tmp/junk HEAD" does create such a ref
> that won't be recognized, and "git check-ref-format tmp/junk" is happy.
>
> I think we would need to restrict check_ref_format() so that these
> commands (and possibly others, but I think that single function will cover
> pretty much everything) also reject "tmp/junk" immediately below $GIT_DIR
> as a bad string. Otherwise we cannot merge these fixups, which would mean
> we would have to revert the "Clean up refname checks and normalization"
> series, at least the part that started emitting the "warning", which is a
> mess I would rather want to avoid.
>
> Opinions on how to get us out of this mess?
Let me step back a bit to avoid making hasty decisions that may cause
negative effect on the end users only because a single topic with a
possible fallout has been merged to 'master' already (a sane position for
such a situation always has been to revert the merge of the topic so far,
but I ended up being hasty, only because the merge was deliberately made
early in the cycle to give us enough time to deal with fallouts).
In general, the "Hey that file that appears in our dwimmed ref namespace
does not store correct [0-9a-f]{40} contents" warning message is a good
thing to have. When we try the dwimmery and disambiguation, we however
look at the potential refs and warn disambiguity only when two or more
such files have good contents. E.g. if I do this:
$ git rev-parse HEAD >.git/refs/heads/frotz
$ echo hello >.git/refs/tags/frotz
$ git show frotz
we have never paid attention to the broken tag and showed the 'frotz'
branch without complaining. Once tags/frotz gets a real object name,
however, we start giving ambiguity warnings.
Perhaps that is what we should be fixing instead.
Right now, dwim_ref() classifies candidates into two categories, ones that
resolve_ref() succeeds, and others that resolve_ref() does not. And when
we have two or more candidates that successfully resolves, there is an
ambiguity.
Perhaps we need the third kind: ones that exist on the filesystem but
are ill-formed.
When naïvely looked at, the code in 'master' may seem to be doing just
that, but it does _not_ have any way to affect the ambiguity detection
logic even if the caller wanted to. The warning is issued at a wrong
level.
Perhaps resolve_ref() should return in its *flag parameter that "a file
exists there but incorrectly formatted", and dwim_ref() should notice and
use that information to warn about ambiguity and also illformed-ness.
A patch is attached at the end of this message to minimally fix what is in
'master' (without the jc/check-ref-format-fixup topic). This update allows
us to make dwim_ref() notice such "exists but broken" candidates and later
take them into consideration when deciding if a name is ambiguous, but I
did not want to change the semantics from the traditional implementation,
so it only uses this information to warn ref breakages. Also this squelches
the phony "index is not well formed" warning against "git show index --"
by knowing that many files directly under $GIT_DIR are not ref-like things.
Michaels's "when taken as a full refname, is this string valid?" update
mentioned in the thread may be used to replace the strchr(fullref, '/') check
that can be seen in this patch.
refs.c | 22 +++++++++++-----------
refs.h | 5 +++--
sha1_name.c | 5 ++++-
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/refs.c b/refs.c
index cab4394..0f58e46 100644
--- a/refs.c
+++ b/refs.c
@@ -4,9 +4,8 @@
#include "tag.h"
#include "dir.h"
-/* ISSYMREF=01 and ISPACKED=02 are public interfaces */
-#define REF_KNOWS_PEELED 04
-#define REF_BROKEN 010
+/* ISSYMREF=0x01, ISPACKED=0x02 and ISBROKEN=0x04 are public interfaces */
+#define REF_KNOWS_PEELED 0x10
struct ref_entry {
unsigned char flag; /* ISSYMREF? ISPACKED? */
@@ -329,12 +328,12 @@ static void get_ref_dir(const char *submodule, const char *base,
flag = 0;
if (resolve_gitlink_ref(submodule, ref, sha1) < 0) {
hashclr(sha1);
- flag |= REF_BROKEN;
+ flag |= REF_ISBROKEN;
}
} else
if (!resolve_ref(ref, sha1, 1, &flag)) {
hashclr(sha1);
- flag |= REF_BROKEN;
+ flag |= REF_ISBROKEN;
}
add_ref(ref, sha1, flag, array, NULL);
}
@@ -501,7 +500,6 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
ssize_t len;
char buffer[256];
static char ref_buffer[256];
- char path[PATH_MAX];
if (flag)
*flag = 0;
@@ -510,6 +508,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
return NULL;
for (;;) {
+ char path[PATH_MAX];
struct stat st;
char *buf;
int fd;
@@ -586,8 +585,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
while (isspace(*buf))
buf++;
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
- warning("symbolic reference in %s is formatted incorrectly",
- path);
+ if (flag)
+ *flag |= REF_ISBROKEN;
return NULL;
}
ref = strcpy(ref_buffer, buf);
@@ -596,7 +595,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
}
/* Please note that FETCH_HEAD has a second line containing other data. */
if (get_sha1_hex(buffer, sha1) || (buffer[40] != '\0' && !isspace(buffer[40]))) {
- warning("reference in %s is formatted incorrectly", path);
+ if (flag)
+ *flag |= REF_ISBROKEN;
return NULL;
}
return ref;
@@ -624,8 +624,8 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,
return 0;
if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
- if (entry->flag & REF_BROKEN)
- return 0; /* ignore dangling symref */
+ if (entry->flag & REF_ISBROKEN)
+ return 0; /* ignore dangling symref and corrupt ref */
if (!has_sha1_file(entry->sha1)) {
error("%s does not point to a valid object!", entry->name);
return 0;
diff --git a/refs.h b/refs.h
index 0229c57..7442b29 100644
--- a/refs.h
+++ b/refs.h
@@ -10,8 +10,9 @@ struct ref_lock {
int force_write;
};
-#define REF_ISSYMREF 01
-#define REF_ISPACKED 02
+#define REF_ISSYMREF 0x01
+#define REF_ISPACKED 0x02
+#define REF_ISBROKEN 0x04
/*
* Calls the specified function for each ref file until it returns nonzero,
diff --git a/sha1_name.c b/sha1_name.c
index ba976b4..1fe37c6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -282,8 +282,11 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
*ref = xstrdup(r);
if (!warn_ambiguous_refs)
break;
- } else if ((flag & REF_ISSYMREF) && strcmp(fullref, "HEAD"))
+ } else if ((flag & REF_ISSYMREF) && strcmp(fullref, "HEAD")) {
warning("ignoring dangling symref %s.", fullref);
+ } else if ((flag & REF_ISBROKEN) && strchr(fullref, '/')) {
+ warning("ignoring broken ref %s.", fullref);
+ }
}
free(last_branch);
return refs_found;
--
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