{"thread":{"id":"40868","subject":"[PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()","startedAt":"2015-11-24T14:45:44Z","lastAt":"2016-01-21T04:52:48Z","messageCount":11,"participants":["SZEDER Gábor","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"273639","messageId":"1448376345-27339-1-git-send-email-szeder@ira.uka.de","threadId":"40868","inReplyTo":null,"subject":"[PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-11-24T14:45:44Z","receivedAt":"2015-11-24T14:45:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Add tests that check require_clean_work_tree() in the common cases,\ni.e. on a branch with all combinations of clean and dirty index and\nworktree, and also add tests that exercise it on an orphan branch.\n\nrequire_clean_work_tree()'s behavior in the orphan branch cases is\nquestionable, as it exits with error on an orphan branch independently\nfrom whether the index and worktree are clean or dirty (and it does so\nbecause of the invalid HEAD, meaning that even when it should exit\nwith error, it does so for the wrong reason). As scripts might rely\non the current behavior, we err on the side of compatibility and\nsafety, and expect the current behavior as success.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t2301-require-clean-work-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 63 insertions(+)\n create mode 100755 t/t2301-require-clean-work-tree.sh\n\ndiff --git a/t/t2301-require-clean-work-tree.sh b/t/t2301-require-clean-work-tree.sh\nnew file mode 100755\nindex 0000000000..1bb41b59a5\n--- /dev/null\n+++ b/t/t2301-require-clean-work-tree.sh\n@@ -0,0 +1,63 @@\n+#!/bin/sh\n+\n+test_description='require_clean_work_tree'\n+\n+. ./test-lib.sh\n+\n+run_require_clean_work_tree () {\n+\t(\n+\t\t. \"$(git --exec-path)\"/git-sh-setup &&\n+\t\trequire_clean_work_tree \"do-something\"\n+\t)\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_commit initial file\n+'\n+\n+test_expect_success 'success on clean index and worktree' '\n+\trun_require_clean_work_tree\n+'\n+\n+test_expect_success 'error on dirty worktree' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo dirty >file &&\n+\ttest_must_fail run_require_clean_work_tree\n+'\n+\n+test_expect_success 'error on dirty index' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo dirty >file &&\n+\tgit add file &&\n+\ttest_must_fail run_require_clean_work_tree\n+'\n+\n+test_expect_success 'error on dirty index and worktree' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo dirty >file &&\n+\tgit add file &&\n+\techo dirtier >file &&\n+\ttest_must_fail run_require_clean_work_tree\n+'\n+\n+test_expect_success 'error on clean index and worktree while on orphan branch' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --orphan orphan &&\n+\tgit reset --hard &&\n+\ttest_must_fail run_require_clean_work_tree\n+'\n+\n+test_expect_success 'error on dirty index while on orphan branch' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --orphan orphan &&\n+\ttest_must_fail run_require_clean_work_tree\n+'\n+\n+test_expect_success 'error on dirty index and work tree while on orphan branch' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --orphan orphan &&\n+\techo dirty >file &&\n+\ttest_must_fail run_require_clean_work_tree\n+'\n+\n+test_done\n-- \n2.6.3.418.gc3b7987\n"},{"id":"273640","messageId":"1448376345-27339-2-git-send-email-szeder@ira.uka.de","threadId":"40868","inReplyTo":"1448376345-27339-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-11-24T14:45:45Z","receivedAt":"2015-11-24T14:45:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"git-sh-setup's require_clean_work_tree() always exits with error on an\norphan branch, even when the index and worktree are both clean. The\nreason is that require_clean_work_tree() starts off with verifying\nHEAD, to make sure that it can safely pass HEAD to 'git diff-index'\nlater when it comes to checking the cleanness of the index, and errors\nout finding the invalid HEAD of the orphan branch.\n\nThere are scripts requiring a clean worktree that should work on an\norphan branch as well, and those should be able to use this function\ninstead of duplicating its functionality and nice error reporting in a\nway that handles orphan branches.\n\nFixing this is easy: the index should be compared to the empty tree\nwhile on an orphan branch, and to HEAD otherwise.\n\nHowever, just fixing require_clean_work_tree() this way is also\ndangerous, because scripts must take care to work properly on orphan\nbranches. Currently a script calling require_clean_work_tree() would\nexit on a clean orphan branch, but with the simple fix it would\ncontinue executing and who knows what the consequences might be if\nthe script is not prepared for orphan branches.\n\nLet scripts tell git-sh-setup that they are capable of dealing with\norphan branches by setting the ORPHAN_OK variable, similar to how the\nability to run in a subdirectory must be signalled by setting\nSUBDIRECTORY_OK. Only if ORPHAN_OK is set while on an orphan branch\nwill require_clean_work_tree() go on and compare the index and\nworktree to the empty tree, otherwise it will exit with error even\nwhen the index and worktree are clean, i.e the default exit behavior\ndoesn't change.\n\nAlso provide an error message in the orphan branch/invalid HEAD case\nthat is consistent in style with the function's other error messages\ninstead of the error message coming straight from 'git rev-parse\n--verify'.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-sh-setup.txt | 3 ++-\n git-sh-setup.sh | 16 ++++++++++++++--\n t/t2301-require-clean-work-tree.sh | 29 +++++++++++++++++++++++++++++\n 3 files changed, 45 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt\nindex 4f67c4cde6..bccaa2488f 100644\n--- a/Documentation/git-sh-setup.txt\n+++ b/Documentation/git-sh-setup.txt\n@@ -25,7 +25,8 @@ Before sourcing it, your script should set up a few variables;\n `USAGE` (and `LONG_USAGE`, if any) is used to define message\n given by `usage()` shell function. `SUBDIRECTORY_OK` can be set\n if the script can run from a subdirectory of the working tree\n-(some commands do not).\n+(some commands do not). `ORPHAN_OK` can be set if the script can\n+work on orphan branches.\n \n The scriptlet sets `GIT_DIR` and `GIT_OBJECT_DIRECTORY` shell\n variables, but does *not* export them to the environment.\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 4691fbcb64..f45b69386e 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -200,7 +200,19 @@ require_work_tree () {\n }\n \n require_clean_work_tree () {\n-\tgit rev-parse --verify HEAD >/dev/null || exit 1\n+\tif git rev-parse --verify HEAD >/dev/null 2>/dev/null\n+\tthen\n+\t\tcompare_to=HEAD\n+\telse\n+\t\tif [ -z \"$ORPHAN_OK\" ]\n+\t\tthen\n+\t\t\techo >&2 \"Cannot $1: Your current branch does not have any commits yet.\"\n+\t\t\texit 1\n+\t\telse\n+\t\t\t# SHA1 of an empty tree\n+\t\t\tcompare_to=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\t\tfi\n+\tfi\n \tgit update-index -q --ignore-submodules --refresh\n \terr=0\n \n@@ -210,7 +222,7 @@ require_clean_work_tree () {\n \t\terr=1\n \tfi\n \n-\tif ! git diff-index --cached --quiet --ignore-submodules HEAD --\n+\tif ! git diff-index --cached --quiet --ignore-submodules $compare_to --\n \tthen\n \t\tif [ $err = 0 ]\n \t\tthen\ndiff --git a/t/t2301-require-clean-work-tree.sh b/t/t2301-require-clean-work-tree.sh\nindex 1bb41b59a5..6d0957981e 100755\n--- a/t/t2301-require-clean-work-tree.sh\n+++ b/t/t2301-require-clean-work-tree.sh\n@@ -60,4 +60,33 @@ test_expect_success 'error on dirty index and work tree while on orphan branch'\n \ttest_must_fail run_require_clean_work_tree\n '\n \n+test_expect_success 'ORPHAN_OK - success on clean index and worktree while on orphan branch' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --orphan orphan &&\n+\tgit reset --hard &&\n+\t(\n+\t\tORPHAN_OK=Yes &&\n+\t\trun_require_clean_work_tree\n+\t)\n+'\n+\n+test_expect_success 'ORPHAN_OK - error on dirty index while on orphan branch' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --orphan orphan &&\n+\t(\n+\t\tORPHAN_OK=Yes &&\n+\t\ttest_must_fail run_require_clean_work_tree\n+\t)\n+'\n+\n+test_expect_success 'ORPHAN_OK - error on dirty index and worktree while on orphan branch' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --orphan orphan &&\n+\techo dirty >file &&\n+\t(\n+\t\tORPHAN_OK=Yes &&\n+\t\ttest_must_fail run_require_clean_work_tree\n+\t)\n+'\n+\n test_done\n-- \n2.6.3.418.gc3b7987\n"},{"id":"273652","messageId":"20151124205036.GF7174@sigill.intra.peff.net","threadId":"40868","inReplyTo":"1448376345-27339-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T20:50:37Z","receivedAt":"2015-11-24T20:50:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 24, 2015 at 03:45:45PM +0100, SZEDER Gábor wrote:\n\n> git-sh-setup's require_clean_work_tree() always exits with error on an\n> orphan branch, even when the index and worktree are both clean. The\n> reason is that require_clean_work_tree() starts off with verifying\n> HEAD, to make sure that it can safely pass HEAD to 'git diff-index'\n> later when it comes to checking the cleanness of the index, and errors\n> out finding the invalid HEAD of the orphan branch.\n> \n> There are scripts requiring a clean worktree that should work on an\n> orphan branch as well, and those should be able to use this function\n> instead of duplicating its functionality and nice error reporting in a\n> way that handles orphan branches.\n> \n> Fixing this is easy: the index should be compared to the empty tree\n> while on an orphan branch, and to HEAD otherwise.\n> \n> However, just fixing require_clean_work_tree() this way is also\n> dangerous, because scripts must take care to work properly on orphan\n> branches. Currently a script calling require_clean_work_tree() would\n> exit on a clean orphan branch, but with the simple fix it would\n> continue executing and who knows what the consequences might be if\n> the script is not prepared for orphan branches.\n\nHmm. I suspect this is not a big deal in practice. Lots of scripts\n(including some of our own, through history) get the orphan case wrong.\nI'm not sure that require_clean_work_tree is necessarily the place to be\nenforcing it, even though it happened to have done so historically.\n\nStill, it may be prudent to err on the side of caution. I'm on the\nfence.\n\n-Peff\n"},{"id":"273702","messageId":"56555A5B.20504@kdbg.org","threadId":"40868","inReplyTo":"1448376345-27339-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-25T06:51:07Z","receivedAt":"2015-11-25T06:51:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.11.2015 um 15:45 schrieb SZEDER Gábor:\n> git-sh-setup's require_clean_work_tree() always exits with error on an\n> orphan branch, even when the index and worktree are both clean. The\n> reason is that require_clean_work_tree() starts off with verifying\n> HEAD, to make sure that it can safely pass HEAD to 'git diff-index'\n> later when it comes to checking the cleanness of the index, and errors\n> out finding the invalid HEAD of the orphan branch.\n>\n> There are scripts requiring a clean worktree that should work on an\n> orphan branch as well, and those should be able to use this function\n> instead of duplicating its functionality and nice error reporting in a\n> way that handles orphan branches.\n>\n> Fixing this is easy: the index should be compared to the empty tree\n> while on an orphan branch, and to HEAD otherwise.\n>\n> However, just fixing require_clean_work_tree() this way is also\n> dangerous, because scripts must take care to work properly on orphan\n> branches. Currently a script calling require_clean_work_tree() would\n> exit on a clean orphan branch, but with the simple fix it would\n> continue executing and who knows what the consequences might be if\n> the script is not prepared for orphan branches.\n>\n> Let scripts tell git-sh-setup that they are capable of dealing with\n> orphan branches by setting the ORPHAN_OK variable, similar to how the\n> ability to run in a subdirectory must be signalled by setting\n> SUBDIRECTORY_OK. Only if ORPHAN_OK is set while on an orphan branch\n> will require_clean_work_tree() go on and compare the index and\n> worktree to the empty tree, otherwise it will exit with error even\n> when the index and worktree are clean, i.e the default exit behavior\n> doesn't change.\n>\n> Also provide an error message in the orphan branch/invalid HEAD case\n> that is consistent in style with the function's other error messages\n> instead of the error message coming straight from 'git rev-parse\n> --verify'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-sh-setup.txt | 3 ++-\n> git-sh-setup.sh | 16 ++++++++++++++--\n> t/t2301-require-clean-work-tree.sh | 29 +++++++++++++++++++++++++++++\n> 3 files changed, 45 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt\n> index 4f67c4cde6..bccaa2488f 100644\n> --- a/Documentation/git-sh-setup.txt\n> +++ b/Documentation/git-sh-setup.txt\n> @@ -25,7 +25,8 @@ Before sourcing it, your script should set up a few variables;\n> `USAGE` (and `LONG_USAGE`, if any) is used to define message\n> given by `usage()` shell function. `SUBDIRECTORY_OK` can be set\n> if the script can run from a subdirectory of the working tree\n> -(some commands do not).\n> +(some commands do not). `ORPHAN_OK` can be set if the script can\n> +work on orphan branches.\n>\n> The scriptlet sets `GIT_DIR` and `GIT_OBJECT_DIRECTORY` shell\n> variables, but does *not* export them to the environment.\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 4691fbcb64..f45b69386e 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -200,7 +200,19 @@ require_work_tree () {\n> }\n>\n> require_clean_work_tree () {\n> -\tgit rev-parse --verify HEAD >/dev/null || exit 1\n> +\tif git rev-parse --verify HEAD >/dev/null 2>/dev/null\n> +\tthen\n> +\t\tcompare_to=HEAD\n> +\telse\n> +\t\tif [ -z \"$ORPHAN_OK\" ]\n> +\t\tthen\n> +\t\t\techo >&2 \"Cannot $1: Your current branch does not have any commits yet.\"\n> +\t\t\texit 1\n> +\t\telse\n> +\t\t\t# SHA1 of an empty tree\n> +\t\t\tcompare_to=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n> +\t\tfi\n> +\tfi\n\nIt is worrysome that this now throws away any error message of \nrev-parse. A more conservative approach would be to test for -z \n\"$ORPHAN_OK\" first and entail new behavior only for the \"$ORPHAN_OK\" case.\n\n> \tgit update-index -q --ignore-submodules --refresh\n> \terr=0\n>\n> @@ -210,7 +222,7 @@ require_clean_work_tree () {\n> \t\terr=1\n> \tfi\n>\n> -\tif ! git diff-index --cached --quiet --ignore-submodules HEAD --\n> +\tif ! git diff-index --cached --quiet --ignore-submodules $compare_to --\n> \tthen\n> \t\tif [ $err = 0 ]\n> \t\tthen\n> diff --git a/t/t2301-require-clean-work-tree.sh b/t/t2301-require-clean-work-tree.sh\n> index 1bb41b59a5..6d0957981e 100755\n> --- a/t/t2301-require-clean-work-tree.sh\n> +++ b/t/t2301-require-clean-work-tree.sh\n> @@ -60,4 +60,33 @@ test_expect_success 'error on dirty index and work tree while on orphan branch'\n> \ttest_must_fail run_require_clean_work_tree\n> '\n>\n> +test_expect_success 'ORPHAN_OK - success on clean index and worktree while on orphan branch' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --orphan orphan &&\n> +\tgit reset --hard &&\n> +\t(\n> +\t\tORPHAN_OK=Yes &&\n> +\t\trun_require_clean_work_tree\n> +\t)\n> +'\n> +\n> +test_expect_success 'ORPHAN_OK - error on dirty index while on orphan branch' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --orphan orphan &&\n> +\t(\n> +\t\tORPHAN_OK=Yes &&\n> +\t\ttest_must_fail run_require_clean_work_tree\n> +\t)\n> +'\n> +\n> +test_expect_success 'ORPHAN_OK - error on dirty index and worktree while on orphan branch' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --orphan orphan &&\n> +\techo dirty >file &&\n> +\t(\n> +\t\tORPHAN_OK=Yes &&\n> +\t\ttest_must_fail run_require_clean_work_tree\n> +\t)\n> +'\n\nTests with ORPHAN_OK=Yes, but being on a non-orphaned branch are missing.\n\n-- Hannes\n"},{"id":"273826","messageId":"20151130132424.Horde.2BMsKJ5eWOY71RtsJbP8UQw@webmail.informatik.kit.edu","threadId":"40868","inReplyTo":"56555A5B.20504@kdbg.org","subject":"Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-11-30T12:24:24Z","receivedAt":"2015-11-30T12:24:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Johannes Sixt :\n\n>> --- a/git-sh-setup.sh\n>> +++ b/git-sh-setup.sh\n>> @@ -200,7 +200,19 @@ require_work_tree () {\n>> }\n>>\n>> require_clean_work_tree () {\n>> -\tgit rev-parse --verify HEAD >/dev/null || exit 1\n>> +\tif git rev-parse --verify HEAD >/dev/null 2>/dev/null\n>> +\tthen\n>> +\t\tcompare_to=HEAD\n>> +\telse\n>> +\t\tif [ -z \"$ORPHAN_OK\" ]\n>> +\t\tthen\n>> +\t\t\techo >&2 \"Cannot $1: Your current branch does not have any commits yet.\"\n>> +\t\t\texit 1\n>> +\t\telse\n>> +\t\t\t# SHA1 of an empty tree\n>> +\t\t\tcompare_to=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n>> +\t\tfi\n>> +\tfi\n>\n> It is worrysome that this now throws away any error message of\n> rev-parse. A more conservative approach would be to test for -z\n> \"$ORPHAN_OK\" first and entail new behavior only for the \"$ORPHAN_OK\"\n> case.\n\nThose error messages didn't seem to be helpful and in some cases they\nwere even misleading, e.g.:\n\n $ git checkout --orphan orphan\n $ git rm -rf .\n $ git script valid-branch\n fatal: Needed a single revision\n $ # Huh? Didn't I just gave you one?!\n\nGábor\n"},{"id":"273827","messageId":"20151130133735.Horde.6JXMnrTluxoV71C8eVKrKLi@webmail.informatik.kit.edu","threadId":"40868","inReplyTo":"20151124205036.GF7174@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-11-30T12:37:35Z","receivedAt":"2015-11-30T12:37:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Jeff King :\n\n> On Tue, Nov 24, 2015 at 03:45:45PM +0100, SZEDER Gábor wrote:\n>\n>> git-sh-setup's require_clean_work_tree() always exits with error on an\n>> orphan branch, even when the index and worktree are both clean. The\n>> reason is that require_clean_work_tree() starts off with verifying\n>> HEAD, to make sure that it can safely pass HEAD to 'git diff-index'\n>> later when it comes to checking the cleanness of the index, and errors\n>> out finding the invalid HEAD of the orphan branch.\n>>\n>> There are scripts requiring a clean worktree that should work on an\n>> orphan branch as well, and those should be able to use this function\n>> instead of duplicating its functionality and nice error reporting in a\n>> way that handles orphan branches.\n>>\n>> Fixing this is easy: the index should be compared to the empty tree\n>> while on an orphan branch, and to HEAD otherwise.\n>>\n>> However, just fixing require_clean_work_tree() this way is also\n>> dangerous, because scripts must take care to work properly on orphan\n>> branches. Currently a script calling require_clean_work_tree() would\n>> exit on a clean orphan branch, but with the simple fix it would\n>> continue executing and who knows what the consequences might be if\n>> the script is not prepared for orphan branches.\n>\n> Hmm. I suspect this is not a big deal in practice. Lots of scripts\n> (including some of our own, through history) get the orphan case wrong.\n\nWell, to be honest, I thought it's not a big deal, too, but I also \nthought that the patch will be quickly shot down during review if I \ndon't include some kind of a defense :)\nI'd be happier to reroll treating the current behavior in the clean \norphan case as a bug, marking that test as expect_failure, and then \njust fixing it without the ORPHAN_OK thing.\n\n> I'm not sure that require_clean_work_tree is necessarily the place to be\n> enforcing it, even though it happened to have done so historically.\n\nYeah, right... It should be checked in the main code path, in \ngit_dir_init(), but then it could hurt scripts that already do the \nright thing on an orphan branch because they don't set the ORPHAN_OK \nvariable. Though that's probably not a big deal in practice either. \nAnyway, as it stands the documentation update of this patch is wrong.\n"},{"id":"273940","messageId":"xmqqwpsw4fhi.fsf@gitster.mtv.corp.google.com","threadId":"40868","inReplyTo":"20151124205036.GF7174@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-02T23:07:05Z","receivedAt":"2015-12-02T23:07:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 24, 2015 at 03:45:45PM +0100, SZEDER Gábor wrote:\n>\n>> git-sh-setup's require_clean_work_tree() always exits with error on an\n>> orphan branch, even when the index and worktree are both clean. The\n>> reason is that require_clean_work_tree() starts off with verifying\n>> HEAD, to make sure that it can safely pass HEAD to 'git diff-index'\n>> later when it comes to checking the cleanness of the index, and errors\n>> out finding the invalid HEAD of the orphan branch.\n>> \n>> There are scripts requiring a clean worktree that should work on an\n>> orphan branch as well, and those should be able to use this function\n>> instead of duplicating its functionality and nice error reporting in a\n>> way that handles orphan branches.\n>> \n>> Fixing this is easy: the index should be compared to the empty tree\n>> while on an orphan branch, and to HEAD otherwise.\n>> \n>> However, just fixing require_clean_work_tree() this way is also\n>> dangerous, because scripts must take care to work properly on orphan\n>> branches. Currently a script calling require_clean_work_tree() would\n>> exit on a clean orphan branch, but with the simple fix it would\n>> continue executing and who knows what the consequences might be if\n>> the script is not prepared for orphan branches.\n>\n> Hmm. I suspect this is not a big deal in practice. Lots of scripts\n> (including some of our own, through history) get the orphan case wrong.\n\nThe state of the repository this topic wants to deal with better\nwould be the same as the state you would get after \"tar xf ... &&\ngit init && git add .\", no? In the \"orphan\" case, unlike such a\nplain vanilla \"created a new repository\" case where the index could\nbe empty for a long time before the initial 'git add', the index is\nalmost always populated, so most likely this change will not make\nmuch difference in that require_clean_work_tree() would stop\noperation until \"rm --cached .\" empties the index.\n\nAnd if the user did \"rm --cached .\" to empty the index, it is fine\nfor us to happily consider all these files as untracked, even when\nHEAD is not pointing at an existing ref. So it is likely that the\nadded ORPHAN_OK knob would be unnecessary for \"orphan\" case.\n\nHowever, I suspect that this would affect the users in the \"I am a\nnew user, I just initialized my first Git repository and I haven't\ndone a 'git add' yet\" status much more. Do we have corner cases\nwhere everyday commands do not work well while on an unborn branch\nimmediately after 'git init'? That is the kind of bug that we need\nto protect users from, either by keeping the \"No HEAD yet, no operation\nthat requires clean working tree\" logic, or by fixing such bugs.\n\n> I'm not sure that require_clean_work_tree is necessarily the place to be\n> enforcing it, even though it happened to have done so historically.\n\nIt is unclear to me what you meant by \"it\" in \"enforcing it\".\n\n> Still, it may be prudent to err on the side of caution. I'm on the\n> fence.\n"},{"id":"276472","messageId":"xmqqbn8f3iq5.fsf@gitster.mtv.corp.google.com","threadId":"40868","inReplyTo":"1448376345-27339-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-20T23:59:46Z","receivedAt":"2016-01-20T23:59:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Add tests that check require_clean_work_tree() in the common cases,\n> i.e. on a branch with all combinations of clean and dirty index and\n> worktree, and also add tests that exercise it on an orphan branch.\n>\n> require_clean_work_tree()'s behavior in the orphan branch cases is\n> questionable, as it exits with error on an orphan branch independently\n> from whether the index and worktree are clean or dirty (and it does so\n> because of the invalid HEAD, meaning that even when it should exit\n> with error, it does so for the wrong reason). As scripts might rely\n> on the current behavior, we err on the side of compatibility and\n> safety, and expect the current behavior as success.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n\nI see this was queued while I was not around, but has been in \"Needs\nreview\" bucket in \"What's cooking\" report forever, without getting\nreviews.\n\n> +test_description='require_clean_work_tree'\n> +\n> +. ./test-lib.sh\n> +\n> +run_require_clean_work_tree () {\n> +\t(\n> +\t\t. \"$(git --exec-path)\"/git-sh-setup &&\n> +\t\trequire_clean_work_tree \"do-something\"\n\n\"do-something\"? What are the typical thing to write here in real\nscripts? A name of the operation that wanted to ensure that the\nworking tree is clean, with an optional hint string?\n\nPerhaps you want to take them as arguments to this helper function?\nThat way you do not have to decide what to pass here, i.e.\n\n-\t\trequire_clean_work_tree \"do-something\"\n+\t\trequire_clean_work_tree \"$@\"\n\n> +\t)\n> +}\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit initial file\n> +'\n> +\n> +test_expect_success 'success on clean index and worktree' '\n> +\trun_require_clean_work_tree\n\n... and if you want to, you can give the comment here, e.g.\n\n\trun_require_clean_work_tree \"verify clean after init\"\n\nor something.\n\n> +'\n> +\n> +test_expect_success 'error on dirty worktree' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\techo dirty >file &&\n> +\ttest_must_fail run_require_clean_work_tree\n\nI think this should be\n\n\t! run_require_clean_work_tree\n\n> +'\n> +\n> +test_expect_success 'error on dirty index' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\techo dirty >file &&\n> +\tgit add file &&\n> +\ttest_must_fail run_require_clean_work_tree\n> +'\n\nLikewise.\n\n> +test_expect_success 'error on dirty index and worktree' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\techo dirty >file &&\n> +\tgit add file &&\n> +\techo dirtier >file &&\n> +\ttest_must_fail run_require_clean_work_tree\n> +'\n> +\n> +test_expect_success 'error on clean index and worktree while on orphan branch' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --orphan orphan &&\n> +\tgit reset --hard &&\n> +\ttest_must_fail run_require_clean_work_tree\n> +'\n\nThe title is wrong. Immediately after creating and getting on an\norphan branch, you have stuff in the index that is not committed to\nthe branch, so your index cannot be clean by definition. The\ncontents of the working tree may or may not be clean immediately\nafter getting on a new orphan branch, but you are doing \"reset\n--hard\" to make the index and the working tree agree, so this is\ntesting the \"clean working tree\" case, I think.\n\n> +test_expect_success 'error on dirty index while on orphan branch' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --orphan orphan &&\n> +\ttest_must_fail run_require_clean_work_tree\n> +'\n\nAssuming that the previous test succeeded and this test started with\na clean index and the working tree, checkout --orphan would give you\na dirty-index with clean working tree state. So both the title and\nthe expectation of the test are correct, I think.\n\nBut it would be better not to rely on the effect of\ntest-when-finished of the previous test too much. \"git checkout\nmaster && git reset --hard\" at the beginning would be easier to read\nthe test and reason about it in isolation.\n\n> +test_expect_success 'error on dirty index and work tree while on orphan branch' '\n> +\ttest_when_finished \"git checkout master\" &&\n> +\tgit checkout --orphan orphan &&\n> +\techo dirty >file &&\n> +\ttest_must_fail run_require_clean_work_tree\n> +'\n\nLikewise.\n\nThanks.\n"},{"id":"276473","messageId":"xmqq7fj33ifq.fsf@gitster.mtv.corp.google.com","threadId":"40868","inReplyTo":"1448376345-27339-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-21T00:06:01Z","receivedAt":"2016-01-21T00:06:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> git-sh-setup's require_clean_work_tree() always exits with error on an\n> orphan branch, even when the index and worktree are both clean.\n\nAh, I just sent a review on 1/2 as I found it was not commented on\nby anybody, but re-reading this sentence (and subsequent review of\nthis 2/2) makes it clear this is not a good idea. By definition,\nthe index immediately after creating and getting on an orphan branch\nis not clean [*1*].\n\nI'll move the topic from \"Needs review\" to \"Will discard\" pile in my\ntree.\n\nSorry for the noise.\n\n\n[Footnote]\n\n*1* Roughly speaking, if \"git commit\" (no arguments) to record the\nstate of the index succeeds without \"--allow-empty\", your index is\ndirty with respect to HEAD.\n"},{"id":"276479","messageId":"20160121022711.Horde.7Nh0N0fTAbd0JWIA99KKFQF@webmail.informatik.kit.edu","threadId":"40868","inReplyTo":"xmqqbn8f3iq5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-01-21T01:27:11Z","receivedAt":"2016-01-21T01:27:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n>> +test_expect_success 'error on clean index and worktree while on \n>> orphan branch' '\n>> +\ttest_when_finished \"git checkout master\" &&\n>> +\tgit checkout --orphan orphan &&\n>> +\tgit reset --hard &&\n>> +\ttest_must_fail run_require_clean_work_tree\n>> +'\n>\n> The title is wrong. Immediately after creating and getting on an\n> orphan branch, you have stuff in the index that is not committed to\n> the branch, so your index cannot be clean by definition.\n\nThe index contains the file 'file', so it's not clean indeed.\n\n> The\n> contents of the working tree may or may not be clean immediately\n> after getting on a new orphan branch, but you are doing \"reset\n> --hard\" to make the index and the working tree agree,\n\n... and match HEAD, which in this case means both the index and the\nworktree become empty.\n\n'git rm -r .' would have made the intent clearer. Or 'git init emptyrepo'.\n\n> so this is\n> testing the \"clean working tree\" case, I think.\n\nSo the question is, before we go any further: are an empty index and\nempty worktree clean when HEAD doesn't point to a commit? (either after\nthe command sequence in the above test, or right after 'git init').\n\nI do think they are clean.\n\n\nGábor\n"},{"id":"276486","messageId":"xmqqa8nzzg7z.fsf@gitster.mtv.corp.google.com","threadId":"40868","inReplyTo":"20160121022711.Horde.7Nh0N0fTAbd0JWIA99KKFQF@webmail.informatik.kit.edu","subject":"Re: [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-21T04:52:48Z","receivedAt":"2016-01-21T04:52:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Quoting Junio C Hamano :\n>\n>>> +test_expect_success 'error on clean index and worktree while on \n>>> orphan branch' '\n>>> +\ttest_when_finished \"git checkout master\" &&\n>>> +\tgit checkout --orphan orphan &&\n>>> +\tgit reset --hard &&\n>>> +\ttest_must_fail run_require_clean_work_tree\n>>> +'\n>>\n>> The title is wrong. Immediately after creating and getting on an\n>> orphan branch, you have stuff in the index that is not committed to\n>> the branch, so your index cannot be clean by definition.\n>\n> The index contains the file 'file', so it's not clean indeed.\n>\n>> The\n>> contents of the working tree may or may not be clean immediately\n>> after getting on a new orphan branch, but you are doing \"reset\n>> --hard\" to make the index and the working tree agree,\n>\n> ... and match HEAD, which in this case means both the index and the\n> worktree become empty.\n>\n> 'git rm -r .' would have made the intent clearer. Or 'git init emptyrepo'.\n>\n>> so this is\n>> testing the \"clean working tree\" case, I think.\n>\n> So the question is, before we go any further: are an empty index and\n> empty worktree clean when HEAD doesn't point to a commit? (either after\n> the command sequence in the above test, or right after 'git init').\n>\n> I do think they are clean.\n\nThat is \"rm -fr test && git init test && cd test\" case, right?\n\nIt may be consistent to define it as clean, but I am not sure\nexisting operations that require a clean working tree is useful in\nsuch a state (filter-branch, pull-rebase and rebase are the big\nusers, and none of them is so useful with an unborn history), so\nloosening the definition would be not just helpful for them but\nwould be harmful, as they (implicitly) rely on require-clean to\nstop the command early before they try to do something that needs\nan existing history.\n\nSo, I am not sure if it is a good idea.\n"}]}