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$ 
---------------

On Wed, Jul 25, 2012 at 2:53 PM, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:

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,

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

Thanks.  This should fix it.

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

On Wed, Jul 25, 2012 at 02:52:54PM -0700, Junio C Hamano wrote:

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(-)

Jeff King <peff@peff.net> writes:

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.

On Wed, Jul 25, 2012 at 03:05:42PM -0700, Junio C Hamano wrote:

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

On 12-07-25 05:52 PM, Junio C Hamano wrote:

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

Paul. --