Bad objects error since upgrading GitHub servers to 1.6.1

Bad objects error since upgrading GitHub servers to 1.6.1

[PJ Hyett]

Hi folks,

We upgraded our servers to Git 1.6.1 yesterday and almost immediately
starting hearing reports of "Fatal: Bad Object Error." I have
experienced this myself, so I'm 99% certain this isn't user error. I'm
also using 1.6.1 locally.

I ran into this error after trying to push code to GitHub after a
series of simple commits, I was doing absolutely nothing out of the
ordinary.

Please see our support thread for more examples:
http://support.github.com/discussions/feature-requests/157-fatal-bad-object-error-when-doing-simple-push

All of the error messages are the same. Can anyone please shed some
light on this, I don't see any other recourse but to downgrade Git
until this is resolved.

Thanks,
PJ

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[PJ Hyett]

To expand further, here's the output from the command line when this happened.

~/Development/github(jetty)$ git push pjhyett jetty
fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820
error: pack-objects died with strange error
error: failed to push some refs to 'git@github.com:pjhyett/github.git'

~/Development/github(jetty)$ git fsck --full
warning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains
zero-padded file modes
warning in tree 56fe2a1a3da446606aadf8861feccd592b636a34: contains
zero-padded file modes
warning in tree 99e2a89db2aa9846fc2491b3e4ccd8861e8d3283: contains
zero-padded file modes
warning in tree a6e532d7451bc4aadab86ade84df69180fab4765: contains
zero-padded file modes
dangling blob 43611213c3eff91e5fe071cf2907f69a99b630b2
dangling commit b28b3ecd85a04ecbd1dcb8aedc6886a465f6ab18
dangling commit 13a70c8687527936d2c375f0f7aefe71142de3c7
dangling commit 2aa94c1199cb332f58b70c6ce19d8de3c45c6f3c
dangling blob 61b910e7a97600691fd279e4db3662e751fb5fb7
dangling commit c4f19e16208d59666323ae0575435720be9b865d
dangling commit 19245f5d77aa449eebb4a0521b5ff4f6ce1865ab
dangling commit 122995fb7c9a7e459b0801e0647eb918bea878bf
dangling commit 7d51e3926b8720d1c7cad19aeb35d6ab4af755fd
dangling commit 1162dd21370439416967a34915832125e4975239
dangling blob 8c630b66927f6022a72e457be308de5c9ad9f4e6
dangling blob 827d4d8855fe6a3a7856ea35cd641192140f2dcd
dangling commit c9824506855d6cad9b52df115aa267d70872c2cc
dangling blob fb9bbfc3aa17c5d1ae4e15c862bd874e3476fcfc
dangling commit 46a4b39245a58ad867010f272991d6233db6288b
dangling commit d6bf5f30853fecea745559dc3a718113f3619634
dangling blob d4d66fc4c3a2cbc94d8ed9cb30a6b56daa86e58f
dangling commit b4f8d7766e8905e5ac6d6cfeeaf7370a716c24a2

Very odd that the bad object didn't appear in the fsck output.

I was able to fix the error by copying a non-corrupted version of the
object back into .git/objects and then running a git fetch.

-PJ

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, PJ Hyett wrote:

> To expand further, here's the output from the command line when this happened.
> 
> ~/Development/github(jetty)$ git push pjhyett jetty
> fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820
> error: pack-objects died with strange error
> error: failed to push some refs to 'git@github.com:pjhyett/github.git'
> 
> ~/Development/github(jetty)$ git fsck --full
> warning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains
> zero-padded file modes
> warning in tree 56fe2a1a3da446606aadf8861feccd592b636a34: contains
> zero-padded file modes
> warning in tree 99e2a89db2aa9846fc2491b3e4ccd8861e8d3283: contains
> zero-padded file modes
> warning in tree a6e532d7451bc4aadab86ade84df69180fab4765: contains
> zero-padded file modes
> dangling blob 43611213c3eff91e5fe071cf2907f69a99b630b2
> dangling commit b28b3ecd85a04ecbd1dcb8aedc6886a465f6ab18
> dangling commit 13a70c8687527936d2c375f0f7aefe71142de3c7
> dangling commit 2aa94c1199cb332f58b70c6ce19d8de3c45c6f3c
> dangling blob 61b910e7a97600691fd279e4db3662e751fb5fb7
> dangling commit c4f19e16208d59666323ae0575435720be9b865d
> dangling commit 19245f5d77aa449eebb4a0521b5ff4f6ce1865ab
> dangling commit 122995fb7c9a7e459b0801e0647eb918bea878bf
> dangling commit 7d51e3926b8720d1c7cad19aeb35d6ab4af755fd
> dangling commit 1162dd21370439416967a34915832125e4975239
> dangling blob 8c630b66927f6022a72e457be308de5c9ad9f4e6
> dangling blob 827d4d8855fe6a3a7856ea35cd641192140f2dcd
> dangling commit c9824506855d6cad9b52df115aa267d70872c2cc
> dangling blob fb9bbfc3aa17c5d1ae4e15c862bd874e3476fcfc
> dangling commit 46a4b39245a58ad867010f272991d6233db6288b
> dangling commit d6bf5f30853fecea745559dc3a718113f3619634
> dangling blob d4d66fc4c3a2cbc94d8ed9cb30a6b56daa86e58f
> dangling commit b4f8d7766e8905e5ac6d6cfeeaf7370a716c24a2
> 
> Very odd that the bad object didn't appear in the fsck output.
> 
> I was able to fix the error by copying a non-corrupted version of the
> object back into .git/objects and then running a git fetch.

Hmm.  The only thing I could think of is that the pack-objects used by 
your git-daemon is somehow not at the right version...

Do you have copies of the "corrupt" objects?

Ciao,
Dscho

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Tue, 27 Jan 2009, PJ Hyett wrote:
> 
> > To expand further, here's the output from the command line when this happened.
> > 
> > ~/Development/github(jetty)$ git push pjhyett jetty
> > fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820
> > error: pack-objects died with strange error
> > error: failed to push some refs to 'git@github.com:pjhyett/github.git'
> 
> Hmm.  The only thing I could think of is that the pack-objects used by 
> your git-daemon is somehow not at the right version...

No, that's pack-objects on the client.

Its freaking weird.  I don't know why a server side upgrade would
cause this on the client side.

FWIW, in 1.6.1 the only mention of those bad object messages
is inside revision.c.  I can't see why we'd get one of those
by itself.  I would have expected messages from deeper down
too, like from sha1_file.c.
 
-- 
Shawn.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> On Tue, 27 Jan 2009, PJ Hyett wrote:
>> 
>> > To expand further, here's the output from the command line when this happened.
>> > 
>> > ~/Development/github(jetty)$ git push pjhyett jetty
>> > fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820
>> > error: pack-objects died with strange error
>> > error: failed to push some refs to 'git@github.com:pjhyett/github.git'
>> 
>> Hmm.  The only thing I could think of is that the pack-objects used by 
>> your git-daemon is somehow not at the right version...
>
> No, that's pack-objects on the client.
>
> Its freaking weird.  I don't know why a server side upgrade would
> cause this on the client side.
>
> FWIW, in 1.6.1 the only mention of those bad object messages
> is inside revision.c.  I can't see why we'd get one of those
> by itself.  I would have expected messages from deeper down
> too, like from sha1_file.c.

