All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rebase -i: add exec command to launch a shell command
@ 2010-08-05 13:00 Matthieu Moy
  2010-08-05 13:31 ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 15+ messages in thread
From: Matthieu Moy @ 2010-08-05 13:00 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

The typical usage pattern would be to run a test (or simply a compilation
command) at given points in history.

The shell command is ran (from the worktree root), and the rebase is
stopped when the command fails, to give the user an opportunity to fix
the problem before continuing with "git rebase --continue".

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

So, back to the "run from tree root", but that't now properly
documented and tested.

One notable difference with my first version is that the command is
ran in a subshell, defaulting to $SHELL (typically for users like me
with $SHELL=zsh who may want to take advantage of their shell's
advanced features)

 Documentation/git-rebase.txt  |   24 +++++++++++++++++++
 git-rebase--interactive.sh    |   20 ++++++++++++++++
 t/lib-rebase.sh               |    2 +
 t/t3404-rebase-interactive.sh |   50 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index be23ad2..4bd4b66 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -459,6 +459,30 @@ sure that the current HEAD is "B", and call
 $ git rebase -i -p --onto Q O
 -----------------------------
 
+Reordering and editing commits usually creates untested intermediate
+steps.  You may want to check that your history editing did not break
+anything by running a test, or at least recompiling at intermediate
+points in history by using the "exec" command (shortcut "x").  You may
+do so by creating a todo list like this one:
+
+-------------------------------------------
+pick deadbee Implement feature XXX
+fixup f1a5c00 Fix to feature XXX
+exec make
+pick c0ffeee The oneline of the next commit
+edit deadbab The oneline of the commit after
+exec cd subdir; make test
+...
+-------------------------------------------
+
+The interactive rebase will stop when a command fails (i.e. exits with
+non-0 status) to give you an opportunity to fix the problem. You can
+continue with `git rebase --continue`.
+
+The "exec" command launches the command in a shell (the one specified
+in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
+use usual shell commands like "cd". The command is run from the
+root of the working tree.
 
 SPLITTING COMMITS
 -----------------
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b94c2a0..33d3087 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -537,6 +537,25 @@ do_next () {
 		esac
 		record_in_rewritten $sha1
 		;;
+	x|"exec")
+		read -r command rest < "$TODO"
+		mark_action_done
+		printf 'Executing: %s\n' "$rest"
+		# "exec" command doesn't take a sha1 in the todo-list.
+		# => can't just use $sha1 here.
+		git rev-parse --verify HEAD > "$DOTEST"/stopped-sha
+		${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
+		status=$?
+		if test "$status" -ne 0
+		then
+			warn "Execution failed: $rest"
+			warn "You can fix the problem, and then run"
+			warn
+			warn "	git rebase --continue"
+			warn
+			exit "$status"
+		fi
+		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		if git rev-parse --verify -q "$sha1" >/dev/null
@@ -957,6 +976,7 @@ first and then run 'git rebase --continue' again."
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
+#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6aefe27..6ccf797 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -47,6 +47,8 @@ for line in $FAKE_LINES; do
 	case $line in
 	squash|fixup|edit|reword)
 		action="$line";;
+	exec*)
+		echo "$line" | sed 's/_/ /g' >> "$1";;
 	"#")
 		echo '# comment' >> "$1";;
 	">")
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9f03ce6..3b07850 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -64,6 +64,56 @@ test_expect_success 'setup' '
 	done
 '
 
+# debugging-friendly alternatives to "test -f" and "test ! -f"
+file_must_exist () {
+    if ! [ -f "$1" ]; then
+	echo "file $1 not created."
+	false
+    fi
+}
+
+file_must_not_exist () {
+    if [ -f "$1" ]; then
+	echo "file $1 created while it shouldn't have. $2"
+	false
+    fi
+}
+
+test_expect_success 'rebase -i with the exec command' '
+	git checkout master &&
+	FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two exec_false exec_touch_touch-three 3 4
+		exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon 5" \
+		test_must_fail git rebase -i A &&
+	file_must_exist touch-one &&
+	file_must_exist touch-two &&
+	file_must_not_exist touch-three "(Should have stopped before)" &&
+	test $(git rev-parse C) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of C)"
+		false
+	} &&
+	git rebase --continue &&
+	file_must_exist touch-three &&
+	file_must_exist "touch-file  name with spaces" &&
+	file_must_exist touch-after-semicolon &&
+	test $(git rev-parse master) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of master)"
+		false
+	} &&
+	rm -f touch-*
+'
+
+test_expect_success 'rebase -i with the exec command runs from tree root' '
+	git checkout master &&
+	mkdir subdir && cd subdir &&
+	FAKE_LINES="1 exec_touch_touch-subdir" \
+		git rebase -i HEAD^ &&
+	cd .. &&
+	file_must_exist touch-subdir &&
+	rm -fr subdir
+'
+
 test_expect_success 'no changes are a nop' '
 	git checkout branch2 &&
 	git rebase -i F &&
-- 
1.7.2.1.30.g18195

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] rebase -i: add exec command to launch a shell command
  2010-08-05 13:00 [PATCH] rebase -i: add exec command to launch a shell command Matthieu Moy
@ 2010-08-05 13:31 ` Ævar Arnfjörð Bjarmason
  2010-08-05 16:47   ` Matthieu Moy
  0 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-05 13:31 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster

