I am puzzled why this is needed in this series.  In other words,

do we have a bug in the current upload-pack that does not have

shallow already by not clearing UNINTERESTING bit?  In yet other

words, I haven't figured out which part of the shallow series

makes it necessary to clear UNINTERESTING bit from wanted
I doubt unregister_shallow() is enough to ensure that the next

parse_commit() re-parses to recover its parents.  parse_commit()

says "if (item->object.parsed) return 0" upfront.  Don't you

need to do:
	object->parsed = 0;
before parse_commit()?
-

Hi,
IIRC (has been a long time, hasn't it?) the test failed without that line, 
Yes. Somehow, an important part of unregister_shallow() went missing (yet

another proof that my poor-man's-StGit does not always work). I think that

the "object->parsed = 0;" should go into unregister_shallow() like this:
diff --git a/commit.c b/commit.c

index d5103cd..4451376 100644

--- a/commit.c

+++ b/commit.c

@@ -258,6 +258,7 @@ int write_shallow_commits(int fd, int us

 int unregister_shallow(const unsigned char *sha1)

 {

 	int pos = commit_graft_pos(sha1);

+	struct commit *commit = lookup_commit(sha1);

 	if (pos < 0)

 		return -1;

 	if (pos + 1 < commit_graft_nr)

@@ -265,6 +266,8 @@ int unregister_shallow(const unsigned ch

 				sizeof(struct commit_graft *)

 				* (commit_graft_nr - pos - 1));

 	commit_graft_nr--;

+	if (commit)

+		commit->object.parsed = 0;

 	return 0;

 }
Ciao,

Dscho
-

That does not fly so well.  Your fetch-pack.c does this in

find_common() and dropping the parsed flag inside unregister

causes it to be parsed again later with its true parents, which

defeats what the commented part of the code wanted to do.
	if (!lookup_object(sha1))

		die("object not found: %s", line);

	/* make sure that it is parsed as shallow */

	parse_object(sha1);

	if (unregister_shallow(sha1))

		die("no shallow found: %s", line);

	continue;
Although I am reasonably sure we can eventually make it work, it

is very subtle and fragile -- somebody touching this code can

easily break it.

.
-

Hi,
I fully agree. Even the OA did not understand the code fully ;-)
How about adding "int force_reparse" to the signature of

unregister_shallow()? (Just like we added "int cleanup" to

get_merge_bases() to avoid pilot errors.)
Ciao,

Dscho
-

I do not think that's where its fragility lies.  It is that any

random place can later call parse_object() on the commit, after

you elaborately pre-parsed it with shallow so that it appears to

have fewer parents, with the expectation that nobody ever would

clear the parsed bit and cause it to be reparsed again.  With

that assumption, find_common() manipulated the shallow entry

after setting that scheme up.  A mechanism to prevent the commit

from getting re-parsed would have made it more robust.
-

Hi,
How about putting yet more meaning into nr_parents: if it is negative, it

is a shallow commit, but write_shallow_commits() only writes the SHA1 if

nr_parents is -1. Hmm?
Ciao,

Dscho
-