{"thread":{"id":"31100","subject":"False positive from orphaned_commit_warning() ?","startedAt":"2012-07-25T18:53:43Z","lastAt":"2012-07-26T13:22:13Z","messageCount":7,"participants":["Paul Gortmaker","Dan Johnson","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"195776","messageId":"20120725185343.GA6937@windriver.com","threadId":"31100","inReplyTo":null,"subject":"False positive from orphaned_commit_warning() ?","fromName":"Paul Gortmaker","fromEmail":"paul.gortmaker@windriver.com","sentAt":"2012-07-25T18:53:43Z","receivedAt":"2012-07-25T18:53:43Z","isPatch":false,"sender":{"key":"paul.gortmaker@windriver.com","avatar":null},"body":"Has anyone else noticed false positives coming from the\norphan check? It is warning me about commits that are\nclearly on master. Here is an example, where I checkout\nmaster~2 and then switch back to master. It somehow thinks\nthat master~2 is orphaned, when master~2 is by definition\nin the commit chain leading to master.\n\nThe repo is tiny, so anyone can try and reproduce this. (I've\ndone so on v1.7.9 and v1.7.11, on two different machines).\n\ngit://git.yoctoproject.org/yocto-kernel-tools.git\n\nPaul.\n-------------\n\npaul@foo:~/git/yocto-kernel-tools$ git checkout master~2\nNote: checking out 'master~2'.\n\nYou are in 'detached HEAD' state. You can look around, make experimental\nchanges and commit them, and you can discard any commits you make in\nthis\nstate without impacting any branches by performing another checkout.\n\nIf you want to create a new branch to retain commits you create, you may\ndo so (now or later) by using -b with the checkout command again.\nExample:\n\n git checkout -b new_branch_name\n\nHEAD is now at e693754... kgit-checkpoint: fix verify_branch variable\nname typo\npaul@foo:~/git/yocto-kernel-tools$ git checkout master\nWarning: you are leaving 38 commits behind, not connected to\nany of your branches:\n\n e693754 kgit-checkpoint: fix verify_branch variable name typo\n ee67a7b kgit-config-cleaner: fix redefintion processing\n 579b1ba meta: support flexible meta branch naming\n 4673bdb scc: allow kconf fragment searching\n ... and 34 more.\n\nIf you want to keep them by creating a new branch, this may be a good time\nto do so with:\n\n git branch new_branch_name e6937544e030637cec029edee34737846a036ece\n\nSwitched to branch 'master'\npaul@foo:~/git/yocto-kernel-tools$ git branch --contains e6937544e030637cec029edee34737846a036ece\n* master\npaul@foo:~/git/yocto-kernel-tools$ git --version\ngit version 1.7.11.1\npaul@foo:~/git/yocto-kernel-tools$ cat .git/config \n[core]\n\trepositoryformatversion = 0\n\tfilemode = true\n\tbare = false\n\tlogallrefupdates = true\n[remote \"origin\"]\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n\turl = git://git.yoctoproject.org/yocto-kernel-tools.git\n[branch \"master\"]\n\tremote = origin\n\tmerge = refs/heads/master\npaul@foo:~/git/yocto-kernel-tools$ \n---------------\n"},{"id":"195783","messageId":"CAPBPrnugVm0RS5+Ljgg2E-AJygYsOSRjZW0Z=o9Mavs-SxTBog@mail.gmail.com","threadId":"31100","inReplyTo":"20120725185343.GA6937@windriver.com","subject":"Re: False positive from orphaned_commit_warning() ?","fromName":"Dan Johnson","fromEmail":"computerdruid@gmail.com","sentAt":"2012-07-25T20:43:38Z","receivedAt":"2012-07-25T20:43:38Z","isPatch":false,"sender":{"key":"computerdruid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/34696?v=4"},"body":"On Wed, Jul 25, 2012 at 2:53 PM, Paul Gortmaker\n wrote:\n> Has anyone else noticed false positives coming from the\n> orphan check? It is warning me about commits that are\n> clearly on master. Here is an example, where I checkout\n> master~2 and then switch back to master. It somehow thinks\n> that master~2 is orphaned, when master~2 is by definition\n> in the commit chain leading to master.\n\nI've been able to reproduce this with the following simplified recipe,\nalthough I still don't know what is causing the failure (I'm not very\nfamiliar with the code)\n\ngit init test\ncd test\n#make 3 commits\ntouch a && git add a && git commit -m a\ntouch b && git add b && git commit -m b\ntouch c && git add c && git commit -m c\n\n#clone it\ncd ..\ngit clone test test2\ncd test2\ngit checkout master~2\ngit checkout master\n#Warning: you are leaving 1 commit behind, not connected to\n#any of your branches\n\n\nI can't figure out what's going wrong here, but the clone is\nimportant; it doesn't fail without it. It appears to have something to\ndo with the fact that the cloned repository has a remote, as:\n#in test2\ngit remote rm origin\ngit checkout master~2\ngit checkout master\n\nDoes not throw the warning, but it's not just the presence of\norigin/master that triggers it, as:\n\ncd ../test\ngit remote add origin ../test2\ngit fetch origin\ngit checkout master~2\ngit checkout master\n\nDoes not trigger it either.\n\nConfused,\n-- \n-Dan\n"},{"id":"195793","messageId":"7va9ynbj9l.fsf@alter.siamese.dyndns.org","threadId":"31100","inReplyTo":"20120725185343.GA6937@windriver.com","subject":"Re: False positive from orphaned_commit_warning() ?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T21:52:54Z","receivedAt":"2012-07-25T21:52:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Gortmaker writes:\n\n> Has anyone else noticed false positives coming from the\n> orphan check?\n\nThanks. This should fix it.\n\n builtin/checkout.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6acca75..d812219 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -606,7 +606,7 @@ static int add_pending_uninteresting_ref(const char *refname,\n \t\t\t\t\t const unsigned char *sha1,\n \t\t\t\t\t int flags, void *cb_data)\n {\n-\tadd_pending_sha1(cb_data, refname, sha1, flags | UNINTERESTING);\n+\tadd_pending_sha1(cb_data, refname, sha1, UNINTERESTING);\n \treturn 0;\n }\n \n"},{"id":"195796","messageId":"20120725215730.GA30966@sigill.intra.peff.net","threadId":"31100","inReplyTo":"7va9ynbj9l.fsf@alter.siamese.dyndns.org","subject":"Re: False positive from orphaned_commit_warning() ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-25T21:57:30Z","receivedAt":"2012-07-25T21:57:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 25, 2012 at 02:52:54PM -0700, Junio C Hamano wrote:\n\n> Paul Gortmaker writes:\n> \n> > Has anyone else noticed false positives coming from the\n> > orphan check?\n> \n> Thanks. This should fix it.\n\nI've just been hunting the same bug and came up with the same answer.\nHere's a commit message. Feel free to apply or steal text for your\ncommit.\n\n-- >8 --\nSubject: [PATCH] checkout: don't confuse ref and object flags\n\nWhen we are leaving a detached HEAD, we do a revision\ntraversal to check whether we are orphaning any commits,\nmarking the commit we're leaving as the start of the\ntraversal, and all existing refs as uninteresting.\n\nPrior to commit 468224e5, we did so by calling for_each_ref,\nand feeding each resulting refname to setup_revisions.\nCommit 468224e5 refactored this to simply mark the pending\nobjects, saving an extra lookup.\n\nHowever, it confused the \"flags\" parameter to the\neach_ref_fn clalback, which is about the flags we found\nwhile looking up the ref (e.g., REF_ISSYMREF) with the\nobject flag (UNINTERESTING), leading to unpredictable\nresults, as we were setting random flag bits on objects in\nthe traversal.\n---\n builtin/checkout.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a76899d..f855489 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -592,7 +592,7 @@ static int add_pending_uninteresting_ref(const char *refname,\n \t\t\t\t\t const unsigned char *sha1,\n \t\t\t\t\t int flags, void *cb_data)\n {\n-\tadd_pending_sha1(cb_data, refname, sha1, flags | UNINTERESTING);\n+\tadd_pending_sha1(cb_data, refname, sha1, UNINTERESTING);\n \treturn 0;\n }\n \n"},{"id":"195797","messageId":"7v629bbio9.fsf@alter.siamese.dyndns.org","threadId":"31100","inReplyTo":"20120725215730.GA30966@sigill.intra.peff.net","subject":"Re: False positive from orphaned_commit_warning() ?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T22:05:42Z","receivedAt":"2012-07-25T22:05:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 25, 2012 at 02:52:54PM -0700, Junio C Hamano wrote:\n>\n>> Paul Gortmaker writes:\n>> \n>> > Has anyone else noticed false positives coming from the\n>> > orphan check?\n>> \n>> Thanks. This should fix it.\n>\n> I've just been hunting the same bug and came up with the same answer.\n> Here's a commit message. Feel free to apply or steal text for your\n> commit.\n\nHeh, let's try not to waste duplicated efforts by being silent next\ntime, OK? Winning such a race by 5 minutes does not buy us much.\n\nI wish we had some type safe way to say \"This uint and the other\nuint are to hold different kinds of flag bits; do not mix them by\nbitwise operators\".\n\nThanks.\n\n> -- >8 --\n> Subject: [PATCH] checkout: don't confuse ref and object flags\n>\n> When we are leaving a detached HEAD, we do a revision\n> traversal to check whether we are orphaning any commits,\n> marking the commit we're leaving as the start of the\n> traversal, and all existing refs as uninteresting.\n>\n> Prior to commit 468224e5, we did so by calling for_each_ref,\n> and feeding each resulting refname to setup_revisions.\n> Commit 468224e5 refactored this to simply mark the pending\n> objects, saving an extra lookup.\n>\n> However, it confused the \"flags\" parameter to the\n> each_ref_fn clalback, which is about the flags we found\n> while looking up the ref (e.g., REF_ISSYMREF) with the\n> object flag (UNINTERESTING), leading to unpredictable\n\ns/UNINTERESTING/SEEN/; I think.\n\nWhat was happening was that the remotes/origin/HEAD symref happened\nto point at the same commit as \"master\", and ^master that was in the\npending array was not transferred to the commit list used by the\nrevision traversal.\n\nWhat's interesting still is that\n\n\tgit checkout master~\n git checkout master\n\ndoes not exhibit this problem in the same repository.\n\n> results, as we were setting random flag bits on objects in\n> the traversal.\n> ---\n> builtin/checkout.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index a76899d..f855489 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -592,7 +592,7 @@ static int add_pending_uninteresting_ref(const char *refname,\n> \t\t\t\t\t const unsigned char *sha1,\n> \t\t\t\t\t int flags, void *cb_data)\n> {\n> -\tadd_pending_sha1(cb_data, refname, sha1, flags | UNINTERESTING);\n> +\tadd_pending_sha1(cb_data, refname, sha1, UNINTERESTING);\n> \treturn 0;\n> }\n> \n"},{"id":"195801","messageId":"20120725223159.GA31134@sigill.intra.peff.net","threadId":"31100","inReplyTo":"7v629bbio9.fsf@alter.siamese.dyndns.org","subject":"Re: False positive from orphaned_commit_warning() ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-25T22:31:59Z","receivedAt":"2012-07-25T22:31:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 25, 2012 at 03:05:42PM -0700, Junio C Hamano wrote:\n\n> > I've just been hunting the same bug and came up with the same answer.\n> > Here's a commit message. Feel free to apply or steal text for your\n> > commit.\n> \n> Heh, let's try not to waste duplicated efforts by being silent next\n> time, OK? Winning such a race by 5 minutes does not buy us much.\n\nI usually do, but this bug was surprisingly easy to find once I\nbisected. I don't think it took more than 5 minutes total. :)\n\n> I wish we had some type safe way to say \"This uint and the other\n> uint are to hold different kinds of flag bits; do not mix them by\n> bitwise operators\".\n\nI believe that bitwise operations are defined for enums, but I might be\nmisremembering my C89.\n\n> > However, it confused the \"flags\" parameter to the\n> > each_ref_fn clalback, which is about the flags we found\n> > while looking up the ref (e.g., REF_ISSYMREF) with the\n> > object flag (UNINTERESTING), leading to unpredictable\n> \n> s/UNINTERESTING/SEEN/; I think.\n> \n> What was happening was that the remotes/origin/HEAD symref happened\n> to point at the same commit as \"master\", and ^master that was in the\n> pending array was not transferred to the commit list used by the\n> revision traversal.\n\nYeah, I that was my guess, too, but I didn't investigate exactly which\nbits were twiddled. By mentioning UNINTERESTING, I meant \"this is the\nflag we actually _care_ about, but we are setting other random ones\ndepending on the ref flags\".\n\n> What's interesting still is that\n> \n> \tgit checkout master~\n> git checkout master\n> \n> does not exhibit this problem in the same repository.\n\nPerhaps we still look at and mark the parents of a SEEN commit during\nour traversal (but not any further). I suspect it is that\nmark_parents_uninteresting does so, but does not bother to parse the\nparent. I didn't check carefully. It may be that we have an\nover-conservative off-by-one at the boundaries of our traversal in some\ncases, but I doubt it is worth the effort to optimize.\n\n-Peff\n"},{"id":"195852","messageId":"50114485.80309@windriver.com","threadId":"31100","inReplyTo":"7va9ynbj9l.fsf@alter.siamese.dyndns.org","subject":"Re: False positive from orphaned_commit_warning() ?","fromName":"Paul Gortmaker","fromEmail":"paul.gortmaker@windriver.com","sentAt":"2012-07-26T13:22:13Z","receivedAt":"2012-07-26T13:22:13Z","isPatch":false,"sender":{"key":"paul.gortmaker@windriver.com","avatar":null},"body":"On 12-07-25 05:52 PM, Junio C Hamano wrote:\n> Paul Gortmaker writes:\n> \n>> Has anyone else noticed false positives coming from the\n>> orphan check?\n> \n> Thanks. This should fix it.\n\nIndeed it does. Thanks for the fix (and git in general).\n\nPaul.\n--\n\n> \n> builtin/checkout.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 6acca75..d812219 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -606,7 +606,7 @@ static int add_pending_uninteresting_ref(const char *refname,\n> \t\t\t\t\t const unsigned char *sha1,\n> \t\t\t\t\t int flags, void *cb_data)\n> {\n> -\tadd_pending_sha1(cb_data, refname, sha1, flags | UNINTERESTING);\n> +\tadd_pending_sha1(cb_data, refname, sha1, UNINTERESTING);\n> \treturn 0;\n> }\n> \n> \n"}]}