On Thu, Aug 5, 2010 at 13:00, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> The typical usage pattern would be to run a test (or simply a compilation
> command) at given points in history.
>
> The shell command is ran (from the worktree root), and the rebase is
> stopped when the command fails, to give the user an opportunity to fix
> the problem before continuing with "git rebase --continue".
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>
> So, back to the "run from tree root", but that't now properly
> documented and tested.
>
> One notable difference with my first version is that the command is
> ran in a subshell, defaulting to $SHELL (typically for users like me
> with $SHELL=zsh who may want to take advantage of their shell's
> advanced features)
>
>  Documentation/git-rebase.txt  |   24 +++++++++++++++++++
>  git-rebase--interactive.sh    |   20 ++++++++++++++++
>  t/lib-rebase.sh               |    2 +
>  t/t3404-rebase-interactive.sh |   50 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 96 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index be23ad2..4bd4b66 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -459,6 +459,30 @@ sure that the current HEAD is "B", and call
>  $ git rebase -i -p --onto Q O
>  -----------------------------
>
> +Reordering and editing commits usually creates untested intermediate
> +steps.  You may want to check that your history editing did not break
> +anything by running a test, or at least recompiling at intermediate
> +points in history by using the "exec" command (shortcut "x").  You may
> +do so by creating a todo list like this one:
> +
> +-------------------------------------------
> +pick deadbee Implement feature XXX
> +fixup f1a5c00 Fix to feature XXX
> +exec make
> +pick c0ffeee The oneline of the next commit
> +edit deadbab The oneline of the commit after
> +exec cd subdir; make test
> +...
> +-------------------------------------------
> +
> +The interactive rebase will stop when a command fails (i.e. exits with
> +non-0 status) to give you an opportunity to fix the problem. You can
> +continue with `git rebase --continue`.
> +
> +The "exec" command launches the command in a shell (the one specified
> +in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
> +use usual shell commands like "cd". The command is run from the

I think that needs a definite article: ".. use the usual ..".

> +# debugging-friendly alternatives to "test -f" and "test ! -f"
> +file_must_exist () {
> +    if ! [ -f "$1" ]; then
> +       echo "file $1 not created."
> +       false
> +    fi
> +}
> +
> +file_must_not_exist () {
> +    if [ -f "$1" ]; then
> +       echo "file $1 created while it shouldn't have. $2"
> +       false
> +    fi
> +}

As I pointed out in a previous comment to the series this sort of
debug code should either be converted to use "test" or we should
incorporate it into test-lib.sh and use it everywhere.

I somewhat like the latter. It's sometimes hard to see what's going
wrong with our tests. It'd also translate to TAP subtests.

Otherwise this all looks good. Especially without the fragile mkdir/chdir
part present in a previous submission.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] rebase -i: add exec command to launch a shell command
  2010-08-05 13:31 ` Ævar Arnfjörð Bjarmason
@ 2010-08-05 16:47   ` Matthieu Moy
  2010-08-05 16:54     ` [PATCH 1/2] " Matthieu Moy
                       ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Matthieu Moy @ 2010-08-05 16:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, gitster

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> +in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
>> +use usual shell commands like "cd". The command is run from the
>
> I think that needs a definite article: ".. use the usual ..".

I don't think so, especially with a plural "commands" after.
Googlefight agrees with me ("use the usual commands" => 350 results,
"use usual commands" => 4900), but that's not a proof. Perhaps a
native speaker could help?

> As I pointed out in a previous comment to the series this sort of
> debug code should either be converted to use "test" or we should
> incorporate it into test-lib.sh and use it everywhere.
>
> I somewhat like the latter. It's sometimes hard to see what's going
> wrong with our tests. It'd also translate to TAP subtests.

I'll do both: use test -f in a first patch, and propose alternative in
the second. New patch serie follows.

I don't know TAP and TAP subtests, but if the functions exist, other
patches can be added on top to improve them.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] rebase -i: add exec command to launch a shell command
  2010-08-05 16:47   ` Matthieu Moy
@ 2010-08-05 16:54     ` Matthieu Moy
  2010-08-05 16:54     ` [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f] Matthieu Moy
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2010-08-05 16:54 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

The typical usage pattern would be to run a test (or simply a compilation
command) at given points in history.

The shell command is ran (from the worktree root), and the rebase is
stopped when the command fails, to give the user an opportunity to fix
the problem before continuing with "git rebase --continue".

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-rebase.txt  |   24 ++++++++++++++++++++++++
 git-rebase--interactive.sh    |   20 ++++++++++++++++++++
 t/lib-rebase.sh               |    2 ++
 t/t3404-rebase-interactive.sh |   35 +++++++++++++++++++++++++++++++++++
 4 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index be23ad2..4bd4b66 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -459,6 +459,30 @@ sure that the current HEAD is "B", and call
 $ git rebase -i -p --onto Q O
 -----------------------------
 
