Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Nov 25, 2015, 06:51 UTC
- 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:
Show 79 quoted lines
> 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
> + fiIt 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.
Show 48 quoted lines
> 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