As we do not know what version github used to run (or for that matter what
custom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6
and did some comparison.  The client side pack_object() learned to take
alternates on the server side into account to avoid pushing objects that
the target repository has through its alternates, so it is not totally
unexpected the client side changes its behaviour depending on what the
server does.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[PJ Hyett]

On Tue, Jan 27, 2009 at 3:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>>> On Tue, 27 Jan 2009, PJ Hyett wrote:
>>>
>>> > To expand further, here's the output from the command line when this happened.
>>> >
>>> > ~/Development/github(jetty)$ git push pjhyett jetty
>>> > fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820
>>> > error: pack-objects died with strange error
>>> > error: failed to push some refs to 'git@github.com:pjhyett/github.git'
>>>
>>> Hmm.  The only thing I could think of is that the pack-objects used by
>>> your git-daemon is somehow not at the right version...
>>
>> No, that's pack-objects on the client.
>>
>> Its freaking weird.  I don't know why a server side upgrade would
>> cause this on the client side.
>>
>> FWIW, in 1.6.1 the only mention of those bad object messages
>> is inside revision.c.  I can't see why we'd get one of those
>> by itself.  I would have expected messages from deeper down
>> too, like from sha1_file.c.
>
> As we do not know what version github used to run (or for that matter what
> custom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6
> and did some comparison.  The client side pack_object() learned to take
> alternates on the server side into account to avoid pushing objects that
> the target repository has through its alternates, so it is not totally
> unexpected the client side changes its behaviour depending on what the
> server does.

Our servers were upgraded from 1.5.5.1 if that helps.

-PJ

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[PJ Hyett]

> As we do not know what version github used to run (or for that matter what
> custom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6
> and did some comparison.  The client side pack_object() learned to take
> alternates on the server side into account to avoid pushing objects that
> the target repository has through its alternates, so it is not totally
> unexpected the client side changes its behaviour depending on what the
> server does.

The only custom code we've written was a patch to git-daemon to map
pjhyett/github.git to a sharded location (eg.
/repositories/1/1e/df/a0/pjhyett/github.git) instead of the default.

The new alternates code in 1.6.1 sounds like that could be the issue.

-PJ

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Linus Torvalds]

On Tue, 27 Jan 2009, PJ Hyett wrote:
> 
> ~/Development/github(jetty)$ git fsck --full
> warning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains zero-padded file modes
> ..

Ouch. This is unrelated to your issue, but I'm wondering what project 
contains these invalid trees, and how they were created.

Zero-padded tree entries can cause "object aliases", ie two trees that 
have logically the same contents end up with different data (due to 
different amounts of padding) and thus different SHA1's. It shouldn't be 
serious per se, but it's somethign that really shouldn't happen.

What project does it come from, and how did such a tree get generated?

			Linus

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

PJ Hyett <pjhyett@gmail.com> writes:

>> As we do not know what version github used to run (or for that matter what
>> custom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6
>> and did some comparison.  The client side pack_object() learned to take
>> alternates on the server side into account to avoid pushing objects that
>> the target repository has through its alternates, so it is not totally
>> unexpected the client side changes its behaviour depending on what the
>> server does.
>
> The only custom code we've written was a patch to git-daemon to map
> pjhyett/github.git to a sharded location (eg.
> /repositories/1/1e/df/a0/pjhyett/github.git) instead of the default.
>
> The new alternates code in 1.6.1 sounds like that could be the issue.

It could be.

With the old server, when project A has a forked project A1, and A1
borrows (via alternates) objects from A, pushing into A1 did not look at
refs in A's repository (this all happens on the server end).

With the new server, the server side also advertises the tips of A's
branches as commits that are fully connected, when the client side tries
to push into A1.  Older clients ignored this advertisement, so when they
pushed into A1, because their push did not depend on what's in repository
A on the server end, did not get affected if repository A (not A1) is
corrupted.  A new client talking to the server would be affected because
it believes what the server says.

Older client ignores this advertisement, so if you are seeing trouble
reports from people who use older clients, then you can dismiss this
conjecture as unrelated.  But if you see the issue only from people with
new clients, this could be just exposing a repository corruption of A (not
A1) on the server end that people did not know about before.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Björn Steinbrink]

On 2009.01.27 17:00:54 -0800, Linus Torvalds wrote:
> 
> 
> On Tue, 27 Jan 2009, PJ Hyett wrote:
> > 
> > ~/Development/github(jetty)$ git fsck --full
> > warning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains zero-padded file modes
> > ..
> 
> Ouch. This is unrelated to your issue, but I'm wondering what project 
> contains these invalid trees, and how they were created.
> 
> Zero-padded tree entries can cause "object aliases", ie two trees that 
> have logically the same contents end up with different data (due to 
> different amounts of padding) and thus different SHA1's. It shouldn't be 
> serious per se, but it's somethign that really shouldn't happen.
> 
> What project does it come from, and how did such a tree get generated?

I guess that's still from their webinterface that allows to edit file
directly, without having a clone ofthe repo. The initial(?) version used
to create such broken objects. It also got the order of entries in a
tree object wrong IIRC. Back then, Scott and myself tracked that down on
#git, to their ruby(?) stuff that creates the objects. But maybe the
breakage is back?

Björn

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

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

> PJ Hyett <pjhyett@gmail.com> writes:
> ...
>> The new alternates code in 1.6.1 sounds like that could be the issue.
>
> It could be.
>
> With the old server, when project A has a forked project A1, and A1
> borrows (via alternates) objects from A, pushing into A1 did not look at
> refs in A's repository (this all happens on the server end).
>
> With the new server, the server side also advertises the tips of A's
> branches as commits that are fully connected, when the client side tries
> to push into A1.  Older clients ignored this advertisement, so when they
> pushed into A1, because their push did not depend on what's in repository
> A on the server end, did not get affected if repository A (not A1) is
> corrupted.  A new client talking to the server would be affected because
> it believes what the server says.
>
> Older client ignores this advertisement, so if you are seeing trouble
> reports from people who use older clients, then you can dismiss this
> conjecture as unrelated.  But if you see the issue only from people with
> new clients, this could be just exposing a repository corruption of A (not
> A1) on the server end that people did not know about before.

The extra "we also have these" advertisement happened as a result of this
discussion:

    http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256

I think I know what is going on.

Consider this sequence of events.

 (0) Alice creates a project and pushes to public.

    alice$ cd $HOME/existing-tarball-extract
    alice$ git init
    alice$ git add .
    alice$ git push /pub/alice.git master
    

 (1) Bob forks it.

    bob$ git clone --bare --reference /pub/alice.git /pub/bob.git

 (2) Bob clones his.

    bob$ cd $HOME && git clone /pub/bob.git bob

 (3) Alice works more and pushes

    alice$ edit foo
    alice$ git add foo
    alice$ git commit -a -m 'more'
    alice$ git push /pub/alice.git master

 (4) Bob works more and tries to push to his.

    bob$ cd $HOME/bob
    bob$ edit bar
    bob$ git add bar
    bob$ git commit -a -m 'yet more'
    bob$ git push /pub/bob.git master

Now, the new server advertises the objects reachable from alice's branch
tips as usable cut-off points for pack-objects bob will run when sending.

And new builtin-send-pack.c has new code that feeds "extra" refs as

	^SHA1\n

to the pack-objects process.

The latest commit Alice created and pushed into her repository is one such
commit.

But the problem is that Bob does *NOT* have it.  His "push" will run pack
object telling it that objects reachable from Alice's top commit do not
have to be sent, which was the whole point of doing this new "we also have
these" advertisement, but instead of ignoring that unknown commit,
pack-objects would say "Huh?  I do not even know that commit" and dies.

This can and should be solved by client updates, as 1.6.1 server can work
with older client just fine.

[PATCH] send-pack: Filter unknown commits from alternates of the remote

[Björn Steinbrink]

Since 40c155ff14c, receive-pack on the remote also sends refs from its
alternates. Unfortunately, we don't filter commits that don't exist in the
local repository from that list.  This made us pass those unknown commits
to pack-objects, causing it to fail with a "bad object" error.

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
---
 builtin-send-pack.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index a9fdbf9..10d7016 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -52,11 +52,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	 * parameters by writing to the pipe.
 	 */
 	for (i = 0; i < extra->nr; i++) {
-		memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
-		buf[0] = '^';
-		buf[41] = '\n';
-		if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
-			break;
+		if (!is_null_sha1(&extra->array[i][0]) &&
+		    has_sha1_file(&extra->array[i][0])) {
+			memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
+			buf[0] = '^';
+			buf[41] = '\n';
+			if (!write_or_whine(po.in, buf, 42,
+						"send-pack: send refs"))
+				break;
+		}
 	}
 
 	while (refs) {
-- 
1.6.1.284.g5dc13.dirty

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

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

> The extra "we also have these" advertisement happened as a result of this
> discussion:
>
>     http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256
>
> I think I know what is going on.
>
> Consider this sequence of events.
>
>  (0) Alice creates a project and pushes to public.
>
>     alice$ cd $HOME/existing-tarball-extract
>     alice$ git init
>     alice$ git add .
>     alice$ git push /pub/alice.git master
>     
>
>  (1) Bob forks it.
>
>     bob$ git clone --bare --reference /pub/alice.git /pub/bob.git

I need another /pub/alice.git here, I think, but I hope I got the point
across to people who are capable of helping us to resolve this issue.

>
>  (2) Bob clones his.
>
>     bob$ cd $HOME && git clone /pub/bob.git bob
>
>  (3) Alice works more and pushes
>
>     alice$ edit foo
>     alice$ git add foo
>     alice$ git commit -a -m 'more'
>     alice$ git push /pub/alice.git master
>
>  (4) Bob works more and tries to push to his.
>
>     bob$ cd $HOME/bob
>     bob$ edit bar
>     bob$ git add bar
>     bob$ git commit -a -m 'yet more'
>     bob$ git push /pub/bob.git master
>
> Now, the new server advertises the objects reachable from alice's branch
> tips as usable cut-off points for pack-objects bob will run when sending.
>
> And new builtin-send-pack.c has new code that feeds "extra" refs as
>
> 	^SHA1\n
>
> to the pack-objects process.
>
> The latest commit Alice created and pushed into her repository is one such
> commit.
>
> But the problem is that Bob does *NOT* have it.  His "push" will run pack
> object telling it that objects reachable from Alice's top commit do not
> have to be sent, which was the whole point of doing this new "we also have
> these" advertisement, but instead of ignoring that unknown commit,
> pack-objects would say "Huh?  I do not even know that commit" and dies.
>
> This can and should be solved by client updates, as 1.6.1 server can work
> with older client just fine.

Here is a *wrong* fix that should work most of the time.  It will
certainly appear to fix the issue in the above reproduction recipe.
You may want to ask your users to try this to see if it makes their
symptom disappear.

When we receive ".have" advertisement, this wrong fix checks if that
object is available locally, and it ignores it otherwise.

This won't be acceptable as the official fix.  We should be doing the
full connectivity check; in other words, not just "do we have it", but "do
we have it *and* is it reachable from any of our own refs".

 connect.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git c/connect.c w/connect.c
index 2f23ab3..8026850 100644
--- c/connect.c
+++ w/connect.c
@@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
 
 static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)
 {
+	if (!has_sha1_file(sha1))
+		return;
+
 	ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);
 	hashcpy(&(extra->array[extra->nr][0]), sha1);
 	extra->nr++;

Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> Since 40c155ff14c, receive-pack on the remote also sends refs from its
> alternates. Unfortunately, we don't filter commits that don't exist in the
> local repository from that list.  This made us pass those unknown commits
> to pack-objects, causing it to fail with a "bad object" error.

Yeah, it is a step in the right direction, but is the *wrong* fix I
described in my previous message.

Our mails crossed ;-)

And I think we should have this in the place where we receive .have,
i.e. inside add_extra_have() in connect.c

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[PJ Hyett]

On Tue, Jan 27, 2009 at 5:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The extra "we also have these" advertisement happened as a result of this
>> discussion:
>>
>>     http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256
>>
>> I think I know what is going on.
>>
>> Consider this sequence of events.
>>
>>  (0) Alice creates a project and pushes to public.
>>
>>     alice$ cd $HOME/existing-tarball-extract
>>     alice$ git init
>>     alice$ git add .
>>     alice$ git push /pub/alice.git master
>>
>>
>>  (1) Bob forks it.
>>
>>     bob$ git clone --bare --reference /pub/alice.git /pub/bob.git
>
> I need another /pub/alice.git here, I think, but I hope I got the point
> across to people who are capable of helping us to resolve this issue.
>
>>
>>  (2) Bob clones his.
>>
>>     bob$ cd $HOME && git clone /pub/bob.git bob
>>
>>  (3) Alice works more and pushes
>>
>>     alice$ edit foo
>>     alice$ git add foo
>>     alice$ git commit -a -m 'more'
>>     alice$ git push /pub/alice.git master
>>
>>  (4) Bob works more and tries to push to his.
>>
>>     bob$ cd $HOME/bob
>>     bob$ edit bar
>>     bob$ git add bar
>>     bob$ git commit -a -m 'yet more'
>>     bob$ git push /pub/bob.git master
>>
>> Now, the new server advertises the objects reachable from alice's branch
>> tips as usable cut-off points for pack-objects bob will run when sending.
>>
>> And new builtin-send-pack.c has new code that feeds "extra" refs as
>>
>>       ^SHA1\n
>>
>> to the pack-objects process.
>>
>> The latest commit Alice created and pushed into her repository is one such
>> commit.
>>
>> But the problem is that Bob does *NOT* have it.  His "push" will run pack
>> object telling it that objects reachable from Alice's top commit do not
>> have to be sent, which was the whole point of doing this new "we also have
>> these" advertisement, but instead of ignoring that unknown commit,
>> pack-objects would say "Huh?  I do not even know that commit" and dies.
>>
>> This can and should be solved by client updates, as 1.6.1 server can work
>> with older client just fine.
>
> Here is a *wrong* fix that should work most of the time.  It will
> certainly appear to fix the issue in the above reproduction recipe.
> You may want to ask your users to try this to see if it makes their
> symptom disappear.
>
> When we receive ".have" advertisement, this wrong fix checks if that
> object is available locally, and it ignores it otherwise.
>
> This won't be acceptable as the official fix.  We should be doing the
> full connectivity check; in other words, not just "do we have it", but "do
> we have it *and* is it reachable from any of our own refs".
>
>  connect.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git c/connect.c w/connect.c
> index 2f23ab3..8026850 100644
> --- c/connect.c
> +++ w/connect.c
> @@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
>
>  static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)
>  {
> +       if (!has_sha1_file(sha1))
> +               return;
> +
>        ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);
>        hashcpy(&(extra->array[extra->nr][0]), sha1);
>        extra->nr++;
>

