Re: Bug report: stash in upstream caused remote fetch to fail

I ran into the same (or similar) problem as reported in <http://www.spinics.net/lists/git/msg173391.html>. I have a script that, among other things, clones a git repository. Here's where it does that:

+ git init -q
+ git fetch -q -fu ../../../other '+refs/*:refs/*'
fatal: bad object 9b985fbe6a2b783c16756077a8be261c94b6c197
error: ../../../other did not send all necessary objects

In ../../../other:

$ git rev-parse stash@{0} 9b985fbe6a2b783c16756077a8be261c94b6c197 $ grep stash .git/packed-refs 9b985fbe6a2b783c16756077a8be261c94b6c197 refs/stash $ cat .git/refs/stash 9b985fbe6a2b783c16756077a8be261c94b6c197

I removed the line from packed-refs and removed the refs/stash file, and the problem went away. Restoring packed-refs or refs/stash makes the error occur again.

When this was reported before, Thomas Rast wrote:

I've still got the repository, and am happy to help debug this.

-- Matt

On Fri, Jan 03, 2014 at 04:12:51PM -0500, Matt Burke wrote:

I was going to ask you to send your repository, but I can easily reproduce here. I guess people don't run into it because it's uncommon to fetch the whole refs/ namespace from a non-bare repo (and bare repos do not tend to have stashes). Here's a minimal reproduction recipe:

  git init repo &&
  cd repo &&
  echo content >foo &&
  git add . &&
  git commit -m foo &&
  echo more >>foo &&
  git stash &&
  git init --bare sub &&
  cd sub &&
  git fetch .. 'refs/*:refs/*'

It looks like we are not feeding refs/stash properly to pack-objects. I'll try to take a closer look later today.

-Peff

Jeff King <peff@peff.net> writes:

I looked at this in the past and I vaguely recall that we reject it in the for-each-ref loop with check-ref-format saying "eh, that is a single-level name".

At that point I stopped digging, thinking it was a feature ;-) based on your exact observation about stash vs bare/non-bare.

On Mon, Jan 06, 2014 at 08:16:31AM -0800, Junio C Hamano wrote:

I am fine with rejecting it with a warning, but we should not then complain that the other side did not send us the object, since we should not be asking for it at all. I also do not see us complaining about the funny ref anywhere. So there is definitely _a_ bug here. :)

I think somebody else mentioned recently that we do not handle malformed refs consistently. I think it was:

  http://article.gmane.org/gmane.comp.version-control.git/239381

which might or might not be related.

-Peff

Jeff King <peff@peff.net> writes:

Oh, no question about that. I was just pointing somebody who already has volunteered to take a look in a direction I recall was where the issue was ;-)

Thanks.

On Mon, Jan 06, 2014 at 12:17:21PM -0800, Junio C Hamano wrote:

Oh, crud, did I volunteer? :)

So I found the problem, but I'm really not sure what to make of it. We do check the refname format when evaluating the refspecs, in:

    do_fetch
      get_ref_map
        get_fetch_map
          check_refname_format

Before calling it, we check that it starts with "refs/", and then pass the _whole_ refname into check_refname_format. So the latter sees "refs/stash". And that's acceptable, as it's not a single-level ref. Thus we do not get the "funny ref" message.

