False positive from orphaned_commit_warning() ?

False positive from orphaned_commit_warning() ?

[Paul Gortmaker]

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

Re: False positive from orphaned_commit_warning() ?

[Dan Johnson]

On Wed, Jul 25, 2012 at 2:53 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> 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.

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

Re: False positive from orphaned_commit_warning() ?

[Junio C Hamano]

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

> Has anyone else noticed false positives coming from the
> orphan check?

Thanks.  This should fix it.

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6acca75..d812219 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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;
 }
 

Re: False positive from orphaned_commit_warning() ?

[Jeff King]

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

> Paul Gortmaker <paul.gortmaker@windriver.com> writes:
> 
> > Has anyone else noticed false positives coming from the
> > orphan check?
> 
> Thanks.  This should fix it.

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a76899d..f855489 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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;
 }
 

Re: False positive from orphaned_commit_warning() ?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 25, 2012 at 02:52:54PM -0700, Junio C Hamano wrote:
>
>> Paul Gortmaker <paul.gortmaker@windriver.com> writes:
>> 
>> > Has anyone else noticed false positives coming from the
>> > orphan check?
>> 
>> Thanks.  This should fix it.
>
> 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.

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.

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

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.

> results, as we were setting random flag bits on objects in
> the traversal.
> ---
>  builtin/checkout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a76899d..f855489 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -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;
>  }
>  

Re: False positive from orphaned_commit_warning() ?

[Jeff King]

On Wed, Jul 25, 2012 at 03:05:42PM -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.
> 
> 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 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 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".

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

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

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".

> What's interesting still is that
> 
> 	git checkout master~
>         git checkout master
> 
> does not exhibit this problem in the same repository.

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

Re: False positive from orphaned_commit_warning() ?

[Paul Gortmaker]

On 12-07-25 05:52 PM, Junio C Hamano wrote:
> Paul Gortmaker <paul.gortmaker@windriver.com> writes:
> 
>> Has anyone else noticed false positives coming from the
>> orphan check?
> 
> Thanks.  This should fix it.

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

Paul.
--

> 
>  builtin/checkout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 6acca75..d812219 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -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;
>  }
>  
>