+Reordering and editing commits usually creates untested intermediate
+steps.  You may want to check that your history editing did not break
+anything by running a test, or at least recompiling at intermediate
+points in history by using the "exec" command (shortcut "x").  You may
+do so by creating a todo list like this one:
+
+-------------------------------------------
+pick deadbee Implement feature XXX
+fixup f1a5c00 Fix to feature XXX
+exec make
+pick c0ffeee The oneline of the next commit
+edit deadbab The oneline of the commit after
+exec cd subdir; make test
+...
+-------------------------------------------
+
+The interactive rebase will stop when a command fails (i.e. exits with
+non-0 status) to give you an opportunity to fix the problem. You can
+continue with `git rebase --continue`.
+
+The "exec" command launches the command in a shell (the one specified
+in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
+use usual shell commands like "cd". The command is run from the
+root of the working tree.
 
 SPLITTING COMMITS
 -----------------
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b94c2a0..33d3087 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -537,6 +537,25 @@ do_next () {
 		esac
 		record_in_rewritten $sha1
 		;;
+	x|"exec")
+		read -r command rest < "$TODO"
+		mark_action_done
+		printf 'Executing: %s\n' "$rest"
+		# "exec" command doesn't take a sha1 in the todo-list.
+		# => can't just use $sha1 here.
+		git rev-parse --verify HEAD > "$DOTEST"/stopped-sha
+		${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
+		status=$?
+		if test "$status" -ne 0
+		then
+			warn "Execution failed: $rest"
+			warn "You can fix the problem, and then run"
+			warn
+			warn "	git rebase --continue"
+			warn
+			exit "$status"
+		fi
+		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		if git rev-parse --verify -q "$sha1" >/dev/null
@@ -957,6 +976,7 @@ first and then run 'git rebase --continue' again."
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
+#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6aefe27..6ccf797 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -47,6 +47,8 @@ for line in $FAKE_LINES; do
 	case $line in
 	squash|fixup|edit|reword)
 		action="$line";;
+	exec*)
+		echo "$line" | sed 's/_/ /g' >> "$1";;
 	"#")
 		echo '# comment' >> "$1";;
 	">")
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9f03ce6..bba220a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -64,6 +64,41 @@ test_expect_success 'setup' '
 	done
 '
 
+test_expect_success 'rebase -i with the exec command' '
+	git checkout master &&
+	FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two exec_false exec_touch_touch-three 3 4
+		exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon 5" \
+		test_must_fail git rebase -i A &&
+	test -f touch-one &&
+	test -f touch-two &&
+	! test -f touch-three &&
+	test $(git rev-parse C) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of C)"
+		false
+	} &&
+	git rebase --continue &&
+	test -f touch-three &&
+	test -f "touch-file  name with spaces" &&
+	test -f touch-after-semicolon &&
+	test $(git rev-parse master) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of master)"
+		false
+	} &&
+	rm -f touch-*
+'
+
+test_expect_success 'rebase -i with the exec command runs from tree root' '
+	git checkout master &&
+	mkdir subdir && cd subdir &&
+	FAKE_LINES="1 exec_touch_touch-subdir" \
+		git rebase -i HEAD^ &&
+	cd .. &&
+	test -f touch-subdir &&
+	rm -fr subdir
+'
+
 test_expect_success 'no changes are a nop' '
 	git checkout branch2 &&
 	git rebase -i F &&
-- 
1.7.2.1.30.g18195

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f]
  2010-08-05 16:47   ` Matthieu Moy
  2010-08-05 16:54     ` [PATCH 1/2] " Matthieu Moy
@ 2010-08-05 16:54     ` Matthieu Moy
  2010-08-05 17:03       ` [PATCH v2] " Matthieu Moy
  2010-08-09 16:25       ` [PATCH 2/2] " Junio C Hamano
  2010-08-05 18:24     ` [PATCH] rebase -i: add exec command to launch a shell command Erik Faye-Lund
  2010-08-05 18:37     ` Jacob Helwig
  3 siblings, 2 replies; 15+ messages in thread
From: Matthieu Moy @ 2010-08-05 16:54 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

The helper functions are implemented, documented, and used in a few
places to validate them, but not everywhere to avoid useless code churn.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 t/README                      |    8 ++++++++
 t/t3404-rebase-interactive.sh |   18 +++++++++---------
 t/t3407-rebase-abort.sh       |    6 +++---
 t/test-lib.sh                 |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..be760b9 100644
--- a/t/README
+++ b/t/README
@@ -467,6 +467,14 @@ library for your script to use.
    <expected> file.  This behaves like "cmp" but produces more
    helpful output when the test is run with "-v" option.
 
+ - test_file_must_exist <file> [<diagnosis>]
+   test_file_must_not_exist <file> [<diagnosis>]
+   test_dir_must_exist <dir> [<diagnosis>]
+   test_dir_must_not_exist <dir> [<diagnosis>]
+
+   check whether a file/directory exists or doesn't. <diagnosis> will
+   be displayed if the test fails.
+
  - test_when_finished <script>
 
    Prepend <script> to a list of commands to run to clean up
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index bba220a..50787c2 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -69,18 +69,18 @@ test_expect_success 'rebase -i with the exec command' '
 	FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two exec_false exec_touch_touch-three 3 4
 		exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon 5" \
 		test_must_fail git rebase -i A &&
-	test -f touch-one &&
-	test -f touch-two &&
-	! test -f touch-three &&
+	test_file_must_exist touch-one &&
+	test_file_must_exist touch-two &&
+	test_file_must_not_exist touch-three "(Rebase should have stopped before)" &&
 	test $(git rev-parse C) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of C)"
 		false
 	} &&
 	git rebase --continue &&
-	test -f touch-three &&
-	test -f "touch-file  name with spaces" &&
-	test -f touch-after-semicolon &&
+	test_file_must_exist touch-three &&
+	test_file_must_exist "touch-file  name with spaces" &&
+	test_file_must_exist touch-after-semicolon &&
 	test $(git rev-parse master) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of master)"
@@ -95,7 +95,7 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	FAKE_LINES="1 exec_touch_touch-subdir" \
 		git rebase -i HEAD^ &&
 	cd .. &&
-	test -f touch-subdir &&
+	test_file_must_exist touch-subdir &&
 	rm -fr subdir
 '
 
@@ -178,7 +178,7 @@ test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
-	! test -d .git/rebase-merge
+	test_dir_must_not_exist .git/rebase-merge
 '
 
 test_expect_success 'abort with error when new base cannot be checked out' '
@@ -187,7 +187,7 @@ test_expect_success 'abort with error when new base cannot be checked out' '
 	test_must_fail git rebase -i master > output 2>&1 &&
 	grep "Untracked working tree file .file1. would be overwritten" \
 		output &&
-	! test -d .git/rebase-merge &&
+	test_dir_must_not_exist .git/rebase-merge &&
 	git reset --hard HEAD^
 '
 
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2999e78..a1615b8 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -38,7 +38,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_dir_must_not_exist "$dotest" &&
 		git rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
 		test ! -d "$dotest"
@@ -49,7 +49,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_dir_must_not_exist "$dotest" &&
 		test_must_fail git rebase --skip &&
 		test $(git rev-parse HEAD) = $(git rev-parse master) &&
 		git rebase --abort &&
@@ -62,7 +62,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_dir_must_not_exist "$dotest" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e8f21d5..3701f2d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -541,6 +541,38 @@ test_external_without_stderr () {
 	fi
 }
 
+# debugging-friendly alternatives to "test [!] [-f|-d]"
+# The commands test the existence or non-existance of $1. $2 can be
+# given to provide a more precise diagnosis.
+test_file_must_exist () {
+    if ! [ -f "$1" ]; then
+	echo "file $1 doesn't exist. $*"
+	false
+    fi
+}
+
+test_file_must_not_exist () {
+    if [ -f "$1" ]; then
+	echo "file $1 exists. $*"
+	false
+    fi
+}
+
+test_dir_must_exist () {
+    if ! [ -d "$1" ]; then
+	echo "directory $1 doesn't exist. $*"
+	false
+    fi
+}
+
+test_file_must_not_exist () {
+    if [ -d "$1" ]; then
+	echo "directory $1 exists. $*"
+	false
+    fi
+}
+
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #
-- 
1.7.2.1.30.g18195

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2] test-lib: user-friendly alternatives to test [!] [-d|-f]
  2010-08-05 16:54     ` [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f] Matthieu Moy
