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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 10/19/2011 09:29 PM, Junio C Hamano wrote:
> 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.)

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