[PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()

[PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()

[SZEDER Gábor]

Add tests that check require_clean_work_tree() in the common cases,
i.e. on a branch with all combinations of clean and dirty index and
worktree, and also add tests that exercise it on an orphan branch.

require_clean_work_tree()'s behavior in the orphan branch cases is
questionable, as it exits with error on an orphan branch independently
from whether the index and worktree are clean or dirty (and it does so
because of the invalid HEAD, meaning that even when it should exit
with error, it does so for the wrong reason).  As scripts might rely
on the current behavior, we err on the side of compatibility and
safety, and expect the current behavior as success.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/t2301-require-clean-work-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100755 t/t2301-require-clean-work-tree.sh

diff --git a/t/t2301-require-clean-work-tree.sh b/t/t2301-require-clean-work-tree.sh
new file mode 100755
index 0000000000..1bb41b59a5
--- /dev/null
+++ b/t/t2301-require-clean-work-tree.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='require_clean_work_tree'
+
+. ./test-lib.sh
+
+run_require_clean_work_tree () {
+	(
+		. "$(git --exec-path)"/git-sh-setup &&
+		require_clean_work_tree "do-something"
+	)
+}
+
+test_expect_success 'setup' '
+	test_commit initial file
+'
+
+test_expect_success 'success on clean index and worktree' '
+	run_require_clean_work_tree
+'
+
+test_expect_success 'error on dirty worktree' '
+	test_when_finished "git reset --hard" &&
+	echo dirty >file &&
+	test_must_fail run_require_clean_work_tree
+'
+
+test_expect_success 'error on dirty index' '
+	test_when_finished "git reset --hard" &&
+	echo dirty >file &&
+	git add file &&
+	test_must_fail run_require_clean_work_tree
+'
+
+test_expect_success 'error on dirty index and worktree' '
+	test_when_finished "git reset --hard" &&
+	echo dirty >file &&
+	git add file &&
+	echo dirtier >file &&
+	test_must_fail run_require_clean_work_tree
+'
+
+test_expect_success 'error on clean index and worktree while on orphan branch' '
+	test_when_finished "git checkout master" &&
+	git checkout --orphan orphan &&
+	git reset --hard &&
+	test_must_fail run_require_clean_work_tree
+'
+
+test_expect_success 'error on dirty index while on orphan branch' '
+	test_when_finished "git checkout master" &&
+	git checkout --orphan orphan &&
+	test_must_fail run_require_clean_work_tree
+'
+
+test_expect_success 'error on dirty index and work tree while on orphan branch' '
+	test_when_finished "git checkout master" &&
+	git checkout --orphan orphan &&
+	echo dirty >file &&
+	test_must_fail run_require_clean_work_tree
+'
+
+test_done
-- 
2.6.3.418.gc3b7987

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

[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
 	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
+	)
+'
+
 test_done
-- 
2.6.3.418.gc3b7987

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

[Jeff King]

On Tue, Nov 24, 2015 at 03:45:45PM +0100, SZEDER Gábor wrote:

> 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.

Hmm. I suspect this is not a big deal in practice. Lots of scripts
(including some of our own, through history) get the orphan case wrong.
I'm not sure that require_clean_work_tree is necessarily the place to be
enforcing it, even though it happened to have done so historically.

Still, it may be prudent to err on the side of caution. I'm on the
fence.

-Peff

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

[Johannes Sixt]

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

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

[SZEDER Gábor]

Quoting Johannes Sixt <j6t@kdbg.org>:

>> --- 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.

Those error messages didn't seem to be helpful and in some cases they
were even misleading, e.g.:

    $ git checkout --orphan orphan
    $ git rm -rf .
    $ git script valid-branch
    fatal: Needed a single revision
    $ # Huh?  Didn't I just gave you one?!

Gábor

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

[SZEDER Gábor]

Quoting Jeff King <peff@peff.net>:

> On Tue, Nov 24, 2015 at 03:45:45PM +0100, SZEDER Gábor wrote:
>
>> 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.
>
> Hmm. I suspect this is not a big deal in practice. Lots of scripts
> (including some of our own, through history) get the orphan case wrong.

Well, to be honest, I thought it's not a big deal, too, but I also  
thought that the patch will be quickly shot down during review if I  
don't include some kind of a defense :)
I'd be happier to reroll treating the current behavior in the clean  
orphan case as a bug, marking that test as expect_failure, and then  
just fixing it without the ORPHAN_OK thing.

> I'm not sure that require_clean_work_tree is necessarily the place to be
> enforcing it, even though it happened to have done so historically.

Yeah, right...  It should be checked in the main code path, in  
git_dir_init(), but then it could hurt scripts that already do the  
right thing on an orphan branch because they don't set the ORPHAN_OK  
variable.  Though that's probably not a big deal in practice either.   
Anyway, as it stands the documentation update of this patch is wrong.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 24, 2015 at 03:45:45PM +0100, SZEDER Gábor wrote:
>
>> 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.
>
> Hmm. I suspect this is not a big deal in practice. Lots of scripts
> (including some of our own, through history) get the orphan case wrong.

The state of the repository this topic wants to deal with better
would be the same as the state you would get after "tar xf ... &&
git init && git add .", no?  In the "orphan" case, unlike such a
plain vanilla "created a new repository" case where the index could
be empty for a long time before the initial 'git add', the index is
almost always populated, so most likely this change will not make
much difference in that require_clean_work_tree() would stop
operation until "rm --cached ." empties the index.

And if the user did "rm --cached ." to empty the index, it is fine
for us to happily consider all these files as untracked, even when
HEAD is not pointing at an existing ref.  So it is likely that the
added ORPHAN_OK knob would be unnecessary for "orphan" case.

However, I suspect that this would affect the users in the "I am a
new user, I just initialized my first Git repository and I haven't
done a 'git add' yet" status much more.  Do we have corner cases
where everyday commands do not work well while on an unborn branch
immediately after 'git init'?  That is the kind of bug that we need
to protect users from, either by keeping the "No HEAD yet, no operation
that requires clean working tree" logic, or by fixing such bugs.

> I'm not sure that require_clean_work_tree is necessarily the place to be
> enforcing it, even though it happened to have done so historically.

It is unclear to me what you meant by "it" in "enforcing it".

> Still, it may be prudent to err on the side of caution. I'm on the
> fence.

Re: [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> Add tests that check require_clean_work_tree() in the common cases,
> i.e. on a branch with all combinations of clean and dirty index and
> worktree, and also add tests that exercise it on an orphan branch.
>
> require_clean_work_tree()'s behavior in the orphan branch cases is
> questionable, as it exits with error on an orphan branch independently
> from whether the index and worktree are clean or dirty (and it does so
> because of the invalid HEAD, meaning that even when it should exit
> with error, it does so for the wrong reason).  As scripts might rely
> on the current behavior, we err on the side of compatibility and
> safety, and expect the current behavior as success.
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---

I see this was queued while I was not around, but has been in "Needs
review" bucket in "What's cooking" report forever, without getting
reviews.

> +test_description='require_clean_work_tree'
> +
> +. ./test-lib.sh
> +
> +run_require_clean_work_tree () {
> +	(
> +		. "$(git --exec-path)"/git-sh-setup &&
> +		require_clean_work_tree "do-something"

"do-something"?  What are the typical thing to write here in real
scripts?  A name of the operation that wanted to ensure that the
working tree is clean, with an optional hint string?

Perhaps you want to take them as arguments to this helper function?
That way you do not have to decide what to pass here, i.e.

-		require_clean_work_tree "do-something"
+		require_clean_work_tree "$@"

> +	)
> +}
> +
> +test_expect_success 'setup' '
> +	test_commit initial file
> +'
> +
> +test_expect_success 'success on clean index and worktree' '
> +	run_require_clean_work_tree

... and if you want to, you can give the comment here, e.g.

	run_require_clean_work_tree "verify clean after init"

or something.

> +'
> +
> +test_expect_success 'error on dirty worktree' '
> +	test_when_finished "git reset --hard" &&
> +	echo dirty >file &&
> +	test_must_fail run_require_clean_work_tree

I think this should be

	! run_require_clean_work_tree

> +'
> +
> +test_expect_success 'error on dirty index' '
> +	test_when_finished "git reset --hard" &&
> +	echo dirty >file &&
> +	git add file &&
> +	test_must_fail run_require_clean_work_tree
> +'

Likewise.

> +test_expect_success 'error on dirty index and worktree' '
> +	test_when_finished "git reset --hard" &&
> +	echo dirty >file &&
> +	git add file &&
> +	echo dirtier >file &&
> +	test_must_fail run_require_clean_work_tree
> +'
> +
> +test_expect_success 'error on clean index and worktree while on orphan branch' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --orphan orphan &&
> +	git reset --hard &&
> +	test_must_fail run_require_clean_work_tree
> +'

The title is wrong.  Immediately after creating and getting on an
orphan branch, you have stuff in the index that is not committed to
the branch, so your index cannot be clean by definition.  The
contents of the working tree may or may not be clean immediately
after getting on a new orphan branch, but you are doing "reset
--hard" to make the index and the working tree agree, so this is
testing the "clean working tree" case, I think.

> +test_expect_success 'error on dirty index while on orphan branch' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --orphan orphan &&
> +	test_must_fail run_require_clean_work_tree
> +'

Assuming that the previous test succeeded and this test started with
a clean index and the working tree, checkout --orphan would give you
a dirty-index with clean working tree state.  So both the title and
the expectation of the test are correct, I think.

But it would be better not to rely on the effect of
test-when-finished of the previous test too much.  "git checkout
master && git reset --hard" at the beginning would be easier to read
the test and reason about it in isolation.

> +test_expect_success 'error on dirty index and work tree while on orphan branch' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --orphan orphan &&
> +	echo dirty >file &&
> +	test_must_fail run_require_clean_work_tree
> +'

Likewise.

Thanks.

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> 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.

Ah, I just sent a review on 1/2 as I found it was not commented on
by anybody, but re-reading this sentence (and subsequent review of
this 2/2) makes it clear this is not a good idea.  By definition,
the index immediately after creating and getting on an orphan branch
is not clean [*1*].

I'll move the topic from "Needs review" to "Will discard" pile in my
tree.

Sorry for the noise.


[Footnote]

*1* Roughly speaking, if "git commit" (no arguments) to record the
state of the index succeeds without "--allow-empty", your index is
dirty with respect to HEAD.

Re: [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

>> +test_expect_success 'error on clean index and worktree while on  
>> orphan branch' '
>> +	test_when_finished "git checkout master" &&
>> +	git checkout --orphan orphan &&
>> +	git reset --hard &&
>> +	test_must_fail run_require_clean_work_tree
>> +'
>
> The title is wrong.  Immediately after creating and getting on an
> orphan branch, you have stuff in the index that is not committed to
> the branch, so your index cannot be clean by definition.

The index contains the file 'file', so it's not clean indeed.

> The
> contents of the working tree may or may not be clean immediately
> after getting on a new orphan branch, but you are doing "reset
> --hard" to make the index and the working tree agree,

... and match HEAD, which in this case means both the index and the
worktree become empty.

'git rm -r .' would have made the intent clearer.  Or 'git init emptyrepo'.

> so this is
> testing the "clean working tree" case, I think.

So the question is, before we go any further: are an empty index and
empty worktree clean when HEAD doesn't point to a commit?  (either after
the command sequence in the above test, or right after 'git init').

I do think they are clean.


Gábor

Re: [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> Quoting Junio C Hamano <gitster@pobox.com>:
>
>>> +test_expect_success 'error on clean index and worktree while on  
>>> orphan branch' '
>>> +	test_when_finished "git checkout master" &&
>>> +	git checkout --orphan orphan &&
>>> +	git reset --hard &&
>>> +	test_must_fail run_require_clean_work_tree
>>> +'
>>
>> The title is wrong.  Immediately after creating and getting on an
>> orphan branch, you have stuff in the index that is not committed to
>> the branch, so your index cannot be clean by definition.
>
> The index contains the file 'file', so it's not clean indeed.
>
>> The
>> contents of the working tree may or may not be clean immediately
>> after getting on a new orphan branch, but you are doing "reset
>> --hard" to make the index and the working tree agree,
>
> ... and match HEAD, which in this case means both the index and the
> worktree become empty.
>
> 'git rm -r .' would have made the intent clearer.  Or 'git init emptyrepo'.
>
>> so this is
>> testing the "clean working tree" case, I think.
>
> So the question is, before we go any further: are an empty index and
> empty worktree clean when HEAD doesn't point to a commit?  (either after
> the command sequence in the above test, or right after 'git init').
>
> I do think they are clean.

That is "rm -fr test && git init test && cd test" case, right?

It may be consistent to define it as clean, but I am not sure
existing operations that require a clean working tree is useful in
such a state (filter-branch, pull-rebase and rebase are the big
users, and none of them is so useful with an unborn history), so
loosening the definition would be not just helpful for them but
would be harmful, as they (implicitly) rely on require-clean to
stop the command early before they try to do something that needs
an existing history.

So, I am not sure if it is a good idea.