@ 2010-08-05 17:03       ` Matthieu Moy
  2010-08-06 22:57         ` Jonathan Nieder
  2010-08-09 16:25       ` [PATCH 2/2] " Junio C Hamano
  1 sibling, 1 reply; 15+ messages in thread
From: Matthieu Moy @ 2010-08-05 17:03 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

The helper functions are implemented, documented, and used in a few
places to validate them, but not everywhere to avoid useless code churn.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Sooooory, the first version was plain broken (the tests did not even
pass anymore), forget it. This should be better.

 t/README                      |    8 ++++++++
 t/t3404-rebase-interactive.sh |   18 +++++++++---------
 t/t3407-rebase-abort.sh       |    6 +++---
 t/test-lib.sh                 |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..be760b9 100644
--- a/t/README
+++ b/t/README
@@ -467,6 +467,14 @@ library for your script to use.
    <expected> file.  This behaves like "cmp" but produces more
    helpful output when the test is run with "-v" option.
 
+ - test_file_must_exist <file> [<diagnosis>]
+   test_file_must_not_exist <file> [<diagnosis>]
+   test_dir_must_exist <dir> [<diagnosis>]
+   test_dir_must_not_exist <dir> [<diagnosis>]
+
+   check whether a file/directory exists or doesn't. <diagnosis> will
+   be displayed if the test fails.
+
  - test_when_finished <script>
 
    Prepend <script> to a list of commands to run to clean up
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index bba220a..50787c2 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -69,18 +69,18 @@ test_expect_success 'rebase -i with the exec command' '
 	FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two exec_false exec_touch_touch-three 3 4
 		exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon 5" \
 		test_must_fail git rebase -i A &&
-	test -f touch-one &&
-	test -f touch-two &&
-	! test -f touch-three &&
+	test_file_must_exist touch-one &&
+	test_file_must_exist touch-two &&
+	test_file_must_not_exist touch-three "(Rebase should have stopped before)" &&
 	test $(git rev-parse C) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of C)"
 		false
 	} &&
 	git rebase --continue &&
-	test -f touch-three &&
-	test -f "touch-file  name with spaces" &&
-	test -f touch-after-semicolon &&
+	test_file_must_exist touch-three &&
+	test_file_must_exist "touch-file  name with spaces" &&
+	test_file_must_exist touch-after-semicolon &&
 	test $(git rev-parse master) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of master)"
@@ -95,7 +95,7 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	FAKE_LINES="1 exec_touch_touch-subdir" \
 		git rebase -i HEAD^ &&
 	cd .. &&
-	test -f touch-subdir &&
+	test_file_must_exist touch-subdir &&
 	rm -fr subdir
 '
 
@@ -178,7 +178,7 @@ test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
-	! test -d .git/rebase-merge
+	test_dir_must_not_exist .git/rebase-merge
 '
 
 test_expect_success 'abort with error when new base cannot be checked out' '
@@ -187,7 +187,7 @@ test_expect_success 'abort with error when new base cannot be checked out' '
 	test_must_fail git rebase -i master > output 2>&1 &&
 	grep "Untracked working tree file .file1. would be overwritten" \
 		output &&
-	! test -d .git/rebase-merge &&
+	test_dir_must_not_exist .git/rebase-merge &&
 	git reset --hard HEAD^
 '
 
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2999e78..0ca81fe 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -38,7 +38,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_dir_must_exist "$dotest" &&
 		git rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
 		test ! -d "$dotest"
@@ -49,7 +49,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_dir_must_exist "$dotest" &&
 		test_must_fail git rebase --skip &&
 		test $(git rev-parse HEAD) = $(git rev-parse master) &&
 		git rebase --abort &&
@@ -62,7 +62,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_dir_must_exist "$dotest" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e8f21d5..694bbe8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -541,6 +541,38 @@ test_external_without_stderr () {
 	fi
 }
 
+# debugging-friendly alternatives to "test [!] [-f|-d]"
+# The commands test the existence or non-existance of $1. $2 can be
+# given to provide a more precise diagnosis.
+test_file_must_exist () {
+	if ! [ -f "$1" ]; then
+		echo "file $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_file_must_not_exist () {
+	if [ -f "$1" ]; then
+		echo "file $1 exists. $*"
+		false
+	fi
+}
+
+test_dir_must_exist () {
+	if ! [ -d "$1" ]; then
+		echo "directory $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_dir_must_not_exist () {
+	if [ -d "$1" ]; then
+		echo "directory $1 exists. $*"
+		false
+	fi
+}
+
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #
-- 
1.7.2.1.30.g18195

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] rebase -i: add exec command to launch a shell command
  2010-08-05 16:47   ` Matthieu Moy
  2010-08-05 16:54     ` [PATCH 1/2] " Matthieu Moy
  2010-08-05 16:54     ` [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f] Matthieu Moy