Thank you for your detailed response. To answer your previous
question, all of the bug reports have been made by users running
1.6.1.

My concern is that we obviously have no control over what version of
Git our 50k+ users are running, and we will be perpetually stuck
running 1.5 on the servers to account for this issue.

Is there any possibility to have the server code in an upcoming
release account for clients running 1.6.1?

-PJ

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Shawn O. Pearce]

PJ Hyett <pjhyett@gmail.com> wrote:
> 
> Is there any possibility to have the server code in an upcoming
> release account for clients running 1.6.1?

I can't think off-hand of a way for the server to know what version
the client is.  There's nothing really different in the protocol
between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of
include-tag) or later client.

So there's no easy way for the server to work around this possible
glitch in the client.

-- 
Shawn.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> PJ Hyett <pjhyett@gmail.com> wrote:
>> 
>> Is there any possibility to have the server code in an upcoming
>> release account for clients running 1.6.1?
>
> I can't think off-hand of a way for the server to know what version
> the client is.  There's nothing really different in the protocol
> between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of
> include-tag) or later client.

Hmm, I am puzzled.

I do not know how 41fa7d2 (Teach git-fetch to exploit server side
automatic tag following, 2008-03-03), which is about the conversation
between fetch-pack and upload-pack, is relevant to the issue at hand,
which is about the conversation between send-pack and receive-pack.

In send-pack receive-pack protocol, the server talks first before
listening to the client, and the .have data is in this first part of the
conversation.

By the way, I think Documentation/technical/pack-protocol.txt needs to be
updated.  send-pack receive-pack protocol uses C and S to mean receiver
and sender respectively.  We should at least s/C/R/ that part, and
possibly add description about ".have" thing.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > PJ Hyett <pjhyett@gmail.com> wrote:
> >> 
> >> Is there any possibility to have the server code in an upcoming
> >> release account for clients running 1.6.1?
> >
> > I can't think off-hand of a way for the server to know what version
> > the client is.  There's nothing really different in the protocol
> > between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of
> > include-tag) or later client.
> 
> Hmm, I am puzzled.
> 
> I do not know how 41fa7d2 (Teach git-fetch to exploit server side
> automatic tag following, 2008-03-03), which is about the conversation
> between fetch-pack and upload-pack, is relevant to the issue at hand,
> which is about the conversation between send-pack and receive-pack.

Oh, right, its not.  I was pointing out that the last time the
protocol changed in a way the server can infer something about the
client, which IIRC was 41fa7d2, we still don't have a way to tell
what the client is.
 
> In send-pack receive-pack protocol, the server talks first before
> listening to the client, and the .have data is in this first part of the
> conversation.

But as you rightly point out, that's the real problem.  Since the
server talks first, there's no way for the server to avoid giving
out the newer ".have" lines to a buggy client, as it knows nothing
at all about the client.  Not even its capabilities.

PJ - the short story here is, to forever work around these buggy
1.6.1 clients, you'd have to either run an old server forever,
or forever run a patched server that disables the newer ".have"
extension in the advertised data written by git-upload-pack.
There just isn't a way to hide this from the client.

Really though, I'd recommend getting your users to upgrade to a
non-buggy client.  Pasky has the same problem on repo.or.cz; if
he doesn't have it already he will soon when he upgrades...

-- 
Shawn.

Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> Since 40c155ff14c, receive-pack on the remote also sends refs from its
> alternates. Unfortunately, we don't filter commits that don't exist in the
> local repository from that list.  This made us pass those unknown commits
> to pack-objects, causing it to fail with a "bad object" error.
>
> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> ---
>  builtin-send-pack.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index a9fdbf9..10d7016 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -52,11 +52,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  	 * parameters by writing to the pipe.
>  	 */
>  	for (i = 0; i < extra->nr; i++) {
> -		memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
> -		buf[0] = '^';
> -		buf[41] = '\n';
> -		if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
> -			break;
> +		if (!is_null_sha1(&extra->array[i][0]) &&
> +		    has_sha1_file(&extra->array[i][0])) {
> +			memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
> +			buf[0] = '^';
> +			buf[41] = '\n';
> +			if (!write_or_whine(po.in, buf, 42,
> +						"send-pack: send refs"))
> +				break;
> +		}
>  	}
>  
>  	while (refs) {

Actually I changed my mind.

We have the exactly the same issue for the real refs the target repository
has, not just borrowed phantom refs, in the code from day one of git-push.
In other words, this issue predates the ".have" extension, and your update
is in line with how the codepath for the real refs does its thing.  So
your fix is not worse than the existing code.

It can be argued that at least in the "real ref" case you are in control
of both ends and if you have a disconnected chain in your local repository
that you do not have a ref for, you are screwing yourself, and it is your
problem.  But when you forked your repository from somebody else on a
hosting site like github, you do not have much control over the other end
(because it is a closed site you cannot ssh in to diagnose what is really
going on), and if you do not exactly know from whom your hosted repository
is borrowing, it is more likely that you will get into a situation where
you may have objects near the tip without having the full chain after an
aborted transfer, and the insufficient check of doing only has_sha1_file()
may become a larger issue in such a settings.

But still, let's take the approach I labeled as *wrong* as an interim
solution for the immediate future.

I'd prefer a small helper function to consolidate the duplicated code,
like the attached patch, though.  How about doing it like this?

 builtin-send-pack.c |   46 ++++++++++++++++++++++++----------------------
 1 files changed, 24 insertions(+), 22 deletions(-)

diff --git c/builtin-send-pack.c w/builtin-send-pack.c
index a9fdbf9..2d24cf2 100644
--- c/builtin-send-pack.c
+++ w/builtin-send-pack.c
@@ -15,6 +15,23 @@ static struct send_pack_args args = {
 	/* .receivepack = */ "git-receive-pack",
 };
 
+static int feed_object(const unsigned char *theirs, int fd, int negative)
+{
+	char buf[42];
+
+	if (!has_sha1_file(theirs))
+		return 1;
+	/*
+	 * NEEDSWORK: we should not be satisfied by simply having
+	 * theirs, but should be making sure it is reachable from
+	 * some of our refs.
+	 */
+	memcpy(buf + negative, sha1_to_hex(theirs), 40);
+	if (negative)
+		buf[0] = '^';
+	buf[40 + negative] = '\n';
+	return write_or_whine(fd, buf, 41 + negative, "send-pack: send refs");
+}
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -35,7 +52,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	};
 	struct child_process po;
 	int i;
