# Re: Bug report: stash in upstream caused remote fetch to fail 10 messages from 2014-01-03 to 2014-01-15. Participants: Matt Burke, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/35605 ## Matt Burke, 2014-01-03 21:12 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: URL: https://gitlist.dev/e/CAECVvXYD69Xrp85bVJg6XogvctUTwrFDae%2B3tawFgoCAUGhwGg%40mail.gmail.com ``` I ran into the same (or similar) problem as reported in . 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 ``` ## Jeff King, 2014-01-06 15:27 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: <20140106152742.GA26221@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140106152742.GA26221%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2014-01-06 16:16 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: URL: https://gitlist.dev/e/xmqqbnzpkrvk.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140106152742.GA26221@sigill.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2014-01-06 19:36 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: <20140106193625.GA27062@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140106193625.GA27062%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2014-01-06 20:17 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: URL: https://gitlist.dev/e/xmqq61pwj25q.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140106193625.GA27062@sigill.intra.peff.net> ``` Jeff King 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 ``` ## Jeff King, 2014-01-06 23:03 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: <20140106230348.GA7811@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140106230348.GA7811%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2014-01-06 23:22 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: URL: https://gitlist.dev/e/xmqq4n5ghf0z.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140106230348.GA7811@sigill.intra.peff.net> ``` Jeff King 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". ``` ## Junio C Hamano, 2014-01-07 03:19 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: <7vtxdg1nsg.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtxdg1nsg.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Junio C Hamano 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. ``` ## Jeff King, 2014-01-15 10:46 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: <20140115104612.GE14335@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140115104612.GE14335%40sigill.intra.peff.net In-Reply-To: <7vtxdg1nsg.fsf@alter.siamese.dyndns.org> ``` 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 --- 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 ``` ## Jeff King, 2014-01-15 10:48 Subject: Re: Bug report: stash in upstream caused remote fetch to fail Message-ID: <20140115104826.GF14335@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140115104826.GF14335%40sigill.intra.peff.net In-Reply-To: <20140115104612.GE14335@sigill.intra.peff.net> ``` 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 ```