@ 2010-08-05 18:24     ` Erik Faye-Lund
  2010-08-05 18:37     ` Jacob Helwig
  3 siblings, 0 replies; 15+ messages in thread
From: Erik Faye-Lund @ 2010-08-05 18:24 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Ævar Arnfjörð, git, gitster

On Thu, Aug 5, 2010 at 6:47 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>
>>> +in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
>>> +use usual shell commands like "cd". The command is run from the
>>
>> I think that needs a definite article: ".. use the usual ..".
>
> I don't think so, especially with a plural "commands" after.
> Googlefight agrees with me ("use the usual commands" => 350 results,
> "use usual commands" => 4900), but that's not a proof.

Being grammatically correct doesn't automatically make a sentence
good. "Use ususal" is a bit of a tongue-twister, so I'd rewrite that
to "use normal" for that purpose alone.

But I'm not a native English speaker, and the natives might disagree with me.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] rebase -i: add exec command to launch a shell command
  2010-08-05 16:47   ` Matthieu Moy
                       ` (2 preceding siblings ...)
  2010-08-05 18:24     ` [PATCH] rebase -i: add exec command to launch a shell command Erik Faye-Lund
@ 2010-08-05 18:37     ` Jacob Helwig
  2010-08-05 20:16       ` Junio C Hamano
  3 siblings, 1 reply; 15+ messages in thread
From: Jacob Helwig @ 2010-08-05 18:37 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Ævar Arnfjörð, git, gitster

On Thu, Aug 5, 2010 at 09:47, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>
>>> +in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
>>> +use usual shell commands like "cd". The command is run from the
>>
>> I think that needs a definite article: ".. use the usual ..".
>
> I don't think so, especially with a plural "commands" after.
> Googlefight agrees with me ("use the usual commands" => 350 results,
> "use usual commands" => 4900), but that's not a proof. Perhaps a
> native speaker could help?
>

You could probably just drop "usual" entirely: ..., so you can use
shell commands like "cd".

I'm not sure that "usual" really adds anything there, and might
actually be confusing.  Does it mean that "unusual" ones won't work?
What are "unusual" ones?

Possibly make 'like "cd"', parenthetical to further show that it's an
example, and not saying that only commands along the lines of "cd"
will work?  ..., so you can use shell commands (for example: cd).

-Jacob

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] rebase -i: add exec command to launch a shell command
  2010-08-05 18:37     ` Jacob Helwig
@ 2010-08-05 20:16       ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2010-08-05 20:16 UTC (permalink / raw)
  To: Jacob Helwig; +Cc: Matthieu Moy, Ævar Arnfjörð, git

Jacob Helwig <jacob.helwig@gmail.com> writes:

> On Thu, Aug 5, 2010 at 09:47, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>>
>>>> +in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
>>>> +use usual shell commands like "cd". The command is run from the
>>>
>>> I think that needs a definite article: ".. use the usual ..".
>> ...
> You could probably just drop "usual" entirely: ..., so you can use
> shell commands like "cd".

Sounds sane.  Will do, unless a native speaker stops me from doing so.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] test-lib: user-friendly alternatives to test [!] [-d|-f]
  2010-08-05 17:03       ` [PATCH v2] " Matthieu Moy
@ 2010-08-06 22:57         ` Jonathan Nieder
  2010-08-07  0:21           ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Nieder @ 2010-08-06 22:57 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster

Matthieu Moy wrote:

> The helper functions are implemented, documented, and used in a few
> places to validate them

When I first read this, I thought you were saying these helpers
already existed.  This is where the rationale goes, anyway, so maybe:

	Add new test_file_must_not_exist et al helpers for
	use by tests to more loudly diagnose failures that
	manifest themselves by the existence or nonexistence
	of a file or directory.

	So now you can use

		test_file_must_exist foo "so there"

	from your test, and when it fails due to foo being
	absent or being a symlink instead, instead of silence
	you will get (if debugging with "-v") the helpful message

		file foo does not exist. so there.

> +++ b/t/README
> @@ -467,6 +467,14 @@ library for your script to use.
>     <expected> file.  This behaves like "cmp" but produces more
>     helpful output when the test is run with "-v" option.
>  
> + - test_file_must_exist <file> [<diagnosis>]
> +   test_file_must_not_exist <file> [<diagnosis>]
> +   test_dir_must_exist <dir> [<diagnosis>]
> +   test_dir_must_not_exist <dir> [<diagnosis>]
> +
> +   check whether a file/directory exists or doesn't. <diagnosis> will
> +   be displayed if the test fails.