-	char buf[42];
 
 	if (args.use_thin_pack)
 		argv[4] = "--thin";
@@ -51,31 +67,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	 * We feed the pack-objects we just spawned with revision
 	 * parameters by writing to the pipe.
 	 */
-	for (i = 0; i < extra->nr; i++) {
-		memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
-		buf[0] = '^';
-		buf[41] = '\n';
-		if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
+	for (i = 0; i < extra->nr; i++)
+		if (!feed_object(extra->array[i], po.in, 1))
 			break;
-	}
 
 	while (refs) {
 		if (!is_null_sha1(refs->old_sha1) &&
-		    has_sha1_file(refs->old_sha1)) {
-			memcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);
-			buf[0] = '^';
-			buf[41] = '\n';
-			if (!write_or_whine(po.in, buf, 42,
-						"send-pack: send refs"))
-				break;
-		}
-		if (!is_null_sha1(refs->new_sha1)) {
-			memcpy(buf, sha1_to_hex(refs->new_sha1), 40);
-			buf[40] = '\n';
-			if (!write_or_whine(po.in, buf, 41,
-						"send-pack: send refs"))
-				break;
-		}
+		    !feed_object(refs->old_sha1, po.in, 1))
+			break;
+		if (!is_null_sha1(refs->new_sha1) &&
+		    !feed_object(refs->new_sha1, po.in, 0))
+			break;
 		refs = refs->next;
 	}
 
.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Stephen Bannasch]

At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:
>PJ - the short story here is, to forever work around these buggy
>1.6.1 clients, you'd have to either run an old server forever,
>or forever run a patched server that disables the newer ".have"
>extension in the advertised data written by git-upload-pack.
>There just isn't a way to hide this from the client.
>
>Really though, I'd recommend getting your users to upgrade to a
>non-buggy client.  Pasky has the same problem on repo.or.cz; if
>he doesn't have it already he will soon when he upgrades...

Do you know if this problem is fixed in tag v1.6.1.1?

   Tagger: Junio C Hamano <gitster@pobox.com>
   Date:   Sun Jan 25 12:41:48 2009 -0800
   commit 5c415311f743ccb11a50f350ff1c385778f049d6

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Shawn O. Pearce]

Stephen Bannasch <stephen.bannasch@deanbrook.org> wrote:
> At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:
>> PJ - the short story here is, to forever work around these buggy
>> 1.6.1 clients, you'd have to either run an old server forever,
>> or forever run a patched server that disables the newer ".have"
>> extension in the advertised data written by git-upload-pack.
>> There just isn't a way to hide this from the client.
>>
>> Really though, I'd recommend getting your users to upgrade to a
>> non-buggy client.  Pasky has the same problem on repo.or.cz; if
>> he doesn't have it already he will soon when he upgrades...
>
> Do you know if this problem is fixed in tag v1.6.1.1?
>
>   Tagger: Junio C Hamano <gitster@pobox.com>
>   Date:   Sun Jan 25 12:41:48 2009 -0800
>   commit 5c415311f743ccb11a50f350ff1c385778f049d6

Without even checking Git I can tell you it isn't fixed by 1.6.1.1.

The date on the tag is Jan 25th, 2 full days before PJ reported
the problem and a solution was proposed...

-- 
Shawn.

Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote

[Björn Steinbrink]

On 2009.01.27 19:33:11 -0800, Junio C Hamano wrote:
> It can be argued that at least in the "real ref" case you are in control
> of both ends and if you have a disconnected chain in your local repository
> that you do not have a ref for, you are screwing yourself, and it is your
> problem.  But when you forked your repository from somebody else on a
> hosting site like github, you do not have much control over the other end
> (because it is a closed site you cannot ssh in to diagnose what is really
> going on), and if you do not exactly know from whom your hosted repository
> is borrowing, it is more likely that you will get into a situation where
> you may have objects near the tip without having the full chain after an
> aborted transfer, and the insufficient check of doing only has_sha1_file()
> may become a larger issue in such a settings.

Uhm, it might be obvious, but what exactly could go wrong? Do we need to
fetch from multiple repos when alternates are involved? Or how would we
end up with a broken chain? I mean, it starts to make some sense to me
why we would need the connectivity check, but how do we end up with a
"partial" fetch at all?

> I'd prefer a small helper function to consolidate the duplicated code,
> like the attached patch, though.  How about doing it like this?

Yeah, that looks a lot nicer :-)

Björn

Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> Uhm, it might be obvious, but what exactly could go wrong?

Between the refs and your object store, there is a contract that
guarantees that everything that is reachable from your refs are complete
and you won't hit unreachable object while traversing the reachability
chain from them.  But your object store can contain other garbage
objects.  The contract is one of the things "git fsck" checks.

Imagine you have fetched from somewhere with a commit walker (e.g. fetch
over http), that started fetching from the tip commit and its associated
objects, and then got interrupted.  Such a transfer will leave the objects
in your local repository but it is safe because it won't update your refs.

>> I'd prefer a small helper function to consolidate the duplicated code,
>> like the attached patch, though.  How about doing it like this?
>
> Yeah, that looks a lot nicer :-)

But it was broken.  The initial check feed_object() does with
has_sha1_file() and NEEDSWORK comment needs to be inside

	if (negative) {
		if (!has_sha1_file(theirs))
			return 1;
		/*
		 * NEEDSWORK: we should not be satisfied by simply having
		 * theirs, but should be making sure it is reachable from
		 * some of our refs.
		 */
	}

to make sure we won't trigger the availability or connectivity check for
positive refs.

Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> ... Do we need to
> fetch from multiple repos when alternates are involved?

This part is a slightly different issue than the rest of your message, so
I'll answer separately.

Yes, in the example, if Bob fetched from Alice before he pushed, his push
will succeed with the 1.6.1 send-pack.

But that is a workaround, and it is not a fix.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> Hmm, I am puzzled.
>> 
>> I do not know how 41fa7d2 (Teach git-fetch to exploit server side
>> automatic tag following, 2008-03-03), which is about the conversation
>> between fetch-pack and upload-pack, is relevant to the issue at hand,
>> which is about the conversation between send-pack and receive-pack.
>
> Oh, right, its not.  I was pointing out that the last time the
> protocol changed in a way the server can infer something about the
> client, which IIRC was 41fa7d2, we still don't have a way to tell
> what the client is.

But you are still talking as if there is one protocol you can call "the
protocol", but it is not.  The send-pack receive-pack protocol is on topic
in this thread; the quoted commit was about a separate and independent
fetch-pack upload-pack protocol.  It does not matter when that unrelated
protocol was enhanced.

> PJ - the short story here is, to forever work around these buggy
> 1.6.1 clients, you'd have to either run an old server forever,
> or forever run a patched server that disables the newer ".have"
> extension in the advertised data written by git-upload-pack.
> There just isn't a way to hide this from the client.
>
> Really though, I'd recommend getting your users to upgrade to a
> non-buggy client.  Pasky has the same problem on repo.or.cz; if
> he doesn't have it already he will soon when he upgrades...

Yeah, I'll apply the attached patch to 'maint' and it will be in the next
1.6.1.X maintenance release.  I suspect that your 1.6.1 users are the ones
who like to be on the cutting edge, and it wouldn't be unreasonable to
expect that they will update soon (1.6.1 has been out only for one month).

-- >8 --
Subject: [PATCH] send-pack: do not send unknown object name from ".have" to pack-objects

v1.6.1 introduced ".have" extension to the protocol to allow the receiving
side to advertise objects that are reachable from refs in the repositories
it borrows from.  This was meant to be used by the sending side to avoid
sending such objects; they are already available through the alternates
mechanism.

The client side implementation in v1.6.1, which was introduced with
40c155f (push: prepare sender to receive extended ref information from the
receiver, 2008-09-09) aka v1.6.1-rc1~203^2~1, were faulty in that it did
not consider the possiblity that the repository receiver borrows from
might have objects it does not know about.

This implements a tentative fix.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-send-pack.c |   49 +++++++++++++++++++++++++++----------------------
 1 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index a9fdbf9..fae597b 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -15,6 +15,26 @@ static struct send_pack_args args = {
 	/* .receivepack = */ "git-receive-pack",
 };
 
+static int feed_object(const unsigned char *theirs, int fd, int negative)
+{
+	char buf[42];
+
+	if (negative) {
+		if (!has_sha1_file(theirs))
+			return 1;
+		/*
+		 * NEEDSWORK: we should not be satisfied by simply having
+		 * theirs, but should be making sure it is reachable from
+		 * some of our refs.
+		 */
+	}
+
+	memcpy(buf + negative, sha1_to_hex(theirs), 40);
+	if (negative)
+		buf[0] = '^';
+	buf[40 + negative] = '\n';
+	return write_or_whine(fd, buf, 41 + negative, "send-pack: send refs");
+}
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -35,7 +55,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	};
 	struct child_process po;
 	int i;
-	char buf[42];
 
 	if (args.use_thin_pack)
 		argv[4] = "--thin";
