Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches

[Johannes Sixt <j6t@kdbg.org>]

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 <szeder@ira.uka.de>
> ---
>   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