Maybe:

	- test_file_exists <name> [<diagnosis>]
	- test_dir_exists <name> [<diagnosis>]

	  Check that <name> exists and is a file or directory,
	  printing a diagnostic if it does not.  The <diagnosis>
	  if present will be used to give some added context to
	  the diagnostic.

	- test_does_not_exist <name> [<diagnosis>]

	  Check that <name> does not exist, printing a
	  diagnostic if it does.  The <diagnosis> will be
	  printed on failure as added context if present.

I think the ..._must_exist names put the emphasis in the
wrong place, and they look funny in "if" statements.

> +++ b/t/t3404-rebase-interactive.sh
> +++ b/t/t3407-rebase-abort.sh
[examples]

Makes sense.

> +++ b/t/test-lib.sh
> @@ -541,6 +541,38 @@ test_external_without_stderr () {
>  	fi
>  }
>  
> +# debugging-friendly alternatives to "test [!] [-f|-d]"
> +# The commands test the existence or non-existance of $1. $2 can be
> +# given to provide a more precise diagnosis.
> +test_file_must_exist () {
> +	if ! [ -f "$1" ]; then
> +		echo "file $1 doesn't exist. $*"
> +		false
> +	fi
> +}

Style nitpick: if statementss in the test-lib have tended to look like

 if [ foo ]
 then
	bar
 fi

so far.  Here the whole function is a glorified "test -f", so I wonder
if

	[ -f "$1" ] ||
	{
		echo >&2 "file $1 doesn't exist. $*"
		false
	}

would not be clearer.  I dunno.

> +test_file_must_not_exist () {
> +	if [ -f "$1" ]; then
> +		echo "file $1 exists. $*"
> +		false
> +	fi
> +}

What should happen if $1 exists and is not a file?

I have often run into silent test failures of the sort your patch
is designed to avoid.  Thanks for tackling it.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] test-lib: user-friendly alternatives to test [!]  [-d|-f]
  2010-08-06 22:57         ` Jonathan Nieder
@ 2010-08-07  0:21           ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-08-07  0:21 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Matthieu Moy, git, gitster

On Fri, Aug 6, 2010 at 22:57, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Matthieu Moy wrote:
>        - test_file_exists <name> [<diagnosis>]
>        - test_dir_exists <name> [<diagnosis>]
>
>          Check that <name> exists and is a file or directory,
>          printing a diagnostic if it does not.  The <diagnosis>
>          if present will be used to give some added context to
>          the diagnostic.
>
>        - test_does_not_exist <name> [<diagnosis>]
>
>          Check that <name> does not exist, printing a
>          diagnostic if it does.  The <diagnosis> will be
>          printed on failure as added context if present.
>
> I think the ..._must_exist names put the emphasis in the
> wrong place, and they look funny in "if" statements.

Personally I'd prefer something where I can just think in the "test"
bultin terms and still get debug info, something like (pseduocode)

    gittest() {
        test "$#" = 3 && { bool='!'; shift; } || bool=
        test=$1; shift
        args="$@"; shift
        case "$test" in
        -f)
            # Handle the common case of -f with a custom message
        ;;
        -d)
            # Same for -d
        ;;
        *)
            # Just pass a switch to test and say "test with the -X
switch", or something
esac
    }

    gittest ! -f ~/.gitconfig
    gittest -f ~/.gitconfig
    gittest -d /tmp
    gittest ! -d /tmp
    gittest -s /tmp

I'll never be able to fit more test_* functions in my brain, and I
wrote the docs :)

> Style nitpick: if statementss in the test-lib have tended to look like
>
>  if [ foo ]
>  then
>        bar
>  fi
>
> so far.  Here the whole function is a glorified "test -f", so I wonder
> if
>
>        [ -f "$1" ] ||
>        {
>                echo >&2 "file $1 doesn't exist. $*"
>                false
>        }
>
> would not be clearer.  I dunno.

This is the style we usually use:

    if ! test -f "$1"
    then
        echo >&2 "file $1 doesn't exist. $*"
        false
    fi

> I have often run into silent test failures of the sort your patch
> is designed to avoid.  Thanks for tackling it.

Yeah, having more intra-test progress is definitely good. Right now I
just remove things from the tests in an ad-hoc fashion until they
start passing if they fail when I debug them.

I mentioned that we could emit these test progress reports as TAP in a
previous E-Mail. Here's how that could look like:

    $ perl -MTest::More=no_plan -E '
        subtest "A git test" => sub {
            pass("doing test -f file");
            pass("git commit ...");
            pass("test_tick...");
            done_testing();
        } for 1 .. 2
    '
        ok 1 - doing test -f file
        ok 2 - git commit ...
        ok 3 - test_tick...
        1..3
    ok 1 - A git test
        ok 1 - doing test -f file
        ok 2 - git commit ...
        ok 3 - test_tick...
        1..3
    ok 2 - A git test
    1..2

I.e. we could make these intra-test progress reports machine readable.

As the example shows the obvious next step would be to make other
utility functions like test_commit() emit a progress status as well.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f]
  2010-08-05 16:54     ` [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f] Matthieu Moy
  2010-08-05 17:03       ` [PATCH v2] " Matthieu Moy
@ 2010-08-09 16:25       ` Junio C Hamano
  2010-08-10 11:00         ` [PATCH] test-lib: user-friendly alternatives to test [-d|-f|-e] Matthieu Moy
  1 sibling, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2010-08-09 16:25 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> +   test_file_must_not_exist <file> [<diagnosis>]
> +   test_dir_must_not_exist <dir> [<diagnosis>]

Should either of these pass?

    mkdir foo && test_file_must_not_exist foo
    rm -fr foo && >foo && test_dir_must_not_exist foo

I think in most of the test cases we want to write "must not exist" to
make sure what we are supposed to remove is gone, which would mean that
(1) we know what that thing is, and (2) we not only just do not want a
file "foo" when we say "file-must-not-exist foo", but we don't expect it
to be a directory either.

I'd say we would probably want three primitives instead of four:

    test_path_is_file        <path>
    test_path_is_directory   <path>
    test_path_is_missing     <path>

Thanks.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] test-lib: user-friendly alternatives to test [-d|-f|-e]
  2010-08-09 16:25       ` [PATCH 2/2] " Junio C Hamano
