threads / discuss / 24691

Black smoke from git rebase -i exec

Subject: Black smoke from git rebase -i exec

## tl;dr

38 messages between Aug 10, 2010 and Jan 26, 2011.

replies: 37people: 11as markdown or json

Ævar Arnfjörð Bjarmason· Aug 10, 2010, 13:08 UTC · lore

There's some black smoke in pu after the git rebase -i series was applied: http://smoke.git.nix.is/app/projects/report_details/14

Note: just the t3404-rebase-interactive.sh failure, not
t6040-tracking-info.sh, that's something else.

Here's the --verbose output from the test, hopefully that helps, if not I can supply some additional info:

    Initialized empty Git repository in
/tmp/build-and-install-git-olpK/t/trash
directory.t3404-rebase-interactive/.git/
    expecting success:
    	test_commit A file1 &&
    	test_commit B file1 &&
    	test_commit C file2 &&
    	test_commit D file1 &&
    	test_commit E file3 &&
    	git checkout -b branch1 A &&
    	test_commit F file4 &&
    	test_commit G file1 &&
    	test_commit H file5 &&
    	git checkout -b branch2 F &&
    	test_commit I file6
    	git checkout -b conflict-branch A &&
    	for n in one two three four
    	do
    		test_commit $n conflict
    	done &&
    	git checkout -b no-conflict-branch A &&
    	for n in J K L M
    	do
    		test_commit $n file$n
    	done &&
    	git checkout -b no-ff-branch A &&
    	for n in N O P
    	do
    		test_commit $n file$n
    	done
    [master (root-commit) 6e62bf8] A
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file1
    [master 313fe96] B
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [master d0f65f2] C
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file2
    [master 0547e3f] D
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [master 8f99a4f] E
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file3
    Switched to a new branch 'branch1'
    [branch1 cfefd94] F
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file4
    [branch1 83751a6] G
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [branch1 4373208] H
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file5
    Switched to a new branch 'branch2'
    [branch2 615be62] I
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file6
    Switched to a new branch 'conflict-branch'
    [conflict-branch b895952] one
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 conflict
    [conflict-branch 766a798] two
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [conflict-branch 1eadf03] three
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [conflict-branch f91a2b3] four
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    Switched to a new branch 'no-conflict-branch'
    [no-conflict-branch 808874f] J
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileJ
    [no-conflict-branch 265b89e] K
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileK
    [no-conflict-branch 6b0f5e6] L
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileL
    [no-conflict-branch 3389558] M
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileM
    Switched to a new branch 'no-ff-branch'
    [no-ff-branch 53b4423] N
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileN
    [no-ff-branch cc47714] O
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileO
    [no-ff-branch faef1a5] P
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileP
    ok 1 - setup
    expecting success:
    	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-*
    Switched to a new branch 'master'
    Rebasing (4/4)
Successfully rebased and updated refs/heads/master.
    Stopped at wrong revision:
    (E instead of C)
    Stopped at wrong revision:
    (E instead of master)
    not ok - 2 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-*
    #	
    expecting success:
    	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
    Already on 'master'
    rebase -i script before editing:
    pick 8f99a4f E
    rebase -i script after editing:
    pick 8f99a4f E
    exec touch touch-subdir
    Rebasing (2/2)
Executing: touch touch-subdir
    Successfully rebased and updated refs/heads/master.
    ok 3 - rebase -i with the exec command runs from tree root
    expecting success:
    	git checkout master &&
    	FAKE_LINES="exec_echo_foo_>file1 1" \
    		test_must_fail git rebase -i HEAD^ &&
    	test $(git rev-parse master^) = $(git rev-parse HEAD) || {
    		echo "Stopped at wrong revision:"
    		echo "($(git describe --tags HEAD) instead of master^)"
    		false
    	} &&
    	git reset --hard &&
    	git rebase --continue
    Already on 'master'
    Rebasing (1/1)
Successfully rebased and updated refs/heads/master.
    Stopped at wrong revision:
    (E instead of master^)
    not ok - 4 rebase -i with the exec command checks tree cleanness
    #	
    #		git checkout master &&
    #		FAKE_LINES="exec_echo_foo_>file1 1" \
    #			test_must_fail git rebase -i HEAD^ &&
    #		test $(git rev-parse master^) = $(git rev-parse HEAD) || {
    #			echo "Stopped at wrong revision:"
    #			echo "($(git describe --tags HEAD) instead of master^)"
    #			false
    #		} &&
    #		git reset --hard &&
    #		git rebase --continue
    #	
    # failed 2 among 4 test(s)
    1..4
(I modified the test to only run the failing tests)
Matthieu Moy· Aug 10, 2010, 13:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Black smoke from git rebase -i exec

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> There's some black smoke in pu after the git rebase -i series was
> applied: http://smoke.git.nix.is/app/projects/report_details/14

Strange, I can't reproduce this on my box (tried on RHEL x86_64 and Debian i686).

>     expecting success:
...
>     	rm -f touch-*
>
>     Switched to a new branch 'master'
At this point, I get 

rebase -i script before editing: pick 313fe96 B pick d0f65f2 C pick 0547e3f D pick 8f99a4f E

rebase -i script after editing: pick 313fe96 B exec touch touch-one pick d0f65f2 C exec touch touch-two exec false exec touch touch-three pick 0547e3f D pick 8f99a4f E exec touch "touch-file name with spaces"; touch touch-after-semicolon

which you don't seem to get on your side. I get the same as you if I comment out the "set_fake_editor" line at the top of the script. So, I suspect there's something very wrong that prevents it from doing its job.

Can you add some debug

echo "$EDITOR" echo "$FAKE_EDITOR"