@@ -51,31 +70,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	 * We feed the pack-objects we just spawned with revision
 	 * parameters by writing to the pipe.
 	 */
-	for (i = 0; i < extra->nr; i++) {
-		memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
-		buf[0] = '^';
-		buf[41] = '\n';
-		if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
+	for (i = 0; i < extra->nr; i++)
+		if (!feed_object(extra->array[i], po.in, 1))
 			break;
-	}
 
 	while (refs) {
 		if (!is_null_sha1(refs->old_sha1) &&
-		    has_sha1_file(refs->old_sha1)) {
-			memcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);
-			buf[0] = '^';
-			buf[41] = '\n';
-			if (!write_or_whine(po.in, buf, 42,
-						"send-pack: send refs"))
-				break;
-		}
-		if (!is_null_sha1(refs->new_sha1)) {
-			memcpy(buf, sha1_to_hex(refs->new_sha1), 40);
-			buf[40] = '\n';
-			if (!write_or_whine(po.in, buf, 41,
-						"send-pack: send refs"))
-				break;
-		}
+		    !feed_object(refs->old_sha1, po.in, 1))
+			break;
+		if (!is_null_sha1(refs->new_sha1) &&
+		    !feed_object(refs->new_sha1, po.in, 0))
+			break;
 		refs = refs->next;
 	}
 
-- 
1.6.1.1.273.g0e555

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> >
> > Oh, right, its not.  I was pointing out that the last time the
> > protocol changed in a way the server can infer something about the
> > client, which IIRC was 41fa7d2, we still don't have a way to tell
> > what the client is.
> 
> But you are still talking as if there is one protocol you can call "the
> protocol", but it is not.  The send-pack receive-pack protocol is on topic
> in this thread; the quoted commit was about a separate and independent
> fetch-pack upload-pack protocol.  It does not matter when that unrelated
> protocol was enhanced.

Blargh.  Of course you are right.  Its been a long 2 months for me
at work.  I'm too #@*#@!@! tired to keep the basics straight anymore.

I'm going to shut up now.
 
-- 
Shawn.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

Stephen Bannasch <stephen.bannasch@deanbrook.org> writes:

> At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:
>>PJ - the short story here is, to forever work around these buggy
>>1.6.1 clients, you'd have to either run an old server forever,
>>or forever run a patched server that disables the newer ".have"
>>extension in the advertised data written by git-upload-pack.
>>There just isn't a way to hide this from the client.
>>
>>Really though, I'd recommend getting your users to upgrade to a
>>non-buggy client.  Pasky has the same problem on repo.or.cz; if
>>he doesn't have it already he will soon when he upgrades...
>
> Do you know if this problem is fixed in tag v1.6.1.1?
>
>   Tagger: Junio C Hamano <gitster@pobox.com>
>   Date:   Sun Jan 25 12:41:48 2009 -0800
>   commit 5c415311f743ccb11a50f350ff1c385778f049d6

Give us a break.  This was reported today and diagnosed a few hours ago.

In the meantime, here is a minimum patch that should help you to help us
convince the approach we decided to take would work fine for people.



 connect.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git c/connect.c w/connect.c
index 2f23ab3..8026850 100644
--- c/connect.c
+++ w/connect.c
@@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
 
 static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)
 {
+	if (!has_sha1_file(sha1))
+		return;
+
 	ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);
 	hashcpy(&(extra->array[extra->nr][0]), sha1);
 	extra->nr++;

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>> >
>> > Oh, right, its not.  I was pointing out that the last time the
>> > protocol changed in a way the server can infer something about the
>> > client, which IIRC was 41fa7d2, we still don't have a way to tell
>> > what the client is.
>> 
>> But you are still talking as if there is one protocol you can call "the
>> protocol", but it is not.  The send-pack receive-pack protocol is on topic
>> in this thread; the quoted commit was about a separate and independent
>> fetch-pack upload-pack protocol.  It does not matter when that unrelated
>> protocol was enhanced.
>
> Blargh.  Of course you are right.  Its been a long 2 months for me
> at work.  I'm too #@*#@!@! tired to keep the basics straight anymore.
>
> I'm going to shut up now.

Please don't.

I've been toying with an idea for an alternative solution, and need
somebody competent to bounce it around with.

pack-objects ends up doing eventually

    rev-list --objects $send1 $send2 $send3 ... --not $have1 $have2 ...

which lists commits and associated objects reachable from $sendN,
excluding the ones that are reachable from $haveN.

The tentative solution Björn Steinbrink and I came up with excludes
missing commit from $haveN to avoid rev-list machinery to barf, but it
violates the ref-object contract as I explained to Björn in my other
message.

	Side note.  We often cite "interrupted commit walkers" as the
	reason why has_sha1_file() is not a good enough check, but you can
	discard a deep commit chain by deleting a branch, and have gc
	expire older commit in the commit chain while retaining newer ones
	near the tip of that branch.  If (1) you earlier gave that branch
	to somebody else, (2) that somebody else has the tip of the branch
	you discarded in his repository, and (3) the repository you are
	pushing into borrows from that somebody else's repository, then
	you have the same situation that your has_sha1_file() succeeds but
	it will fail when you start digging deeper.

Checking if each commit is reachable from any of the refs is quite
expensive, and it would especially be so if it is done once per ".have"
and real ref we receive from the other end.

An alternative is to realize that rev-list traversal already does
something quite similar to what is needed to prove if these ".have"s are
reachable from refs when listing the reachable objects.  This computation
is what it needs to do anyway, so if we teach rev-list to ignore missing
or broken chain while traversing negative refs, we do not have to incur
any overhead over existing code.

Here is my work in progress.  It introduces "ignore-missing-negative"
option to the revision traversal machinery, and squelches the places we
currently complain loudly and die when we expect an object to be
available, when the color we are going to paint the object with is
UNINTERESTING.

I have a mild suspicion that it may even be the right thing to ignore them
unconditionally, and it might even match the intention of Linus's original
code.  That would make many hunks in this patch much simpler.

The evidences behind this suspicion are found in a handful of places in
revision.c.  mark_blob_uninteresting() does not complain if the caller
fails to find the blob.  mark_tree_uninteresting() does not, either.
mark_parents_uninteresting() does not, either, and it even has a comment
that strongly suggests the original intention was not to care about
missing UNINTERESTING objects.

 builtin-pack-objects.c |    1 +
 revision.c             |   24 ++++++++++++++++++++----
 revision.h             |    1 +
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git i/builtin-pack-objects.c w/builtin-pack-objects.c
index cedef52..c615a2f 100644
--- i/builtin-pack-objects.c
+++ w/builtin-pack-objects.c
@@ -2026,6 +2026,7 @@ static void get_object_list(int ac, const char **av)
 	int flags = 0;
 
 	init_revisions(&revs, NULL);
+	revs.ignore_missing_negative = 1;
 	save_commit_buffer = 0;
 	setup_revisions(ac, av, &revs, NULL);
 
