On Fri, May 27, 2022 at 12:30:05AM +0200, Omar Polo wrote:
> Unfortunately sending tags from deleted branches is still broken.  The
> test passes because the parent of the commit pointed by the tag is
> reachable from the main branch, but if you add another one (or more)
> commit in between then there is a "gap" and got fails to produce a
> correct pack file.
> 
> I poked around in pack_create.c but i'm not sure what's the best way to
> handle this (assuming we care.)  ideally we should include all the
> reachable commits from the selected tags.

Nice find!

The problem is that accounting of 'nskip' in findtwixt() is wrong.
'nskip' is supposed to reflect the number of commit IDs which are waiting
on the 'ids' queue and which have the color 'skip'. But we increase 'nskip'
even for IDs which are not on the queue anymore.

This happens when we decide to switch the color of parent commits to 'skip'.
In your test case this happens for the root commit which is in our drop set
and doesn't have any parents. So nskip gets incremented while no new commit
gets added to the queue.

At this point the queue still contains the pending parent commit, with color
'keep', of the branch we want to include.
But we exit the findtwixt loop anyway because nskip == nqueued happens to
be true (both counters are 1).
This results in missing parent commits in the generated pack file.

Below the diff with my fix and your test, you can find my debug log.

ok?

diff d6a28ffe187127e3247254d7e242bb52d66eb26b /home/stsp/src/got
blob - bb8a404c064277aaedb6f0a5bd168e7765442df7
file + lib/pack_create.c
--- lib/pack_create.c
+++ lib/pack_create.c
@@ -1292,7 +1292,6 @@ paint_commits(int *ncolored, struct got_object_id_queu
 				err = paint_commit(qid, COLOR_SKIP);
 				if (err)
 					goto done;
-				nskip++;
 			} else
 				(*ncolored)++;
 			err = got_object_idset_add(keep, &qid->id, NULL);
@@ -1308,7 +1307,6 @@ paint_commits(int *ncolored, struct got_object_id_queu
 				err = paint_commit(qid, COLOR_SKIP);
 				if (err)
 					goto done;
-				nskip++;
 			} else
 				(*ncolored)++;
 			err = got_object_idset_add(drop, &qid->id, NULL);
blob - f96b6f089a1474bc624180cf278501ffb7f16af7
file + regress/cmdline/send.sh
--- regress/cmdline/send.sh
+++ regress/cmdline/send.sh
@@ -845,11 +845,17 @@ remote "origin" {
 EOF
 	got branch -r $testroot/repo foo
 
-	# modify alpha on branch foo
+	# modify beta on branch foo
 	got checkout -b foo $testroot/repo $testroot/wt > /dev/null
 	echo boo >> $testroot/wt/beta
 	(cd $testroot/wt && got commit -m 'changed beta on branch foo' \
 		> /dev/null)
+	echo buu >> $testroot/wt/beta
+	(cd $testroot/wt && got commit -m 'changed beta again on branch foo' \
+		> /dev/null)
+	echo baa >> $testroot/wt/beta
+	(cd $testroot/wt && got commit -m 'changed beta again on branch foo' \
+		> /dev/null)
 	local commit_id2=`git_show_branch_head $testroot/repo foo`
 
 	# tag HEAD commit of branch foo


got-send-pack: my capabilities: report-status delete-refs ofs-delta agent=got/0.71-current
got-send-pack: remote has refs/heads/master 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810
got-send-pack: remote has refs/remotes/origin/HEAD 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810
got-send-pack: remote has refs/remotes/origin/master 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810
got-send-pack: updating refs/heads/master 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 -> f94d6f451e31a1e0a75f09a56a160bf344e19f48
got-send-pack: creating refs/tags/1.0 de80598ac363b4eb8cd0573a07888c09962867a6
queue_commit_or_tag_id: keep f94d6f451e31a1e0a75f09a56a160bf344e19f48 type 1
queue_commit_id: queue f94d6f451e31a1e0a75f09a56a160bf344e19f48 color 0
queue_commit_or_tag_id: keep df43e4be82b4975a31366db7669255b0fc50ed44 type 1
queue_commit_id: queue df43e4be82b4975a31366db7669255b0fc50ed44 color 0
queue_commit_or_tag_id: drop 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 type 1
queue_commit_id: queue 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color 1
queue_commit_or_tag_id: drop 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 type 1
queue_commit_id: queue 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color 1
queue_commit_or_tag_id: drop 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 type 1
queue_commit_id: queue 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color 1
paint_commits: processing f94d6f451e31a1e0a75f09a56a160bf344e19f48 color keep
paint_commits: parent 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color keep
queue_commit_id: queue 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color 0
paint_commits: ids not empty, nskip=0 nqueued=5
paint_commits: processing df43e4be82b4975a31366db7669255b0fc50ed44 color keep
paint_commits: parent e0d261b0f00b427d936bc27b8bcbe982c3bc0ab1 color keep
queue_commit_id: queue e0d261b0f00b427d936bc27b8bcbe982c3bc0ab1 color 0
paint_commits: ids not empty, nskip=0 nqueued=5
paint_commits: processing 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color drop
paint_commits: ids not empty, nskip=0 nqueued=4
paint_commits: processing 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color drop
paint_commits: processing 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color drop
paint_commits: processing 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 color keep
paint_commits: 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 is in drop set, setting color to skip
paint_commits: nskip++, now 1
paint_commits: ids not empty, nskip=1 nqueued=1
append_id: keeping f94d6f451e31a1e0a75f09a56a160bf344e19f48
append_id: dropping 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810
append_id: keeping df43e4be82b4975a31366db7669255b0fc50ed44
add_object: exclude 1b204e7c7fd8c1cd29075c1bf798bcb75dbaa810 type 1
add_object: exclude df48b98c04c61ad621e55006e154cf76f50580be type 2
add_object: exclude 4a58007052a65fbc2fc3f910f2855f45a4058e74 type 3
add_object: exclude 65b2df87f7df3aeedef04be96703e55ac19c2cfb type 3
add_object: exclude f009e7cde4d52767055b91ddc8f1d32367f476b6 type 2
add_object: exclude fd08df0afa4d1d3faece37798d169e5a46d9d3fd type 3
add_object: exclude 43d3931c967119e70d9d2c0ea8ecc6695275a54a type 2
add_object: exclude ab135eefea6f73b921c7fec469b5f0e9db86b910 type 3
add_object: include f94d6f451e31a1e0a75f09a56a160bf344e19f48 type 1
add_object: include ea301a108fd72acf6afec2cbe4e24f3c844be1dd type 2
add_object: include b5ceb0306811eb823d1ecd6a6913fd0906062bba type 3
add_object: include df43e4be82b4975a31366db7669255b0fc50ed44 type 1
add_object: include 57a1b0e258432381b2492f131bcac7b3b937987d type 2
add_object: include 004b31fc53ed6d5e20bb33e4adf54288aa30df94 type 3
add_object: include de80598ac363b4eb8cd0573a07888c09962867a6 type 4
tag points to object df43e4be82b4975a31366db7669255b0fc50ed44 of type 1
error: Could not read e0d261b0f00b427d936bc27b8bcbe982c3bc0ab1
fatal: revision walk setup failed
error: Could not read e0d261b0f00b427d936bc27b8bcbe982c3bc0ab1
fatal: revision walk setup failed