somewhere in the test to see what happens?
Show 6 quoted lines
>     Rebasing (4/4)
> Successfully rebased and updated refs/heads/master.
>     Stopped at wrong revision:
>     (E instead of C)
>     Stopped at wrong revision:
>     (E instead of master)
(here, it's definitely doing as if the todolist had not been edited)
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Ævar Arnfjörð Bjarmason· Aug 10, 2010, 13:57 UTC · re: Matthieu Moy · lore

Re: Black smoke from git rebase -i exec

On Tue, Aug 10, 2010 at 13:37, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 7 quoted lines
> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>
>> There's some black smoke in pu after the git rebase -i series was
>> applied: http://smoke.git.nix.is/app/projects/report_details/14
>
> Strange, I can't reproduce this on my box (tried on RHEL x86_64 and
> Debian i686).

Hi. The issue appears to be that there's some non-POSIX code in your patch (but I didn't check what). The test works for me with bash, but fails with dash (which is the Debian testing /bin/sh).

Can you try with dash or some other non-bash POSIX shell and see if it fails?

Johannes Sixt· Aug 10, 2010, 14:12 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Black smoke from git rebase -i exec

Am 8/10/2010 15:57, schrieb Ævar Arnfjörð Bjarmason:
Show 16 quoted lines
> On Tue, Aug 10, 2010 at 13:37, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>>
>>> There's some black smoke in pu after the git rebase -i series was
>>> applied: http://smoke.git.nix.is/app/projects/report_details/14
>>
>> Strange, I can't reproduce this on my box (tried on RHEL x86_64 and
>> Debian i686).
> 
> Hi. The issue appears to be that there's some non-POSIX code in your
> patch (but I didn't check what). The test works for me with bash, but
> fails with dash (which is the Debian testing /bin/sh).
> 
> Can you try with dash or some other non-bash POSIX shell and see if it
> fails?
The culprit is commands like these:
	FAKE_LINES="exec_echo_foo_>file1 1" \
		test_must_fail git rebase -i HEAD^ &&

You cannot apply single-command-export if the command is a shell function. You must rewrite this as:

	(
		export FAKE_LINES="..." &&
		test_must_fail git rebase ....
	) &&
-- Hannes
Ævar Arnfjörð Bjarmason· Aug 10, 2010, 14:16 UTC · re: Johannes Sixt · lore

Re: Black smoke from git rebase -i exec

On Tue, Aug 10, 2010 at 14:12, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 7 quoted lines
> You cannot apply single-command-export if the command is a shell function.
> You must rewrite this as:
>
>        (
>                export FAKE_LINES="..." &&
>                test_must_fail git rebase ....
>        ) &&
Except that's not portable either, it should be:
    FAKE_LINES="..." &&
    export FAKE_LINES &&
	test_must_fail git rebase ...
See the other examples in t3404-rebase-interactive.sh
Matthieu Moy· Aug 10, 2010, 15:05 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Black smoke from git rebase -i exec

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 16 quoted lines
> On Tue, Aug 10, 2010 at 14:12, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> You cannot apply single-command-export if the command is a shell function.
>> You must rewrite this as:
>>
>>        (
>>                export FAKE_LINES="..." &&
>>                test_must_fail git rebase ....
>>        ) &&
>
> Except that's not portable either, it should be:
>
>     FAKE_LINES="..." &&
>     export FAKE_LINES &&
> 	test_must_fail git rebase ...
>
> See the other examples in t3404-rebase-interactive.sh
Yes, I had found this. New patch comming soon.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Aug 10, 2010, 15:17 UTC · re: Matthieu Moy · lore

[PATCH 1/2 (fix broken test)] rebase -i: add exec command to launch a shell command

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

This needs a little rework of skip_unnecessary_picks, which wasn't robust enough to deal with lines like

  exec >"file    name with many spaces"

in the todolist. The new version extracts command, sha1 and rest from each line, but outputs the line itself verbatim to avoid changing the whitespace layout.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This fixes the non-POSIX behavior of the tests found by Ævar Arnfjörð
Bjarmason (FAKE_LINES=foo test_must_fail ... does not work).

Also, I replaced "touch foo" with ">foo" and found a small bug. This is the skip_unnecessary_picks of the commit message and of the patch below.

 Documentation/git-rebase.txt  |   24 ++++++++++++++++
 git-rebase--interactive.sh    |   38 +++++++++++++++++++++++--
 t/lib-rebase.sh               |    2 +
 t/t3404-rebase-interactive.sh |   61 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 122 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index be23ad2..9c68b66 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 shell features (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..bf49b5b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -537,6 +537,34 @@ 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
+		# Run in subshell because require_clean_work_tree can die.
+		if ! (require_clean_work_tree)
+		then
+			warn "Commit or stash your changes, and then run"
+			warn
+			warn "	git rebase --continue"
+			warn
+			exit 1
+		fi
+		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		if git rev-parse --verify -q "$sha1" >/dev/null
@@ -591,10 +619,13 @@ do_rest () {
 # skip picking commits whose parents are unchanged
 skip_unnecessary_picks () {
 	fd=3
-	while read -r command sha1 rest
+	while read -r line
 	do
+		command=$(echo "$line" | sed 's/  */ /' | cut -d ' ' -f 1)
+		sha1=$(echo "$line"    | sed 's/  */ /' | cut -d ' ' -f 2)
+		rest=$(echo "$line"    | sed 's/  */ /' | cut -d ' ' -f 3-)
 		# fd=3 means we skip the command
-		case "$fd,$command,$(git rev-parse --verify --quiet $sha1^)" in
+		case "$fd,$command,$(git rev-parse --verify --quiet "$sha1"^)" in
 		3,pick,"$ONTO"*|3,p,"$ONTO"*)
 			# pick a commit whose parent is current $ONTO -> skip
 			ONTO=$sha1
@@ -606,7 +637,7 @@ skip_unnecessary_picks () {
 			fd=1
 			;;
 		esac
-		printf '%s\n' "$command${sha1:+ }$sha1${rest:+ }$rest" >&$fd
+		echo "$line" >&$fd
 	done <"$TODO" >"$TODO.new" 3>>"$DONE" &&
 	mv -f "$TODO".new "$TODO" &&
 	case "$(peek_next_command)" in
@@ -957,6 +988,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..93b181e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -64,6 +64,67 @@ test_expect_success 'setup' '
 	done
 '
 
+# "exec" commands are ran with the user shell by default, but this may
+# be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
+# to create a file. Unseting SHELL avoids such non-portable behavior
+# in tests.
+SHELL=
+
+test_expect_success 'rebase -i with the exec command' '
+	git checkout master &&
+	(
+	FAKE_LINES="1 exec_>touch-one
+		2 exec_>touch-two exec_false exec_>touch-three
+		3 4 exec_>\"touch-file__name_with_spaces\";_>touch-after-semicolon 5" &&
+	export FAKE_LINES &&
+	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-subdir" \
+		git rebase -i HEAD^ &&
+	cd .. &&
+	test -f touch-subdir &&
+	rm -fr subdir
+'
+
+test_expect_success 'rebase -i with the exec command checks tree cleanness' '
+	git checkout master &&
+	(
+	FAKE_LINES="exec_echo_foo_>file1 1" &&
+	export FAKE_LINES &&
+	test_must_fail git rebase -i HEAD^
+	) &&
+	test $(git rev-parse master^) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of master^)"
+		false
+	} &&
+	git reset --hard &&
+	git rebase --continue
+'
+
 test_expect_success 'no changes are a nop' '
 	git checkout branch2 &&
 	git rebase -i F &&
-- 
1.7.2.1.52.g95e25.dirty
Junio C Hamano· Aug 11, 2010, 18:31 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/2 (fix broken test)] rebase -i: add exec command to launch a shell command

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 15 quoted lines
> 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".
>
> This needs a little rework of skip_unnecessary_picks, which wasn't robust
> enough to deal with lines like
>
>   exec >"file    name with many spaces"
>
> in the todolist. The new version extracts command, sha1 and rest from
> each line, but outputs the line itself verbatim to avoid changing the
> whitespace layout.
Thanks.
Show 13 quoted lines
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 9f03ce6..93b181e 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -64,6 +64,67 @@ test_expect_success 'setup' '
>  	done
>  '
>  
> +# "exec" commands are ran with the user shell by default, but this may
> +# be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
> +# to create a file. Unseting SHELL avoids such non-portable behavior
> +# in tests.
> +SHELL=
Tricky but true.

Do we have other callouts that we use $SHELL from the environment? execv_shell_cmd() just runs "sh -c" from $PATH so diff (when running external diff) nor ll-merge (when running external merge driver) that call it via run_command_v_opt(RUN_USING_SHELL) are immune to this issue.

Matthieu Moy· Aug 12, 2010, 07:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2 (fix broken test)] rebase -i: add exec command to launch a shell command

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> +# "exec" commands are ran with the user shell by default, but this may
>> +# be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
>> +# to create a file. Unseting SHELL avoids such non-portable behavior
>> +# in tests.
>> +SHELL=
>
> Tricky but true.
>
> Do we have other callouts that we use $SHELL from the environment?

Not as far as I know. "git grep SHELL" show mostly "SHELL_PATH", and the testsuite passes for me with SHELL=zsh.

This exec command is a bit of a special case: I wanted the user to keep the advanced features of his shell (for example, the ** wildcard of zsh and recent bash), not just allow executing commands.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jonathan Nieder· Jan 16, 2011, 01:59 UTC · re: Matthieu Moy · lore

[PATCH 0/2] rebase -i: in-editor documentation nits

Matthieu Moy wrote:
Show 9 quoted lines
> +++ b/git-rebase--interactive.sh
> @@ -957,6 +988,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.
Nit: the "exec" command is formatted differently from the commands
around it, making it stand out (which I don't think is intended).

While we're there, patch 2 adds some brief documentation for the "noop" command.

Roughly based on [1] (which might be a nice patch to revive, by the way). Sane?

Jonathan Nieder (2):
  rebase -i: reword in-editor documentation of "exec"
  rebase -i: explain how to discard all commits
 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
[1] http://thread.gmane.org/gmane.comp.version-control.git/161120/focus=162079
Jonathan Nieder· Jan 16, 2011, 02:01 UTC · re: Jonathan Nieder · lore

[PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

The argument to the "exec" insn represents a command to be passed to the user's shell. (At first I misread the description as meaning it should itself be the name of a shell.)

While fixing that, format the description to more closely parallel the descriptions of other commands.

Before:
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but [...]
 #  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.
After:
 [...]
 #  f, fixup = like "squash", but [...]
 #  x, exec = run command using shell, and stop if it fails
 #
 # If you remove a line [...]
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
It would be nice to say "stop for amending if it fails" (or similar)
to make the relationship to the edit insn clearer, but it is not clear
how to make room for that.
 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a5ffd9a..09aeecf 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1021,7 +1021,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
+#  x, exec = run command using shell, 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.
-- 
1.7.4.rc2
Matthieu Moy· Jan 16, 2011, 10:27 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

Jonathan Nieder <jrnieder@gmail.com> writes:
> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
> +#  x, exec = run command using shell, and stop if it fails

I don't think this is a good change to remove the <cmd> part. All other commands are used with

<command> <sha1> <subject line>

and I don't think the user would be able to guess that exec is different without a hint.

If the problem is the wording of the sentence that may imply that <cmd> should be the shell itself, then why not

x <cmd>, exec <cmd> = run command <cmd> using shell, and stop if it fails
?
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jan 18, 2011, 15:05 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 12 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>> +#  x, exec = run command using shell, and stop if it fails
>
> I don't think this is a good change to remove the <cmd> part. All
> other commands are used with
>
> <command> <sha1> <subject line>
>
> and I don't think the user would be able to guess that exec is
> different without a hint.
I tend to agree with you here.
Jonathan Nieder· Jan 20, 2011, 20:09 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
>> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>> +#  x, exec = run command using shell, and stop if it fails
>
> I don't think this is a good change to remove the <cmd> part. All
> other commands are used with
>
> <command> <sha1> <subject line>
>
> and I don't think the user would be able to guess that exec is
> different without a hint.
> 
> If the problem is the wording of the sentence that may imply that
> <cmd> should be the shell itself, then why not

Yes, sorry, I combined two problems into a single patch. That was a mistake. The current cheat sheet says:

# Rebase 3f14246..a1d7e01 onto 3f14246 # # Commands: # p, pick = use commit # r, reword = use commit, but edit the commit message # 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. #

This does not make it clear that the format of each line is
	<instruction> <commit id> <explanatory text that will be printed>

but the reader will probably infer that from the automatically generated pick examples above.

What about the "exec" instruction? By analogy, I might imagine that the format of that line is

	exec <command> <explanatory text that will be printed>

So the "<cmd>" does not address that question for me. It does succeed in clarifying that "a shell command" does not mean an arbitrary shell command but a user-specified one.

Meanwhile, it makes the cheat sheet harder to visually scan as a table
 i, instruction = action performed by instruction

Maybe "exec" should be explained outside this table? For example, maybe something along the lines of

	 x, exec = run an arbitrary command (see below)
	A line of the form "exec <command>" will run <command> using your
	shell and stop for investigation or amending if the command fails.
	If you remove a line here, THAT COMMIT WILL BE LOST.
	However, if you remove everything, the rebase will be aborted.
Junio C Hamano· Jan 20, 2011, 20:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

Jonathan Nieder <jrnieder@gmail.com> writes:
> Maybe "exec" should be explained outside this table?  For example,
> maybe something along the lines of
>
> 	 x, exec = run an arbitrary command (see below)

Ok, none of the other insns in the insn sheet mention what the argument to the command means anyway (e.g. "p, pick = replay the commit" doesn't say explicitly where the commit comes from), so I think the original patch is probably fine.

If we wanted to be more helpful, perhaps s/(see below)/specified on the rest of the line/ should be sufficient without adding extra lines.

Jonathan Nieder· Jan 21, 2011, 00:36 UTC · re: Junio C Hamano · lore

[PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

The hints in the current "instruction sheet" template look like so:
 # Rebase 3f14246..a1d7e01 onto 3f14246
 #
 # Commands:
 #  p, pick = use commit
 #  r, reword = use commit, but edit the commit message
 #  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.
 #
This does not make it clear that the format of each line is
	<insn> <commit id> <explanatory text that will be printed>

but the reader will probably infer that from the automatically generated pick examples above it.

What about the "exec" instruction? By analogy, I might imagine that the format of that line is "exec <command> <explanatory text>", and the "x <cmd>" hint does not address that question (at first I read it as taking an argument <cmd> that is the name of a shell). Meanwhile, the mention of <cmd> makes the hints harder to scan as a table.

So remove the <cmd> and add some words to remind the reader that "exec" runs a command named by the rest of the line. To make room, it is left to the manpage to explain that that command is run using $SHELL and that nonzero status from that command will pause the rebase.

Wording from Junio.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> If we wanted to be more helpful, perhaps s/(see below)/specified on the
> rest of the line/ should be sufficient without adding extra lines.
Sounds good.
 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a5ffd9a..a18c9b1 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1021,7 +1021,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
+#  x, exec = run command specified on the rest of the line
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
-- 
1.7.4.rc2
Matthieu Moy· Jan 21, 2011, 06:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

Jonathan Nieder <jrnieder@gmail.com> writes:
> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
> +#  x, exec = run command specified on the rest of the line

I don't think dropping "shell" is a good idea. In this context, "command" could mean "one of pick/fixup/squash/...", a Git command, and at last, an arbitrary line of shell.

I agree that my "shell command" wording was confusing too, but maybe just adding "using the shell" at the end of line would do it. Otherwise, I prefered the "see below + 2 lines explanation" proposal above.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jonathan Nieder· Jan 21, 2011, 07:47 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>> +#  x, exec = run command specified on the rest of the line
>
> I don't think dropping "shell" is a good idea. In this context,
> "command" could mean "one of pick/fixup/squash/...", a Git command,
> and at last, an arbitrary line of shell.
Hmm.  I suppose

# Commands: # p, pick = use commit # r, reword = use commit, but edit the commit message # 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, exec = run command (the rest of the line) using shell

would do?
Sorry to take so long to get this right.
Matthieu Moy· Jan 21, 2011, 10:43 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
> # Commands:
> #  p, pick = use commit
> #  r, reword = use commit, but edit the commit message
> #  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, exec = run command (the rest of the line) using shell
I'm fine with that.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jonathan Nieder· Jan 16, 2011, 02:02 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] rebase -i: explain how to discard all commits

Preparing a patch series for submission (as explained under INTERACTIVE MODE in the git rebase manual) sometimes involves discarding commits representing changes that turned out to be a bad idea. Usually this is quite simple to do by deleting the appropriate "pick" lines, but if all commits are removed then the "remove everything means abort" logic kicks in and the rebase is cancelled. One can override that behavior by adding a line with the text "noop".

This is a follow-up to v1.6.0.3~21 (rebase -i: do not fail when there is no commit to cherry-pick, 2008-10-10).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-rebase--interactive.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 09aeecf..d9dfc75 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1025,6 +1025,7 @@ first and then run 'git rebase --continue' again."
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
+# Use the "noop" command if you really want to remove all commits.
 #
 EOF
 
-- 
1.7.4.rc2
Nicolas Sebrecht· Jan 20, 2011, 19:39 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] Re: rebase -i: explain how to discard all commits

The 15/01/11, Jonathan Nieder wrote:
Show 27 quoted lines
> Preparing a patch series for submission (as explained under
> INTERACTIVE MODE in the git rebase manual) sometimes involves
> discarding commits representing changes that turned out to be a bad
> idea.  Usually this is quite simple to do by deleting the appropriate
> "pick" lines, but if all commits are removed then the "remove
> everything means abort" logic kicks in and the rebase is cancelled.
> One can override that behavior by adding a line with the text "noop".
> 
> This is a follow-up to v1.6.0.3~21 (rebase -i: do not fail when there
> is no commit to cherry-pick, 2008-10-10).
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  git-rebase--interactive.sh |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 09aeecf..d9dfc75 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1025,6 +1025,7 @@ first and then run 'git rebase --continue' again."
>  #
>  # If you remove a line here THAT COMMIT WILL BE LOST.
>  # However, if you remove everything, the rebase will be aborted.
> +# Use the "noop" command if you really want to remove all commits.
>  #
>  EOF

Sorry, I think it is confusing. With this help we could understand that the "noop" will either

  (a) discard the interactive rebase
or
  (b) _really remove commits_ from that branch

I'm not sure to know how it will act myself. If (a), we could use something like

  "However, if you remove everything or use the "noop" command, the rebase will be aborted."

but if we are in case (b), I guess it is not necessary and we should point to the 'git reset' command.

-- 
Nicolas Sebrecht
Jonathan Nieder· Jan 20, 2011, 19:57 UTC · re: Nicolas Sebrecht · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

Nicolas Sebrecht wrote:
> The 15/01/11, Jonathan Nieder wrote:
>> This is a follow-up to v1.6.0.3~21 (rebase -i: do not fail when there
>> is no commit to cherry-pick, 2008-10-10).
[...]
>>  # However, if you remove everything, the rebase will be aborted.
>> +# Use the "noop" command if you really want to remove all commits.
[...]
Show 16 quoted lines
> Sorry, I think it is confusing. With this help we could understand that
> the "noop" will either
>
>   (a) discard the interactive rebase
>
> or
>
>   (b) _really remove commits_ from that branch
>
> I'm not sure to know how it will act myself. If (a), we could use
> something like
>
>   "However, if you remove everything or use the "noop" command, the rebase will be aborted."
>
> but if we are in case (b), I guess it is not necessary and we should
> point to the 'git reset' command.

Okay. I agree that my particular wording was confusing. Are you saying the "noop" command in general is confusing?

The "noop" is itself a non-operation; if you combine "noop" with other instructions then the noop itself will have no effect. Meanwhile if you have _no_ instructions then the rebase is cancelled, while if you have a single "noop" instruction, that means "I have discarded all the commits, but please rebase anyway".

Jonathan
Nicolas Sebrecht· Jan 20, 2011, 20:08 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] Re: rebase -i: explain how to discard all commits

The 20/01/11, Jonathan Nieder wrote:
Show 8 quoted lines
> Okay.  I agree that my particular wording was confusing.  Are you
> saying the "noop" command in general is confusing?
> 
> The "noop" is itself a non-operation; if you combine "noop" with other
> instructions then the noop itself will have no effect.  Meanwhile if
> you have _no_ instructions then the rebase is cancelled, while if you
> have a single "noop" instruction, that means "I have discarded all the
> commits, but please rebase anyway".
Ok, I think I get it now. What about adding
  Use "noop" with no other instruction to fallback to a non-interactive
  rebase. If other instructions are present, "noop" has no effect.
?
-- 
Nicolas Sebrecht
Thomas Rast· Jan 20, 2011, 20:34 UTC · re: Nicolas Sebrecht · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

Nicolas Sebrecht wrote:
Show 12 quoted lines
> The 20/01/11, Jonathan Nieder wrote:
> 
> > if you
> > have a single "noop" instruction, that means "I have discarded all the
> > commits, but please rebase anyway".
> 
> Ok, I think I get it now. What about adding
> 
>   Use "noop" with no other instruction to fallback to a non-interactive
>   rebase. If other instructions are present, "noop" has no effect.
> 
> ?
No, that's quite wrong.

The TODO list is the list of all commits that need to be rebased. It does not contain commits that (according to patch-id) are already contained in the upstream (i.e., the base you are rebasing on). If the list is empty after filtering out such commits, rebase puts 'noop' as the only command since "empty TODO" is already taken to mean "abort"

If you then accept this 'noop' rebase, this effectively makes the rebased branch the same as the base branch, sort of like resetting.

(I for one have never accepted such a rebase; if the TODO only consists of noop, that means I made a mistake.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Jan 20, 2011, 21:28 UTC · re: Thomas Rast · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

Thomas Rast <trast@student.ethz.ch> writes:
> (I for one have never accepted such a rebase; if the TODO only
> consists of noop, that means I made a mistake.)

Wouldn't that suggest us that if we were to do anything to this message it would be a good idea to teach the user to "reset --hard" the branch if no commits truly needs to be replayed on top of the onto-commit?

Johannes Schindelin· Jan 21, 2011, 07:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

Hi,
On Thu, 20 Jan 2011, Junio C Hamano wrote:
Show 8 quoted lines
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > (I for one have never accepted such a rebase; if the TODO only 
> > consists of noop, that means I made a mistake.)
> 
> Wouldn't that suggest us that if we were to do anything to this message 
> it would be a good idea to teach the user to "reset --hard" the branch 
> if no commits truly needs to be replayed on top of the onto-commit?

The important difference between rebase -i && noop on the one, and reset --hard on the other hand is that the latter is completely unsafe. I mean, utterly completely super-unsafe. And I say that because _this here developer_ who is not exactly a Git noob lost stuff that way.

rebase -i checks that all is well and we could come back to the current status later if we realized that things went horribly wrong.

reset --hard does not do that. No safety net. No reflog. Nada.

Hth, Dscho

Jonathan Nieder· Jan 21, 2011, 07:37 UTC · re: Johannes Schindelin · lore

[PATCH] Documentation: suggest "reset --keep" to undo a commit

When one's only goal is to move from one commit to another, reset --keep is simply better than reset --hard, since it preserves local changes in the index and worktree when easy and errors out without doing anything when not. Update the two "how to remove commits" examples in this vein. "reset --hard" is still explained in a later example about cleaning up during a merge.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Johannes Schindelin wrote:
> rebase -i checks that all is well and we could come back to the current 
> status later if we realized that things went horribly wrong.
>
> reset --hard does not do that. No safety net. No reflog. Nada.

Right. I think we should encourage people to use "reset --keep" more often. (In general. The particular "rebase to pull" example just mentioned is less obvious.)

 Documentation/git-reset.txt |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index fd72976..1f13a1e 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -148,7 +148,7 @@ Undo a commit, making it a topic branch::
 +
 ------------
 $ git branch topic/wip     <1>
-$ git reset --hard HEAD~3  <2>
+$ git reset --keep HEAD~3  <2>
 $ git checkout topic/wip   <3>
 ------------
 +
@@ -163,7 +163,7 @@ Undo commits permanently::
 +
 ------------
 $ git commit ...
-$ git reset --hard HEAD~3   <1>
+$ git reset --keep HEAD~3   <1>
 ------------
 +
 <1> The last three commits (HEAD, HEAD^, and HEAD~2) were bad
-- 
1.7.4.rc2
Junio C Hamano· Jan 21, 2011, 17:34 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Documentation: suggest "reset --keep" to undo a commit

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
> When one's only goal is to move from one commit to another, reset
> --keep is simply better than reset --hard, since it preserves local
> changes in the index and worktree when easy and errors out without
> doing anything when not.  Update the two "how to remove commits"
> examples in this vein.  "reset --hard" is still explained in a later
> example about cleaning up during a merge.

I agree with the first sentence but I do not think its conclusion would lead to changes to "how to _remove_ commits". The examples were written in contexts (explanatory text <$n>) where hard makes sense, and the context needs tweaking to make keep makes more sense than hard does.

For example, the first one's original sequence is this:
    $ git branch topic/wip
    $ git reset --hard HEAD~3
    $ git checkout topic/wip

The text explains the motivation behind these series of commands in <1> but it has one untold assumption behind it; the user did the review to reach the conclusion that the recent changes are premature after fully committing (i.e. the working tree is clean). That is why "hard" worked just fine.

But the user could do the reviewing and thinking with some local changes still in the working tree (they are incredients for the fourth commit yet to be made) and decide to branch at that point. The description in <1> needs to be updated to hint that there can be uncommitted changes, e.g.

	You have worked for some time, made a few commits, and may have
	uncommitted changes.  After reviewing the current state, you
	realized that ...

Using --keep may help the user do so, but only if the local changes do not conflict with the changes in the recent commits to be discarded, right?

    Side note: Regardless of any of the above, the section header needs to
    be updated---it is not "Undo *A* commit", we are excluding three from
    the current branch.
By the way, a more natural way to do this would actually be:
    $ git checkout -b topic/wip
    $ git branch -f @{-1} HEAD~3
or using the stash:
    $ git stash ;# save local changes
    $ git branch topic/wip ;# and mark the tip before rewinding
    $ git reset --hard HEAD~3 ;# you could say --keep here too
    $ git checkout topic/wip ;# and then continue
    $ git stash pop ;# with the local changes

The branch/reset/checkout sequence itself, either with hard or keep, looks more like a contrived example to show "reset" than the best way to solve the problem in the scenario presented there. Probably we would want to drop this one altogether, or keep the scenario and explain the best solution in somewhere else (e.g. tutorial).

Show 9 quoted lines
> @@ -163,7 +163,7 @@ Undo commits permanently::
>  +
>  ------------
>  $ git commit ...
> -$ git reset --hard HEAD~3   <1>
> +$ git reset --keep HEAD~3   <1>
>  ------------
>  +
>  <1> The last three commits (HEAD, HEAD^, and HEAD~2) were bad

Please tell a story where keep makes more sense than hard by enhancing the explanatory text <1> associated with this section. The current text says that the three topmost commit representing what you have recently worked so far are all unwanted, strongly hinting that hard is more appropriate thing to do than keep, which is not what we want if we are changing the example to use keep.

It would be sufficient to just hint that the uncommitted changes that you have in your working tree are unrelated to what these three commits wanted to do (e.g. you always keep small changes around, such as debugging printf's and a change to the version string in Makefile---you do not intend to commit them and they are unrelated to the commits you are discarding), and you do want to keep them around if you can.

Jonathan Nieder· Jan 21, 2011, 19:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] Documentation: suggest "reset --keep" to undo a commit

Junio C Hamano wrote:
Show 11 quoted lines
> But the user could do the reviewing and thinking with some local changes
> still in the working tree (they are incredients for the fourth commit yet
> to be made) and decide to branch at that point.  The description in <1>
> needs to be updated to hint that there can be uncommitted changes, e.g.
>
> 	You have worked for some time, made a few commits, and may have
> 	uncommitted changes.  After reviewing the current state, you
> 	realized that ...
>
> Using --keep may help the user do so, but only if the local changes do not
> conflict with the changes in the recent commits to be discarded, right?
I think this explanation misses out on something.

I may be abusing git in a certain way, but I find myself in the following situation fairly often:

	... hack hack hack ...
	git add -p;	# hmm, looks like multiple features.
	git stash -k
	... test ...
	git commit;	# commit feature #1
	git stash pop
	git add -p
	git stash -k
	... test ...
	git commit; # commit feature #2
	git stash pop
	# hmm, feature #2 is not suitable for this branch.
	git branch wip/feature-2
	git reset --keep HEAD^;	# <*>
	git add -p
	git stash -k
	... test ...
	git commit; # commit feature #3

On line <*>, I am just not thinking about the uncommitted changes. They may be there or they may not. If they are in the way of what I am trying to do, "git reset --keep" will politely inform me so I can act accordingly (usually stash, commit, or discard them).

> By the way, a more natural way to do this would actually be:
>
>     $ git checkout -b topic/wip
>     $ git branch -f @{-1} HEAD~3
True.  (I think the intended scenario was
	git branch topic/wip; # save the tip for later
	git reset --keep HEAD~3
	# now what was I working on?
	... hack hack hack ...
	# okay, now we have time for that diversion.
	git checkout topic/wip
but it would be nice to contrast it with the one you described.)
Show 7 quoted lines
> or using the stash:
>
>     $ git stash ;# save local changes
>     $ git branch topic/wip ;# and mark the tip before rewinding
>     $ git reset --hard HEAD~3 ;# you could say --keep here too
>     $ git checkout topic/wip ;# and then continue
>     $ git stash pop ;# with the local changes

This approach leaves more files touched and more targets to be rebuilt by "make".

Show 6 quoted lines
> Please tell a story where keep makes more sense than hard by enhancing the
> explanatory text <1> associated with this section.  The current text says
> that the three topmost commit representing what you have recently worked
> so far are all unwanted, strongly hinting that hard is more appropriate
> thing to do than keep, which is not what we want if we are changing the
> example to use keep.

Maybe the best story would be "you have just explored a blind alley and decided the last three commits are not a good idea at all", with reference to a new section explaining that

 * --soft is for when the commit in preparation has the right content
   but should be on top of a different parent (e.g., squashing commits)
 * --keep is for transporting your local changes to a different commit
   (e.g., rewinding a branch or transplanting changes)
 - --merge is a limited and low-level tool for recovering from a
   conflicted merge and most often will take ORIG_HEAD as its argument.
   Maybe in the future merges will save more information so reset --merge
   can error out more often.
 - --hard is for resetting to a known state
 - --mixed is for resetting to a known state but leaving the worktree
   alone
> It would be sufficient to just hint that the uncommitted changes that you
> have in your working tree are unrelated to what these three commits wanted
> to do (e.g. you always keep small changes around, such as debugging
> printf's

That use case is less interesting to me --- it is relatively harmless to clobber such content.

Junio C Hamano· Jan 21, 2011, 20:28 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Documentation: suggest "reset --keep" to undo a commit

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
>> Please tell a story where keep makes more sense than hard by enhancing the
>> explanatory text <1> associated with this section.  The current text says
>> that the three topmost commit representing what you have recently worked
>> so far are all unwanted, strongly hinting that hard is more appropriate
>> thing to do than keep, which is not what we want if we are changing the
>> example to use keep.
>
> Maybe the best story would be "you have just explored a blind alley
> and decided the last three commits are not a good idea at all", with

That unfortunately does not seem to describe the nature of the local changes at all, which I think is the whole point of this topic to encourage use of --keep over --hard.

Show 7 quoted lines
>> It would be sufficient to just hint that the uncommitted changes that you
>> have in your working tree are unrelated to what these three commits wanted
>> to do (e.g. you always keep small changes around, such as debugging
>> printf's
>
> That use case is less interesting to me --- it is relatively harmless
> to clobber such content.

Actually I think that is the primary use case of the feature, as --keep was done as a parallel to the behaviour of checkout that checks out a different branch while keeping local changes.

Junio C Hamano· Jan 21, 2011, 16:51 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
>> Wouldn't that suggest us that if we were to do anything to this message 
>> it would be a good idea to teach the user to "reset --hard" the branch 
>> if no commits truly needs to be replayed on top of the onto-commit?
>
> The important difference between rebase -i && noop on the one, and reset 
> --hard on the other hand is that the latter is completely unsafe. I mean, 
> utterly completely super-unsafe. And I say that because _this here 
> developer_ who is not exactly a Git noob lost stuff that way.

I think "rebase" already checks that the index and the working tree is clean before starting, so referring to "reset --hard" when "rebase -i" notices there is absolutely nothing to do is _not_ unsafe, no?

Matthieu Moy· Jan 21, 2011, 17:05 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> Wouldn't that suggest us that if we were to do anything to this message 
>>> it would be a good idea to teach the user to "reset --hard" the branch 
>>> if no commits truly needs to be replayed on top of the onto-commit?
>>
>> The important difference between rebase -i && noop on the one, and reset 
>> --hard on the other hand is that the latter is completely unsafe. I mean, 
>> utterly completely super-unsafe. And I say that because _this here 
>> developer_ who is not exactly a Git noob lost stuff that way.
>
> I think "rebase" already checks that the index and the working tree is
> clean before starting, so referring to "reset --hard" when "rebase -i"
> notices there is absolutely nothing to do is _not_ unsafe, no?

The point is not about letting rebase do a "reset --hard", but to tell the user s/he should have ran "reset --hard" instead of rebase. The danger is to teach the user's fingers to type "reset --hard" too often, which is unsafe ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Joshua Jensen· Jan 21, 2011, 17:57 UTC · re: Matthieu Moy · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

----- Original Message -----
From: Matthieu Moy
Date: 1/21/2011 10:05 AM
Show 18 quoted lines
> Junio C Hamano<gitster@pobox.com>  writes:
>
>> Johannes Schindelin<Johannes.Schindelin@gmx.de>  writes:
>>
>>>> Wouldn't that suggest us that if we were to do anything to this message
>>>> it would be a good idea to teach the user to "reset --hard" the branch
>>>> if no commits truly needs to be replayed on top of the onto-commit?
>>> The important difference between rebase -i&&  noop on the one, and reset
>>> --hard on the other hand is that the latter is completely unsafe. I mean,
>>> utterly completely super-unsafe. And I say that because _this here
>>> developer_ who is not exactly a Git noob lost stuff that way.
>> I think "rebase" already checks that the index and the working tree is
>> clean before starting, so referring to "reset --hard" when "rebase -i"
>> notices there is absolutely nothing to do is _not_ unsafe, no?
> The point is not about letting rebase do a "reset --hard", but to tell
> the user s/he should have ran "reset --hard" instead of rebase. The
> danger is to teach the user's fingers to type "reset --hard" too
> often, which is unsafe ;-).

I've always wished "git reset --hard" would tell me there are modified files and force me to type "git reset --hard --force" to overwrite them. It is a dangerous command, and I stupidly run it sometimes without running "git status" first.

-Josh
Jonathan Nieder· Jan 21, 2011, 18:37 UTC · re: Joshua Jensen · lore

[PATCH] Documentation: do not treat reset --keep as a special case

The current treatment of "git reset --keep" emphasizes how it differs from --hard (treatment of local changes) and how it breaks down into plumbing (git read-tree -m -u HEAD <commit> followed by git update-ref HEAD <commit>). This can discourage people from using it, since it might seem to be a complex or niche option.

Better to emphasize what the --keep flag is intended for --- moving the index and worktree from one commit to another, like "git checkout" would --- so the reader can make a more informed decision about the appropriate situations in which to use it.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Joshua Jensen wrote:
> I've always wished "git reset --hard" would tell me there are
> modified files and force me to type "git reset --hard --force" to
> overwrite them.  It is a dangerous command, and I stupidly run it
> sometimes without running "git status" first.

Have you tried "git reset --keep"? How does it compare to your wish?

 Documentation/git-reset.txt |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index fd72976..927ecee 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -76,15 +76,10 @@ In other words, --merge does something like a 'git read-tree -u -m <commit>',
 but carries forward unmerged index entries.
 
 --keep::
-	Resets the index, updates files in the working tree that are
-	different between <commit> and HEAD, but keeps those
-	which are different between HEAD and the working tree (i.e.
-	which have local changes).
+	Resets index entries and updates files in the working tree that are
+	different between <commit> and HEAD.
 	If a file that is different between <commit> and HEAD has local changes,
 	reset is aborted.
-+
-In other words, --keep does a 2-way merge between <commit> and HEAD followed by
-'git reset --mixed <commit>'.
 --
 
 If you want to undo a commit other than the latest on a branch,
-- 
1.7.4.rc2
Junio C Hamano· Jan 21, 2011, 20:35 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Documentation: do not treat reset --keep as a special case

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
> The current treatment of "git reset --keep" emphasizes how it
> differs from --hard (treatment of local changes) and how it breaks
> down into plumbing (git read-tree -m -u HEAD <commit> followed by git
> update-ref HEAD <commit>).  This can discourage people from using
> it, since it might seem to be a complex or niche option.
>
> Better to emphasize what the --keep flag is intended for --- moving
> the index and worktree from one commit to another, like "git checkout"
> would --- so the reader can make a more informed decision about the
> appropriate situations in which to use it.

The updated text makes quite a lot of sense ;-) while the old text doesn't. What were we smoking when we wrote it and passed it through the review?

Show 20 quoted lines
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>  Documentation/git-reset.txt |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index fd72976..927ecee 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -76,15 +76,10 @@ In other words, --merge does something like a 'git read-tree -u -m <commit>',
>  but carries forward unmerged index entries.
>  
>  --keep::
> -	Resets the index, updates files in the working tree that are
> -	different between <commit> and HEAD, but keeps those
> -	which are different between HEAD and the working tree (i.e.
> -	which have local changes).
> +	Resets index entries and updates files in the working tree that are
> +	different between <commit> and HEAD.
>  	If a file that is different between <commit> and HEAD has local changes,
>  	reset is aborted.

I saw "updates files" and one question immediately came to mind: update how? "... to match what is in HEAD"? "Resets index entries" is less of a problem as the word "reset" already strongly suggests that the current state does not matter as much as the target state, though.

Thanks.
Jay Soffian· Jan 26, 2011, 07:33 UTC · re: Joshua Jensen · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

On Fri, Jan 21, 2011 at 12:57 PM, Joshua Jensen <jjensen@workspacewhiz.com> wrote:

> I've always wished "git reset --hard" would tell me there are modified files
> and force me to type "git reset --hard --force" to overwrite them.  It is a
> dangerous command, and I stupidly run it sometimes without running "git
> status" first.
Something analogous to clean.requireForce?
j.
Johannes Schindelin· Jan 23, 2011, 20:10 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

Hi,
On Fri, 21 Jan 2011, Junio C Hamano wrote:
Show 16 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Wouldn't that suggest us that if we were to do anything to this 
> >> message it would be a good idea to teach the user to "reset --hard" 
> >> the branch if no commits truly needs to be replayed on top of the 
> >> onto-commit?
> >
> > The important difference between rebase -i && noop on the one, and 
> > reset --hard on the other hand is that the latter is completely 
> > unsafe. I mean, utterly completely super-unsafe. And I say that 
> > because _this here developer_ who is not exactly a Git noob lost stuff 
> > that way.
> 
> I think "rebase" already checks that the index and the working tree is 
> clean before starting, so referring to "reset --hard" when "rebase -i" 
> notices there is absolutely nothing to do is _not_ unsafe, no?

Oh, so you want to suggest using "reset --hard" but warn at the same time that this command on its own is dangerous unless you run rebase first? :-)

Ciao, Johannes

Matthieu Moy· Aug 10, 2010, 15:17 UTC · re: Matthieu Moy · lore

[PATCH 2/2] test-lib: user-friendly alternatives to test [-d|-f|-e]

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>
---
Just resending this one since I modified PATCH 1/2 and had to resolve
a minor conflict.
 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 93b181e..fa02eb3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -79,18 +79,18 @@ test_expect_success 'rebase -i with the exec command' '
 	export FAKE_LINES &&
 	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 " (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)"
@@ -105,7 +105,7 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	FAKE_LINES="1 exec_>touch-subdir" \
 		git rebase -i HEAD^ &&
 	cd .. &&
-	test -f touch-subdir &&
+	test_path_is_file touch-subdir &&
 	rm -fr subdir
 '
 
@@ -204,7 +204,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' '
@@ -213,7 +213,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

← back to recent threads