@ 2010-08-10 11:00         ` Matthieu Moy
  2010-08-10 11:11           ` Joshua Juran
  0 siblings, 1 reply; 15+ messages in thread
From: Matthieu Moy @ 2010-08-10 11:00 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

The helper functions are implemented, documented, and used in a few
places to validate them, but not everywhere to avoid useless code churn.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> I'd say we would probably want three primitives instead of four:
> 
>     test_path_is_file        <path>
>     test_path_is_directory   <path>
>     test_path_is_missing     <path>

I buy this. Thanks,

 t/README                      |    7 +++++++
 t/t3404-rebase-interactive.sh |   18 +++++++++---------
 t/t3407-rebase-abort.sh       |    6 +++---
 t/test-lib.sh                 |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..410499a 100644
--- a/t/README
+++ b/t/README
@@ -467,6 +467,13 @@ library for your script to use.
    <expected> file.  This behaves like "cmp" but produces more
    helpful output when the test is run with "-v" option.
 
+ - test_path_is_file <file> [<diagnosis>]
+   test_path_is_dir <dir> [<diagnosis>]
+   test_path_is_missing <path> [<diagnosis>]
+
+   Check whether a file/directory exists or doesn't. <diagnosis> will
+   be displayed if the test fails.
+
  - test_when_finished <script>
 
    Prepend <script> to a list of commands to run to clean up
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7b0026e..f78c364 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -69,18 +69,18 @@ test_expect_success 'rebase -i with the exec command' '
 	FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two exec_false exec_touch_touch-three 3 4
 		exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon 5" \
 		test_must_fail git rebase -i A &&
-	test -f touch-one &&
-	test -f touch-two &&
-	! test -f touch-three &&
+	test_path_is_file touch-one &&
+	test_path_is_file touch-two &&
+	test_path_is_missing touch-three "(Rebase should have stopped before)" &&
 	test $(git rev-parse C) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of C)"
 		false
 	} &&
 	git rebase --continue &&
-	test -f touch-three &&
-	test -f "touch-file  name with spaces" &&
-	test -f touch-after-semicolon &&
+	test_path_is_file touch-three &&
+	test_path_is_file "touch-file  name with spaces" &&
+	test_path_is_file touch-after-semicolon &&
 	test $(git rev-parse master) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of master)"
@@ -95,7 +95,7 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	FAKE_LINES="1 exec_touch_touch-subdir" \
 		git rebase -i HEAD^ &&
 	cd .. &&
-	test -f touch-subdir &&
+	test_path_is_file touch-subdir &&
 	rm -fr subdir
 '
 
@@ -191,7 +191,7 @@ test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
-	! test -d .git/rebase-merge
+	test_path_is_missing .git/rebase-merge
 '
 
 test_expect_success 'abort with error when new base cannot be checked out' '
@@ -200,7 +200,7 @@ test_expect_success 'abort with error when new base cannot be checked out' '
 	test_must_fail git rebase -i master > output 2>&1 &&
 	grep "Untracked working tree file .file1. would be overwritten" \
 		output &&
-	! test -d .git/rebase-merge &&
+	test_path_is_missing .git/rebase-merge &&
 	git reset --hard HEAD^
 '
 
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2999e78..fbb3f2e 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -38,7 +38,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		git rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
 		test ! -d "$dotest"
@@ -49,7 +49,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		test_must_fail git rebase --skip &&
 		test $(git rev-parse HEAD) = $(git rev-parse master) &&
 		git rebase --abort &&
@@ -62,7 +62,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e8f21d5..a2173dd 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -541,6 +541,38 @@ test_external_without_stderr () {
 	fi
 }
 
+# debugging-friendly alternatives to "test [-f|-d|-e]"
+# The commands test the existence or non-existance of $1. $2 can be
+# given to provide a more precise diagnosis.
+test_path_is_file () {
+	if ! [ -f "$1" ]
+	then
+		echo "File $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_path_is_dir () {
+	if ! [ -d "$1" ]
+	then
+		echo "Directory $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_path_is_missing () {
+	if [ -e "$1" ]
+	then
+		echo "Path exists:"
+		ls -ld "$1"
+		if [ $# -ge 1 ]; then
+			echo "$*"
+		fi
+		false
+	fi
+}
+
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #
-- 
1.7.2.1.52.g95e25.dirty

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] test-lib: user-friendly alternatives to test [-d|-f|-e]
  2010-08-10 11:00         ` [PATCH] test-lib: user-friendly alternatives to test [-d|-f|-e] Matthieu Moy
@ 2010-08-10 11:11           ` Joshua Juran
  2010-08-10 11:18             ` [PATCH v3] " Matthieu Moy
  0 siblings, 1 reply; 15+ messages in thread
From: Joshua Juran @ 2010-08-10 11:11 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster

On Aug 10, 2010, at 4:00 AM, Matthieu Moy wrote:

> +# debugging-friendly alternatives to "test [-f|-d|-e]"
> +# The commands test the existence or non-existance of $1. $2 can be

s/existance/existence/

Josh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] test-lib: user-friendly alternatives to test [-d|-f|-e]
  2010-08-10 11:11           ` Joshua Juran