The code looks like this:

  if (!starts_with((*rmp)->peer_ref->name, "refs/") ||
      check_refname_format((*rmp)->peer_ref->name, 0)) {
        /* print funny ref and ignore */

Then we ask fetch_refs_via_pack to get the actual objects for us. And it checks our refs again, with this call chain:

  do_fetch
    fetch_refs
      transport_fetch_refs
        fetch_refs_via_pack
          fetch_pack
            do_fetch_pack
              everything_local
                filter_refs
                  check_refname_format

Here, the code looks like this:

  if (!memcmp(ref->name, "refs/", 5) &&
      check_refname_format(ref->name + 5, 0))
    ; /* trash */

At first I thought we are doing the same check (is it syntactically valid, and is it in "refs/"), but we're not. We are actually checking the format _only_ of stuff in "refs/", and ignoring the rest. Which really makes no sense to me.

If it were "memcmp(...) || check_refname_format()", then it would be roughly the same check. But it would still be wrong, because note that we pass only the bits under "refs/" to check_refname_format. So it sees only "stash", and then complains that it is single-level.

So the symptom we are seeing is because we are filtering with two different rulesets in different places. But I'm really not sure how to resolve it. The one in filter_refs seems nonsensical to me.

Checking _only_ things under refs/ doesn't make sense. And even if that was sensible, feeding half a ref to check_refname_format does not work. In addition to the single-level check, it has other rules that want to see the whole ref (e.g., the ref "@" is not valid, but "refs/@" is OK; it cannot distinguish them without seeing the prefix).

So I can see two options:

  1. Make the check in filter_refs look like the one in get_fetch_map.
     That at least makes them the same, which alleviates the symptom.
     But we still are running two checks, and if they ever get out of
     sync, it causes problems.

  2. Abolish the check in filter_refs. I think this makes the most sense
     from the perspective of fetch, because we will already have cleaned up
     the ref list. But we might need to leave the filtering in place for
     people who call fetch-pack as a bare plumbing command.

It's really not clear to me what the check in filter_refs was trying to do. It dates all the way back to 1baaae5 (Make maximal use of the remote refs, 2005-10-28), but there is not much explanation. I haven't dug into the list around that time to see if there's any discussion.

-Peff

Jeff King <peff@peff.net> writes:

I think this should feed ref->name, not ref->name + 5; an obvious alternative would be to use REFNAME_ALLOW_ONELEVEL; you are also right that && is probably a misspelt ||; this is about protecting ourselves from creating garbage in our ref namespace, so accepting anything outside refs/ makes little sense.

I think the "funny refs" the log message talks about is about filtering "we know we can reach these objects via our alternates, but these are not refs we actually have".

Junio C Hamano <gitster@pobox.com> writes:

Actually, I think the above recollection of mine was completely bogus. The && is there because we do allow things like "HEAD" (they are the funny ones) as well as things inside refs/, and the latter is the only thing we had a check-ref-format to dictate the format when the code was written.

I do not mind tightening things a bit (e.g. outside refs/, only allow HEAD and nothing else). A good first step might be to enforce allow-onelevel outside refs/ (so that we can allow "HEAD") and for those inside refs/ check without allow-onelevel but without skipping the prefix.

It is a separate story if it makes much sense to allow fetching refs/stash or ignoring when running "git clone". Operationally, I think it makes more sense to ignore refs/stash, not because it is a one-level name, but because what a stash is.

On Mon, Jan 06, 2014 at 07:19:43PM -0800, Junio C Hamano wrote:

This discussion stalled, but I finally got around to looking at it today. I agree that we should leave aside more complex policy for now, and start with bringing the "fetch" and "fetch-pack" filters into harmony. That turns off format-checking for things outside "refs/" (so allows "HEAD"), and checks the whole string for things inside "refs/" (so it does not fall afoul of the one-level check).

I ended up with the patch below, which converts fetch-pack to match fetch. However, reading the original one-level check in 03feddd (git-check-ref-format: reject funny ref names., 2005-10-13), it does seem like it was meant to reject "refs/foo". It contains:

        if (level < 2)
                return -1; /* at least of form "heads/blah" */

It seems like the meaning of this check changed over the years. I am not sure if that was intentional or not. :)

So we could go the other direction, and harmonize on disallowing "refs/foo". I don't particularly care that much. The nasty thing now is the mismatch between the two spots, which means that "fetch" doesn't just ignore the ref, but bombs out with a missing object.

-- >8 --
Subject: fetch-pack: do not filter out one-level refs

Currently fetching a one-level ref like "refs/foo" does not work consistently. The outer "git fetch" program filters the list of refs, checking each against check_refname_format. Then it feeds the result to do_fetch_pack to actually negotiate the haves/wants and get the pack. The fetch-pack code does its own filter, and it behaves differently.

The fetch-pack filter looks for refs in "refs/", and then feeds everything _after_ the slash (i.e., just "foo") into check_refname_format. But check_refname_format is not designed to look at a partial refname. It complains that the ref has only one component, thinking it is at the root (i.e., alongside "HEAD"), when in reality we just fed it a partial refname.

As a result, we omit a ref like "refs/foo" from the pack request, even though "git fetch" then tries to store the resulting ref. If we happen to get the object anyway (e.g., because the ref is contained in another ref we are fetching), then the fetch succeeds. But if it is a unique object, we fail when trying to update "refs/foo".

We can fix this by just passing the whole refname into check_refname_format; we know the part we were omitting is "refs/", which is acceptable in a refname. This at least makes the checks consistent with each other.

This problem happens most commonly with "refs/stash", which is the only one-level ref in wide use. However, our test does not use "refs/stash", as we may later want to restrict it specifically (not because it is one-level, but because of the semantics of stashes).

We may also want to do away with the multiple levels of filtering (which can cause problems when they are out of sync), or even forbid one-level refs entirely. However, those decisions can come later; this fixes the most immediate problem, which is the mismatch between the two.

Signed-off-by: Jeff King <peff@peff.net>
---
 fetch-pack.c     |  2 +-
 t/t5510-fetch.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

On Wed, Jan 15, 2014 at 05:46:13AM -0500, Jeff King wrote:

By the way, an interesting implication of this is that I do not think there is any format check on things outside of refs/. If you were to do

  git fetch ... +*:*

you would write whatever crap the other side gives you. I can't imagine any reason a client would _ever_ do that, though, so I don't think it's a big deal. We tend to fetch HEAD explicitly by name, and that's it.

-Peff