diff --git i/revision.c w/revision.c
index db60f06..314341b 100644
--- i/revision.c
+++ w/revision.c
@@ -132,6 +132,8 @@ void mark_parents_uninteresting(struct commit *commit)
 
 static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)
 {
+	if (!obj)
+		return;
 	if (revs->no_walk && (obj->flags & UNINTERESTING))
 		die("object ranges do not make sense when not walking revisions");
 	if (revs->reflog_info && obj->type == OBJ_COMMIT &&
@@ -163,8 +165,11 @@ static struct object *get_reference(struct rev_info *revs, const char *name, con
 	struct object *object;
 
 	object = parse_object(sha1);
-	if (!object)
+	if (!object) {
+		if (revs->ignore_missing_negative && (flags & UNINTERESTING))
+			return NULL;
 		die("bad object %s", name);
+	}
 	object->flags |= flags;
 	return object;
 }
@@ -183,8 +188,11 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		if (!tag->tagged)
 			die("bad tag");
 		object = parse_object(tag->tagged->sha1);
-		if (!object)
+		if (!object) {
+			if (revs->ignore_missing_negative && (flags & UNINTERESTING))
+				return NULL;
 			die("bad object %s", sha1_to_hex(tag->tagged->sha1));
+		}
 	}
 
 	/*
@@ -193,8 +201,11 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 	 */
 	if (object->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)object;
-		if (parse_commit(commit) < 0)
+		if (parse_commit(commit) < 0) {
+			if (revs->ignore_missing_negative && (flags & UNINTERESTING))
+				return NULL;
 			die("unable to parse commit %s", name);
+		}
 		if (flags & UNINTERESTING) {
 			commit->object.flags |= UNINTERESTING;
 			mark_parents_uninteresting(commit);
@@ -479,8 +490,11 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
 		while (parent) {
 			struct commit *p = parent->item;
 			parent = parent->next;
-			if (parse_commit(p) < 0)
+			if (parse_commit(p) < 0) {
+				if (revs->ignore_missing_negative)
+					return 0;
 				return -1;
+			}
 			p->object.flags |= UNINTERESTING;
 			if (p->parents)
 				mark_parents_uninteresting(p);
@@ -1110,6 +1124,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->tree_objects = 1;
 		revs->blob_objects = 1;
 		revs->edge_hint = 1;
+	} else if (!strcmp(arg, "--ignore-missing-negative")) {
+		revs->ignore_missing_negative = 1;
 	} else if (!strcmp(arg, "--unpacked")) {
 		revs->unpacked = 1;
 		free(revs->ignore_packed);
diff --git i/revision.h w/revision.h
index 7cf8487..bb90399 100644
--- i/revision.h
+++ w/revision.h
@@ -48,6 +48,7 @@ struct rev_info {
 			tree_objects:1,
 			blob_objects:1,
 			edge_hint:1,
+			ignore_missing_negative:1,
 			limited:1,
 			unpacked:1, /* see also ignore_packed below */
 			boundary:2,

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

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

> Here is my work in progress.  It introduces "ignore-missing-negative"
> option to the revision traversal machinery, and squelches the places we
> currently complain loudly and die when we expect an object to be
> available, when the color we are going to paint the object with is
> UNINTERESTING.
>
> I have a mild suspicion that it may even be the right thing to ignore them
> unconditionally, and it might even match the intention of Linus's original
> code.  That would make many hunks in this patch much simpler.
>
> The evidences behind this suspicion are found in a handful of places in
> revision.c.  mark_blob_uninteresting() does not complain if the caller
> fails to find the blob.  mark_tree_uninteresting() does not, either.
> mark_parents_uninteresting() does not, either, and it even has a comment
> that strongly suggests the original intention was not to care about
> missing UNINTERESTING objects.

Here is what I ended up with doing.  It lost "ignore-missing-negative"
so missing UNINTERESTING objects are non-error events more uniformly,
but on the other hand get_reference() which is about the command line
arguments always wants the named objects to exist, even if they are
marked as UNINTERESTING.

I'll send [PATCH 1/2] which is an update to my previous fix as a follow-up
to this message.

-- >8 --
Subject: [PATCH 2/2] revision traversal: allow UNINTERESTING objects to be missing

Most of the existing codepaths were meant to treat missing uninteresting
objects to be a silently ignored non-error, but there were a few places
in handle_commit() and add_parents_to_list(), which are two key functions
in the revision traversal machinery, that cared:

 - When a tag refers to an object that we do not have, we barfed.  We
   ignore such a tag if it is painted as UNINTERESTING with this change.

 - When digging deeper into the ancestry chain of a commit that is already
   painted as UNINTERESTING, in order to paint its parents UNINTERESTING,
   we barfed if parse_parent() for a parent commit object failed.  We can
   ignore such a parent commit object.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c                 |    7 +++++--
 t/t5519-push-alternates.sh |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index db60f06..ea8ba0f 100644
--- a/revision.c
+++ b/revision.c
@@ -183,8 +183,11 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		if (!tag->tagged)
 			die("bad tag");
 		object = parse_object(tag->tagged->sha1);
-		if (!object)
+		if (!object) {
+			if (flags & UNINTERESTING)
+				return NULL;
 			die("bad object %s", sha1_to_hex(tag->tagged->sha1));
+		}
 	}
 
 	/*
@@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
 			struct commit *p = parent->item;
 			parent = parent->next;
 			if (parse_commit(p) < 0)
-				return -1;
+				continue;
 			p->object.flags |= UNINTERESTING;
 			if (p->parents)
 				mark_parents_uninteresting(p);
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
index 6dfc55a..96be523 100755
--- a/t/t5519-push-alternates.sh
+++ b/t/t5519-push-alternates.sh
@@ -103,4 +103,41 @@ test_expect_success 'bob works and pushes' '
 	)
 '
 
+test_expect_success 'alice works and pushes yet again' '
+	(
+		# Alice does not care what Bob does.  She does not
+		# even have to be aware of his existence.  She just
+		# keeps working and pushing
+		cd alice-work &&
+		echo more and more alice >file &&
+		git commit -a -m sixth.1 &&
+		echo more and more alice >>file &&
+		git commit -a -m sixth.2 &&
+		echo more and more alice >>file &&
+		git commit -a -m sixth.3 &&
+		git push ../alice-pub
+	)
+'
+
+test_expect_success 'bob works and pushes again' '
+	(
+		cd alice-pub &&
+		git cat-file commit master >../bob-work/commit
+	)
+	(
+		# This time Bob does not pull from Alice, and
+		# the master branch at her public repository points
+		# at a commit Bob does not fully know about, but
+		# he happens to have the commit object (but not the
+		# necessary tree) in his repository from Alice.
+		# This should not prevent the push by Bob from
+		# succeeding.
+		cd bob-work &&
+		git hash-object -t commit -w commit &&
+		echo even more bob >file &&
+		git commit -a -m seventh &&
+		git push ../bob-pub
+	)
+'
+
 test_done
-- 
1.6.1.1.273.g0e555

[PATCH 1/2] send-pack: do not send unknown object name from ".have" to pack-objects

[Junio C Hamano]

v1.6.1 introduced ".have" extension to the protocol to allow the receiving
side to advertise objects that are reachable from refs in the repositories
it borrows from.  This was meant to be used by the sending side to avoid
sending such objects; they are already available through the alternates
mechanism.

The client side implementation in v1.6.1, which was introduced with
40c155f (push: prepare sender to receive extended ref information from the
receiver, 2008-09-09) aka v1.6.1-rc1~203^2~1, were faulty in that it did
not consider the possiblity that the repository receiver borrows from
might have objects it does not know about.

This fixes it by refraining from passing missing commits to underlying
pack-objects.  Revision machinery may need to be tightened further to
treat missing uninteresting objects as non-error events, but this is an
obvious and safe fix for a maintenance release that is almost good enough.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This lost the "NEEDSWORK" comment, as it turns out that revision
   traversal machinery mostly ignores missing uninteresting objects as
   non-error events except for a few corner cases, which we can tighten
   separately.

 builtin-send-pack.c        |   43 +++++++++---------
 t/t5519-push-alternates.sh |  106 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 127 insertions(+), 22 deletions(-)
 create mode 100755 t/t5519-push-alternates.sh

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index a9fdbf9..d65d019 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -15,6 +15,20 @@ static struct send_pack_args args = {
 	/* .receivepack = */ "git-receive-pack",
 };
 
+static int feed_object(const unsigned char *sha1, int fd, int negative)
+{
+	char buf[42];
+
+	if (negative && !has_sha1_file(sha1))
+		return 1;
+
+	memcpy(buf + negative, sha1_to_hex(sha1), 40);
+	if (negative)
+		buf[0] = '^';
+	buf[40 + negative] = '\n';
+	return write_or_whine(fd, buf, 41 + negative, "send-pack: send refs");
+}
+
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -35,7 +49,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	};
 	struct child_process po;
 	int i;
-	char buf[42];
 
 	if (args.use_thin_pack)
 		argv[4] = "--thin";
@@ -51,31 +64,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	 * We feed the pack-objects we just spawned with revision
 	 * parameters by writing to the pipe.
 	 */
-	for (i = 0; i < extra->nr; i++) {
-		memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
-		buf[0] = '^';
-		buf[41] = '\n';
-		if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
+	for (i = 0; i < extra->nr; i++)
+		if (!feed_object(extra->array[i], po.in, 1))
 			break;
-	}
 
 	while (refs) {
 		if (!is_null_sha1(refs->old_sha1) &&
-		    has_sha1_file(refs->old_sha1)) {
-			memcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);
-			buf[0] = '^';
-			buf[41] = '\n';
-			if (!write_or_whine(po.in, buf, 42,
-						"send-pack: send refs"))
-				break;
-		}
-		if (!is_null_sha1(refs->new_sha1)) {
-			memcpy(buf, sha1_to_hex(refs->new_sha1), 40);
-			buf[40] = '\n';
-			if (!write_or_whine(po.in, buf, 41,
-						"send-pack: send refs"))
-				break;
-		}
+		    !feed_object(refs->old_sha1, po.in, 1))
+			break;
+		if (!is_null_sha1(refs->new_sha1) &&
+		    !feed_object(refs->new_sha1, po.in, 0))
+			break;
 		refs = refs->next;
 	}
 
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
new file mode 100755
index 0000000..6dfc55a
--- /dev/null
+++ b/t/t5519-push-alternates.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='push to a repository that borrows from elsewhere'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	mkdir alice-pub &&
+	(
+		cd alice-pub &&
+		GIT_DIR=. git init
+	) &&
+	mkdir alice-work &&
+	(
+		cd alice-work &&
+		git init &&
+		>file &&
+		git add . &&
+		git commit -m initial &&
+		git push ../alice-pub master
+	) &&
+
+	# Project Bob is a fork of project Alice
+	mkdir bob-pub &&
+	(
+		cd bob-pub &&
+		GIT_DIR=. git init &&
+		mkdir -p objects/info &&
+		echo ../../alice-pub/objects >objects/info/alternates
+	) &&
+	git clone alice-pub bob-work &&
+	(
+		cd bob-work &&
+		git push ../bob-pub master
+	)
+'
+
+test_expect_success 'alice works and pushes' '
+	(
+		cd alice-work &&
+		echo more >file &&
+		git commit -a -m second &&
+		git push ../alice-pub
+	)
+'
+
+test_expect_success 'bob fetches from alice, works and pushes' '
+	(
+		# Bob acquires what Alice did in his work tree first.
+		# Even though these objects are not directly in
+		# the public repository of Bob, this push does not
+		# need to send the commit Bob received from Alice
+		# to his public repository, as all the object Alice
+		# has at her public repository are available to it
+		# via its alternates.
+		cd bob-work &&
+		git pull ../alice-pub master &&
+		echo more bob >file &&
+		git commit -a -m third &&
+		git push ../bob-pub
+	) &&
+
+	# Check that the second commit by Alice is not sent
+	# to ../bob-pub
+	(
+		cd bob-pub &&
+		second=$(git rev-parse HEAD^) &&
+		rm -f objects/info/alternates &&
+		test_must_fail git cat-file -t $second &&
+		echo ../../alice-pub/objects >objects/info/alternates
+	)
+'
+
+test_expect_success 'clean-up in case the previous failed' '
+	(
+		cd bob-pub &&
+		echo ../../alice-pub/objects >objects/info/alternates
+	)
+'
+
+test_expect_success 'alice works and pushes again' '
+	(
+		# Alice does not care what Bob does.  She does not
+		# even have to be aware of his existence.  She just
+		# keeps working and pushing
+		cd alice-work &&
+		echo more alice >file &&
+		git commit -a -m fourth &&
+		git push ../alice-pub
+	)
+'
+
+test_expect_success 'bob works and pushes' '
+	(
+		# This time Bob does not pull from Alice, and
+		# the master branch at her public repository points
+		# at a commit Bob does not know about.  This should
+		# not prevent the push by Bob from succeeding.
+		cd bob-work &&
+		echo yet more bob >file &&
+		git commit -a -m fifth &&
+		git push ../bob-pub
+	)
+'
+
+test_done
-- 
1.6.1.1.273.g0e555

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Jeff King]

