Bug report: stash in upstream caused remote fetch to fail

Bug report: stash in upstream caused remote fetch to fail

[Andrew Walrond]

A bit of cut and paste will explain better than me...

LOCAL $ git remote update
    Fetching origin
    remote: Counting objects: 25, done.
    remote: Compressing objects: 100% (12/12), done.
    remote: Total 13 (delta 10), reused 0 (delta 0)
    Unpacking objects: 100% (13/13), done.
    fatal: bad object fa0da15b2ea5cc3e4eb9ed414b99d6a9d7da7864
    error: git://git.heresymail.org/lib%2Fmpfr did not send all necessary
    objects

UPSTREAM $ git fsck
    dangling blob ded848b21db04fcadf77a4a5d9f81955b4315c9f
    dangling blob 9c3976919b3cee56eabc3c9c9dfe5d223ce32686
    dangling blob e17ab25a3a91bed830ddb06da4af1132434d5ee4
    dangling blob 20a612ab361058838f680d72c1f4f8cb462ce1a2

UPSTREAM $ git gc
    Counting objects: 974, done.
    Delta compression using up to 8 threads.
    Compressing objects: 100% (954/954), done.
    Writing objects: 100% (974/974), done.
    Total 974 (delta 572), reused 0 (delta 0)

LOCAL $ git remote update
    Fetching origin
    remote: Counting objects: 25, done.
    remote: Compressing objects: 100% (10/10), done.
    remote: Total 13 (delta 3), reused 11 (delta 2)
    Unpacking objects: 100% (13/13), done.
    fatal: bad object fa0da15b2ea5cc3e4eb9ed414b99d6a9d7da7864
    error: git://git.heresymail.org/lib%2Fmpfr did not send all necessary
    objects

LOCAL $ DELETE LOCAL REPO

LOCAL $ git clone <upstream>
    Cloning into bare repository '/src/lib/mpfr'...
    remote: Counting objects: 972, done.
    remote: Compressing objects: 100% (382/382), done.
    remote: Total 972 (delta 570), reused 971 (delta 570)
    Receiving objects: 100% (972/972), 2.01 MiB, done.
    Resolving deltas: 100% (570/570), done.
    error: refs/stash does not point to a valid object!

UPSTREAM $ git stash list
    stash@{0}: WIP on (no branch): f648dd0 Import 3.1.0 from tarball

UPSTREAM $ git stash clear

LOCAL $ DELETE LOCAL REPO

LOCAL $ git clone <upstream>
    Cloning into bare repository '/src/lib/mpfr'...
    remote: Counting objects: 972, done.
    remote: Compressing objects: 100% (382/382), done.
    remote: Total 972 (delta 570), reused 971 (delta 570)
    Receiving objects: 100% (972/972), 2.01 MiB, done.
    Resolving deltas: 100% (570/570), done.

UPSTREAM $ git version
    git version 1.7.8.2

LOCAL $ git version
    git version 1.7.8.2

Hope that's useful!

Andrew Walrond

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

[Andrew Walrond]

On Wed, Feb 01, 2012 at 09:46:48PM +0100, Thomas Rast wrote:
> 
> Do you, by any chance, still have a copy of the upstream repo before you
> trashed the stash?  It would be interesting to know whether there was
> actually some repository corruption going on (that went unnoticed by
> fsck, no less) or if there was a bug in the transmission.

I tried to reproduce the problem but without success :(
Disk corruption is unlikely - raided drives etc etc. More likely stash
related somehow as I was applying patches manually on older versions
checked out in a temp branch, stashing the results then applying the
stash onto master/HEAD.
But why was a remote clone involved with the upstream stash at all I
wonder??

Andrew Walrond

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

[Matt Burke]

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:

> Do you, by any chance, still have a copy of the upstream repo before you
> trashed the stash?  It would be interesting to know whether there was
> actually some repository corruption going on (that went unnoticed by
> fsck, no less) or if there was a bug in the transmission.

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

--
Matt

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 03, 2014 at 04:12:51PM -0500, Matt Burke wrote:
>
>> + git init -q
>> + git fetch -q -fu ../../../other '+refs/*:refs/*'
>> fatal: bad object 9b985fbe6a2b783c16756077a8be261c94b6c197
>> error: ../../../other did not send all necessary objects
>
> 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.

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.

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

[Jeff King]

On Mon, Jan 06, 2014 at 08:16:31AM -0800, Junio C Hamano 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.
> 
> 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.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 06, 2014 at 08:16:31AM -0800, Junio C Hamano 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.
>> 
>> 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.
>
> 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. :)

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.

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

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

[Jeff King]

On Mon, Jan 06, 2014 at 12:17:21PM -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. :)
> 
> 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 ;-)

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

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

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

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

[Junio C Hamano]

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

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

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.

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

[Jeff King]

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

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

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(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 760ed16..b28ccd1 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -505,7 +505,7 @@ static void filter_refs(struct fetch_pack_args *args,
 		next = ref->next;
 
 		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_refname_format(ref->name + 5, 0))
+		    check_refname_format(ref->name, 0))
 			; /* trash */
 		else {
 			while (i < nr_sought) {
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 12674ac..ab28594 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -640,4 +640,15 @@ test_expect_success 'branchname D/F conflict resolved by --prune' '
 	test_cmp expect actual
 '
 
+test_expect_success 'fetching a one-level ref works' '
+	test_commit extra &&
+	git reset --hard HEAD^ &&
+	git update-ref refs/foo extra &&
+	git init one-level &&
+	(
+		cd one-level &&
+		git fetch .. HEAD refs/foo
+	)
+'
+
 test_done
-- 
1.8.5.2.500.g8060133

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

[Jeff King]

On Wed, Jan 15, 2014 at 05:46:13AM -0500, Jeff King 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).

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