{"thread":{"id":"18186","subject":"[PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","startedAt":"2009-03-07T09:30:51Z","lastAt":"2009-03-22T22:38:34Z","messageCount":21,"participants":["Chris Johnsen","Johannes Schindelin","Junio C Hamano","Jeff King","Brandon Casey","Tomas Carnecky","Mike Ralphson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"107280","messageId":"1236418251-16947-1-git-send-email-chris_johnsen@pobox.com","threadId":"18186","inReplyTo":null,"subject":"[PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Chris Johnsen","fromEmail":"chris_johnsen@pobox.com","sentAt":"2009-03-07T09:30:51Z","receivedAt":"2009-03-07T09:30:51Z","isPatch":true,"sender":{"key":"chris_johnsen@pobox.com","avatar":"https://avatars.githubusercontent.com/u/107071?v=4"},"body":"When a cherry-pick of an empty commit is done, release the lock\nheld on the index.\n\nThe fix is the same as was applied to similar code in 4271666046.\n\nSigned-off-by: Chris Johnsen \n---\n\nRELEVANT HISTORY\n\nThe previous code was introduced in 6eb1b43793.\n\nIt seems to have been copied from builtin-merge.c, where it was\nintroduced in 18668f5319 (6eb1's parent).\n\nThe code introduced in 1866 was fixed with the addition of\nrollback_lock_file (the same fix as applied here) in 4271666046.\n\nCONTEXT\n\nI ran into the code path that this patch modifies while using\n'git rebase -i' on a branch that had empty commits. When rebase\ntried to cherry-pick an empty commit, the cherry-pick returned an\nerror and failed to unlocking the index. While this patch does\nnot allow 'git rebase -i' to \"correctly\" process empty commits,\nit does prevent 'git cherry-pick' from exiting without unlocking\nthe index.\n\n\tT=\"$(mktemp -d -t empty-cherry)\"\n\tcd \"$T\"\n\tgit init\n\techo a > a\n\tgit add a\n\tgit commit -m 'add a'\n\tgit checkout -b empty\n\tgit commit --allow-empty -m 'empty'\n\tgit checkout master\n\tgit cherry-pick empty\n\n> Finished one cherry-pick.\n> fatal: Unable to create '.git/index.lock': File exists.\n> \n> If no other git process is currently running, this probably means a\n> git process crashed in this repository earlier. Make sure no other git\n> process is running and remove the file manually to continue.\n\nI was originally appending empty commits with descriptive\nmessages to mark \"interesting\" points in a stream of otherwise\nautomatic commits (made by an Xcode build script). My idea was to\nuse these commits as markers that would flow with the rest of the\ncommits through a subsequent series of cleanups done with 'git\nrebase -i'. After encountering this problem, however, I moved\naway from using empty commits (I have since been using 'git\ncommit --amend' to rewrite the last (generic, automatic) commit\nmessage), but the bug left me wondering about the the status of\nemtpy commits.\n\nUNEVEN TREATMENT OF EMPTY CHANGES\n\nIt seems that empty commits suffer uneven treatment under various\npatch-transport/history-rewriting mechanisms. They seem to be\nhandled okay in the most of the core (fetch, push, bundle all\nseem to preserve empty commits, though I have not done rigorous\ntesting). But there are various problems in other areas:\n'format-patch', 'send-email', 'apply', 'am', 'rebase' (automatic,\nnon-fast-forward; and --interactive).\n\nDISCUSSION\n\n36863af16e (git-commit --allow-empty) says \"This is primarily for\nuse by foreign scm interface scripts.\". Is this the only case\nwhere empty commits _should_ be used? Or is the uneven treatment\njust a matter nobody having an itch to use empty commits in\nsituations where the above commands would interact with them?\n\nI played with having builtin-revert.c always use 'git commit\n--allow-empty ...', but I was not confident that such behavior\nwould match \"official policy for empty commits\" (if such policy\neven exists). Should empty commits be preserved (by default) once\nthey are in the commit stream? Should commands that do \"patch\nmanipulation\" only preserve empty commits if they are explicitly\ninstructed to do so (with their own --allow-emtpy option)? Should\nsomething completely different happen?\n\nI realize that rebasing and cherry-picking need to have special\nconsideration for \"effectively empty\" patches (those that\nintroduce changes already present; usually because one side\nalready picked some changes from the other), but maybe \"actually\nempty, yet informational\" commits also deserve some\nconsideration.\n\nIf such \"actually empty, yet informational\" commits (no content\nchanges, but a useful commit messages) are a valid use of empty\ncommits, then maybe \"actually empty\" commits in general deserve\ntreatment equal to that of normal commits.\n\n---\n builtin-revert.c | 1 +\n t/t3505-cherry-pick-empty.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 0 deletions(-)\n create mode 100755 t/t3505-cherry-pick-empty.sh\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex d210150..3f2614e 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -376,6 +376,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t (write_cache(index_fd, active_cache, active_nr) ||\n \t commit_locked_index(&index_lock)))\n \t\tdie(\"%s: Unable to write new index file\", me);\n+\trollback_lock_file(&index_lock);\n \n \tif (!clean) {\n \t\tadd_to_msg(\"\\nConflicts:\\n\\n\");\ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nnew file mode 100755\nindex 0000000..9aaeabd\n--- /dev/null\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -0,0 +1,33 @@\n+#!/bin/sh\n+\n+test_description='test cherry-picking an empty commit'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\techo first > file1 &&\n+\tgit add file1 &&\n+\ttest_tick &&\n+\tgit commit -m \"first\" &&\n+\n+\tgit checkout -b empty-branch &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"empty\"\n+\n+'\n+\n+test_expect_code 1 'cherry-pick an empty commit' '\n+\n+\tgit checkout master &&\n+\tgit cherry-pick empty-branch\n+\n+'\n+\n+test_expect_success 'index lockfile was removed' '\n+\n+\ttest ! -f .git/index.lock\n+\n+'\n+\n+test_done\n-- \n1.6.2\n"},{"id":"107286","messageId":"alpine.DEB.1.00.0903071212350.10279@pacific.mpi-cbg.de","threadId":"18186","inReplyTo":"1236418251-16947-1-git-send-email-chris_johnsen@pobox.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-07T11:15:58Z","receivedAt":"2009-03-07T11:15:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 7 Mar 2009, Chris Johnsen wrote:\n\n> When a cherry-pick of an empty commit is done, release the lock\n> held on the index.\n> \n> The fix is the same as was applied to similar code in 4271666046.\n> \n> Signed-off-by: Chris Johnsen \n> ---\n> [...]\n\nThanks for the detailed explanation, and the patch!\n\nI wonder, though, if the real root of the problem is that there is \ncopied code. IOW I think it would be better to introduce a global \nfunction that writes the index to a tree. A quick \"git grep \ncommit_locked_index\" reveals quite a few code sites...\n\nThanks,\nDscho\n"},{"id":"107355","messageId":"EEE56CB5-BBC7-45F4-801B-062349E07730@pobox.com","threadId":"18186","inReplyTo":"alpine.DEB.1.00.0903071212350.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Chris Johnsen","fromEmail":"chris_johnsen@pobox.com","sentAt":"2009-03-07T22:57:40Z","receivedAt":"2009-03-07T22:57:40Z","isPatch":true,"sender":{"key":"chris_johnsen@pobox.com","avatar":"https://avatars.githubusercontent.com/u/107071?v=4"},"body":"On 2009 Mar 7, Johannes Schindelin wrote:\n> I wonder, though, if the real root of the problem is that there is\n> copied code.\n\nAgreed.\n\n> IOW I think it would be better to introduce a global\n> function that writes the index to a tree.\n\nI am not quite sure I follow your meaning here.\n\nWe have write_cache_as_tree in cache-tree.c. Is something like that \nwhat you had in mind for \"write the index to a tree\"?\n\nCould you elaborate on how an \"index to tree\" function could be \napplied to the problem of the inconsistent lock releasing around \ncommit_locked_index calls? Sorry, for my lack of a clue, I am fairly \nnew to the code base. Or are you seeing a different code duplication \nproblem here?\n\nThe general form of the code around calls to commit_locked_index \nseems to be of the this form:\n\n\tif (some_condition) /* not all call sites use this */\n\t\tif (write_cache(...) || commit_locked_index(...))\n\t\t\tdie(...); /* or return error(...) */\n\trollback_lock_file(...); /* sometimes missing or distant */\n\nIn most cases some_condition is active_cache_changed, but not always \n(builtin-apply.c, rerere.c).\n\nThe problem with cherry-picking empty commits was that \nactive_cache_changed (used as some_condition in the general pattern \nshown above) would be false, so the write and commit was skipped, but \nthere was also never any rollback. Later, when cherry-pick exec-ed \ncommit, the lock file still existed and commit dies.\n\nTo make sure a commit or a rollback always happens at every call \nsite, each one would have to unconditionally call some global \nfunction (write_and_commit_or_rollback_locked_index?, ick) that \nconditionally did the write and commit, but unconditionally did the \nrollback (basically a no-op if the commit went OK).\n\n> A quick \"git grep commit_locked_index\" reveals quite a few code \n> sites...\n\nIndeed, would such a cleanup be worth the churn? I do not have any \nmodifications for which this cleanup could be considered preparatory.\n\nThank you for your help!\n\n-- \nChris\n"},{"id":"107365","messageId":"alpine.DEB.1.00.0903080218070.10279@pacific.mpi-cbg.de","threadId":"18186","inReplyTo":"EEE56CB5-BBC7-45F4-801B-062349E07730@pobox.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-08T01:19:29Z","receivedAt":"2009-03-08T01:19:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 7 Mar 2009, Chris Johnsen wrote:\n\n> On 2009 Mar 7, Johannes Schindelin wrote:\n> >I wonder, though, if the real root of the problem is that there is\n> >copied code.\n> \n> Agreed.\n> \n> >IOW I think it would be better to introduce a global\n> >function that writes the index to a tree.\n> \n> I am not quite sure I follow your meaning here.\n\nWell, my thinking was that instead of imitating what merge-recursive does, \nyou could refactor that very code into a function that gets called from \nboth merge-recursive and revert.\n\nCiao,\nDscho\n"},{"id":"107372","messageId":"7v63ikmz11.fsf@gitster.siamese.dyndns.org","threadId":"18186","inReplyTo":"1236418251-16947-1-git-send-email-chris_johnsen@pobox.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-08T04:14:34Z","receivedAt":"2009-03-08T04:14:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Johnsen writes:\n\n> When a cherry-pick of an empty commit is done, release the lock\n> held on the index.\n>\n> The fix is the same as was applied to similar code in 4271666046.\n>\n> Signed-off-by: Chris Johnsen \n\nThanks. Will apply. We should handle possible refactoring as a separate\ntopic.\n\n> UNEVEN TREATMENT OF EMPTY CHANGES\n>\n> It seems that empty commits suffer uneven treatment under various\n> patch-transport/history-rewriting mechanisms. They seem to be\n> handled okay in the most of the core (fetch, push, bundle all\n> seem to preserve empty commits, though I have not done rigorous\n> testing).\n\nThey just transfer an existing history from one place to another without\nmodifying, so it is unfortunately true that they preserve such a broken\nhistory with empty commits.\n\n> 'format-patch', 'send-email', 'apply', 'am', 'rebase' (automatic,\n> non-fast-forward; and --interactive).\n\nThese are all about creating history afresh, and they actively discourage\nempty commits to be (re)created.\n\nThere is no \"uneven treatment\".\n\n> 36863af16e (git-commit --allow-empty) says \"This is primarily for\n> use by foreign scm interface scripts.\". Is this the only case\n> where empty commits _should_ be used?\n\nIf foreign scm recorded an empty commit, it would be nice to be able to\nrecreate such a broken history _if the user wanted to_, and that is where\nthe --allow-empty option can be used.\n"},{"id":"107382","messageId":"20090308144240.GA30794@coredump.intra.peff.net","threadId":"18186","inReplyTo":"1236418251-16947-1-git-send-email-chris_johnsen@pobox.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-08T14:42:40Z","receivedAt":"2009-03-08T14:42:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 07, 2009 at 03:30:51AM -0600, Chris Johnsen wrote:\n\n> +test_expect_code 1 'cherry-pick an empty commit' '\n> +\n> +\tgit checkout master &&\n> +\tgit cherry-pick empty-branch\n> +\n> +'\n\nHmm. This test fails for me on FreeBSD. However, it seems to be related\nto a shell portability issue with the test suite. The extra newline\ninside the shell snippet seems to lose the last status. The following\nworks fine for me with bash or dash:\n\n-- >8 --\n#!/bin/sh\n\ntest_description='test that shell handles whitespace in eval'\n. ./test-lib.sh\n\ntest_expect_code 1 'no newline' 'false'\ntest_expect_code 1 'one newline' 'false\n'\ntest_expect_code 1 'extra newline' 'false\n\n'\n\ntest_done\n-- 8< --\n\nbut on a FreeBSD 6.1 box generates:\n\n * ok 1: no newline\n * ok 2: one newline\n * FAIL 3: 1\n extra newline false\n\nWith this minimal example, you can see that the problem is not some\nsubtle bug in the test suite:\n\n-- >8 --\n#!/bin/sh\n\neval 'false'\necho status is $?\neval 'false\n'\necho status is $?\neval 'false\n\n'\necho status is $?\n-- 8< --\n\ngenerates:\n\n status is 1\n status is 1\n status is 0\n\nwhich means that any tests of the form\n\n test_expect_success description '\n\n contents\n\n '\n\nare not actually having their exit code checked properly, and are just\nclaiming success regardless of what happened. So this definitely needs\nto be addressed, I think. I'm not sure of the best course of action,\nthough. Our options include:\n\n 1. Declare appended newline a forbidden style, fix all existing cases\n in the test suite, and be on the lookout for new ones.\n\n The biggest problem with this option is that we have no automated\n way of policing. Such tests will just silently pass on the broken\n platform.\n\n 2. Have test_run_ canonicalize the snippet by removing trailing\n newlines.\n\n 3. Declare FreeBSD's /bin/sh unfit for git consumption, and require\n bash for the test suite.\n\nI think (2) is the most reasonable option of those choices.\n\nWe could also try to convince FreeBSD that it's a bug, but that doesn't\nchange the fact that the tests are broken on every existing version.\n\n-Peff\n"},{"id":"107383","messageId":"20090308150944.GA32294@coredump.intra.peff.net","threadId":"18186","inReplyTo":"20090308144240.GA30794@coredump.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-08T15:09:44Z","receivedAt":"2009-03-08T15:09:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 08, 2009 at 10:42:40AM -0400, Jeff King wrote:\n\n> 2. Have test_run_ canonicalize the snippet by removing trailing\n> newlines.\n> [...]\n> I think (2) is the most reasonable option of those choices.\n\nAnd here's what that would look like:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7a847ec..276a14d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -273,7 +273,7 @@ test_debug () {\n }\n \n test_run_ () {\n-\teval >&3 2>&4 \"$1\"\n+\teval >&3 2>&4 \"`echo \"$1\" | sed -e :a -e '/^ *\\n*$/{$d;N;ba' -e '}'`\"\n \teval_ret=\"$?\"\n \treturn 0\n }\n\nThat is a truly hideous sed expression, and I would be happy to hear\nmore readable suggestions (it is based on one from the \"sed one-liners\"\ncompilation).\n\nWith this applied, t3505 passes for me. However, some other random tests\nare broken as a result. It looks like it might be related to an extra\nround of '\\' expansion.\n\n-Peff\n"},{"id":"107395","messageId":"7v8wnflrws.fsf@gitster.siamese.dyndns.org","threadId":"18186","inReplyTo":"20090308144240.GA30794@coredump.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-08T19:45:55Z","receivedAt":"2009-03-08T19:45:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 1. Declare appended newline a forbidden style, fix all existing cases\n> in the test suite, and be on the lookout for new ones.\n>\n> The biggest problem with this option is that we have no automated\n> way of policing. Such tests will just silently pass on the broken\n> platform.\n>\n> 2. Have test_run_ canonicalize the snippet by removing trailing\n> newlines.\n>\n> 3. Declare FreeBSD's /bin/sh unfit for git consumption, and require\n> bash for the test suite.\n>\n> I think (2) is the most reasonable option of those choices.\n>\n> We could also try to convince FreeBSD that it's a bug, but that doesn't\n> change the fact that the tests are broken on every existing version.\n\nIf this part from your analysis is true for a shell:\n\n> eval 'false\n>\n> '\n> echo status is $?\n>\n> generates:\n> ...\n> status is 0\n\nI would be very tempted to declare that shell is unfit for any serious\nuse, not just for test suite. Removing the empty line at the end of a\nscriptlet that such a broken shell misinterprets as an empty command\nthat is equivalent to \":\" (or \"true\") might hide breakages in the test\nsuite, but\n\n (1) eval \"$string\" is used outside of test suite, most notably \"am\" and\n \"bisect\". I think \"am\"'s use is safe, but I wouldn't be surprised if\n the scriptlet \"bisect\" internally creates has empty lines if only for\n debuggability; and more importantly\n\n (2) who knows what _other_ things may be broken in such a shell?\n"},{"id":"107404","messageId":"B0CBEE84-0F46-4AF2-86B1-C80BADAEF4E5@pobox.com","threadId":"18186","inReplyTo":"7v63ikmz11.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Chris Johnsen","fromEmail":"chris_johnsen@pobox.com","sentAt":"2009-03-08T21:09:50Z","receivedAt":"2009-03-08T21:09:50Z","isPatch":true,"sender":{"key":"chris_johnsen@pobox.com","avatar":"https://avatars.githubusercontent.com/u/107071?v=4"},"body":"On 2009 Mar 7, at 22:14, Junio C Hamano wrote:\n>> UNEVEN TREATMENT OF EMPTY CHANGES\n>>\n>> fetch, push, bundle\n> They just transfer an existing history from one place to another \n> without\n> modifying, so it is unfortunately true that they preserve such a \n> broken\n> history with empty commits.\n\n>> 'format-patch', 'send-email', 'apply', 'am', 'rebase' (automatic,\n>> non-fast-forward; and --interactive).\n>\n> These are all about creating history afresh, and they actively \n> discourage\n> empty commits to be (re)created.\n>\n> There is no \"uneven treatment\".\n\n>> 36863af16e (git-commit --allow-empty) says \"This is primarily for\n>> use by foreign scm interface scripts.\". Is this the only case\n>> where empty commits _should_ be used?\n>\n> If foreign scm recorded an empty commit, it would be nice to be \n> able to\n> recreate such a broken history _if the user wanted to_, and that is \n> where\n> the --allow-empty option can be used.\n\nThank you for the clarification. I will explain the source of my \nconfusion.\n\nThe current documentation \"Usually recording [an empty commit] is a \nmistake... This option ... is primarily for use by foreign scm \ninterface scripts.\" implied to me that there was room outside foreign \nscm interface for \"normal\" use of empty commits.\n\nMy confusion was that I took \"usually a mistake\" to refer to the case \nwhere the user meant to commit content changes but forgot to first \nstage any changed content. But your clarification shows that \"usually \na mistake\" really means that making any empty commit, intentional or \nnot, is (considered to be) a fundamental misuse of SCM machinery.\n\nWould it be acceptable to strengthen the language in the \ndocumentation for --allow-empty? Possibly something like the \nfollowing paragraph:\n\nEmpty commits are a broken concept and should never be made during\nnormal usage. By default, the command prevents you\nfrom making such a commit. This option bypasses the safety, and is\nprimarily for use by foreign scm interface scripts.\n\nIf such a change is acceptable, I will send a patch.\n\n-- \nChris\n"},{"id":"107411","messageId":"7v7i2zk7fn.fsf@gitster.siamese.dyndns.org","threadId":"18186","inReplyTo":"B0CBEE84-0F46-4AF2-86B1-C80BADAEF4E5@pobox.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-08T21:53:32Z","receivedAt":"2009-03-08T21:53:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Johnsen writes:\n\n> My confusion was that I took \"usually a mistake\" to refer to the case\n> where the user meant to commit content changes but forgot to first\n> stage any changed content. But your clarification shows that \"usually\n> a mistake\" really means that making any empty commit, intentional or\n> not, is (considered to be) a fundamental misuse of SCM machinery.\n\nThe empty commits in your example a few messages ago are used as \"piss in\nthe snow\" marking. If you did not have tags (and commit notes), it may be\nthe only workaround to say \"here is an interesting point\", but even then\nsuch a workaround can only be made while the commit is at the tip, and be\nmade useful only by forcing all the other commits on the branch be on top\nof that \"piss in the snow\" commit, so it is a flawed workaround.\n\nSuppose you have this history.\n\n ---A---B---C\n\nYou found that the point C is interesting in some way, so you mark it:\n\n ---A---B---C---P\n\nBut somebody else may have developed on top of C bypassing P\n\n D---E---F\n /\n ---A---B---C---P\n\nWhat would you do in such a case? You cannot leave P dangling, as that\nwould mean P will not participate in future rebases (and you do not want\nto rebase P on top of F because C is the point that is interesting to you,\nnot F). Do you merge F and P only to make P not dangling? What does such\na merge mean?\n\nWorse yet, if you stared from the original history with three commits, how\nwould you mark that B is interesting?\n\n P D---E---F\n / /\n ---A---B---C\n\n\nThe facility git and other SCM offer you to leave such mark (possibly\nafter the fact) is to use tags.\n\nSo in your particular \"piss in the snow\" usage, I would agree that such an\nempty commit is a misuse.\n\nI am not however claiming that all uses of an empty commit are fundamental\nmisuses here, though. Somebody else may have other valid uses.\n"},{"id":"107488","messageId":"wesyHaGNnlB5oyurcOVfszxul5Cz5GxJhdT6fDpND5VxH9x2-iHvHg@cipher.nrlssc.navy.mil","threadId":"18186","inReplyTo":"20090308144240.GA30794@coredump.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-03-09T17:36:55Z","receivedAt":"2009-03-09T17:36:55Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Jeff King wrote:\n> On Sat, Mar 07, 2009 at 03:30:51AM -0600, Chris Johnsen wrote:\n> \n>> +test_expect_code 1 'cherry-pick an empty commit' '\n>> +\n>> +\tgit checkout master &&\n>> +\tgit cherry-pick empty-branch\n>> +\n>> +'\n> \n> Hmm. This test fails for me on FreeBSD. However, it seems to be related\n> to a shell portability issue with the test suite. The extra newline\n> inside the shell snippet seems to lose the last status. The following\n> works fine for me with bash or dash:\n\n> With this minimal example, you can see that the problem is not some\n> subtle bug in the test suite:\n> \n> -- >8 --\n> #!/bin/sh\n> \n> eval 'false'\n> echo status is $?\n> eval 'false\n> '\n> echo status is $?\n> eval 'false\n> \n> '\n> echo status is $?\n> -- 8< --\n> \n> generates:\n> \n> status is 1\n> status is 1\n> status is 0\n\nEven /bin/sh (which is unfit for git consumption) on Solaris 7 produces\nnon-zero for all three tests:\n\n status is 255\n status is 255\n status is 255\n\nI set SHELL_PATH=/usr/xpg4/bin/sh aka ksh when compiling git which also\nhandles your test correctly:\n\n status is 1\n status is 1\n status is 1\n\nIRIX6.5 /bin/sh and /bin/ksh produce the correct results also.\n\n-brandon\n"},{"id":"107600","messageId":"20090310181730.GD26351@sigill.intra.peff.net","threadId":"18186","inReplyTo":"7v8wnflrws.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-10T18:17:30Z","receivedAt":"2009-03-10T18:17:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 08, 2009 at 12:45:55PM -0700, Junio C Hamano wrote:\n\n> If this part from your analysis is true for a shell:\n> \n> > eval 'false\n> >\n> > '\n> > echo status is $?\n> >\n> > generates:\n> > ...\n> > status is 0\n> \n> I would be very tempted to declare that shell is unfit for any serious\n> use, not just for test suite. Removing the empty line at the end of a\n> scriptlet that such a broken shell misinterprets as an empty command\n> that is equivalent to \":\" (or \"true\") might hide breakages in the test\n> suite, but\n> \n> (1) eval \"$string\" is used outside of test suite, most notably \"am\" and\n> \"bisect\". I think \"am\"'s use is safe, but I wouldn't be surprised if\n> the scriptlet \"bisect\" internally creates has empty lines if only for\n> debuggability; and more importantly\n> \n> (2) who knows what _other_ things may be broken in such a shell?\n\nOK, good points. I was just hoping not to cause people on FreeBSD undue\npain. What is the best way to make such a declaration? I can think of:\n\n 1. A mention in the release notes.\n\n 2. A test in the Makefile similar to the $(:) test.\n\n 3. Getting in touch with the freebsd ports maintainer for git and\n suggesting a dependency on bash (and/or seeing if he wants to push\n through a fix for /bin/sh).\n\n I don't know if the same problem exists on other BSD-influenced systems,\n or how closely they share the ports collection (it's been quite a\n while since I've really admin'd a freebsd box). For that matter, I\n wonder if this is also a problem on OS X. Can somebody with an OS X\n box try:\n\n $ /bin/sh\n $ eval 'false\n\n '\n $ echo $?\n\n It should print '1'; if it prints '0', the shell is broken.\n\n-Peff\n"},{"id":"107606","messageId":"B9753A8E-43EB-4FA7-B573-14AEE78DD5BD@dbservice.com","threadId":"18186","inReplyTo":"20090310181730.GD26351@sigill.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Tomas Carnecky","fromEmail":"tom@dbservice.com","sentAt":"2009-03-10T18:25:19Z","receivedAt":"2009-03-10T18:25:19Z","isPatch":true,"sender":{"key":"tom@dbservice.com","avatar":"https://gravatar.com/avatar/900a300bdd1a8bbe086008ad78210bbee2ad2803b7d50a5cba04c1e9404bd6d2?d=mp&s=160"},"body":"\nOn Mar 10, 2009, at 7:17 PM, Jeff King wrote:\n\n> On Sun, Mar 08, 2009 at 12:45:55PM -0700, Junio C Hamano wrote:\n>\n>> If this part from your analysis is true for a shell:\n>>\n>>> eval 'false\n>>>\n>>> '\n>>> echo status is $?\n>>>\n>>> generates:\n>>> ...\n>>> status is 0\n>>\n>> I would be very tempted to declare that shell is unfit for any \n>> serious\n>> use, not just for test suite. Removing the empty line at the end \n>> of a\n>> scriptlet that such a broken shell misinterprets as an empty command\n>> that is equivalent to \":\" (or \"true\") might hide breakages in the \n>> test\n>> suite, but\n>>\n>> (1) eval \"$string\" is used outside of test suite, most notably \"am\" \n>> and\n>> \"bisect\". I think \"am\"'s use is safe, but I wouldn't be \n>> surprised if\n>> the scriptlet \"bisect\" internally creates has empty lines if \n>> only for\n>> debuggability; and more importantly\n>>\n>> (2) who knows what _other_ things may be broken in such a shell?\n>\n> OK, good points. I was just hoping not to cause people on FreeBSD \n> undue\n> pain. What is the best way to make such a declaration? I can think of:\n>\n> 1. A mention in the release notes.\n>\n> 2. A test in the Makefile similar to the $(:) test.\n>\n> 3. Getting in touch with the freebsd ports maintainer for git and\n> suggesting a dependency on bash (and/or seeing if he wants to push\n> through a fix for /bin/sh).\n>\n> I don't know if the same problem exists on other BSD-influenced \n> systems,\n> or how closely they share the ports collection (it's been quite a\n> while since I've really admin'd a freebsd box). For that matter, I\n> wonder if this is also a problem on OS X. Can somebody with an \n> OS X\n> box try:\n>\n> $ /bin/sh\n> $ eval 'false\n>\n> '\n> $ echo $?\n>\n> It should print '1'; if it prints '0', the shell is broken.\n\nprints '1' here (10.5.6)\n\ntom\n"},{"id":"107605","messageId":"B3DB8CA0-F6B2-4E68-B8E1-1B59731E4ED5@dbservice.com","threadId":"18186","inReplyTo":"20090310181730.GD26351@sigill.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Tomas Carnecky","fromEmail":"tom@dbservice.com","sentAt":"2009-03-10T19:33:12Z","receivedAt":"2009-03-10T19:33:12Z","isPatch":true,"sender":{"key":"tom@dbservice.com","avatar":"https://gravatar.com/avatar/900a300bdd1a8bbe086008ad78210bbee2ad2803b7d50a5cba04c1e9404bd6d2?d=mp&s=160"},"body":"\nOn Mar 10, 2009, at 7:17 PM, Jeff King wrote:\n\n> On Sun, Mar 08, 2009 at 12:45:55PM -0700, Junio C Hamano wrote:\n>\n>> If this part from your analysis is true for a shell:\n>>\n>>> eval 'false\n>>>\n>>> '\n>>> echo status is $?\n>>>\n>>> generates:\n>>> ...\n>>> status is 0\n>>\n>> I would be very tempted to declare that shell is unfit for any \n>> serious\n>> use, not just for test suite. Removing the empty line at the end \n>> of a\n>> scriptlet that such a broken shell misinterprets as an empty command\n>> that is equivalent to \":\" (or \"true\") might hide breakages in the \n>> test\n>> suite, but\n>>\n>> (1) eval \"$string\" is used outside of test suite, most notably \"am\" \n>> and\n>> \"bisect\". I think \"am\"'s use is safe, but I wouldn't be \n>> surprised if\n>> the scriptlet \"bisect\" internally creates has empty lines if \n>> only for\n>> debuggability; and more importantly\n>>\n>> (2) who knows what _other_ things may be broken in such a shell?\n>\n> OK, good points. I was just hoping not to cause people on FreeBSD \n> undue\n> pain. What is the best way to make such a declaration? I can think of:\n>\n> 1. A mention in the release notes.\n>\n> 2. A test in the Makefile similar to the $(:) test.\n>\n> 3. Getting in touch with the freebsd ports maintainer for git and\n> suggesting a dependency on bash (and/or seeing if he wants to push\n> through a fix for /bin/sh).\n>\n> I don't know if the same problem exists on other BSD-influenced \n> systems,\n> or how closely they share the ports collection (it's been quite a\n> while since I've really admin'd a freebsd box). For that matter, I\n> wonder if this is also a problem on OS X. Can somebody with an OS X\n> box try:\n>\n> $ /bin/sh\n> $ eval 'false\n>\n> '\n> $ echo $?\n>\n> It should print '1'; if it prints '0', the shell is broken.\n\nprints '1' here (10.5.6)\n\ntom\n"},{"id":"107619","messageId":"AA6A171F-70CE-4CB3-9AE1-27CD69C3202C@pobox.com","threadId":"18186","inReplyTo":"20090310181730.GD26351@sigill.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Chris Johnsen","fromEmail":"chris_johnsen@pobox.com","sentAt":"2009-03-10T23:57:55Z","receivedAt":"2009-03-10T23:57:55Z","isPatch":true,"sender":{"key":"chris_johnsen@pobox.com","avatar":"https://avatars.githubusercontent.com/u/107071?v=4"},"body":"On 2009 Mar 10, at 13:17, Jeff King wrote:\n> Can somebody with an OS X box try:\n>\n> $ /bin/sh\n> $ eval 'false\n>\n> '\n> $ echo $?\n>\n> It should print '1'; if it prints '0', the shell is broken.\n\nI wrote t3505 on a Mac OS X 10.4.11 system. On that system, /bin/sh \nis a copy of bash v2.05b. Your test code prints 1 here.\n\n-- \nChris\n"},{"id":"107622","messageId":"20090311003022.GA22273@coredump.intra.peff.net","threadId":"18186","inReplyTo":"AA6A171F-70CE-4CB3-9AE1-27CD69C3202C@pobox.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-11T00:30:22Z","receivedAt":"2009-03-11T00:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 10, 2009 at 06:57:55PM -0500, Chris Johnsen wrote:\n\n> On 2009 Mar 10, at 13:17, Jeff King wrote:\n>> Can somebody with an OS X box try:\n>>\n>> $ /bin/sh\n>> $ eval 'false\n>>\n>> '\n>> $ echo $?\n>>\n>> It should print '1'; if it prints '0', the shell is broken.\n>\n> I wrote t3505 on a Mac OS X 10.4.11 system. On that system, /bin/sh is a \n> copy of bash v2.05b. Your test code prints 1 here.\n\nOK, then nothing to worry about there. I have no idea which shell\nOpenBSD and NetBSD use these days, and I don't have access to a box.\nAnybody?\n\n-Peff\n"},{"id":"107669","messageId":"e2b179460903110408i4ab3c9cg3c863b89a2f57cba@mail.gmail.com","threadId":"18186","inReplyTo":"20090311003022.GA22273@coredump.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2009-03-11T11:08:06Z","receivedAt":"2009-03-11T11:08:06Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2009/3/11 Jeff King :\n> OK, then nothing to worry about there. I have no idea which shell\n> OpenBSD and NetBSD use these days, and I don't have access to a box.\n> Anybody?\n\nOpenBSD uses pdksh in Bourne shell mode for non-root shells (ksh mode\nfor root) [1].\n\nNetBSD >=4 uses a Bourne shell but I don't know the exact provenance.\n[2] \"A sh command appeared in Version 1 AT&T UNIX. It was, however,\nunmaintainable so we wrote this one.\"\n\n[1] http://www.openbsd.org/faq/faq10.html#ksh\n[2] http://www.netbsd.org/docs/misc/index.html#shells\n"},{"id":"107718","messageId":"e2b179460903111002p195628ddka23529d898a2ec1f@mail.gmail.com","threadId":"18186","inReplyTo":"e2b179460903110408i4ab3c9cg3c863b89a2f57cba@mail.gmail.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2009-03-11T17:02:04Z","receivedAt":"2009-03-11T17:02:04Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2009/3/11 Mike Ralphson \n>\n> 2009/3/11 Jeff King :\n> > OK, then nothing to worry about there. I have no idea which shell\n> > OpenBSD and NetBSD use these days, and I don't have access to a box.\n> > Anybody?\n>\n> OpenBSD uses pdksh in Bourne shell mode for non-root shells (ksh mode\n> for root)\n\n... and isn't broken in this instance (OpenBSD v4.1)\n\nWeird test failures though, so now I'm looking at that 8-)\n"},{"id":"108889","messageId":"20090322094139.GA10599@coredump.intra.peff.net","threadId":"18186","inReplyTo":"e2b179460903110408i4ab3c9cg3c863b89a2f57cba@mail.gmail.com","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-22T09:41:39Z","receivedAt":"2009-03-22T09:41:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[this is a follow-up on the \"eval 'false\\n\\n'\" returns 0 issue on\nFreeBSD]\n\nOn Wed, Mar 11, 2009 at 11:08:06AM +0000, Mike Ralphson wrote:\n\n> 2009/3/11 Jeff King :\n> > OK, then nothing to worry about there. I have no idea which shell\n> > OpenBSD and NetBSD use these days, and I don't have access to a box.\n> > Anybody?\n> \n> OpenBSD uses pdksh in Bourne shell mode for non-root shells (ksh mode\n> for root) [1].\n> \n> NetBSD >=4 uses a Bourne shell but I don't know the exact provenance.\n> [2] \"A sh command appeared in Version 1 AT&T UNIX. It was, however,\n> unmaintainable so we wrote this one.\"\n> \n> [1] http://www.openbsd.org/faq/faq10.html#ksh\n> [2] http://www.netbsd.org/docs/misc/index.html#shells\n\nThanks for looking this up, Mike. It sounds like FreeBSD is probably the\nonly problematic one. I confirmed that the problem still exists in\nFreeBSD 7.1, and I've mailed the git ports maintainer off-list to\nmake him aware of the issue. So we'll see what happens.\n\nJunio, do you want to put anything in the release notes warning people\nwho build from source that this is a potential issue? Do you want\nsomething in the Makefile detecting that the shell is broken?\n\n-Peff\n"},{"id":"108938","messageId":"7veiwpdxtg.fsf@gitster.siamese.dyndns.org","threadId":"18186","inReplyTo":"20090322094139.GA10599@coredump.intra.peff.net","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T21:58:35Z","receivedAt":"2009-03-22T21:58:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [this is a follow-up on the \"eval 'false\\n\\n'\" returns 0 issue on\n> FreeBSD]\n\nThanks for keeping track of this one.\n\n> Thanks for looking this up, Mike. It sounds like FreeBSD is probably the\n> only problematic one. I confirmed that the problem still exists in\n> FreeBSD 7.1, and I've mailed the git ports maintainer off-list to\n> make him aware of the issue. So we'll see what happens.\n>\n> Junio, do you want to put anything in the release notes warning people\n> who build from source that this is a potential issue? Do you want\n> something in the Makefile detecting that the shell is broken?\n\nA sentence or two in INSTALL will not hurt.\n\nI would not worry too much about the test scripts, but I would worry more\nabout getting phantom bug reports for our shell script Porcelains that get\nhit by this. Earlier I mentioned bisect is the only heavy user, but the\nissue is more severe with filter-branch that is designed to eval end user\nscripts (calls to 'eval \"$filter_frotz\"' check the exit status and die on\nfailure---with trailing blank lines the failure the filter reports will\nnot get caught).\n"},{"id":"108946","messageId":"20090322223834.GB22428@sigill.intra.peff.net","threadId":"18186","inReplyTo":"7veiwpdxtg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-22T22:38:34Z","receivedAt":"2009-03-22T22:38:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2009 at 02:58:35PM -0700, Junio C Hamano wrote:\n\n> > Junio, do you want to put anything in the release notes warning people\n> > who build from source that this is a potential issue? Do you want\n> > something in the Makefile detecting that the shell is broken?\n> \n> A sentence or two in INSTALL will not hurt.\n> \n> I would not worry too much about the test scripts, but I would worry more\n> about getting phantom bug reports for our shell script Porcelains that get\n> hit by this. Earlier I mentioned bisect is the only heavy user, but the\n> issue is more severe with filter-branch that is designed to eval end user\n> scripts (calls to 'eval \"$filter_frotz\"' check the exit status and die on\n> failure---with trailing blank lines the failure the filter reports will\n> not get caught).\n\nAgreed. The good news is that the /bin/sh people are treating it like a\nbug:\n\n http://lists.freebsd.org/pipermail/freebsd-standards/2009-March/001721.html\n\nso it will hopefully be fixed soon. It might still be worth warning\nusers of older releases in INSTALL, though.\n\n-Peff\n"}]}