On Tue, Jan 27, 2009 at 11:14:56PM -0800, Junio C Hamano wrote:

> I've been toying with an idea for an alternative solution, and need
> somebody competent to bounce it around with.

Well, unfortunately for you, you are stuck with me. ;P

> Here is my work in progress.  It introduces "ignore-missing-negative"
> option to the revision traversal machinery, and squelches the places we
> currently complain loudly and die when we expect an object to be
> available, when the color we are going to paint the object with is
> UNINTERESTING.
> 
> I have a mild suspicion that it may even be the right thing to ignore them
> unconditionally, and it might even match the intention of Linus's original
> code.  That would make many hunks in this patch much simpler.

I'm not sure it is a good idea to do so unconditionally. In the case of
negatives for transferring files, a missed negative is simply a missed
opportunity for optimizing the resulting pack.

But in other cases, it silently gives you the wrong answer.  For
example, consider a history like:

       C--D
      /
  A--B
      \
       E--F

now let's suppose I have everything except 'E'. If I ask for

  git rev-list F..D

then it will not realize that A and B are uninteresting, and I will get
A-B-C-D. I think it is much better for git to complain loudly that it
could not compute the correct answer.

Am I understanding the issue correctly?

-Peff

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But in other cases, it silently gives you the wrong answer.  For
> example, consider a history like:
>
>        C--D
>       /
>   A--B
>       \
>        E--F
>
> now let's suppose I have everything except 'E'. If I ask for
>
>   git rev-list F..D
>
> then it will not realize that A and B are uninteresting, and I will get
> A-B-C-D. I think it is much better for git to complain loudly that it
> could not compute the correct answer.

Fair enough.  I think we can resurrect the conditional and the traversal
option revs->ignore_missing_negative only for this hunk in my [2/2] patch
to support that use case.

@@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
 			struct commit *p = parent->item;
 			parent = parent->next;
 			if (parse_commit(p) < 0)
-				return -1;
+				continue;
 			p->object.flags |= UNINTERESTING;
 			if (p->parents)
 				mark_parents_uninteresting(p);

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Jeff King]

On Wed, Jan 28, 2009 at 12:05:33AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But in other cases, it silently gives you the wrong answer.  For
> > example, consider a history like:
> >
> >        C--D
> >       /
> >   A--B
> >       \
> >        E--F
> >
> > now let's suppose I have everything except 'E'. If I ask for
> >
> >   git rev-list F..D
> >
> > then it will not realize that A and B are uninteresting, and I will get
> > A-B-C-D. I think it is much better for git to complain loudly that it
> > could not compute the correct answer.
> 
> Fair enough.  I think we can resurrect the conditional and the traversal
> option revs->ignore_missing_negative only for this hunk in my [2/2] patch
> to support that use case.
> [ hunk handling parent lookup]

Don't the other changes have similar parallel use cases? [2/2] also deals
with tag lookup. Wouldn't you also expect, if you had a tag "T" pointing
to "E" in the above scenario that "git rev-list T..D" would barf? I
really think you don't want to ignore missing negations _ever_ unless
the caller knows that such a miss is really only about optimization and
not correctness.

Side note:

As you described, we expect to reach this situation from a partial
transfer. Which means that you don't actually have a _ref_ for "T" (or
"F"). So it is unlikely to come up in normal use (you would have to
manually specify the sha1 of a broken portion of the graph).

But what is more important is that your repository _is_ corrupted, I
think we are losing an important method by which the user finds out. Git
is usually very good at informing you of a problem in the repo early,
and I think unconditionally ignoring missing objects would lose that.

-Peff

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> But in other cases, it silently gives you the wrong answer.  For
>> example, consider a history like:
>>
>>        C--D
>>       /
>>   A--B
>>       \
>>        E--F
>>
>> now let's suppose I have everything except 'E'. If I ask for
>>
>>   git rev-list F..D
>>
>> then it will not realize that A and B are uninteresting, and I will get
>> A-B-C-D. I think it is much better for git to complain loudly that it
>> could not compute the correct answer.
>
> Fair enough.  I think we can resurrect the conditional and the traversal
> option revs->ignore_missing_negative only for this hunk in my [2/2] patch
> to support that use case.
> ...

Nah, I take that back.

Even the original code does not consider this case an error.

If you really want that, the revision machinery needs major surgery, as I
already noted that the design of mark_parents_uninteresting() wants to
treat a missing uninteresting commit as a non-error event.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Jeff King]

On Wed, Jan 28, 2009 at 12:22:01AM -0800, Junio C Hamano wrote:

> Nah, I take that back.
> 
> Even the original code does not consider this case an error.
> 
> If you really want that, the revision machinery needs major surgery, as I
> already noted that the design of mark_parents_uninteresting() wants to
> treat a missing uninteresting commit as a non-error event.

Hrm. Never mind my concern, then. I was worried that we were losing some
existing corruption checks, but it seems they are not there in the first
place.

-Peff

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Linus Torvalds]

On Tue, 27 Jan 2009, Junio C Hamano wrote:
> 
>  - When digging deeper into the ancestry chain of a commit that is already
>    painted as UNINTERESTING, in order to paint its parents UNINTERESTING,
>    we barfed if parse_parent() for a parent commit object failed.  We can
>    ignore such a parent commit object.

Wouldn't it be better to still mark it UNINTERESTING too?

> @@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
>  			struct commit *p = parent->item;
>  			parent = parent->next;
>  			if (parse_commit(p) < 0)
> -				return -1;
> +				continue;
>  			p->object.flags |= UNINTERESTING;
>  			if (p->parents)
>  				mark_parents_uninteresting(p);

IOW, move that

	p->object.flags |= UNINTERESTING;

to before parse_commit(). That's assuming 'parent' is never NULL, of 
course.

Side note: parse_commit() is still going to print out the error message 
if the object is missing ("Could not read %s"). I guess that's fine, but 
if you really want to make this a "not an error at all" condition...

		Linus

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> 
> I've been toying with an idea for an alternative solution, and need
> somebody competent to bounce it around with.

Heh, then we need to wait for Nico... :-)
 
> pack-objects ends up doing eventually
> 
>     rev-list --objects $send1 $send2 $send3 ... --not $have1 $have2 ...
> 
> which lists commits and associated objects reachable from $sendN,
> excluding the ones that are reachable from $haveN.
> 
> The tentative solution Björn Steinbrink and I came up with excludes
> missing commit from $haveN to avoid rev-list machinery to barf, but it
> violates the ref-object contract as I explained to Björn in my other
> message.

Oh, OK, I now _finally_ understand what you were trying to say
by the reachability thing.  I kept scratching my head trying to
understand you, and was going to say something stupid on list;
but waited because I just didn't get what the big deal was...

Its the crash in rev-list that you were worried about.

> Checking if each commit is reachable from any of the refs is quite
> expensive, and it would especially be so if it is done once per ".have"
> and real ref we receive from the other end.

Yup.
 
> An alternative is to realize that rev-list traversal already does
> something quite similar to what is needed to prove if these ".have"s are
> reachable from refs when listing the reachable objects.  This computation
> is what it needs to do anyway, so if we teach rev-list to ignore missing
> or broken chain while traversing negative refs, we do not have to incur
> any overhead over existing code.

EXACTLY.

JGit does this.

The functional equivilant of rev-list in JGit will by default
throw an exception if any object is missing when we try to walk it.
That includes things we've painted UNINTERESTING, as it is a sure
sign of repository corruption.

However; our equivilant of pack-objects can toggle what you are
calling "ignore-missing-negative" when it starts enumeration.
Any UNINTERESTING object which is missing or failed to parse is
simply tossed aside.  Yes, the pack may be larger than necessary
like in Peff's example of:

       Q-R
      /
  D--E
      \
       A-C

If the other side has C reachable, we are pushing R, and we have
C but are missing A, we'll "over push" D-E, but its still a clean
and valid push.  Its no worse than we were before the ".have" came
about, or if C hadn't been downloaded locally at all.  (Of course
your tell-me-more extension would help fix this over-push, but lets
not get off topic.)

