{"thread":{"id":"61384","subject":"[PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","startedAt":"2024-04-29T08:33:26Z","lastAt":"2024-04-30T17:00:18Z","messageCount":8,"participants":["Jeff King","Karthik Nayak","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"493617","messageId":"20240429083325.GE233423@coredump.intra.peff.net","threadId":"61384","inReplyTo":null,"subject":"[PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T08:33:25Z","receivedAt":"2024-04-29T08:33:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Before operating on a refname we get from a user, we usually check that\nit's syntactically valid. As a general rule, refs should be in the\n\"refs/\" namespace, the exception being HEAD and pseudorefs like\nFETCH_HEAD, etc. Those pseudorefs should consist only of all-caps and\ndash. But the syntactic rules are not enforced by check_refname_format().\nSo for example we will happily operate on a ref \"foo/bar\" that will use\nthe file \".git/foo/bar\" under the hood (when using the files backend, of\ncourse).\n\nMaking things even more complicated, refname_is_safe() does enforce\nthese syntax restrictions! When that function was added in 2014, we\nwould have refused to work with such refs entirely. But we stopped being\nso picky in 03afcbee9b (read_packed_refs: avoid double-checking sane\nrefs, 2015-04-16). That rationale there is that check_refname_format()\nis supposed to contain a superset of the checks of refname_is_safe().\nThe idea being that we usually would rely on the more-strict\ncheck_refname_format(), but for certain operations (e.g., deleting a\nref) we want to allow invalid names as long as they are not unsafe\n(e.g., not escaping the on-disk \"refs/\" hierarchy).\n\nBut the pseudoref handling flips this logic; check_refname_format() is\nmore lenient than refname_is_safe(). So you can create \"foo/bar\" and\nread it, but you cannot delete it:\n\n  $ git update-ref foo/bar HEAD\n  $ git rev-parse foo/bar\n  747a29934757b7e695781e13e2511c43b951da2\n  $ git update-ref -d foo/bar\n  error: refusing to update ref with bad name 'foo/bar'\n\nSo we probably want check_ref_format() to learn the same syntactic\nrestrictions that refname_is_safe() uses (namely insisting that anything\noutside of \"refs/\" matches the pseudoref syntax). The most obvious way\nto do that is simply to call refname_is_safe(). But the point of\n03afcbee9b is that doing so is expensive. Without the syntactic\nrestrictions of check_refname_format(), refname_is_safe() has to\nactually normalize the pathname to make sure it does not escape \"refs/\".\nThat's redundant for us in check_refname_format(); we just need to make\nsure it either starts with \"refs/\" or is a pseudoref.\n\nBut wait, it gets more complicated! We also allow \"worktrees/foo/$X\"\nand \"main-worktree/$X\". In that case we should only be checking \"$X\"\n(which should be either a pseudoref or start with \"refs/\"). We can\nuse parse_worktree_ref(), which fairly efficiently gives us the \"bare\"\nrefname (even for a non-worktree ref, where it returns the original\nname).\n\nAnd now when should this new logic kick in? Unfortunately we can't just\ndo it all the time, because many callers pass in partial ref components.\nE.g., if they are thinking about making \"refs/heads/foo\", they'll pass\nus \"foo\". This is usually accompanied by the ALLOW_ONELEVEL flag. But we\nlikewise can't take the absence of ALLOW_ONELEVEL as a hint that the\nname is fully qualified, because that flag is also used to indicate that\npseudorefs should be allowed!\n\nWe need a new flag to tell these two situations apart. So let's add a\nFULLY_QUALIFIED flag that callers can use to ask us to enforce these\nsyntactic rules. There are no callers yet, but we should be able to\nexamine users of ALLOW_ONELEVEL, figure out which semantics they\nwanted, and convert as needed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe whole ALLOW_ONELEVEL thing is a long-standing confusion, and\nunfortunately has made it into the hands of users via \"git\ncheck-ref-format --allow-onelevel\". So I think it is there to stay.\nPossibly we should expose this new feature as --fully-qualified or\nsimilar.\n\n refs.c | 14 +++++++++++++-\n refs.h |  1 +\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8cac7e7e59..44b4419050 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -288,6 +288,15 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n {\n \tint component_len, component_count = 0;\n \n+\tif ((flags & REFNAME_FULLY_QUALIFIED)) {\n+\t\tconst char *bare_ref;\n+\n+\t\tparse_worktree_ref(refname, NULL, NULL, &bare_ref);\n+\t\tif (!starts_with(bare_ref, \"refs/\") &&\n+\t\t    !is_pseudoref_syntax(bare_ref))\n+\t\t\treturn -1;\n+\t}\n+\n \tif (!strcmp(refname, \"@\")) {\n \t\t/* Refname is a single character '@'. */\n \t\tif (sanitized)\n@@ -322,8 +331,11 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n \t\telse\n \t\t\treturn -1;\n \t}\n-\tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n+\n+\tif (!(flags & (REFNAME_ALLOW_ONELEVEL | REFNAME_FULLY_QUALIFIED)) &&\n+\t    component_count < 2)\n \t\treturn -1; /* Refname has only one component. */\n+\n \treturn 0;\n }\n \ndiff --git a/refs.h b/refs.h\nindex d278775e08..cdd859c8b7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -571,6 +571,7 @@ int for_each_reflog(each_reflog_fn fn, void *cb_data);\n \n #define REFNAME_ALLOW_ONELEVEL 1\n #define REFNAME_REFSPEC_PATTERN 2\n+#define REFNAME_FULLY_QUALIFIED 4\n \n /*\n  * Return 0 iff refname has the correct format for a refname according\n-- \n2.45.0.rc1.416.gbe2a76c799\n\n"},{"id":"493645","messageId":"CAOLa=ZR+nunNg8_LMTzwoHzbBU2EboMSsZRodswLP9bMLhtCpg@mail.gmail.com","threadId":"61384","inReplyTo":"20240429083325.GE233423@coredump.intra.peff.net","subject":"Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-04-29T16:01:52Z","receivedAt":"2024-04-29T16:01:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Before operating on a refname we get from a user, we usually check that\n> it's syntactically valid. As a general rule, refs should be in the\n> \"refs/\" namespace, the exception being HEAD and pseudorefs like\n> FETCH_HEAD, etc. Those pseudorefs should consist only of all-caps and\n> dash. But the syntactic rules are not enforced by check_refname_format().\n\nNit: s/dash/underscore\n\nAlso FETCH_HEAD is a special_ref, this confusion should however be\nresolved with Patrick's patches [1].\n\n> So for example we will happily operate on a ref \"foo/bar\" that will use\n> the file \".git/foo/bar\" under the hood (when using the files backend, of\n> course).\n>\n> Making things even more complicated, refname_is_safe() does enforce\n> these syntax restrictions! When that function was added in 2014, we\n> would have refused to work with such refs entirely. But we stopped being\n> so picky in 03afcbee9b (read_packed_refs: avoid double-checking sane\n> refs, 2015-04-16). That rationale there is that check_refname_format()\n> is supposed to contain a superset of the checks of refname_is_safe().\n> The idea being that we usually would rely on the more-strict\n> check_refname_format(), but for certain operations (e.g., deleting a\n> ref) we want to allow invalid names as long as they are not unsafe\n> (e.g., not escaping the on-disk \"refs/\" hierarchy).\n>\n> But the pseudoref handling flips this logic; check_refname_format() is\n> more lenient than refname_is_safe(). So you can create \"foo/bar\" and\n> read it, but you cannot delete it:\n>\n>   $ git update-ref foo/bar HEAD\n>   $ git rev-parse foo/bar\n>   747a29934757b7e695781e13e2511c43b951da2\n>   $ git update-ref -d foo/bar\n>   error: refusing to update ref with bad name 'foo/bar'\n>\n> So we probably want check_ref_format() to learn the same syntactic\n> restrictions that refname_is_safe() uses (namely insisting that anything\n> outside of \"refs/\" matches the pseudoref syntax). The most obvious way\n> to do that is simply to call refname_is_safe(). But the point of\n> 03afcbee9b is that doing so is expensive. Without the syntactic\n> restrictions of check_refname_format(), refname_is_safe() has to\n> actually normalize the pathname to make sure it does not escape \"refs/\".\n> That's redundant for us in check_refname_format(); we just need to make\n> sure it either starts with \"refs/\" or is a pseudoref.\n>\n> But wait, it gets more complicated! We also allow \"worktrees/foo/$X\"\n> and \"main-worktree/$X\". In that case we should only be checking \"$X\"\n> (which should be either a pseudoref or start with \"refs/\"). We can\n> use parse_worktree_ref(), which fairly efficiently gives us the \"bare\"\n> refname (even for a non-worktree ref, where it returns the original\n> name).\n>\n> And now when should this new logic kick in? Unfortunately we can't just\n> do it all the time, because many callers pass in partial ref components.\n> E.g., if they are thinking about making \"refs/heads/foo\", they'll pass\n> us \"foo\". This is usually accompanied by the ALLOW_ONELEVEL flag. But we\n> likewise can't take the absence of ALLOW_ONELEVEL as a hint that the\n> name is fully qualified, because that flag is also used to indicate that\n> pseudorefs should be allowed!\n>\n> We need a new flag to tell these two situations apart. So let's add a\n> FULLY_QUALIFIED flag that callers can use to ask us to enforce these\n> syntactic rules. There are no callers yet, but we should be able to\n> examine users of ALLOW_ONELEVEL, figure out which semantics they\n> wanted, and convert as needed.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The whole ALLOW_ONELEVEL thing is a long-standing confusion, and\n> unfortunately has made it into the hands of users via \"git\n> check-ref-format --allow-onelevel\". So I think it is there to stay.\n> Possibly we should expose this new feature as --fully-qualified or\n> similar.\n>\n>  refs.c | 14 +++++++++++++-\n>  refs.h |  1 +\n>  2 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 8cac7e7e59..44b4419050 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -288,6 +288,15 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n>  {\n>  \tint component_len, component_count = 0;\n>\n> +\tif ((flags & REFNAME_FULLY_QUALIFIED)) {\n> +\t\tconst char *bare_ref;\n> +\n> +\t\tparse_worktree_ref(refname, NULL, NULL, &bare_ref);\n> +\t\tif (!starts_with(bare_ref, \"refs/\") &&\n> +\t\t    !is_pseudoref_syntax(bare_ref))\n> +\t\t\treturn -1;\n> +\t}\n> +\n>  \tif (!strcmp(refname, \"@\")) {\n>  \t\t/* Refname is a single character '@'. */\n>  \t\tif (sanitized)\n> @@ -322,8 +331,11 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n>  \t\telse\n>  \t\t\treturn -1;\n>  \t}\n> -\tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n> +\n> +\tif (!(flags & (REFNAME_ALLOW_ONELEVEL | REFNAME_FULLY_QUALIFIED)) &&\n> +\t    component_count < 2)\n>  \t\treturn -1; /* Refname has only one component. */\n> +\n>  \treturn 0;\n>  }\n>\n> diff --git a/refs.h b/refs.h\n> index d278775e08..cdd859c8b7 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -571,6 +571,7 @@ int for_each_reflog(each_reflog_fn fn, void *cb_data);\n>\n>  #define REFNAME_ALLOW_ONELEVEL 1\n>  #define REFNAME_REFSPEC_PATTERN 2\n> +#define REFNAME_FULLY_QUALIFIED 4\n>\n>  /*\n>   * Return 0 iff refname has the correct format for a refname according\n> --\n> 2.45.0.rc1.416.gbe2a76c799\n\n[1]: https://lore.kernel.org/r/cover.1714398019.git.ps@pks.im\n"},{"id":"493725","messageId":"ZjB5bHiF5kAcRMpP@tanuki","threadId":"61384","inReplyTo":"20240429083325.GE233423@coredump.intra.peff.net","subject":"Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T04:54:04Z","receivedAt":"2024-04-30T04:54:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 29, 2024 at 04:33:25AM -0400, Jeff King wrote:\n> Before operating on a refname we get from a user, we usually check that\n> it's syntactically valid. As a general rule, refs should be in the\n> \"refs/\" namespace, the exception being HEAD and pseudorefs like\n> FETCH_HEAD, etc. Those pseudorefs should consist only of all-caps and\n> dash. But the syntactic rules are not enforced by check_refname_format().\n\ns/dash/underscore, right?\n\n> So for example we will happily operate on a ref \"foo/bar\" that will use\n> the file \".git/foo/bar\" under the hood (when using the files backend, of\n> course).\n> \n> Making things even more complicated, refname_is_safe() does enforce\n> these syntax restrictions! When that function was added in 2014, we\n> would have refused to work with such refs entirely. But we stopped being\n> so picky in 03afcbee9b (read_packed_refs: avoid double-checking sane\n> refs, 2015-04-16). That rationale there is that check_refname_format()\n> is supposed to contain a superset of the checks of refname_is_safe().\n> The idea being that we usually would rely on the more-strict\n> check_refname_format(), but for certain operations (e.g., deleting a\n> ref) we want to allow invalid names as long as they are not unsafe\n> (e.g., not escaping the on-disk \"refs/\" hierarchy).\n\nI still think we should eventually merge these functions. It's not\nexactly obvious why one would use one or the other. So if we had a\nfunction with strict default behaviour, where the caller can ask for\nsome loosening of the behaviour via flags, then I think it would become\na ton easier to do the right thing.\n\nIn any case, that doesn't need to be part of this patch series.\n\n> But the pseudoref handling flips this logic; check_refname_format() is\n> more lenient than refname_is_safe(). So you can create \"foo/bar\" and\n> read it, but you cannot delete it:\n> \n>   $ git update-ref foo/bar HEAD\n>   $ git rev-parse foo/bar\n>   747a29934757b7e695781e13e2511c43b951da2\n>   $ git update-ref -d foo/bar\n>   error: refusing to update ref with bad name 'foo/bar'\n> \n> So we probably want check_ref_format() to learn the same syntactic\n> restrictions that refname_is_safe() uses (namely insisting that anything\n> outside of \"refs/\" matches the pseudoref syntax). The most obvious way\n> to do that is simply to call refname_is_safe(). But the point of\n> 03afcbee9b is that doing so is expensive. Without the syntactic\n> restrictions of check_refname_format(), refname_is_safe() has to\n> actually normalize the pathname to make sure it does not escape \"refs/\".\n> That's redundant for us in check_refname_format(); we just need to make\n> sure it either starts with \"refs/\" or is a pseudoref.\n> \n> But wait, it gets more complicated! We also allow \"worktrees/foo/$X\"\n> and \"main-worktree/$X\". In that case we should only be checking \"$X\"\n> (which should be either a pseudoref or start with \"refs/\"). We can\n> use parse_worktree_ref(), which fairly efficiently gives us the \"bare\"\n> refname (even for a non-worktree ref, where it returns the original\n> name).\n> \n> And now when should this new logic kick in? Unfortunately we can't just\n> do it all the time, because many callers pass in partial ref components.\n> E.g., if they are thinking about making \"refs/heads/foo\", they'll pass\n> us \"foo\". This is usually accompanied by the ALLOW_ONELEVEL flag. But we\n> likewise can't take the absence of ALLOW_ONELEVEL as a hint that the\n> name is fully qualified, because that flag is also used to indicate that\n> pseudorefs should be allowed!\n\nIndeed, it's a proper mess :)\n\n> We need a new flag to tell these two situations apart. So let's add a\n> FULLY_QUALIFIED flag that callers can use to ask us to enforce these\n> syntactic rules. There are no callers yet, but we should be able to\n> examine users of ALLOW_ONELEVEL, figure out which semantics they\n> wanted, and convert as needed.\n\nMakes sense.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The whole ALLOW_ONELEVEL thing is a long-standing confusion, and\n> unfortunately has made it into the hands of users via \"git\n> check-ref-format --allow-onelevel\". So I think it is there to stay.\n> Possibly we should expose this new feature as --fully-qualified or\n> similar.\n\nHm, that's really too bad. I wonder whether we should eventually start\nto deprecate `--allow-onelevel` in favor of `--fully-qualified`. We\nwould continue to accept the flag, but remove it from our documentation\nsuch that scripts start to move over. Then some day, we may replace\n`ALLOW_ONELEVEL` with something like `ALLOW_ROOT_REF` that allows refs\nin the root directory while honoring `is_pseudoref_syntax()`.\n\n>  refs.c | 14 +++++++++++++-\n>  refs.h |  1 +\n>  2 files changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 8cac7e7e59..44b4419050 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -288,6 +288,15 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n>  {\n>  \tint component_len, component_count = 0;\n>  \n> +\tif ((flags & REFNAME_FULLY_QUALIFIED)) {\n> +\t\tconst char *bare_ref;\n> +\n> +\t\tparse_worktree_ref(refname, NULL, NULL, &bare_ref);\n> +\t\tif (!starts_with(bare_ref, \"refs/\") &&\n> +\t\t    !is_pseudoref_syntax(bare_ref))\n> +\t\t\treturn -1;\n> +\t}\n> +\n>  \tif (!strcmp(refname, \"@\")) {\n>  \t\t/* Refname is a single character '@'. */\n>  \t\tif (sanitized)\n> @@ -322,8 +331,11 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n>  \t\telse\n>  \t\t\treturn -1;\n>  \t}\n> -\tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n> +\n> +\tif (!(flags & (REFNAME_ALLOW_ONELEVEL | REFNAME_FULLY_QUALIFIED)) &&\n> +\t    component_count < 2)\n>  \t\treturn -1; /* Refname has only one component. */\n> +\n\nI first thought that we don't have to handle REFNAME_FULLY_QUALIFIED\nhere because the above should already handle it. But we can of course\nhave a single component, only, when the ref is \"refs/\".\n\nPatrick\n\n>  \treturn 0;\n>  }\n>  \n> diff --git a/refs.h b/refs.h\n> index d278775e08..cdd859c8b7 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -571,6 +571,7 @@ int for_each_reflog(each_reflog_fn fn, void *cb_data);\n>  \n>  #define REFNAME_ALLOW_ONELEVEL 1\n>  #define REFNAME_REFSPEC_PATTERN 2\n> +#define REFNAME_FULLY_QUALIFIED 4\n>  \n>  /*\n>   * Return 0 iff refname has the correct format for a refname according\n> -- \n> 2.45.0.rc1.416.gbe2a76c799\n> \n"},{"id":"493741","messageId":"20240430094738.GA1279403@coredump.intra.peff.net","threadId":"61384","inReplyTo":"CAOLa=ZR+nunNg8_LMTzwoHzbBU2EboMSsZRodswLP9bMLhtCpg@mail.gmail.com","subject":"Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-30T09:47:38Z","receivedAt":"2024-04-30T09:47:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2024 at 09:01:52AM -0700, Karthik Nayak wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Before operating on a refname we get from a user, we usually check that\n> > it's syntactically valid. As a general rule, refs should be in the\n> > \"refs/\" namespace, the exception being HEAD and pseudorefs like\n> > FETCH_HEAD, etc. Those pseudorefs should consist only of all-caps and\n> > dash. But the syntactic rules are not enforced by check_refname_format().\n> \n> Nit: s/dash/underscore\n\nOops, yes. Will fix (and the same mistake in patch 8). Thanks.\n\n> Also FETCH_HEAD is a special_ref, this confusion should however be\n> resolved with Patrick's patches [1].\n\nHmph, I guess I was not paying attention and missed that the distinction\neven existed. But yeah, I think for the purposes here it is not at all\nimportant. This is purely about the syntax rules, which are the same for\nHEAD, pseudorefs, and special refs.\n\n-Peff\n"},{"id":"493743","messageId":"20240430100145.GB1279403@coredump.intra.peff.net","threadId":"61384","inReplyTo":"ZjB5bHiF5kAcRMpP@tanuki","subject":"Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-30T10:01:45Z","receivedAt":"2024-04-30T10:01:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2024 at 06:54:04AM +0200, Patrick Steinhardt wrote:\n\n> On Mon, Apr 29, 2024 at 04:33:25AM -0400, Jeff King wrote:\n> > Before operating on a refname we get from a user, we usually check that\n> > it's syntactically valid. As a general rule, refs should be in the\n> > \"refs/\" namespace, the exception being HEAD and pseudorefs like\n> > FETCH_HEAD, etc. Those pseudorefs should consist only of all-caps and\n> > dash. But the syntactic rules are not enforced by check_refname_format().\n> \n> s/dash/underscore, right?\n\nYep, thanks.\n\n> > Making things even more complicated, refname_is_safe() does enforce\n> > these syntax restrictions! When that function was added in 2014, we\n> > would have refused to work with such refs entirely. But we stopped being\n> > so picky in 03afcbee9b (read_packed_refs: avoid double-checking sane\n> > refs, 2015-04-16). That rationale there is that check_refname_format()\n> > is supposed to contain a superset of the checks of refname_is_safe().\n> > The idea being that we usually would rely on the more-strict\n> > check_refname_format(), but for certain operations (e.g., deleting a\n> > ref) we want to allow invalid names as long as they are not unsafe\n> > (e.g., not escaping the on-disk \"refs/\" hierarchy).\n> \n> I still think we should eventually merge these functions. It's not\n> exactly obvious why one would use one or the other. So if we had a\n> function with strict default behaviour, where the caller can ask for\n> some loosening of the behaviour via flags, then I think it would become\n> a ton easier to do the right thing.\n> \n> In any case, that doesn't need to be part of this patch series.\n\nYeah, I think it would be fine to merge them with a flag. With the one\nexception that started this topic (and which I think could go away after\nthis series), every caller of refname_is_safe() only does so in\nconjunction with check_refname_format() to check \"well, is it at least\nsafe?\".\n\nYou will have to tweak the return value somehow to indicate \"ok\" versus\n\"bad but safe\" versus \"unsafe\" (e.g., resolving sets REF_BAD_NAME but\ncontinues for the middle one). I think I'd prefer to leave that out of\nthis series (and after this, I think it becomes easier as a pure\nrefactoring since the behavior remains the same).\n\n> > The whole ALLOW_ONELEVEL thing is a long-standing confusion, and\n> > unfortunately has made it into the hands of users via \"git\n> > check-ref-format --allow-onelevel\". So I think it is there to stay.\n> > Possibly we should expose this new feature as --fully-qualified or\n> > similar.\n> \n> Hm, that's really too bad. I wonder whether we should eventually start\n> to deprecate `--allow-onelevel` in favor of `--fully-qualified`. We\n> would continue to accept the flag, but remove it from our documentation\n> such that scripts start to move over. Then some day, we may replace\n> `ALLOW_ONELEVEL` with something like `ALLOW_ROOT_REF` that allows refs\n> in the root directory while honoring `is_pseudoref_syntax()`.\n\nI don't know if we could ever get rid of --allow-onelevel. If you want\nto check a branch name, say, the replacement for it is to ask about\n\"refs/heads/$name\". But sometimes you don't actually know how the short\nname is going to be used, but you want to make sure it's syntactically\nvalid. E.g., validating a refspec may involve a name like \"main\" on its\nown. I suspect it would be OK in practice to just give it an arbitrary\n\"refs/foo/$main\", but that feels kind of hacky.\n\n-Peff\n\n> > @@ -288,6 +288,15 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n> >  {\n> >  \tint component_len, component_count = 0;\n> >  \n> > +\tif ((flags & REFNAME_FULLY_QUALIFIED)) {\n> > +\t\tconst char *bare_ref;\n> > +\n> > +\t\tparse_worktree_ref(refname, NULL, NULL, &bare_ref);\n> > +\t\tif (!starts_with(bare_ref, \"refs/\") &&\n> > +\t\t    !is_pseudoref_syntax(bare_ref))\n> > +\t\t\treturn -1;\n> > +\t}\n> > +\n> >  \tif (!strcmp(refname, \"@\")) {\n> >  \t\t/* Refname is a single character '@'. */\n> >  \t\tif (sanitized)\n> > @@ -322,8 +331,11 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n> >  \t\telse\n> >  \t\t\treturn -1;\n> >  \t}\n> > -\tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n> > +\n> > +\tif (!(flags & (REFNAME_ALLOW_ONELEVEL | REFNAME_FULLY_QUALIFIED)) &&\n> > +\t    component_count < 2)\n> >  \t\treturn -1; /* Refname has only one component. */\n> > +\n> \n> I first thought that we don't have to handle REFNAME_FULLY_QUALIFIED\n> here because the above should already handle it. But we can of course\n> have a single component, only, when the ref is \"refs/\".\n\nI hadn't really considered that case. The reason we have to handle\nFULLY_QUALIFIED here is that without it, \"FETCH_HEAD\" (or for that\nmatter \"HEAD\") is forbidden as having only a single component. The\nearlier hunk only rejects bad things, so we still end up in this code.\n\nI think that \"refs/\" is forbidden both before and after my patch because\nit's invalid to have a zero-length component (so \"foo//bar\" is\nforbidden, but so is \"foo/\" because of the empty component on the end).a\n\n-Peff\n"},{"id":"493744","messageId":"ZjDCd3z1MeFlvh7Q@tanuki","threadId":"61384","inReplyTo":"20240430094738.GA1279403@coredump.intra.peff.net","subject":"Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T10:05:43Z","receivedAt":"2024-04-30T10:05:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 05:47:38AM -0400, Jeff King wrote:\n> On Mon, Apr 29, 2024 at 09:01:52AM -0700, Karthik Nayak wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> > \n> > > Before operating on a refname we get from a user, we usually check that\n> > > it's syntactically valid. As a general rule, refs should be in the\n> > > \"refs/\" namespace, the exception being HEAD and pseudorefs like\n> > > FETCH_HEAD, etc. Those pseudorefs should consist only of all-caps and\n> > > dash. But the syntactic rules are not enforced by check_refname_format().\n> > \n> > Nit: s/dash/underscore\n> \n> Oops, yes. Will fix (and the same mistake in patch 8). Thanks.\n> \n> > Also FETCH_HEAD is a special_ref, this confusion should however be\n> > resolved with Patrick's patches [1].\n> \n> Hmph, I guess I was not paying attention and missed that the distinction\n> even existed. But yeah, I think for the purposes here it is not at all\n> important. This is purely about the syntax rules, which are the same for\n> HEAD, pseudorefs, and special refs.\n\nAgreed. I'll send out a complete rewrite of that patch series in a bit\nwhere I drop the distinction of special and pseudo refs completely. From\nthereon, pseudo refs return to their original meaning, which is a file\nthat is not a ref, but that can be parsed as a ref via git-rev-parse(1).\n\nI think that the current state of refs, special refs, root refs, pseudo\nrefs and head refs is not something that anybody can reasonably\nunderstand without studying our ref code for months. And I did study it\nfor months now and still feel like I don't always get the full picture.\n\nPatrick\n"},{"id":"493745","messageId":"ZjDDa5PIxPCFzMhq@tanuki","threadId":"61384","inReplyTo":"20240430100145.GB1279403@coredump.intra.peff.net","subject":"Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T10:09:47Z","receivedAt":"2024-04-30T10:09:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 06:01:45AM -0400, Jeff King wrote:\n> On Tue, Apr 30, 2024 at 06:54:04AM +0200, Patrick Steinhardt wrote:\n> > On Mon, Apr 29, 2024 at 04:33:25AM -0400, Jeff King wrote:\n[snip]\n> > > The whole ALLOW_ONELEVEL thing is a long-standing confusion, and\n> > > unfortunately has made it into the hands of users via \"git\n> > > check-ref-format --allow-onelevel\". So I think it is there to stay.\n> > > Possibly we should expose this new feature as --fully-qualified or\n> > > similar.\n> > \n> > Hm, that's really too bad. I wonder whether we should eventually start\n> > to deprecate `--allow-onelevel` in favor of `--fully-qualified`. We\n> > would continue to accept the flag, but remove it from our documentation\n> > such that scripts start to move over. Then some day, we may replace\n> > `ALLOW_ONELEVEL` with something like `ALLOW_ROOT_REF` that allows refs\n> > in the root directory while honoring `is_pseudoref_syntax()`.\n> \n> I don't know if we could ever get rid of --allow-onelevel. If you want\n> to check a branch name, say, the replacement for it is to ask about\n> \"refs/heads/$name\". But sometimes you don't actually know how the short\n> name is going to be used, but you want to make sure it's syntactically\n> valid. E.g., validating a refspec may involve a name like \"main\" on its\n> own. I suspect it would be OK in practice to just give it an arbitrary\n> \"refs/foo/$main\", but that feels kind of hacky.\n\nAh, fair enough.\n\n[snip]\n> > > @@ -322,8 +331,11 @@ static int check_or_sanitize_refname(const char *refname, int flags,\n> > >  \t\telse\n> > >  \t\t\treturn -1;\n> > >  \t}\n> > > -\tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n> > > +\n> > > +\tif (!(flags & (REFNAME_ALLOW_ONELEVEL | REFNAME_FULLY_QUALIFIED)) &&\n> > > +\t    component_count < 2)\n> > >  \t\treturn -1; /* Refname has only one component. */\n> > > +\n> > \n> > I first thought that we don't have to handle REFNAME_FULLY_QUALIFIED\n> > here because the above should already handle it. But we can of course\n> > have a single component, only, when the ref is \"refs/\".\n> \n> I hadn't really considered that case. The reason we have to handle\n> FULLY_QUALIFIED here is that without it, \"FETCH_HEAD\" (or for that\n> matter \"HEAD\") is forbidden as having only a single component. The\n> earlier hunk only rejects bad things, so we still end up in this code.\n> \n> I think that \"refs/\" is forbidden both before and after my patch because\n> it's invalid to have a zero-length component (so \"foo//bar\" is\n> forbidden, but so is \"foo/\" because of the empty component on the end).a\n\nMakes sense, thanks.\n\nPatrick\n"},{"id":"493798","messageId":"xmqqmspardk5.fsf@gitster.g","threadId":"61384","inReplyTo":"ZjDDa5PIxPCFzMhq@tanuki","subject":"Re: [PATCH 6/8] check_refname_format(): add FULLY_QUALIFIED flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-30T17:00:10Z","receivedAt":"2024-04-30T17:00:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> valid. E.g., validating a refspec may involve a name like \"main\" on its\n>> own. I suspect it would be OK in practice to just give it an arbitrary\n>> \"refs/foo/$main\", but that feels kind of hacky.\n>\n> Ah, fair enough.\n\nI actually do not think it is fair enough.  Why does the caller want\nto validate \"main\" in the first place?  To make the example more\nrealistic, lets imagine a caller wants to validate \"HEAD\".  We can\nsay \"it is syntactically correct\", but in what context is that\nanswer useful?  If the caller is contemplating to create a new\nbranch given a short-name, we would want to say \"no, refs/heads/HEAD\nis not something we want you to create\", but if the caller is\nplanning to create refs/remotes/origin/HEAD, our answer would be\ndifferent.\n\n\n"}]}