Re: Bug: git-upload-pack will return successfully even when it can't read all references

> On Mon, Sep 07, 2015 at 02:11:15PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> This turned out to be a pretty trivial filesystem error.
>> refs/heads/master wasn't readable by the backup process, but some
>> other stuff in refs/heads and objects/* was.
>>
>> [...]
>>
>> I wanted to check if this was a regression and got as far back as
>> v1.4.3 with the same behavior before the commands wouldn't work
>> anymore due to changes in the git config parsing code.
>
> Right, it has basically always been this way. for_each_ref() silently
> eats oddities or errors while reading refs. Calling for_each_rawref()
> will include them, but we don't do it in most places; it would make
> non-critical operations on a corrupted repo barf.  And it is difficult
> to know what is "critical" inside the code. You might be calling
> "upload-pack" to salvage what you can from a corrupted repo, or to make
> a backup where you want to know what is corrupted and what is not.
>
> Commit 49672f2 introduced a "ref paranoia" environment variable to let
> you specify this (and robust backups was definitely one of the use cases
> I had in mind). It's a little tricky to use with upload-pack because you
> may be crossing an ssh boundary, but:
>
>   git clone -u 'GIT_REF_PARANOIA=1 git-upload-pack' ...
>
> should work.
>
> With your case:
>
>   $ git clone --no-local -u 'GIT_REF_PARANOIA=1 git-upload-pack' repo repo-checkout
>   Cloning into 'repo-checkout'...
>   fatal: git upload-pack: not our ref 0000000000000000000000000000000000000000
>   fatal: The remote end hung up unexpectedly
>
> Without "--no-local" it behaves weirdly, but I would not recommend local
> clones in general if you are trying to be careful. They optimize out a
> lot of the safety checks, and we do things like copy the packed-refs
> file wholesale.
>
> And certainly the error message is not the greatest. upload-pack is not
> checking for the REF_ISBROKEN flag, so it just dumps:
>
>   0000000000000000000000000000000000000000 refs/heads/master
>
> in the advertisement, and the client happily requests that object.
> REF_PARANOIA is really just a band-aid to feed the broken refs to the
> normal code paths, which typically barf on their own. :)
>
> Something like this:
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 89e832b..3c621a5 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -731,6 +731,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
>         if (mark_our_ref(refname, oid))
>                 return 0;
>
> +       if (flag & REF_ISBROKEN)
> +               warning("remote ref '%s' is broken", refname);
> +
>         if (capabilities) {
>                 struct strbuf symref_info = STRBUF_INIT;
>
> kind of helps, but the advertisement is too early for us to send
> sideband messages. So it makes it to the user if the transport is local
> or ssh, but not over git:// or http.
>
> That's something we could do better with protocol v2 (we'll negotiate
> capabilities before the advertisement).