IMHO, this corruption of A is harmless if C isn't reachable.

It isn't really local corruption unless C was reachable by a ref.
But we don't tend to see much corruption like that, and if it did
exist, it would show up during *other* operations that access a
larger set of local refs, such as "git gc".

> I have a mild suspicion that it may even be the right thing to ignore them
> unconditionally, and it might even match the intention of Linus's original
> code.  That would make many hunks in this patch much simpler.

I don't think its right to ignore broken UNINTERESTING chains all
of the time.  Today we would see fatal errors if I asked for

  git log R ^C

and A was missing, but R and C are both local refs.  I still want
to see that fatal error.  Its a local corruption that should be
raised quickly to the user.  In fact by A missing we'd compute the
wrong result and produce D-E too, which is wrong.

IMHO, the *only* time this missing uninteresting A is safe is
during send-pack, upload-pack, or bundle creation, where you are
bringing the other side up to R by transferring any amount of data
necessary to reach that goal.  Which is why JGit enables this.
(Though at the API level we do let the caller flag if they want
the error to be fatal instead, but AFAIK nobody sets it for "fatal".)

FWIW, Linus' most recent message on this thread about hoisting the
UNINTERESTING test up sooner makes sense too.
 
> The evidences behind this suspicion are found in a handful of places in
> revision.c.  mark_blob_uninteresting() does not complain if the caller
> fails to find the blob.  mark_tree_uninteresting() does not, either.
> mark_parents_uninteresting() does not, either, and it even has a comment
> that strongly suggests the original intention was not to care about
> missing UNINTERESTING objects.

That feels wrong to me... given the "git log R ^C" example I give above.
 
-- 
Shawn.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Wed, Jan 28, 2009 at 12:05:33AM -0800, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> > >
> > >        C--D
> > >       /
> > >   A--B
> > >       \
> > >        E--F
> 
> Don't the other changes have similar parallel use cases? [2/2] also deals
> with tag lookup. Wouldn't you also expect, if you had a tag "T" pointing
> to "E" in the above scenario that "git rev-list T..D" would barf? I
> really think you don't want to ignore missing negations _ever_ unless
> the caller knows that such a miss is really only about optimization and
> not correctness.

Exactly what I just said in my other message.
 
> Side note:
> 
> As you described, we expect to reach this situation from a partial
> transfer. Which means that you don't actually have a _ref_ for "T" (or
> "F"). So it is unlikely to come up in normal use (you would have to
> manually specify the sha1 of a broken portion of the graph).

True, but in the send-pack case we are discussing the remote side
has specified the SHA-1 of broken portions of the graph to us,
and we've taken that into consideration.  So we have to fix that
assumption we've made.

> But what is more important is that your repository _is_ corrupted,

Depends.  If the SHA-1 came from the remote side during send-pack,
it doesn't matter that we have a broken chain along that path,
it may have been a dumb transport fetch that was interrupted.
Our local repository isn't corrupt, it just has some extra crap
laying around that hasn't gc'd yet.

If the SHA-1 came from the user, then it depends on the context
of why the user is giving it to us.  In pretty much every case,
yes, its a corruption and we should be aborting.  :-)

Actually, the only time where it *isn't* a corruption is when its
input to "git bundle create A.bdl ... -not $SOMEBADID" as that is
the exact same thing as coming from the other side via send-pack.

> I
> think we are losing an important method by which the user finds out. Git
> is usually very good at informing you of a problem in the repo early,
> and I think unconditionally ignoring missing objects would lose that.

Yup, I agree.  But as you and Junio have already pointed out, C Git
can miss some types of corruption because the revision machinary has
some gaps.  *sigh*

I'd really like to see those gaps closed.  But I don't have a good
enough handle on the code structure of the C Git revision machinary
to do that myself in a short period of time.  I know JGit's well...
but that's only because I wrote it.  ;-)

Its now on my wish list of things I wish I had time for in C Git.
But perhaps someone who is more familiar with the revision machinary
will get to it first.

-- 
Shawn.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Nicolas Pitre]

On Wed, 28 Jan 2009, Shawn O. Pearce wrote:

> Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > I've been toying with an idea for an alternative solution, and need
> > somebody competent to bounce it around with.
> 
> Heh, then we need to wait for Nico... :-)

Hmmm... ehhh... what have I done?

/me tries to walk by innocently without being noticed  ;-)


Nicolas

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Jeff King]

On Wed, Jan 28, 2009 at 08:09:00AM -0800, Shawn O. Pearce wrote:

> I don't think its right to ignore broken UNINTERESTING chains all
> of the time.  Today we would see fatal errors if I asked for
> 
>   git log R ^C
> 
> and A was missing, but R and C are both local refs.  I still want
> to see that fatal error.  Its a local corruption that should be
> raised quickly to the user.  In fact by A missing we'd compute the
> wrong result and produce D-E too, which is wrong.

I think you wrote this before reading the other part of the thread where
we see that many of these checks are not in C git. But to be clear, even
without Junio's patches the exact case I mentioned is not currently
reported as an error (i.e., will produce incorrect results). I tested
with:

-- >8 --
commit() {
    echo $1 >$1 && git add $1 && git commit -m $1 && git tag $1
}

mkdir repo && cd repo && git init
commit A
commit B
commit C
commit D
git checkout -b other B
commit E
commit F

rm -f .git/objects/`git rev-parse E | sed 's,^..,&/,'`
git log F..D
-- 8< --

which shows A-B-C-D.

-Peff

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Jeff King]

On Wed, Jan 28, 2009 at 08:16:53AM -0800, Shawn O. Pearce wrote:

> > But what is more important is that your repository _is_ corrupted,
> 
> Depends.  If the SHA-1 came from the remote side during send-pack,

Sorry, there is a critical typo in there. I meant to say "_if_ your
repository is corrupted" (and I have no idea how I ended up not only
omitting a word, but emphasizing the wrong one, so I can only suspect
the typo was in my brain and not my fingers).

So basically I agree with everything you said.

> Yup, I agree.  But as you and Junio have already pointed out, C Git
> can miss some types of corruption because the revision machinary has
> some gaps.  *sigh*
> 
> I'd really like to see those gaps closed.  But I don't have a good
> enough handle on the code structure of the C Git revision machinary
> to do that myself in a short period of time.  I know JGit's well...
> but that's only because I wrote it.  ;-)

I would like to see them closed, too. But keep in mind that this may
actually be a harder form of corruption to achieve than something like
just flipping some bits. Once the objects are in a packfile, you are
unlikely to lose a single or a small number of objects. But like with
any type of corruption, it is infrequent enough that it is hard to say
which is more common or "worse".

-Peff

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Actually, the only time where it *isn't* a corruption is when its
> input to "git bundle create A.bdl ... -not $SOMEBADID" as that is
> the exact same thing as coming from the other side via send-pack.

And notice that it is about a nagative ref.

Another case you may use an object ID that may or may not be good and it
is not a corruption is when a Porcelain has an object ID obtained from
somewhere, and wants to know if it is safe to use the object.  After
determining that the object itself exists (e.g. via "cat-file -t"),
you run

	rev-list --objects $THAT_UNKNOWN_ID --not --all

to see if it is reachable from some of your own refs, or at least it is
connected to them without gaps.  If it errors out while traversing, you
know it is bad; if it doesn't, you know you can merge one of the commits
reachable from your refs with it and put the result in your ref without
violating the ref-objects contract.

Notice that in this case, it is about a positive ref, and revision
machinery is set to notice the breakage.

So in that sense, the existing semantics is internally consistent.  The
rules (I am not making up a new rule here, but just spelling out) are:

 (1) You cannot just pick a random object that happens to exist in your
     repository, traverse to the objects it refers to and expect
     everything exists;

 (2) If an object is reachable from any of your refs, however, you can
     expect everything reachable from that object exists.  Otherwise you
     have a corrupt repository [*1*]).

 (3) Your object store may have garbage objects that are not reachable
     from any of your refs and it is normal.

 (4) You can use random objects that may not be well connected as negative
     revs to limit the range of revs (and optionally objects reachable
     from them) listed by object traversal.  If they are well connected,
     they will affect the outcome, but it is not an error if they are
     leftover cruft that is not connected to the positive ones you start
     your listing traversal at.


[Footnote]

*1* You don't have to bring up grafts and shallow.  People who know about
them know they are ways to hide or deliberately introduce this type of
corruption while keeping the system (mostly) working.

Re: Bad objects error since upgrading GitHub servers to 1.6.1

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, 27 Jan 2009, Junio C Hamano wrote:
>> 
>>  - When digging deeper into the ancestry chain of a commit that is already
>>    painted as UNINTERESTING, in order to paint its parents UNINTERESTING,
>>    we barfed if parse_parent() for a parent commit object failed.  We can
>>    ignore such a parent commit object.
>
> Wouldn't it be better to still mark it UNINTERESTING too?
>
>> @@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
>>  			struct commit *p = parent->item;
>>  			parent = parent->next;
>>  			if (parse_commit(p) < 0)
>> -				return -1;
>> +				continue;
>>  			p->object.flags |= UNINTERESTING;
>>  			if (p->parents)
>>  				mark_parents_uninteresting(p);
>
> IOW, move that
>
> 	p->object.flags |= UNINTERESTING;
>
> to before parse_commit(). That's assuming 'parent' is never NULL, of 
> course.

Ok, makes sense.  Will do.