False positive from orphaned_commit_warning() ?

Has anyone else noticed false positives coming from the orphan check? It is warning me about commits that are clearly on master. Here is an example, where I checkout master~2 and then switch back to master. It somehow thinks that master~2 is orphaned, when master~2 is by definition in the commit chain leading to master.

The repo is tiny, so anyone can try and reproduce this. (I've done so on v1.7.9 and v1.7.11, on two different machines).

git://git.yoctoproject.org/yocto-kernel-tools.git

Paul.
-------------

paul@foo:~/git/yocto-kernel-tools$ git checkout master~2
Note: checking out 'master~2'.

You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may do so (now or later) by using -b with the checkout command again. Example:

git checkout -b new_branch_name

HEAD is now at e693754... kgit-checkpoint: fix verify_branch variable name typo

paul@foo:~/git/yocto-kernel-tools$ git checkout master
Warning: you are leaving 38 commits behind, not connected to
any of your branches:

e693754 kgit-checkpoint: fix verify_branch variable name typo ee67a7b kgit-config-cleaner: fix redefintion processing 579b1ba meta: support flexible meta branch naming 4673bdb scc: allow kconf fragment searching
... and 34 more.

If you want to keep them by creating a new branch, this may be a good time to do so with:

git branch new_branch_name e6937544e030637cec029edee34737846a036ece

Switched to branch 'master'

paul@foo:~/git/yocto-kernel-tools$ git branch --contains e6937544e030637cec029edee34737846a036ece
* master
paul@foo:~/git/yocto-kernel-tools$ git --version
git version 1.7.11.1
paul@foo:~/git/yocto-kernel-tools$ cat .git/config 
[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	fetch = +refs/heads/*:refs/remotes/origin/*
	url = git://git.yoctoproject.org/yocto-kernel-tools.git
[branch "master"]
	remote = origin
	merge = refs/heads/master
paul@foo:~/git/yocto-kernel-tools$ 
---------------

Thanks. This should fix it.

 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6acca75..d812219 100644
@@ -606,7 +606,7 @@ static int add_pending_uninteresting_ref(const char *refname,
 					 const unsigned char *sha1,
 					 int flags, void *cb_data)
 {
-	add_pending_sha1(cb_data, refname, sha1, flags | UNINTERESTING);
+	add_pending_sha1(cb_data, refname, sha1, UNINTERESTING);
 	return 0;
 }
 

I've just been hunting the same bug and came up with the same answer. Here's a commit message. Feel free to apply or steal text for your commit.

-- >8 --
Subject: [PATCH] checkout: don't confuse ref and object flags

When we are leaving a detached HEAD, we do a revision traversal to check whether we are orphaning any commits, marking the commit we're leaving as the start of the traversal, and all existing refs as uninteresting.

Prior to commit 468224e5, we did so by calling for_each_ref, and feeding each resulting refname to setup_revisions. Commit 468224e5 refactored this to simply mark the pending objects, saving an extra lookup.

However, it confused the "flags" parameter to the each_ref_fn clalback, which is about the flags we found while looking up the ref (e.g., REF_ISSYMREF) with the object flag (UNINTERESTING), leading to unpredictable results, as we were setting random flag bits on objects in the traversal.

---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a76899d..f855489 100644
@@ -592,7 +592,7 @@ static int add_pending_uninteresting_ref(const char *refname,
 					 const unsigned char *sha1,
 					 int flags, void *cb_data)
 {
-	add_pending_sha1(cb_data, refname, sha1, flags | UNINTERESTING);
+	add_pending_sha1(cb_data, refname, sha1, UNINTERESTING);
 	return 0;
 }
 

Heh, let's try not to waste duplicated efforts by being silent next time, OK? Winning such a race by 5 minutes does not buy us much.

I wish we had some type safe way to say "This uint and the other uint are to hold different kinds of flag bits; do not mix them by bitwise operators".

Thanks.

s/UNINTERESTING/SEEN/; I think.

What was happening was that the remotes/origin/HEAD symref happened to point at the same commit as "master", and ^master that was in the pending array was not transferred to the commit list used by the revision traversal.

What's interesting still is that

git checkout master~
git checkout master

does not exhibit this problem in the same repository.

I usually do, but this bug was surprisingly easy to find once I bisected. I don't think it took more than 5 minutes total. :)

I believe that bitwise operations are defined for enums, but I might be misremembering my C89.

Yeah, I that was my guess, too, but I didn't investigate exactly which bits were twiddled. By mentioning UNINTERESTING, I meant "this is the flag we actually _care_ about, but we are setting other random ones depending on the ref flags".

Perhaps we still look at and mark the parents of a SEEN commit during our traversal (but not any further). I suspect it is that mark_parents_uninteresting does so, but does not bother to parse the parent. I didn't check carefully. It may be that we have an over-conservative off-by-one at the boundaries of our traversal in some cases, but I doubt it is worth the effort to optimize.

-Peff

Indeed it does. Thanks for the fix (and git in general).

Paul.
--

I've been able to reproduce this with the following simplified recipe, although I still don't know what is causing the failure (I'm not very familiar with the code)

git init test
cd test
#make 3 commits
touch a && git add a && git commit -m a
touch b && git add b && git commit -m b
touch c && git add c && git commit -m c

#clone it
cd ..
git clone test test2
cd test2
git checkout master~2
git checkout master
#Warning: you are leaving 1 commit behind, not connected to #any of your branches

I can't figure out what's going wrong here, but the clone is important; it doesn't fail without it. It appears to have something to do with the fact that the cloned repository has a remote, as:
#in test2
git remote rm origin
git checkout master~2
git checkout master

Does not throw the warning, but it's not just the presence of origin/master that triggers it, as:

cd ../test
git remote add origin ../test2
git fetch origin
git checkout master~2
git checkout master

Does not trigger it either.

Confused,

-Dan