git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Bad objects error since upgrading GitHub servers to 1.6.1

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 28, 2009, 07:14 UTC
Message-ID
<7vd4e7x5ov.fsf@gitster.siamese.dyndns.org>
In-Reply-To
<20090128044150.GI1321@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 18 quoted lines
> 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,
Previous: Shawn O. PearceNext: Junio C Hamano
Message 26 of 43 in “Bad objects error since upgrading GitHub servers to 1.6.1”
  1. PJ HyettJan 27, 2009
  2. PJ HyettJan 27, 2009
  3. Johannes SchindelinJan 27, 2009
  4. Shawn O. PearceJan 27, 2009
  5. Junio C HamanoJan 27, 2009
  6. PJ HyettJan 28, 2009
  7. PJ HyettJan 28, 2009
  8. Junio C HamanoJan 28, 2009
  9. Junio C HamanoJan 28, 2009
  10. send-pack: Filter unknown commits from alternates of the remoteBjörn Steinbrink, Jan 28, 2009
  11. Junio C HamanoJan 28, 2009
  12. Junio C HamanoJan 28, 2009
  13. Björn SteinbrinkJan 28, 2009
  14. Junio C HamanoJan 28, 2009
  15. Junio C HamanoJan 28, 2009
  16. Junio C HamanoJan 28, 2009
  17. PJ HyettJan 28, 2009
  18. Shawn O. PearceJan 28, 2009
  19. Junio C HamanoJan 28, 2009
  20. Shawn O. PearceJan 28, 2009
  21. Stephen BannaschJan 28, 2009
  22. Shawn O. PearceJan 28, 2009
  23. Junio C HamanoJan 28, 2009
  24. Junio C HamanoJan 28, 2009
  25. Shawn O. PearceJan 28, 2009
  26. Junio C HamanoJan 28, 2009
  27. Junio C HamanoJan 28, 2009
  28. 1/2 send-pack: do not send unknown object name from ".have" to pack-objectsJunio C Hamano, Jan 28, 2009
  29. Linus TorvaldsJan 28, 2009
  30. Junio C HamanoJan 28, 2009
  31. Jeff KingJan 28, 2009
  32. Junio C HamanoJan 28, 2009
  33. Jeff KingJan 28, 2009
  34. Shawn O. PearceJan 28, 2009
  35. Jeff KingJan 28, 2009
  36. Junio C HamanoJan 28, 2009
  37. Junio C HamanoJan 28, 2009
  38. Jeff KingJan 28, 2009
  39. Shawn O. PearceJan 28, 2009
  40. Nicolas PitreJan 28, 2009
  41. Jeff KingJan 28, 2009
  42. Linus TorvaldsJan 28, 2009
  43. Björn SteinbrinkJan 28, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.