From: Johannes Sixt Date: Wed, 25 Nov 2015 06:51:07 GMT Subject: Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches Message-ID: <56555A5B.20504@kdbg.org> In-Reply-To: <1448376345-27339-2-git-send-email-szeder@ira.uka.de> Am 24.11.2015 um 15:45 schrieb SZEDER Gábor: > git-sh-setup's require_clean_work_tree() always exits with error on an > orphan branch, even when the index and worktree are both clean. The > reason is that require_clean_work_tree() starts off with verifying > HEAD, to make sure that it can safely pass HEAD to 'git diff-index' > later when it comes to checking the cleanness of the index, and errors > out finding the invalid HEAD of the orphan branch. > > There are scripts requiring a clean worktree that should work on an > orphan branch as well, and those should be able to use this function > instead of duplicating its functionality and nice error reporting in a > way that handles orphan branches. > > Fixing this is easy: the index should be compared to the empty tree > while on an orphan branch, and to HEAD otherwise. > > However, just fixing require_clean_work_tree() this way is also > dangerous, because scripts must take care to work properly on orphan > branches. Currently a script calling require_clean_work_tree() would > exit on a clean orphan branch, but with the simple fix it would > continue executing and who knows what the consequences might be if > the script is not prepared for orphan branches. > > Let scripts tell git-sh-setup that they are capable of dealing with > orphan branches by setting the ORPHAN_OK variable, similar to how the > ability to run in a subdirectory must be signalled by setting > SUBDIRECTORY_OK. Only if ORPHAN_OK is set while on an orphan branch > will require_clean_work_tree() go on and compare the index and > worktree to the empty tree, otherwise it will exit with error even > when the index and worktree are clean, i.e the default exit behavior > doesn't change. > > Also provide an error message in the orphan branch/invalid HEAD case > that is consistent in style with the function's other error messages > instead of the error message coming straight from 'git rev-parse > --verify'. > > Signed-off-by: SZEDER Gábor > --- > Documentation/git-sh-setup.txt | 3 ++- > git-sh-setup.sh | 16 ++++++++++++++-- > t/t2301-require-clean-work-tree.sh | 29 +++++++++++++++++++++++++++++ > 3 files changed, 45 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt > index 4f67c4cde6..bccaa2488f 100644 > --- a/Documentation/git-sh-setup.txt > +++ b/Documentation/git-sh-setup.txt > @@ -25,7 +25,8 @@ Before sourcing it, your script should set up a few variables; > `USAGE` (and `LONG_USAGE`, if any) is used to define message > given by `usage()` shell function. `SUBDIRECTORY_OK` can be set > if the script can run from a subdirectory of the working tree > -(some commands do not). > +(some commands do not). `ORPHAN_OK` can be set if the script can > +work on orphan branches. > > The scriptlet sets `GIT_DIR` and `GIT_OBJECT_DIRECTORY` shell > variables, but does *not* export them to the environment. > diff --git a/git-sh-setup.sh b/git-sh-setup.sh > index 4691fbcb64..f45b69386e 100644 > --- a/git-sh-setup.sh > +++ b/git-sh-setup.sh > @@ -200,7 +200,19 @@ require_work_tree () { > } > > require_clean_work_tree () { > - git rev-parse --verify HEAD >/dev/null || exit 1 > + if git rev-parse --verify HEAD >/dev/null 2>/dev/null > + then > + compare_to=HEAD > + else > + if [ -z "$ORPHAN_OK" ] > + then > + echo >&2 "Cannot $1: Your current branch does not have any commits yet." > + exit 1 > + else > + # SHA1 of an empty tree > + compare_to=4b825dc642cb6eb9a060e54bf8d69288fbee4904 > + fi > + fi It is worrysome that this now throws away any error message of rev-parse. A more conservative approach would be to test for -z "$ORPHAN_OK" first and entail new behavior only for the "$ORPHAN_OK" case. > git update-index -q --ignore-submodules --refresh > err=0 > > @@ -210,7 +222,7 @@ require_clean_work_tree () { > err=1 > fi > > - if ! git diff-index --cached --quiet --ignore-submodules HEAD -- > + if ! git diff-index --cached --quiet --ignore-submodules $compare_to -- > then > if [ $err = 0 ] > then > diff --git a/t/t2301-require-clean-work-tree.sh b/t/t2301-require-clean-work-tree.sh > index 1bb41b59a5..6d0957981e 100755 > --- a/t/t2301-require-clean-work-tree.sh > +++ b/t/t2301-require-clean-work-tree.sh > @@ -60,4 +60,33 @@ test_expect_success 'error on dirty index and work tree while on orphan branch' > test_must_fail run_require_clean_work_tree > ' > > +test_expect_success 'ORPHAN_OK - success on clean index and worktree while on orphan branch' ' > + test_when_finished "git checkout master" && > + git checkout --orphan orphan && > + git reset --hard && > + ( > + ORPHAN_OK=Yes && > + run_require_clean_work_tree > + ) > +' > + > +test_expect_success 'ORPHAN_OK - error on dirty index while on orphan branch' ' > + test_when_finished "git checkout master" && > + git checkout --orphan orphan && > + ( > + ORPHAN_OK=Yes && > + test_must_fail run_require_clean_work_tree > + ) > +' > + > +test_expect_success 'ORPHAN_OK - error on dirty index and worktree while on orphan branch' ' > + test_when_finished "git checkout master" && > + git checkout --orphan orphan && > + echo dirty >file && > + ( > + ORPHAN_OK=Yes && > + test_must_fail run_require_clean_work_tree > + ) > +' Tests with ORPHAN_OK=Yes, but being on a non-orphaned branch are missing. -- Hannes