@ 2010-08-10 11:18             ` Matthieu Moy
  0 siblings, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2010-08-10 11:18 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

The helper functions are implemented, documented, and used in a few
places to validate them, but not everywhere to avoid useless code churn.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
With Junio's comment and a typo noticed by Joshua Juran.

 t/README                      |    7 +++++++
 t/t3404-rebase-interactive.sh |   18 +++++++++---------
 t/t3407-rebase-abort.sh       |    6 +++---
 t/test-lib.sh                 |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..410499a 100644
--- a/t/README
+++ b/t/README
@@ -467,6 +467,13 @@ library for your script to use.
    <expected> file.  This behaves like "cmp" but produces more
    helpful output when the test is run with "-v" option.
 
+ - test_path_is_file <file> [<diagnosis>]
+   test_path_is_dir <dir> [<diagnosis>]
+   test_path_is_missing <path> [<diagnosis>]
+
+   Check whether a file/directory exists or doesn't. <diagnosis> will
+   be displayed if the test fails.
+
  - test_when_finished <script>
 
    Prepend <script> to a list of commands to run to clean up
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7b0026e..f78c364 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -69,18 +69,18 @@ test_expect_success 'rebase -i with the exec command' '
 	FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two exec_false exec_touch_touch-three 3 4
 		exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon 5" \
 		test_must_fail git rebase -i A &&
-	test -f touch-one &&
-	test -f touch-two &&
-	! test -f touch-three &&
+	test_path_is_file touch-one &&
+	test_path_is_file touch-two &&
+	test_path_is_missing touch-three "(Rebase should have stopped before)" &&
 	test $(git rev-parse C) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of C)"
 		false
 	} &&
 	git rebase --continue &&
-	test -f touch-three &&
-	test -f "touch-file  name with spaces" &&
-	test -f touch-after-semicolon &&
+	test_path_is_file touch-three &&
+	test_path_is_file "touch-file  name with spaces" &&
+	test_path_is_file touch-after-semicolon &&
 	test $(git rev-parse master) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of master)"
@@ -95,7 +95,7 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	FAKE_LINES="1 exec_touch_touch-subdir" \
 		git rebase -i HEAD^ &&
 	cd .. &&
-	test -f touch-subdir &&
+	test_path_is_file touch-subdir &&
 	rm -fr subdir
 '
 
@@ -191,7 +191,7 @@ test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
-	! test -d .git/rebase-merge
+	test_path_is_missing .git/rebase-merge
 '
 
 test_expect_success 'abort with error when new base cannot be checked out' '
@@ -200,7 +200,7 @@ test_expect_success 'abort with error when new base cannot be checked out' '
 	test_must_fail git rebase -i master > output 2>&1 &&
 	grep "Untracked working tree file .file1. would be overwritten" \
 		output &&
-	! test -d .git/rebase-merge &&
+	test_path_is_missing .git/rebase-merge &&
 	git reset --hard HEAD^
 '
 
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2999e78..fbb3f2e 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -38,7 +38,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		git rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
 		test ! -d "$dotest"
@@ -49,7 +49,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		test_must_fail git rebase --skip &&
 		test $(git rev-parse HEAD) = $(git rev-parse master) &&
 		git rebase --abort &&
@@ -62,7 +62,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e8f21d5..d584194 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -541,6 +541,38 @@ test_external_without_stderr () {
 	fi
 }
 
+# debugging-friendly alternatives to "test [-f|-d|-e]"
+# The commands test the existence or non-existence of $1. $2 can be
+# given to provide a more precise diagnosis.
+test_path_is_file () {
+	if ! [ -f "$1" ]
+	then
+		echo "File $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_path_is_dir () {
+	if ! [ -d "$1" ]
+	then
+		echo "Directory $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_path_is_missing () {
+	if [ -e "$1" ]
+	then
+		echo "Path exists:"
+		ls -ld "$1"
+		if [ $# -ge 1 ]; then
+			echo "$*"
+		fi
+		false
+	fi
+}
+
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #
-- 
1.7.2.1.52.g95e25.dirty

^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2010-08-10 11:23 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-05 13:00 [PATCH] rebase -i: add exec command to launch a shell command Matthieu Moy
2010-08-05 13:31 ` Ævar Arnfjörð Bjarmason
2010-08-05 16:47   ` Matthieu Moy
2010-08-05 16:54     ` [PATCH 1/2] " Matthieu Moy
2010-08-05 16:54     ` [PATCH 2/2] test-lib: user-friendly alternatives to test [!] [-d|-f] Matthieu Moy
2010-08-05 17:03       ` [PATCH v2] " Matthieu Moy
2010-08-06 22:57         ` Jonathan Nieder
2010-08-07  0:21           ` Ævar Arnfjörð Bjarmason
2010-08-09 16:25       ` [PATCH 2/2] " Junio C Hamano
2010-08-10 11:00         ` [PATCH] test-lib: user-friendly alternatives to test [-d|-f|-e] Matthieu Moy
2010-08-10 11:11           ` Joshua Juran
2010-08-10 11:18             ` [PATCH v3] " Matthieu Moy
2010-08-05 18:24     ` [PATCH] rebase -i: add exec command to launch a shell command Erik Faye-Lund
2010-08-05 18:37     ` Jacob Helwig
2010-08-05 20:16       ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.