{"thread":{"id":"24464","subject":"[PATCH] t2017: redo physical reflog existance check","startedAt":"2010-07-22T01:46:30Z","lastAt":"2010-08-24T18:57:53Z","messageCount":8,"participants":["Erick Mattos","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"145978","messageId":"1279763190-32757-1-git-send-email-erick.mattos@gmail.com","threadId":"24464","inReplyTo":null,"subject":"[PATCH] t2017: redo physical reflog existance check","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-07-22T01:46:30Z","receivedAt":"2010-07-22T01:46:30Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Commit 6e6842e wiped out physical reflog checks of tests because of\nredundancy and to hide low level detail of the implementation.\n\nAlthough this is not a problem to all the other changes, it is laming to\nthe tenth test. The implementation of the correspondent problem creates\na \"touch\" reflog that must be wiped out if not used by committing the\nnew branch.\n\nSigned-off-by: Erick Mattos \n---\n t/t2017-checkout-orphan.sh | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh\nindex 2d2f63f..0d5d0a0 100755\n--- a/t/t2017-checkout-orphan.sh\n+++ b/t/t2017-checkout-orphan.sh\n@@ -93,8 +93,10 @@ test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates =\n test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '\n \tgit checkout master &&\n \tgit checkout -l --orphan eta &&\n+\ttest -f .git/logs/refs/heads/eta &&\n \ttest_must_fail git rev-parse --verify eta@{0} &&\n \tgit checkout master &&\n+\t! test -f .git/logs/refs/heads/eta &&\n \ttest_must_fail git rev-parse --verify eta@{0}\n '\n \n-- \n1.7.2.1.ga86e3\n"},{"id":"146002","messageId":"7vlj93h120.fsf@alter.siamese.dyndns.org","threadId":"24464","inReplyTo":"1279763190-32757-1-git-send-email-erick.mattos@gmail.com","subject":"Re: [PATCH] t2017: redo physical reflog existance check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-22T17:35:35Z","receivedAt":"2010-07-22T17:35:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erick Mattos writes:\n\n> Although this is not a problem to all the other changes, it is laming to\n> the tenth test. The implementation of the correspondent problem creates\n> a \"touch\" reflog that must be wiped out if not used by committing the\n> new branch.\n\nI thought about it a bit when I sent out my patch, but I do not think that\nis necessary.\n\nThe things you care about, after running \"-l --orphan eta\", are:\n\n - If you make a commit, you get eta@{...} reflog that records it; and\n\n - If you leave the still-to-be-born eta branch without making a commit,\n you do not leave eta@{...} reflog behind.\n\nYour zeta@{...} test is about the former, and your eta@{...} test is about\nthe latter. I think they already check what they want to see happen.\n\nI also am afraid that the \"test -f\" check would expose the implementation\ndetail more than necessary. We may want to come up with a different\nimplementation of this behaviour later that may not create an empty file\nthere.\n"},{"id":"146009","messageId":"AANLkTilt5gx3Wj4eANfkIFm869Olns1rsMpCS81hS2BV@mail.gmail.com","threadId":"24464","inReplyTo":"7vlj93h120.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t2017: redo physical reflog existance check","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-07-22T18:58:42Z","receivedAt":"2010-07-22T18:58:42Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\n2010/7/22 Junio C Hamano \n> I thought about it a bit when I sent out my patch, but I do not think that\n> is necessary.\n>\n> The things you care about, after running \"-l --orphan eta\", are:\n>\n>  - If you make a commit, you get eta@{...} reflog that records it; and\n>\n>  - If you leave the still-to-be-born eta branch without making a commit,\n>   you do not leave eta@{...} reflog behind.\n>\n> Your zeta@{...} test is about the former, and your eta@{...} test is about\n> the latter.  I think they already check what they want to see happen.\n\nYou have separate my concerns in the scripts very well but the result\nyou pointed out is not quite true.\n\nFor zeta, not testing physical existence of reflog file is not really\nimportant because at the end you will have the reflog created anyway,\nwhich is well tested by latter \"git rev-parse --verify\". But that is\nnot the case of eta therefore the check is necessary.\n\nThe solution to the problem of creating reflogs when using -l and\n--orphan while configured core.logAllRefUpdates=false was a simple\ntrick, completely based on actual implementation of reflog saving:\nreflogs are saved when there is already a reflog file.\n\nTo make the new orphan branch ready to have a reflog on that config\nwas as simple as creating a \"touch file\" for reflog. This is the goal\nachieved by code. Testing this goal by checking the touch reflog file\nis important while in zeta case redundant, because of the final result\nof having a reflog saved and consequently the touch file filled with\nreal reflog data.\n\nBut this clean solution should not leave a bogus file in case it is\nnot being used by canceling the creation of the new orphan branch and\ndoing a checkout to another branch.\n\nSo in eta case it is important to check if the reflog physical file is\nreally deleted. No reflog will be created anyway in eta case so \"git\nref-parse --verify\" is not even relevant. I have added formal reflog\ncheck just in case.\n\nTesting more is always better than testing less so I prefered to be\nredundant and test thoroughly in all levels of detail.\n\n> I also am afraid that the \"test -f\" check would expose the implementation\n> detail more than necessary.  We may want to come up with a different\n> implementation of this behaviour later that may not create an empty file\n> there.\n\nNo exposure is being done by using \"test -f\" inside a script which its\nsole purpose is to check a controlled event for developers. Folder t\nhas \"test -f\" being employed in 92 scripts.\n\nThe only reason for the t folder is to let the developers be aware of\nwhat they are possibly breaking, isn't it?!\n\nI think this implementation is quite good. I don't see a reason for\nchanging it.\n\nThe point is that you give a command (-l --orphan on\ncore.logAllRefUpdates=false config) that have to save the preference\nof creating the reflog for later. Data need to be saved once git is a\nsimple call-run-quit software. And the way it is being saved is, at\nminimum, very efficient. No outside file or special config is being\ncreated and no memory persistent variable/objects is being left\nbehind. The implementation is working as expected and the subject is\nonly the testing script.\n\nWe have to remember that all we are talking here is about a very\nuncommon situation when core.logAllRefUpdates is set to false. I\npersonally don't even foresee a possible reason for not having reflogs\nsaved automatically anyway. :-|\n\nRegards\n"},{"id":"146038","messageId":"7vsk3bey1e.fsf@alter.siamese.dyndns.org","threadId":"24464","inReplyTo":"AANLkTilt5gx3Wj4eANfkIFm869Olns1rsMpCS81hS2BV@mail.gmail.com","subject":"Re: [PATCH] t2017: redo physical reflog existance check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-23T02:23:41Z","receivedAt":"2010-07-23T02:23:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erick Mattos writes:\n\n> To make the new orphan branch ready to have a reflog on that config\n> was as simple as creating a \"touch file\" for reflog. This is the goal\n> achieved by code.\n\nI have to say that you are somewhat confused about the _goal_ then. touch\nis not a goal, it is means to a goal.\n\nIt does not matter how you implement the user visible effect, be it a\ncreation of an empty file, or some other means [*1*]. What matters is\nthat the user won't get a reflog for a branch that really didn't get\ncreated and must-fail \"rev-parse --verify\" test checks that.\n\nAnother thing that could matter would be that future actions that want to\ncreate a reflog for the same branch (perhaps after the user switches to\n'master', another attempt is made to create eta with \"checkout -b eta\") or\nanother branch with a similar or related name (say \"eta/real\") are not get\nbroken by whatever you do to implement the \"we want to create a reflog\nwhen a ref is actually made but not right now\" feature. Perhaps the right\nway to test that would be to actually try to run such operations and make\nsure they do not fail.\n\n\n[Footnote]\n\n*1* For example, you could have implemented the feature by adding a config\nitem in \".git/config: [branch \"eta\"] need-to-create-reflog\", and taught\nrefs.c::update_ref() to pay attention to it (I am not saying that it would\nbe a better implementation).\n"},{"id":"146075","messageId":"AANLkTin76s-ONFuP+OWdxB5LJNf2D1Du+hKxB2s_WhTa@mail.gmail.com","threadId":"24464","inReplyTo":"7vsk3bey1e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t2017: redo physical reflog existance check","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-07-23T15:04:37Z","receivedAt":"2010-07-23T15:04:37Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"2010/7/22 Junio C Hamano :\n> Erick Mattos writes:\n>\n>> To make the new orphan branch ready to have a reflog on that config\n>> was as simple as creating a \"touch file\" for reflog.  This is the goal\n>> achieved by code.\n>\n> I have to say that you are somewhat confused about the _goal_ then.  touch\n> is not a goal, it is means to a goal.\n\nI believe you have understood the whole idea of my previous email even\nthough _words_ means a lot to you. :-1\n\nI meant that that was the code goal. It was the implementation\nobjective which by itself was the solution chosen for the real\nproblem.\n\n> It does not matter how you implement the user visible effect, be it a\n> creation of an empty file, or some other means [*1*].  What matters is\n> that the user won't get a reflog for a branch that really didn't get\n> created and must-fail \"rev-parse --verify\" test checks that.\n>\n> Another thing that could matter would be that future actions that want to\n> create a reflog for the same branch (perhaps after the user switches to\n> 'master', another attempt is made to create eta with \"checkout -b eta\") or\n> another branch with a similar or related name (say \"eta/real\") are not get\n> broken by whatever you do to implement the \"we want to create a reflog\n> when a ref is actually made but not right now\" feature.  Perhaps the right\n> way to test that would be to actually try to run such operations and make\n> sure they do not fail.\n\nYou were cutting off redundancy checks, weren't you? If there is no\nreflog file (tested by \"test -f\"), consequently no reflog (tested by\n\"rev-parse --verify\") and no branch with the chosen name, certainly\nsomeone can create one the \"almost used\" name, once there is no any\ndifference from a normal situation.\n\n> [Footnote]\n>\n> *1* For example, you could have implemented the feature by adding a config\n> item in \".git/config: [branch \"eta\"] need-to-create-reflog\", and taught\n> refs.c::update_ref() to pay attention to it (I am not saying that it would\n> be a better implementation).\n\nAbout the late parenthesis: thank God! ;-)\n\nI don't see a need for so much reluctance: \"test -f\" is not a taboo\ninside a script in t folder and the added tests don't change anything\nabout the design and implementation which IMHO is well fit.\n\nWith those two patch lines of mine \"--orphan with -l and\ncore.logAllRefUpdates=false\" testing script is finished.\n\nFinally: you are the man in charge so I would really like to enforce\nthat if you need me to do anything more I will be _really_ glad to\nhelp. I love git and everything good done to it it is done to me too\nas one of its daily user.\n\nBest regards\n"},{"id":"148809","messageId":"20100824040347.GA19817@burratino","threadId":"24464","inReplyTo":"AANLkTin76s-ONFuP+OWdxB5LJNf2D1Du+hKxB2s_WhTa@mail.gmail.com","subject":"Re: [PATCH] t2017: redo physical reflog existance check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-24T04:03:47Z","receivedAt":"2010-08-24T04:03:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Erick,\n\nFirst, thanks for checkout --orphan. I was a skeptic but now that I\nhave seen it being used for things similar to \"going open source\" (and\nthe related \"simplifying logs while working privately on a patch\nseries\") it looks to be a pretty nice tool.\n\nErick Mattos wrote:\n\n> I don't see a need for so much reluctance: \"test -f\" is not a taboo\n> inside a script in t folder and the added tests don't change anything\n> about the design and implementation which IMHO is well fit.\n\nThe principle (though we do not always adhere to it) is that test\nscripts should pass or fail based only on advertised behavior, not\nimplementation details. That way, _later_ any person who wants to\nimprove the implementation will not be impeded by tests.\n\nThe behavior that \"test -f .git/logs/refs/heads/eta\" checks for is not\npart of the advertised behavior and though it does affect the\nobservable behavior, it is not immediately obvious how. Wouldn't it\nbe best if the test described that advertised behavior while checking\nfor it?\n\ne.g.:\n\n\tgit config core.logallrefupdates false &&\n\ttest_when_finished \"git config core.logallrefupdates true\" &&\n\n \tgit checkout master &&\n \tgit checkout -l --orphan eta &&\n\ttest_must_fail git rev-parse --verify eta@{0} &&\n\n\ttest_tick &&\n\tgit commit -m \"initial commit\" &&\n\tgit rev-parse --verify eta@{0}\n\nHappily, I am not the man in charge, so feel free to take my words\nat whatever value you choose. :)\n\nRegards,\nJonathan\n"},{"id":"148848","messageId":"7v8w3wx823.fsf@alter.siamese.dyndns.org","threadId":"24464","inReplyTo":"20100824040347.GA19817@burratino","subject":"Re: [PATCH] t2017: redo physical reflog existance check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-24T16:57:24Z","receivedAt":"2010-08-24T16:57:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The principle (though we do not always adhere to it) is that test\n> scripts should pass or fail based only on advertised behavior, not\n> implementation details. That way, _later_ any person who wants to\n> improve the implementation will not be impeded by tests.\n\nWell and 'nuff said ;-) Thanks.\n"},{"id":"148857","messageId":"AANLkTiki33oaQjnY8oxdAH1NLuTV0epCRz1XSp0irg9f@mail.gmail.com","threadId":"24464","inReplyTo":"20100824040347.GA19817@burratino","subject":"Re: [PATCH] t2017: redo physical reflog existance check","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-08-24T18:57:53Z","receivedAt":"2010-08-24T18:57:53Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\n2010/8/24 Jonathan Nieder \n> Hi Erick,\n>\n> First, thanks for checkout --orphan.  I was a skeptic but now that I\n> have seen it being used for things similar to \"going open source\" (and\n> the related \"simplifying logs while working privately on a patch\n> series\") it looks to be a pretty nice tool.\n\nYou're welcome! It is my pleasure, really! The reason for me to start\nparticipating is a concern to pay back some of the benefits I have\nbeen having by using free software.\n\nI wish I could help all the excellent programs I use but I intend to\nhelp at least some. Nonetheless I would like to do it to all.\n\nSo I am really grateful that you let me know my work was useful to you.\n\nI have been addressing problems I had when dealing with git and\n--orphan is important for private branch working. Even that I use\n--reset-author more often.\n\nI would eventually add more stuff to git, while it is already quite a\nwonderful tool right now though It can be enhanced. However I don't\nplan to because I had been facing extreme opposition and I don't like\nto disturb. I only want to spend my effort where it helps.\n\n> Erick Mattos wrote:\n>\n> > I don't see a need for so much reluctance: \"test -f\" is not a taboo\n> > inside a script in t folder and the added tests don't change anything\n> > about the design and implementation which IMHO is well fit.\n>\n> The principle (though we do not always adhere to it) is that test\n> scripts should pass or fail based only on advertised behavior, not\n> implementation details.  That way, _later_ any person who wants to\n> improve the implementation will not be impeded by tests.\n\nTests won't impede a developer but it will show him that he is\nchanging something in fact. And they will tell him he needs to adjust\nthe tests too.\n\nBut in general it is a good practice to adhere to that principle\nunless implementation is what you are really testing. At this case,\nall right behavior had their tests before it and this one is to check\nfor a bad implementation.\n\n\"The problem\", test -f, is used 428 times inside 92 scripts and in a\nsimilar way (test -f .git/...) 48 times inside 19 scripts. So I don't\nthink it is correct that I have to struggle so much to use it too.\n\nAnd the major fact is that the actual test is testing NOTHING and that\nthe test lamed IS important!\n\n> The behavior that \"test -f .git/logs/refs/heads/eta\" checks for is not\n> part of the advertised behavior and though it does affect the\n> observable behavior, it is not immediately obvious how.  Wouldn't it\n> be best if the test described that advertised behavior while checking\n> for it?\n\nI really haven't got you point. Let's see:\n\n'giving up --orphan not committed when -l and core.logAllRefUpdates =\nfalse deletes reflog'\n\nDon't you think reflog deletion is immediately obvious?\n\n> e.g.:\n>\n>        git config core.logallrefupdates false &&\n>        test_when_finished \"git config core.logallrefupdates true\" &&\n>\n>        git checkout master &&\n>        git checkout -l --orphan eta &&\n>        test_must_fail git rev-parse --verify eta@{0} &&\n>\n>        test_tick &&\n>        git commit -m \"initial commit\" &&\n>        git rev-parse --verify eta@{0}\n\nYour test is not the same and that is already done by zeta: '--orphan\nwith -l makes reflog when core.logAllRefUpdates = false'.\n\n> Happily, I am not the man in charge, so feel free to take my words\n> at whatever value you choose. :)\n\nNot quite happily though.\n\nUnfortunately the problem which git does not address for now is that\ngit have cemented the distributed repository but it really haven't\ncreated a distributed development workflow.\n\nSomeone still have to create a workflow where people merge code,\ndesign, give the directions, do quality optimization and more managing\nthings TOGETHER.\n\nWhile everybody have the right to access and work separately, there is\nno existent tool to integrate the work without a need for a moderator\nwhich is always a flow constriction.\n\nYou can see Git and Linux as example: while the widespread use of\ndistributed repositories, every management is closed to the\nmaintainers which can possibly ignore good stuff or hasty accept later\nproved bad deals.\n\nThat dictatorial guidance leads to political problems, inherent of\nhuman being existence, putting code subscribed to who is writing it\nand not to how good it is.\n\nI think that is the major reason for so much forks and developer\ndesertions nowadays and for not merging potential good stuff because\nof people animosity.\n\nI believe someone very clever will soon find a way for not depending\nso much of one person alone and get a way of making code evolution\nhappen by itself in a real distributed work flow.\n\n> Regards,\n> Jonathan\n\nYour words value much. So I would appreciate to know your following perceptions.\n\nIt is always a pleasure to talk to you and to have an opportunity to\nget your always objective and balanced point-of-views.\n\nBest regards\n"}]}