Junio C Hamano [off-list ref] 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?
Addendum.
I was digging this further and see fairly large conflicts between the bulk
of "clean up refname checks and normalization" topic and the logic to
avoid the additional warning by tightening the dwimmery.
check_refname_format() has always assumed that it is OK to be called at
any level of substring, and there are many code like this one (example is
from remote.c::get_fetch_map()):
for (rmp = &ref_map; *rmp; ) {
if ((*rmp)->peer_ref) {
if (check_refname_format((*rmp)->peer_ref->name + 5,
REFNAME_ALLOW_ONELEVEL)) {
struct ref *ignore = *rmp;
error("* Ignoring funny ref '%s' locally",
(*rmp)->peer_ref->name);
This code somehow _knows_ that peer_ref->name begins with "refs/" and that
is the reason it adds 5 to skip that known part. In this particular case,
I think we can simply drop the +5 and allow-onelevel, but there are other
instances of the calls to the function that feeds the rest of the refname
string, skipping leading substring (not necessarily "refs/"), assuming
that any component string is either valid or invalid no matter where it
appears in the full refname. I wouldn't be surprised if some callers do
not even have enough information to tell what the leading substring would
be in the full refname context (e.g. parsing of "master:master" refspec,
relying on the later dwimmery to add refs/heads/ in front, could just
verify that "master" is in good format with allow-onelevel).
The new restriction bolted on to that logic in jc/check-ref-format-fixup
series to work around the new warning in 629cd3a (resolve_ref(): emit
warnings for improperly-formatted references, 2011-09-15) is incompatible
with the assumption, as we would need to check full refname, and treat the
first refname component very differently from other components. It has to
be either "refs" in multi-component refname, or all caps in a one-level
one, but the callers of check_refname_format() are not designed to feed
the full refname to begin with.
I am tempted to revert 629cd3a (resolve_ref(): emit warnings for
improperly-formatted references, 2011-09-15) that started giving the
warnings, and drop the jc/check-ref-format-fixup topic [*1*] altogether,
but that is a short-sighted workaround I would rather want to avoid. It
essentially declares that the "Clean up refname checks" topic was a
failure and did not manage to really clean things up.
A possible alternative might be to leave check_refname_format() and its
callers as they are, introduce check_full_refname() function that knows
the new restriction on top of check-ref-format-fixup, and use that in
lock_ref_sha1(), lock_any_ref_for_update() and is_refname_available()
[*2*]. That way, we can keep the potentially useful "ill-formed contents
in the ref" warning and avoid possible confusion caused by random files
that are directly under $GIT_DIR, which would be far more preferable in
the longer term.
Anybody wants to give it a try?
[Footnote]
*1* The topic is misnamed; it is about fixing dwimmery, not checking.
*2* This currently does not seem to check the well-formedness of the ref
it is creating at all; it should check the "ref", but not "oldref" to
allow renaming a malformed ref to correct earlier mistakes.
On 10/19/2011 08:19 AM, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:
quoted
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?
Addendum.
I was digging this further and see fairly large conflicts between the bulk
of "clean up refname checks and normalization" topic and the logic to
avoid the additional warning by tightening the dwimmery.
check_refname_format() has always assumed that it is OK to be called at
any level of substring, and there are many code like this one (example is
from remote.c::get_fetch_map()):
for (rmp = &ref_map; *rmp; ) {
if ((*rmp)->peer_ref) {
if (check_refname_format((*rmp)->peer_ref->name + 5,
REFNAME_ALLOW_ONELEVEL)) {
struct ref *ignore = *rmp;
error("* Ignoring funny ref '%s' locally",
(*rmp)->peer_ref->name);
This code somehow _knows_ that peer_ref->name begins with "refs/" and that
is the reason it adds 5 to skip that known part. In this particular case,
I think we can simply drop the +5 and allow-onelevel, but there are other
instances of the calls to the function that feeds the rest of the refname
string, skipping leading substring (not necessarily "refs/"), assuming
that any component string is either valid or invalid no matter where it
appears in the full refname. I wouldn't be surprised if some callers do
not even have enough information to tell what the leading substring would
be in the full refname context (e.g. parsing of "master:master" refspec,
relying on the later dwimmery to add refs/heads/ in front, could just
verify that "master" is in good format with allow-onelevel).
The new restriction bolted on to that logic in jc/check-ref-format-fixup
series to work around the new warning in 629cd3a (resolve_ref(): emit
warnings for improperly-formatted references, 2011-09-15) is incompatible
with the assumption, as we would need to check full refname, and treat the
first refname component very differently from other components. It has to
be either "refs" in multi-component refname, or all caps in a one-level
one, but the callers of check_refname_format() are not designed to feed
the full refname to begin with.
I am tempted to revert 629cd3a (resolve_ref(): emit warnings for
improperly-formatted references, 2011-09-15) that started giving the
warnings, and drop the jc/check-ref-format-fixup topic [*1*] altogether,
but that is a short-sighted workaround I would rather want to avoid. It
essentially declares that the "Clean up refname checks" topic was a
failure and did not manage to really clean things up.
A possible alternative might be to leave check_refname_format() and its
callers as they are, introduce check_full_refname() function that knows
the new restriction on top of check-ref-format-fixup, and use that in
lock_ref_sha1(), lock_any_ref_for_update() and is_refname_available()
[*2*]. That way, we can keep the potentially useful "ill-formed contents
in the ref" warning and avoid possible confusion caused by random files
that are directly under $GIT_DIR, which would be far more preferable in
the longer term.
Anybody wants to give it a try?
I think that the refs/-or-ALL_CAPS test fits most naturally in
check_refname_format(), controlled by an option flag.
I'm starting by building parts of the solution, namely something like:
* Add an option REFNAME_ALLOW_DWIM which causes check_refname_format()
to accept reference names that *could* be dwimmed into a valid refname.
Specifically, when this option is *not* specified, then the option must
start with "refs/" or be ALL_CAPS. When it *is* specified, the behavior
will be much like the current behavior when ALLOW_ONELEVEL is specified
(in fact, the new option might make ALLOW_ONELEVEL obsolete).
* Add a new function parse_refname_prefix(str, len, flags) which tries
to read a refname from the front of str (of length len, not necessarily
NUL-terminated) and returns the length of the part that could be used.
This function will support the same flag argument as
check_refname_format() (in fact the latter would become a trivial
wrapper of the new function). I expect that this function will be
useful for dwimmery.
Hopefully I'll have patches before the end of the (Berlin) day.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/