git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Oct 19, 2011, 20:31 UTC
Message-ID
<4E9F33B9.4040803@alum.mit.edu>
In-Reply-To
<7vaa8wdbld.fsf@alter.siamese.dyndns.org>
On 10/19/2011 09:29 PM, Junio C Hamano wrote:
Show 15 quoted lines
> 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.

This all sounds very sane. dwim_ref() currently casts about wildly looking for possible references, so it makes sense that it be selective about what warnings it emits. I also agree with the principle that this warning is better emitted from higher-level code.

> 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.

ISTM that such warnings should also be emitted when (for example) the following commands encounter corrupt references: "git fsck", "git pack-refs", "git gc", and "git push". (Maybe these commands already emit warnings; I haven't checked.) These commands are not run so frequently (so that the warnings would not be so annoying). They are also presumably not so promiscuous about where they look for refs and therefore won't generate so many false alarms.

But it will be easy to add warnings to these commands using the REF_ISBROKEN flag that you made public.

> 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).  [...]

I would have preferred this change being applied on top of your first patch in jc/check-ref-format-fixup, because moving these functions to refs.c makes sense. (Not to mention that my "REFNAME_FULL" patch series is built on top of jc/check-ref-format-fixup.)

Show 16 quoted lines
>  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
> @@ -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;
>  				}

The renaming of this constant could have been done in a separate patch to reduce noise like this in the main patch.

Otherwise the patch looks fine to me.
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Junio C HamanoNext: Junio C Hamano
Message 52 of 60 in “Clean up refname checks and normalization”
  1. 00/22 Clean up refname checks and normalizationMichael Haggerty, Sep 15, 2011
  2. 01/22 t1402: add some more testsMichael Haggerty, Sep 15, 2011
  3. 02/22 git check-ref-format: add options --allow-onelevel and --refspec-patternMichael Haggerty, Sep 15, 2011
  4. 03/22 Change bad_ref_char() to return a boolean valueMichael Haggerty, Sep 15, 2011
  5. 04/22 Change check_ref_format() to take a flags argumentMichael Haggerty, Sep 15, 2011
  6. 05/22 Refactor check_refname_format()Michael Haggerty, Sep 15, 2011
  7. 06/22 Do not allow ".lock" at the end of any refname componentMichael Haggerty, Sep 15, 2011
  8. 07/22 Make collapse_slashes() allocate memory for its resultMichael Haggerty, Sep 15, 2011
  9. 08/22 Inline function refname_format_print()Michael Haggerty, Sep 15, 2011
  10. 09/22 Change check_refname_format() to reject unnormalized refnamesMichael Haggerty, Sep 15, 2011
  11. 10/22 resolve_ref(): explicitly fail if a symlink is not readableMichael Haggerty, Sep 15, 2011
  12. 11/22 resolve_ref(): use prefixcmp()Michael Haggerty, Sep 15, 2011
  13. 12/22 resolve_ref(): only follow a symlink that contains a valid, normalized refnameMichael Haggerty, Sep 15, 2011
  14. 13/22 resolve_ref(): turn buffer into a proper string as soon as possibleMichael Haggerty, Sep 15, 2011
  15. Thomas RastSep 23, 2011
  16. Michael HaggertySep 23, 2011
  17. 1/1 get_sha1_hex(): do not read past a NUL characterMichael Haggerty, Sep 23, 2011
  18. Junio C HamanoSep 23, 2011
  19. Thomas RastOct 5, 2011
  20. Junio C HamanoOct 5, 2011
  21. 14/22 resolve_ref(): extract a function get_packed_ref()Michael Haggerty, Sep 15, 2011
  22. 15/22 resolve_ref(): do not follow incorrectly-formatted symbolic refsMichael Haggerty, Sep 15, 2011
  23. 16/22 remote: use xstrdup() instead of strdup()Michael Haggerty, Sep 15, 2011
  24. 17/22 remote: avoid passing NULL to read_ref()Michael Haggerty, Sep 15, 2011
  25. 18/22 resolve_ref(): verify that the input refname has the right formatMichael Haggerty, Sep 15, 2011
  26. 19/22 resolve_ref(): emit warnings for improperly-formatted referencesMichael Haggerty, Sep 15, 2011
  27. Jeff KingOct 11, 2011
  28. Junio C HamanoOct 11, 2011
  29. Junio C HamanoOct 11, 2011
  30. Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted referencesJunio C Hamano, Oct 11, 2011
  31. Jeff KingOct 11, 2011
  32. Junio C HamanoOct 11, 2011
  33. Jeff KingOct 11, 2011
  34. Junio C HamanoOct 12, 2011
  35. Jeff KingOct 11, 2011
  36. Junio C HamanoOct 11, 2011
  37. Jeff KingOct 12, 2011
  38. Junio C HamanoOct 12, 2011
  39. Jeff KingOct 12, 2011
  40. 1/2 refs.c: move dwim_ref()/dwim_log() from sha1_name.cJunio C Hamano, Oct 12, 2011
  41. 2/2 Restrict ref-like names immediately below $GIT_DIRJunio C Hamano, Oct 12, 2011
  42. Michael HaggertyOct 12, 2011
  43. Junio C HamanoOct 12, 2011
  44. Michael HaggertyOct 12, 2011
  45. Junio C HamanoOct 12, 2011
  46. Junio C HamanoOct 19, 2011
  47. Junio C HamanoOct 19, 2011
  48. Michael HaggertyOct 19, 2011
  49. Junio C HamanoOct 19, 2011
  50. Junio C HamanoOct 19, 2011
  51. resolve_ref(): report breakage to the caller without warningJunio C Hamano, Oct 19, 2011
  52. Michael HaggertyOct 19, 2011
  53. Junio C HamanoOct 19, 2011
  54. Jeff KingOct 12, 2011
  55. Michael HaggertyOct 12, 2011
  56. Junio C HamanoOct 12, 2011
  57. Jeff KingOct 12, 2011
  58. 20/22 resolve_ref(): also treat a too-long SHA1 as invalidMichael Haggerty, Sep 15, 2011
  59. 21/22 resolve_ref(): expand documentationMichael Haggerty, Sep 15, 2011
  60. 22/22 add_ref(): verify that the refname is formatted correctlyMichael Haggerty, Sep 15, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.