From: Johannes Sixt <j6t@kdbg•org>
To: "SZEDER Gábor" <szeder@ira•uka.de>
Cc: Jeff King <peff@peff•net>, git@vger•kernel.org
Subject: Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches
Date: Wed, 25 Nov 2015 07:51:07 +0100 [thread overview]
Message-ID: <56555A5B.20504@kdbg.org> (raw)
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 <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
next prev parent reply other threads:[~2015-11-25 6:51 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-24 14:45 [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree() SZEDER Gábor
2015-11-24 14:45 ` [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches SZEDER Gábor
2015-11-24 20:50 ` Jeff King
2015-11-30 12:37 ` SZEDER Gábor
2015-12-02 23:07 ` Junio C Hamano
2015-11-25 6:51 ` Johannes Sixt [this message]
2015-11-30 12:24 ` SZEDER Gábor
2016-01-21 0:06 ` Junio C Hamano
2016-01-20 23:59 ` [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree() Junio C Hamano
2016-01-21 1:27 ` SZEDER Gábor
2016-01-21 4:52 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56555A5B.20504@kdbg.org \
--to=j6t@kdbg$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=szeder@ira$(echo .)uka.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox